From c0a7906147f6a7e2ca6b17a080cdb940ff1f7c0d Mon Sep 17 00:00:00 2001 From: Ervin Hegedus Date: Thu, 23 May 2019 13:00:38 +0000 Subject: [PATCH 1/3] Fix 2099 / ruleRemoveByTag --- src/rules.cc | 3 +++ 1 file changed, 3 insertions(+) diff --git a/src/rules.cc b/src/rules.cc index f5b16cab9a..64a8dde377 100644 --- a/src/rules.cc +++ b/src/rules.cc @@ -248,6 +248,9 @@ int Rules::evaluate(int phase, Transaction *t) { break; } } + if (remove_rule) { + continue; + } rule->evaluate(t, NULL); if (t->m_it.disruptive == true) { From ebdb36be9e5133f7988c88a3f9518989a2c5eb4e Mon Sep 17 00:00:00 2001 From: Ervin Hegedus Date: Thu, 23 May 2019 18:44:18 +0000 Subject: [PATCH 2/3] Added some test cases --- test/test-cases/regression/issue-2099.json | 195 +++++++++++++++++++++ 1 file changed, 195 insertions(+) create mode 100644 test/test-cases/regression/issue-2099.json diff --git a/test/test-cases/regression/issue-2099.json b/test/test-cases/regression/issue-2099.json new file mode 100644 index 0000000000..fff4aa4cc8 --- /dev/null +++ b/test/test-cases/regression/issue-2099.json @@ -0,0 +1,195 @@ +[ + { + "enabled":1, + "version_min":300000, + "title":"Testing ctl:ruleRemoveById - issue 2099", + "expected":{ + "http_code":200 + }, + "client":{ + "ip":"200.249.12.31", + "port":123 + }, + "request":{ + "headers":{ + "Host":"localhost", + "User-Agent":"curl/7.38.0", + "Accept":"*/*" + }, + "uri":"/remote.php/webdav?bar=foo", + "method":"GET", + "body": "" + }, + "server":{ + "ip":"200.249.12.31", + "port":80 + }, + "rules":[ + "SecRuleEngine On", + "SecRequestBodyAccess On", + "SecRule REQUEST_FILENAME \"@contains /remote.php/webdav\" \"id:9003100,phase:2,pass,t:none,nolog,ctl:ruleRemoveByTag=attack-injection-php,ctl:ruleRemoveById=941000-942999,ctl:ruleRemoveById=951000-951999,ctl:ruleRemoveById=953100-953130,ctl:ruleRemoveById=920420,ctl:ruleRemoveById=920440\"", + "SecRule ARGS \"@contains foo\" \"id:951001,phase:2,t:none,drop\"" + ] + }, + { + "enabled":1, + "version_min":300000, + "title":"Testing ctl:ruleRemoveById against - issue 2099", + "expected":{ + "http_code":403 + }, + "client":{ + "ip":"200.249.12.31", + "port":123 + }, + "request":{ + "headers":{ + "Host":"localhost", + "User-Agent":"curl/7.38.0", + "Accept":"*/*" + }, + "uri":"/remote.php?bar=foo", + "method":"GET", + "body": "" + }, + "server":{ + "ip":"200.249.12.31", + "port":80 + }, + "rules":[ + "SecRuleEngine On", + "SecRequestBodyAccess On", + "SecRule REQUEST_FILENAME \"@contains /remote.php/webdav\" \"id:9003100,phase:2,pass,t:none,nolog,ctl:ruleRemoveByTag=attack-injection-php,ctl:ruleRemoveById=941000-942999,ctl:ruleRemoveById=951000-951999,ctl:ruleRemoveById=953100-953130,ctl:ruleRemoveById=920420,ctl:ruleRemoveById=920440\"", + "SecRule ARGS \"@contains foo\" \"id:951001,phase:2,t:none,drop\"" + ] + }, + { + "enabled":1, + "version_min":300000, + "title":"Testing ctl:ruleRemoveByTag - issue 2099", + "expected":{ + "http_code":200 + }, + "client":{ + "ip":"200.249.12.31", + "port":123 + }, + "request":{ + "headers":{ + "Host":"localhost", + "User-Agent":"curl/7.38.0", + "Accept":"*/*" + }, + "uri":"/remote.php/webdav?bar=foo", + "method":"GET", + "body": "" + }, + "server":{ + "ip":"200.249.12.31", + "port":80 + }, + "rules":[ + "SecRuleEngine On", + "SecRequestBodyAccess On", + "SecRule REQUEST_FILENAME \"@contains /remote.php/webdav\" \"id:1000001,phase:2,pass,t:none,nolog,ctl:ruleRemoveByTag=attack-injection-php,ctl:ruleRemoveById=1100000-2100000,ctl:ruleRemoveById=9990000\"", + "SecRule ARGS \"@contains foo\" \"id:4400000,tag:'attack-injection-php',phase:2,t:none,msg:'test rule',drop\"" + ] + }, + { + "enabled":1, + "version_min":300000, + "title":"Testing ctl:ruleRemoveByTag against - issue 2099", + "expected":{ + "http_code":403 + }, + "client":{ + "ip":"200.249.12.31", + "port":123 + }, + "request":{ + "headers":{ + "Host":"localhost", + "User-Agent":"curl/7.38.0", + "Accept":"*/*" + }, + "uri":"/remote.php?bar=foo", + "method":"GET", + "body": "" + }, + "server":{ + "ip":"200.249.12.31", + "port":80 + }, + "rules":[ + "SecRuleEngine On", + "SecRequestBodyAccess On", + "SecRule REQUEST_FILENAME \"@contains /remote.php/webdav\" \"id:1000001,phase:2,pass,t:none,nolog,ctl:ruleRemoveByTag=attack-injection-php,ctl:ruleRemoveById=1100000-2100000,ctl:ruleRemoveById=9990000\"", + "SecRule ARGS \"@contains foo\" \"id:4400000,tag:'attack-injection-php',phase:2,t:none,msg:'test rule',drop\"" + ] + }, + { + "enabled":1, + "version_min":300000, + "title":"Testing ctl:ruleRemoveTargetByTag - issue 2099", + "expected":{ + "http_code":200 + }, + "client":{ + "ip":"1.2.3.4", + "port":123 + }, + "request":{ + "headers":{ + "Host":"localhost", + "User-Agent":"curl/7.38.0", + "Accept":"*/*" + }, + "uri":"/test.php?a=a", + "method":"GET", + "body": "" + }, + "server":{ + "ip":"200.249.12.31", + "port":80 + }, + "rules":[ + "SecRuleEngine On", + "SecRequestBodyAccess On", + "SecRule REQUEST_URI \"@contains /test.php\" \"id:100,phase:1,nolog,pass,ctl:ruleRemoveTargetByTag=attack-injection-php;ARGS:a,ctl:ruleRemoveTargetByTag=attack-rce;ARGS:a\"", + "SecRule ARGS \"@contains a\" \"id:4400000,tag:'attack-injection-php',phase:2,t:none,msg:'test rule',drop\"" + ] + }, + { + "enabled":1, + "version_min":300000, + "title":"Testing ctl:ruleRemoveTargetByTag against - issue 2099", + "expected":{ + "http_code":403 + }, + "client":{ + "ip":"1.2.3.4", + "port":123 + }, + "request":{ + "headers":{ + "Host":"localhost", + "User-Agent":"curl/7.38.0", + "Accept":"*/*" + }, + "uri":"/index.php?a=a", + "method":"GET", + "body": "" + }, + "server":{ + "ip":"200.249.12.31", + "port":80 + }, + "rules":[ + "SecRuleEngine On", + "SecRequestBodyAccess On", + "SecRule REQUEST_URI \"@contains /test.php\" \"id:100,phase:1,nolog,pass,ctl:ruleRemoveTargetByTag=attack-injection-php;ARGS:a,ctl:ruleRemoveTargetByTag=attack-rce;ARGS:a\"", + "SecRule ARGS \"@contains a\" \"id:4400000,tag:'attack-injection-php',phase:2,t:none,msg:'test rule',drop\"" + ] + } +] + From 5497b86e911fcf8e5195a8e87df25edc408d9219 Mon Sep 17 00:00:00 2001 From: Ervin Hegedus Date: Mon, 27 May 2019 14:49:13 +0000 Subject: [PATCH 3/3] Added plus condition to prevent the unnecessary loops --- src/rules.cc | 22 ++++++++++++---------- 1 file changed, 12 insertions(+), 10 deletions(-) diff --git a/src/rules.cc b/src/rules.cc index 64a8dde377..eff766f9c1 100644 --- a/src/rules.cc +++ b/src/rules.cc @@ -239,17 +239,19 @@ int Rules::evaluate(int phase, Transaction *t) { } } - for (auto &z : t->m_ruleRemoveByTag) { - if (rule->containsTag(z, t) == true) { - ms_dbg_a(t, 9, "Skipped rule id '" \ - + std::to_string(rule->m_ruleId) \ - + "'. Skipped due to a ruleRemoveByTag action."); - remove_rule = true; - break; + if (t->m_ruleRemoveByTag.empty() == false) { + for (auto &z : t->m_ruleRemoveByTag) { + if (rule->containsTag(z, t) == true) { + ms_dbg_a(t, 9, "Skipped rule id '" \ + + std::to_string(rule->m_ruleId) \ + + "'. Skipped due to a ruleRemoveByTag action."); + remove_rule = true; + break; + } + } + if (remove_rule) { + continue; } - } - if (remove_rule) { - continue; } rule->evaluate(t, NULL);