Proposal for removing negative lookbehind from 920120 #2360
Conversation
920120 now checks for HTML entities and skips the next rule if it finds any. 920122 does the actual blocking, but first checks for ['\;"=]. Removed 920120-no-backtracking.data. Removed scripts and facilities for generating and dealing with negative lookbehind related expressions.
|
@theseion Thinking on the semantic changes in the rule: what happens when we have this payload? 🤔
I think this is why we need a chain... |
|
I didn't intend to change semantics. Did I? The current rule will already pass on |
|
Nice find @fzipi! Because of the inverted logic we now have to check both variables separately. Also, we need to match the entire string. |
|
Also checked against ReDos. |
🤔 I think that this works because the content is the same in FILES and FILES_NAMES. Can those be different? A chain will be a logical AND of those rules. |
|
Yeah... I don't like that... Funny story, after thinking about it some more I realised that the solution to the problem is In conclusion, the trick to removing negative lookahead is to require a match on the inverted expression. |
|
I've been playing with this in regex101... and looks like it works. I'm puzzled also! Nice finding 🎉 Just for the sake of future generations, do you think we should document this in the util/regexp-assemble/data/920120.data file, or maybe in that wiki page about technical decisions? |
|
If you aim for the clean solution, then it's the wiki page and the data file links to the right section of the wiki page. |
|
The PR currently does a ton of additional changes. Is that a basing problem? Also: With the recently discovered solution, @theseion, did you update the PR already? |
|
Looks like there are additional changes, but it is just good cleaning up. It is not a basing problem. This is the last and only rule that uses negative lookbehind, so there is the removal of the preprocessor and all comments related to it in multiple files. The "real" change is the rule, the regexp, and tests (lets say the usual changes). And yes, the proposed change is there in rules/REQUEST-920-PROTOCOL-ENFORCEMENT.conf:104. |
|
Got it. Thanks. |
Added test for empty values Improved documentation on 920120
|
I've made a small adjustment to the expression to allow empty values (this matches the previous behaviour). I also cleaned up the comments (and tests, thanks @fzipi) and added a link to the new wiki page I set up. @dune73 @RedXanadu I went ahead and created a new page https://github.com/coreruleset/coreruleset/wiki/Technical-Decisions-and-Best-Practices (linked to from Home). It will probably make sense to split the two topics "technical decisions" and "best practices" at some point but I couldn't quite decide which of the two I was writing. |
There was a problem hiding this comment.
I am really happy with the results here.
The only drawback is that we are indeed relying on modsecurity "trick", but this is preferable to the lookbehind itself, IMHO.
There is a good wiki article about why we did this.
I would like to get additional approvals also.
|
👋 Sorry for the late response. This looks good but I'm a bit concerned it touches 53 files.
Let me know your thoughts and whether I can help with any of this. |
|
Thanks @fgsch. You're right, that does make sense. I'll split this up later today. |
|
Closed in favor of the following four PR's (in order):
|
* Added additional tests for 920120 Related to #2360
920120 now checks for HTML entities and skips the next rule if it finds
any.
920122 does the actual blocking, but first checks for [';"=].
Removed 920120-no-backtracking.data.
Removed scripts and facilities for generating and dealing with negative
lookbehind related expressions.
The rule numbering is currently experimental. I need some guidance on how to go forward (there's a PL2 rule with ID 920121). The rule comment should also be updated to explain the additional rule.
There's also the issue that the test file (920120.yaml) now actually tests two rules and looks for 920122 in the log. Not bad per se but it clashes somewhat with the naming of the test file.
This PR follows up on #2334 and #2339, where I discovered that
!@rxis not a negative regular expression match but a "rule inversion" (docs: https://github.com/SpiderLabs/ModSecurity/wiki/Reference-Manual-%28v2.x%29#args_names).