coreruleset
fix: adding missing tags and ver action
Adding missing tag OWASP_CRS to (almost) all rules so it's all disabled when using ctl:ruleRemoveByTag=OWASP_CRS.
Also adding missing ver action to (almost) all rules.
Not sure if tag OWASP_CRS should be added also to rules used for skipping paranoia levels and anomaly score reporting rules in RESPONSE-980-CORRELATION.conf.
I'll do a review tomorrow. Is there maybe a reason that not all rules have the tag yet? @dune73?
Apparently linter is not checking this. I see 3 tags missing from detection rules. That's clearly a bug.
I think the policy was to only tag the detection rules. The theory being if you want everything removed, you can use rule-remove-by-id for the entire range.
I do not remember the arguments for said discussion though. Maybe it's just annoying if everything in crs-setup.conf plus all the plugins ought to be tagged.
There's a potential performance penalty using a tag instead of a simple rule range, IIRC. But I don't think we have any hard numbers on it. It could be significant or hardly measurable. And almost certainly different between ModSecurity v2 and v3.
Also, I've personally disabled CRS using the OWASP_CRS tag in the past, so… :shrug:
Hold on, can we talk this through before we change the policy here?
I am the 3 detection rules getting the tag is important. But most of the other rules but not the skipping rules is a bit arbitrary.
I think that adding the tag and version to all rules makes sense. If you disable by tag then there's no point in running the initialisation rules or the correlation rules. Often, the initialisation rules will have already been executed, so they won't actually be removed, but that's beside the point. I did not come across a situation, where removing any of the rules would lead to unexpected behaviour (for example, the URL body processor would be disabled and rules relying on it being enabled would no longer work; this is an unlikely scenario because the rules are usually disabled in order to let a benign request pass, so there's no point in checking it anyway).
I agree with you mostly. But then the PR should include tagging the skipping rules too.
And the change is so big I think it could do with discussing it on Monday in the meeting. Maybe we are overlooking something.
So we agreed to implement this at the last meeting.
I removed the do-not-merge label an will now merge. Thanks for the PR @azurit.
@dune73 As far as i rememeber, we agreed to add OWASP_CRS tag also to PL skipping rules (and all other rules, including crs-setup). I was planning to complete this PR but have not time for it yet. Should i open a new one?