Skip to content

fix(933150): reduce substring false positive matches#4340

Merged
fzipi merged 14 commits into
coreruleset:mainfrom
EsadCetiner:fix-intval-fp
Jan 27, 2026
Merged

fix(933150): reduce substring false positive matches#4340
fzipi merged 14 commits into
coreruleset:mainfrom
EsadCetiner:fix-intval-fp

Conversation

@EsadCetiner
Copy link
Copy Markdown
Member

@EsadCetiner EsadCetiner commented Nov 14, 2025 •
edited
Loading

Proposed changes

This PR reduces substring false positive matches with rule 933150 by moving the affected entries to 933160 which has some additional checks to avoid false positives.

I've also added a chained rule to 933150 which is meant to approximate the extra checks done by 933160 to further reduce false positives.

closes #4335

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
Copy link
Copy Markdown
Contributor

github-actions Bot commented Nov 14, 2025 •
edited
Loading

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

@EsadCetiner EsadCetiner mentioned this pull request Nov 14, 2025
Closed
fzipi
fzipi reviewed Nov 14, 2025
View reviewed changes
Copy link
Copy Markdown
Member

@fzipi fzipi left a comment

Choose a reason for hiding this comment

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

Maybe also updating https://github.com/EsadCetiner/coreruleset/blob/f5d2eeb0dd761ada71fd5ea1703647f13c6bdfbb/rules/REQUEST-933-APPLICATION-ATTACK-PHP.conf#L284 and https://github.com/EsadCetiner/coreruleset/blob/f5d2eeb0dd761ada71fd5ea1703647f13c6bdfbb/rules/REQUEST-933-APPLICATION-ATTACK-PHP.conf#L290 with the updated count?

@EsadCetiner
Copy link
Copy Markdown
Member Author

EsadCetiner commented Nov 15, 2025

@fzipi Thanks, updated

fzipi
fzipi requested changes Nov 15, 2025
View reviewed changes
Copy link
Copy Markdown
Member

@fzipi fzipi left a comment

Choose a reason for hiding this comment

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

Can you add tests for only one opening parenthesis and another one with an encoded form %28?

@EsadCetiner EsadCetiner requested a review from fzipi November 17, 2025 02:16
@github-actions github-actions Bot added the Stale label Dec 18, 2025
@EsadCetiner EsadCetiner removed the Stale label Dec 22, 2025
@theseion theseion mentioned this pull request Jan 5, 2026
Closed
@github-actions github-actions Bot added the Stale label Jan 22, 2026
@EsadCetiner EsadCetiner removed the Stale label Jan 24, 2026
@fzipi
Copy link
Copy Markdown
Member

fzipi commented Jan 26, 2026

Can you add tests for only one opening parenthesis and another one with an encoded form %28?

Sorry to ask @EsadCetiner , but did you got time to add additional tests for this? So we can move on.

@EsadCetiner
Copy link
Copy Markdown
Member Author

EsadCetiner commented Jan 26, 2026 •
edited
Loading

@fzipi

Sorry to ask @EsadCetiner , but did you got time to add additional tests for this? So we can move on.

Didn't I add the test you wanted here? cc5eb0d

@fzipi
Copy link
Copy Markdown
Member

fzipi commented Jan 26, 2026

Sorry, didn't saw any answer and assumed you didn't. Will check now.

fzipi
fzipi reviewed Jan 26, 2026
View reviewed changes
Comment thread rules/REQUEST-933-APPLICATION-ATTACK-PHP.conf Outdated
fzipi
fzipi previously approved these changes Jan 26, 2026
View reviewed changes
Copy link
Copy Markdown
Member

@fzipi fzipi left a comment

Choose a reason for hiding this comment

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

Approved. If you want to update the text to my comment, better.

@EsadCetiner @fzipi
Apply suggestion from @fzipi
4c66251 
Co-authored-by: Felipe Zipitría <3012076+fzipi@users.noreply.github.com>
@EsadCetiner EsadCetiner requested a review from fzipi January 27, 2026 00:49
fzipi
fzipi approved these changes Jan 27, 2026
View reviewed changes
Copy link
Copy Markdown
Member

@fzipi fzipi left a comment

Choose a reason for hiding this comment

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

Thanks! Great work here 💪 !

@fzipi fzipi added this pull request to the merge queue Jan 27, 2026
Merged via the queue into coreruleset:main with commit 40706b5 Jan 27, 2026
8 checks passed
@EsadCetiner EsadCetiner deleted the fix-intval-fp branch January 27, 2026 01:37
@theseion theseion mentioned this pull request Feb 2, 2026
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@fzipi fzipi fzipi approved these changes

Assignees

No one assigned

Labels

release:fix

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Rule 933150 Has False Positive for PHP Functions in Substrings

2 participants

@EsadCetiner @fzipi