Skip to content

Conversation

@EsadCetiner
Copy link
Member

@EsadCetiner EsadCetiner commented Nov 14, 2025

Proposed changes

This PR adds a chained rule to rule 932180 to reduce false positive with substring matches. It checks for entries in restricted-upload.data that commonly result in false positives and in effect enforces a word boundary for those keywords if they exist within the parameter.

closes #4320

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

N/A

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
Contributor

github-actions bot commented Nov 14, 2025

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

@theseion
Copy link
Contributor

I'm not a fan of hardcoding these values. I've thought about using the delimiter technique with @within (https://github.com/owasp-modsecurity/ModSecurity/wiki/Reference-Manual-%28v2.x%29#within), but I don't see how we can make it work better than what you already have.

I also don't think the delimiter technique described for @pmFromFile will work in this case.

Let's ask for more ideas.

@theseion
Copy link
Contributor

Actually, I tried to use a macro with @rx and it seems to work with both httpd and nginx:

SecRule MATCHED_VAR "@rx (?i)%{MATCHED_VAR}\s" \

@EsadCetiner
Copy link
Member Author

@theseion

I'm not a fan of hardcoding these values.

Agreed, but the only other solution I see is moving to a regex.

Actually, I tried to use a macro with @rx and it seems to work with both httpd and nginx:

aah I see what happened, you originally mentioned using @rx %{MATCH_DATA}\b which isn't valid, I autocorrected it to @rx %{MATCHED_DATA}\b which is what I used when I tested the rule but I didn't pick up on the other mistake.

After digging a bit deeper, I can see this won't work as well because there are some entries such as:

wp-config-
wp-config.
wp-config_

which is intentionally meant to match permutations, adding a word boundary for all entries will result in false negatives. I'll add some tests for this case as the tests are a bit bare for this rule.

@theseion theseion added this pull request to the merge queue Nov 15, 2025
Merged via the queue into coreruleset:main with commit d917d66 Nov 15, 2025
7 checks passed
@EsadCetiner EsadCetiner deleted the fix-932180-fp branch November 15, 2025 11:52
@fzipi fzipi mentioned this pull request Dec 1, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Rule 932180 FP on Filename Substring

2 participants