Owasp-modsecurity-crs: Rule 921160 matches with empty lines and looks for non-existing HTTP headers

[mentalstring]

I'm new to mod_security so I hope I'm not missing something obvious... but I believe there's a couple of issues with the regex of rule 921160 (HTTP Header Injection Attack via payload (CR/LF and header-name detected).

SecRule ARGS_NAMES|ARGS|XML:/* "(?:\n|\r)+(?:\s+|location|refresh|(?:set-)?cookie|(X-)?(?:forwarded-(?:for|host|server)|host|via|remote-ip|remote-addr|originating-IP))\s*:"

The first issue: empty lines followed by just : will cause a FP. For example, it will match with the body of a POST that just contains:

"

:-)"

or, in other words, it will match without an actual (injected) HTTP header being present.

The second issue is the placement of parenthesis on the (X-)? part of the regex: I'm not completely familiar with this type of attack, is it intended to look for headers such as X-remote-addr or X-via? And, shouldn't that be a non-capturing?

This is on owasp-modsecurity-crs-3.0.2.

[dune73]

Thank you for reporting this @mentalstring.

The current dev-tree (v3.1/dev) has your second issue covered.

I have a hard time reproducing your first issue, though. My setup is not triggering it. Could be due to using v3.1/dev. Could you try with that tree, please?

[mentalstring]

@dune73 Thanks for the quick reply!

About the first issue. I tried with v3.1/dev and got the same result. I can trigger 921160 with the data below which has no (injected) HTTP header in it:

curl http://localhost --data "param1=%0A%0A%3A"

About the second issue, I see that on v3.1/dev the regex is slightly different, but the difference is only that the (X-)? becomes non-capturing. It still tries to match headers that I don't think were meant (eg: X-host, X-remote-ip, etc). I think only X-Forwarded-For, X-Forwarded-Host and X-Forwarded-Server are the only meant to have the X- prefix in there? But then again, X- being for non-standard headers I can be wrong, but I never seen the other combinations in the wild.

[dune73]

Confirm. The provided payload triggers 921160 at PL1. (Do not know why I could not trigger it myself, but I was in a hurry...)

The payload boils down to 2 <CR> followed by a colon.

It's quite a peculiar payload, but then this is paranoia level 1 and we try to avoid all FPs here. Also I wonder if this FP is not only a side-effect of the way the rule is written, since there actually is no header name. So I am confident we can avoid that.

As for the 2nd issue: I brushed that away too quickly. I take it the optional X- is applied to all the headers to make for an easier rule. Do you think we get additional FPs for that? If yes, we can reduce to the headers where an X- is actually used.

[mentalstring]

This is actually triggering fairly often to me as it's not uncommon for textareas to have lines starting with smileys in them (think blog comments, users editing their profile texts, etc).

As for the X-, I don't think it generates more FP — just that it's matching headers that don't seem to be used, ie, just the forwarded-* seems to need it.

Perhaps an updated version to address both issues could be something along the lines of:

(?:\n|\r)(?:\s*)?(?:location|refresh|(?:set-)?cookie|(?:x-)?(?:forwarded-(?:for|host|server))|host|via|remote-(?:ip|addr)|originating-ip)\s*:

But, again, I'm not familiar with this type of attack, so I don't know if both of those \s* are needed in there; for instance, a somewhat similar rule (921120) doesn't seem to have \s* between the header name and the colon.

In any case, I'll be happy if at least it doesn't trigger when no header name is present.

[dune73]

Thank you for that proposal. Having users come up with rule improvements themselves is just awesome!

We'll look into it and get back to you!

[dune73]

@franbuehler, would this be an issue you would enjoy writing a PR?

[spartantri]

Good report, I agree it is not rare to have users entering blank lines, plus the uppercase X- into a rule with t:lowercase will never get a hit, as this is not anchored it still work but will be good to fix it.
Also ARGS_NAMES could be removed as 921150 already check for [\n\r]

[franbuehler]

In my opinion, this FP has been solved.

In Commit https://github.com/SpiderLabs/owasp-modsecurity-crs/commit/0d5110dcebefa1b845a06635d267b20b03d378c7, ARGS has been replaced with ARGS_GET. The post body is thus not checked anymore.

The problem was, that the minimum payload \n\s: triggered rule 921160:

 :

In the RFC 2616 (https://tools.ietf.org/html/rfc2616#page-31) I can not see if a headers name can consist of spaces:

message-header = field-name ":" [field-value]
       field-name = token
       field-value = * (field-content | LWS)
       field-content = <the OCTET's making up the field-value
                        and consisting of either * TEXT or combinations
                        of token, separators, and quoted-string>

But I think spaces are not forbidden and therefore
(?:\n|\r)(?:\s*)?(?:location|refresh|(?:set-)?cookie|(?:x-)?(?:forwarded-(?:for|host|server))|host|via|remote-(?:ip|addr)|originating-ip)\s*:
instead of
(?:\n|\r)+(?:\s|location|refresh|(?:set-)?cookie|(?:x-)?(?:forwarded-(?:for|host|server)|host|via|remote-ip|remote-addr|originating-IP))\s*:
is not an option. But I am not sure.

I would close this issue at the moment without further adjustment of the rule, because:

• this reported FP was eliminated
• since then no more FP were reported
• we potentially create a gap in the rule, when empty header names are not checked

Can I close the issue with this reason or are there counterarguments?

[lifeforms]

Agreed @franbuehler IIRC we removed the POST content exactly because of this problem - Thanks for the detailed review!

[franbuehler]

Thank you @dune73 and @lifeforms for the confirmation. I closed the issue.