API compatibility improvements

[l0kod]

Add several API improvements, including for the compatibility/best-effort handling.

[l0kod]

@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.

[gnoack]

@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)

[gnoack]

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 to RulesetCreated::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, if restrict_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() and handle_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?

[l0kod]

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 to RulesetCreated::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, if restrict_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() and handle_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 ¯\_(シ)_/¯ ).

[gnoack]

• I suspect (but am not fully sure) that in SoftRequirement mode, if restrict_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 between create() and add_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() and handle_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. ;)

OK then :)

[l0kod]

• I suspect (but am not fully sure) that in SoftRequirement mode, if restrict_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 between create() and add_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.

[gnoack]

Ah thanks, that makes it clearer; the example really helped me understand this better.

[l0kod]

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 the AccessFs::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.

[cd-work]

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.

[l0kod]

To summarize, here are the two use cases:

1. 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;
2. 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.

[cd-work]

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.

[bjorn3]

I agree. This proposal is clearer and much easier to write IMHO.

[l0kod]

I still need to update the documentation.

[l0kod]

@cd-work, @andreaphylum, if you can take a look that would be nice.

[l0kod]

@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.

[l0kod]

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.

[l0kod]

Previously this resulted in FullyEnforced but now gives PartiallyEnforced. Is this expected? What changed in the compatibility logic?

Thanks for testing. The behavior should not change, but I cannot reproduce your test. The check_ruleset_support helper should test all meaningful combinations, and it didn't change with these commits.

Can you give more context? What is the kernel version you're using? Does path1 exists? Can you bisect to identify the faulty commit?

[l0kod]

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?

[l0kod]

Ok, this is indeed the result of a fix. The issue was missing checks for access rights of rules added with add_rules() according to the file type. Indeed, allowing all access rights on a file will make the kernel return an error because some of them don't make sense for a file (e.g. create a new file... in a file). The commit you pointed out fixes this behavior, which resulted of a visible behavior change for your code.

I changed a few things:

• added the AccessFs::from_file() helper;
• changed path_beneath_rules() to automatically check and downgrade access rights for files (i.e. remove superfluous directory-only access rights);
• added tests, especially the (full) too_much_access_rights_for_a_file() and path_beneath_rules_with_too_much_access_rights_for_a_file() (more generic than the one we wrote).

This should fix your test and keep the same behavior for your (legitimate) use case.

[mrcnski]

Thanks, it works now!

[l0kod]

I plan to merge this next week if there is no issue found.

Cc @mrcnski @bjorn3 @cd-work @int5-grey