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

Fix vulnerable regexp in rule 942330 #1361

Open
wants to merge 1 commit into
base: v3.2/dev
from

Conversation

Projects
None yet
6 participants
@s0md3v
Copy link

commented Apr 16, 2019

  • Remove duplicate \ from character group
  • Resolve intersecting alternate patterns

Fixes #1359 .

Mitigate ReDOS Vulnerability (Fixes #1359)
- Remove duplicate `\` from character group
- Resolve intersecting alternate patterns
@spartantri

This comment has been minimized.

Copy link
Collaborator

commented Apr 16, 2019

what about the extra couple \ in \\\\x(?:23|27|3d) at the end? are those required?

@s0md3v

This comment has been minimized.

Copy link
Author

commented Apr 16, 2019

@spartantri That really depends on what you want to match.

[cb]at will match both cat and bat. If we add another b to the character group [cbb], it will still behave the same and that's the reason the duplicate \\ was removed.

On the other hand, \\\\x tries to match the string \\x literally. The person who wrote this rule might know about an attack vector which contains \\x at this point, you better ask that person.

@spartantri

This comment has been minimized.

Copy link
Collaborator

commented Apr 16, 2019

yes don't worry, I left it as open question so all other guys can look at it, to me \x would be enough but I don't know who wrote this.

@fgsch

This comment has been minimized.

Copy link
Collaborator

commented Apr 16, 2019

Have you actually reproduced this issue with modsecurity?

@fgsch

This comment has been minimized.

Copy link
Collaborator

commented Apr 16, 2019

Also the regexp is generated from the file mentioned in the comment.

@s0md3v

This comment has been minimized.

Copy link
Author

commented Apr 16, 2019

@fgsch The vulnerability exists in the rule, not mod_security. mod_security won't spend more than 1500 milliseconds while matching a string against a rule.

@fgsch

This comment has been minimized.

Copy link
Collaborator

commented Apr 16, 2019

@s0md3v Yup, I appreciate that. We should try to fix the patterns, but my point is that we should distinguish between "this is actually exploitable" from "this should be fixed but won't be exploitable because there are mitigations in place".

@s0md3v

This comment has been minimized.

Copy link
Author

commented Apr 16, 2019

@fgsch I am sorry if I am wrong but isn't this rule set being used by other WAFs then mod_security?
If yes, what if they don't have a timeout on matching? Or what if they are letting the request pass if it times out?
This rule set is a third party kind of thing and hence prevention of exploitation of it's vulnerable components shouldn't depend on the program using it as a dependency.

@fgsch

This comment has been minimized.

Copy link
Collaborator

commented Apr 16, 2019

@s0md3v That's fair, and I agree we should fix it.
I'm not sure if the project officially supports its usage outside modsecurity though.

@s0md3v

This comment has been minimized.

Copy link
Author

commented Apr 16, 2019

Well I just made an assumption after reading this in the README

The OWASP ModSecurity Core Rule Set (CRS) is a set of generic attack detection rules for use with ModSecurity or compatible web application firewalls.

Anyways, let me know if you have any concern about the patches.

Also, thanks for contributing such great stuff to the open source community, stay hydrated ❤️

@s0md3v

This comment has been minimized.

Copy link
Author

commented Apr 23, 2019

These changes do not affect the functionality of Mod Security, why these pull requests haven't merged yet?

@fgsch

This comment has been minimized.

Copy link
Collaborator

commented Apr 23, 2019

@s0md3v as I said earlier:

Also the regexp is generated from the file mentioned in the comment.

Also please be patient. This is a volunteer driven project and people might be (and often is) busy.

@emphazer

This comment has been minimized.

Copy link
Collaborator

commented Apr 24, 2019

@s0md3v im pretty sure that it will be merged after the next monthly monday meeting. the next is at 6. mai i think. you can be definetly sure that if the tests are positive and the project members agree that we will merge it before the final v3.2 release.
and your name with PR title will appear in the changelog

feel free to join us at the meeting

@airween

This comment has been minimized.

Copy link

commented Apr 24, 2019

Note, that the double \ is justifiable (in normal case). In version 2 (aka mod_security v2) the rules read by Apache, which strips the strings (I think this routine does it).

That's why it would be better to check the rules through ModSecurity, not with the regex browser.

If the admin merges this request, the affected rule will not work (in v2 - but will right in v3) - hope the regression text will catch it.

@csanders-git

This comment has been minimized.

Copy link
Collaborator

commented Apr 24, 2019

@airween , i'm a bit lost as to the regression you are discussing, can you give an example?

@airween

This comment has been minimized.

Copy link

commented Apr 24, 2019

Sure :).

@fgsch

This comment has been minimized.

Copy link
Collaborator

commented Apr 26, 2019

Tests are failing:

AH00526: Syntax error on line 936 of /etc/apache2/modsecurity.d/owasp-crs/rules/REQUEST-942-APPLICATION-ATTACK-SQLI.conf:
Error creating rule: Error compiling pattern (offset 414): unmatched parentheses
@s0md3v

This comment has been minimized.

Copy link
Author

commented Apr 26, 2019

@fgsch I didn't touch line 936

@fgsch

This comment has been minimized.

Copy link
Collaborator

commented Apr 26, 2019

It is reporting line 936 because that's where the rule ends, but the error is caused by this change.
In fact, it is due to the removal of the \.

@fgsch fgsch changed the title Mitigate ReDOS Vulnerability (Fixes #1359) Vulnerable regexp in rule 942330 Apr 29, 2019

@fgsch fgsch changed the title Vulnerable regexp in rule 942330 Fix vulnerable regexp in rule 942330 Apr 29, 2019

@fgsch fgsch changed the title Fix vulnerable regexp in rule 942330 Fix vulnerable regexp in rule 942490 Apr 29, 2019

@fgsch fgsch changed the title Fix vulnerable regexp in rule 942490 Fix vulnerable regexp in rule 942430 Apr 29, 2019

@fgsch fgsch changed the title Fix vulnerable regexp in rule 942430 Fix vulnerable regexp in rule 942330 Apr 29, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.