Error when simple and conditional rules conflict in RuleSet

When a RuleSet provides both a simple and conditional rule for a single
syscall, in the past extrasafe would ignore the simple rule and only use
the conditional rule.

Now extrasafe will raise the same error as when two different RuleSets
provided both simple and conditional rules.

This fixes GitHub issue #6

CHANGELOG.md

@@ -3,6 +3,9 @@ unreleased
 - reexport syscalls dependency
 - Rename `Rule` to `SeccompRule` in preparation for `LandlockRule`
     - Update relevant documentation
+- Disallow adding simple SeccompRules when the syscall is already restricted
+  with comparators, or adding rules with comparators when the syscall is
+  restricted with a simple rule. GitHub Issue #6
 
 0.1.4
 -----

src/lib.rs

@@ -121,12 +121,10 @@ impl SafetyContext {
         let base_syscalls = rules.simple_rules();
         let mut rules = rules.conditional_rules();
         for syscall in base_syscalls {
-            if !rules.contains_key(&syscall) {
-                let rule = SeccompRule::new(syscall);
-                rules.entry(syscall)
-                    .or_insert_with(Vec::new)
-                    .push(rule);
-            }
+            let rule = SeccompRule::new(syscall);
+            rules.entry(syscall)
+                .or_insert_with(Vec::new)
+                .push(rule);
         }
 
         rules.into_values().flatten()
@@ -157,22 +155,30 @@ impl SafetyContext {
 
                     let new_is_simple = new_rule.comparators.is_empty();
                     let existing_is_simple = existing_rule.comparators.is_empty();
-                    let same_syscall = new_rule.syscall == existing_rule.syscall;
 
-                    if same_syscall && new_is_simple && !existing_is_simple {
+                    // if one rule is conditional and the other is simple, let the user know there
+                    // would be a conflict and raise an error.
+                    if new_is_simple && !existing_is_simple {
                         return Err(ExtraSafeError::ConditionalNoEffectError(
                             new_rule.syscall,
                             labeled_existing_rule.0,
                             labeled_new_rule.0,
                         ));
                     }
-                    if same_syscall && !new_is_simple && existing_is_simple {
+                    else if !new_is_simple && existing_is_simple {
                         return Err(ExtraSafeError::ConditionalNoEffectError(
                             new_rule.syscall,
                             labeled_new_rule.0,
                             labeled_existing_rule.0,
                         ));
                     }
+                    // otherwise, they're either both conditional rules or both simple rules,
+                    // in which case we continue to check the existing filters, and then add the
+                    // rules to our filter as normal if all checks pass.
+                    //
+                    // In the end, the rules for a syscall must either be all simple (i.e.
+                    // duplicates from different rulesets) or all conditional (e.g. multiple rules
+                    // allowing read to be called on specific fds)
                 }
             }
 
@@ -248,7 +254,8 @@ pub enum ExtraSafeError {
     /// Error created when trying to apply filters on non-Linux operating systems. Should never
     /// occur.
     UnsupportedOSError,
-    /// Error created when a simple rule would override a conditional rule.
+    /// Error created when a simple Seccomp rule would override a conditional rule, or when trying to add a
+    /// conditional rule when there's already a simple rule with the same syscall.
     ConditionalNoEffectError(syscalls::Sysno, &'static str, &'static str),
     /// An error from the underlying seccomp library.
     SeccompError(libseccomp::error::SeccompError),

tests/bad_combination.rs

@@ -76,3 +76,81 @@ fn invalid_combination_new_conditional_different_name() {
     let err = res.unwrap_err();
     assert_eq!(err.to_string(), "A conditional rule on syscall `write` from RuleSet `SystemIO` would be overridden by a simple rule from RuleSet `JustWrite`.");
 }
+
+#[test]
+/// Test that adding a conditional and simple rule in the same RuleSet produces an error
+fn invalid_combination_read_and_stdin() {
+
+    let res = extrasafe::SafetyContext::new()
+        .enable(SystemIO::nothing()
+            .allow_read()
+            .allow_stdin()
+        );
+    assert!(res.is_err(), "Extrasafe didn't fail when adding conflicting rules");
+
+    let err = res.unwrap_err();
+    assert_eq!(err.to_string(), "A conditional rule on syscall `read` from RuleSet `SystemIO` would be overridden by a simple rule from RuleSet `SystemIO`.");
+}
+
+#[test]
+/// Test that adding duplicate simple rules in the same RuleSet doesn't produce an error
+fn not_invalid_combination_duplicate_simple() {
+
+    let res = extrasafe::SafetyContext::new()
+        .enable(SystemIO::nothing()
+            .allow_read()
+            .allow_read()
+        );
+    assert!(res.is_ok());
+
+    let res = res.unwrap().apply_to_current_thread();
+    assert!(res.is_ok());
+}
+
+#[test]
+/// Test that adding duplicate simple rules in the same RuleSet doesn't produce an error
+fn not_invalid_combination_duplicate_simple2() {
+
+    let res = extrasafe::SafetyContext::new()
+        .enable(SystemIO::nothing()
+            .allow_read()).unwrap()
+        .enable(SystemIO::nothing()
+            .allow_read()
+        );
+    assert!(res.is_ok());
+
+    let res = res.unwrap().apply_to_current_thread();
+    assert!(res.is_ok());
+}
+
+#[test]
+/// Test that adding duplicate conditional rules in the same RuleSet doesn't produce an error
+fn not_invalid_combination_duplicate_conditional() {
+
+    let res = extrasafe::SafetyContext::new()
+        .enable(SystemIO::nothing()
+            .allow_stdin()
+            .allow_stdin()
+        );
+    assert!(res.is_ok());
+
+    let res = res.unwrap().apply_to_current_thread();
+    assert!(res.is_ok());
+}
+
+#[test]
+/// Test that adding duplicate conditional rules in the same RuleSet doesn't produce an error
+fn not_invalid_combination_duplicate_conditional2() {
+
+    let res = extrasafe::SafetyContext::new()
+        .enable(SystemIO::nothing()
+            .allow_stdin()
+        ).unwrap()
+        .enable(SystemIO::nothing()
+            .allow_stdin()
+        );
+    assert!(res.is_ok());
+
+    let res = res.unwrap().apply_to_current_thread();
+    assert!(res.is_ok());
+}