From 16f8e07aecfc071469ed18f7131731aa456dc55a Mon Sep 17 00:00:00 2001 From: Nicolas Dreno Date: Fri, 11 Sep 2026 09:32:37 +0200 Subject: [PATCH] Refuse invalid selectors and backward skipAfter at compile time Two constructs compiled without error and then failed silently at request time, which is exactly what this engine's compile-time refusal is meant to prevent. A regex member-selector (`ARGS:/^user/`) is compiled with the regex crate at resolve time, and a compile failure resolved to no members. Only XPath selectors were validated at compile time, so a selector using a construct the regex crate rejects (a lookahead, an invalid class, a backreference) compiled fine and then silently inspected nothing: a rule turned into a bypass with no signal. check_targets now compiles every regex selector with the same constructor the resolver uses and refuses the rule set if one does not compile, alongside the existing XPath check. A `skipAfter:` naming a marker at or before the rule sent evaluation backward: the rule re-ran and, if it kept matching, looped forever, hanging the request. The marker pass only checked that the marker existed. It now also refuses a marker that is not ahead of the rule. Neither triggers on the OWASP Core Rule Set, which uses only trivial selectors and forward skips: CRS 4.9.0 still compiles to 590 rules with the same 4 detectSQLi/detectXSS refusals. These close the gap for custom and future rule sets, and for a rule set with a typo that would otherwise ship a silent hole or a hang. Selector regexes are still recompiled per value at resolve time; caching the compiled form is a separate performance change, left for a follow-up because it touches the serialized rule AST. --- crates/parapet/src/engine.rs | 90 +++++++++++++++++++++++++------ crates/parapet/src/transaction.rs | 54 +++++++++++++++++++ 2 files changed, 127 insertions(+), 17 deletions(-) diff --git a/crates/parapet/src/engine.rs b/crates/parapet/src/engine.rs index f63e88d..8fd8999 100644 --- a/crates/parapet/src/engine.rs +++ b/crates/parapet/src/engine.rs @@ -158,6 +158,37 @@ pub enum CompileError { /// The marker name. marker: String, }, + /// A `skipAfter:` names a marker at or before the rule itself. + /// + /// Evaluation resumes just after the marker, so a marker that is not ahead + /// of the rule sends evaluation backward. A rule that skips backward and + /// still matches re-runs forever, hanging the request. Refused rather than + /// allowed: a jump that loops is not a jump. + #[error("rule {id} (line {line}) skips to {marker:?}, which is not ahead of it; skipAfter must jump forward")] + BackwardSkip { + /// The rule id, or 0 when it has none. + id: u32, + /// Source line. + line: usize, + /// The marker name. + marker: String, + }, + /// A regex member-selector the engine cannot compile. + /// + /// Refused rather than resolved to nothing: an uncompilable selector + /// silently selects no members, which turns the rule into a bypass with no + /// signal, exactly as an unsupported XPath would. + #[error("rule {id} (line {line}) selects members with /{selector}/, which is not a valid regex: {error}")] + InvalidSelector { + /// The rule id, or 0 when it has none. + id: u32, + /// Source line. + line: usize, + /// The selector pattern as written. + selector: String, + /// The regex parse error. + error: String, + }, } impl RuleSet { @@ -259,24 +290,34 @@ impl RuleSet { }); } - // Resolve every skipAfter now, so a typo is a compile error rather - // than a jump that silently does nothing at request time. - let mut unknown_markers: Vec = Vec::new(); - for entry in &set.entries { + // Resolve every skipAfter now, so a typo, or a jump that would loop, is + // a compile error rather than a surprise at request time. + let mut marker_errors: Vec = Vec::new(); + for (index, entry) in set.entries.iter().enumerate() { if let Entry::Rule(rule) = entry { if let Some(marker) = &rule.skip_after { - if !set.marker_index.contains_key(marker) { - unknown_markers.push(CompileError::UnknownMarker { + match set.marker_index.get(marker) { + None => marker_errors.push(CompileError::UnknownMarker { id: rule.id.unwrap_or(0), line: rule.line, marker: marker.clone(), - }); + }), + // Evaluation resumes at the marker's slot + 1, so a + // marker that is not ahead of the rule loops. + Some(&position) if position <= index => { + marker_errors.push(CompileError::BackwardSkip { + id: rule.id.unwrap_or(0), + line: rule.line, + marker: marker.clone(), + }) + } + Some(_) => {} } } } } - errors.extend(unknown_markers); + errors.extend(marker_errors); (set, errors) } @@ -386,6 +427,11 @@ fn compile_rule( } /// Reject targets Parapet cannot resolve, before they become silent no-ops. +/// +/// A selector that cannot be evaluated selects nothing, and a rule that +/// inspects nothing cannot fire. Both an unsupported XPath and an uncompilable +/// regex selector are refused here so that failure surfaces at compile time +/// rather than as a silent bypass at request time. fn check_targets( targets: &[Target], id: impl Into>, @@ -393,18 +439,28 @@ fn check_targets( ) -> Result<(), CompileError> { let id = id.into(); for target in targets { - if target.collection != Collection::Xml { - continue; - } - if let Some(Selector::XPath(expression)) = &target.selector { - if !crate::xml::xpath_is_supported(expression) { - return Err(CompileError::UnsupportedXPath { + match &target.selector { + Some(Selector::XPath(expression)) if target.collection == Collection::Xml => { + if !crate::xml::xpath_is_supported(expression) { + return Err(CompileError::UnsupportedXPath { + id: id.unwrap_or(0), + line, + expression: expression.clone(), + supported: crate::xml::SUPPORTED_XPATH, + }); + } + } + // The same engine compiles this at resolve time. Validating it here + // with the same constructor guarantees that never fails silently. + Some(Selector::Regex(pattern)) => { + regex::Regex::new(pattern).map_err(|e| CompileError::InvalidSelector { id: id.unwrap_or(0), line, - expression: expression.clone(), - supported: crate::xml::SUPPORTED_XPATH, - }); + selector: pattern.clone(), + error: e.to_string(), + })?; } + _ => {} } } Ok(()) diff --git a/crates/parapet/src/transaction.rs b/crates/parapet/src/transaction.rs index aed7e04..096e385 100644 --- a/crates/parapet/src/transaction.rs +++ b/crates/parapet/src/transaction.rs @@ -907,6 +907,60 @@ SecRule ARGS "@rx attack" "id:3,phase:1,pass,setvar:'tx.reached=1'" )); } + #[test] + fn a_backward_skip_after_is_a_compile_error() { + // A marker before a rule that skips to it: evaluation would resume just + // after the marker, re-run the rule, and loop forever. It must be + // refused at compile time. + let directives = parse( + "SecMarker LOOP\nSecAction \"id:1,phase:1,pass,skipAfter:LOOP\"\n", + "test.conf", + ) + .unwrap(); + let err = RuleSet::compile(&directives, &NoDataLoader).unwrap_err(); + assert!(matches!( + err, + crate::engine::CompileError::BackwardSkip { .. } + )); + } + + #[test] + fn a_forward_skip_after_still_compiles() { + let directives = parse( + "SecAction \"id:1,phase:1,pass,skipAfter:AHEAD\"\nSecMarker AHEAD\n", + "test.conf", + ) + .unwrap(); + assert!(RuleSet::compile(&directives, &NoDataLoader).is_ok()); + } + + #[test] + fn an_invalid_selector_regex_is_a_compile_error() { + // A member selector the regex engine cannot compile would select + // nothing at request time, turning the rule into a silent bypass. It is + // refused at compile time instead, exactly as an unsupported XPath is. + let directives = parse( + r#"SecRule ARGS:/(?=x)user/ "@rx attack" "id:1,phase:2,deny""#, + "test.conf", + ) + .unwrap(); + let err = RuleSet::compile(&directives, &NoDataLoader).unwrap_err(); + assert!(matches!( + err, + crate::engine::CompileError::InvalidSelector { .. } + )); + } + + #[test] + fn a_valid_selector_regex_compiles() { + let directives = parse( + r#"SecRule REQUEST_COOKIES|!REQUEST_COOKIES:/__utm/ "@rx attack" "id:1,phase:2,deny""#, + "test.conf", + ) + .unwrap(); + assert!(RuleSet::compile(&directives, &NoDataLoader).is_ok()); + } + #[test] fn a_dangling_chain_is_a_compile_error() { let directives = parse(