From eecb90cfd0cfdfc914091a16d5e2303b78a03d85 Mon Sep 17 00:00:00 2001 From: Felipe Zimmerle Date: Wed, 23 Nov 2016 09:29:12 -0300 Subject: [PATCH] setvar: needs review --- src/actions/capture.cc | 11 ----------- src/operators/operator.cc | 11 +++++++++++ src/operators/operator.h | 8 ++++++++ src/operators/rx.cc | 16 ++++++++++++---- src/operators/rx.h | 7 ++++++- src/rule.cc | 19 ++++++++++++++----- test/test-cases/regression/action-msg.json | 4 ++-- 7 files changed, 53 insertions(+), 23 deletions(-) diff --git a/src/actions/capture.cc b/src/actions/capture.cc index 3ca91a2e..a02c2c78 100644 --- a/src/actions/capture.cc +++ b/src/actions/capture.cc @@ -33,17 +33,6 @@ namespace actions { bool Capture::evaluate(Rule *rule, Transaction *transaction) { - if (transaction->m_matched.empty()) { - return false; - } - - int i = 0; - while (transaction->m_matched.empty() == false) { - transaction->m_collections.storeOrUpdateFirst("TX", - std::to_string(i), transaction->m_matched.back()); - transaction->m_matched.pop_back(); - i++; - } return true; } diff --git a/src/operators/operator.cc b/src/operators/operator.cc index 60b681e7..361b4ffa 100644 --- a/src/operators/operator.cc +++ b/src/operators/operator.cc @@ -75,6 +75,17 @@ bool Operator::debug(Transaction *transaction, int x, std::string a) { } +bool Operator::evaluateInternal(Transaction *transaction, + Rule *rule, const std::string& a) { + bool res = evaluate(transaction, rule, a); + + if (m_negation) { + return !res; + } + + return res; +} + bool Operator::evaluateInternal(Transaction *transaction, const std::string& a) { bool res = evaluate(transaction, a); diff --git a/src/operators/operator.h b/src/operators/operator.h index 74f2bbc5..5aef651d 100644 --- a/src/operators/operator.h +++ b/src/operators/operator.h @@ -21,6 +21,7 @@ #define SRC_OPERATORS_OPERATOR_H__ #include "modsecurity/transaction.h" +#include "modsecurity/rule.h" #ifdef __cplusplus namespace modsecurity { @@ -48,7 +49,14 @@ class Operator { } bool evaluateInternal(Transaction *t, const std::string& a); + bool evaluateInternal(Transaction *t, Rule *rule, + const std::string& a); + virtual bool evaluate(Transaction *transaction, const std::string &str); + virtual bool evaluate(Transaction *transaction, Rule *rule, + const std::string &str) { + return evaluate(transaction, str); + } bool m_negation; std::string m_match_message; diff --git a/src/operators/rx.cc b/src/operators/rx.cc index 74464b76..04cbdd08 100644 --- a/src/operators/rx.cc +++ b/src/operators/rx.cc @@ -20,13 +20,15 @@ #include "src/operators/operator.h" #include "src/macro_expansion.h" +#include "modsecurity/rule.h" namespace modsecurity { namespace operators { -bool Rx::evaluate(Transaction *transaction, const std::string& input) { +bool Rx::evaluate(Transaction *transaction, Rule *rule, + const std::string& input) { SMatch match; std::list matches; @@ -35,10 +37,16 @@ bool Rx::evaluate(Transaction *transaction, const std::string& input) { } matches = m_re->searchAll(input); - for (const SMatch& a : matches) { - if (transaction) { + if (rule && rule->getActionsByName("capture").size() > 0 && transaction) { + int i = 0; + matches.reverse(); + for (const SMatch& a : matches) { + transaction->m_collections.storeOrUpdateFirst("TX", + std::to_string(i), a.match); + transaction->debug(7, "Added regex subexpression TX." + + std::to_string(i) + ": " + a.match); transaction->m_matched.push_back(a.match); - transaction->debug(7, "Added regex subexpression: " + a.match); + i++; } } diff --git a/src/operators/rx.h b/src/operators/rx.h index 3ba67663..cabe8c22 100644 --- a/src/operators/rx.h +++ b/src/operators/rx.h @@ -43,7 +43,12 @@ class Rx : public Operator { ~Rx() { delete m_re; } - bool evaluate(Transaction *transaction, const std::string &input); + bool evaluate(Transaction *transaction, Rule *rule, + const std::string &input) override; + bool evaluate(Transaction *transaction, + const std::string &input) override { + return evaluate(transaction, NULL, input); + } private: std::string m_param; diff --git a/src/rule.cc b/src/rule.cc index ae5344b5..460695e4 100644 --- a/src/rule.cc +++ b/src/rule.cc @@ -208,7 +208,7 @@ bool Rule::evaluateActions(Transaction *trasn) { for (Action *a : this->actions_runtime_pos) { if (a->isDisruptive() == false) { #ifndef NO_LOGS - trasn->debug(4, "Running (_non_ disruptive) action: " + + trasn->debug(4, "Running [III] (_non_ disruptive) action: " + a->m_name + "."); #endif a->evaluate(this, trasn); @@ -438,7 +438,7 @@ bool Rule::evaluate(Transaction *trasn) { + "\" (Variable: " + v->m_key + ")"); #endif - ret = this->op->evaluateInternal(trasn, value); + ret = this->op->evaluateInternal(trasn, this, value); #ifndef NO_LOGS clock_t end = clock(); @@ -487,6 +487,12 @@ bool Rule::evaluate(Transaction *trasn) { } else { containsDisruptive = true; } + } else { + if (a->m_name == "setvar") { + trasn->debug(4, "Running [I] (_non_ disruptive) " \ + "action: " + a->m_name); + a->evaluate(this, trasn, ruleMessage); + } } } @@ -574,11 +580,14 @@ bool Rule::evaluate(Transaction *trasn) { #endif } } else if (!a->isDisruptive()) { + // here + if (a->m_name != "capture" && a->m_name != "setvar") { #ifndef NO_LOGS - trasn->debug(4, "Running (_non_ disruptive) " \ - "action: " + a->m_name); + trasn->debug(4, "Running [II] (_non_ disruptive) " \ + "action: " + a->m_name); #endif - a->evaluate(this, trasn, ruleMessage); + a->evaluate(this, trasn, ruleMessage); + } } } } diff --git a/test/test-cases/regression/action-msg.json b/test/test-cases/regression/action-msg.json index 6198d7a6..6933be8a 100644 --- a/test/test-cases/regression/action-msg.json +++ b/test/test-cases/regression/action-msg.json @@ -111,8 +111,8 @@ }, "rules":[ "SecRuleEngine On", - "SecRule REQUEST_HEADERS \"@contains PHPSESSID\" \"id:1,capture,t:lowercase,t:none,msg:'This is a test: %{TX.0}% ops'\"", - "SecRule TX \"@contains to_test\" \"id:2,t:lowercase,capture,t:none\"" + "SecRule REQUEST_HEADERS \"@rx PHPSESSID\" \"id:1,capture,t:lowercase,t:none,msg:'This is a test: %{TX.0}% ops'\"", + "SecRule TX \"@rx to_test\" \"id:2,t:lowercase,capture,t:none\"" ] } ]