SpiderLabs / owasp-modsecurity-crs

OWASP ModSecurity Core Rule Set (CRS) Project (Official Repository)
https://modsecurity.org/crs
Apache License 2.0
2.45k stars 726 forks source link

Rule 920450 and modsec 3x #1741

Closed mirkodziadzka-avi closed 4 years ago

mirkodziadzka-avi commented 4 years ago

Regarding https://github.com/SpiderLabs/ModSecurity/issues/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

airween commented 4 years ago

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}'"
airween commented 4 years ago

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.

mirkodziadzka-avi commented 4 years ago

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

airween commented 4 years ago

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.