Update error priority and no_new_privs behavior

src/access.rs

@@ -52,34 +52,36 @@ impl<A> TryCompat<A> for BitFlags<A>
 where
     A: Access,
 {
-    fn try_compat_inner(self, abi: ABI) -> Result<CompatResult<Self, A>, CompatError<A>> {
+    fn try_compat_inner(&mut self, abi: ABI) -> Result<CompatResult<A>, CompatError<A>> {
         if self.is_empty() {
             // Empty access-rights would result to a runtime error.
             Err(AccessError::Empty.into())
-        } else if !Self::all().contains(self) {
+        } else if !Self::all().contains(*self) {
             // Unknown access-rights (at build time) would result to a runtime error.
             // This can only be reached by using the unsafe BitFlags::from_bits_unchecked().
             Err(AccessError::Unknown {
-                access: self,
-                unknown: self & full_negation(Self::all()),
+                access: *self,
+                unknown: *self & full_negation(Self::all()),
             }
             .into())
         } else {
-            let compat = self & A::from_all(abi);
-            if compat.is_empty() {
+            let compat = *self & A::from_all(abi);
+            let ret = if compat.is_empty() {
                 Ok(CompatResult::No(
-                    AccessError::Incompatible { access: self }.into(),
+                    AccessError::Incompatible { access: *self }.into(),
                 ))
-            } else if compat != self {
+            } else if compat != *self {
                 let error = AccessError::PartiallyCompatible {
-                    access: self,
-                    incompatible: self & full_negation(compat),
+                    access: *self,
+                    incompatible: *self & full_negation(compat),
                 }
                 .into();
-                Ok(CompatResult::Partial(compat, error))
+                Ok(CompatResult::Partial(error))
             } else {
-                Ok(CompatResult::Full(self))
-            }
+                Ok(CompatResult::Full)
+            };
+            *self = compat;
+            ret
         }
     }
 }
@@ -127,7 +129,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 +141,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);

src/compat.rs

@@ -269,7 +269,7 @@ impl From<ABI> 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,
             },
         }
@@ -558,20 +558,15 @@ fn tailored_compat_level() {
     }
 }
 
-// CompatResult is useful because we don't want to duplicate objects (potentially wrapping a file
-// descriptor), and we may not have compatibility errors for some objects.  TryCompat::try_compat()
-// is responsible to either take T or CompatError<A> according to the compatibility level.
-//
 // CompatResult is not public outside this crate.
-pub enum CompatResult<T, A>
+pub enum CompatResult<A>
 where
-    T: TryCompat<A>,
     A: Access,
 {
     // Fully matches the request.
-    Full(T),
+    Full,
     // Partially matches the request.
-    Partial(T, CompatError<A>),
+    Partial(CompatError<A>),
     // Doesn't matches the request.
     No(CompatError<A>),
 }
