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

V3/collection re fix #2297

Closed

airween wants to merge 2 commits into owasp-modsecurity:v3/master from airween:v3/collection_re_fix2

Member

airween commented Apr 22, 2020 •

edited

This PR is the cleaned version of #2107.

I've updated my local branch and forked a new one from current v3/master. Contains only one necessary commit, changed the variable name for better readability.

Edit: this PR fixes the #2296.


          Regex search in collection data case insensitive fix

df9e453

This was referenced Apr 22, 2020

V3/collection re fix #2107

Closed

Rule 920450 and modsec 3x SpiderLabs/owasp-modsecurity-crs#1741

Closed

CRS-migration-bot mentioned this pull request

Rule 920450 and modsec 3x coreruleset/coreruleset#1741

Closed

Contributor

martinhsv commented Jul 3, 2020 •

edited

A couple of observations:

What if someone wanted to write a rule where they did indeed want the TX:/[regex-content]/ to be case sensitive? Wouldn't these code changes make that impossible?
Would your use case be satisfied by instead modifying the regex itself. For example in your first test use something like 'TX:/^(?i)HEADER_NAME_/ ' ?

Member Author

airween commented Jul 3, 2020

1. What if someone wanted to write a rule where they did indeed want the TX:/[regex-content]/ to be case sensitive?  Wouldn't these code changes make that impossible?

