From ed8b0c85d774ec6e4bf0a0ce3f85501fc929cf6a Mon Sep 17 00:00:00 2001 From: Felipe Zimmerle Date: Tue, 16 Feb 2016 23:20:23 -0300 Subject: [PATCH] Fix `capture' memory management The capture action was implemented before the transaction concept. While partially ported to use the transaction, some of the elements were not freed correctly. Now it is fully ported to use the class Transaction. --- headers/modsecurity/transaction.h | 7 +++++++ src/actions/capture.cc | 31 ++++--------------------------- src/operators/contains.cc | 2 +- src/operators/contains.h | 3 +-- src/operators/detect_sqli.cc | 2 +- src/operators/detect_sqli.h | 2 -- src/operators/pm.cc | 2 +- src/operators/pm.h | 1 - src/operators/rx.cc | 2 +- src/operators/rx.h | 1 - src/rule.cc | 1 + 11 files changed, 17 insertions(+), 37 deletions(-) diff --git a/headers/modsecurity/transaction.h b/headers/modsecurity/transaction.h index 39d8ec36..e65cb9e3 100644 --- a/headers/modsecurity/transaction.h +++ b/headers/modsecurity/transaction.h @@ -318,6 +318,13 @@ class Transaction { */ transaction::Collections m_collections; + /** + * Holds the whatever matched in the operation utilization. + * That variable will be further used by the capture action. + * + */ + std::list m_matched; + private: std::string *m_ARGScombinedSizeStr; std::string *m_namesArgs; diff --git a/src/actions/capture.cc b/src/actions/capture.cc index 5dcb97e2..613654f0 100644 --- a/src/actions/capture.cc +++ b/src/actions/capture.cc @@ -32,38 +32,15 @@ namespace modsecurity { namespace actions { bool Capture::evaluate(Rule *rule, Transaction *transaction) { - operators::Operator *op = rule->op; - std::list *match; - - operators::Pm *pm = dynamic_cast(op); - if (pm != NULL) { - match = &pm->matched; - } - - operators::Rx *rx = dynamic_cast(op); - if (rx != NULL) { - match = &rx->matched; - } - - operators::Contains *contains = dynamic_cast(op); - if (contains != NULL) { - match = &contains->matched; - } - - operators::DetectSQLi *dsqli = dynamic_cast(op); - if (dsqli != NULL) { - match = &dsqli->matched; - } - - if (match->empty()) { + if (transaction->m_matched.empty()) { return false; } int i = 0; - while (match->empty() == false) { + while (transaction->m_matched.empty() == false) { transaction->m_collections.storeOrUpdateFirst("TX", - std::to_string(i), match->back()); - match->pop_back(); + std::to_string(i), transaction->m_matched.back()); + transaction->m_matched.pop_back(); i++; } return true; diff --git a/src/operators/contains.cc b/src/operators/contains.cc index 47695d8e..91de6d78 100644 --- a/src/operators/contains.cc +++ b/src/operators/contains.cc @@ -27,7 +27,7 @@ bool Contains::evaluate(Transaction *transaction, const std::string &input) { bool contains = input.find(p) != std::string::npos; if (contains) { - matched.push_back(p); + transaction->m_matched.push_back(p); } if (negation) { diff --git a/src/operators/contains.h b/src/operators/contains.h index 6acc8d7b..ba9d58c6 100644 --- a/src/operators/contains.h +++ b/src/operators/contains.h @@ -31,9 +31,8 @@ class Contains : public Operator { /** @ingroup ModSecurity_Operator */ Contains(std::string op, std::string param, bool negation) : Operator(op, param, negation) { } - bool evaluate(Transaction *transaction, const std::string &exp) override; - std::list matched; + bool evaluate(Transaction *transaction, const std::string &exp) override; }; } // namespace operators diff --git a/src/operators/detect_sqli.cc b/src/operators/detect_sqli.cc index 115aee2b..a56f9406 100644 --- a/src/operators/detect_sqli.cc +++ b/src/operators/detect_sqli.cc @@ -32,8 +32,8 @@ bool DetectSQLi::evaluate(Transaction *transaction, const std::string &input) { issqli = libinjection_sqli(input.c_str(), input.length(), fingerprint); if (issqli) { - matched.push_back(fingerprint); if (transaction) { + transaction->m_matched.push_back(fingerprint); #ifndef NO_LOGS transaction->debug(4, "detected SQLi using libinjection with " \ "fingerprint '" + std::string(fingerprint) + "' at: '" + diff --git a/src/operators/detect_sqli.h b/src/operators/detect_sqli.h index 91c93aaf..d4f9df92 100644 --- a/src/operators/detect_sqli.h +++ b/src/operators/detect_sqli.h @@ -31,8 +31,6 @@ class DetectSQLi : public Operator { : Operator(op, param, negation) { } bool evaluate(Transaction *transaction, const std::string &input); - - std::list matched; }; } // namespace operators diff --git a/src/operators/pm.cc b/src/operators/pm.cc index b149705e..2a8a2f70 100644 --- a/src/operators/pm.cc +++ b/src/operators/pm.cc @@ -78,7 +78,7 @@ bool Pm::evaluate(Transaction *transaction, const std::string &input) { rc = acmp_process_quick(&pt, &match, input.c_str(), input.length()); if (rc == 1) { - this->matched.push_back(std::string(match)); + transaction->m_matched.push_back(std::string(match)); } return rc == 1; diff --git a/src/operators/pm.h b/src/operators/pm.h index 57037ace..7f5def2c 100644 --- a/src/operators/pm.h +++ b/src/operators/pm.h @@ -42,7 +42,6 @@ class Pm : public Operator { bool init(const std::string &file, const char **error) override; void postOrderTraversal(acmp_btree_node_t *node); - std::list matched; protected: ACMP *m_p; }; diff --git a/src/operators/rx.cc b/src/operators/rx.cc index 2ffd4c45..1d13c882 100644 --- a/src/operators/rx.cc +++ b/src/operators/rx.cc @@ -30,7 +30,7 @@ bool Rx::evaluate(Transaction *transaction, const std::string& input) { SMatch match; if (regex_search(input, &match, *m_re) && match.size() >= 1) { - this->matched.push_back(match.match); + transaction->m_matched.push_back(match.match); return true; } diff --git a/src/operators/rx.h b/src/operators/rx.h index 0812b817..f56713fc 100644 --- a/src/operators/rx.h +++ b/src/operators/rx.h @@ -42,7 +42,6 @@ class Rx : public Operator { bool evaluate(Transaction *transaction, const std::string &input); - std::list matched; private: std::string m_param; Regex *m_re; diff --git a/src/rule.cc b/src/rule.cc index 6483f101..dc918194 100644 --- a/src/rule.cc +++ b/src/rule.cc @@ -272,6 +272,7 @@ bool Rule::evaluate(Transaction *trasn) { std::vector *variables = this->variables; RuleMessage *ruleMessage = new modsecurity::RuleMessage(this, m_log_message); + trasn->m_matched.clear(); if (m_secmarker == true) { return true;