Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Question about conflicting modsecurity_rules/modsecurity_rules_file precedence #280

Open
mac-chaffee opened this issue Apr 11, 2022 · 10 comments

Comments

@mac-chaffee
Copy link

mac-chaffee commented Apr 11, 2022

This question is in relation to some troubles the downstream ingress-nginx project has with modsecurity: kubernetes/ingress-nginx#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: kubernetes/ingress-nginx#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
Copy link
Contributor

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 ( #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
Copy link
Author

'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
Copy link
Author

mac-chaffee commented Apr 11, 2022

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
Copy link

it will just merge.

@martinhsv
Copy link
Contributor

Anything more on this?

@mac-chaffee
Copy link
Author

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
Copy link
Contributor

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

@mac-chaffee
Copy link
Author

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
Copy link
Contributor

Hi @mac-chaffee ,

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

  • a probable reason that you were experiencing a result that surprised you and
  • a way to avoid that surprising result (not mixing the two)

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
Copy link

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..

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants