specs-go/config: add Landlock LSM support #1111

kailun-qin · 2021-08-02T07:44:42Z

Linux kernel 5.13 adds support for Landlock Linux Security Module (LSM).
This allows unprivileged processes to create safe security sandboxes
that can securely restrict the ambient rights (e.g. global filesystem
access) for themselves.

#1110

Signed-off-by: Kailun Qin kailun.qin@intel.com

cyphar · 2021-08-02T08:37:38Z

config.md

+* **`landlock`** (object, OPTIONAL) specifies the Landlock unprivileged access control settings for the container process.
+    For more information about Landlock, see [Landlock documentation][landlock].
+    `landlock` contains the following properties:
+
+    * **`ruleset`** (object, OPTIONAL) the `ruleset` field identifies a set of rules (i.e., actions on objects) that need to be handled (i.e., restricted).
+    * **`rules`** (array of objects, OPTIONAL) the `rules` field specifies the security policies (i.e., actions allowed on objects) to be added to an existing ruleset
+    * **`abi`** (object, OPTIONAL) the `abi` field defines the specific Landlock ABI version.


So I'm of two minds about how much of the Landlock ruleset language we want to embed in the runtime-spec. I haven't looked too deeply into the version of Landlock that was merged -- is there any serialisation format they describe? Or is it like libseccomp where it's a bunch of function calls and we need to create our own way of describing it?

I don't really have a conclusion with my below points, I'm more wondering what other maintainers think about it.

Pros

Given that adding bits to runtime-spec configuration is a somewhat supported thing by some runtimes, making things configurable through the runtime-spec directly without needing any host setup (such as is the case with AppArmor and SELinux configuration) means that higher-level users can support this feature transparently by just passing whatever ruleset the user specified. This is one of the (on paper) benefits of our seccomp configuration, though in practice Docker has its own custom ruleset and crun is toying with an even more custom system. Though seccomp is a lot more granular than landlock so maybe this problem won't come up in practice.

Cons

We embed some fairly hairy bits of the kernel ABI into our format, meaning that if a new kernel feature is added, runtimes won't gain support for it until after the spec is updated and all of the other runtimes update. If there is a liblandlock with their own serialisation format, that would be preferable. This is especially true when it comes to the LANDLOCK_* constants being defined -- in seccomp these are a bit of a pain point because we need to manually translate them several times, meaning that we end up with multiple definitions of the constant mapping (none of which are in the spec).

The fact there's an ABI version field tells me that they might want to have new ABIs in the future. This is going to cause a problem with Go in particular, because a new ABI format that changes the structure of the landlock config object is going to make supporting multiple ABI versions quite difficult (in Rust you could do it with unions but we don't have that option in Go and we'd probably have to use some kind of awful reflection or creating a partial struct to just extract the version number).

is there any serialisation format they describe? Or is it like libseccomp where it's a bunch of function calls and we need to create our own way of describing it?

There is no "language" serialization done by the kernel. It is similar to openat* syscalls: a path rule is defined by a file descriptor to identify the path/hierarchy to be restricted with a set of rights as a bitmask.

Landlock is designed to handle layers of policies. This is similar to seccomp, but with less overhead, and focusing on semantic, which should avoid the seccomp issues. It then makes sense for runtime-spec to support a Landlock configuration. Other tools, either outside the container or inside, could still use Landlock the way it suits them.

We embed some fairly hairy bits of the kernel ABI into our format, meaning that if a new kernel feature is added, runtimes won't gain support for it until after the spec is updated and all of the other runtimes update.

This is correct but like I said above, nothing stops another component to use a newer or older set of Landlock features, independently of runtime-spec. I can help you keep an up-to-date spec as soon as a new features are in mainline.

If there is a liblandlock with their own serialisation format, that would be preferable. This is especially true when it comes to the LANDLOCK_* constants being defined -- in seccomp these are a bit of a pain point because we need to manually translate them several times, meaning that we end up with multiple definitions of the constant mapping (none of which are in the spec).

About the constants, you can use golandlock developped by @gnoack (package renaming is ongoing though).

