diff --git a/CHANGELOG.md b/CHANGELOG.md index d8d7007..5f21ff1 100644 --- a/CHANGELOG.md +++ b/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 ----- diff --git a/src/lib.rs b/src/lib.rs index 759e178..fb0750c 100644 --- a/src/lib.rs +++ b/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), diff --git a/tests/bad_combination.rs b/tests/bad_combination.rs index 9bda19d..d323b5f 100644 --- a/tests/bad_combination.rs +++ b/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()); +}