Skip to content

fix(operators): keep the @rx prefilter from rejecting what the regex matches - #27

Merged
ggrigoras-ops merged 1 commit into
mainfrom
def-57621-rx-prefilter-fix
Oct 14, 2026
Merged

ggrigoras-ops merged 1 commit into
mainfrom
def-57621-rx-prefilter-fix

Conversation

@ggrigoras-ops

@ggrigoras-ops ggrigoras-ops commented Oct 13, 2026 •

Copy link
Copy Markdown
Collaborator

Fixes @rx prefilter false negatives in trie reconstruction. Found while checking SecRxPreFilter On against a production ruleset.

What

With SecRxPreFilter On, the prefilter rejected inputs the regex matches. trieReconstruct prepends the literal before an alternation to the strings rawExtractSuffixes took from each branch, but a branch such as 0*60; or x0*3c; has a star or a shorter literal in front of the literal that was extracted. "#"+"60;" was required although the two are not adjacent: < has no #60;.

rawExtractSuffixes is replaced by leadingLiterals, which returns only text anchored at the start of the node and stops at a star, an optional, a class or an assertion. The trie words that were correct (select|set|sleep, the SQL keyword alternations) come out the same.

Impact

An XSS rule in that ruleset starts with (?:&#0*60;|&#x0*3c;|<|&#0*60;|&#x0*3c;|<). With the prefilter on it skipped, among others, <javascript:, <onfocus = and <input', which the regex matches: a missed detection. The prefilter is off by default, so a default build is not affected; it is a bug for anyone who sets SecRxPreFilter On. The same mistake applies to a suffix behind an optional, a .*, or a nested group whose literal is not at its start (a(?:b?cd|ef) required acd, so abcd was rejected).

How it was found: a local differential test that builds every @rx pattern of that ruleset with and without the prefilter and compares the answers on inputs generated from the pattern's own syntax tree (1,393 patterns, about 1.46 million inputs, 1,295 disagreements before the fix; the 25 that were printed were all one pattern). The test is not part of this PR.

Tests

  • Rows in TestPrefilterNeverCausesFalseNegatives for the entity pattern and for each shape above. They fail on main.
  • leadingLiterals/anchored_at_start: what is and is not reported, including 0*60;, x0*3c;, ab*c, \belect.
  • TestPrefilterAgreesWithRegexOnGeneratedInputs: a property test. For a set of patterns (the entity rules, the trie shapes, SQL keyword alternations) it generates strings from the syntax tree, mostly ones the regex matches, in seven variants (alone, embedded, with newlines, upper and lower case, padded), and requires the operator with the prefilter to answer like the operator without it. It also fails if the generator produces no matching input for a pattern. Fails on main.

Verification

The same differential test on the fixed code: 1,393 distinct @rx patterns of the ruleset, 1,363 of them with a prefilter, minimum length or exact match, 300 generated inputs x 7 variants per pattern: 2,925,300 inputs compared, 2,295,485 of them matched by the regex, 0 disagreements.

Locally: gofmt, go build ./..., go vet ./..., go test ./..., go test -tags coraza.rule.rx_prefilter ./internal/operators/, go test -race ./internal/operators/, tinygo test -short -v ./internal/... with and without coraza.no_memoize, go run mage.go lint. No ADR: this is a bug fix (AGENTS.md).

Not included

Anything about the CPU cost itself (see the @pm zero-copy PR, #26), and minLen for a case-insensitive non-ASCII literal that folds to ASCII ((?i)ſ, the Kelvin sign), a similar class of problem that the ruleset checked here does not exercise.

…matches

trieReconstruct prepends the literal before an alternation to the strings that
rawExtractSuffixes took from each branch. A branch such as 0*60; or x0*3c; has a star
or a shorter literal in front of the literal that was extracted, so "#"+"60;" was
required although the two are not adjacent: "&corazawaf#60;" has no "corazawaf#60;". With
SecRxPreFilter On the Imunify360 XSS rule (&#0*60;|&#x0*3c;|<|...) skipped
inputs such as "&corazawaf#60;javascript:" and "<input'", a missed detection.
The same happened for a suffix behind an optional, a wildcard, or a nested group
whose literal is not at its start.

Replace rawExtractSuffixes with leadingLiterals, which returns only text anchored at
the start of the node and stops at a star, an optional, a class or an assertion.
The trie words that were correct (select|set|sleep and the like) are unchanged.

Add rows to the false-negative table, a unit test for leadingLiterals, and a
property test that compares the operator with and without the prefilter on strings
generated from the syntax tree of a set of patterns.
@ggrigoras-ops
ggrigoras-ops marked this pull request as ready for review October 13, 2026 11:47
@ggrigoras-ops
ggrigoras-ops merged commit a2f6202 into main Oct 14, 2026
25 checks passed
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.

2 participants