From cce6179dcc8e0bae5fbcf8a90ddd97b586ba66ee Mon Sep 17 00:00:00 2001 From: Felipe Zimmerle Date: Thu, 1 Dec 2016 14:14:54 -0300 Subject: [PATCH] Refactoring: new structure for logging alerts Disruptive actions were moved to actions::disruptive namespace --- headers/modsecurity/intervention.h | 40 ++++++++++++++++++++++++++++-- src/actions/data/status.h | 6 ++--- src/actions/disruptive/allow.h | 6 ++--- src/actions/disruptive/block.cc | 2 -- src/actions/disruptive/block.h | 6 ++--- src/actions/disruptive/deny.cc | 6 +++-- src/actions/disruptive/deny.h | 6 ++--- src/actions/disruptive/pass.cc | 7 ++---- src/actions/disruptive/redirect.cc | 10 +++++--- src/actions/disruptive/redirect.h | 9 ++++--- src/rule.cc | 17 +++++++++++-- src/rule_message.cc | 4 +-- src/transaction.cc | 30 ++++++++++------------ src/utils/string.cc | 1 - 14 files changed, 98 insertions(+), 52 deletions(-) diff --git a/headers/modsecurity/intervention.h b/headers/modsecurity/intervention.h index ebaa5099..2b621122 100644 --- a/headers/modsecurity/intervention.h +++ b/headers/modsecurity/intervention.h @@ -23,11 +23,47 @@ namespace modsecurity { typedef struct ModSecurityIntervention_t { int status; int pause; - const char *url; - const char *log; + char *url; + char *log; int disruptive; } ModSecurityIntervention; +#ifdef __cplusplus +namespace intervention { + static void reset(ModSecurityIntervention_t *i) { + i->status = 200; + i->pause = 0; + i->disruptive = 0; + } + + static void clean(ModSecurityIntervention_t *i) { + i->url = NULL; + i->log = NULL; + reset(i); + } + + static void freeUrl(ModSecurityIntervention_t *i) { + if (i->url) { + free(i->url); + i->url = NULL; + } + } + + static void freeLog(ModSecurityIntervention_t *i) { + if (i->log) { + free(i->log); + i->log = NULL; + } + } + + static void free(ModSecurityIntervention_t *i) { + freeUrl(i); + freeLog(i); + } + +} // namespace modsecurity +#endif + #ifdef __cplusplus } // namespace modsecurity #endif diff --git a/src/actions/data/status.h b/src/actions/data/status.h index 1ceef328..556212c2 100644 --- a/src/actions/data/status.h +++ b/src/actions/data/status.h @@ -18,8 +18,8 @@ #include "modsecurity/actions/action.h" #include "modsecurity/rule_message.h" -#ifndef SRC_ACTIONS_STATUS_H_ -#define SRC_ACTIONS_STATUS_H_ +#ifndef SRC_ACTIONS_DATA_STATUS_H_ +#define SRC_ACTIONS_DATA_STATUS_H_ #ifdef __cplusplus class Transaction; @@ -48,4 +48,4 @@ class Status : public Action { } // namespace modsecurity #endif -#endif // SRC_ACTIONS_STATUS_H_ +#endif // SRC_ACTIONS_DATA_STATUS_H_ diff --git a/src/actions/disruptive/allow.h b/src/actions/disruptive/allow.h index 1cd905c8..b28d63ba 100644 --- a/src/actions/disruptive/allow.h +++ b/src/actions/disruptive/allow.h @@ -17,8 +17,8 @@ #include "modsecurity/actions/action.h" -#ifndef SRC_ACTIONS_ALLOW_H_ -#define SRC_ACTIONS_ALLOW_H_ +#ifndef SRC_ACTIONS_DISRUPTIVE_ALLOW_H_ +#define SRC_ACTIONS_DISRUPTIVE_ALLOW_H_ #ifdef __cplusplus class Transaction; @@ -84,4 +84,4 @@ class Allow : public Action { } // namespace modsecurity #endif -#endif // SRC_ACTIONS_ALLOW_H_ +#endif // SRC_ACTIONS_DISRUPTIVE_ALLOW_H_ diff --git a/src/actions/disruptive/block.cc b/src/actions/disruptive/block.cc index dd5a84c7..830972b8 100644 --- a/src/actions/disruptive/block.cc +++ b/src/actions/disruptive/block.cc @@ -30,8 +30,6 @@ namespace disruptive { bool Block::evaluate(Rule *rule, Transaction *transaction, RuleMessage *rm) { - std::string log; - transaction->debug(8, "Marking request as disruptive."); for (Action *a : transaction->m_rules->defaultActions[rule->phase]) { diff --git a/src/actions/disruptive/block.h b/src/actions/disruptive/block.h index 33c3417f..44359a10 100644 --- a/src/actions/disruptive/block.h +++ b/src/actions/disruptive/block.h @@ -18,8 +18,8 @@ #include "modsecurity/actions/action.h" #include "modsecurity/rule_message.h" -#ifndef SRC_ACTIONS_BLOCK_H_ -#define SRC_ACTIONS_BLOCK_H_ +#ifndef SRC_ACTIONS_DISRUPTIVE_BLOCK_H_ +#define SRC_ACTIONS_DISRUPTIVE_BLOCK_H_ #ifdef __cplusplus class Transaction; @@ -46,4 +46,4 @@ class Block : public Action { } // namespace modsecurity #endif -#endif // SRC_ACTIONS_BLOCK_H_ +#endif // SRC_ACTIONS_DISRUPTIVE_BLOCK_H_ diff --git a/src/actions/disruptive/deny.cc b/src/actions/disruptive/deny.cc index 458e2218..0b780126 100644 --- a/src/actions/disruptive/deny.cc +++ b/src/actions/disruptive/deny.cc @@ -15,10 +15,10 @@ #include "src/actions/disruptive/deny.h" +#include #include #include #include -#include #include "modsecurity/transaction.h" @@ -42,7 +42,9 @@ bool Deny::evaluate(Rule *rule, Transaction *transaction, RuleMessage *rm) { log.append(std::to_string(rm->m_rule->phase - 1) + "). "); transaction->m_it.disruptive = true; - transaction->m_it.log = strdup(rm->disruptiveErrorLog(transaction, log).c_str()); + intervention::freeLog(&transaction->m_it); + transaction->m_it.log = strdup( + rm->disruptiveErrorLog(transaction, log).c_str()); return true; } diff --git a/src/actions/disruptive/deny.h b/src/actions/disruptive/deny.h index fda5d8e5..6b64f93b 100644 --- a/src/actions/disruptive/deny.h +++ b/src/actions/disruptive/deny.h @@ -19,8 +19,8 @@ #include "modsecurity/transaction.h" #include "modsecurity/rule_message.h" -#ifndef SRC_ACTIONS_DENY_H_ -#define SRC_ACTIONS_DENY_H_ +#ifndef SRC_ACTIONS_DISRUPTIVE_DENY_H_ +#define SRC_ACTIONS_DISRUPTIVE_DENY_H_ namespace modsecurity { namespace actions { @@ -41,4 +41,4 @@ class Deny : public Action { } // namespace actions } // namespace modsecurity -#endif // SRC_ACTIONS_DENY_H_ +#endif // SRC_ACTIONS_DISRUPTIVE_DENY_H_ diff --git a/src/actions/disruptive/pass.cc b/src/actions/disruptive/pass.cc index 3b737fbc..b4638272 100644 --- a/src/actions/disruptive/pass.cc +++ b/src/actions/disruptive/pass.cc @@ -28,11 +28,8 @@ namespace disruptive { bool Pass::evaluate(Rule *rule, Transaction *transaction, RuleMessage *rm) { - transaction->m_it.status = 200; - transaction->m_it.disruptive = false; - transaction->m_it.url = NULL; - transaction->m_it.log = NULL; - transaction->m_it.pause = 0; + intervention::free(&transaction->m_it); + intervention::reset(&transaction->m_it); transaction->debug(8, "Running action pass"); diff --git a/src/actions/disruptive/redirect.cc b/src/actions/disruptive/redirect.cc index 533f9fec..9784e286 100644 --- a/src/actions/disruptive/redirect.cc +++ b/src/actions/disruptive/redirect.cc @@ -15,9 +15,9 @@ #include "src/actions/disruptive/redirect.h" +#include #include #include -#include #include "modsecurity/transaction.h" @@ -35,7 +35,8 @@ bool Redirect::init(std::string *error) { } -bool Redirect::evaluate(Rule *rule, Transaction *transaction, RuleMessage *rm) { +bool Redirect::evaluate(Rule *rule, Transaction *transaction, + RuleMessage *rm) { m_urlExpanded = MacroExpansion::expand(m_url, transaction); std::string log; @@ -47,9 +48,12 @@ bool Redirect::evaluate(Rule *rule, Transaction *transaction, RuleMessage *rm) { log.append(" (phase "); log.append(std::to_string(rm->m_rule->phase - 1) + "). "); + intervention::freeUrl(&transaction->m_it); transaction->m_it.url = strdup(m_urlExpanded.c_str()); transaction->m_it.disruptive = true; - transaction->m_it.log = strdup(rm->disruptiveErrorLog(transaction, log).c_str()); + intervention::freeLog(&transaction->m_it); + transaction->m_it.log = strdup( + rm->disruptiveErrorLog(transaction, log).c_str()); return true; } diff --git a/src/actions/disruptive/redirect.h b/src/actions/disruptive/redirect.h index 6d8612f2..3773593b 100644 --- a/src/actions/disruptive/redirect.h +++ b/src/actions/disruptive/redirect.h @@ -18,8 +18,8 @@ #include "modsecurity/actions/action.h" #include "modsecurity/rule_message.h" -#ifndef SRC_ACTIONS_REDIRECT_H_ -#define SRC_ACTIONS_REDIRECT_H_ +#ifndef SRC_ACTIONS_DISRUPTIVE_REDIRECT_H_ +#define SRC_ACTIONS_DISRUPTIVE_REDIRECT_H_ #ifdef __cplusplus class Transaction; @@ -39,7 +39,8 @@ class Redirect : public Action { m_urlExpanded(""), m_url("") { } - bool evaluate(Rule *rule, Transaction *transaction, RuleMessage *rm) override; + bool evaluate(Rule *rule, Transaction *transaction, RuleMessage *rm) + override; bool init(std::string *error) override; bool isDisruptive() override { return true; } @@ -55,4 +56,4 @@ class Redirect : public Action { } // namespace modsecurity #endif -#endif // SRC_ACTIONS_REDIRECT_H_ +#endif // SRC_ACTIONS_DISRUPTIVE_REDIRECT_H_ diff --git a/src/rule.cc b/src/rule.cc index e0b621eb..2a2b337b 100644 --- a/src/rule.cc +++ b/src/rule.cc @@ -587,7 +587,7 @@ bool Rule::evaluate(Transaction *trasn) { if (globalRet == false) { trasn->debug(4, "Rule returned 0."); cleanMatchedVars(trasn); - return false; + goto end_clean; } trasn->debug(4, "Rule returned 1."); @@ -599,7 +599,7 @@ bool Rule::evaluate(Transaction *trasn) { if (this->chainedRule == NULL) { trasn->debug(4, "Rule is marked as chained but there " \ "isn't a subsequent rule."); - return false; + goto end_clean; } trasn->debug(4, "Executing chained rule."); @@ -609,6 +609,13 @@ bool Rule::evaluate(Transaction *trasn) { goto end_exec; } +end_clean: + while (finalVars.empty() == false) { + auto *a = finalVars.back(); + finalVars.pop_back(); + delete a; + } + return false; end_exec: @@ -617,6 +624,12 @@ end_exec: trasn->serverLog(u); } + while (finalVars.empty() == false) { + auto *a = finalVars.back(); + finalVars.pop_back(); + delete a; + } + return true; } diff --git a/src/rule_message.cc b/src/rule_message.cc index fd67bfff..7be8ea47 100644 --- a/src/rule_message.cc +++ b/src/rule_message.cc @@ -23,7 +23,8 @@ namespace modsecurity { -std::string RuleMessage::disruptiveErrorLog(Transaction *trans, std::string msg2) { +std::string RuleMessage::disruptiveErrorLog(Transaction *trans, + std::string msg2) { std::string msg; msg.append("[client " + std::string(trans->m_clientIpAddress) + "]"); @@ -50,7 +51,6 @@ std::string RuleMessage::disruptiveErrorLog(Transaction *trans, std::string msg2 msg.append(" [unique_id \"" + trans->m_id + "\"]"); return modsecurity::utils::string::toHexIfNeeded(msg); - } std::string RuleMessage::errorLog(Transaction *trans) { diff --git a/src/transaction.cc b/src/transaction.cc index 5f1578cc..72633911 100644 --- a/src/transaction.cc +++ b/src/transaction.cc @@ -161,11 +161,7 @@ Transaction::Transaction(ModSecurity *ms, Rules *rules, void *logCbData) this->debug(4, "Initializing transaction"); #endif - m_it.status = 200; - m_it.disruptive = false; - m_it.url = NULL; - m_it.log = NULL; - m_it.pause = 0; + intervention::clean(&m_it); } @@ -183,6 +179,9 @@ Transaction::~Transaction() { m_rules->decrementReferenceCount(); + intervention::free(&m_it); + intervention::clean(&m_it); + delete m_json; delete m_xml; } @@ -885,7 +884,8 @@ int Transaction::appendRequestBody(const unsigned char *buf, size_t len) { debug(5, "Request body limit is marked to reject the " \ "request"); #endif - m_it.log = "Request body limit is marked to reject the request"; + intervention::free(&m_it); + m_it.log = strdup("Request body limit is marked to reject the request"); m_it.status = 403; m_it.disruptive = true; } @@ -1142,8 +1142,9 @@ int Transaction::appendResponseBody(const unsigned char *buf, size_t len) { debug(5, "Response body limit is marked to reject the " \ "request"); #endif - - m_it.log = "Response body limit is marked to reject the request"; + intervention::free(&m_it); + m_it.log = strdup("Response body limit is marked to reject " \ + "the request"); m_it.status = 403; m_it.disruptive = true; } @@ -1299,17 +1300,12 @@ bool Transaction::intervention(ModSecurityIntervention *it) { if (m_it.log != NULL) { std::string log(""); - const char *log_str; log.append(m_it.log); - utils::string::replaceAll(&log, std::string("%d"), std::to_string(it->status)); - log_str = strdup(log.c_str()); - it->log = log_str; + utils::string::replaceAll(&log, std::string("%d"), + std::to_string(it->status)); + it->log = strdup(log.c_str()); } - m_it.status = 200; - m_it.disruptive = false; - m_it.url = NULL; - m_it.log = NULL; - m_it.pause = 0; + intervention::reset(&m_it); } return it->disruptive; diff --git a/src/utils/string.cc b/src/utils/string.cc index 5fedb1b9..7f07e9c5 100644 --- a/src/utils/string.cc +++ b/src/utils/string.cc @@ -210,7 +210,6 @@ void replaceAll(std::string *str, const std::string& from, const std::string& to) { size_t start_pos = 0; while ((start_pos = str->find(from, start_pos)) != std::string::npos) { - size_t end_pos = start_pos + from.length(); str->replace(start_pos, from.length(), to); start_pos += to.length(); }