internet.nl website test triggering OWASP CRS WAF

[ne20002]

I updated one of my own websites' Modsecurity WAF with the new Core Rule Set from OWASP. As I started to test it with the website test the test failed due to a rule violation:

This is the entry in the logfile:

2024/03/20 09:15:41 [info] 98#98: *14976 ModSecurity: Warning. Matched "OperatorGt' with parameter 50' against variableREQUEST_HEADERS:Accept-Encoding' (Value: compress, deflate, exi, gzip, pack200-gzip, x-compress, x-gzip' ) [file "/etc/modsecurity.d/owasp-crs/rules/REQUEST-920-PROTOCOL-ENFORCEMENT.conf"] [line "1207"] [id "920520"] [rev ""] [msg "Accept-Encoding header exceeded sensible length"] [data "62"] [severity "2"] [ver "OWASP_CRS/4.0.0"] [maturity "0"] [accuracy "0"] [tag "modsecurity"] [tag "application-multi"] [tag "language-multi"] [tag "platform-multi"] [tag "attack-protocol"] [tag "paranoia-level/1"] [tag "OWASP_CRS"] [tag "capec/1000/255/153"] [tag "PCI/12.1"] [hostname "10.0.2.100"] [uri "/"] [unique_id "171092254153.715809"] [ref "v585,62t:lowercase,t:length"], client: 62.204.66.10, server: , request: "GET / HTTP/1.1", host: "fedi.****.ch"

The rule 920520 is defined as:

#
# Rule against CVE-2022-21907
# This rule blocks Accept-Encoding headers longer than 50 characters.
# The length of 50 is a heuristic based on the length of values from
# the RFC (https://datatracker.ietf.org/doc/rfc9110/)
# and the respective values assigned by IANA
# (https://www.iana.org/assignments/http-parameters/http-parameters.xml#content-coding).
#
# This rule has a stricter sibling: 920521
#
SecRule REQUEST_HEADERS:Accept-Encoding "@gt 50" \
    "id:920520,\
    phase:1,\
    block,\
    t:none,t:lowercase,t:length,\
    msg:'Accept-Encoding header exceeded sensible length',\
    logdata:'%{MATCHED_VAR}',\
    tag:'application-multi',\
    tag:'language-multi',\
    tag:'platform-multi',\
    tag:'attack-protocol',\
    tag:'paranoia-level/1',\
    tag:'OWASP_CRS',\
    tag:'capec/1000/255/153',\
    tag:'PCI/12.1',\
    ver:'OWASP_CRS/4.0.1-dev',\
    severity:'CRITICAL',\
    setvar:'tx.inbound_anomaly_score_pl1=+%{tx.critical_anomaly_score}'

It is that the Accept-Encoding header value from your test is violating the 50 characters limit.

[dune73]

Oopsie. That could be ours. But that Accept-Encoding header is rather crazy.

Let's say what they have to say. If they reject a failure on their side we may have to make the value 50 configurable.

[bwbroersma]

I agree this one is a bit exotic: https://github.com/internetstandards/Internet.nl/blob/a469e4c151c1740d3f69e36235bc854b0099004a/checks/tasks/http_headers.py#L733 It's to check if there is any compression on the HTTP layer, but it's also no longer current with the IANA registered content-encodings: br (Brotli - RFC 7932) and zstd (Zstandard Compression) are missing, so actually the header can be even longer!

Also I'm actually missing the option to use compression on the Transfer-Encoding level.

It might be a bit obscure to support all compression options (although in this case it's not supported, it's just to test if the server will use compression, to test for the option that BREACH attack might be an issue), but it's perfectly valid. So the WAF rule should allow for all the values (and some white space).

[ne20002]

If even a longer header is valid, maybe the rule should allow longer headers and maybe it should not be critical and block on paranoia level 1?

[dune73]

Thank you for that extensive explanation @bwbroersma. I understand your use case and I think it's a smart application of the header.

I'm willing to discuss this, but the use case is indeed so exotic I doubt CRS would adopt a change.

The RFC defines the header and this header is clearly within the definition of the header, so it is valid.

But CRS attempts to detect malicious traffic within the RFC definition. The length limit has never lead to any problem so far and I dare say this is the first occurrence of a false positive on this rule reported like ever.

