Fix FP 953120 brotli compression #2101
Conversation
|
Meeting decision June (#2074 (comment)): |
| chain" | ||
| SecRule RESPONSE_HEADERS:Content-Encoding "!@streq br" \ | ||
| "setvar:'tx.outbound_anomaly_score_pl1=+%{tx.error_anomaly_score}',\ | ||
| setvar:'tx.anomaly_score_pl1=+%{tx.error_anomaly_score}'" |
There was a problem hiding this comment.
Would it be more efficient to reverse the rules? String equlity check should be much less costly than worst case regexp evaluation.
There was a problem hiding this comment.
Ah, that's smart. I had not thought of that.
I will switch that. Thank you!
There was a problem hiding this comment.
Actually, I am not so sure of that. Because the current first rule is really selective. It will fail most of the time. With the reverse order, we run the non-selective rule first and will have to run the 2nd rule almost always on top. So from the perspective of REQUEST_BODY regex inspection not much changed, but an additional rule is running.
There was a problem hiding this comment.
Yes, but it's a "not" regexp... and the response text could be massive, I suspect? The regexp would be applied to each line in the response body... (I'm guessing here...)
There was a problem hiding this comment.
Let's think about this: the negation isn't part of the regular expression but of the annotation. This means that the regular expression will be applied until it finds a match or the end of the input is reached, only then can the result be checked and negated (unless the engine does some magic on top of PCRE). So for every line of input the expression engine will have to check every branch of the expression against the entire line to check for a match, by eye it looks like there are about 3 top level branches. Even with optimizations, I think the number of operations will be greater than for the string equality check (there's also some overhead for parsing the expression or looking it up from the cache).
There was a problem hiding this comment.
What should I do??
There was a problem hiding this comment.
Well, in reality it probably won't matter too much. The performance gain in either case would be marginal. You choose the explanation that sounds best to you, I suggest ;).
|
Sorry for this taking so much time to me! Unfortunately, this solution cannot be used because it will create a way how to bypass rule When using a compression done by Apache web server (via mod_deflate for example), data are compressed just before sending them to the client i. e. How to reproduce bypass (for
|
|
Great catch! I also found your mention of Brotli detection issues: #1968. Basically, as long as we have no reliable way to detect Brotli (e.g. by magic number) we won't be able to do this as we have to assume that response headers can be manipulated, correct? Do you see any other way this could be solved? Or do we maybe need to change the paranoia level of this rule? |
|
Let's take this to the chat on Monday. It really is worth a discussion. |
|
Meeting decision July: #2141 (comment): |
|
Any updates on your tests here @azurit? |
|
@dune73 Sorry for takeing this so long to me, I'm working on it. |
|
I did lots of tests and i wasn't able to reproduce my previous behavior, where Anyway, while testing, i noticed that
This behavior is really weird so, please, anyone, test it too. |
|
when will this be merged? |
|
OK, I've spent some time on this and I am a bit at a loss. Thank you for taking a deep, deep look at this @azurit. What I can say is this: When Apache runs mod_brotli, then there is no Content-Encoding header in phase 4, when this rule happens. The CE header is only added by mod_brotli after ModSec phase 4. So this PR effectively kills the rule for this scenario. If we want to create a workaround for this FP, then I reckon it ought to be a separate rule that sets a variable that we can then check in a chain or something along those lines. I reckon we also need a decent way to reproduce the FP in question here. We need a src text that triggers 953120 when compressed with brotli. And then a NGINX with brotli, so we can really test it. Apparently the header setting in the phases is different there according to @fl0ppy-d1sk in #2064. Maybe also time to re-open said issue. |
|
why isn't there magic bytes for brotli, seriously.. @dune73 , I understand, until then I've just added the filter locally based on content-Encoding, It worked, am using NGINX. |
|
I also wonder why that is the case. Probably in order to save space... You came across these FPs on NGINX, I think. Can you share a payload that leads to an FP when crunched with brotli? |
|
Thank you very much @fl0ppy-d1sk. This is very helpful. It's probably the rainbow hair, though. :) |
|
-> We talked about this at the August project meeting but we did not really come up with a solution. @spartantri suggested we might have to do a rule exclusion package for brotli, that way users could enable the rule exclusion package when they face this situation. That would be better than going something that is on by default and can lead to rule bypasses. I'm inclined to close the PR and continue the conversation in issue #2064. Thoughts? |
|
I still wonder why is |
|
Sorry for being late. I wanted to try this out, but now I realize it's far easier. It does not evaluate to What you can do to catch this is Please also note that you can play around with the headers by using mod_headers with the flag |
|
We cannot use |
|
Would such a sibling solve our problem? Given this is a broetli-Problem, I think the plugin idea is worth considering. Conceptually a broetlli rule exclusion package. And then close this PR. Which is kind of sad given all the energy that went into this. |
|
Meeting decision Oct 4: In fact there is no way to really detect broetli and if broetli is executed on the RP, we won't really know and it's all a mess and despite all the work that went into the review of this PR I think we have to close this. @spartantri proposed to do this as a plugin. |
This PR solves a false positive with Content-Type: br responses by adding a chained rule as discussed in the related issue #2064.
I didn't test this PR and I would be glad if someone else could review and maybe test the chained rule.