@@ -582,14 +577,19 @@ where
     Self: Sized + TailoredCompatLevel,
     A: Access,
 {
-    fn try_compat_inner(self, abi: ABI) -> Result<CompatResult<Self, A>, CompatError<A>>;
+    fn try_compat_inner(&mut self, abi: ABI) -> Result<CompatResult<A>, CompatError<A>>;
 
     // Default implementation for objects without children.
     //
     // If returning something other than Ok(Some(self)), the implementation must use its own
     // compatibility level, if any, with self.tailored_compat_level(default_compat_level), and pass
     // it with the abi and compat_state to each child.try_compat().  See PathBeneath implementation
     // and the self.allowed_access.try_compat() call.
+    //
+    // # Warning
+    //
+    // Errors must be prioritized over incompatibility (i.e. return Err(e) over Ok(None)) for all
+    // children.
     fn try_compat_children<L>(
         self,
         _abi: ABI,
@@ -614,48 +614,51 @@ where
         L: Into<CompatLevel>,
     {
         let compat_level = self.tailored_compat_level(parent_level);
-        let new_self = match self.try_compat_children(abi, compat_level, compat_state)? {
-            Some(n) => n,
-            None => return Ok(None),
-        };
-        match new_self.try_compat_inner(abi) {
-            Ok(CompatResult::Full(new_self)) => {
+        let some_inner = match self.try_compat_inner(abi) {
+            Ok(CompatResult::Full) => {
                 compat_state.update(CompatState::Full);
-                Ok(Some(new_self))
+                true
             }
-            Ok(CompatResult::Partial(new_self, error)) => match compat_level {
+            Ok(CompatResult::Partial(error)) => match compat_level {
                 CompatLevel::BestEffort => {
                     compat_state.update(CompatState::Partial);
-                    Ok(Some(new_self))
+                    true
                 }
                 CompatLevel::SoftRequirement => {
                     compat_state.update(CompatState::Dummy);
-                    Ok(None)
+                    false
                 }
                 CompatLevel::HardRequirement => {
                     compat_state.update(CompatState::Dummy);
-                    Err(error)
+                    return Err(error);
                 }
             },
             Ok(CompatResult::No(error)) => match compat_level {
                 CompatLevel::BestEffort => {
                     compat_state.update(CompatState::No);
-                    Ok(None)
+                    false
                 }
                 CompatLevel::SoftRequirement => {
                     compat_state.update(CompatState::Dummy);
-                    Ok(None)
+                    false
                 }
                 CompatLevel::HardRequirement => {
                     compat_state.update(CompatState::Dummy);
-                    Err(error)
+                    return Err(error);
                 }
             },
-            Err(e) => {
+            Err(error) => {
                 // Safeguard to help for test consistency.
                 compat_state.update(CompatState::Dummy);
-                Err(e)
+                return Err(error);
             }
+        };
+
+        // At this point, any inner error have been returned, so we can proceed with
+        // try_compat_children()?.
+        match self.try_compat_children(abi, compat_level, compat_state)? {
+            Some(n) if some_inner => Ok(Some(n)),
+            _ => Ok(None),
         }
     }
 }

src/fs.rs

@@ -269,11 +269,9 @@ where
     }
 
     fn try_compat_inner(
-        mut self,
+        &mut self,
         _abi: ABI,
-    ) -> Result<CompatResult<Self, AccessFs>, CompatError<AccessFs>> {
-        // self.allowed_access was updated with try_compat_children(), called by try_compat().
-
+    ) -> Result<CompatResult<AccessFs>, CompatError<AccessFs>> {
         // Gets subset of valid accesses according the FD type.
         let valid_access =
             if is_file(&self.parent_fd).map_err(|e| PathBeneathError::StatCall { source: e })? {
@@ -290,22 +288,56 @@ where
             .into();
             self.allowed_access = valid_access;
             // Linux would return EINVAL.
-            Ok(CompatResult::Partial(self, error))
+            Ok(CompatResult::Partial(error))
         } else {
-            Ok(CompatResult::Full(self))
+            Ok(CompatResult::Full)
         }
     }
 }
 
+#[test]
+fn path_beneath_try_compat_children() {
+    use crate::*;
+
+    // AccessFs::Refer is not handled by ABI::V1 and only for directories.
+    let access_file = AccessFs::ReadFile | AccessFs::Refer;
+
+    // Test error ordering with ABI::V1
+    let mut ruleset = Ruleset::from(ABI::V1).handle_access(access_file).unwrap();
+    // Do not actually perform any syscall.
+    ruleset.compat.state = CompatState::Dummy;
+    assert!(matches!(
+        RulesetCreated::new(ruleset, -1)
+            .set_compatibility(CompatLevel::HardRequirement)
+            .add_rule(PathBeneath::new(PathFd::new("/dev/null").unwrap(), access_file))
+            .unwrap_err(),
+        RulesetError::AddRules(AddRulesError::Fs(AddRuleError::Compat(
+            CompatError::PathBeneath(PathBeneathError::DirectoryAccess { access, incompatible })
+        ))) if access == access_file && incompatible == AccessFs::Refer
+    ));
+
+    // Test error ordering with ABI::V2
+    let mut ruleset = Ruleset::from(ABI::V2).handle_access(access_file).unwrap();
+    // Do not actually perform any syscall.
+    ruleset.compat.state = CompatState::Dummy;
+    assert!(matches!(
+        RulesetCreated::new(ruleset, -1)
+            .set_compatibility(CompatLevel::HardRequirement)
+            .add_rule(PathBeneath::new(PathFd::new("/dev/null").unwrap(), access_file))
+            .unwrap_err(),
+        RulesetError::AddRules(AddRulesError::Fs(AddRuleError::Compat(
+            CompatError::PathBeneath(PathBeneathError::DirectoryAccess { access, incompatible })
+        ))) if access == access_file && incompatible == AccessFs::Refer
+    ));
+}
+
 #[test]
 fn path_beneath_try_compat() {
     use crate::*;
 
     let abi = ABI::V1;
 
     for file in &["/etc/passwd", "/dev/null"] {
-        // TODO: test try_compat_children
-
         let mut compat_state = CompatState::Init;
         let ro_access = AccessFs::ReadDir | AccessFs::ReadFile;
         assert!(matches!(

src/ruleset.rs

@@ -544,9 +544,8 @@ pub trait RulesetCreatedAttr: Sized + AsMut<RulesetCreated> + 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 {
         <Self as AsMut<RulesetCreated>>::as_mut(&mut self).no_new_privs = no_new_privs;
         self
@@ -563,7 +562,7 @@ pub struct RulesetCreated {
 }
 
 impl RulesetCreated {
-    fn new(ruleset: Ruleset, fd: RawFd) -> Self {
+    pub(crate) fn new(ruleset: Ruleset, fd: RawFd) -> Self {
         // The compatibility state is initialized by Ruleset::create().
         #[cfg(test)]
         assert!(!matches!(ruleset.compat.state, CompatState::Init));
@@ -585,13 +584,10 @@ impl RulesetCreated {
     /// On error, returns a wrapped [`RestrictSelfError`].
     pub fn restrict_self(mut self) -> Result<RestrictionStatus, RulesetError> {
         let mut body = || -> Result<RestrictionStatus, RestrictSelfError> {
-            // 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,52 @@ 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);
+}
+
 #[test]
 fn ruleset_unsupported() {
     assert_eq!(
@@ -768,8 +810,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 +857,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 +959,7 @@ fn ignore_abi_v2_with_abi_v1() {
             .unwrap(),
         RestrictionStatus {
             ruleset: RulesetStatus::NotEnforced,
-            no_new_privs: false,
+            no_new_privs: true,
         }
     );
 }