From 35b016cc8ab94b32c9a590e2429ab1e60beb533e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Micka=C3=ABl=20Sala=C3=BCn?= Date: Sat, 1 Jun 2024 11:08:15 +0200 Subject: [PATCH] ruleset: Fix compatibility inconsistency MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit When adding a rule to a ruleset created with ABI::Unsupported, its compatibility state could change from CompatLevel::No to CompatLevel::Partial, which is wrong and could lead to an unexpected error when trying to restrict the caller with -1 as a ruleset file descriptor. This is especially visible with a following change to prioritize error over incompatibility. Fix that by setting the mapping of ABI::Unsupported to CompatLevel::Dummy instead of CompatLevel::No. As a side effect, prctl(PR_SET_NO_NEW_PRIVS, 1) is now always called when no_new_privs is set to true. This was not the case before when the ruleset's compatibility level was explicitly set to SoftRequirement. This means that no RestrictSelfError::SetNoNewPrivsCall can now be returned in this specific case. The rationale is that no_new_privs was introduced with Linux 3.5 whereas the oldest supported kernel is now Linux 4.19 . It is not worth it to handle compatibility complexity for such widely available feature. Moreover, this new behavior lead to a more deterministic and secure execution, and no error should be returned by the kernel. Add tests to check theses changes, and change some existing tests to follow the new logic. Signed-off-by: Mickaël Salaün --- src/access.rs | 4 +-- src/compat.rs | 2 +- src/ruleset.rs | 91 +++++++++++++++++++++++++++++++++++++++++--------- 3 files changed, 79 insertions(+), 18 deletions(-) diff --git a/src/access.rs b/src/access.rs index 451efff..9cf0024 100644 --- a/src/access.rs +++ b/src/access.rs @@ -127,7 +127,7 @@ fn compat_bit_flags() { compat = ABI::Unsupported.into(); // Tests that the ruleset is marked as unsupported. - assert!(compat.state == CompatState::No); + assert!(compat.state == CompatState::Dummy); // Access-rights are valid (but ignored) when they are not required for the current ABI. assert_eq!( @@ -139,7 +139,7 @@ fn compat_bit_flags() { // Tests that the ruleset is in an unsupported state, which is important to be able to still // enforce no_new_privs. - assert!(compat.state == CompatState::No); + assert!(compat.state == CompatState::Dummy); // Access-rights are not valid when they are required for the current ABI. compat.level = Some(CompatLevel::HardRequirement); diff --git a/src/compat.rs b/src/compat.rs index a61f24e..3842c91 100644 --- a/src/compat.rs +++ b/src/compat.rs @@ -269,7 +269,7 @@ impl From for Compatibility { level: Default::default(), state: match abi { // Don't forces the state as Dummy because no_new_privs may still be legitimate. - ABI::Unsupported => CompatState::No, + ABI::Unsupported => CompatState::Dummy, _ => CompatState::Init, }, } diff --git a/src/ruleset.rs b/src/ruleset.rs index 47653a5..ba8515d 100644 --- a/src/ruleset.rs +++ b/src/ruleset.rs @@ -544,9 +544,8 @@ pub trait RulesetCreatedAttr: Sized + AsMut + Compatible { /// Configures the ruleset to call `prctl(2)` with the `PR_SET_NO_NEW_PRIVS` command /// in [`restrict_self()`](RulesetCreated::restrict_self). /// - /// This is ignored if an error was encountered to a [`Ruleset`] or [`RulesetCreated`] method - /// call while [`CompatLevel::SoftRequirement`] was set (with - /// [`set_compatibility()`](Compatible::set_compatibility)). + /// This `prctl(2)` call is never ignored, even if an error was encountered on a [`Ruleset`] or + /// [`RulesetCreated`] method call while [`CompatLevel::SoftRequirement`] was set. fn set_no_new_privs(mut self, no_new_privs: bool) -> Self { >::as_mut(&mut self).no_new_privs = no_new_privs; self @@ -585,13 +584,10 @@ impl RulesetCreated { /// On error, returns a wrapped [`RestrictSelfError`]. pub fn restrict_self(mut self) -> Result { let mut body = || -> Result { - // FIXME: Enforce no_new_privs even if something failed with SoftRequirement. The - // rationale is that no_new_privs should not be an issue on its own if it is not - // explicitly deactivated. - // - // Ignores prctl_set_no_new_privs() if an error was encountered with - // CompatLevel::SoftRequirement set. - let enforced_nnp = if self.compat.state != CompatState::Dummy && self.no_new_privs { + // Enforce no_new_privs even if something failed with SoftRequirement. The rationale is + // that no_new_privs should not be an issue on its own if it is not explicitly + // deactivated. + let enforced_nnp = if self.no_new_privs { if let Err(e) = prctl_set_no_new_privs() { match self.compat.level.into() { CompatLevel::BestEffort => {} @@ -738,6 +734,71 @@ fn ruleset_created_attr() { ); } +#[test] +fn ruleset_compat_dummy() { + for level in [CompatLevel::BestEffort, CompatLevel::SoftRequirement] { + println!("level: {:?}", level); + + // ABI:Unsupported does not support AccessFs::Execute. + let ruleset = Ruleset::from(ABI::Unsupported); + assert_eq!(ruleset.compat.state, CompatState::Dummy); + + let ruleset = ruleset.set_compatibility(level); + assert_eq!(ruleset.compat.state, CompatState::Dummy); + + let ruleset = ruleset.handle_access(AccessFs::Execute).unwrap(); + assert_eq!(ruleset.compat.state, CompatState::Dummy); + + let ruleset_created = ruleset.create().unwrap(); + assert_eq!(ruleset_created.compat.state, CompatState::Dummy); + + let ruleset_created = ruleset_created + .add_rule(PathBeneath::new( + PathFd::new("/usr").unwrap(), + AccessFs::Execute, + )) + .unwrap(); + assert_eq!(ruleset_created.compat.state, CompatState::Dummy); + } +} + +#[test] +fn ruleset_compat_partial() { + // CompatLevel::BestEffort + let ruleset = Ruleset::from(ABI::V1); + assert_eq!(ruleset.compat.state, CompatState::Init); + + // ABI::V1 does not support AccessFs::Refer. + let ruleset = ruleset.handle_access(AccessFs::Refer).unwrap(); + assert_eq!(ruleset.compat.state, CompatState::No); + + let ruleset = ruleset.handle_access(AccessFs::Execute).unwrap(); + assert_eq!(ruleset.compat.state, CompatState::Partial); + + // Requesting to handle another unsupported handled access does not change anything. + let ruleset = ruleset.handle_access(AccessFs::Refer).unwrap(); + assert_eq!(ruleset.compat.state, CompatState::Partial); + + let ruleset_created = ruleset.create().unwrap(); + assert_eq!(ruleset_created.compat.state, CompatState::Partial); + + let ruleset_created = ruleset_created + .add_rule(PathBeneath::new( + PathFd::new("/usr").unwrap(), + AccessFs::Execute, + )) + .unwrap(); + assert_eq!(ruleset_created.compat.state, CompatState::Partial); + + let ruleset_created = ruleset_created + .add_rule(PathBeneath::new( + PathFd::new("/usr").unwrap(), + AccessFs::Refer, + )) + .unwrap(); + assert_eq!(ruleset_created.compat.state, CompatState::Partial); +} + #[test] fn ruleset_unsupported() { assert_eq!( @@ -768,8 +829,8 @@ fn ruleset_unsupported() { .unwrap(), RestrictionStatus { ruleset: RulesetStatus::NotEnforced, - // With SoftRequirement, no_new_privs is discarded. - no_new_privs: false, + // With SoftRequirement, no_new_privs is still enabled. + no_new_privs: true, } ); @@ -815,9 +876,9 @@ fn ruleset_unsupported() { .unwrap(), RestrictionStatus { ruleset: RulesetStatus::NotEnforced, - // With SoftRequirement, no_new_privs is discarded if there is an error + // With SoftRequirement, no_new_privs is still enabled, even if there is an error // (e.g. unsupported access right). - no_new_privs: false, + no_new_privs: true, } ); } @@ -917,7 +978,7 @@ fn ignore_abi_v2_with_abi_v1() { .unwrap(), RestrictionStatus { ruleset: RulesetStatus::NotEnforced, - no_new_privs: false, + no_new_privs: true, } ); }