diff --git a/crates/parapet/src/collections.rs b/crates/parapet/src/collections.rs index f5ae4de..00b4d4a 100644 --- a/crates/parapet/src/collections.rs +++ b/crates/parapet/src/collections.rs @@ -241,24 +241,33 @@ impl Variables { out.retain(|value| { !excluded.iter().any(|(collection, selector)| { let prefix = collection_name(*collection); - if !value.name.starts_with(prefix) { - return false; - } - match selector { - None => true, - Some(CompiledSelector::Name(n)) => value - .name - .strip_prefix(prefix) - .and_then(|r| r.strip_prefix(':')) - .is_some_and(|member| member.eq_ignore_ascii_case(n)), - Some(CompiledSelector::Regex(re)) => value - .name - .strip_prefix(prefix) - .and_then(|r| r.strip_prefix(':')) - .is_some_and(|member| re.is_match(member)), + // Match the collection exactly, not by string prefix: a + // value is named `PREFIX` (a scalar) or `PREFIX:member` (a + // map). A bare `starts_with(prefix)` would let `!ARGS` also + // exclude `ARGS_GET:x` and `ARGS_NAMES:x`, since those names + // begin with "ARGS". + let member = match value.name.strip_prefix(prefix) { + Some("") => None, + Some(rest) => match rest.strip_prefix(':') { + Some(member) => Some(member), + // A longer collection name that merely starts with + // `prefix`, such as ARGS_GET against ARGS. + None => return false, + }, + None => return false, + }; + match (selector, member) { + // Whole-collection exclusion: every member of it. + (None, _) => true, + // A selector cannot match a scalar, which has no member. + (Some(_), None) => false, + (Some(CompiledSelector::Name(n)), Some(member)) => { + member.eq_ignore_ascii_case(n) + } + (Some(CompiledSelector::Regex(re)), Some(member)) => re.is_match(member), // An XPath exclusion cannot be evaluated without an // XML tree, and XML is never populated yet. - Some(CompiledSelector::XPath(_)) => false, + (Some(CompiledSelector::XPath(_)), _) => false, } }) }); diff --git a/crates/parapet/src/transaction.rs b/crates/parapet/src/transaction.rs index 3b9db73..e6d4ada 100644 --- a/crates/parapet/src/transaction.rs +++ b/crates/parapet/src/transaction.rs @@ -4,6 +4,8 @@ //! `process_*` call runs the rules for that phase. Evaluation stops at the //! first disruptive action, as SecLang specifies. +use std::borrow::Cow; + use crate::action::{Ctl, RuleEngineMode, SetVarOp, Transformation}; use crate::collections::{BodyError, CompiledTarget, OwnedValue, Variables}; use crate::engine::{ChainLink, CompiledRule, Disruptive, RuleSet, SetVarSpec}; @@ -540,23 +542,33 @@ impl<'r> Transaction<'r> { let mut hit: Option = None; for value in values { - // `multiMatch` tests after every transformation, not only the last, - // so a payload that is detectable at an intermediate decoding - // stage is still caught. - let mut candidates: Vec> = Vec::new(); - let mut current = value.value.to_vec(); - if multi_match { - candidates.push(current.clone()); - } - for t in transformations { - current = t.apply(¤t).into_owned(); - if multi_match { - candidates.push(current.clone()); + // The candidates to test, borrowing the resolved value where no copy + // is needed. Without transformations that is the value itself, tested + // in place. `multiMatch` also tests the value before and after each + // transformation, so a payload detectable at an intermediate decoding + // stage is still caught; without it only the fully transformed value + // is tested. + let candidates: Vec> = if multi_match { + let mut cs: Vec> = Vec::with_capacity(transformations.len() + 1); + let mut current: Cow<[u8]> = Cow::Borrowed(&value.value); + cs.push(current.clone()); + for t in transformations { + current = Cow::Owned(t.apply(¤t).into_owned()); + cs.push(current.clone()); } - } - if !multi_match { - candidates.push(current); - } + cs + } else { + let mut current: Cow<[u8]> = Cow::Borrowed(&value.value); + for t in transformations { + // A transformation that changes nothing (e.g. `t:none`, or + // `t:lowercase` on already-lowercase input) returns a borrow; + // only an actual change costs an allocation. + if let Cow::Owned(changed) = t.apply(¤t) { + current = Cow::Owned(changed); + } + } + vec![current] + }; for candidate in candidates { let result = operator.evaluate(&candidate, &self.vars, capture); @@ -566,7 +578,7 @@ impl<'r> Transaction<'r> { } hit = Some(OwnedValue { name: value.name.to_string(), - value: candidate, + value: candidate.into_owned(), }); break; } @@ -1013,6 +1025,32 @@ SecRule ARGS "@rx attack" "id:3,phase:1,pass,setvar:'tx.reached=1'" assert!(matches!(tx.process_request_headers(), Verdict::Deny { .. })); } + #[test] + fn a_noop_transformation_in_the_chain_does_not_break_matching() { + // `t:none` returns its input unchanged; the value still reaches the + // later `t:lowercase` and the anchored operator matches. + let rs = rules(r#"SecRule ARGS "@rx ^attack$" "id:1,phase:1,deny,t:none,t:lowercase""#); + let mut tx = get(&rs, "/?q=ATTACK"); + assert!(matches!(tx.process_request_headers(), Verdict::Deny { .. })); + } + + #[test] + fn a_rule_with_no_transformation_matches_the_raw_value() { + let rs = rules(r#"SecRule ARGS "@rx attack" "id:1,phase:1,deny""#); + let mut tx = get(&rs, "/?q=attack"); + assert!(matches!(tx.process_request_headers(), Verdict::Deny { .. })); + } + + #[test] + fn a_whole_collection_exclusion_does_not_catch_a_longer_collection() { + // `!ARGS` excludes the ARGS collection, not every collection whose name + // starts with "ARGS". A rule inspecting ARGS_GET with that exclusion + // must still see its members. + let rs = rules(r#"SecRule ARGS_GET|!ARGS "@rx attack" "id:1,phase:1,deny""#); + let mut tx = get(&rs, "/?q=attack"); + assert!(matches!(tx.process_request_headers(), Verdict::Deny { .. })); + } + #[test] fn a_regex_selector_narrows_a_collection() { let rs = rules(r#"SecRule REQUEST_HEADERS:/^X-/ "@rx attack" "id:1,phase:1,deny""#);