Skip to content

fix: remove bypass-vulnerable content types from default allow lists #4365

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Open
RedXanadu wants to merge 2 commits into
base: main
Choose a base branch
from
Open

fix: remove bypass-vulnerable content types from default allow lists #4365

RedXanadu wants to merge 2 commits into from

Conversation

@RedXanadu
Copy link
Member

@RedXanadu RedXanadu commented Nov 28, 2025 •
edited
Loading

Proposed changes

Fixes #4362

Remove content types that we do not parse/process by default from the stock list of allowed request content types.

Note: This PR is intentionally limited in scope to fixing the original bypass/issue. If we want to discuss "should we add more default allowed content types?" and "should we override modsecurity.conf-recommended body processor rules in CRS?" then we can have that conversation separately.

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 n/a
  • In case you changed a regular expression, you are not adding a ReDOS for pcre. You can check this using regexploit n/a
  • My test use the comment field to write the expected behavior
  • I have added documentation for the rule or change (when appropriate)

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

@RedXanadu RedXanadu self-assigned this Nov 28, 2025
@github-actions
Copy link
Contributor

github-actions bot commented Nov 28, 2025

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

@RedXanadu RedXanadu mentioned this pull request Nov 28, 2025
Open
@fzipi fzipi changed the title Remove bypass-vulnerable content types from default allow lists fix: remove bypass-vulnerable content types from default allow lists Nov 28, 2025
@touchweb-vincent
Copy link
Contributor

touchweb-vincent commented Dec 7, 2025 •
edited
Loading

Hello,

I don’t understand why the conditional enabling of the XML/JSON processors isn’t in crs-setup.conf.example but instead here:
https://github.com/owasp-modsecurity/ModSecurity/blob/v3/master/modsecurity.conf-recommended

What you just reported is going to happen again.

In my opinion, we should add a comment above 900220 stating that it is forbidden to add items to this stack without simultaneously adding a corresponding declaration to one of the body processors.

@fzipi fzipi requested review from airween and theseion December 8, 2025 16:56
@theseion
Copy link
Contributor

theseion commented Dec 9, 2025

I don’t understand why the conditional enabling of the XML/JSON processors isn’t in crs-setup.conf.example but instead here

For historical reasons and now we're stuck with modsecurity.conf-recommended. We've had issues with the interoperability before but we can't simply remove / change that file, since CRS is technically only one potential choice of rule set, so the defaults can't rely on the CRS configuration.

What you just reported is going to happen again.

Probably, but if I had done my job right, I would have read the existing comment and realised that I was making a mistake.

In my opinion, we should add a comment above 900220 stating that it is forbidden to add items to this stack without simultaneously adding a corresponding declaration to one of the body processors.

That comment exists in crs-setup.conf.example, where it belongs.

@touchweb-vincent
Copy link
Contributor

touchweb-vincent commented Dec 9, 2025 •
edited
Loading

You’re right, there’s already a long explanation about this. https://github.com/coreruleset/coreruleset/blob/main/crs-setup.conf.example#L499C1-L506C64

# When additional JSON content types are legitimately used in a deployment,
# e.g. application/cloudevents+json, it is extremely important to ensure that a
# rule exists to enable the engine's JSON body processor for these additional
# JSON content types. Failure to do so can lead to a request body bypass. The
# default JSON rule in modsecurity.conf-recommended (200001) will only activate
# the JSON body processor for the specific content type application/json. The
# optional modsecurity.conf-recommended rule 200006 can be used to enable the
# JSON body processor for a wide variety of JSON content types.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@theseion theseion theseion approved these changes

@airween airween Awaiting requested review from airween

Assignees

@RedXanadu RedXanadu

Labels

🏁 ready to merge

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Default list of allowed request content types includes non-processed types

4 participants

@RedXanadu @touchweb-vincent @theseion