Skip to content

V3/collection re fix #2018

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

Closed

Conversation

airween
Copy link
Member

@airween airween commented Feb 5, 2019

Looks like the Regex::searchAll() method is not case insensitive (InMemoryPerProcess::resolveSingleMatch() needs that). This bug detected with help of CRS regression test 920450.

Also fix (temporary) the 'ovector' limit bug, which triggered when the regex contains too much brackets, eg. in CRS rule 942130.

@victorhora victorhora added this to the v3.0.4 milestone Feb 8, 2019
@victorhora victorhora added the 3.x Related to ModSecurity version 3.x label Feb 8, 2019
@airween
Copy link
Member Author

airween commented Feb 11, 2019

Could somebody re-run the CI build? Looks like it was failed (by Travis):

InstalledDir: /Applications/Xcode-9.4.1.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/bin
before_script.1
0.06s$ echo $TRAVIS_OS_NAME
osx
$ [ "$TRAVIS_OS_NAME" != osx ] || brew update
==> Downloading https://homebrew.bintray.com/bottles-portable-ruby/portable-ruby-2.3.7.mavericks.bottle.tar.gz
######################################################################## 100.0%
==> Pouring portable-ruby-2.3.7.mavericks.bottle.tar.gz
==> Homebrew is run entirely by unpaid volunteers. Please consider donating:
  https://github.com/Homebrew/brew#donations


No output has been received in the last 10m0s

Thanks :)

@victorhora
Copy link
Contributor

hey @airween, I've triggered a rebuild on the failed build. We should have the results in a moment.

Thanks for your contribution, me or @zimmerle will have a chance to review it soon :)

@zimmerle zimmerle self-requested a review February 12, 2019 12:18
@zimmerle
Copy link
Contributor

zimmerle commented Feb 12, 2019

Merged ea937cf and 640daa4. I do have something that I want to discuss about 3334d2a and 4e1b99a.

The regular expression implementation is self-contained in the regex class. It means that this is the only class which effectively depends on libpcre*. Within this patch, the definition PCRE_CASELESS will be spread among other classes making them also dependent on pcre. As of now, it won't be a problem, but, as we aiming to support different regular expression backends, we better avoid this scenario. I would rather prefer to have a new signature to the method -- considering the PCRE_CASELESS flag only inside the regex class or even a modificator on the regular expression string.

  • VerifyCC it is an exception and considered to be a bug to be fixed.

@WGH-
Copy link
Contributor

WGH- commented Feb 14, 2019

You can make a regular expression case insensitive by prefixing it with (?i). It seems to be supported almost everywhere (I checked libpcre, Python re, RE2, Hyperscan).

@airween
Copy link
Member Author

airween commented Feb 14, 2019

Hi @WGH-, thanks, I know it - but the problem is not that.

The problem is, the comparing variables on collections (in-memory and LMDB) must be case insensitive.

This condition is not met.
See this PR, and the documentation.

BTW, the ModSec2 follows this way... so I think we have to align the ModSec3 too.

@@ -50,8 +50,7 @@ class SMatch {

class Regex {
public:
explicit Regex(const std::string& pattern_);
explicit Regex(const std::string& pattern_, int flags);
explicit Regex(const std::string& pattern_, bool caseSensitive = false);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You may not need this to be explicit anylonger.

@zimmerle
Copy link
Contributor

@airween as commented by @WGH- the regular expression could be handled this with the ignore case "modificator". In any case, your pull request is currently holding four commits:

  • 8040904 - Removed pcre dependency from outside of regex class
  • 3334d2a - Regex search in collection data case insensitive fix
  • 4e1b99a - Fixed regex case insensitive search bug in LMDB
  • 640daa4 - Added test cases

In 8040904 you have fixed a problem added in 3334d2a and 3334d2a. Do you mind to change 3334d2a and 3334d2a to actually incorporate 8040904 ?

@airween
Copy link
Member Author

airween commented Mar 11, 2019

The 8040904 should ignore. Do I revoke it?

I know what @WGH- said and your idea, but the CRS contains regex's without modifications, and modsec2 works with it. I'ld like to follow that way, tha's why I sent this PR.

The other solution would be that all occurrences of necessary regex's need to add the modifier, and works of libmodsec3 should differ from ModSec2.

@airween airween closed this Jun 1, 2019
@airween
Copy link
Member Author

airween commented Jun 1, 2019

Closed this PR, and re-opened a new one here: #2107

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.

4 participants