Skip to content

fix(942200): reduce false positives on payloads with comments#4608

Merged
fzipi merged 7 commits into
coreruleset:mainfrom
EsadCetiner:fix-942200
Apr 18, 2026
Merged

fix(942200): reduce false positives on payloads with comments#4608
fzipi merged 7 commits into
coreruleset:mainfrom
EsadCetiner:fix-942200

Conversation

@EsadCetiner

Copy link
Copy Markdown
Member

Proposed changes

PR #4476 fixed a false positive in a French address which accidently introduced a regression resulting in virtually all user-agents with a comma being blocked. While most issues were fixed in #4537 there are still issues with payloads that contain a comma but no whitespace.

I think we should backport this for the LTS since it's a common enough false positive.

PR Checklist

  • I have read the CONTRIBUTING doc
  • I have added positive tests proving my fix/feature works as intended.
  • I have added negative tests that prove my fix/feature considers common cases that might end in false positives
  • In case you changed a regular expression, you are not adding a ReDOS for pcre. You can check this using regexploit
  • My test use the comment field to write the expected behavior
  • I have added documentation for the rule or change (when appropriate)

Further comments

For the reviewer

  • Positive and negative tests were added
  • Tests cover the intended fix/feature properly
  • No usage of dangerous constructs like ctl:requestBodyAccess=Off were used in the rule
  • In case a regular expression was changed, there is no ReDOS
  • Documentation is clear for the rule/change

@github-actions

github-actions Bot commented Apr 7, 2026

Copy link
Copy Markdown
Contributor

📊 Quantitative test results for language: eng, year: 2023, size: 10K, paranoia level: 1:
🚀 Quantitative testing did not detect new false positives

@EsadCetiner EsadCetiner added the backport:lts-4.25.1 PR that must be backported to LTS release 4.25.1 label Apr 16, 2026

@fzipi fzipi left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks!

@fzipi
fzipi added this pull request to the merge queue Apr 18, 2026
Merged via the queue into coreruleset:main with commit 2c64b25 Apr 18, 2026
8 checks passed
@EsadCetiner
EsadCetiner deleted the fix-942200 branch April 18, 2026 16:15
@fzipi fzipi mentioned this pull request Jul 1, 2026
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>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

backport:lts-4.25.1 PR that must be backported to LTS release 4.25.1

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants