From 1b1fdc055b8071ad3b24573abfe9b96e546c7abf Mon Sep 17 00:00:00 2001 From: martinhsv <55407942+martinhsv@users.noreply.github.com> Date: Wed, 5 Feb 2020 06:57:01 -0800 Subject: [PATCH] Fix rule-update-target exclusions for plain (non-regex) variables --- CHANGES | 2 + src/rule.cc | 4 +- src/variables/variable.h | 6 +-- .../config-update-target-by-id.json | 48 +++++++++++++++++-- .../config-update-target-by-tag.json | 40 ++++++++++++++++ 5 files changed, 92 insertions(+), 8 deletions(-) diff --git a/CHANGES b/CHANGES index b020d30c..38d76dcc 100644 --- a/CHANGES +++ b/CHANGES @@ -1,6 +1,8 @@ v3.x.y - YYYY-MMM-DD (to be released) ------------------------------------- + - Fix rule-update-target for non-regex + [Issue 2251 - @martinhsv] - Fix configure script when packaging for Buildroot [Issue 2235 - @frankvanbever] - modsecurity.pc.in: add Libs.private diff --git a/src/rule.cc b/src/rule.cc index 117d1fd4..15603373 100644 --- a/src/rule.cc +++ b/src/rule.cc @@ -697,7 +697,7 @@ bool Rule::evaluate(Transaction *trans, const std::string &value = v->getValue(); const std::string &key = v->getKeyWithCollection(); - if (exclusion.contains(v->getKey()) || + if (exclusion.contains(v) || std::find_if(trans->m_ruleRemoveTargetById.begin(), trans->m_ruleRemoveTargetById.end(), [&, v, this](std::pair &m) -> bool { @@ -708,7 +708,7 @@ bool Rule::evaluate(Transaction *trans, v = NULL; continue; } - if (exclusion.contains(v->getKey()) || + if (exclusion.contains(v) || std::find_if(trans->m_ruleRemoveTargetByTag.begin(), trans->m_ruleRemoveTargetByTag.end(), [&, v, trans, this](std::pair &m) -> bool { diff --git a/src/variables/variable.h b/src/variables/variable.h index d7789f74..6cbd94d1 100644 --- a/src/variables/variable.h +++ b/src/variables/variable.h @@ -610,14 +610,14 @@ class Variables : public std::vector { return std::find_if(begin(), end(), [v](Variable *m) -> bool { return *v == *m; }) != end(); }; - bool contains(const std::string &v) { + bool contains(const VariableValue *v) { return std::find_if(begin(), end(), [v](Variable *m) -> bool { VariableRegex *r = dynamic_cast(m); if (r) { - return r->m_r.searchAll(v).size() > 0; + return r->m_r.searchAll(v->getKey()).size() > 0; } - return v == *m->m_fullName.get(); + return v->getKeyWithCollection() == *m->m_fullName.get(); }) != end(); }; }; diff --git a/test/test-cases/regression/config-update-target-by-id.json b/test/test-cases/regression/config-update-target-by-id.json index 3d7c3c73..fa5a7f9a 100644 --- a/test/test-cases/regression/config-update-target-by-id.json +++ b/test/test-cases/regression/config-update-target-by-id.json @@ -2,7 +2,7 @@ { "enabled":1, "version_min":300000, - "title":"SecRuleUpdateTargetById", + "title":"SecRuleUpdateTargetById - exclude whole collection", "client":{ "ip":"200.249.12.31", "port":123 @@ -34,6 +34,7 @@ "http_code": 200 }, "rules":[ + "SecRuleEngine On", "SecRuleUpdateTargetById 1 !ARGS", "SecRule ARGS \"@contains value\" \"id:1,pass,t:trim,tag:'test',deny\"" ] @@ -41,7 +42,7 @@ { "enabled":1, "version_min":300000, - "title":"SecRuleUpdateTargetById", + "title":"SecRuleUpdateTargetById - exclude using regex", "client":{ "ip":"200.249.12.31", "port":123 @@ -56,7 +57,7 @@ "User-Agent":"curl/7.38.0", "Accept":"*/*" }, - "uri":"/?mixpanel$=value&mixpanel$=other_value", + "uri":"/?mixpanel=value&mixpanel=other_value", "method":"GET" }, "response":{ @@ -73,8 +74,49 @@ "http_code": 200 }, "rules":[ + "SecRuleEngine On", "SecRuleUpdateTargetById 1 !ARGS:/mixpanel$/", "SecRule ARGS \"@contains value\" \"id:1,pass,t:trim,tag:'test',deny\"" ] + }, + { + "enabled":1, + "version_min":300000, + "title":"SecRuleUpdateTargetById - exclude using full name", + "client":{ + "ip":"200.249.12.31", + "port":123 + }, + "server":{ + "ip":"200.249.12.31", + "port":80 + }, + "request":{ + "headers":{ + "Host":"localhost", + "User-Agent":"curl/7.38.0", + "Accept":"*/*" + }, + "uri":"/?mixpanel=value", + "method":"GET" + }, + "response":{ + "headers":{ + "Date":"Mon, 13 Jul 2015 20:02:41 GMT", + "Last-Modified":"Sun, 26 Oct 2014 22:33:37 GMT", + "Content-Type":"text/html" + }, + "body":[ + "no need." + ] + }, + "expected":{ + "http_code": 200 + }, + "rules":[ + "SecRuleEngine On", + "SecRuleUpdateTargetById 1 !ARGS:mixpanel", + "SecRule ARGS \"@contains value\" \"id:1,t:trim,tag:'test',deny\"" + ] } ] diff --git a/test/test-cases/regression/config-update-target-by-tag.json b/test/test-cases/regression/config-update-target-by-tag.json index 4061d52a..10d4c1b4 100644 --- a/test/test-cases/regression/config-update-target-by-tag.json +++ b/test/test-cases/regression/config-update-target-by-tag.json @@ -238,5 +238,45 @@ "SecRuleUpdateTargetByTag test !ARGS:'/^ref/'", "SecRule ARGS \"@contains something\" \"id:1,pass,t:trim,tag:'test',deny\"" ] + }, + { + "enabled":1, + "version_min":300000, + "title":"SecRuleUpdateTargetByTag Test (7/6) Exclusion by full name", + "client":{ + "ip":"200.249.12.31", + "port":123 + }, + "server":{ + "ip":"200.249.12.31", + "port":80 + }, + "request":{ + "headers":{ + "Host":"localhost", + "User-Agent":"curl/7.38.0", + "Accept":"*/*" + }, + "uri":"/?key=value&ref=something", + "method":"GET" + }, + "response":{ + "headers":{ + "Date":"Mon, 13 Jul 2015 20:02:41 GMT", + "Last-Modified":"Sun, 26 Oct 2014 22:33:37 GMT", + "Content-Type":"text/html" + }, + "body":[ + "no need." + ] + }, + "expected":{ + "http_code": 200 + }, + "rules":[ + "SecRuleEngine On", + "SecRuleUpdateTargetByTag test !ARGS:ref", + "SecRule ARGS \"@contains something\" \"id:1,pass,t:trim,tag:'test',deny\"" + ] } ]