Update REQUEST-900-EXCLUSION-RULES-BEFORE-CRS.conf.example

[fardarter]

No description provided.

[RedXanadu]

Hi @fardarter. Can you provide some context for your pull request, please?

Are you trying to add rules that perform some sort of detection and add to the anomaly score in REQUEST-900-EXCLUSION-RULES-BEFORE-CRS.conf.example?

[fardarter]

Hi @fardarter. Can you provide some context for your pull request, please?

Are you trying to add rules that perform some sort of detection and add to the anomaly score in REQUEST-900-EXCLUSION-RULES-BEFORE-CRS.conf.example?

OK, let me give some more context. I'm trying to make this my rule:

SecRule REQUEST_HEADERS:Content-Type "!@rx ^[\w/.+-]+(?:\s?;\s?(?:action|fhirversion|boundary|charset|type|start(?:-info)?)\s?=\s?['\"\w.()+,/:=?<>@-]+)*$" \
 "id:10020, \
 phase:1, \
 block, \
 t:none,t:lowercase, \
 msg:'Illegal Content-Type header', \
 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/3.3.4', \
 severity:'CRITICAL', \
 setvar:'tx.anomaly_score_pl1=+%{tx.critical_anomaly_score}'"

Instead of the default at 920470. I need to permit the fhirversion as a param on content-type

The runtime override doesn't help because it matches on the inverse case, so I need to replace the rule.

I wasn't able to find any examples with operator replacement (@rx) on update, and didn't manage to get any of my fiddles working.

At this point I've actually just inserted a ruleset after initialisation, which I'm not sure is the best practice here. It's been difficult to find docs on best practices for this other than the comments in REQUEST-900-EXCLUSION-RULES-BEFORE-CRS.conf and RESPONSE-999-EXCLUSION-RULES-AFTER-CRS.conf.

Previously I had it in REQUEST-900-EXCLUSION-RULES-BEFORE-CRS.conf but it wasn't contributing to the anomaly score. This was a bit of a gotcha till I understood how those were set up.

Any advice/guidance welcome.

[franbuehler]

Thank you for this PR and explaining it.

We have multiple problems here and it's a bit tricky. Great you found the reason behind it.

First, like you already noticed, the rule 920470 uses a denylist (!@rx) instead of an allowlist.
Therefore we can not use the runtime exclusion in RESPONSE-999-EXCLUSION-RULES-AFTER-CRS.conf like you mentioned.

So correct solution, like you described, is:

1. To remove rule 920470 in RESPONSE-999-EXCLUSION-RULES-AFTER-CRS.conf with SecRuleRemoveById 920470
2. Copy and adapt rule 920470 to REQUEST-900-EXCLUSION-RULES-BEFORE-CRS.conf and change id to 9204700 for example.

But AFTER the rule 9204700 runs, the rule 901200 in REQUEST-901-INITIALIZATION.conf kicks in and sets the tx.inbound_anomaly_score_pl1 to 0 again.

Another solution to your problem is to put the new rule 9204700 into phase:2. This is a bit less complicated. And it works, I just tested it.

But we definitely have to document this somewhere.

@RedXanadu : Do you also think we have to document this special case when a new rule in phase:1 is added to REQUEST-900-EXCLUSION-RULES-BEFORE-CRS.conf and do you know where? Or do you even have a better solution than putting the rule into phase:2? Thank you.

[fardarter]

Hi @franbuehler. One suggestion I might make is reserving a rules file number (say 902) for post initialisation rules. I understand phase 1 to semantically be dealing with headers, so if I'm right there this is the correct phase?

I know i can set some of this stuff in the settings file, but it's much easier from an automation perspective to move files around rather than trying to comment/insert/uncomment in bash.

What do you think?

[franbuehler]

@dune73: I think you know this problem that a new rule at PL1 can not be added to REQUEST-900-EXCLUSION-RULES-BEFORE-CRS.conf because the the rule 901200 in REQUEST-901-INITIALIZATION.conf initializes the anomaly score to 0.

What do you think of adding it to our documentation (add rule to PL2) or making a new file 902... as the reporter suggested.

[fardarter]

Thanks @franbuehler. :)

Just as a note, it would be lovely if these files were highlighted in the docs more (not sure if the guidance isn't given or I just didn't find it), but I had to dig deep into some long blog posts to discover them and read the source comments.

[RedXanadu]

Personally, when I need to do something like this (an operator change) in production and a rule exclusion is not possible for some reason, I use a SecRuleRemoveById directive (as suggested above) and then add my modified rule after the CRS includes. So, a rule replacement.

As long as you're not using early blocking mode and it's a phase 1 rule then I think it should work fine. Like:

