fix(941140): remove exponential backtracking in CSS url(javascript) detection#4670
Merged
Conversation
…acking Move the inline pattern to regex-assembly/941140.ra and replace the free '.+' in the declaration list with '[^;]+', removing the ambiguity that caused exponential backtracking on backtracking engines (PCRE2/RE2 were unaffected). Detection is unchanged (differential-tested).
fzipi
added
release:fix
backport:lts-4.25.1
Contributor
|
📊 Quantitative test results for language: |
theseion
requested changes
Jun 19, 2026
| log: | ||
| no_expect_ids: [941140] | ||
| - test_id: 13 | ||
| desc: "ReDoS regression: long declaration list, no payload" |
Contributor
There was a problem hiding this comment.
Comment on what the effect would be here. I suspect the test would result in an error if ReDoS occurred?
Member
Author
There was a problem hiding this comment.
you're right to question it — and the honest answer is that this test does not error/time out if the unbounded value comes back. this rule is in the "moderate" ReDoS tier: the old (?:[^:=]+:.+;)*?... pattern is exponential on naive-backtracking engines (Python re, Perl, PHP-preg), but PCRE2 — the engine go-ftw runs here — tames it via auto-possessification plus the required url(javascript literal study, so this input completes fast even with the old pattern. go-ftw also has no per-test latency assertion.
so test_id 13 is really a no-match canary: it asserts a long key:value; list with no payload does not falsely match (no_expect_ids), and documents the shape that motivated the bound. it is not a timing gate. i've reworded the desc in 0587ee1 to say exactly that, so it isn't mistaken for one.
Address review feedback on #4670: - note that t:removeWhitespace strips whitespace before the regex runs, so the character classes intentionally do not account for it - reword test_id 13 desc: it is a no-match canary for the unbounded-value shape (PCRE2 tames both old and new patterns), not a latency assertion
theseion
approved these changes
Jun 20, 2026
fzipi
added a commit
that referenced
this pull request
Jun 22, 2026
…etection (#4670) * fix(941140): bound CSS declaration value to remove exponential backtracking Move the inline pattern to regex-assembly/941140.ra and replace the free '.+' in the declaration list with '[^;]+', removing the ambiguity that caused exponential backtracking on backtracking engines (PCRE2/RE2 were unaffected). Detection is unchanged (differential-tested). * docs(941140): clarify whitespace handling and regression-test intent Address review feedback on #4670: - note that t:removeWhitespace strips whitespace before the regex runs, so the character classes intentionally do not account for it - reword test_id 13 desc: it is a no-match canary for the unbounded-value shape (PCRE2 tames both old and new patterns), not a latency assertion
Merged
fzipi
added a commit
that referenced
this pull request
Jul 2, 2026
* fix: false positive with parameter name `.history` (#4614) (cherry picked from commit 0885958) * fix(942200): reduce false positives on payloads with comments (#4608) * fix(942200): reduce false positives on payloads with comments * add known fp test case * improve test comments * fix: wrong test * add known fp test case * fix typo * fix typo (cherry picked from commit 2c64b25) * fix(unix): exclude `pg` command from pl-1 (#4613) (cherry picked from commit 82195a4) * fix(920100): drop HTTP/0.9 GET support from request line validation (#4621) * fix(920100): drop HTTP/0.9 GET support from request line validation remove the GET-without-protocol alternative from the regex assembly. HTTP/0.9 is obsolete per RFC 9110 and the dedicated alternative was the only one that did not require a protocol suffix in the request line. GET requests with proper HTTP/1.x or HTTP/2 protocol versions remain covered by the generic method alternative. * Apply suggestions from code review Co-authored-by: Max Leske <[email protected]> --------- Co-authored-by: Max Leske <[email protected]> (cherry picked from commit f8402a9) * fix(920240, 920400): don't rely on content-type header (#4639) (cherry picked from commit 2f1afb7) * fix: remove exponential backtracking in 933160/933161 comment suffix (#4666) * fix: remove exponential backtracking in 933160/933161 comment suffix The suffix that skips whitespace and PHP comments between a function name and the opening parenthesis used an ambiguous alternation where the whitespace branch and the comment bodies both matched the same characters under a `*` quantifier. On a backtracking engine a payload that never reaches `(` backtracks exponentially. De-ambiguate the alternation while keeping RE2 compatibility (no atomic groups): - block comments use the unrolled form instead of `/\*.*\*/` - line comments (`#`, `//`) must terminate at a line break, because a line comment before `(` is only reachable across a newline; otherwise the `(` is inside the comment and there is no function call Add positive tests for the comment/whitespace evasions (block, line with newline, multi-line block, vertical tab, mixed) and negatives to 933161. * docs: explain unrolled comment form in 933160/933161 .ra * Update regex-assembly/933161.ra Co-authored-by: Max Leske <[email protected]> --------- Co-authored-by: Max Leske <[email protected]> (cherry picked from commit c1df902) * fix(941140): remove exponential backtracking in CSS url(javascript) detection (#4670) * fix(941140): bound CSS declaration value to remove exponential backtracking Move the inline pattern to regex-assembly/941140.ra and replace the free '.+' in the declaration list with '[^;]+', removing the ambiguity that caused exponential backtracking on backtracking engines (PCRE2/RE2 were unaffected). Detection is unchanged (differential-tested). * docs(941140): clarify whitespace handling and regression-test intent Address review feedback on #4670: - note that t:removeWhitespace strips whitespace before the regex runs, so the character classes intentionally do not account for it - reword test_id 13 desc: it is a no-match canary for the unbounded-value shape (PCRE2 tames both old and new patterns), not a latency assertion (cherry picked from commit fe59387) * fix(933180): remove exponential backtracking in variable-function noise suffix (#4669) * fix(933180): remove exponential backtracking in variable-function noise suffix * fix(933180): require non-empty array/brace bodies in noise suffix Address review on #4669: use + instead of * for the array-access and curly-brace noise branches so empty [] / {} are not matched. This restores the non-empty semantics of the original .+ patterns; the * form had accidentally broadened them. Differential vs the original rule shows zero true-positive loss; REQUEST-933 416/416 pass. * test(933180): verify empty array/brace bodies are not matched (cherry picked from commit 3828246) * feat(crs-setup): add default actions for phases 3-5 (#4675) (cherry picked from commit 09c1a11) * fix: 4RI-250413 (#4672) * fix: 4RI-250413 * fix: tests * chore(formatting): auto fixes from pre-commit hooks for more information, see https://pre-commit.ci * fix: typo * fix: tests * apply code review suggestions * apply code review suggestions * chore(formatting): auto fixes from pre-commit hooks for more information, see https://pre-commit.ci * chore(942360): update regex with better format Signed-off-by: Felipe Zipitria <[email protected]> * fix(942362): add json. prefix support for JSON ARGS_NAMES Rule 942362 uses a ^[\W\d]+ anchor that silently fails on libModSecurity3 because JSON payloads prepend json. (starting with j, a letter) to ARGS_NAMES. Apply the same fix used in 942360/942361: ^(?:json\.)?[\W\d]+. * Apply suggestions from code review Co-authored-by: Max Leske <[email protected]> * apply code review suggestions * Update regex-assembly/942360.ra Co-authored-by: Max Leske <[email protected]> --------- Signed-off-by: Felipe Zipitria <[email protected]> Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com> Co-authored-by: Felipe Zipitria <[email protected]> Co-authored-by: Max Leske <[email protected]> (cherry picked from commit 6e9ff3c) * chore: release v4.25.1 * fix: correct indentation and action order in commented 900510 example * docs: note libmodsecurity v3.0.16 requirement in CHANGES * chore(tests): bump modsecurity-crs nginx image to libmodsecurity v3.0.16 * fix(901180): split XML attribute inspection gate to fix unused TX variable lint error * fix: renumber duplicated test Signed-off-by: Felipe Zipitria <[email protected]> * fix: restore dropped json. prefix fixes and fix stale nginx CONNECT override Cherry-picking PR #4672 (4RI-250413) took the LTS side wholesale for rules/REQUEST-932-APPLICATION-ATTACK-RCE.conf and rules/REQUEST-942-APPLICATION-ATTACK-SQLI.conf, then only regenerated the rule IDs backed by a regex-assembly (.ra) file. Two hand-written inline regexes with no .ra file (932171, 942361) were silently dropped in that process and never got the `(?:json\.)?` ARGS_NAMES prefix fix, so 942361-9 failed to detect the SQLi payload on nginx/libmodsecurity3. Also updates the nginx-overrides.yaml entry for 920100 test 4: the libmodsecurity v3.0.16 image bump also picked up nginx 1.30.3 (up from 1.28.2), which now rejects CONNECT with 405 instead of 400. * docs: note ModSecurity v2 limitation for XML attribute opt-out in CHANGES --------- Signed-off-by: Felipe Zipitria <[email protected]> Co-authored-by: Esad Cetiner <[email protected]> Co-authored-by: Max Leske <[email protected]> Co-authored-by: Prateek saini <[email protected]> Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
what
moves rule 941140 (CSS-based XSS via
url(javascript:...)) from an inline pattern toregex-assembly/941140.raand removes its exponential backtracking. the declaration-list loop previously used a free.+:(?i)[a-z]+=(?:[^:=]+:.+;)*?[^:=]+:url\(javascript(?i)[a-z]+=(?:[^:=]+:[^;]+;)*?[^:=]+:url\(javascriptthe
.+(DOTALL) sat next to the[^:=]+key inside the*-quantified declaration list, so the same characters could be consumed in more than one way. bounding the value with[^;](it stops at the declaration separator) makes each declaration match exactly once.the new
.radecomposes the pattern with named defines (decl-key,decl-value) and a three-part sequence so the bound is self-documenting. expands the tests from 4 to 14 (multi-declaration lists, colon-containing values, case-insensitivity, thet:removeWhitespacepath, the User-Agent source, benign no-match cases, and a ReDoS-regression no-match with a long declaration list).why
with
.matching newlines (DOTALL, as ModSecurity runs@rx), the free.+next to[^:=]+under the repeat is ambiguous, so an input without a trailingurl(javascriptbacktracks exponentially. measured on Pythonre(DOTALL): the old pattern timed out by n≈30 (a=+x:y;×n); the new pattern is flat (1.4 ms at n=8000). PCRE2 and RE2 already tame the old form (PCRE2 via auto-possessification, RE2 has no backtracking), so this is a moderate-severity hardening on backtracking engines, the same tier as the 933160/933161 and 933180 fixes, not a runtime DoS on CRS's supported engines.detection is unchanged: a differential test of the old vs new pattern over 400,000 random inputs plus targeted templates found zero divergences. PL1, so the rule is always active; the full REQUEST-941 suite passes (228/228).
refs
ai disclosure
.ra(defines + sequence), drafting the ten additional go-ftw tests, PR description draftingre(n up to 8000) and verified PCRE2 does not exceed its backtracking limit withpcre2test(match_limit=1,000,000); ran a 400,000-input differential test of old vs new pattern (zero divergences) to confirm no detection change; ran the full REQUEST-941 go-ftw suite on modsec2-apache (228/228) and confirmed all 14 941140 tests pass; verified the regex-assembly round-trip withcrs-toolchain regex compare; ran crs-linter 1.0.0 (clean)