If we change the limit, we make all other use cases of CRS less secure. And if we make it a configurable variable we slow down every HTTP requests on every CRS installation globally (this directly translates to a hard to quantify CO2 consumption). It is also very unlikely the default value is changed. So your users / your installation would have to reconfigure the default value anyways. And if you need to touch the installation, then you can also place a rule exclusion for this rule for this request.

I therefore see several courses of action for you:

• You convince the CRS project to update the rule
• You place a rule exclusion that disables this rule for this very request (in your installations), this could also be done via a 3rd party rule exclusion plugin; worthwhile if other false positives pop up
• You work around the false positive by serializing the request with the header into multiple requests with shorter header

[bwbroersma]

@dune73: to be honest, I don't like this kind of WAF behavior. I see benefit in protecting against malicious stuff being send to an application server, but blocking anything 'out of the ordinary' that is valid and passes a non-malicious check (e.g. no overflows, special chars, out of spec stuff etc.), should pass. Especially since in most cases Content-Encoding is done 'on-the-fly' (although Transport-Encoding was meant for this) by the webserver. I see too often that WAFs are used on static content sites that block non-default UA's (e.g. curl or request), resulting in actually that /.well-known/security.txt cannot easily be automatically fetched. I don't like this kind of behavior, I think blocking a valid case of Accept-Encoding should not be blocked (like blocking curl on sectxt). I know the behavior of WAFs all to well, and I do think it's beneficial to conform to some standards. That is why I proposed and changed (not yet deployed):

• 1224

The CRS rule should accept valid options, can the length limit be changed for only the Accept-Encoding header? Because I cannot see changing the limit on this header will make stuff more insecure. But again: I'm not a WAF salesman.

BTW who is 'you' in your option list? At first I thought you meant internet.nl (since the issue is in that repo), but now I read it again, the second question cannot be for internet.nl right?

[bwbroersma]

Let's reference https://github.com/coreruleset/coreruleset/pull/2357, they mention *, which of course would not be the same / as explicit. But could be an alternative, could be tested if it results in the same result, and will not return aes128gcm or identity. Opened issue to discuss this:

• 1352

[bwbroersma]

I did some testing, * cannot be used.

[bwbroersma]

The whole problem is the 50, I asked https://github.com/coreruleset/coreruleset/issues/3623#issuecomment-2010088617 how they ended up with 50.

[dune73]

I feel the conversation is heating up a bit and you start to bring in arguments like UA checks that have nothing to do with this. As the developer who personally ripped out most of the UA-checks (because of the very arguments you present above), I would rather stay on topic.

You have meanwhile found out and linked yourself, the arbitrary value of 50 was introduced to fight CVE-2022-21907. The value is a heuristic that we set based on our knowledge in complete ignorance of your special use case. We are a small project with very limited resources, so this is a path that we often use. When we see that a value causes too much problems, we adjust accordingly. Maybe you this is the case here. Maybe it's not.

I proposed three possible courses of action above. I do not see you consider option 2 and 3, so I expect your PR to change the default value or make it configurable.

[bwbroersma]

I do understand the goal here to protect against overflows. Internet.nl is trying to protect against compression on application level, and therefore being potentially vulnerable for the BREACH attack. I tried the alternative to shorten this header by using *, but that does not work in practice. Splitting the test would introducing an extra request for every website test, just to be conform with a scanner that does not respect the HTTP specification.

I don't think it's on the Dutch Internet Standards Platform to create PR's in other projects that are not RFC conformant. Internet.nl does not use this software nor raised the issue, I think I find the suggestion that Internet.nl needs to fix it unreasonable.

BTW even if I wanted it I wouldn't be able to fix it, since I would want to create a RFC conforming rule, that would properly validate the complete header (parse the ABNF), accepting any length.

[ne20002]

Interessting discussion.

As a user of both tools helping me to protect my servers and to configure and check my servers I have a slightly different position.

If the rule does have a reason as it is mitigating an attack vector (by limiting the accepted headers based on its length) it doesn't matter if the RFC allows longer headers. If this attack vector exists this has priority. And if it must be 50 and can't be 83 than this is ok.

But as I'm also a user of the internet.nl website check it is annoying for a user if the test fails due to this rule. At the moment I have disabled the rule but I don't feel very happy with this. And as said, the header used by internet.nl is exotic even though with a good reason.

I can handle this for myself but others might also trap into this situation (failing test). The best solution I see is really to split the test in internet.nl into two requests (or if you get a 403 on the request than retry with two requests with smaler headers). Just for the users.

