8000 fix(942100): remove multiMatch action by theMiddleBlue · Pull Request #2478 · coreruleset/coreruleset · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content

fix(942100): remove multiMatch action #2478

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.

Sign up for GitHub

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

Jump to bottom
Closed
wants to merge 1 commit into from
Closed

fix(942100): remove multiMatch action #2478

wants to merge 1 commit into from

Conversation

theMiddleBlue
Copy link
Contributor

from the ModSecurity wiki:

multiMatch
If enabled, ModSecurity will perform multiple operator invocations for every target, before and after every anti-evasion transformation is performed.

Action Group: Non-disruptive

Example:

SecRule ARGS "attack" "phase1,log,deny,id:119,t:removeNulls,t:lowercase,multiMatch"

Normally, variables are inspected only once per rule, and only after all transformation functions have been completed. With multiMatch, variables are checked against the operator before and after every transformation function that changes the input.

@theMiddleBlue theMiddleBlue added the 🐛 bug Something isn't working label Apr 6, 2022
@theMiddleBlue
Copy link
Contributor Author

seems that all tests for 942100 passed. any thoughts?

👉 executing tests in file 942100.yaml
	running 942100-1: ✔ passed in 7.072268ms
	running 942100-2: ✔ passed in 5.299051ms
	running 942100-3: ✔ passed in 5.571369ms
	running 942100-4: ✔ passed in 5.229046ms
	running 942100-5: ✔ passed in 4.710112ms
	running 942100-6: ✔ passed in 4.743414ms
	running 942100-7: ✔ passed in 5.44836ms
	running 942100-8: ✔ passed in 5.096438ms
	running 942100-9: ✔ passed in 4.815319ms
	running 942100-10: ✔ passed in 7.343286ms
	running 942100-11: ✔ passed in 4.880923ms
	running 942100-12: ✔ passed in 10.082467ms
	running 942100-13: ✔ passed in 5.153141ms
	running 942100-14: ✔ passed in 5.858088ms

@theMiddleBlue theMiddleBlue mentioned this pull request Apr 6, 2022
Closed
@dune73
Copy link
Member
dune73 commented Apr 6, 2022

If loginject does the lowercase itself, then there is maybe really no point in carrying it. Yet this is the workhorse of sqli detection, so this is a substantial risk for false negatives. So I'd really think twice, even if our tests seem to be limited in this regard.

@theseion
Copy link
Contributor
theseion commented Apr 6, 2022

... loginject ...

libinject?

@theseion
Copy link
Contributor
theseion commented Apr 6, 2022

I agree with @dune73. I would say, write a test case that requires multiMatch:

  • the test should be positive (contain a log entry) after t:removeNulls
  • the test should be negative (contain no log entry) after t:removeNulls,t:lowercase

This should show us whether libinjection requires multiMatch. Careful though, your example is obviously simple. But what about something like t:none,t:decodeBase64,t:removeNulls,t:lowercase,t:replaceComments? I'm pretty sure that libinjection can't handle that without multiMatch

@dune73
Copy link
Member
dune73 commented Apr 8, 2022

Added "needs action" label so it really stands out in the PR overview.

@dune73 dune73 mentioned this pull request May 2, 2022
Closed
@fzipi fzipi mentioned this pull request Jun 5, 2022
Closed
@dune73 dune73 mentioned this pull request Jul 4, 2022
Closed
@fzipi fzipi changed the title remove multiMatch action from 942100 fix(942100): remove multiMatch action Jul 31, 2022
@fzipi fzipi mentioned this pull request Aug 1, 2022
Closed
@fzipi fzipi mentioned this pull request Sep 5, 2022
Closed
@fzipi fzipi mentioned this pull request Oct 2, 2022
Closed
@theMiddleBlue
Copy link
Contributor Author

IIRC this was related to an old libmodsecurity bug (sort of owasp-modsecurity/ModSecurity#2573) and now seems to be fixed.

closing, feel free to reopen if needed

@theseion
Copy link
Contributor

Not sure why you had opened this PR. It seemed to me that you wanted to remove the overhead of multiMatch. This didn't have any thing to do with the logging bug, as far as I was aware. As the overhead of multiMatch is probably not that big of an issue ATM, I think closing this makes sense.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
🐛 bug Something isn't working 👀 Needs action
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@theMiddleBlue @dune73 @theseion
0