diff --git a/src/backend/filter.rs b/src/backend/filter.rs index e7757a0ae..ffc06273d 100644 --- a/src/backend/filter.rs +++ b/src/backend/filter.rs @@ -129,9 +129,11 @@ impl SeccompFilter { let chain: Vec<_> = chain .into_iter() .map(|rule| { + // A rule may override the filter's match_action with its own action. + let action = rule.action().unwrap_or_else(|| match_action.clone()); let mut bpf: BpfProgram = rule.into(); - // Last statement is the on-match action of the filter. - bpf.push(bpf_stmt(BPF_RET | BPF_K, u32::from(match_action.clone()))); + // Last statement is the on-match action for this rule. + bpf.push(bpf_stmt(BPF_RET | BPF_K, u32::from(action))); bpf }) .collect(); @@ -457,4 +459,125 @@ mod tests { let bpfprog: BpfProgram = filter.try_into().unwrap(); assert_eq!(bpfprog, instructions); } + + #[test] + fn test_per_rule_action_overrides_match_action() { + // syscall 42 takes a per-rule action; syscall 7 falls back to match_action. + let rules: BTreeMap> = [ + (42, vec![SeccompRule::always(SeccompAction::Trap)]), + (7, Vec::new()), + ] + .into_iter() + .collect(); + + let filter = SeccompFilter::new( + rules, + SeccompAction::Allow, + SeccompAction::Trace(0), + ARCH.try_into().unwrap(), + ) + .unwrap(); + + let prog: BpfProgram = filter.try_into().unwrap(); + + let trap = u32::from(SeccompAction::Trap); + let trace = u32::from(SeccompAction::Trace(0)); + let allow = u32::from(SeccompAction::Allow); + + let rets: Vec = prog + .iter() + .filter(|f| f.code == (BPF_RET | BPF_K)) + .map(|f| f.k) + .collect(); + + assert!(rets.contains(&trap), "expected Trap, got {rets:?}"); + assert!(rets.contains(&trace), "expected Trace, got {rets:?}"); + assert!(rets.contains(&allow), "expected Allow, got {rets:?}"); + } + + #[test] + fn test_first_matching_rule_wins_its_action() { + let rules: BTreeMap> = [( + 42, + vec![ + SeccompRule::always(SeccompAction::Trap), + SeccompRule::always(SeccompAction::Log), + ], + )] + .into_iter() + .collect(); + + let filter = SeccompFilter::new( + rules, + SeccompAction::Allow, + SeccompAction::Trace(0), + ARCH.try_into().unwrap(), + ) + .unwrap(); + + let prog: BpfProgram = filter.try_into().unwrap(); + let rets: Vec = prog + .iter() + .filter(|f| f.code == (BPF_RET | BPF_K)) + .map(|f| f.k) + .collect(); + + assert!(rets.contains(&u32::from(SeccompAction::Trap))); + assert!(rets.contains(&u32::from(SeccompAction::Log))); + // BPF text can't show runtime selection, so assert RET ordering instead. + let trap_pos = rets + .iter() + .position(|&k| k == u32::from(SeccompAction::Trap)) + .unwrap(); + let log_pos = rets + .iter() + .position(|&k| k == u32::from(SeccompAction::Log)) + .unwrap(); + assert!( + trap_pos < log_pos, + "first rule's action must precede the second's" + ); + } + + #[test] + fn test_conditional_rule_action_emitted() { + // new_with_action: conditions + per-rule action. The rule's Trap is + // emitted; the overridden match_action (Log) is absent. + let rules: BTreeMap> = [( + 42, + vec![SeccompRule::new_with_action( + vec![Cond::new(0, ArgLen::Dword, Eq, 7).unwrap()], + SeccompAction::Trap, + ) + .unwrap()], + )] + .into_iter() + .collect(); + + let filter = SeccompFilter::new( + rules, + SeccompAction::Allow, + SeccompAction::Log, + ARCH.try_into().unwrap(), + ) + .unwrap(); + + let prog: BpfProgram = filter.try_into().unwrap(); + + assert!( + prog.iter().any(|f| f.code == (BPF_LD | BPF_W | BPF_ABS)), + "expected an argument load" + ); + assert!( + prog.iter() + .any(|f| { f.code == (BPF_RET | BPF_K) && f.k == u32::from(SeccompAction::Trap) }), + "expected a Trap return" + ); + assert!( + !prog + .iter() + .any(|f| { f.code == (BPF_RET | BPF_K) && f.k == u32::from(SeccompAction::Log) }), + "match_action must not be emitted when overridden" + ); + } } diff --git a/src/backend/rule.rs b/src/backend/rule.rs index b87e62a0d..21d72836a 100644 --- a/src/backend/rule.rs +++ b/src/backend/rule.rs @@ -1,19 +1,23 @@ // Copyright 2021 Amazon.com, Inc. or its affiliates. All Rights Reserved. // SPDX-License-Identifier: Apache-2.0 OR BSD-3-Clause -use crate::backend::{bpf::*, condition::SeccompCondition, Error, Result}; +use crate::backend::{bpf::*, condition::SeccompCondition, Error, Result, SeccompAction}; /// Rule that a filter attempts to match for a syscall. /// -/// If all conditions match then rule gets matched. -/// A syscall can have many rules associated. If either of them matches, the `match_action` of the -/// [`SeccompFilter`] is triggered. +/// If all conditions match then the rule is matched. A syscall can have many +/// rules associated; the first match wins. A matched rule with its own +/// [`action`](Self::new_with_action) takes that action, otherwise the +/// `match_action` of the [`SeccompFilter`] is triggered. /// /// [`SeccompFilter`]: struct.SeccompFilter.html #[derive(Clone, Debug, PartialEq, Eq)] pub struct SeccompRule { /// Conditions of rule that need to match in order for the rule to get matched. conditions: Vec, + /// Action to take when this rule matches. `None` falls back to the + /// [`SeccompFilter`]'s `match_action`. + action: Option, } impl SeccompRule { @@ -21,6 +25,9 @@ impl SeccompRule { /// of argument values, map the syscall number to an empty vector of rules when constructing /// the [`SeccompFilter`](super::SeccompFilter) instead. /// + /// On match this rule triggers the filter's `match_action`; for a per-rule + /// action see [`new_with_action`](Self::new_with_action). + /// /// # Arguments /// /// * `conditions` - Vector of [`SeccompCondition`]s that the syscall must match. @@ -39,17 +46,51 @@ impl SeccompRule { /// /// [`SeccompCondition`]: struct.SeccompCondition.html pub fn new(conditions: Vec) -> Result { - let instance = Self { conditions }; + let instance = Self { + conditions, + action: None, + }; instance.validate()?; Ok(instance) } + /// Creates a rule that takes `action` on match, overriding the filter's + /// `match_action`. Requires at least one condition; for an unconditional + /// per-syscall action use [`always`](Self::always). + pub fn new_with_action( + conditions: Vec, + action: SeccompAction, + ) -> Result { + if conditions.is_empty() { + return Err(Error::EmptyRule); + } + let instance = Self { + conditions, + action: Some(action), + }; + instance.validate()?; + + Ok(instance) + } + + /// An unconditional rule: matches the syscall regardless of arguments and + /// takes `action`. + pub fn always(action: SeccompAction) -> Self { + SeccompRule { + conditions: Vec::new(), + action: Some(action), + } + } + + pub(crate) fn action(&self) -> Option { + self.action.clone() + } + /// Performs semantic checks on the SeccompRule. fn validate(&self) -> Result<()> { - // Rules with no conditions are not allowed. Syscalls mappings to empty rule vectors are to - // be used instead, for matching only on the syscall number. - if self.conditions.is_empty() { + // A condition-less rule is only valid via `always` (which sets an action). + if self.conditions.is_empty() && self.action.is_none() { return Err(Error::EmptyRule); } @@ -148,7 +189,7 @@ mod tests { use super::SeccompRule; use crate::backend::bpf::*; use crate::backend::{ - Error, SeccompCmpArgLen as ArgLen, SeccompCmpOp::*, SeccompCondition as Cond, + Error, SeccompAction, SeccompCmpArgLen as ArgLen, SeccompCmpOp::*, SeccompCondition as Cond, }; #[test] @@ -156,6 +197,24 @@ mod tests { assert_eq!(SeccompRule::new(vec![]).unwrap_err(), Error::EmptyRule); } + #[test] + fn test_new_with_action_requires_conditions() { + assert_eq!( + SeccompRule::new_with_action(vec![], SeccompAction::Trap).unwrap_err(), + Error::EmptyRule + ); + } + + #[test] + fn test_new_with_action_and_always_carry_action() { + let cond = Cond::new(0, ArgLen::Dword, Eq, 1).unwrap(); + let rule = SeccompRule::new_with_action(vec![cond], SeccompAction::Trap).unwrap(); + assert_eq!(rule.action(), Some(SeccompAction::Trap)); + + let always = SeccompRule::always(SeccompAction::Trap); + assert_eq!(always.action(), Some(SeccompAction::Trap)); + } + // Checks that rule gets translated correctly into BPF statements. #[test] fn test_rule_bpf_output() { diff --git a/tests/integration_tests.rs b/tests/integration_tests.rs index cb6c1f350..b2588ebb4 100644 --- a/tests/integration_tests.rs +++ b/tests/integration_tests.rs @@ -789,3 +789,95 @@ fn test_filter_apply() { .join() .unwrap(); } + +#[test] +fn test_per_rule_action_runtime() { + // Per-rule Errno on getpid; everything else allowed via mismatch_action. + // Do not map unrelated syscalls to empty rule chains: empty chain ⇒ + // match_action (here Trap), which would SIGSYS on thread teardown. + // match_action is only a distinct dummy for SeccompFilter::validate. + let rule_map: BTreeMap> = [( + libc::SYS_getpid, + vec![SeccompRule::always(SeccompAction::Errno( + libc::EPERM as u32, + ))], + )] + .into_iter() + .collect(); + + let filter = SeccompFilter::new( + rule_map, + SeccompAction::Allow, + SeccompAction::Trap, + ARCH.try_into().unwrap(), + ) + .unwrap(); + let prog: BpfProgram = filter.try_into().unwrap(); + + thread::spawn(move || { + apply_filter(&prog).unwrap(); + + // SAFETY: clear errno then issue getpid via syscall(2) so errno is set. + unsafe { + *libc::__errno_location() = 0; + } + let rc = unsafe { libc::syscall(libc::SYS_getpid) }; + let errno = std::io::Error::last_os_error().raw_os_error().unwrap(); + assert_eq!(rc, -1, "getpid should be blocked by per-rule Errno"); + assert_eq!(errno, libc::EPERM); + + // Unrelated syscall still allowed (mismatch_action). + let tid = unsafe { libc::syscall(libc::SYS_gettid) }; + assert!(tid > 0, "gettid should be allowed, got {tid}"); + }) + .join() + .unwrap(); +} + +#[test] +fn test_per_rule_conditional_action_runtime() { + // Deny write(fd == 1) with a per-rule Errno; other fds fall through to + // mismatch Allow. match_action is an unused distinct dummy. + let rule_map: BTreeMap> = [( + libc::SYS_write, + vec![SeccompRule::new_with_action( + vec![Cond::new(0, Dword, Eq, 1).unwrap()], + SeccompAction::Errno(libc::EPERM as u32), + ) + .unwrap()], + )] + .into_iter() + .collect(); + + let filter = SeccompFilter::new( + rule_map, + SeccompAction::Allow, + SeccompAction::Trap, + ARCH.try_into().unwrap(), + ) + .unwrap(); + let prog: BpfProgram = filter.try_into().unwrap(); + + thread::spawn(move || { + apply_filter(&prog).unwrap(); + + let buf = b"x"; + // SAFETY: write(2) with a valid buffer; we only check the return/errno. + unsafe { + *libc::__errno_location() = 0; + } + let rc = unsafe { libc::syscall(libc::SYS_write, 1i32, buf.as_ptr(), buf.len()) }; + let errno = std::io::Error::last_os_error().raw_os_error().unwrap(); + assert_eq!(rc, -1, "write(1, …) should hit per-rule Errno"); + assert_eq!(errno, libc::EPERM); + + // fd 2 does not match the condition → mismatch Allow. + unsafe { + *libc::__errno_location() = 0; + } + let rc = unsafe { libc::syscall(libc::SYS_write, 2i32, buf.as_ptr(), buf.len()) }; + assert_eq!(rc, 1, "write(2, …) should be allowed"); + }) + .join() + .unwrap(); +}