8000 Rule 932200, now inspecting Referer headers, matches any query string that contains spaces · Issue #3180 · coreruleset/coreruleset · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content

Rule 932200, now inspecting Referer headers, matches any query string that contains spaces #3180

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

Closed
RedXanadu opened this issue Mar 30, 2023 · 21 comments · Fixed by #3300
Closed

Comments

@RedXanadu
Copy link
Member

Description

Rule 932200 was recently updated to add the following items for inspection:

REQUEST_HEADERS:Referer|REQUEST_HEADERS:User-Agent

Unfortunately, with the Referer header, this rule now causes false positives for the following scenario:

  • referer header contains a URL, and
  • URL includes a query string, and
  • query string includes one or more space characters (+) and/or URL includes one or more space characters (%20).

For example, these headers both cause a false positive:

Referer: http://www.example.com/page?param=test+test'

Referer: http://www.example.com/page%20test?param=test'

This header does not cause a false positive, as there are no encoded space characters:

Referer: http://www.example.com/page?param=test_no_space_char'


Cause

Rule 932200 has a regular expression looking for one of a number of optional patterns. This is the problematic pattern:

/[^/]+?[*?`\x5c']

<forward slash><non-forward slash chars><question mark> -> this will match URLs that feature a query string, e.g.

/some/dir/page?foo=bar

The presence of this chained rule:

SecRule MATCHED_VAR "@rx \s" "t:none,t:urlDecodeUni

means that a space character must also be present.

How to reproduce the misbehavior (-> curl call)

Testing the CRS Docker container:

curl -v -o/dev/null localhost -A 'test' -H 'Referer: http://www.example.com/page?param=test+test'

=>

[Thu Mar 30 11:13:44.997007 2023] [security2:error] [pid 40:tid 140616113563392] [client 172.19.0.1:54008] [client 172.19.0.1] ModSecurity: Warning. Pattern match "\\\\s" at MATCHED_VAR. [file "/etc/modsecurity.d/owasp-crs/rules/REQUEST-932-APPLICATION-ATTACK-RCE.conf"] [line "942"] [id "932200"] [msg "RCE Bypass Technique"] [data "Matched Data: /page? found within REQUEST_HEADERS:Referer: http://www.example.com/page?param=test test"] [severity "CRITICAL"] [ver "OWASP_CRS/4.0.0-rc1"] [tag "modsecurity"] [tag "application-multi"] [tag "language-multi"] [tag "platform-multi"] [tag "attack-rce"] [tag "paranoia-level/2"] [tag "OWASP_CRS"] [tag "capec/1000/152/248/88"] [tag "PCI/6.5.2"] [hostname "localhost"] [uri "/"] [unique_id "ZCVu6EXwaRSBQCgL9kHo9wAAAIw"], referer: http://www.example.com/page?param=test+test

Your Environment

  • CRS version (e.g., v3.3.4): v4.0.0-rc1
  • Paranoia level setting (e.g. PL1) : PL 2
  • ModSecurity version (e.g., 2.9.6): n/a
  • Web Server and version or cloud provider / CDN (e.g., Apache httpd 2.4.54): Apache
  • Operating System and version: n/a

Confirmation

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

@theseion
Copy link
Contributor

I vote for simply removing Referer matching again. There's no quick fix here I can see, we would need to craft a new rule that targets the actual format of the Referer header and checks for URL related bypass techniques specifically.

@Xhoenix
Copy link
Member
Xhoenix commented Mar 31, 2023

I currently using the following rule as an exclusion. It handles the above situation and another situation that my personal blog faced.

SecRule SERVER_NAME "@contains blog.jitendrapatro.me" \
     "id:1058,\
     phase:1,\
     pass,\
     nolog,\
     chain"
     SecRule REQUEST_HEADERS:Referer "@rx /\\w+(?:\.|'')\\w+\\?" \
        "ctl:ruleRemoveTargetById=932200;REQUEST_HEADERS:Referer"

@Xhoenix
Copy link
Member
Xhoenix commented Mar 31, 2023

The exclusion rule regex can be simplified to /[^/]+\\? but this allows all non / characters, which includes the entire ascii character set.

I think the regex should be improved to leave out the alphanumeric character range and a . (as I did above), while taking into account everything else as that can be a part of malicious payload.

@lifeforms
Copy link
Member

I also think we should remove Referer again.

@Xhoenix
Copy link
Member
Xhoenix commented Mar 31, 2023

If you're going to remove it, then atleast take into consideration what @theseion suggested. There are cases in which the Referer header can be exploited by attackers and we need to have separate rule targeting common bypass techniques.

@Xhoenix
Copy link
Member
Xhoenix commented Apr 3, 2023

I thought this over and I think removing is totally not an solution. Rather, there should be a separate rule specifically targeting common bypasses.

@emphazer
Copy link
Contributor
emphazer commented Apr 4, 2023

another really bad match is letsencrypt User-Agent...
User-Agent: Mozilla/5.0 (compatible; Let's Encrypt validation server; +https://www.letsencrypt.org)

Message: Warning. Pattern match "\\s" at MATCHED_VAR. [file "/etc/httpd/modsecurity.d/activated_rules/REQUEST-932-APPLICATION-ATTACK-RCE.conf"] [line "888"] [id "932200"] [msg "RCE Bypass Technique"] [data "Matched Data: /5.0 (compatible; let' found within REQUEST_HEADERS:User-Agent: mozilla/5.0 (compatible; let's encrypt validation server; https://www.letsencrypt.org)"] [severity "CRITICAL"] [ver "OWASP_CRS/4.0.0-rc1"] [tag "attack-rce"] [tag "paranoia-level/2"

@Xhoenix
Copy link
Member
Xhoenix commented Apr 4, 2023

How did you get the Let's Encrypt FP? I've set Certbot to automatically update Certificates at the first of every month and have never faced any problems.

@emphazer
Copy link
Contributor
emphazer commented Apr 4, 2023

@Xhoenix I installed the newest v4 version on several servers and they had all the same problem...

if this won't get fixed we definitely will exclude this rule for User-Agent for our deployment.

@theseion
Copy link
Contributor
theseion commented Apr 4, 2023

There's already a separate issue for this: #3163.

@Xhoenix
Copy link
Member
Xhoenix commented Apr 5, 2023

@emphazer I think the problem is while installing new certificates and not while renewing them. Did the FP also shows up when renewing certs?

@emphazer
Copy link
Contributor
emphazer commented Apr 5, 2023

@Xhoenix i didn't try it with new certificates yet.
it occurred on round about 20 different 8000 vhosts on different servers while renewing the certificate.

we run everything at PL2 detectiononly with the newest dev/4 release for testing purposes on production machines.

@Xhoenix
Copy link
Member
Xhoenix commented Apr 6, 2023

@RedXanadu How about this:-
/[^ a-zA-Z0-9._-]+?[*?`\x5c'])

In the above regex if I've used the / then a payload like cat /etc/passwd would've been allowed. This above also excludes common command injection characters like &|;`$\ and more. This also protects against all command injection payloads presently known publicly.

For a basic test:-

echo "$testing/t so-me.php?isanothertest" | grep -oP "/[ a-zA-Z0-9._-]+"

Is there any reason this shouldn't be used? I'll be happy to open a PR if you want this. 🙂

@Xhoenix
Copy link
Member
Xhoenix commented Apr 6, 2023

Okay, the hyphen character(-) allows payloads like sh -c whoami, but will most likely get blocked by other rules. Still, I think removing it would be a better choice.

@RedXanadu
Copy link
Member Author

@Xhoenix Not sure if that would work, you'd have to test it. (I'm not working on a fix for this issue, just FYI.)

The outcome from the meeting discussion was to try and fix the existing regular expression. If that does not work then 'Referer' inspection for this case will be moved to paranoia level 3 as we cannot afford these false positives at PL 2.

theseion added a commit to theseion/coreruleset that referenced this issue Sep 9, 2023
New rules 932205, 932206 to handle the Referer header explicitly.

The regular expression in 932200 leads to false positives against URLs
with query strings (due to the `?`). 932205 uses an additional prefix in
the regular expression that matches the first `?` so that the following
expressions will only match question marks that are part of the payload.

932206 uses an additional prefix to match only when the Referer value is
not a URL (which is illegal). 932206 is thus equivalent to 932200 but is
required to distinguish the case where the Referer header does actually
contain a URL.

Fixes coreruleset#3180
@theseion
Copy link
Contributor
theseion commented Sep 9, 2023

I've opened a PR that addresses this issue: #3300. I was forced to create two additional rules. @Xhoenix, I would appreciated it if you'd also take a look at the PR.

@Xhoenix
Copy link
Member
Xhoenix commented Sep 9, 2023

Small typos in rule 932200 regex. Updated regex can be viewed here:- https://regex101.com/r/w3kLqB/1

Checking the other rules.

@theseion
Copy link
Contributor

As far as I can tell, your version is identical to the one in this PR...?

@RedXanadu
Copy link
Member Author
RedXanadu commented Sep 11, 2023

@Xhoenix There's a CRS rule that states that forward slashes are not escaped in regular expression patterns, in the interests of making patterns slightly easier to read. That might help explain why the patterns in the PR look the way that they do 🙂

@theseion I've suggested (via a PR to your PR) to also add tests to check against the original rule that was reported to have false positives. I've also asked a question in the PR review as I don't follow what part of the PR is doing.

@theseion
Copy link
Contributor

@RedXanadu I can't find the PR that you say you've opened.

@theseion
Copy link
Contributor

Found it. Since the PR has already been merged, I've created a new PR with your changes: #3309.

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

Successfully merging a pull request may close this issue.

5 participants
0