Skip to content

Refuse invalid selectors and backward skipAfter at compile time - #1

Merged
ndreno merged 1 commit into
mainfrom
fix/fail-closed-selectors-and-skipafter
Sep 11, 2026
Merged

ndreno merged 1 commit into
mainfrom
fix/fail-closed-selectors-and-skipafter

Conversation

@ndreno

@ndreno ndreno commented Sep 11, 2026

Copy link
Copy Markdown
Contributor

Why

A focused review turned up two places where the engine compiled a rule set without error and then failed silently at request time, which is exactly the behaviour its compile-time refusal exists to prevent (unknown directives, unknown collections, unsupported XPath, unimplemented operators, dangling chains, and unknown markers are all refused). Both were verified with a running test, not reasoning.

Neither triggers on the OWASP Core Rule Set. This closes the gap for custom rule sets and for a CRS rule set with a typo.

1. Regex member-selectors were never validated → silent bypass

A selector like ARGS:/^user/ is compiled with the regex crate at resolve time, and a compile failure resolved to .unwrap_or(false), i.e. no members selected. check_targets validated XPath selectors but not regex ones. Demonstrated before the fix:

valid   ARGS:/^user/      compile=OK  ?user_id=attack caught=true
PCRE    ARGS:/(?=x)user/  compile=OK  ?user_id=attack caught=false   ← silent no-op
badcls  ARGS:/[a-/        compile=OK  ?user_id=attack caught=false   ← silent no-op

A selector using a construct the regex crate rejects (lookahead, invalid class, backreference) compiled fine and then silently inspected nothing — a rule turned into a bypass with no signal, while the operator-level @rx gets the full validate-and-repair path. check_targets now compiles every regex selector with the same constructor the resolver uses, and refuses the rule set if one fails.

2. Backward skipAfter → infinite loop

run_phase resumes at marker_position + 1 with no check that the marker is ahead of the rule. A marker before a rule that skips to it and keeps matching loops forever, hanging the request. Demonstrated before the fix:

compile ACCEPTED a backward skipAfter (no forward-only check)
running phase 1 ... >>> TIMED OUT: infinite loop confirmed

The marker pass only checked existence; it now also refuses a marker that is not ahead of the rule.

Not a regression on CRS

Real CRS 4.9.0 compiles to the same 590 rules with the same 4 detectSQLi/detectXSS refusals after the change — its selectors are all trivial (__utm, _pk_ref, rfi_parameter_.*) and its skips are all forward. The FTW conformance job will confirm detection is unchanged.

Tests

Four added, all passing (169 total, was 165):

  • an_invalid_selector_regex_is_a_compile_error
  • a_valid_selector_regex_compiles (the CRS !REQUEST_COOKIES:/__utm/ shape)
  • a_backward_skip_after_is_a_compile_error
  • a_forward_skip_after_still_compiles

Deliberately out of scope

Regex selectors are still recompiled per value at resolve time (162 CRS rules recompile __utm/_pk_ref per cookie per request). Caching the compiled form is a real performance win but touches the serialized rule AST, so it belongs in its own change. The evaluate-step per-value copy and the loose whole-collection exclusion prefix are two more follow-ups noted in the review.

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.
@ndreno
ndreno merged commit 03cae9d into main Sep 11, 2026
8 checks passed
@ndreno
ndreno deleted the fix/fail-closed-selectors-and-skipafter branch September 11, 2026 07:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant