From da267d3b835ce9c34ee0dd2487c53d94109117af Mon Sep 17 00:00:00 2001 From: Gabriel Grigoras Date: Tue, 13 Oct 2026 13:32:39 +0300 Subject: [PATCH] fix(operators): keep the @rx prefilter from rejecting what the regex 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: "<" has no "#60;". With SecRxPreFilter On the Imunify360 XSS rule (�*60;|�*3c;|<|...) skipped inputs such as "<javascript:" and "&#x3c;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. --- internal/operators/rxprefilter.go | 151 +++++++------- internal/operators/rxprefilter_gen_test.go | 226 +++++++++++++++++++++ internal/operators/rxprefilter_test.go | 85 +++++++- 3 files changed, 380 insertions(+), 82 deletions(-) create mode 100644 internal/operators/rxprefilter_gen_test.go diff --git a/internal/operators/rxprefilter.go b/internal/operators/rxprefilter.go index 6c410a594..cbcd23453 100644 --- a/internal/operators/rxprefilter.go +++ b/internal/operators/rxprefilter.go @@ -529,12 +529,13 @@ func extractLiterals(re *syntax.Regexp, ci bool) interface{} { // select|sleep|substr → s(?:elect|leep|ubstr) // AST: OpConcat([OpLiteral("s"), OpAlternate([...])]) // -// It prepends the raw prefix literal to every element returned by -// rawExtractSuffixes, recovering the full literal words. The result is returned -// as anyRequired so the caller knows "at least one of these must be present". +// It prepends the raw prefix literal to every string that leadingLiterals says +// the alternation must begin with, recovering the full literal words. The result +// is returned as anyRequired so the caller knows "at least one of these must be +// present". // -// Handles arbitrary nesting depth: rawExtractSuffixes recurses back into -// extractLiterals → trieReconstruct for inner OpConcat nodes, so patterns like +// Handles arbitrary nesting depth: leadingLiterals recurses through nested +// alternations and concatenations, so patterns like // // select|set|sleep → s(?:e(?:lect|t)|leep) // @@ -555,100 +556,104 @@ func trieReconstruct(concat *syntax.Regexp, ci bool) anyRequired { return nil } - // Collect suffix candidates from remaining children using the permissive - // extractor that allows short literals (they will be combined with prefix). - var suffixes []string - for _, sub := range concat.Sub[1:] { - lits := rawExtractSuffixes(sub, ci) - if lits == nil { - continue - } - suffixes = append(suffixes, lits...) - } + // The second child directly follows the prefix, so every match of the + // concat contains prefix+s for one of the strings s that the child must + // begin with. Only text at the very start of the child can be glued to the + // prefix: a literal further in (after a star, an optional group, a class...) + // is not adjacent to it and would make a phantom string, e.g. the "60;" of + // �*60; must not become "#60;" because the input "<" has no such + // substring. + suffixes := leadingLiterals(concat.Sub[1], ci) if len(suffixes) == 0 { return nil } result := make(anyRequired, 0, len(suffixes)) for _, s := range suffixes { - if full := prefix + s; len(full) >= 2 { - result = append(result, full) + full := prefix + s + if len(full) < 2 { + // Cannot happen (both parts are non-empty), but never drop an + // element of an anyRequired set: that would reject its matches. + return nil } - } - if len(result) == 0 { - return nil + result = append(result, full) } return result } -// rawExtractSuffixes extracts literal candidates from a regex subtree for use -// as trie-reconstruction suffixes. Unlike extractLiterals it permits short -// literals (< 2 bytes) because they will be combined with a prefix by the -// caller, producing a longer and more selective string. +// maxLeadingLiterals bounds the number of strings leadingLiterals builds. +const maxLeadingLiterals = anyRequiredMaxN + +// leadingLiterals returns a set of non-empty strings such that every match of +// re begins with at least one of them, or nil when it cannot say. Unlike +// extractLiterals it permits short literals (< 2 bytes), because the caller +// (trieReconstruct) combines them with a prefix into a longer string. // -// Returns nil if any branch of an alternation has no extractable literal (same -// semantics as extractLiterals for OpAlternate: we can't safely omit a branch). -func rawExtractSuffixes(re *syntax.Regexp, ci bool) []string { +// The strings are anchored at the start of the match. That is what makes it safe +// to glue them to a literal that directly precedes re: nothing may be skipped +// between the two. A node that can consume other text first (a star, an optional +// or class, a zero-width assertion) stops the walk, and so does anything that +// is not a fixed string, after contributing what it knows its match starts with. +// +// An alternation needs a leading set from every branch; one branch without it +// makes the whole alternation nil, as in extractLiterals, because that branch +// could match without any of the strings. +func leadingLiterals(re *syntax.Regexp, ci bool) []string { switch re.Op { case syntax.OpLiteral: - s := rawLiteral(re, ci) - if s == "" { - return nil + if s := rawLiteral(re, ci); s != "" { + return []string{s} } - return []string{s} + return nil + + case syntax.OpCapture, syntax.OpPlus: + return leadingLiterals(re.Sub[0], ci) + + case syntax.OpRepeat: + if re.Min >= 1 { + return leadingLiterals(re.Sub[0], ci) + } + return nil case syntax.OpAlternate: - var result []string + var out []string for _, sub := range re.Sub { - lits := rawExtractSuffixes(sub, ci) + lits := leadingLiterals(sub, ci) if lits == nil { - return nil // one branch has no extractable suffix → bail + return nil + } + out = append(out, lits...) + if len(out) > maxLeadingLiterals { + return nil } - result = append(result, lits...) } - return result + return out case syntax.OpConcat: - // Try the full extractLiterals pipeline first (handles deeper nesting - // through the trieReconstruct fallback it already calls). - lits := extractLiterals(re, ci) - if lits != nil { - switch v := lits.(type) { - case allRequired: - // Only safe to return a single trie suffix when the concat - // collapses to exactly one contiguous literal. Multiple - // allRequired elements mean there are wildcards between them - // (e.g. "elect.*from" → allRequired{"elect","from"}). Joining - // them would produce "electfrom" — a phantom string that never - // appears contiguously in a real input — causing false negatives - // on valid matches like "select x from". Return nil here so the - // caller falls back to the safer anyRequired propagation instead. - if len(v) == 1 { - return []string{v[0]} - } - return nil - case anyRequired: - return []string(v) - case combinedRequired: - // For trie-reconstruction we need a suffix that is *always* present - // when this sub-concat fires. The .all elements are guaranteed; - // .any elements are only conditionally present (one of them must be - // present, but not a specific one). Returning a .any element would - // let the outer prefix combine with a wrong suffix (e.g. "s"+"execute" - // instead of "s"+"p_"+"execute" → "sp_execute"), producing a phantom - // literal that never appears contiguously in real input. - // Return the single longest .all element as the guaranteed suffix. - rep := longest([]string(v.all)) - if rep == "" { - return nil + // Start from the empty string and extend it child by child. A fixed + // literal lets the walk continue to the next child; any other child + // ends it after contributing its own leading strings. + acc := []string{""} + for _, sub := range re.Sub { + lits := leadingLiterals(sub, ci) + if lits == nil || len(acc)*len(lits) > maxLeadingLiterals { + break + } + next := make([]string, 0, len(acc)*len(lits)) + for _, a := range acc { + for _, l := range lits { + next = append(next, a+l) } - return []string{rep} + } + acc = next + if sub.Op != syntax.OpLiteral { + break } } - return nil - - case syntax.OpCapture: - return rawExtractSuffixes(re.Sub[0], ci) + if len(acc) == 1 && acc[0] == "" { + return nil + } + return acc default: return nil diff --git a/internal/operators/rxprefilter_gen_test.go b/internal/operators/rxprefilter_gen_test.go new file mode 100644 index 000000000..d8f3fad00 --- /dev/null +++ b/internal/operators/rxprefilter_gen_test.go @@ -0,0 +1,226 @@ +// Copyright 2026 CloudLinux +// SPDX-License-Identifier: Apache-2.0 + +//go:build !tinygo + +// rxprefilter_gen_test.go: a property test for the @rx prefilter. For a set of +// patterns it generates inputs from the pattern's own syntax tree, so that most +// of them are strings the regular expression matches, and requires the operator +// with the prefilter to give the same answer as the operator without it. A +// wrong prefilter rejects exactly such inputs, which is a missed attack. + +package operators + +import ( + "math/rand" + "regexp/syntax" + "strings" + "testing" + "unicode" + + "github.com/corazawaf/coraza/v3/experimental/plugins/plugintypes" + "github.com/corazawaf/coraza/v3/internal/corazawaf" +) + +// regexGen writes strings that a regular expression is likely to match. +type regexGen struct { + rnd *rand.Rand + sb strings.Builder +} + +const regexGenMaxLen = 3000 + +func newRegexGen(seed int64) *regexGen { + return ®exGen{rnd: rand.New(rand.NewSource(seed))} //nolint:gosec // reproducible test input, not security +} + +func (g *regexGen) class(re *syntax.Regexp) { + rs := re.Rune + // Prefer printable ASCII ranges of the class, sometimes take any. + type rng struct{ lo, hi rune } + var all, ascii []rng + for i := 0; i+1 < len(rs); i += 2 { + r := rng{rs[i], rs[i+1]} + all = append(all, r) + lo, hi := max(r.lo, 0x20), min(r.hi, 0x7e) + if lo <= hi { + ascii = append(ascii, rng{lo, hi}) + } + } + pool := all + if len(ascii) > 0 && g.rnd.Intn(100) < 85 { + pool = ascii + } + if len(pool) == 0 { + return + } + r := pool[g.rnd.Intn(len(pool))] + g.sb.WriteRune(r.lo + rune(g.rnd.Int63n(int64(r.hi-r.lo)+1))) +} + +func (g *regexGen) gen(re *syntax.Regexp) { + if g.sb.Len() > regexGenMaxLen { + return + } + switch re.Op { + case syntax.OpLiteral: + for _, r := range re.Rune { + if re.Flags&syntax.FoldCase != 0 && g.rnd.Intn(2) == 0 { + // Walk the case-folding orbit a random number of steps. + for n := g.rnd.Intn(3); n > 0; n-- { + r = unicode.SimpleFold(r) + } + } + g.sb.WriteRune(r) + } + case syntax.OpCharClass: + g.class(re) + case syntax.OpAnyCharNotNL: + g.sb.WriteByte(byte(0x20 + g.rnd.Intn(0x5f))) + case syntax.OpAnyChar: + if g.rnd.Intn(20) == 0 { + g.sb.WriteByte('\n') + } else { + g.sb.WriteByte(byte(0x20 + g.rnd.Intn(0x5f))) + } + case syntax.OpCapture: + g.gen(re.Sub[0]) + case syntax.OpStar: + g.repeat(re.Sub[0], g.rnd.Intn(3)) + case syntax.OpPlus: + g.repeat(re.Sub[0], 1+g.rnd.Intn(3)) + case syntax.OpQuest: + g.repeat(re.Sub[0], g.rnd.Intn(2)) + case syntax.OpRepeat: + hi := re.Max + if hi < 0 || hi > re.Min+3 { + hi = re.Min + 3 + } + g.repeat(re.Sub[0], re.Min+g.rnd.Intn(hi-re.Min+1)) + case syntax.OpConcat: + for _, s := range re.Sub { + g.gen(s) + } + case syntax.OpAlternate: + g.gen(re.Sub[g.rnd.Intn(len(re.Sub))]) + default: + // Empty match and the zero-width assertions write nothing; OpNoMatch cannot + // be generated, and an input the regex rejects is still a fair input. + } +} + +func (g *regexGen) repeat(re *syntax.Regexp, n int) { + for i := 0; i < n && g.sb.Len() <= regexGenMaxLen; i++ { + g.gen(re) + } +} + +// regexGenVariants returns s in the shapes a request value takes: on its own, +// embedded in other text, with a newline around it, in other cases and padded. +func regexGenVariants(s string) []string { + return []string{ + s, + "xx " + s + " yy", + s + "\n", + "\n" + s, + strings.ToUpper(s), + strings.ToLower(s), + strings.Repeat("a", 64) + s + strings.Repeat("a", 64), + } +} + +// prefilterDisagreement compares the @rx operator with and without the prefilter +// on inputs generated from pattern. It returns how many inputs the regular +// expression matched, how many were compared, and the first input on which the +// two operators differ ("" if none). +func prefilterDisagreement(pattern string, samples int, tx plugintypes.TransactionState) (matched, compared, disagreements int, first string, err error) { + off, err := newRX(plugintypes.OperatorOptions{Arguments: pattern, RxPreFilterEnabled: false}) + if err != nil { + return 0, 0, 0, "", err + } + on, err := newRX(plugintypes.OperatorOptions{Arguments: pattern, RxPreFilterEnabled: true}) + if err != nil { + return 0, 0, 0, "", err + } + + // The tree is generated from the same expression newRX compiles. + data := "(?s)" + pattern + if !shouldNotUseMultilineRegexesOperatorByDefault { + data = "(?sm)" + pattern + } + parsed, err := syntax.Parse(data, syntax.Perl) + if err != nil { + return 0, 0, 0, "", err + } + // A fixed seed per pattern keeps a failure reproducible. + g := newRegexGen(int64(len(pattern))*7919 + int64(pattern[0])) + for i := 0; i < samples; i++ { + g.sb.Reset() + g.gen(parsed) + for _, v := range regexGenVariants(g.sb.String()) { + want := off.Evaluate(tx, v) + got := on.Evaluate(tx, v) + compared++ + if want { + matched++ + } + if got != want { + if disagreements == 0 { + first = v + } + disagreements++ + } + } + } + return matched, compared, disagreements, first, nil +} + +// TestPrefilterAgreesWithRegexOnGeneratedInputs: the prefilter may only ever +// skip the regular expression for an input it cannot match. +func TestPrefilterAgreesWithRegexOnGeneratedInputs(t *testing.T) { + patterns := []string{ + // Entities as the XSS rules of the Imunify360 ruleset write them: the + // parser factors the common "&" and "#", leaving a star in front of a + // literal inside a trie branch. + `(?:�*60;|�*3c;|<|&#0*60;|&#x0*3c;|&lt;)(?:script|img|svg|javascript:)`, + `(?i)(?:�*60;|�*3c;|<|&#0*60;|&#x0*3c;|&lt;)(?:(?:script|img|svg)[\s/>=\x00-\x20&"']|[a-z][a-z0-9-]{0,30}[\s/][^>]{0,300}\bon(?:error|load)\s*=|javascript:)`, + `�*60;|�*3c;`, + + // A suffix in a trie branch that is not at the branch's start. + `s(?:x*elect|leep)`, + `s(?:e+lect|leep)`, + `a(?:b?cd|ef)`, + `a(?:b.*cde|fg)`, + `a(?:x*bcd(?:ef|gh)|y)`, + `a(?:x*(?:bcd|efg)|yz)`, + `(?i)s(?:elect.*into|ubstr)`, + `s(?:elect.*from|leep)`, + + // The shapes the trie reconstruction is meant for. + `(?i)(?:select|set|sleep)`, + `(?i)(?:select|sleep|substr|union|update|insert|delete|alter|create|benchmark|floor|format|length|concat|decode|encode|replace|reverse|trim|upper)`, + `(?i)(?:exec|execute|call|sp_|xp_|@@version|information_schema)`, + `(?i)(?:^|["':;=])\s*(?:alert|prompt|confirm)\s*\(`, + `(?i)union\s+(?:all\s+)?select`, + `(?i)(?:e(?:xec|val)|s(?:ystem|hell_exec)|p(?:assthru|open))\s*\(`, + `\.ph(?:p\d*|tml|ar|ps|t|pt)\.*$`, + } + + tx := corazawaf.NewWAF().NewTransaction() + defer tx.Close() + + for _, pat := range patterns { + t.Run(pat, func(t *testing.T) { + matched, compared, disagreements, first, err := prefilterDisagreement(pat, 300, tx) + if err != nil { + t.Fatalf("pattern: %v", err) + } + if matched == 0 { + t.Errorf("no generated input matched the pattern: the generator is not exercising it (%d inputs)", compared) + } + if disagreements > 0 { + t.Errorf("the prefilter changed the answer on %d of %d inputs; first: %q", disagreements, compared, first) + } + }) + } +} diff --git a/internal/operators/rxprefilter_test.go b/internal/operators/rxprefilter_test.go index 727949a16..958460016 100644 --- a/internal/operators/rxprefilter_test.go +++ b/internal/operators/rxprefilter_test.go @@ -7,6 +7,8 @@ import ( "fmt" "regexp" "regexp/syntax" + "slices" + "sort" "strings" "testing" "unicode/utf8" @@ -182,6 +184,32 @@ func TestPrefilterNeverCausesFalseNegatives(t *testing.T) { // "bc", so "bc" is NOT a true prefix constraint. {`^(?:a|.)bc.*$`, "xbc", true}, {`^(?:a|.)bc.*$`, "xyz", false}, + + // Regression: trie reconstruction glued a literal that follows a star onto + // the common prefix. The lab ruleset's XSS rule starts with + // �*60;|�*3c;|<..., which the parser factors into + // &(?:#(?:0*60;|x0*3c;)|lt;|...); the prefilter required "#60;" and + // "#3c;", neither of which is in "<" or "<", and rejected the + // payloads the regex matches (a missed attack under SecRxPreFilter On). + {`(?:�*60;|�*3c;|<)(?:script|javascript:)`, "<javascript:", true}, + {`(?:�*60;|�*3c;|<)(?:script|javascript:)`, "<script", true}, + {`(?:�*60;|�*3c;|<)(?:script|javascript:)`, "<script", true}, + {`(?:�*60;|�*3c;|<)(?:script|javascript:)`, "<script", true}, + {`(?:�*60;|�*3c;|<)(?:script|javascript:)`, "benign text", false}, + {`(?i)(?:�*60;|�*3c;|<|&#0*60;|&#x0*3c;|&lt;)(?:script|javascript:)`, "&#x3C;javascript:", true}, + {`(?i)(?:�*60;|�*3c;|<|&#0*60;|&#x0*3c;|&lt;)(?:script|javascript:)`, "&#0060;SCRIPT", true}, + + // The same mistake in the other shapes a suffix can take: after a star, an + // optional, a wildcard, or in front of a nested alternation. + {`s(?:x*elect|leep)`, "sxxelect", true}, + {`s(?:x*elect|leep)`, "select", true}, + {`a(?:b?cd|ef)`, "abcd", true}, + {`a(?:b?cd|ef)`, "acd", true}, + {`a(?:b.*cde|fg)`, "abxxcde", true}, + {`a(?:x*bcd(?:ef|gh)|y)`, "axxbcdef", true}, + {`a(?:x*(?:bcd|efg)|y)`, "axxbcd", true}, + {`a(?:x*(?:bcd|efg)|yz)`, "ayz", true}, + {`a(?:x*(?:bcd|efg)|yz)`, "nothing here", false}, } for _, tc := range tests { t.Run(fmt.Sprintf("%s/%s", tc.pattern, tc.input), func(t *testing.T) { @@ -1696,24 +1724,63 @@ func TestInternalNodeCoverage(t *testing.T) { } }) - // ── rawExtractSuffixes ──────────────────────────────────────────────────── + // ── leadingLiterals ─────────────────────────────────────────────────────── - t.Run("rawExtractSuffixes/RuneError_literal", func(t *testing.T) { - // rawExtractSuffixes must return nil for a literal containing RuneError + t.Run("leadingLiterals/RuneError_literal", func(t *testing.T) { + // leadingLiterals must return nil for a literal containing RuneError // so the calling trieReconstruct bails out rather than building a wrong prefilter. re := &syntax.Regexp{Op: syntax.OpLiteral, Rune: []rune{utf8.RuneError}} - if got := rawExtractSuffixes(re, false); got != nil { - t.Errorf("rawExtractSuffixes(RuneError) = %v, want nil", got) + if got := leadingLiterals(re, false); got != nil { + t.Errorf("leadingLiterals(RuneError) = %v, want nil", got) } }) - t.Run("rawExtractSuffixes/OpCapture", func(t *testing.T) { - // Capture groups are transparent — rawExtractSuffixes must unwrap them. + t.Run("leadingLiterals/OpCapture", func(t *testing.T) { + // Capture groups are transparent — leadingLiterals must unwrap them. inner := &syntax.Regexp{Op: syntax.OpLiteral, Rune: []rune("elect")} cap := &syntax.Regexp{Op: syntax.OpCapture, Sub: []*syntax.Regexp{inner}} - got := rawExtractSuffixes(cap, false) + got := leadingLiterals(cap, false) if len(got) != 1 || got[0] != "elect" { - t.Errorf("rawExtractSuffixes(OpCapture(elect)) = %v, want [elect]", got) + t.Errorf("leadingLiterals(OpCapture(elect)) = %v, want [elect]", got) + } + }) + + t.Run("leadingLiterals/anchored_at_start", func(t *testing.T) { + // Only text at the very start of the match counts. Whatever follows a + // star, an optional group or a class is not adjacent to a preceding + // prefix, so it must not be reported. + tests := []struct { + pattern string + want []string // nil: no leading set; compared as a sorted set + }{ + {`lect`, []string{"lect"}}, + {`lect|t`, []string{"lect", "t"}}, + {`e(?:lect|t)`, []string{"elect", "et"}}, + {`(?:e)(?:lect|t)`, []string{"elect", "et"}}, + {`ab+c`, []string{"ab"}}, + {`(?:ab)+c`, []string{"ab"}}, + {`ab*c`, []string{"a"}}, + {`ab?cd`, []string{"a"}}, + {`0*60;`, nil}, + {`0*60;|x0*3c;`, nil}, + {`x0*3c;`, []string{"x"}}, + {`elect.*from`, []string{"elect"}}, + {`.*elect`, nil}, + {`[a-z]elect`, nil}, + {`\belect`, nil}, + {`(?:elect|)x`, nil}, + {`(?:a|b*c)`, nil}, + } + for _, tc := range tests { + re, err := syntax.Parse(tc.pattern, syntax.Perl) + if err != nil { + t.Fatalf("parse %q: %v", tc.pattern, err) + } + got := leadingLiterals(re.Simplify(), false) + sort.Strings(got) + if !slices.Equal(got, tc.want) { + t.Errorf("leadingLiterals(%q) = %q, want %q", tc.pattern, got, tc.want) + } } })