Join GitHub today
GitHub is home to over 28 million developers working together to host and review code, manage projects, and build software together.Sign up
Introducing an additional monitoring or hybrid paranoia level setting #1076
This implements #1050.
We have an issue with this PR and rule 944400. Said PL4 rule in phase 1 changes the engine behaviour and can thus trigger new alerts at PL1 in phase 2.
A comment next to 944400 implies this rule is meant to be moved to 901. But regardless, we have constructed a behaviour change in PL4 that affects PL1 rules and this collides with this PR.
@spartantri : Can you elaborate a bit on 944400, please?
I tested this PR and it works as advertised!
Here are my tests:
Test with a lower monitoring_paranoia_level than paranoia_level:
Test with PL=2, monitoring PL=2 and a high inbound threshold:
Test with PL=2, monitoring PL=4 and a high threshold:
Test with PL=2, monitoring PL=4 and a low threshold of 1:
Test with PL=2, monitoring PL=3 and a low threshold of 1:
I even tested it on a real service and it worked too! This means higher paranoia levels appeared in the log!
I thought a long time about correct naming here.
Right now we use the terms Paranoia Mode and Paranoia Level. Technically, there is no Paranoia Mode, there is just a default PL and the higher PLs.
So we have the variable
How about calling it the Execution Paranoia Level because the rules at this level are being executed.
Sorry for the delay on giving any feedback, I had several trips in America and Europe and a full agenda so I had no time to check PR's, I will try to fix the issues found by @franbuehler in 914 rules by end of week.
Rule 944400 is not java specific is something generic that's why it should be moved to 901 to my point of view. Now about the phase, as this rule is meant to check if there is already a body processor loaded and if there is none then force urlencoded as default engine and the creation of a request_body variable it is mandatory to run in phase:1. This rule is meant to execute only in PL4 because it will cause that potentially many requests that are currently ignored to be inspected.
For this particular PR, what can circumvent the monitoring paranoia from executing it is modify the rule as shown in the proposed rule at the end to check if the paranoia level is set to 4 to execute BUT that will partially defeat the purpose of monitoring paranoia and you may have the surprise of additional non previously seen rules being triggered. We can create PL5 and move 944400 to 901500 which means inspect everything no matter if it is a text file which currently is a big time rule bypass if the app behind does not handle CT properly or if it get reflected back to user or somewhere else without setting it to text.
We should add a warning somewhere that the perf impact of running monitoring paranoia = 4 or paranoia = 4 are very close even in PL=1 if MPL=4.
It maybe useful for slow/old/loaded/low memory servers to add a performance booster on RESPONSE-999-EXCLUSION-RULES-AFTER-CRS.conf and remove the entire PL at configuration time instead of runtime (
Proposed 944400 change:
Hi @spartantri, thank you for your consideration. I took a lot of time to think this through. And it seems we are between a rock and a hard place with this rule 944400.
You mention that having this rule defeats the idea of the monitoring paranoia levels. The reason being it brings a change to the behavior of the engine. Enable a higher monitoring PL and you start to see new FPs at a lower PL. We can tell people in the release notes / small print of the documentation, but nobody will read it and they will bump into production problems and blame our project.
The way the rule exists now (and after your proposed update) the rule has a troubling behavior too, because you suddenly start to see alerts in lower PLs when you enable PL4. I think this should have been discussed before the merge. Sorry for not speaking up back then, I did not notice and I do not know who reviewed this formally.
The rule changes the behavior of the engine via changing the request body processor. It picks one when there is none defined. This functionality is thus very close to the ModSecurity recommended rules. I do not know how many FPs this rule brings (and I did not check), so I am unsure if it could be added to said rules by default. Alternatively, it could be added to the recommended rules in a commented out form and people would enable it when they feel like high-security. If we would follow this path, then I think our PL4 documentation should highlight this important option in the recommended rules.
If we do not want to throw it over to the ModSec project, we could also enable it via a special lever / option. It would be disabled by default. You get an option to enable it in
As far as I can see it is one of very few rules that introduces a state / behavior change of the engine. I like to think of CRS as body of rules that does not have any effect on rules outside CRS. Encapsulated and no unforseen side effects. "Principle of least astonishment" if you know the term.
I fear 944400 breaks with this behavior / principle.
So here are the options I see:
None of this is ideal, I agree. Yet the rule has some merits and we need a solution. Personally, I opt to go with option 4 and trigger option 3 too. Then, a few years down the lane when people have updated their recommended rules, we remove it from CRS in favor of the recommended rules.
@spartantri: You also touch on performance on monitoring PL in the last two paragraphs of your comment. That's true and it warrants a remark in the documentation in
This was referenced
May 30, 2018
Previous comments on performance?
I've done several updates:
What I do not get is @spartantri's proposal to suggest adding
Other than that, I think this PR is ready for a final review and hopefully a merge.