-
Notifications
You must be signed in to change notification settings - Fork 9
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
API compatibility improvements #12
Conversation
@gnoack, commit compat: Switch from set_best_effort() to set_compatibility() should particularly be of interest. I developed that in November (before the Landlock's refer right was developed) though. |
Thanks, I'll have a look. (might take me a bit longer to re-familiarize myself with the Rust code) |
/// the call to [`RulesetCreated::restrict_self()`](crate::RulesetCreated::restrict_self()) | ||
/// will return a | ||
/// [`RestrictionStatus { ruleset: RulesetStatus::NotEnforced, no_new_privs: false, }`](crate::RestrictionStatus). | ||
SoftRequirement, |
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 tried to figure out what the SoftRequirement
does, but am not sure I fully understood it.
It has taken me a while to make sense of the documentation (and I had a hard time following the indirection through the different types and traits, due to my unfamiliarity with Rust, I think).
Do I understand correctly that:
- When using
HardRequirement
, you'll get an error straight away when adding the specific file hierarchy rule, if the given access rights are not supported by the kernel. - When using
SoftRequirement
, the compatibility error will be "postponed" and only be returned by the call toRulesetCreated::restrict_self()
.- Both in "best effort" and "soft" mode, you cannot get a compatibility error directly.
- I suspect (but am not fully sure) that in
SoftRequirement
mode, ifrestrict_self()
returns this error, nothing was enforced. - Is it right that the compatibility mode takes effect both on the Ruleset when adding handled accesses as well as on the CreatedRuleset when adding rules?
- I only noticed that this is meant to apply to handled accesses and be stateful when I looked at tests in ruleset.rs.
- Is it right that the term "request" in this documentation is the abstraction over both handled accesses as well as individual rules? (I wonder whether this generality is worth it. Why not make it a flag in the
add_rule()
andhandle_access()
functions? This would remove this additional "request" concept and the statefulness.)
As you can see, I'm somewhat confused about a few things here. If some of what I'm saying here doesn't make sense, a good chunk of the reason is probably that I don't understand Rust that well, so some of it is probably pointless and might not be actionable. I'm sending you my vague understanding anyway, in the hope that it'll help to surface some problems that a Rust beginner might run into. %-)
P.S.: I tried following the https://docs.rs/landlock/ link from the README to refresh my memory, and found the documentation page to be empty. I'm not sure what went wrong there?
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 I understand correctly that:
- When using
HardRequirement
, you'll get an error straight away when adding the specific file hierarchy rule, if the given access rights are not supported by the kernel.
Right, it as an impact on the returned Result<>
type, which is then automatically caught with the trailing ?
.
- When using
SoftRequirement
, the compatibility error will be "postponed" and only be returned by the call toRulesetCreated::restrict_self()
.
Right, it moot the ruleset, but always return Ok(_)
(unwrapped with ?
).
- Both in "best effort" and "soft" mode, you cannot get a compatibility error directly.
Right, but you can still get the status (RestrictionStatus
) of the enforced (or not) ruleset at the end.
- I suspect (but am not fully sure) that in
SoftRequirement
mode, ifrestrict_self()
returns this error, nothing was enforced.
Right, it is especially useful e.g. for kernel not supporting the "refer" right: in this case it would not return an error when building the ruleset but it will not call landlock_restrict_self(), only return a status (RestrictionStatus
).
- Is it right that the compatibility mode takes effect both on the Ruleset when adding handled accesses as well as on the CreatedRuleset when adding rules?
Yes, the compatibility level is taken into account for all handle_access()
and add_rule()
calls thanks to the impl Compatible for Ruleset
and impl Compatible for RulesetCreated
.
- I only noticed that this is meant to apply to handled accesses and be stateful when I looked at tests in ruleset.rs.
Ruleset
and RulesetCreated
are indeed stateful, which enable them to check if access rights are consistent.
- Is it right that the term "request" in this documentation is the abstraction over both handled accesses as well as individual rules? (I wonder whether this generality is worth it. Why not make it a flag in the
add_rule()
andhandle_access()
functions? This would remove this additional "request" concept and the statefulness.)
The requested restrictions are the access rights passed to handled_access()
and add_rule()
. They are what the developer would like to enforce if possible (according to CompatLevel
). We could replace the set_compatibility()
call with a dedicated argument in handle_access()
and add_rule()
(or PathBeneath::new()
and other future type of rules), but that increases complexity for developers whereas most of the time they want a best-effort approach. This approach is simpler for most use cases (i.e. no call to set_compatibility()
), but still enables to fine-tune the Landlock compatibility. I think the builder pattern is cleaner and all optional arguments should be optional. ;)
As you can see, I'm somewhat confused about a few things here. If some of what I'm saying here doesn't make sense, a good chunk of the reason is probably that I don't understand Rust that well, so some of it is probably pointless and might not be actionable. I'm sending you my vague understanding anyway, in the hope that it'll help to surface some problems that a Rust beginner might run into. %-)
Your feedback is useful. My use of Rust may be a bit complex for this library internals but I'm looking for a simpler but still flexible and updatable-without-breaking-the-strong-type-system developer API.
P.S.: I tried following the https://docs.rs/landlock/ link from the README to refresh my memory, and found the documentation page to be empty. I'm not sure what went wrong there?
Right, I didn't uploaded to crates.io (which builds the doc and exposes it on docs.rs) yet. You can generate the documentation with cargo doc
and I just published the current documentation at https://landlock.io/rust-landlock/landlock/ (a lot of Landlock nesting but ¯\_(シ)_/¯ ).
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 suspect (but am not fully sure) that in
SoftRequirement
mode, ifrestrict_self()
returns this error, nothing was enforced.Right, it is especially useful e.g. for kernel not supporting the "refer" right: in this case it would not return an error when building the ruleset but it will not call landlock_restrict_self(), only return a status (
RestrictionStatus
).
So, to make sure I understand; you'd use it like this (pseudocode)?
- set compatibility level to "hard requirement"
- add handled access for all the V1 rights
- set compatibility level to "soft requirement"
- add handled access for "refer"
- create()
- add_rule("/tmp", read+write+...+refer)
- add_rule("/usr", read+readdir)
- restrict_self()
and when that is running on a kernel with only V1 support, then it would raise a less dramatic error during restrict_self()
, but it would still not restrict anything? And the intent is to simplify error handling?
Some small questions:
- Does there need to be another
set_compatibility()
call betweencreate()
andadd_rule()
? - Which would be the case where you would still use the hard requirement? If the only difference is in the time of surfacing the error, why not just always surface it only during
restrict_self()
?
- Is it right that the term "request" in this documentation is the abstraction over both handled accesses as well as individual rules? (I wonder whether this generality is worth it. Why not make it a flag in the
add_rule()
andhandle_access()
functions? This would remove this additional "request" concept and the statefulness.)The requested restrictions are the access rights passed to
handled_access()
andadd_rule()
. They are what the developer would like to enforce if possible (according toCompatLevel
). We could replace theset_compatibility()
call with a dedicated argument inhandle_access()
andadd_rule()
(orPathBeneath::new()
and other future type of rules), but that increases complexity for developers whereas most of the time they want a best-effort approach. This approach is simpler for most use cases (i.e. no call toset_compatibility()
), but still enables to fine-tune the Landlock compatibility. I think the builder pattern is cleaner and all optional arguments should be optional. ;)
OK then :)
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 suspect (but am not fully sure) that in
SoftRequirement
mode, ifrestrict_self()
returns this error, nothing was enforced.Right, it is especially useful e.g. for kernel not supporting the "refer" right: in this case it would not return an error when building the ruleset but it will not call landlock_restrict_self(), only return a status (
RestrictionStatus
).So, to make sure I understand; you'd use it like this (pseudocode)?
- set compatibility level to "hard requirement"
- add handled access for all the V1 rights
- set compatibility level to "soft requirement"
- add handled access for "refer"
- create()
- add_rule("/tmp", read+write+...+refer)
- add_rule("/usr", read+readdir)
- restrict_self()
and when that is running on a kernel with only V1 support, then it would raise a less dramatic error during
restrict_self()
, but it would still not restrict anything? And the intent is to simplify error handling?
Correct! In fact I was waiting for a v2 to test this behavior. ;)
I just added support for ABI v2 (no PR yet) and here is your test (which passes):
#[test]
fn ignore_abi_v2_with_abi_v1() {
assert_eq!(
Ruleset::from(ABI::V1)
.set_compatibility(CompatLevel::HardRequirement)
.handle_access(AccessFs::from_all(ABI::V1))
.unwrap()
.set_compatibility(CompatLevel::SoftRequirement)
.handle_access(AccessFs::Refer)
.unwrap()
.create()
.unwrap()
.add_rule(PathBeneath::new(
PathFd::new("/tmp").unwrap(),
AccessFs::from_all(ABI::V2)
))
.unwrap()
.add_rule(PathBeneath::new(
PathFd::new("/usr").unwrap(),
make_bitflags!(AccessFs::{ReadFile | ReadDir})
))
.unwrap()
.restrict_self()
.unwrap(),
RestrictionStatus {
ruleset: RulesetStatus::NotEnforced,
no_new_privs: false,
}
);
}
It's verbose but it is for the test. #10 and the current features makes writing such rule much more easier and less verbose. I guess I should create the PR for ABI v2 and rebase this one on top of it with your test.
Without the any set_compatibility()
call, this example returns RestrictionStatus { ruleset: PartiallyEnforced, no_new_privs: true }
I noticed there is an issue with the HardRequirement when added after the first rule though, I'll fix that.
Some small questions:
- Does there need to be another
set_compatibility()
call betweencreate()
andadd_rule()
?
You only call set_compatibility()
when you want the following part of the builder behavior to change.
- Which would be the case where you would still use the hard requirement? If the only difference is in the time of surfacing the error, why not just always surface it only during
restrict_self()
?
The use case for the hard requirement is when you control the kernel and the sandbox, and you want to make sure that you launch a process in a sandboxed environment.
So yes, the time of surfacing the error changes. The use case for the soft requirement is to not apply the sandbox when a specific access right (e.g. refer) is not supported, without throwing an error because you still want to sandbox as much as possible, but want to know e.g. if a legitimate path is not accessible whereas it should be, which brings confidence in the sandbox configuration even when the running kernel doesn't (fully) support it. I want to help developers catch configuration errors as soon as possible, e.g. in alpha releases.
The difference is really in the way Rust helps to handle errors (i.e. with the ?
) and the error (strong) types.
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.
Ah thanks, that makes it clearer; the example really helped me understand this better.
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 think the "footgun" issue here is mainly that you need to really understand what it is doing, because otherwise it will do something less restrictive than what you'd expect. One might for example think that adding
AccessFs::Refer
as a soft requirement will just ignore theAccessFs::Refer
bit and you're all good for the rest of the sandbox. But the fact that it drops the sandbox entirely opens up people to unwelcome surprises.This is especially relevant for people that are from category 1 (bubblewrap etc.), which might look at this initially and think "sweet, I can just use soft requirements and then it'll always allow Refer and just enforce it in version 2", whereas what actually happens is that it allows everything.
Hmm, this is the description of BestEffort
. Would renaming SoftRequirement
to something else help? How would you improve the situation?
I get that one issue would be that users forget to "reset" the compatibility level by calling set_compatibility(CompatLevel::BestEffort)
after switching to SoftRequirement
for AccessFs::Refer
. Instead of changing the ruleset's compatibility level, what about tuning the access types? That could be something like:
ruleset.handle_access(AccessFs::Refer.set_compatibility(CompatLevel::SoftRequirement))?;
This may look a bit verbose but it would be easy to use some helpers to tune a set of access rights, and this use case is not the "default safe-for-most-sandboxing-use-cases" 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.
Changing the name to something more alarming was something I did consider myself as well, but I wasn't able to come up with a good suggestion.
I'd go for something akin to CompatLevel::NoSandboxIfMissing
, but that doesn't exactly roll off the tongue and fit in well with the rest. But it would be a lot more descriptive and I think something like this could communicate better what the actual behavior is.
And while I think that not making it sticky would probably be a good idea too, because people tend to forget stuff a lot, I don't think that would address my concerns completely.
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.
To summarize, here are the two use cases:
- to detect any incompatibility, e.g.
- for developers and CI tests, to be sure that sandboxing is not an issue for legitimate use,
- for security software, to be sure that a set of security properties are guarantee;
- to disable the whole sandboxing if a required feature is not supported (e.g.
AccessFs::Refer
).
Thinking more about it, here is another proposal:
- add a new (alternative) constructor to
Ruleset
to be able to enforce a "hard requirement" for everything (set to "best effort" with the default() constructor); - add a method to access rights to be able to set the "soft requirement" property.
Example with current API:
Ruleset::new()
.set_compatibility(CompatLevel::HardRequirement)
.handle_access(AccessFs::from_all(ABI::V1))?
.set_compatibility(CompatLevel::SoftRequirement)
.handle_access(AccessFs::Refer)?
.set_compatibility(CompatLevel::HardRequirement)
Same example with the proposed API:
Ruleset::new_compatible(CompatRuleset::ErrorIfUnmet)
.handle_access(AccessFs::from_all(ABI::V1))?
.handle_access(AccessFs::Refer.disable_sandbox_if_unmet(true))?
Example with the best effort behavior:
Ruleset::default()
.handle_access(AccessFs::from_all(ABI::V1))?
.handle_access(AccessFs::Refer)?
This design is simpler and it removes the mutable builder property (multiple .set_compatibility()
calls) that may be confusing and error prone.
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.
Yeah I like that, especially the "disable_sandbox_if_unmet" name is very clear and I don't see anyone shooting themselves in the foot with that. There might be a shorter option that says the same, but I like its clarity.
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. This proposal is clearer and much easier to write IMHO.
2888e02
to
5cf1708
Compare
5cf1708
to
64e8850
Compare
I still need to update the documentation. |
64e8850
to
1aa1c83
Compare
1aa1c83
to
e152e27
Compare
@cd-work, @andreaphylum, if you can take a look that would be nice. |
@gnoack and @dburgener, I updated the documentation, added tests, and cleaned up some code to make it more generic. I removed unrelated patches that are either already merged or will be part of an other PR. |
c5687aa
to
b647052
Compare
b647052
to
bf3210a
Compare
I revamped a big part of the compatibility code (mostly with the latest commit) to make it safer and give more compatibility guarantees. The user API is unchanged. |
This comment was marked as resolved.
This comment was marked as resolved.
Thanks for testing. The behavior should not change, but I cannot reproduce your test. The Can you give more context? What is the kernel version you're using? Does |
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
I tried to reproduce your code with this patch on Linux 5.19.17 but it works before and after this PR: diff --git a/src/lib.rs b/src/lib.rs
index fe62032c5899..b2fcf5b94364 100644
--- a/src/lib.rs
+++ b/src/lib.rs
@@ -203,6 +203,21 @@ mod tests {
);
}
+ #[test]
+ fn ruleset_compat_v1() {
+ let status = Ruleset::new()
+ .handle_access(AccessFs::from_all(ABI::V1))
+ .unwrap()
+ .create()
+ .unwrap()
+ .add_rules(path_beneath_rules(["/"], AccessFs::from_read(ABI::V1)))
+ .unwrap()
+ .restrict_self()
+ .unwrap();
+
+ assert_eq!(status.ruleset, RulesetStatus::FullyEnforced);
+ }
+
#[test]
fn ruleset_enforced() {
let abi = ABI::V1; Could you please try this patch or try to write a test that works before this PR but not after? |
This comment was marked as resolved.
This comment was marked as resolved.
Ok, this is indeed the result of a fix. The issue was missing checks for access rights of rules added with I changed a few things:
This should fix your test and keep the same behavior for your (legitimate) use case. |
Thanks, it works now! |
f396d11
to
d4927ae
Compare
I plan to merge this next week if there is no issue found. |
The set_compatibility() build method enables to set the compatibility level of an object builder (e.g. Ruleset). - set_compatibility(CompatLevel::BestEffort) is like set_best_effort(true). - set_compatibility(CompatLevel::HardRequirement) is like set_best_effort(false). - set_compatibility(CompatLevel::SoftRequirement) makes the ruleset moot if one of the following build requests (part of a build method) are not supported, but don't return a compatibility error. CompatLevel::SoftRequirement is particularly useful for access rights such as AccessFs::Refer. Indeed, it can be used to ignore the whole sandboxing if this could break the normal workflow of an application (which legitimately requires to move files to arbitrary directories). set_best_effort() is kept for API compatibility. Signed-off-by: Mickaël Salaün <mic@digikod.net>
This enables to change the compatibility for builder objects or their references like it is done for RulesetAttr and RulesetCreatedAttr. Make Compatibility a requirement for RulesetAttr and RulesetCreatedAttr traits to ensure consistency. This change makes it possible to read the CompatLevel tied to an object. It may be superfluous but should not harm. Signed-off-by: Mickaël Salaün <mic@digikod.net>
This gives a guarantee that abi will not be modified outside of the compat module. Signed-off-by: Mickaël Salaün <mic@digikod.net>
Check that a ruleset really handles at least one access right. Signed-off-by: Mickaël Salaün <mic@digikod.net>
Complete CompatState with the Init state to fully rely on CompatState instead of ABI or is_mooted(), and rename Final to Dummy. This enables to not change a ruleset ABI once set. Always update CompatState for any BitFlags<Access>::try_compat() call. Make can_emulate() handles ABIs not supporting a minimal supported version, which is required for the new abi_v2_refer_only() test. Signed-off-by: Mickaël Salaün <mic@digikod.net>
Add too_much_access_rights_for_a_file() to test access rights on non-directory. It will be changed/fixed with a following commit to reflect the partial enforcement according to the (erroneous) request. Add path_beneath_rules_with_too_much_access_rights_for_a_file() to test the current behavior of path_beneath_rules(), which will be changed with a following commit. Signed-off-by: Mickaël Salaün <mic@digikod.net>
This is useful and will help avoid existing code to break because of the upcoming compatibility error fix. Signed-off-by: Mickaël Salaün <mic@digikod.net>
Extend check_ruleset_support() to be able to handle never-fully-supported rulesets. This is useful to test permanent incompatibilities such as directory-only access rights requested on files, which will be fixed and tested with too_much_access_rights_for_a_file() in the next commit. Add AccessFs::From_file(), which indirectly exposes ACCESS_FILE according to an ABI. This is useful for this test and other similar code that don't rely on path_beneath_rules(). Signed-off-by: Mickaël Salaün <mic@digikod.net>
It is possible to implement try_compat() in an inconsistent way compared to other implementations. Instead, implement the whole compatibility logic in the default try_compat() implementation, and create tailored method to be implemented by objects: try_compat_inner() and try_compat_children(). The new CompatResult enum contains all the required informations to implement a consistent try_compat() for all trait implementations: the full, partial or not-supported state, the associated compatible value, and the potential associated error. This way, try_compat() has everything to take the decision to either return a Some(Self) or a CompatError according to the CompatLevel. The try_compat() default implementation now always update the self's CompatState, and then return a result according to its CompatLevel thanks to clean and exhaustive CompatResult matches. We now only use the Compatibility struct for Ruleset and RulesetCreated, but we pass separately ABI, CompatLevel and &mut CompatState to try_compat(). This enables to differentiate mutability and store CompatState in all Compatible types. ABI and CompatLevel should only be inherited from Ruleset and RulesetCreated. try_compat_inner() needs to be implemented by all TryCompat types. This method only gets a non-mutable ABI, and returns a new CompatResult or a CompatError. Delegating nested TryCompat objects management (e.g. allowed_access by PathBeneath) can lead to inconsistencies between different objects. To avoid this issue as much as possible, create a new try_compat_children() method to specifically call try_compat() on all the nested objects, if any. For instance, PathBeneath objects contain an AccessFs object and they both implement Compatible, which lets them have an independent CompatLevel thanks to set_compatible(). It is not possible to call try_compat() from a try_compat_inner() implementation because they don't return the same result type. Instead, the new try_compat_children() returns the same type as try_compat() and can then call it for all the children. try_compat_children() is only called by try_compat(), which then makes all the compatibility process consistent across different object implementations. Test this fixed behavior with a more complete too_much_access_rights_for_a_file(), initially introduced with a previous commit for the purpose of this fix. As a side effect, the PathBeneath implementation now first checks its allowed_access generic compatibility (with try_compat_children), and then the compatibility with the file type (with try_compat_inner). To better fit with the Compatible contract, replace CompatLevel with Option<CompatLevel>. None is set by default until users change it. This enables to inherit the parent compatibility level if the current one is not set. Add the TailoredCompatLevel trait with the tailored_compat_level() method to get the current compatibility level accordingly. Automatically implements TailoredCompatLevel for each Compatible implementation (only PathBeneath for now), which helps avoid missing an implementation for a new Compatible types. Signed-off-by: Mickaël Salaün <mic@digikod.net>
d4927ae
to
62a0778
Compare
Add several API improvements, including for the compatibility/best-effort handling.