From 274f9e5aa1572af36c2a212b8dea1d45556074e2 Mon Sep 17 00:00:00 2001 From: Felipe Zimmerle Date: Thu, 19 Oct 2017 23:00:25 -0300 Subject: [PATCH] Refactoring on RuleMessage class, now accepting http code as parameter --- CHANGES | 2 + headers/modsecurity/rule_message.h | 50 ++++++------ src/actions/disruptive/deny.cc | 8 +- src/actions/disruptive/redirect.cc | 7 +- src/rule_message.cc | 121 ++++++++++------------------- src/transaction.cc | 6 +- 6 files changed, 72 insertions(+), 122 deletions(-) diff --git a/CHANGES b/CHANGES index 61b28a3a..6f4a3eb2 100644 --- a/CHANGES +++ b/CHANGES @@ -2,6 +2,8 @@ v3.0.????? - ? --------------------------- + - Refactoring on RuleMessage class, now accepting http code as parameter. + [@zimmerle] - Having disruptive msgs as disruptive [instead of warnings] on audit log [Issue #1592 - @zimmerle, @nobodysz] - Parser: Pipes are no longer welcomed inside regex dict element selection. diff --git a/headers/modsecurity/rule_message.h b/headers/modsecurity/rule_message.h index 9d145a19..d8ae71f1 100644 --- a/headers/modsecurity/rule_message.h +++ b/headers/modsecurity/rule_message.h @@ -32,13 +32,19 @@ namespace modsecurity { + + class RuleMessage { public: + enum LogMessageInfo { + ErrorLogTailLogMessageInfo = 2, + ClientLogMessageInfo = 4 + }; + explicit RuleMessage(Rule *rule, Transaction *trans) : m_accuracy(rule->m_accuracy), m_clientIpAddress(trans->m_clientIpAddress), m_data(""), - m_disruptiveMessage(""), m_id(trans->m_id), m_isDisruptive(false), m_match(""), @@ -59,35 +65,31 @@ class RuleMessage { m_ver(rule->m_ver) { } - std::string errorLog() { - return RuleMessage::errorLog(this); - } - std::string disruptiveErrorLog() { - return RuleMessage::disruptiveErrorLog(this); - } - std::string noClientErrorLog() { - return RuleMessage::noClientErrorLog(this); - } - std::string noClientErrorLog(bool disrupt) { - return RuleMessage::noClientErrorLog(this, disrupt); - } - std::string errorLogTail() { - return RuleMessage::errorLogTail(this); - } + std::string log() { - return RuleMessage::log(this); + return RuleMessage::log(this, 0); } - static std::string disruptiveErrorLog(const RuleMessage *rm); - static std::string noClientErrorLog(const RuleMessage *rm); - static std::string noClientErrorLog(const RuleMessage *rm, bool disrupt); - static std::string errorLogTail(const RuleMessage *rm); - static std::string errorLog(const RuleMessage *rm); - static std::string log(const RuleMessage *rm); + std::string log(int props) { + return RuleMessage::log(this, props); + } + std::string errorLog() { + return RuleMessage::log(this, ClientLogMessageInfo | ErrorLogTailLogMessageInfo); + } + + static std::string log(const RuleMessage *rm, int props, int code); + static std::string log(const RuleMessage *rm, int props) { + return RuleMessage::log(rm, props, -1); + } + static std::string log(const RuleMessage *rm) { + return RuleMessage::log(rm, 0); + } + + static std::string _details(const RuleMessage *rm); + static std::string _errorLogTail(const RuleMessage *rm); int m_accuracy; std::string m_clientIpAddress; std::string m_data; - std::string m_disruptiveMessage; std::string m_id; bool m_isDisruptive; std::string m_match; diff --git a/src/actions/disruptive/deny.cc b/src/actions/disruptive/deny.cc index 8d845696..2258b5fd 100644 --- a/src/actions/disruptive/deny.cc +++ b/src/actions/disruptive/deny.cc @@ -33,21 +33,15 @@ bool Deny::evaluate(Rule *rule, Transaction *transaction, #ifndef NO_LOGS transaction->debug(8, "Running action deny"); #endif - std::string log; if (transaction->m_it.status == 200) { transaction->m_it.status = 403; } - log.append("Access denied with code %d"); - log.append(" (phase "); - log.append(std::to_string(rm->m_rule->m_phase - 1) + "). "); - - rm->m_disruptiveMessage.assign(log); transaction->m_it.disruptive = true; intervention::freeLog(&transaction->m_it); transaction->m_it.log = strdup( - rm->disruptiveErrorLog().c_str()); + rm->log(RuleMessage::LogMessageInfo::ClientLogMessageInfo).c_str()); rm->m_isDisruptive = true; return true; diff --git a/src/actions/disruptive/redirect.cc b/src/actions/disruptive/redirect.cc index 13a23c5d..306a241b 100644 --- a/src/actions/disruptive/redirect.cc +++ b/src/actions/disruptive/redirect.cc @@ -40,23 +40,18 @@ bool Redirect::init(std::string *error) { bool Redirect::evaluate(Rule *rule, Transaction *transaction, std::shared_ptr rm) { m_urlExpanded = MacroExpansion::expand(m_url, transaction); - std::string log; /* if it was changed before, lets keep it. */ if (transaction->m_it.status == 200) { transaction->m_it.status = m_status; } - log.append("Access denied with code %d"); - log.append(" (phase "); - log.append(std::to_string(rm->m_rule->m_phase - 1) + "). "); - rm->m_disruptiveMessage.assign(log); intervention::freeUrl(&transaction->m_it); transaction->m_it.url = strdup(m_urlExpanded.c_str()); transaction->m_it.disruptive = true; intervention::freeLog(&transaction->m_it); transaction->m_it.log = strdup( - rm->disruptiveErrorLog().c_str()); + rm->log(RuleMessage::LogMessageInfo::ClientLogMessageInfo).c_str()); rm->m_isDisruptive = true; return true; diff --git a/src/rule_message.cc b/src/rule_message.cc index f863ae90..bf208960 100644 --- a/src/rule_message.cc +++ b/src/rule_message.cc @@ -23,13 +23,10 @@ namespace modsecurity { -std::string RuleMessage::disruptiveErrorLog(const RuleMessage *rm) { + +std::string RuleMessage::_details(const RuleMessage *rm) { std::string msg; - msg.append("[client " + std::string(rm->m_clientIpAddress) + "]"); - msg.append(" ModSecurity: "); - msg.append(rm->m_disruptiveMessage); - msg.append(rm->m_match); msg.append(" [file \"" + std::string(rm->m_ruleFile) + "\"]"); msg.append(" [line \"" + std::to_string(rm->m_ruleLine) + "\"]"); msg.append(" [id \"" + std::to_string(rm->m_ruleId) + "\"]"); @@ -50,91 +47,55 @@ std::string RuleMessage::disruptiveErrorLog(const RuleMessage *rm) { msg.append(" [unique_id \"" + rm->m_id + "\"]"); msg.append(" [ref \"" + rm->m_reference + "\"]"); - return modsecurity::utils::string::toHexIfNeeded(msg); + return msg; } -std::string RuleMessage::noClientErrorLog(const RuleMessage *rm, bool disruptive) { - std::string msg; - if (disruptive == false) { - return RuleMessage::noClientErrorLog(rm); - } - msg.append("Message: "); - msg.append(rm->m_disruptiveMessage); - msg.append(rm->m_match); - msg.append(" [file \"" + std::string(rm->m_ruleFile) + "\"]"); - msg.append(" [line \"" + std::to_string(rm->m_ruleLine) + "\"]"); - msg.append(" [id \"" + std::to_string(rm->m_ruleId) + "\"]"); - msg.append(" [rev \"" + rm->m_rev + "\"]"); - msg.append(" [msg \"" + rm->m_message + "\"]"); - msg.append(" [data \"" + rm->m_data + "\"]"); - msg.append(" [severity \"" + - std::to_string(rm->m_severity) + "\"]"); - msg.append(" [ver \"" + rm->m_ver + "\"]"); - msg.append(" [maturity \"" + std::to_string(rm->m_maturity) + "\"]"); - msg.append(" [accuracy \"" + std::to_string(rm->m_accuracy) + "\"]"); - for (auto &a : rm->m_tags) { - msg.append(" [tag \"" + a + "\"]"); - } - msg.append(" [ref \"" + rm->m_reference + "\"]"); - - return modsecurity::utils::string::toHexIfNeeded(msg); -} - -std::string RuleMessage::noClientErrorLog(const RuleMessage *rm) { +std::string RuleMessage::_errorLogTail(const RuleMessage *rm) { std::string msg; - msg.append("ModSecurity: Warning. "); - msg.append(rm->m_match); - msg.append(" [file \"" + std::string(rm->m_ruleFile) + "\"]"); - msg.append(" [line \"" + std::to_string(rm->m_ruleLine) + "\"]"); - msg.append(" [id \"" + std::to_string(rm->m_ruleId) + "\"]"); - msg.append(" [rev \"" + rm->m_rev + "\"]"); - msg.append(" [msg \"" + rm->m_message + "\"]"); - msg.append(" [data \"" + rm->m_data + "\"]"); - msg.append(" [severity \"" + - std::to_string(rm->m_severity) + "\"]"); - msg.append(" [ver \"" + rm->m_ver + "\"]"); - msg.append(" [maturity \"" + std::to_string(rm->m_maturity) + "\"]"); - msg.append(" [accuracy \"" + std::to_string(rm->m_accuracy) + "\"]"); - for (auto &a : rm->m_tags) { - msg.append(" [tag \"" + a + "\"]"); - } - msg.append(" [ref \"" + rm->m_reference + "\"]"); - - return modsecurity::utils::string::toHexIfNeeded(msg); -} - -std::string RuleMessage::errorLogTail(const RuleMessage *rm) { - std::string msg; - - msg.append("[hostname \"" + std::string(rm->m_serverIpAddress) \ - + "\"]"); + msg.append("[hostname \"" + std::string(rm->m_serverIpAddress) + "\"]"); msg.append(" [uri \"" + rm->m_uriNoQueryStringDecoded + "\"]"); msg.append(" [unique_id \"" + rm->m_id + "\"]"); + return msg; +} + + +std::string RuleMessage::log(const RuleMessage *rm, int props, int code) { + std::string msg(""); + + if (props & ClientLogMessageInfo) { + msg.append("[client " + std::string(rm->m_clientIpAddress) + "] "); + } + + if (rm->m_isDisruptive) + { + msg.append("ModSecurity: Access denied with code "); + if (code == -1) { + msg.append("%d"); + } + else + { + msg.append(std::to_string(code)); + } + msg.append(" (phase "); + msg.append(std::to_string(rm->m_rule->m_phase - 1) + "). "); + } + else + { + msg.append("ModSecurity: Warning. "); + } + + msg.append(rm->m_match); + msg.append(_details(rm)); + + if (props & ErrorLogTailLogMessageInfo) { + msg.append(" " + _errorLogTail(rm)); + } + return modsecurity::utils::string::toHexIfNeeded(msg); } -std::string RuleMessage::errorLog(const RuleMessage *rm) { - std::string msg; - - msg.append("[client " + std::string(rm->m_clientIpAddress) + "] "); - msg.append(noClientErrorLog(rm)); - msg.append(" " + errorLogTail(rm)); - - return msg; -} - -std::string RuleMessage::log(const RuleMessage *rm) { - std::string msg(""); - if (rm->m_isDisruptive) { - msg.append(disruptiveErrorLog(rm)); - } else { - msg.append(errorLog(rm)); - } - - return msg; -} } // namespace modsecurity diff --git a/src/transaction.cc b/src/transaction.cc index 647ad824..8eb2bd5f 100644 --- a/src/transaction.cc +++ b/src/transaction.cc @@ -1485,11 +1485,7 @@ std::string Transaction::toOldAuditLogFormat(int parts, if (parts & audit_log::AuditLog::HAuditLogPart) { audit_log << "--" << trailer << "-" << "H--" << std::endl; for (auto a : m_rulesMessages) { - if (a.m_isDisruptive == true) { - audit_log << a.noClientErrorLog(true) << std::endl; - } else { - audit_log << a.noClientErrorLog() << std::endl; - } + audit_log << a.log() << std::endl; } audit_log << std::endl; /** TODO: write audit_log H part. */