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

Complete merging of particular rule properties #1978

Conversation

defanator
Copy link
Contributor

@defanator
Copy link
Contributor Author

The PR may be incomplete, see
owasp-modsecurity/ModSecurity-nginx#142 (comment) (and below) for details.

@zimmerle zimmerle self-requested a review December 10, 2018 19:07
@zimmerle zimmerle self-assigned this Dec 10, 2018
@zimmerle zimmerle added the 3.x Related to ModSecurity version 3.x label Dec 10, 2018
@zimmerle
Copy link
Contributor

Hi @defanator,

Thank you for the report :) that is an interesting subject, because it lead us to a bigger discussion. Back on v2, some configuration items had o global scope even when placed inside a virtual host. That is annoying as hell, as it was counterintuitive. Reading the name of the configuration is/was not possible to tell (or to expect) when/how/if it is going to work.

There are some technical limitations that are important, for instance, very earlier on the request - when the vhost and/or the destination folder is not know - is the place that we want to threat slow dos. Therefore this particular configuration will no be able to target a single vhost, but the entire server.

That may not be the case of SecRequestBodyLimit. Yet, it is on the same set of the configurations class that will be good to be not-global. IMHO, the only configurations that should be global are the ones that can't be different due to technical limitations. Working towards it. On v3/dev/reload we are have a differentiation between what is a rule and what is a configuration.

As v3/dev/reload is not yet ready for production, let me see what can i do to circumvent this specific problem.

@defanator
Copy link
Contributor Author

@zimmerle in other words, a configuration like the one described in
owasp-modsecurity/ModSecurity-nginx#142 (comment) definitely should be considered as valid, right?

Let me know if you have any other concerns that may (or may not) be related to v3/dev/reload in this context (i.e. if that branch is going to be heavily refactored and will unlikely go into 3.0.4, I'd merge this one into v3/master as it fixes the initial issue reported at owasp-modsecurity/ModSecurity-nginx#142 - it wasn't about redefining values, it's about the fact that the documented behavior of SecRequestBodyLimit does not work entirely if modsecurity_rules_file is specified in top contexts).

zimmerle pushed a commit that referenced this pull request Dec 24, 2018
@zimmerle
Copy link
Contributor

Merged! Thanks :)

@zimmerle zimmerle closed this Dec 24, 2018
@defanator
Copy link
Contributor Author

@zimmerle oh, I was going to update the PR to do actual merges like here -
owasp-modsecurity/ModSecurity-nginx#142 (comment) - so I'll test this once again and submit a new one.

Also, here's the full list of directives which do not get merged:

SecRuleEngine
SecRequestBodyAccess
SecResponseBodyAccess
SecXmlExternalEntity
SecUploadKeepFiles
SecTmpSaveUploadedFiles
SecRequestBodyLimit
SecResponseBodyLimit
SecRequestBodyLimitAction
SecResponseBodyLimitAction
SecUploadFileLimit
SecUploadFileMode

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

Successfully merging this pull request may close these issues.

2 participants