From ce3abf262695dd857938caa8ee5fb30235651ddc Mon Sep 17 00:00:00 2001 From: Felipe Zimmerle Date: Mon, 26 Nov 2018 20:46:05 -0300 Subject: [PATCH] Adds support to multiple ranges in ctl:ruleRemoveById Issue #1956 --- CHANGES | 2 + Makefile.am | 1 + headers/modsecurity/transaction.h | 1 + src/actions/ctl/rule_remove_by_id.cc | 66 ++++++- src/actions/ctl/rule_remove_by_id.h | 6 +- src/rule.cc | 8 + test/test-cases/regression/issue-1956.json | 192 +++++++++++++++++++++ 7 files changed, 265 insertions(+), 11 deletions(-) create mode 100644 test/test-cases/regression/issue-1956.json diff --git a/CHANGES b/CHANGES index c3ef2c41..98a856f6 100644 --- a/CHANGES +++ b/CHANGES @@ -1,6 +1,8 @@ v3.0.4 - YYYY-MMM-DD (to be released) ------------------------------------- + - Adds support to multiple ranges in ctl:ruleRemoveById + [Issue #1956 - @theseion, @victorhora, @zimmerle] - Rule variable interpolation broken [Issue #1961 - @soonum, @zimmerle] - Make the boundary check less strict as per RFC2046 diff --git a/Makefile.am b/Makefile.am index 51946a37..e4718789 100644 --- a/Makefile.am +++ b/Makefile.am @@ -95,6 +95,7 @@ TESTS+=test/test-cases/regression/issue-1850.json TESTS+=test/test-cases/regression/issue-1725.json TESTS+=test/test-cases/regression/issue-1941.json TESTS+=test/test-cases/regression/issue-1943.json +TESTS+=test/test-cases/regression/issue-1956.json TESTS+=test/test-cases/regression/variable-RESPONSE_HEADERS.json TESTS+=test/test-cases/regression/config-include.json TESTS+=test/test-cases/regression/variable-WEBSERVER_ERROR_LOG.json diff --git a/headers/modsecurity/transaction.h b/headers/modsecurity/transaction.h index b6cfca33..dd2e9cf8 100644 --- a/headers/modsecurity/transaction.h +++ b/headers/modsecurity/transaction.h @@ -461,6 +461,7 @@ class Transaction : public TransactionAnchoredVariables { * */ std::list m_ruleRemoveById; + std::list > m_ruleRemoveByIdRange; /** * diff --git a/src/actions/ctl/rule_remove_by_id.cc b/src/actions/ctl/rule_remove_by_id.cc index 4dab37ab..678252d1 100644 --- a/src/actions/ctl/rule_remove_by_id.cc +++ b/src/actions/ctl/rule_remove_by_id.cc @@ -19,6 +19,7 @@ #include #include "modsecurity/transaction.h" +#include "src/utils/string.h" namespace modsecurity { namespace actions { @@ -27,20 +28,69 @@ namespace ctl { bool RuleRemoveById::init(std::string *error) { std::string what(m_parser_payload, 15, m_parser_payload.size() - 15); + bool added = false; + std::vector toRemove = utils::string::ssplit(what, ' '); + for (std::string &a : toRemove) { + std::string b = modsecurity::utils::string::parserSanitizer(a); + if (b.size() == 0) { + continue; + } - try { - m_id = std::stoi(what); - } catch(...) { - error->assign("Not able to convert '" + what + - "' into a number"); - return false; + size_t dash = b.find('-'); + if (dash != std::string::npos) { + std::string n1s = std::string(b, 0, dash); + std::string n2s = std::string(b, dash + 1, b.size() - (dash + 1)); + int n1n = 0; + int n2n = 0; + try { + n1n = std::stoi(n1s); + added = true; + } catch (...) { + error->assign("Not a number: " + n1s); + return false; + } + try { + n2n = std::stoi(n2s); + added = true; + } catch (...) { + error->assign("Not a number: " + n2s); + return false; + } + + if (n1s > n2s) { + error->assign("Invalid range: " + b); + return false; + } + m_ranges.push_back(std::make_pair(n1n, n2n)); + added = true; + } else { + try { + int num = std::stoi(b); + m_ids.push_back(num); + added = true; + } catch (...) { + error->assign("Not a number or range: " + b); + return false; + } + } } - return true; + if (added) { + return true; + } + + error->assign("Not a number or range: " + what); + return false; } bool RuleRemoveById::evaluate(Rule *rule, Transaction *transaction) { - transaction->m_ruleRemoveById.push_back(m_id); + for (auto &i : m_ids) { + transaction->m_ruleRemoveById.push_back(i); + } + for (auto &i : m_ranges) { + transaction->m_ruleRemoveByIdRange.push_back(i); + } + return true; } diff --git a/src/actions/ctl/rule_remove_by_id.h b/src/actions/ctl/rule_remove_by_id.h index e3082b10..efb0dc85 100644 --- a/src/actions/ctl/rule_remove_by_id.h +++ b/src/actions/ctl/rule_remove_by_id.h @@ -30,13 +30,13 @@ namespace ctl { class RuleRemoveById : public Action { public: explicit RuleRemoveById(std::string action) - : Action(action, RunTimeOnlyIfMatchKind), - m_id(0) { } + : Action(action, RunTimeOnlyIfMatchKind) { } bool init(std::string *error) override; bool evaluate(Rule *rule, Transaction *transaction) override; - int m_id; + std::list > m_ranges; + std::list m_ids; }; diff --git a/src/rule.cc b/src/rule.cc index ebe0c3a7..70f281e0 100644 --- a/src/rule.cc +++ b/src/rule.cc @@ -656,6 +656,14 @@ bool Rule::evaluate(Transaction *trans, " was skipped due to a ruleRemoveById action..."); return true; } + for (auto &i : trans->m_ruleRemoveByIdRange) { + if (!(i.first <= m_ruleId && i.second >= m_ruleId)) { + continue; + } + ms_dbg_a(trans, 9, "Rule id: " + std::to_string(m_ruleId) + + " was skipped due to a ruleRemoveById action..."); + return true; + } if (m_op->m_string) { eparam = m_op->m_string->evaluate(trans); diff --git a/test/test-cases/regression/issue-1956.json b/test/test-cases/regression/issue-1956.json new file mode 100644 index 00000000..ead45da2 --- /dev/null +++ b/test/test-cases/regression/issue-1956.json @@ -0,0 +1,192 @@ +[ +{ + "enabled": 1, + "version_min": 209000, + "version_max": -1, + "title": "ctl:ruleRemoveById doesn't handle all ranges equally 1", + "url": "https:\/\/github.com\/SpiderLabs\/ModSecurity\/issues\/1956", + "gihub_issue": 1956, + "client": { + "ip": "200.249.12.31", + "port": 2313 + }, + "server": { + "ip": "200.249.12.31", + "port": 80 + }, + "request": { + "headers": { + "Host": "www.google.com" + }, + "uri": "\/test.pl?param1= test ¶m2=)", + "body": "", + "method": "GET", + "http_version": 1.1 + }, + "response": { + "headers": "", + "body": "" + }, + "expected": { + "audit_log": "", + "debug_log": "Rule id: 913104 was skipped due to a ruleRemoveById", + "error_log": "" + }, + "rules": [ + "SecRule REQUEST_URI \"@beginsWith /test\" \"id:1001,phase:request,pass,nolog,t:none,ctl:ruleRemoveById=913103-913105\"", + "SecRule REQUEST_URI \"@beginsWith /test\" \"id:913104,phase:request,pass,nolog,t:none,msg:'whee'\"" + ] + }, + { + "enabled": 1, + "version_min": 209000, + "version_max": -1, + "title": "ctl:ruleRemoveById doesn't handle all ranges equally 2", + "url": "https:\/\/github.com\/SpiderLabs\/ModSecurity\/issues\/1956", + "gihub_issue": 1956, + "client": { + "ip": "200.249.12.31", + "port": 2313 + }, + "server": { + "ip": "200.249.12.31", + "port": 80 + }, + "request": { + "headers": { + "Host": "www.google.com" + }, + "uri": "\/test.pl?param1= test ¶m2=)", + "body": "", + "method": "GET", + "http_version": 1.1 + }, + "response": { + "headers": "", + "body": "" + }, + "expected": { + "audit_log": "", + "debug_log": "Rule id: 913104 was skipped due to a ruleRemoveById", + "error_log": "" + }, + "rules": [ + "SecRule REQUEST_URI \"@beginsWith /test\" \"id:1001,phase:request,pass,nolog,t:none,ctl:ruleRemoveById=913104\"", + "SecRule REQUEST_URI \"@beginsWith /test\" \"id:913104,phase:request,pass,nolog,t:none,msg:'whee'\"" + ] + }, + { + "enabled": 1, + "version_min": 209000, + "version_max": -1, + "title": "ctl:ruleRemoveById doesn't handle all ranges equally 3", + "url": "https:\/\/github.com\/SpiderLabs\/ModSecurity\/issues\/1956", + "gihub_issue": 1956, + "client": { + "ip": "200.249.12.31", + "port": 2313 + }, + "server": { + "ip": "200.249.12.31", + "port": 80 + }, + "request": { + "headers": { + "Host": "www.google.com" + }, + "uri": "\/test.pl?param1= test ¶m2=)", + "body": "", + "method": "GET", + "http_version": 1.1 + }, + "response": { + "headers": "", + "body": "" + }, + "expected": { + "audit_log": "", + "debug_log": "Rule id: 913103 was skipped due to a ruleRemoveById", + "error_log": "" + }, + "rules": [ + "SecRule REQUEST_URI \"@beginsWith /test\" \"id:1001,phase:request,pass,nolog,t:none,ctl:ruleRemoveById=913103-913105\"", + "SecRule REQUEST_URI \"@beginsWith /test\" \"id:913103,phase:request,pass,nolog,t:none,msg:'whee'\"" + ] + }, + { + "enabled": 1, + "version_min": 209000, + "version_max": -1, + "title": "ctl:ruleRemoveById doesn't handle all ranges equally 4", + "url": "https:\/\/github.com\/SpiderLabs\/ModSecurity\/issues\/1956", + "gihub_issue": 1956, + "client": { + "ip": "200.249.12.31", + "port": 2313 + }, + "server": { + "ip": "200.249.12.31", + "port": 80 + }, + "request": { + "headers": { + "Host": "www.google.com" + }, + "uri": "\/test.pl?param1= test ¶m2=)", + "body": "", + "method": "GET", + "http_version": 1.1 + }, + "response": { + "headers": "", + "body": "" + }, + "expected": { + "audit_log": "", + "debug_log": "Rule id: 913105 was skipped due to a ruleRemoveById", + "error_log": "" + }, + "rules": [ + "SecRule REQUEST_URI \"@beginsWith /test\" \"id:1001,phase:request,pass,nolog,t:none,ctl:ruleRemoveById=913103-913105\"", + "SecRule REQUEST_URI \"@beginsWith /test\" \"id:913105,phase:request,pass,nolog,t:none,msg:'whee'\"" + ] + }, + { + "enabled": 1, + "version_min": 209000, + "version_max": -1, + "title": "ctl:ruleRemoveById doesn't handle all ranges equally 5", + "url": "https:\/\/github.com\/SpiderLabs\/ModSecurity\/issues\/1956", + "gihub_issue": 1956, + "client": { + "ip": "200.249.12.31", + "port": 2313 + }, + "server": { + "ip": "200.249.12.31", + "port": 80 + }, + "request": { + "headers": { + "Host": "www.google.com" + }, + "uri": "\/test.pl?param1= test ¶m2=)", + "body": "", + "method": "GET", + "http_version": 1.1 + }, + "response": { + "headers": "", + "body": "" + }, + "expected": { + "audit_log": "", + "debug_log": "Rule: 913102. Executing operator", + "error_log": "" + }, + "rules": [ + "SecRule REQUEST_URI \"@beginsWith /test\" \"id:1001,phase:request,pass,nolog,t:none,ctl:ruleRemoveById=913103-913105\"", + "SecRule REQUEST_URI \"@beginsWith /test\" \"id:913102,phase:request,pass,nolog,t:none,msg:'whee'\"" + ] + } +]