8000 fix: Changed regex (920470) to match multiple whitespaces after `Content-Type` parameters to avoid false-positives by lostmann-owl-it · Pull Request #3818 · coreruleset/coreruleset · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content

fix: Changed regex (920470) to match multiple whitespaces after Content-Type parameters to avoid false-positives #3818

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.

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

Merged
merged 3 commits into from
Sep 16, 2024

Conversation

lostmann-owl-it
Copy link
Contributor
@lostmann-owl-it lostmann-owl-it commented Sep 12, 2024

Description

This pull request addresses a false positive related to the Content-Type header being incorrectly flagged by rule ID 920470. The issue arises due to overly restrictive whitespace handling in the regex validation. The original regex allowed only an optional whitespace between parameters, which blocked valid headers containing additional whitespaces, even though these are permitted according to RFC standards.

  • ModSecurity Version: OWASP CRS/3.3.4

Problem

The original regex for rule ID 920470 did not account for multiple or flexible whitespaces between header parameters. This caused valid Content-Type headers, compliant with RFC 2045 and RFC 7231, to be blocked unnecessarily.

Original regex

^[\w/.+*-]+(?:\s?;\s?(?:action|boundary|charset|component|start(?:-info)?|type|version)\s?=\s?['\"\w.()+,/:=?<>@#*-]+)*$

Updated regex

^[\w/.+*-]+(?:\s?;\s*(?:action|boundary|charset|component|start(?:-info)?|type|version)\s?=\s?['\"\w.()+,/:=?<>@#*-]+)*$

Changes

  • The \s?;\s? has been updated to \s?;\s* to allow for flexible whitespace usage, in accordance with RFC 2045 and RFC 7231, which permit multiple whitespaces between parameters.

Example

The blocked request contained the following Content-Type header:

Content-Type: multipart/related;    boundary="----=_part_224_386771358.1725612529970";    type="application/soap+xml"

This valid header was blocked because of extra spaces around the parameters, which are permitted under RFC specifications.

Impact

This update will prevent unnecessary blocking of valid HTTP requests while still maintaining the security provided by the rule.

Attachments

Original regex

image

Updated regex

image

@theseion
Copy link
Contributor

Thanks for the PR and the thorough description. Please add a test case for your change to https://github.com/coreruleset/coreruleset/blob/main/tests/regression/tests/REQUEST-920-PROTOCOL-ENFORCEMENT/920470.yaml.

@RedXanadu
Copy link
Member

@lostmann-owl-it CRS Dev on Duty reporting. As requested above, please can you add a test case(s) to cover your proposed change? We generally require a working test for a new PR.

If you need any help with this, please shout! Thanks.

@lostmann-owl-it
Copy link
Contributor Author

@lostmann-owl-it CRS Dev on Duty reporting. As requested above, please can you add a test case(s) to cover your proposed change? We generally require a working test for a new PR.

If you need any help with this, please shout! Thanks.

To make sure if I understand those checks correctly:

A test with the following output will be blocked by the WAF:

output:
  log:
    expect_ids: [920470]

A test with the following output will not be blocked by the WAF:

output:
  log:
    no_expect_ids: [920470]

Am I correct with this assumption?
I don't have a WAF to test this on.

@EsadCetiner
Copy link
Member

@lostmann-owl-it Yes you've got the idea

the output is checking if rule 920470 is being triggered or not based on the payload your sending for the test.

@RedXanadu
Copy link
Member

@lostmann-owl-it I wish I'd remembered to include this when I posted here on Friday, but we also have our gold standard test templates in our documentation. They can be copy and pasted to do 95% of the work! 🙂

@EsadCetiner
Copy link
Member

@lostmann-owl-it Could you also add yourself to the contributors.md file? It's sorted in alphabetical order by last name

lostmann-owl-it added a commit to lostmann-owl-it/coreruleset that referenced this pull request Sep 16, 2024
@lostmann-owl-it
Copy link
Contributor Author

I added myself. :)
Thanks!

@EsadCetiner EsadCetiner added this pull request to the merge queue Sep 16, 2024
Merged via the queue into coreruleset:main with commit d03e50b Sep 16, 2024
4 checks passed
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.

5 participants
0