8000 False positive for rule 921110 · Issue #2054 · coreruleset/coreruleset · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content

False positive for rule 921110 #2054

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
Closed
jdevreese opened this issue Apr 13, 2021 · 9 comments
Closed

False positive for rule 921110 #2054

jdevreese opened this issue Apr 13, 2021 · 9 comments
Assignees
@franbuehler
Labels
➕ False Positive ❓ Need more info

Comments

@jdevreese
Copy link
jdevreese commented Apr 13, 2021 •
edited by azurit
Loading

Description

Uploading an xml containing text budget foo)</bar>\n triggers this false positive.

Audit Logs / Triggered Rule Numbers

Rule ID: 921110

[13/Apr/2021:14:41:44 +0200] 161831770463.585568 172.19.0.167 63907 192.168.1.151 443 ModSecurity: Warning. Matched "Operator `Rx' with parameter `(?:get|post|head|options|connect|put|delete|trace|track|patch|propfind|propatch|mkcol|copy|move|lock|unlock)\s+(?:\/|\w)[^\s]*(?:\s+http\/\d|[\r\n])' against variable `REQUEST_BODY' (Value: `------WebKitFormBoundaryFYfRC2r9AA3TKtgx\x0d\x0aContent-Disposition: form-data; name="file"; filenam (303055 characters omitted)' ) [file "/etc/modsecurity.d/owasp-crs/rules/REQUEST-921-PROTOCOL-ATTACK.conf"] [line "33"] [id "921110"] [rev ""] [msg "HTTP Request Smuggling Attack"] [data "Matched Data: get foo)</bar>\x0a found within REQUEST_BODY: ------webkitformboundaryfyfrc2r9aa3tktgx\x0d\x0acontent-disposition: form-data; name="file"; filename="redacted (278202 characters omitted)"] [severity "2"] [ver "OWASP_CRS/3.3.0"] [maturity "0"] [accuracy "0"] [tag "application-multi"] [tag "language-multi"] [tag "platform-multi"] [tag "attack-protocol"] [tag "paranoia-level/1"] [tag "OWASP_CRS"] [tag "capec/1000/210/272/220/33"] [hostname "192.168.1.151"] [uri "/redacted"] [unique_id "161831770463.585568"] [ref "o67861,34o117443,34v2811,278366t:urlDecodeUni,t:htmlEntityDecode,t:lowercase"]

Your Environment

  • CRS version: 3.3.0
  • ModSecurity version: 3.0.4
  • Web Server and version: Nginx 1.19.8

Confirmation

[X] I have removed any personal data (email addresses, IP addresses,
passwords, domain names) from any logs posted.

Probably caused by the changes in #1883
@azurit
Copy link
Member
azurit commented Apr 13, 2021 •
edited
Loading

Thank you for reporting this!

CRS version: 3.3.0
Probably caused by the changes in #1883

#1883 is a reported issue so there are no changes caused by it. This issue was resolved by PR #1966 (created on 1 Jan 2021), are you talking about this PR? If so, can you be more specific in CRS version which you are using? PR #1966 was not part of CRS 3.3.0 (release on 1 Jul 2020).

@azurit
Copy link
Member
azurit commented Apr 16, 2021

@jdevreese Can you, please, provide more info? See above. Thank you!

@jdevreese
Copy link
Author

Must have made a mistake referencing that issue.

But I was looking at this commit: 8f53825

Before: https://regex101.com/r/IgQJtL/2
After: https://regex101.com/r/qSMFIv/2

Suggestion: check for word boundary before the keyword?
https://regex101.com/r/JpmJOC/1

@azurit
Copy link
Member
azurit commented Apr 17, 2021

@franbuehler Can you, please, explain why have you removed [\n\r]+ from regexp of rule 921110? See above. Thank you!

@franbuehler
Copy link
Contributor
franbuehler commented Apr 18, 2021 •
edited
Loading

With this PR I wanted to resolve an issue reported by the security researcher Amit Klein.
He responsibly disclosed a CRS bypass for HTTP request smuggling.

One important test for the reported issues (there were multiple) is:
8f53825#diff-46cb817d4e84d68effd07dc75e00482fb6fee70cf5e1f57c0e03abb0695ca9baR110

His bypass was not detected, because of two reasons:

  1. ARGS was not checked/filled for text/plain requests. We added REQUEST_BODY.
  2. And we also removed the [\n\r]+ at the beginning.

Comparison of regexes with the reported issue:

Before: https://regex101.com/r/WolWP9/1
After: https://regex101.com/r/BcDmYV/1
With word boundary (new proposal): https://regex101.com/r/vOG4RW/1

We see that with the word boundary we don't find both HTTP requests.
But to be honest, I don't know if the finding of the second HTTP request is enough. If yes, we can add the word boundary.

I hope this helps? Let me know if I can do anything, a PR or so...

@lifeforms lifeforms mentioned this issue Apr 19, 2021
Closed
@dune73
Copy link
Member
dune73 commented Apr 19, 2021

We talked about this in the April issue chat. Here is our conclusion:

We lack the context of the commit that @franbuehler explained above. We need to wait for her to provide that context after her holidays. We can then look into the context and ideally come up with a better PR that does not lead to that many new false positives.

@franbuehler franbuehler self-assigned this May 17, 2021
@franbuehler
Copy link
Contributor
franbuehler commented May 17, 2021 •
edited
Loading

Hi @theMiddleBlue
You helped me with solving the issue reported by Amit Klein.

Current regex and coverage is: https://regex101.com/r/BcDmYV/1
New proposal would be: https://regex101.com/r/vOG4RW/1

We see that with the word boundary we don't find both HTTP requests.
But to be honest, I don't know if the finding of the second HTTP request is enough. If yes, we can add the word boundary.

Can you help me and do you know if the new proposal would be enough to find the request smuggling??

For more info see my comment above.

Thank you :-)

@franbuehler franbuehler mentioned this issue May 17, 2021
Closed
@theMiddleBlue
Copy link
Contributor

Hi,

thank you for the recap! I tried the new regex but it seems prone to similar false positives such as (get it).\n. Maybe to reduce FP in PL1 we should remove the |[\r\n] part from (?:\s+http\/\d|[\r\n]). Also, we can move a copy of the actual rule to PL2 to have a stricter rule? What do you think about it?

Thanks!

@franbuehler franbuehler mentioned this issue May 24, 2021
Merged
@franbuehler
Copy link
Contributor

I had a look at this issue again and wanted to implement @theMiddleBlue's proposal when I saw that the false positive has already been resolved by Federico in 3.4/dev:
3af9507

However, I added two tests for the false positive and @theMiddleBlue's test in PR #2102.

So this issue could be closed in my opinion.

@dune73 dune73 mentioned this issue Jun 21, 2021
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@franbuehler franbuehler

Labels
➕ False Positive ❓ Need more info
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
5 participants
@dune73 @azurit @theMiddleBlue @jdevreese @franbuehler
0