V3/collection re fix #2107
Closed
V3/collection re fix #2107
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Closed
victorhora
added
3.x
|
IMO this bug is not a duplicate as the other bug does not involve regular expression search of variables, but this one does. |
|
|
||
| m_pc = pcre_compile(pattern.c_str(), PCRE_DOTALL|PCRE_MULTILINE, | ||
| if (caseSensitive == true) { |
There was a problem hiding this comment.
The name of the variable is wrong: we add the CASELESS flag if caseSensitive is true. Either keep the name, pass false in the invocations of the constructor changed above and test for !caseSensitive here; or change the name to ignoreCase.
As written it will confuse countless readers :(
There was a problem hiding this comment.
Thank for your remark - you're right.
Because this PR contains so many unnecessary commit, and this name convention is really illogical, I created a new one - see #2297.
Now I close this PR. Thanks for all helps to everybody.
Closed
|
Closing this PR, reopened in #2297. |
zimmerle
added a commit
that referenced
this pull request
Nov 30, 2020
This issue was initially reported by @michaelgranzow-avi on #2296. @airween made an initial attempt to provide a fixed at #2107; As a consequence of the pull request review - provided by @victorhora, @zimmerle, and @michaelgranzow-avi - @airween made a second attempt at #2297. After reviewing by @martinhsv, @zimmerle, I have absorbed the essential pieces from @airween patch into this one. This patch differs from @airween's because @airween's patches were partially working: Key exclusions with regex weren't covered, same for anchored variables (e.g. ARGS). During the review, I have highlighted the importance of having elementary test cases. A simple test case on ARGS could spot the issue. Since that is an important fix, I don't want to hold this for one more review cycle; therefore, I am committing the fix myself. Thank you all involved in the solution of this very own issue.
zimmerle
added a commit
that referenced
this pull request
Dec 10, 2020
This issue was initially reported by @michaelgranzow-avi on #2296. @airween made an initial attempt to provide a fixed at #2107; As a consequence of the pull request review - provided by @victorhora, @zimmerle, and @michaelgranzow-avi - @airween made a second attempt at #2297. After reviewing by @martinhsv, @zimmerle, I have absorbed the essential pieces from @airween patch into this one. This patch differs from @airween's because @airween's patches were partially working: Key exclusions with regex weren't covered, same for anchored variables (e.g. ARGS). During the review, I have highlighted the importance of having elementary test cases. A simple test case on ARGS could spot the issue. Since that is an important fix, I don't want to hold this for one more review cycle; therefore, I am committing the fix myself. Thank you all involved in the solution of this very own issue.
vladbukin
pushed a commit
to vladbukin/ModSecurity
that referenced
this pull request
Apr 12, 2022
This issue was initially reported by @michaelgranzow-avi on owasp-modsecurity#2296. @airween made an initial attempt to provide a fixed at owasp-modsecurity#2107; As a consequence of the pull request review - provided by @victorhora, @zimmerle, and @michaelgranzow-avi - @airween made a second attempt at owasp-modsecurity#2297. After reviewing by @martinhsv, @zimmerle, I have absorbed the essential pieces from @airween patch into this one. This patch differs from @airween's because @airween's patches were partially working: Key exclusions with regex weren't covered, same for anchored variables (e.g. ARGS). During the review, I have highlighted the importance of having elementary test cases. A simple test case on ARGS could spot the issue. Since that is an important fix, I don't want to hold this for one more review cycle; therefore, I am committing the fix myself. Thank you all involved in the solution of this very own issue.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Looks like the Regex::searchAll() method is not case insensitive (InMemoryPerProcess::resolveSingleMatch() needs that). This bug detected with help of CRS regression test 920450.
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.
I'ld like to apply this patch to v3/master, because I found here, and I think the v3.1 regex module is different.