If someone wants to do that, how can he/she does it in case of mod_security2? The expected mode is case insensitive (but the documentation doesn't contain it).

I think if you want to implement this feature, it would be more correct to introduce some new syntax (and make the relevant code).

2. Would your use case be satisfied by instead modifying the regex itself.  For example in your first test use something like 'TX:/^(?i)HEADER_NAME_/ ' ?

Yes, that works as well - but you can see there are two issues about this problem (#2296, #1808) and one PR (#1810), so I think the users accustomed this form.

zimmerle added this to In progress in v3.1.0 via automation

zimmerle moved this from In progress to Backlog in v3.1.0

martinhsv moved this from Backlog to In progress in v3.1.0

Contributor

martinhsv commented Oct 22, 2020

First off, apologies for the delay.

I previously expressed some reservations about the change. I have put some more thought into this and am persuaded that this is the right thing to do.

A couple of reasons:

Given that exact names in the VARIABLES portion of a rule (e.g. ARGS:myid) are treated as case insensitive, making the regex equivalent (ARGS:/myid/) behave likewise is good for consistency
this change probably doesn't reduce user flexibility as much as I had feared. For example, in PCRE if a rule writer specifically wants to make the test case sensitive, one can prefix the main portion of the pattern with (?i-). (Not to mention that the cases where one has a compelling reason to do this are likely rare.)

(We also wanted to check that some futures features, such as supporting PCRE alternatives, would be unlikely to be hampered by this change.)

@zimmerle and I discussed this earlier today, and I don't think we have any further concerns.

This should get merged within a few days.

Member Author

airween commented Oct 22, 2020

This should get merged within a few days.

Thanks, it's good to read :).

zimmerle self-requested a review

October 23, 2020 17:57

zimmerle self-assigned this

zimmerle reviewed

View reviewed changes

Contributor

zimmerle left a comment

I have read the code and made some comments.

I am going the make the modifications by myself as this is going back and forth from a while. We want to have a fix for it.

Vide: #2297, #2296, and #2107.

@airween let me know if you have any doubts or concerns on my comments.

test/test-cases/regression/rule-920450.json Outdated

+                {
+                  "enabled":1,
+                  "version_min":300000,
+                  "title":"OWASP CRS id:920450",

Contributor

zimmerle Oct 23, 2020

I would prefer to have a title with a more meaningful message. If the test "OWASP CRS id:920450" fails, what does it means to the user?

Member Author

airween Oct 23, 2020

Changed in ceb0e3

test/test-cases/regression/rule-920450.json Outdated

+                {
+                  "enabled":1,
+                  "version_min":300000,
+                  "title":"OWASP CRS id:920450",

Contributor

zimmerle Oct 23, 2020

Same here.

Member Author

airween Oct 23, 2020

It's fixed too.

test/test-cases/regression/rule-920450.json Outdated Show resolved Hide resolved

Makefile.am Outdated

               TESTS+=test/test-cases/regression/rule-920120.json
               TESTS+=test/test-cases/regression/rule-920200.json
               TESTS+=test/test-cases/regression/rule-920274.json
+              TESTS+=test/test-cases/regression/rule-920450.json

Contributor

zimmerle Oct 23, 2020

I would expect a different name for that file. Explained below.

Member Author

airween Oct 23, 2020

Fixed.

test/test-cases/regression/rule-920450.json Outdated

@@ @@ -0,0 +1,87 @@ @@
+              [
+                {
+                  "enabled":1,

Contributor

zimmerle Oct 23, 2020

Since this is a pull request that was well described in a issue i miss the github_issue tag.

As listed -
https://github.com/SpiderLabs/ModSecurity/blob/785958f9b5089b918c7d054cbcc2fe4a3c7b3788/test/test-cases/regression/issue-960.json#L7-L9

Member Author

airween Oct 23, 2020

Thanks for info. The last commit contains both 'url' and 'github_issue' keys, in both test cases.

Note: it would be good to make some wiki page about your expectations for contributors. I'm sure these keywords are part of the regression test structure, but after 17 commits I've met with them first.

test/test-cases/regression/rule-920450.json Outdated

+                  },
+                  "rules":[
+                    "SecRuleEngine On",
+                    "SecRule &TX:restricted_headers \"@eq 0\" \"id:901165,phase:1,pass,nolog,setvar:'tx.restricted_headers=/proxy/ /lock-token/ /content-range/ /translate/ /if/'\"",

Contributor

zimmerle Oct 23, 2020

There is a value of having complex rules on the regression tests as it increases surface area for testing features. However, I am afraid that together with the complex example, we need to have a very simple use case scenario. If/When this test fails, it was a consequence of a problem on the selection of variables using regular expression or elsewhere (e.g. setvar)?

Member Author

airween Oct 23, 2020

I changed the rules in the new regression test file. I based @michaelgranzow-avi test cases, but there are only two cases, with same context: only the TX variable name format differ.

test/test-cases/regression/rule-920450.json Outdated

+                  },
+                  "request":{
+                    "headers":{
+                      "TRANSLATE": "test"

Contributor

zimmerle Oct 23, 2020

I would prefer to use real request headers, to mimic a real world example.

Member Author

airween Oct 23, 2020

Changed.

test/test-cases/regression/rule-920450.json Outdated

+                  },
+                  "rules":[
+                    "SecRuleEngine On",
+                    "SecRule &TX:restricted_headers \"@eq 0\" \"id:901165,phase:1,pass,nolog,setvar:'tx.restricted_headers=/proxy/ /lock-token/ /content-range/ /translate/ /if/'\"",

Contributor

zimmerle Oct 23, 2020

Same rule complexity issue explained below.

Member Author

airween Oct 23, 2020

Fixed.

test/test-cases/regression/rule-920450.json Outdated

+                      "no need."
+                    ]
+                  },
+                  "expected":{

Contributor

zimmerle Oct 23, 2020

Specially because the rules are complex, this test case could fail silent... as a combination of two (or more) failures. Better to also considers the debug and error log (when possible).

Member Author

airween Oct 23, 2020

I changed the test cases: both of them block the request, and gives the error_log, which used in "expected" block.

test/test-cases/regression/rule-920450.json Outdated

+                      "no need."
+                    ]
+                  },
+                  "expected":{

Contributor

zimmerle Oct 23, 2020

Same comment above. Use error and debug logs.

Member Author

airween Oct 23, 2020

Fixed too.


          Replace regression test as it expected

ceb0e38

Member Author

airween commented Oct 23, 2020

@airween let me know if you have any doubts or concerns on my comments.

See my answers and the new commit: ceb0e38.

zimmerle moved this from In progress to Needs review in v3.1.0

zimmerle added a commit that referenced this pull request


          Makes regular expression selection on collections key case insensitive

910a187

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 moved this from Needs review to Under QA in v3.1.0

Contributor

zimmerle commented Nov 30, 2020

Closing this as consequence of 910a187. Thanks everybody involved on this issue resolution.

zimmerle closed this

v3.1.0 automation moved this from Under QA to Done

zimmerle added a commit that referenced this pull request


          Makes regular expression selection on collections key case insensitive

f18595f

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


          Makes regular expression selection on collections key case insensitive

101504a

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