From 0ce156fb899b046fb171182b12f8a85120fe0d5b Mon Sep 17 00:00:00 2001 From: Harry Stern Date: Thu, 31 Aug 2023 19:47:25 -0400 Subject: [PATCH] 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 ++ src/lib.rs | 27 ++++++++------ tests/bad_combination.rs | 78 ++++++++++++++++++++++++++++++++++++++++ 3 files changed, 98 insertions(+), 10 deletions(-) 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()); +}