From 2ce5baed32eef6ee18345b24a8f12d97450a71ee Mon Sep 17 00:00:00 2001 From: asterite Date: Thu, 17 Aug 2017 21:03:39 +0300 Subject: [PATCH 1/4] support macro expansion in @rx try to use macro expansion on @rx argument before matching. If after expansion argument changed, make new Regex from the macro-expanded argument and use that for matching. Fixes #1528 --- src/operators/rx.cc | 13 ++++++++++++- 1 file changed, 12 insertions(+), 1 deletion(-) diff --git a/src/operators/rx.cc b/src/operators/rx.cc index 9ed07cb975..5c4cdcf708 100644 --- a/src/operators/rx.cc +++ b/src/operators/rx.cc @@ -33,12 +33,19 @@ bool Rx::evaluate(Transaction *transaction, Rule *rule, const std::string& input, std::shared_ptr ruleMessage) { SMatch match; std::list matches; + Regex * re = m_re; if (m_param.empty()) { return true; } - matches = m_re->searchAll(input); + std::string eparam = MacroExpansion::expand(m_param, transaction); + + if (eparam != m_param) { + re = new Regex(eparam); + } + + matches = re->searchAll(input); if (rule && rule->getActionsByName("capture").size() > 0 && transaction) { int i = 0; matches.reverse(); @@ -56,6 +63,10 @@ bool Rx::evaluate(Transaction *transaction, Rule *rule, logOffset(ruleMessage, i.m_offset, i.m_length); } + if (re != m_re) { + delete re; + } + if (matches.size() > 0) { return true; } From 3145bb4b424482d9e27d825b3c5c7d4f49e52868 Mon Sep 17 00:00:00 2001 From: asterite Date: Sat, 19 Aug 2017 09:49:55 +0300 Subject: [PATCH 2/4] Optimize: Rx ctor checks if macro expansion used Added method checkIfMacroExpansionPossible() called by Rx constructor to check if "%{...}" construct is present in regex. If not, Rx::evaluate should not try macro expansion on regex before matching. --- src/operators/rx.cc | 20 +++++++++++++++++--- src/operators/rx.h | 9 +++++++++ 2 files changed, 26 insertions(+), 3 deletions(-) diff --git a/src/operators/rx.cc b/src/operators/rx.cc index 5c4cdcf708..746d60e2b7 100644 --- a/src/operators/rx.cc +++ b/src/operators/rx.cc @@ -39,10 +39,12 @@ bool Rx::evaluate(Transaction *transaction, Rule *rule, return true; } - std::string eparam = MacroExpansion::expand(m_param, transaction); + if (m_macro_expansion_possible) { + std::string eparam = MacroExpansion::expand(m_param, transaction); - if (eparam != m_param) { - re = new Regex(eparam); + if (eparam != m_param) { + re = new Regex(eparam); + } } matches = re->searchAll(input); @@ -75,5 +77,17 @@ bool Rx::evaluate(Transaction *transaction, Rule *rule, } +void Rx::checkIfMacroExpansionPossible() { + size_t pos = m_param.find("%{"); + + if (pos == std::string::npos) { + m_macro_expansion_possible = false; + return; + } + + m_macro_expansion_possible = (m_param.find('}', pos) != std::string::npos); +} + + } // namespace operators } // namespace modsecurity diff --git a/src/operators/rx.h b/src/operators/rx.h index 713aed2252..c2fd0c6473 100644 --- a/src/operators/rx.h +++ b/src/operators/rx.h @@ -38,14 +38,20 @@ class Rx : public Operator { Rx(std::string op, std::string param, bool negation) : Operator(op, param, negation) { m_re = new Regex(param); + + checkIfMacroExpansionPossible(); } Rx(std::string name, std::string param) : Operator(name, param) { m_re = new Regex(param); + + checkIfMacroExpansionPossible(); } explicit Rx(std::string param) : Operator("Rx", param) { m_re = new Regex(param); + + checkIfMacroExpansionPossible(); } ~Rx() { @@ -65,6 +71,9 @@ class Rx : public Operator { private: Regex *m_re; + bool m_macro_expansion_possible; + + void checkIfMacroExpansionPossible(); }; From d1bc038dc68ad0664d38e4af6ea5cf518cf64f55 Mon Sep 17 00:00:00 2001 From: asterite Date: Sat, 19 Aug 2017 10:21:57 +0300 Subject: [PATCH 3/4] add a test for macro expansion in @rx --- test/test-cases/regression/issue-1528.json | 38 ++++++++++++++++++++++ 1 file changed, 38 insertions(+) create mode 100644 test/test-cases/regression/issue-1528.json diff --git a/test/test-cases/regression/issue-1528.json b/test/test-cases/regression/issue-1528.json new file mode 100644 index 0000000000..1899fbc501 --- /dev/null +++ b/test/test-cases/regression/issue-1528.json @@ -0,0 +1,38 @@ +[ +{ + "enabled": 1, + "version_min": 209000, + "version_max": -1, + "title": "Macro expansion inside regex does not work", + "url": "https:\/\/github.com\/SpiderLabs\/ModSecurity\/issues\/1528", + "gihub_issue": 1528, + "client": { + "ip": "200.249.12.31", + "port": 2313 + }, + "server": { + "ip": "200.249.12.31", + "port": 80 + }, + "request": { + "uri":"/?param=attack", + "headers": "", + "body": "", + "method": "GET", + "http_version": 1.1 + }, + "response": { + "headers": "", + "body": "" + }, + "expected": { + "debug_log": "Rule returned 1", + "error_log": "Matched \"Operator `Rx' with parameter `\\^%{tx\\.bad_value}\\$' against variable `ARGS:param' \\(Value: `attack' " + }, + "rules": [ + "SecRuleEngine On", + "SecAction \"id:1, nolog, setvar:tx.bad_value=attack\"", + "SecRule ARGS:param \"@rx ^%{tx.bad_value}$\" id:2" + ] +} +] From d7937cbdff3580669560c9f4be984ac16314865b Mon Sep 17 00:00:00 2001 From: asterite Date: Sat, 19 Aug 2017 11:16:54 +0300 Subject: [PATCH 4/4] add @rx macro expansion test to list in Makefile --- Makefile.am | 1 + 1 file changed, 1 insertion(+) diff --git a/Makefile.am b/Makefile.am index fd0884092c..2ff2f6ad48 100644 --- a/Makefile.am +++ b/Makefile.am @@ -128,6 +128,7 @@ TESTS+=test/test-cases/regression/variable-TIME_MIN.json TESTS+=test/test-cases/regression/action-setuid.json TESTS+=test/test-cases/regression/action-setrsc.json TESTS+=test/test-cases/regression/issue-1152.json +TESTS+=test/test-cases/regression/issue-1528.json TESTS+=test/test-cases/regression/config-calling_phases_by_name.json TESTS+=test/test-cases/regression/variable-USERID.json TESTS+=test/test-cases/regression/request-body-parser-multipart.json