Modsecurity: v3/master: ruleRemoveTargetByTag works only on exact string match
Hi,
referring to https://github.com/SpiderLabs/owasp-modsecurity-crs/issues/1444 opened by @j0k2r it seems that in the v3/master there's a bug on ruleRemoveTargetByTag. It removes the target only if the specified tag matches exactly a rule tag. In v2 it removes the target even if the specified tag is contained in a tag string of a rule.
example:
test rules:
SecRule ARGS:foo "@rx ^bar$" "id:999,\
phase:1,\
pass,\
log,\
msg:'arg foo contains bar, removing tag CRS',\
ctl:ruleRemoveTargetByTag=CRS;ARGS:z"
SecRule ARGS:z "@rx ^xxx$" "id:1000,\
phase:1,\
block,\
log,\
msg:'arg z contains xxx',\
tag:'OWASP_CRS/WEB_ATTACK/TEST'"
test request:
curl -v 'http://localhost/?foo=bar&z=xxx'
on v3:
both rules match and the request is blocked
[155956900159.171422] [/?foo=bar&z=xxx] [4] (Rule: 999) Executing operator "Rx" with param "^bar$" against ARGS:foo.
[155956900159.171422] [/?foo=bar&z=xxx] [9] Target value: "bar" (Variable: ARGS:foo)
[155956900159.171422] [/?foo=bar&z=xxx] [9] Matched vars updated.
[155956900159.171422] [/?foo=bar&z=xxx] [9] Saving msg: arg foo contains bar, removing tag CRS
[155956900159.171422] [/?foo=bar&z=xxx] [4] Rule returned 1.
[155956900159.171422] [/?foo=bar&z=xxx] [9] Running action: log
[155956900159.171422] [/?foo=bar&z=xxx] [9] Saving transaction to logs
[155956900159.171422] [/?foo=bar&z=xxx] [9] Running action: auditlog
[155956900159.171422] [/?foo=bar&z=xxx] [9] Running action: status
[155956900159.171422] [/?foo=bar&z=xxx] [9] Running action: log
[155956900159.171422] [/?foo=bar&z=xxx] [9] Saving transaction to logs
[155956900159.171422] [/?foo=bar&z=xxx] [9] Running action: ctl
[155956900159.171422] [/?foo=bar&z=xxx] [4] Running (disruptive) action: pass.
[155956900159.171422] [/?foo=bar&z=xxx] [8] Running action pass
[155956900159.171422] [/?foo=bar&z=xxx] [4] (Rule: 1000) Executing operator "Rx" with param "^xxx$" against ARGS:z.
[155956900159.171422] [/?foo=bar&z=xxx] [9] Target value: "xxx" (Variable: ARGS:z)
[155956900159.171422] [/?foo=bar&z=xxx] [9] Matched vars updated.
[155956900159.171422] [/?foo=bar&z=xxx] [9] Saving msg: arg z contains xxx
[155956900159.171422] [/?foo=bar&z=xxx] [4] Rule returned 1.
[155956900159.171422] [/?foo=bar&z=xxx] [9] Running action: log
[155956900159.171422] [/?foo=bar&z=xxx] [9] Saving transaction to logs
[155956900159.171422] [/?foo=bar&z=xxx] [9] Running action: auditlog
[155956900159.171422] [/?foo=bar&z=xxx] [9] Running action: status
[155956900159.171422] [/?foo=bar&z=xxx] [4] Running (non-disruptive) action: tag
[155956900159.171422] [/?foo=bar&z=xxx] [9] Rule tag: OWASP_CRS/WEB_ATTACK/SQL_INJECTION
[155956900159.171422] [/?foo=bar&z=xxx] [9] Running action: block
[155956900159.171422] [/?foo=bar&z=xxx] [8] Marking request as disruptive.
[155956900159.171422] [/?foo=bar&z=xxx] [8] Running action deny
[155956900159.171422] [/?foo=bar&z=xxx] [9] Running action: log
[155956900159.171422] [/?foo=bar&z=xxx] [9] Saving transaction to logs
on v2:
just the rule 999 match and the request is not blocked
[Mon Jun 03 13:32:28.281279 2019] [:error] [pid 77:tid 139980241995520] [client 127.0.0.1:35802] [client 127.0.0.1] ModSecurity: Warning. Pattern match "^bar$" at ARGS:foo. [file "/etc/apache2/modsecurity.d/modsecurity.conf"] [line "234"] [id "999"] [msg "arg foo contains bar, removing tag CRS"] [hostname "localhost"] [uri "/"] [unique_id "XPUhbFVic4UT1IQXSsNGbQAAAEI"]
This makes the following CRS3 exclusion rule set incompatible with the current v3/master:
- rules/REQUEST-903.9002-WORDPRESS-EXCLUSION-RULES.conf
- rules/REQUEST-903.9001-DRUPAL-EXCLUSION-RULES.conf
- rules/REQUEST-903.9004-DOKUWIKI-EXCLUSION-RULES.conf
- rules/REQUEST-903.9003-NEXTCLOUD-EXCLUSION-RULES.conf
thanks!
theMiddleBlue
All 7 comments
thanks to @airween we've started to investigate more, we've got a regression test:
Regression test:
[
{
"enabled":1,
"version_min":300000,
"title":"Testing ctl:ruleRemoveTargetByTag - issue 2109",
"expected":{
"http_code":200
},
"client":{
"ip":"200.249.12.31",
"port":123
},
"request":{
"headers":{
"Host":"localhost",
"User-Agent":"curl/7.38.0",
"Accept":"*/*"
},
"uri":"index.php?foo=bar&z=xxx",
"method":"GET",
"body": ""
},
"server":{
"ip":"200.249.12.31",
"port":80
},
"rules":[
"SecRuleEngine On",
"SecRequestBodyAccess On",
"SecRule ARGS:foo \"@rx ^bar$\" \"id:999,phase:1,pass,log,msg:'arg foo contains bar',ctl:ruleRemoveTargetByTag=CRS;ARGS:z\"",
"SecRule ARGS:z \"@rx ^xxx$\" \"id:1000,phase:1,deny,status:403,log,msg:'arg z contains xxx',tag:'OWASP_CRS/WEB_ATTACK/TEST'\""
]
}
]
Executing test:
ModSecurity 3.0.3 - tests
(options are not available -- missing GetOpt)
# File Name Test Name Passed?
--- --------- --------- -------
1 issue-2109.json Testing ctl:ruleRemoveTargetByTag - issue 2109 failed!
Test failed. From: test-cases/regression/issue-2109.json.
Test name: Testing ctl:ruleRemoveTargetByTag - issue 2109.
Reason:
HTTP code mismatch. expecting: 200 got: 403
Debug log:
[155957567397.400054] [] [4] Initializing transaction
[155957567397.400054] [] [4] Transaction context created.
[155957567397.400054] [] [4] Starting phase CONNECTION. (SecRules 0)
[155957567397.400054] [] [9] This phase consists of 0 rule(s).
[155957567397.400054] [] [4] Starting phase URI. (SecRules 0 + 1/2)
[155957567397.400054] [index.php?foo=bar&z=xxx] [4] Adding request argument (GET): name "foo", value "bar"
[155957567397.400054] [index.php?foo=bar&z=xxx] [4] Adding request argument (GET): name "z", value "xxx"
[155957567397.400054] [index.php?foo=bar&z=xxx] [4] Starting phase REQUEST_HEADERS. (SecRules 1)
[155957567397.400054] [index.php?foo=bar&z=xxx] [9] This phase consists of 2 rule(s).
[155957567397.400054] [index.php?foo=bar&z=xxx] [4] (Rule: 999) Executing operator "Rx" with param "^bar$" against ARGS:foo.
[155957567397.400054] [index.php?foo=bar&z=xxx] [9] Target value: "bar" (Variable: ARGS:foo)
[155957567397.400054] [index.php?foo=bar&z=xxx] [9] Matched vars updated.
[155957567397.400054] [index.php?foo=bar&z=xxx] [9] Saving msg: arg foo contains bar
[155957567397.400054] [index.php?foo=bar&z=xxx] [4] Rule returned 1.
[155957567397.400054] [index.php?foo=bar&z=xxx] [9] Running action: log
[155957567397.400054] [index.php?foo=bar&z=xxx] [9] Saving transaction to logs
[155957567397.400054] [index.php?foo=bar&z=xxx] [9] Running action: ctl
[155957567397.400054] [index.php?foo=bar&z=xxx] [4] Running (disruptive) action: pass.
[155957567397.400054] [index.php?foo=bar&z=xxx] [8] Running action pass
[155957567397.400054] [index.php?foo=bar&z=xxx] [4] (Rule: 1000) Executing operator "Rx" with param "^xxx$" against ARGS:z.
[155957567397.400054] [index.php?foo=bar&z=xxx] [9] Target value: "xxx" (Variable: ARGS:z)
[155957567397.400054] [index.php?foo=bar&z=xxx] [9] Matched vars updated.
[155957567397.400054] [index.php?foo=bar&z=xxx] [9] Saving msg: arg z contains xxx
[155957567397.400054] [index.php?foo=bar&z=xxx] [4] Rule returned 1.
[155957567397.400054] [index.php?foo=bar&z=xxx] [4] Running (non-disruptive) action: tag
[155957567397.400054] [index.php?foo=bar&z=xxx] [9] Rule tag: OWASP_CRS/WEB_ATTACK/TEST
[155957567397.400054] [index.php?foo=bar&z=xxx] [9] Running action: status
[155957567397.400054] [index.php?foo=bar&z=xxx] [9] Running action: log
[155957567397.400054] [index.php?foo=bar&z=xxx] [9] Saving transaction to logs
[155957567397.400054] [index.php?foo=bar&z=xxx] [4] Running (disruptive) action: deny.
[155957567397.400054] [index.php?foo=bar&z=xxx] [8] Running action deny
[155957567397.400054] [index.php?foo=bar&z=xxx] [8] Skipping this phase as this request was already intercepted.
[155957567397.400054] [index.php?foo=bar&z=xxx] [9] Appending request body: 0 bytes. Limit set to: 0.000000
[155957567397.400054] [index.php?foo=bar&z=xxx] [4] Starting phase REQUEST_BODY. (SecRules 2)
[155957567397.400054] [index.php?foo=bar&z=xxx] [9] This phase consists of 0 rule(s).
[155957567397.400054] [index.php?foo=bar&z=xxx] [4] Starting phase RESPONSE_HEADERS. (SecRules 3)
[155957567397.400054] [index.php?foo=bar&z=xxx] [9] This phase consists of 0 rule(s).
[155957567397.400054] [index.php?foo=bar&z=xxx] [9] Appending response body: 0 bytes. Limit set to: 0.000000
[155957567397.400054] [index.php?foo=bar&z=xxx] [4] Starting phase RESPONSE_BODY. (SecRules 4)
[155957567397.400054] [index.php?foo=bar&z=xxx] [4] Response body is disabled, returning... 2
[155957567397.400054] [index.php?foo=bar&z=xxx] [4] Starting phase LOGGING. (SecRules 5)
[155957567397.400054] [index.php?foo=bar&z=xxx] [9] This phase consists of 0 rule(s).
[155957567397.400054] [index.php?foo=bar&z=xxx] [8] Checking if this request is suitable to be saved as an audit log.
[155957567397.400054] [index.php?foo=bar&z=xxx] [8] Checking if this request is relevant to be part of the audit logs.
[155957567397.400054] [index.php?foo=bar&z=xxx] [5] Audit log engine was not set.
[155957567397.400054] [index.php?foo=bar&z=xxx] [8] Request was relevant to be saved. Parts: 4430
Error log:
ModSecurity: Warning. Matched "Operator `Rx' with parameter `^bar$' against variable `ARGS:foo' (Value: `bar' ) [file "issue-2109.json"] [line "3"] [id "999"] [rev ""] [msg "arg foo contains bar"] [data ""] [severity "0"] [ver ""] [maturity "0"] [accuracy "0"] [hostname "200.249.12.31"] [uri "index.php"] [unique_id "155957567397.400054"] [ref "o0,3v18,3"]
[client 200.249.12.31] ModSecurity: Access denied with code 403 (phase 1). Matched "Operator `Rx' with parameter `^xxx$' against variable `ARGS:z' (Value: `xxx' ) [file "issue-2109.json"] [line "4"] [id "1000"] [rev ""] [msg "arg z contains xxx"] [data ""] [severity "0"] [ver ""] [maturity "0"] [accuracy "0"] [tag "OWASP_CRS/WEB_ATTACK/TEST"] [hostname "200.249.12.31"] [uri "index.php"] [unique_id "155957567397.400054"] [ref "o0,3v24,3"]
Ran a total of: 1 regression tests - 1 failed. 0 skipped test(s). 0 disabled test(s).
Thank you, @theMiddleBlue. That was o great catch!
In your regression test, you may want to narrow down to the resources to precisely what you want to test. For instance, there is no need to have SecRequestBodyAccess On nor pass, log,msg:'arg foo contains bar' as the test is just looking for the http return code. The main reason for that is to make it simple to read (and further to index).
I will have a look at the issue
zimmerle
on 3 Jun 2019
thanks @zimmerle!
in the meantime we're replacing each ctl:ruleRemoveTargetByTag=CRS with ctl:ruleRemoveTargetById=910000-999999 on CRS in order to include it in the upcoming 3.1.1
theMiddleBlue
on 4 Jun 2019
Hi @theMiddleBlue,
In fact, the match for a tag in v2 is a regular expression by default, that is why it matches partially a string.
When implemented on v3 we made it a string comparison; due to performance reasons, and, because there may not be a reasonable reason for it to be a regex.
Allow me to explain, instead of matches partially the tag we can simply split the data into multiple tags. Let's have the example below:
tag:Cavendish,Banana,Fruit
Instead, we can have:
tag:Cavendish,tag:Banana,tag:Fruit
Given the example above, let's have a look at the table below:
| Attempt to match | v2 | v3 |
| ------------- | ------------- | -------- |
| Banana | Match | Match |
| Banana,Fruit | Match | Not Match |
| , | Match | Not Match |
| Fruit | Match | Match |
| Fruit, | Not Match | Not Match |
| Cavendish, | Match | Not Match |
As you can see, the semantics of the match is cleaner on v3, making the life of the user easy and it also has the benefit of being a way faster to match. That was discussed back on the implementation phase but I don't think it was documented. Is that any use case that you think of that cannot fit on v3 design?
zimmerle
on 4 Jun 2019
thank you @zimmerle
Sorry, I didn't know about this different implementation between v2 and v3. What you explain looks good to me and I agree with you that splitting in multiple tags is better. When #2110 will be fixed on v3/master we can definitely switch from ctl:ruleRemoveTargetByTag=CRS to ctl:ruleRemoveTargetById.
This could be a good starting point to review all CRS tags and replace something like OWASP_CRS/WEB_ATTACK/SQL_INJECTION in three different tags.
thanks
theMiddleBlue
on 5 Jun 2019
The explanations are appreciated @zimmerle. I agree that the semantics are cleaner that way and the use of tags in CRS is not very clean. This has been on the 2-Do list for quite some time.
What is problematic is the many little changes of behaviour with v3 that are undocumented. When you stated it would be compatible, we assumed it would be compatible. To learn step by step that it is not compatible is painful.
dune73
on 7 Jun 2019
Hi, @dune73
The explanations are appreciated @zimmerle. I agree that the semantics are cleaner that way and the use of tags in CRS is not very clean. This has been on the 2-Do list for quite some time.
What is problematic is the many little changes of behavior with v3 that are undocumented. When you stated it would be compatible, we assumed it would be compatible. To learn step by step that it is not compatible is painful.
I understand. On that matter, I suggest you engage with version 3 development, participate in our Slack channel, so that you will be aware of those changes. Unfortunately, I don't have time to document every discussion and every change.
This particular behavior, for instance, is pretty obvious. Anyone who tries to use the function will notice if it is not working as expected, regardless of what to expect. It may not need to be documented.
zimmerle
on 17 Jun 2019
Related issues
GustavoKatel
路
6Comments
Daijobou
路
6Comments
mmojadad
路
3Comments
davidjrh
路
5Comments
SteffenAL
路
5Comments
Most helpful comment
Hi @theMiddleBlue,
In fact, the match for a tag in v2 is a regular expression by default, that is why it matches partially a string.
When implemented on v3 we made it a string comparison; due to performance reasons, and, because there may not be a reasonable reason for it to be a regex.
Allow me to explain, instead of matches partially the tag we can simply split the data into multiple tags. Let's have the example below:
Instead, we can have:
Given the example above, let's have a look at the table below:
| Attempt to match | v2 | v3 |
| ------------- | ------------- | -------- |
| Banana | Match | Match |
| Banana,Fruit | Match | Not Match |
| , | Match | Not Match |
| Fruit | Match | Match |
| Fruit, | Not Match | Not Match |
| Cavendish, | Match | Not Match |
As you can see, the semantics of the match is cleaner on v3, making the life of the user easy and it also has the benefit of being a way faster to match. That was discussed back on the implementation phase but I don't think it was documented. Is that any use case that you think of that cannot fit on v3 design?