I've looked into the CVE description and if I understand it correctly, this CVE only affects Windows Systems. So I'm happy with disabling the rule. As CRS does have plugins for certain applications, wouldn't it be possible to seperate relevant rules by backend server if the rules are only relevant for certain systems (this would also reduce CO2 consumption)?

[bwbroersma]

Indeed an optional split, only if there is a 403, would be a better than always splitting the request. However doing this would make the code more complex, and increasing code complexity to conform to some arbitrary limit set by one vendor is not a sustainable path to take. I asked in the CRS codebase about this limit. This rule was made to protect an unpatched Windows servers, if you can patch the system it's wise to use a rule to guard it, but it's always better to patch it. If you're completely aware of all backend versions you can probably disable a lot of CRS rules.

[dune73]

Very good. We're back on track with a fruitful discussion.

I see we need to balance the needs of the internet.nl security check failing on CRS-equipped targets and CRS creating over-zealous rules that try to protect the servers from mischief in the form of overlong HTTP headers.

@ne20002 : I do not think you need to disable the rule completely. You could limit the rule exclusion to the IP ranges internet.nl is using - , or to the path of the request in question. From a WAF perspective, this would be perfectly adequate. I see that internet.nl uses an IPv4 and an IPv6 address and requests "/" for the requests triggering CRS 920520. This is not very unique, but probably needed. Still, limiting the rule exclusion with to SecRule REQUEST_URI "@streq /" ... should do the trick.

Having to instruct your users to implement this to get decent results from the scanning service is not going to work of course.

But let's see if we can change the default value and what that could mean.

@franbuehler : you created CRS 920520 in PR https://github.com/coreruleset/coreruleset/pull/2357 against CVE-2022-21907. You have also set an arbitrary length of 50 bytes for the header. Internet.nl (think Dutch SSLLabs) currently sends an Accept-Encoding header with a length of 62 bytes, probably going to 83 bytes in the future.

Would these values still be an adequate defense or are we entering dangerous waters?

Let's bring in @fzipi as well. Maybe he has an additional perspective.

There is also an issue with using * in the header, but I did not investigate this from CRS side so far.

[bwbroersma]

I asked the same question in https://github.com/coreruleset/coreruleset/issues/3623, I thinks its better to discuss the CRS pick of 50 in that repo, instead of this one. Same goes for adding exceptions and the CO₂ discussions about adding rules to CRS.

In my view this issue (in this repo) can be closed.

[ne20002]

@dune73 If the rule is mitigating an existing attack vector only in Windows systems then disabling the rule is OK for me. In terms of resources (or CO2 ;)) I believe it would make sense to have a list of rules that are only relevant for certain systems.

[dune73]

That exists via tags and via rule files grouped by application / language.

People disable the 942 rules on systems without SQL for example.

This is not really perfect and certainly not for the protocol rules in 920 and 921 and given we are meant to be generic we never know if a protocol problem affecting Windows today will not hit another stack tomorrow in a similar way.

This is all very unfortunate and means a lot of blood and tears when running a WAF. But if you do a good job, then it's also blood and tears for the attackers.

[mxsasha]

This is a useful discussion, but I do think the rest belongs in https://github.com/coreruleset/coreruleset/issues/3623 instead. My general view is that internet.nl tries to do some non-standard things by design, and some WAF/IDS/etc may see us as a threat, which may affect test results. There is some shared responsibility here, as we do want our tests to work as well as possible, but we can not accommodate every case, and need to prioritize our efforts. For example, we do try to avoid email connection rate limiting. But we have recently rejected the request to make our user agent fully imitate a regular browser, though we are updating it to fit more common formatting. Also, we recently decided certain tests, like the compression test, do not have to support ancient SSL versions.

I do not think this rule is common enough for us to add the complexity of splitting the request into two when finding a 403. From our side, that closes the issue.

[baknu]

internet.nl tries to do some non-standard things by design

Small nuance: "internet.nl tries to do some uncommon (but still RFC's/standards compliant) things by design"

[bwbroersma]

Note, if https://datatracker.ietf.org/doc/html/draft-ietf-httpbis-compression-dictionary-03 becomes final, a pretty normal user can emit gzip, deflate, compress, zstd, br, zstd-d, br-d (47 chars).

https://github.com/coreruleset/coreruleset/issues/3636#issuecomment-2031698769 decided on 100 chars.