⋮
Include crs/crs-setup.conf
Include crs/rules/*.conf

SecRuleRemoveById 123456

# Add modified rule 123456
SecRule…
⋮

Alternatively, you could try writing a runtime rule exclusion to verify / check / validate specifically for your excluded situation. Something like:

SecRule REQUEST_HEADERS:Content-Type "@rx ^[\w/.+-]+(?:\s?;\s?(?:fhirversion)\s?=\s?['\"\w.()+,/:=?<>@-]+)*$" \
 "id:123456, \
 phase:1, \
 t:none,t:lowercase, \
 ctl:ruleRemoveById=920470"

[fardarter]

Personally, when I need to do something like this (an operator change) in production and a rule exclusion is not possible for some reason, I use a SecRuleRemoveById directive (as suggested above) and then add my modified rule after the CRS includes. So, a rule replacement.

As long as you're not using early blocking mode and it's a phase 1 rule then I think it should work fine. Like:

⋮
Include crs/crs-setup.conf
Include crs/rules/*.conf

SecRuleRemoveById 123456

# Add modified rule 123456
SecRule…
⋮

Alternatively, you could try writing a runtime rule exclusion to verify / check / validate specifically for your excluded situation. Something like:

SecRule REQUEST_HEADERS:Content-Type "@rx ^[\w/.+-]+(?:\s?;\s?(?:fhirversion)\s?=\s?['\"\w.()+,/:=?<>@-]+)*$" \
 "id:123456, \
 phase:1, \
 t:none,t:lowercase, \
 ctl:ruleRemoveById=920470"

Wouldn't it need to run prior to the BLOCKING-EVALUATION rules to be additive to that?

Do you have a reason against including it at 902?

Also, noob question, but what is early blocking mode and how do I know if I'm running it?

[dune73]

This is a peculiar problem and a tricky workaround.

I never work with rule exclusion files 900 and 999, but my technique (-> REs are always in webserver config, not hidden in a file) would bump into the same problem. I tend to use the variant proposed by @franbuehler with the additional rule. I usually place it in phase 1, but after the include. That would correspond to the 999 file; but in phase 1.

A clean solution could indeed be the move 900 to 902. One would need to look at it in detail though, because some REs people have written probably depend on 901 not being executed yet.

Also wondering wether this pecular RE / rule replacement would not look better as a plugin. foobar-plugin-after.conf; phase 1.

[fardarter]

What I've done is made a ruleset REQUEST-902-EXCLUSION-RULES-BEFORE-CRS-BUT-AFTER-INITIALIZATION.conf.

Probably not a good idea to move 900 for breaking consumers, as you say, but also 900 gives one an opportunity to interfere with settings before init, which also seems like something someone might want to do. Again, files are much nicer than uncommenting for automation.

So what I'd suggest is preserve both locations for user consumption but with the 900 file stating that it should only be user to override settings or operate statelessly whereas any interactions with state that should precede initialisation but which should be evaluated for blocking should go in 902. What do you think?

I'm not familiar with the plugin system. I'd suggest keeping it as simple as possible, but I don't know what plugins should do.

What I would say is that something like this would might make a good case study for the docs. What do you think?

[fardarter]

While I have anyone's attention, do you know if the rules order on nginx is still unstable? I've seen a few blogs say for nginx (not nginx+, I'm compiling this myself) saying that one has to declare the includes in order versus using the wildcard(* -- as opposed to apache) but I've not been able to validate that yet (though I imagine load logs might be in the debug logs?).

[fardarter]

Hi all. Wondering if anyone considered my suggestion. Would like to set up in an authorised way.

[dune73]

I think the wildcard problem with Include on nginx is solved.

As far as your change is concerned. It might be a good idea, or it might not be a good idea. Hard to tell and it would take a very deep analysis to know for sure and I get the feeling none of the developers is particularly interested in the question. This may look or sound a bit unfriendly, but the situation is that your problem is acknowledged, but the devs do not consider it big enough to invest into it. Honestly, we have far bigger problems with CRS4 drawing closer and other big changes.

So maybe this PR will just fade, maybe somebody picks it up and merges it eventually, or maybe somebody else picks it up, rewrites into something different and then it gets merged. Please do not be offended, it's just the way open source projects work.

[fardarter]

No offence taken. Not particularly concerned about the PR anymore.

More just wondering if it's possible to put a simple proposition to vote: That the 902 range be reserved for userspace.

Using a file there seems to me to be the cleanest solution but if it isn't policy then my deployment could break down the line in a fairly invisible way for other maintainers.

Haven't seem any reasons against being offered in the above discussion. All it would really need is a note on the docs to make it real.

[fzipi]

@dune73 @RedXanadu What do you think we should do here?

[dune73]

I think the least we can do is document it as an edge case and propose one or two workarounds, for example the one by @fardarter and the one by @RedXanadu which is very close to @franbuehler's and what I suggested above.

@RedXanadu is this something you could provide?

If yes, we can then close this PR in favor of the documentation change.

@fardarter : We are planning to replace the ModSec recommended rules after CRSv4 came out in favor of our own version. This might be a moment where we look at CRS initialization in a holistic way and we would then also try and solve this problem for good. It's really quite hairy if we want to solve this without breaking anything and also try and avoid other pitfalls in this direction. Thank you for your understanding above.

[fardarter]

@dune73 If I'm right about the semantics, this does rightly belong in phase 1?

In general my request is only that it's well specified (so that anyone coming later will know what it is) and can be easily achieved by copying a file into a location (for automation).

[dune73]

It does belong into phase 1. It is supposed to be early, but not so early as to be overwritten later by CRS initialization. Or CRS initialization should make sure it is not overwriting existing default values (as it correctly does for anomaly thresholds etc).

We have the same problems and limitations with plugins btw.

A clean and intuitive solution would be big step forward.

[RedXanadu]

@dune73 Yes, I can add this corner case as a 'Known Issue'.

I'll include @franbuehler's suggestion and also @fardarter suggestion of adding a new file to the running order like '902-CUSTOM-RULES-AFTER-INIT' etc.

[dune73]

Thank you very much indeed.

[RedXanadu]

In order to resolve this pull request, we revisited and discussed the overall issue in our team chat this evening. (The full archive of the discussion will be available very soon from here: https://github.com/coreruleset/project-chat-archive/).

The decisions made were as follows:

1. The documentation issue (Add a new known issue for replacement phase 1 rules documentation#79) will remain open. It is still the plan to have a re-think about this problem and to address it after the release of CRS 4.0.
2. We are not planning to make any immediate changes, and certainly not before the release of CRS 4.0. As such, this specific PR will be closed (with the issue above staying open to address the problem described here).