ruleset: Fix compatibility inconsistency

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 <[email protected]>

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

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,
             },
         }

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
@@ -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,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,
         }
     );
 }