The fact there's an ABI version field tells me that they might want to have new ABIs in the future. This is going to cause a problem with Go in particular, because a new ABI format that changes the structure of the landlock config object is going to make supporting multiple ABI versions quite difficult (in Rust you could do it with unions but we don't have that option in Go and we'd probably have to use some kind of awful reflection or creating a partial struct to just extract the version number).

Landlock will gain new features over time, but the old ones will (of course) still be supported (i.e. no compatibility breakage). The ABI version is there to probe the kernel and know if it supports a set of features. This version ABI should not be feared, nothing will break when a new one will be supported by the kernel. For more details, there is an issue for golandlock about that: landlock-lsm/go-landlock#4

kailun-qin · 2021-08-02T08:43:10Z

specs-go/config.go

+// LandlockRule represents the security policies (i.e., actions allowed on objects) .
+type LandlockRule struct {
+	// Type is the Landlock rule type pointing to the rules to be added to an existing ruleset.
+	Type LandlockRuleType `json:"type,omitempty" platform:"linux"`


Do we really need multiple LandlockRules of the same type?

For example, we can do instead:

"rules": { "path_beneath": [ { "allowedAccess": ["xxx"], "paths": ["xxx"] }, { "allowedAccess": ["xxx"], "paths": ["xxx"] } ] }

I agree that it would make the configuration lighter.

Thanks for the feedbacks! Use an object with rule-type keys instead to make it lighter.

h-vetinari · 2021-08-02T08:45:20Z

So I'm of two minds about how much of the Landlock ruleset language we want to embed in the runtime-spec. I haven't looked too deeply into the version of Landlock that was merged -- is there any serialisation format they describe? Or is it like libseccomp where it's a bunch of function calls and we need to create our own way of describing it?

[...]

CC @l0kod

BoardzMaster · 2021-08-04T15:30:45Z

Having this format of Landlock rules I think is enough (like @kailun-qin suggested)

Go LibLandlock is already implemented by Günther Noack https://github.com/gnoack/golandlock. It can be used in runc.
Another issue is the future support for containerd/k8s. Here there must be serialization format like @cyphar addressed in his comment ( grpc mechanisms in particular)

Mickaël Salaün, please give your opinion CC @l0kod

AkihiroSuda · 2021-08-18T05:16:38Z

@kailun-qin
Do you have a POC implementation of runc/crun to verify this design?

l0kod · 2021-08-22T14:15:39Z

Sorry for the delay, I missed the GitHub notification. Thanks for working on this, I'm happy to help! However, I'm not familiar with runtime-spec, my suggestions may not be accurate.

To be sure everyone is on the same page, I want to say that Landlock may not currently be used in a generic container because of the reparenting limitation. I plan to address this in the following months.
However, I think it is a good idea to start the integration now and that the current status can still be useful to users.

The goal of Landlock is to give (for this use case) the ability to container managers to handle access rights independently to the parent system configuration (i.e. it doesn't require admin rights). It seems that options such as maskedPaths and readonlyPaths have a similar purpose (but they use the namespace features underneath). However, because runtime-spec is close to the underlying kernel/OS, it seems that it makes sense to have a specific "landlock" object for Linux.

FYI, to be able to use Landlock, the noNewPrivileges option must be enabled. It may be a good idea to hint the user and error out if Landlock is configured without this option to avoid false sense of security.

Because a kernel may not support Landlock (because the kernel is too old, or Landlock is not configured, or not enabled at boot time), I would advise to follow a best-effort security approach, which is to just drop a warning and continue interpreting the configuration by default. However, it may be wise to let users choose to enforce Landlock restrictions for specific use cases (e.g. when we create a configuration for our own systems that we know should support Landlock). I don't know how this is currently handled for seccomp though, but I guess the behavior should be the same.

How do you plan to use the "abi" field?

If you want to follow Landlock development, you can subscribe to the appropriate mailing list: https://subspace.kernel.org/lists.linux.dev.html. I'll send news soon.

l0kod · 2021-08-22T14:19:36Z

config.md

@@ -253,6 +260,65 @@ _Note: symbolic name for uid and gid, such as uname and gname respectively, are
    ],
    "apparmorProfile": "acme_secure_profile",
    "selinuxLabel": "system_u:system_r:svirt_lxc_net_t:s0:c124,c675",
+    "landlock": {
+        "ruleset": {
+            "handledAcessFS": [


handledAccessFS (a "c" is missing)

Nice catch. Fixed, thanks!

l0kod · 2021-08-22T14:25:21Z

config.md

+
+    * **`ruleset`** (object, OPTIONAL) the `ruleset` field identifies a set of rules (i.e., actions on objects) that need to be handled (i.e., restricted).
+    * **`rules`** (array of objects, OPTIONAL) the `rules` field specifies the security policies (i.e., actions allowed on objects) to be added to an existing ruleset
+    * **`abi`** (object, OPTIONAL) the `abi` field defines the specific Landlock ABI version.


About the "abi" field, it is not explained how this will be used. Is it a requirement to only enforce the Landlock rules with a kernel supporting this version? Is it a requirement for a minimal version?

The ABI version should be used by the container runtime to know if a set of rules is supported by the kernel, which should then help to only enforce those that are indeed supported and ignores unsupported features (i.e. best-effort security).

I'm not sure it makes sense to expose this in the configuration.

I personally agree w/ the best-effort security approach and I'm currently implementing this way (i.e., if not supported, just drop a warning and continue interpreting the configuration).
Added with more elaborations on the usage of this abi field. IMO, it's fine to expose this in the config. Feedback is welcome.

Are the abi and handledAccessFS settings trying to solve the same problem here? In the golandlock module, picking an ABI version is just a shortcut for asking for the maximum set of handledAccessFS possible under that ABI.

@gnoack Thanks! Great question.

Yes, from the current golandlock implementation, the abi maps to the max set of handledAccessFS possible under it. I.e., they may try to solve the same problem at this moment.

For the new abis in the future, should they normally point to sets of features to be supported by the kernel and those might go beyond handledAccessFS? I'm not sure. I'll leave the question here to @l0kod.

The ABI version is not only linked to handledAccessFS, the version may increase for it or for other features (independently). See my comment just bellow.

l0kod · 2021-08-22T14:26:56Z

config.md

+    "landlock": {
+        "ruleset": {
+            "handledAcessFS": [
+                "LANDLOCK_ACCESS_FS_EXECUTE",


These access rights could be truncated and simplified because they are scoped and typed by the "handledAccessFS" (e.g. "execute" instead of "LANDLOCK_ACCESS_FS_EXECUTE").

Make sense to me. Updated, thanks!

l0kod · 2021-08-22T14:29:24Z

specs-go/config.go

+// LandlockRule represents the security policies (i.e., actions allowed on objects) .
+type LandlockRule struct {
+	// Type is the Landlock rule type pointing to the rules to be added to an existing ruleset.
+	Type LandlockRuleType `json:"type,omitempty" platform:"linux"`


I agree that it would make the configuration lighter.

BoardzMaster · 2021-08-23T08:58:29Z

I suggest using a simplified version of a file/dir access rules like RW, RO. In this case, we could use RODirs()...RwFiles() restrict path options from the golandlock library https://github.com/gnoack/golandlock and a spec config.json file would be like this:

It will be more convenient for a user.

Mickaël Salaün, please give your opinion CC @l0kod

l0kod · 2021-08-23T20:26:25Z

I suggest using a simplified version of a file/dir access rules like RW, RO. In this case, we could use RODirs()...RwFiles()

RW and RO are misleading because there is not yet full support for all read and write operations on the filesystem. Moreover, runtime-spec seems to deal with low-level system config (e.g. mount options). I think that maintaining a high-level and evolving configuration will not fit with the purpose of runtime-spec.

However, it is still possible to create variables containing a pre-defined set of access rights.

gnoack · 2021-08-23T21:41:43Z

I suggest using a simplified version of a file/dir access rules like RW, RO. In this case, we could use RODirs()...RwFiles()

RW and RO are misleading because there is not yet full support for all read and write operations on the filesystem. Moreover, runtime-spec seems to deal with low-level system config (e.g. mount options). I think that maintaining a high-level and evolving configuration will not fit with the purpose of runtime-spec.

However, it is still possible to create variables containing a pre-defined set of access rights.

+1

The intent with ROFiles() and friends is to provide a way to coarsely permit operations and have the permissions "grow" along when new Landlock ABI versions become available. I'm hoping that in most cases it will only be needed to increment the used Landlock ABI version from time to time, and leaving the ROFiles() and similar rules untouched. But my compatibility assumptions are difficult to test, and @l0kod has a better hunch what the future might bring.

Please open a feature request on golandlock if you want more direct access for specifying ruleset.handledAccessFS. (Right now, it's implicitly set to the "best possible" value in the golandlock.V1 constant. The only reason this is not exposed so far was because I could not imagine a use case for it...)

kailun-qin · 2021-08-24T09:05:36Z

Thank you for all your inputs and comments! @l0kod @cyphar @BoardzMaster @h-vetinari @AkihiroSuda @gnoack

I've updated with a new version mainly w/ below changes based on the feedbacks:

Use an object with rule-type keys instead of an array of rules with specific types to make configuration lighter.
Simplify the access rights consts as they are scoped and typed by the LandlockFSAction.
Update some field comments and add elaborations on the usage of abi etc.
Misc minor fixes.

@AkihiroSuda Regarding POC implementation, we're working on it based on runc and golandlock. We'll submit PRs for review once it's ready.

Please kindly take another look. Let me know if any further suggestions. Thanks!

l0kod · 2021-08-25T21:44:38Z

config.md

+
+    * **`ruleset`** (object, OPTIONAL) the `ruleset` field identifies a set of rules (i.e., actions on objects) that need to be handled (i.e., restricted).
+    * **`rules`** (array of objects, OPTIONAL) the `rules` field specifies the security policies (i.e., actions allowed on objects) to be added to an existing ruleset
+    * **`abi`** (object, OPTIONAL) the `abi` field defines the specific Landlock ABI version.


The ABI version is not only linked to handledAccessFS, the version may increase for it or for other features (independently). See my comment just bellow.

l0kod · 2021-08-25T21:46:19Z

config.md

    * **`abi`** (object, OPTIONAL) the `abi` field defines the specific Landlock ABI version.
+    This should be used by the runtime to check if the kernel supports the specified sets of Landlock features and then enforce those following a best-effort security approach.


Interesting, I didn't designed the ABI version to be used this way. The idea is to know which features are supported before asking the kernel to build a ruleset with them (and not more), not the other way around. This helps make sure that no error should occur because of unknown features.

What you are doing here is to use this version to check that the container configuration was tested with a kernel supporting at least this ABI version, which are implicit default configurations (not defined in this spec anyway). I think it is safer to not set and use this version in a configuration but instead have an explicit configuration (i.e. explicit access rights). This would also avoid the case where different runtimes define their own default values according to a version which is not yet in the specification.

Later, we could add groups of access rights that could be used for handledAccessFS and allowedAccess, but the content of these groups should not change over time.

@l0kod Thanks for the comments! Make sense to me.

Removing the abi field from the config and will leverage the explicit access rights specified.

In this case, I think we need direct access to ruleset.handledAccessFS in go-landlock? @gnoack

Sounds good, I'll expose it. Tracking it in landlock-lsm/go-landlock#12

kailun-qin · 2021-08-26T04:27:51Z

config.md

    * **`abi`** (object, OPTIONAL) the `abi` field defines the specific Landlock ABI version.
+    This should be used by the runtime to check if the kernel supports the specified sets of Landlock features and then enforce those following a best-effort security approach.


@l0kod Thanks for the comments! Make sense to me.

Removing the abi field from the config and will leverage the explicit access rights specified.

In this case, I think we need direct access to ruleset.handledAccessFS in go-landlock? @gnoack

kailun-qin · 2021-08-26T04:29:28Z

specs-go/config.go

+	// This is for conditions when the Landlock access rights explicitly configured by the container are not
+	// supported or available in the running kernel.
+	// Default is false, i.e., following a best-effort security approach.
+	DisableBestEffort bool `json:"disableBestEffort,omitempty" platform:"linux"`


Also adding a best-effort control here, see if it makes sense to you @l0kod @gnoack. Thanks!

It looks good to me!

Thinking more about that, I think it would be more flexible to replace this global field with two per feature block: to be able to have a soft requirement (by default) or a hard requirement. This would enable for instance to require that the kernel supports a specific feature that would otherwise break the container (for instance, supporting file reparenting, which would probably be a new kind of option for the ruleset).
This would be complementary to a soft error (by default, best-effort: only log a warning) or a hard error (don't launch the container) if the feature is not supported. Being able to have no warning at all (not by default) could be useful too.

To sum up, that may look like this:

"ruleset": { "handledAccessFS": [ "execute", "write_file", "read_file", "read_dir", "remove_dir", "remove_file", "make_char", "make_dir", "make_reg", "make_sock", "make_fifo", "make_block", "make_sym" ], // optional field, default value (all handledAccessFS elements may not be known to the kernel; take into account as much as possible). // If "required" is true and the kernel doesn't support all the handledAccessFS rights, then ignores the whole "ruleset" block and the associated "rules". "required": false, // optional field, default value (if an handledAccessFS element is unknown to the kernel, then log a warning). Other values could be "silent" and "error". "unsupported": "warning" }, "rules": { "pathBeneath": [ { "allowedAccess": [ "execute", "read_file", "read_dir" ], "paths": [ "/usr", "/bin" ], // optional field, default value (all allowedAccess elements may not be known to the kernel; take into account as much as possible) // If "required" is true and the kernel doesn't support all the allowedAccess rights, then ignores this rule. "required": false, // optional field, default value (if an allowedAccess element is unknown to the kernel, then log a warning) "unsupported": "warning" },

Thanks @l0kod for the comments!

I do agree this adds more flexibility. Just wondering whether this adds more complexity as well? If supported, the flag check and intersect functions in go-landlock may better be exposed and the logic will be handled in runc. Beyond that, a RestrictPathsNoCheck API may need to be added as the checks in the current implementations are redundant in go-landlock.

I'm currently leveraging the best effort mode provided directly in go-landlock (which is a single control point). Any thoughts/comments? @gnoack

cc @kolyshkin @BoardzMaster

Regarding soft/hard requirements for what the kernel supports:

I've been playing with the thought of adding that to go-landlock, but was not convinced whether it would be actually needed. The way I considered implementing it would have been to have one AccessFSSet that is ideal and one AccessFSSet that is the minimum. So instead of .BestEffort(), you'd then call something like .BestEffortButAtLeast(AccessFSSet(...)). (or maybe I can find a nicer name for that...)

It would be reasonably easy to add to go-landlock, required changes would be: (a) to add a minHandledAccessFS field to the Config struct, (b) expose a suitable method to set it, and then (c) in restrictPaths(), just before the RulesetAttr{} is created, return an error if the minimal requirement is not fulfilled. I was just not convinced whether there is actually a need for it and it felt like speculative generality... do you think this is a use case that you'd need?

Regarding file reparenting:

If I understand this correctly, file reparenting is currently forbidden in a Landlock-restricted process in kernel 5.13, and with kernel 5.X you want to permit this in Landlock-restricted processes if they ask for it at enforcement time. Do I understand that correctly?

If that is the case, then it sounds like it should maybe not be part of the Config (RulesetAttr), but might logically fit more as an argument to RestrictPaths() (invocation of landlock_add_rule), like this:

err := landlock.V99.BestEffort().RestrictPaths( variousPaths..., landlock.EnableFileReparenting(params...), )

The thinking is:

the Config (landlock.V99, RulesetAttr) describes what should be forbidden (and with abi→∞, thats everything :))

the arguments to RestrictPaths() (landlock_add_rule) describe the exceptions to what is forbidden (the operations that the process intends to do)

So yes, the above invocation is not compatible with kernel 5.13. The go-landlock library's BestEffort() mode should restrict the strongest rules it can, but it might have to fall back to not enforcing anything at all, if file reparenting is something that the program needs, and if that is not compatible with the current kernel.

(I feel like I'm making a few assumptions here, there is a good chance that I'm misunderstanding this... :) let me know)

Regarding evolution flexibility of the config:

If there will be more fields added to RulesetAttr in the future, it might be easier to think of this as a "permits-more-or-the-same-as" relationship on RulesetAttr structs rather than as a "subset" relationship on AccessFSSets as it's implemented now in go-landlock. So maybe it would be better to have a "ruleset" and "min_ruleset" rather than having a "handled_access_fs" and "min_handled_access_fs" in the ruleset? WDYT?

Thinking more about that, I think it would be more flexible to replace this global field with two per feature block: to be able to have a soft requirement (by default) or a hard requirement. This would enable for instance to require that the kernel supports a specific feature that would otherwise break the container (for instance, supporting file reparenting, which would probably be a new kind of option for the ruleset).

If we really want to support a soft requirement (by default) or a hard requirement in ruleset to specify at least some features shall be supported, the format would be something similar to what @gnoack suggested - ruleset and min_ruleset (where one ruleset not be enough).

A simplified version of the proposition would be to only add required and unsupported (names may not be clear enough) to the ruleset block. I don't know about the evolution flexibility of this specification though.

This also makes me wonder about rules not being nested into ruleset. That would make more sense. It would be lighter to merge landlock and ruleset into landlockRuleset (the same way there is apparmorProfile and selinuxLabel). What do you think?

For rules not being nested into ruleset, they are currenlty being errored out (and should be considered as misconfigured maybe?). In this case, I think it's fine to only control in the ruleset block (simplified version).

Regarding soft/hard requirements for what the kernel supports:

I've been playing with the thought of adding that to go-landlock, but was not convinced whether it would be actually needed. The way I considered implementing it would have been to have one AccessFSSet that is ideal and one AccessFSSet that is the minimum. So instead of .BestEffort(), you'd then call something like .BestEffortButAtLeast(AccessFSSet(...)). (or maybe I can find a nicer name for that...)

It would be reasonably easy to add to go-landlock, required changes would be: (a) to add a minHandledAccessFS field to the Config struct, (b) expose a suitable method to set it, and then (c) in restrictPaths(), just before the RulesetAttr{} is created, return an error if the minimal requirement is not fulfilled. I was just not convinced whether there is actually a need for it and it felt like speculative generality... do you think this is a use case that you'd need?

If this needs to go to rules as well (as @l0kod suggested in the above with sample), then this would not be enough.

Regarding file reparenting:

If I understand this correctly, file reparenting is currently forbidden in a Landlock-restricted process in kernel 5.13, and with kernel 5.X you want to permit this in Landlock-restricted processes if they ask for it at enforcement time. Do I understand that correctly?

If that is the case, then it sounds like it should maybe not be part of the Config (RulesetAttr), but might logically fit more as an argument to RestrictPaths() (invocation of landlock_add_rule), like this:

err := landlock.V99.BestEffort().RestrictPaths( variousPaths..., landlock.EnableFileReparenting(params...), )

The thinking is:

the Config (landlock.V99, RulesetAttr) describes what should be forbidden (and with abi→∞, thats everything :))

the arguments to RestrictPaths() (landlock_add_rule) describe the exceptions to what is forbidden (the operations that the process intends to do)

So yes, the above invocation is not compatible with kernel 5.13. The go-landlock library's BestEffort() mode should restrict the strongest rules it can, but it might have to fall back to not enforcing anything at all, if file reparenting is something that the program needs, and if that is not compatible with the current kernel.

(I feel like I'm making a few assumptions here, there is a good chance that I'm misunderstanding this... :) let me know)

I have the same understanding...

Regarding evolution flexibility of the config:

If there will be more fields added to RulesetAttr in the future, it might be easier to think of this as a "permits-more-or-the-same-as" relationship on RulesetAttr structs rather than as a "subset" relationship on AccessFSSets as it's implemented now in go-landlock. So maybe it would be better to have a "ruleset" and "min_ruleset" rather than having a "handled_access_fs" and "min_handled_access_fs" in the ruleset? WDYT?

Yes, it makes sense to me.

So instead of .BestEffort(), you'd then call something like .BestEffortButAtLeast(AccessFSSet(...)). (or maybe I can find a nicer name for that...)

In the Rust library, I use ErrorThreshold and set_error_threshold() to be able to require or ignore specific features. By default, it is a best-effort approach.

If I understand this correctly, file reparenting is currently forbidden in a Landlock-restricted process in kernel 5.13, and with kernel 5.X you want to permit this in Landlock-restricted processes if they ask for it at enforcement time. Do I understand that correctly?

Yes, I think it will be set in a new bitfield in the ruleset attribute struct. An additional access-right might come as well.

the Config (landlock.V99, RulesetAttr) describes what should be forbidden (and with abi→∞, thats everything :))

…potentially everything, according to the ruleset.

So yes, the above invocation is not compatible with kernel 5.13. The go-landlock library's BestEffort() mode should restrict the strongest rules it can, but it might have to fall back to not enforcing anything at all, if file reparenting is something that the program needs, and if that is not compatible with the current kernel.

(I feel like I'm making a few assumptions here, there is a good chance that I'm misunderstanding this... :) let me know)

That is correct. :)

If there will be more fields added to RulesetAttr in the future, it might be easier to think of this as a "permits-more-or-the-same-as" relationship on RulesetAttr structs rather than as a "subset" relationship on AccessFSSets as it's implemented now in go-landlock. So maybe it would be better to have a "ruleset" and "min_ruleset" rather than having a "handled_access_fs" and "min_handled_access_fs" in the ruleset? WDYT?

Yes, good idea, it would make sense to have an optional "min_ruleset" or something similar (complementary to "ruleset").

This patch introduces Landlock Linux Security Module (LSM) support in runc, which was landed in Linux kernel 5.13. This allows unprivileged processes to create safe security sandboxes that can securely restrict the ambient rights (e.g. global filesystem access) for themselves. runtime-spec: opencontainers/runtime-spec#1111 Fixes opencontainers#2859 Signed-off-by: Kailun Qin <kailun.qin@intel.com>

AkihiroSuda · 2021-11-29T07:22:24Z

What do we need to move this forward?

vbatts · 2022-04-20T19:35:33Z

@AkihiroSuda the discussion looks good and healthy. And it looks like implementations are iterating to find consensus and polish out issues found. I haven't seen any feedback that should stop this issue from a final review.

vbatts

it SGTM apart from minimizing top-level variable names.

vbatts · 2022-04-20T19:42:02Z

specs-go/config.go

+
+// Define actions on files and directories that Landlock can restrict a sandboxed process to.
+const (
+	FSActExecute    LandlockFSAction = "execute"


Since all these FSAct* will be top-level variables, can we prefix with LL for LandLock? I'm just thinking that FSActions may overlap with another LSM or something like fanotify

Ping @kailun-qin 🙏

Done, thanks for the suggestions!

kailun-qin

Landlock has recently introduced v2 ABI in Linux 5.19: https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=cb44e4f061e16be65b8a16505e121490c66d30d0.

Shall we support it also in this PR or thru a follow-up one?

giuseppe

config schema must be updated as well

kailun-qin · 2022-08-10T11:08:04Z

config schema must be updated as well

Updated, thanks!

AkihiroSuda · 2022-09-01T16:13:30Z

Could you consider squashing commits?

kailun-qin · 2022-09-02T05:45:15Z

Could you consider squashing commits?

Squashed and rebased, PTAL, thanks!

Linux kernel 5.13 adds support for Landlock Linux Security Module (LSM). This allows unprivileged processes to create safe security sandboxes that can securely restrict the ambient rights (e.g. global filesystem access) for themselves. opencontainers#1110 Signed-off-by: Kailun Qin <kailun.qin@intel.com>

vbatts · 2023-02-01T16:17:04Z

config.md

+        Entries in the array contain the following properties:
+            * **`allowedAccess`** (array of strings, OPTIONAL) is an array of FS typed actions that are allowed by a rule.
+            * **`paths`** (array of strings, OPTIONAL) is an array of files or parent directories of the file hierarchies to restrict.
+    * **`disableBestEffort`** (bool, OPTIONAL) the `disableBestEffort` field disables the best-effort security approach for Landlock access rights.


is this warning something that the kernel/landlock generates? or is the runtime expected to map this?
I'm reading through the kernel docs and not seeing this best-effort parameter.

The "best effort" feature is implemented in user space based on the Landlock ABI level exposed by the kernel. The idea is to use Landlock if it's available and to the extent that it is available, but to fall back to a lower level of protection if the kernel is older.

At the moment, the support matrix for file system access rights is:

ABI v1 (kernel 5.13) supports all rights up to MAKE_SYM

ABI v2 (kernel 5.19) supports additionally the REFER right

And for yet-unreleased kernels:

ABI v3 (in upcoming kernel 6.2, if everything goes well) will support additionally the TRUNCATE right

So the kernel interface to this is:

use a special flag for landlock_create_ruleset(2) to figure out the ABI level

then use the Landlock API with the right access rights that are available at that level

Also see https://docs.google.com/document/d/1SkFpl_Xxyl4E6G2uYIlzL0gY2PFo-Nl8ikblLvnpvlU/edit for a bit more background and a visualization of how the API evolves

AkihiroSuda · 2023-09-26T12:39:30Z

@kailun-qin What's current status of this?

kailun-qin mentioned this pull request Aug 2, 2021

Support Landlock LSM? opencontainers/runc#2859

Open

cyphar reviewed Aug 2, 2021

View reviewed changes

kailun-qin commented Aug 2, 2021

View reviewed changes

l0kod suggested changes Aug 22, 2021

View reviewed changes

l0kod suggested changes Aug 25, 2021

View reviewed changes

kailun-qin commented Aug 26, 2021

View reviewed changes

gnoack mentioned this pull request Aug 26, 2021

Make the handledAccessFS set configurable. landlock-lsm/go-landlock#12

Closed

BoardzMaster mentioned this pull request Aug 27, 2021

Converting allowedAccess strings into AccessFSSet uint64 landlock-lsm/go-landlock#14

Open

kailun-qin mentioned this pull request Sep 1, 2021

libcontainer: add support for Landlock opencontainers/runc#3194

Draft

BoardzMaster mentioned this pull request Oct 6, 2021

Support Landlock LSM containerd/containerd#6056

Closed

tonistiigi mentioned this pull request Jan 31, 2022

seccomp: kernel 5.11 -> 5.16 containerd/containerd#6494

Merged

vbatts requested changes Apr 20, 2022

View reviewed changes

kailun-qin requested review from crosbymichael, dqminh, giuseppe, hqhq, mrunalp and tianon as code owners August 10, 2022 09:07

kailun-qin commented Aug 10, 2022

View reviewed changes

giuseppe requested changes Aug 10, 2022

View reviewed changes

kailun-qin force-pushed the add-landlock-support branch from 545f68d to 3b00e43 Compare September 2, 2022 05:44

AkihiroSuda requested a review from cyphar September 2, 2022 05:51

AkihiroSuda requested a review from giuseppe January 24, 2023 10:40

vbatts reviewed Feb 1, 2023

View reviewed changes

AkihiroSuda added this to the v1.2.0 milestone Mar 28, 2023

Zheaoli mentioned this pull request Jan 2, 2024

Carry #1111: specs-go/config: add Landlock LSM support #1241

Open

		* `abi` (object, OPTIONAL) the `abi` field defines the specific Landlock ABI version.
		This should be used by the runtime to check if the kernel supports the specified sets of Landlock features and then enforce those following a best-effort security approach.

specs-go/config: add Landlock LSM support #1111

Are you sure you want to change the base?

specs-go/config: add Landlock LSM support #1111

Conversation

kailun-qin commented Aug 2, 2021

cyphar Aug 2, 2021 • edited Loading

Choose a reason for hiding this comment

Pros

Cons

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

h-vetinari commented Aug 2, 2021

BoardzMaster commented Aug 4, 2021 • edited Loading

AkihiroSuda commented Aug 18, 2021

l0kod commented Aug 22, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

BoardzMaster commented Aug 23, 2021 • edited Loading

l0kod commented Aug 23, 2021

gnoack commented Aug 23, 2021

kailun-qin commented Aug 24, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kailun-qin Sep 3, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AkihiroSuda commented Nov 29, 2021

vbatts commented Apr 20, 2022

vbatts left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kailun-qin left a comment

Choose a reason for hiding this comment

giuseppe left a comment

Choose a reason for hiding this comment

kailun-qin commented Aug 10, 2022

AkihiroSuda commented Sep 1, 2022

kailun-qin commented Sep 2, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AkihiroSuda commented Sep 26, 2023

cyphar Aug 2, 2021 •

edited

Loading

BoardzMaster commented Aug 4, 2021 •

edited

Loading

BoardzMaster commented Aug 23, 2021 •

edited

Loading

kailun-qin Sep 3, 2021 •

edited

Loading