-
Notifications
You must be signed in to change notification settings - Fork 553
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
specs-go/config: add Landlock LSM support #1111
base: main
Are you sure you want to change the base?
Conversation
config.md
Outdated
* **`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. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
specs-go/config.go
Outdated
// 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"` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we really need multiple LandlockRule
s of the same type?
For example, we can do instead:
"rules": {
"path_beneath": [
{
"allowedAccess": ["xxx"],
"paths": ["xxx"]
},
{
"allowedAccess": ["xxx"],
"paths": ["xxx"]
}
]
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree that it would make the configuration lighter.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the feedbacks! Use an object with rule-type keys instead to make it lighter.
CC @l0kod |
Having this format of Landlock rules I think is enough (like @kailun-qin suggested)
Mickaël Salaün, please give your opinion CC @l0kod |
@kailun-qin |
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. 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 FYI, to be able to use Landlock, the 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. |
config.md
Outdated
@@ -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": [ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
handledAccessFS
(a "c" is missing)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice catch. Fixed, thanks!
config.md
Outdated
|
||
* **`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. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@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 abi
s 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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
config.md
Outdated
"landlock": { | ||
"ruleset": { | ||
"handledAcessFS": [ | ||
"LANDLOCK_ACCESS_FS_EXECUTE", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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").
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Make sense to me. Updated, thanks!
specs-go/config.go
Outdated
// 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"` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree that it would make the configuration lighter.
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 |
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 Please open a feature request on |
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:
@AkihiroSuda Regarding POC implementation, we're working on it based on Please kindly take another look. Let me know if any further suggestions. Thanks! |
config.md
Outdated
|
||
* **`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. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
config.md
Outdated
* **`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. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sounds good, I'll expose it. Tracking it in landlock-lsm/go-landlock#12
config.md
Outdated
* **`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. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
// 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"` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It looks good to me!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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"
},
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A simplified version of the proposition would be to only add
required
andunsupported
(names may not be clear enough) to theruleset
block. I don't know about the evolution flexibility of this specification though.This also makes me wonder about
rules
not being nested intoruleset
. That would make more sense. It would be lighter to mergelandlock
andruleset
intolandlockRuleset
(the same way there isapparmorProfile
andselinuxLabel
). 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).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 <[email protected]>
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 <[email protected]>
What do we need to move this forward? |
@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. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it SGTM apart from minimizing top-level variable names.
specs-go/config.go
Outdated
|
||
// Define actions on files and directories that Landlock can restrict a sandboxed process to. | ||
const ( | ||
FSActExecute LandlockFSAction = "execute" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ping @kailun-qin 🙏
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done, thanks for the suggestions!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
config schema must be updated as well
Updated, thanks! |
Could you consider squashing commits? |
545f68d
to
3b00e43
Compare
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 <[email protected]>
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. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
@kailun-qin What's current status of this? |
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 Co-authored-by: Zheao Li <[email protected]> Signed-off-by: Kailun Qin <[email protected]>
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 Co-authored-by: Zheao Li <[email protected]> Signed-off-by: Kailun Qin <[email protected]> Signed-off-by: Manjusaka <[email protected]>
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 [email protected]