owasp-modsecurity / ModSecurity-nginx

ModSecurity v3 Nginx Connector
Apache License 2.0
1.48k stars 274 forks source link

Question about conflicting modsecurity_rules/modsecurity_rules_file precedence #280

Open mac-chaffee opened 2 years ago

mac-chaffee commented 2 years ago

This question is in relation to some troubles the downstream ingress-nginx project has with modsecurity: https://github.com/kubernetes/ingress-nginx/issues/8388

When conflicting modsecurity rules/settings are loaded, how is that conflict resolved? Is it always "last write wins"? Seems that's not always the case.

For example, say I have the following config:

modsecurity on;
modsecurity_rules 'SecRuleEngine On';
modsecurity_rules_file /etc/nginx/modsecurity/modsecurity.conf;
  # modsecurity.conf includes the line: "SecRuleEngine DetectionOnly"
modsecurity_rules_file /etc/nginx/owasp-modsecurity-crs/nginx-modsecurity.conf;
  # Just Includes all the coreruleset rules files

If "last write wins", I would expect that modsecurity_rules 'SecRuleEngine On'; would do nothing; the subsequent line would override that and set the mode to DetectionOnly. But for me, the mode stays "On" with the above config and requests get blocked instead of just logged. However other users have reported the opposite behavior as well: https://github.com/kubernetes/ingress-nginx/pull/8021

Looking at the source code, I would expect that "last write wins" is the intended behavior: https://github.com/SpiderLabs/ModSecurity/blob/5519f6cfae45a47d8dc3ac1b084319611a9b386b/headers/modsecurity/rules_set_properties.h#L45-L48

But are there situations where "last write wins" doesn't hold?

martinhsv commented 2 years ago

Hi @mac-chaffee ,

I'd have to take a more in-depth look to provide a definitive answer, but a similar-sounding question arose at least once before ( https://github.com/SpiderLabs/ModSecurity-nginx/issues/183 ).

The hypothesis in that case was that using two different nginx directives (i.e. both 'modsecurity_rules' and 'modsecurity_rules_file') may result in the precedence of those directives being dominant.

Based on that earlier issue, if you were to use either 'modsecurity_rules' or 'modsecurity_rules_file' in both of those cases, that would likely give you the 'last wins' behaviour that you are expecting.

mac-chaffee commented 2 years ago

'modsecurity_rules' and 'modsecurity_rules_file' have precedence of their own outside of just the order in which they're listed in nginx.conf? Just skimming the code, it looks like those directives both just edit the same RulesSet struct:

https://github.com/SpiderLabs/ModSecurity-nginx/blob/2497e6ac654d0b117b9534aa735b757c6b11c84f/src/ngx_http_modsecurity_module.c#L310-L375

Which calls these functions:

https://github.com/SpiderLabs/ModSecurity/blob/b89c737ad3f20cccfe966631b28157f3b5d25832/src/rules_set.cc#L294-L311

mac-chaffee commented 2 years ago

Oh I do see they are changing mmcf->rules_inline vs. mmcf->rules_file, so maybe there is a difference, hmm

EDIT: oh that's just a counter, nvm I should stop guessing haha

liudongmiao commented 2 years ago

it will just merge.

martinhsv commented 2 years ago

Anything more on this?

mac-chaffee commented 2 years ago

Yes, sounds like we haven't identified exactly what should take precedence over what.

It would be good to document this somewhere because the prevailing wisdom is incorrect: that last writer always wins and they will "just merge".

In my experiments, it seems that modsecurity_rules does take precedence over modsecurity_rules_file, but as I mentioned other users have reported the opposite. So there's also the possibility there could be a bug in the code that makes the order inconsistent.

We'd need a firm answer on how rule precedence works in modsecurity-nginx before documenting anything I think.

martinhsv commented 2 years ago

So your issue is resolved? -- perhaps by using either of those directives both times and not mixing?

mac-chaffee commented 2 years ago

If mixing modsecurity_rules/modsecurity_rules_file is supported, then my question is still unanswered. But if you're saying mixing modsecurity_rules/modsecurity_rules_file isn't supported, I'd think we should definitely document that.

I'd be happy to contribute documentation and inform the ingress-nginx folks (they mix directives too), just not sure what to tell them.

martinhsv commented 2 years ago

Hi @mac-chaffee ,

I didn't say it isn't supported. I was only suggesting:

At the moment, I cannot really devote time to investigating a more in-depth answer. I did hope, however, to help you deal with your immediate issue.

pasikarkkainen commented 1 year ago

Any new info here? What's the expected behaviour with both "modsecurity_rules" and "modsecurity_rules_file" options in use, as that's what you easily end up with when using k8s ingress-nginx modsecurity features and config options..