8000 Rule 920450 and modsec 3x · Issue #1741 · coreruleset/coreruleset · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content

Rule 920450 and modsec 3x #1741

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
CRS-migration-bot opened this issue May 13, 2020 · 4 comments
Closed

Rule 920450 and modsec 3x #1741

CRS-migration-bot opened this issue May 13, 2020 · 4 comments
Labels
🐛 bug Something isn't working

Comments

@CRS-migration-bot
Copy link

Issue originally created by user mirkodziadzka-avi on date 2020-04-20 16:01:48.
Link to original issue: SpiderLabs/owasp-modsecurity-crs#1741.

Regarding owasp-modsecurity/ModSecurity#2296

We detected this, because rule 920450 is setting setvar:'tx.header_name_%{tx.0}=/%{tx.0}/' but accessing it via SecRule TX:/^HEADER_NAME_/

I think since modsec 3x seems to be (wrongly) case sensitive on regex, rule 920450 could be changed as

diff --git a/rules/REQUEST-920-PROTOCOL-ENFORCEMENT.conf b/rules/REQUEST-920-PROTOCOL-ENFORCEMENT.conf
index 880c8c4..49ea353 100644
--- a/rules/REQUEST-920-PROTOCOL-ENFORCEMENT.conf
+++ b/rules/REQUEST-920-PROTOCOL-ENFORCEMENT.conf
@@ -1130,7 +1130,7 @@ SecRule REQUEST_HEADERS_NAMES "@rx ^.*$" \
     severity:'CRITICAL',\
     setvar:'tx.header_name_%{tx.0}=/%{tx.0}/',\
     chain"
-    SecRule TX:/^HEADER_NAME_/ "@within %{tx.restricted_headers}" \
+    SecRule TX:/^header_name_/ "@within %{tx.restricted_headers}" \
         "setvar:'tx.anomaly_score_pl1=+%{tx.critical_anomaly_score}'"

Note that I did not test the new rule, but this case difference is a problem with modsec 3x
@CRS-migration-bot CRS-migration-bot added the 🐛 bug Something isn't working label May 13, 2020
@CRS-migration-bot
Copy link
Author

User airween commented on date 2020-04-22 12:35:26:

Hi mirkodziadzka-avi, thanks for the report. Yes, this is an "old" and "know" problem, and guess you saw the PR for modsec 3x :).

I'll review the whole rule set for all occurrence of TX:UPPER_CASE_VARIABLES, and make a PR for this issue - or feel free to make it.

Anyway (just for the sake of completeness), there is an another way to fix (but I'm sure your suggestion is more elegant and clear of course).

diff --git a/rules/REQUEST-920-PROTOCOL-ENFORCEMENT.conf b/rules/REQUEST-920-PROTOCOL-ENFORCEMENT.conf
index 880c8c4..666f59b 100644
--- a/rules/REQUEST-920-PROTOCOL-ENFORCEMENT.conf
+++ b/rules/REQUEST-920-PROTOCOL-ENFORCEMENT.conf
@@ -1130,7 +1130,7 @@ SecRule REQUEST_HEADERS_NAMES "@rx ^.*$" \
     severity:'CRITICAL',\
     setvar:'tx.header_name_%{tx.0}=/%{tx.0}/',\
     chain"
-    SecRule TX:/^HEADER_NAME_/ "@within %{tx.restricted_headers}" \
+    SecRule TX:/(?i)^HEADER_NAME_/ "@within %{tx.restricted_headers}" \
         "setvar:'tx.anomaly_score_pl1=+%{tx.critical_anomaly_score}'"

@CRS-migration-bot
Copy link
Author

User airween commented on date 2020-04-22 21:16:32:

First, there is a new PR which fixes this bug.

I did some research on how we use the TX variables in SecRule's.

There are 3 rule, where the variable contains a regex:

920450: https://github.com/SpiderLabs/owasp-modsecurity-crs/blob/cf57fd53de06b87b90d2cc5d61d602df81b2dd70/rules/REQUEST-920-PROTOCOL-ENFORCEMENT.conf#L1133-L1134

921180: https://github.com/SpiderLabs/owasp-modsecurity-crs/blob/cf57fd53de06b87b90d2cc5d61d602df81b2dd70/rules/REQUEST-921-PROTOCOL-ATTACK.conf#L296-L297

931130: https://github.com/SpiderLabs/owasp-modsecurity-crs/blob/cf57fd53de06b87b90d2cc5d61d602df81b2dd70/rules/REQUEST-931-APPLICATION-ATTACK-RFI.conf#L127-L128

Only this one (920450) where the variable name is with uppercase.

But there are many other occurrance of TX:VARIABLE, and in most cases the names are typed with uppercase too. Eg: TX:EXECUTING_PARANOIA_LEVEL, TX:PARANOIA_LEVEL, TX:INBOUND_ANOMALY_SCORE... Perhaps that's why the author created it like this.

I think using of this form contradicts the naming convention.

Any opinion?

I'm going to make a PR for this issue soon.

@CRS-migration-bot
Copy link
Author

User mirkodziadzka-avi commented on date 2020-04-28 09:03:42:

But there are many other occurrance of TX:VARIABLE, and in most cases the names are typed with uppercase too. Eg: TX:EXECUTING_PARANOIA_LEVEL, TX:PARANOIA_LEVEL, TX:INBOUND_ANOMALY_SCORE... Perhaps that's why the author created it like this.

Yes. But for normal access TX:foo and TX:FOO was always be the same (case insensitive). And modsec did not change the behaviour. As far as I know this is true for all collections. And I can see a reason why this is (was?) a good thing to do.

I can also find a reason why regexes should be case sensitive by default. Although I do not know if the change between modsec 2 and 3 is on purpose or by accident.

By the way, thanks for the fix

@CRS-migration-bot
Copy link
Author

User airween commented on date 2020-04-28 10:09:32:

Yes. But for normal access TX:foo and TX:FOO was always be the same (case insensitive). And modsec did not change the behaviour. As far as I know this is true for all collections. And I can see a reason why this is (was?) a good thing to do.

sure, that's no problem, it works as well.

I can also find a reason why regexes should be case sensitive by default. Although I do not know if the change between modsec 2 and 3 is on purpose or by accident.

I just found a short text about this behavior in documentation:

Variable names are case-insensitive.

So because it doesn't matter the variable is with lower or uppercase, if the rule references that with a regex, there also no matter - I assume this is an accident.

By the way, thanks for the fix

You're welcome.

This was referenced May 13, 2020
Closed
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
🐛 bug Something isn't working
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
1 participant
@CRS-migration-bot
0