diff --git a/CHANGES b/CHANGES index 1086a331..c399b667 100644 --- a/CHANGES +++ b/CHANGES @@ -1,6 +1,8 @@ v3.x.y - YYYY-MMM-DD (to be released) ------------------------------------- + - auditlog: Computes whether or not to save while loading the rules. + [@zimmerle] - actions: Computes Rule association while loading the rules given a performance boost on run time. [@zimmerle, @martinhsv, @WGH-] diff --git a/headers/modsecurity/actions/action.h b/headers/modsecurity/actions/action.h index 5ccd9bdb..66283993 100644 --- a/headers/modsecurity/actions/action.h +++ b/headers/modsecurity/actions/action.h @@ -33,20 +33,20 @@ namespace actions { class Action { public: Action() - : m_name(""), - m_parserPayload("") + : m_parserPayload(""), + m_name("") { } explicit Action(const std::string& action) - : m_name(sort_name(action)), - m_parserPayload(sort_payload(action)) + : m_parserPayload(sort_payload(action)), + m_name(sort_name(action)) { } Action(const Action &a) - : m_name(a.m_name), - m_parserPayload(a.m_parserPayload) + : m_parserPayload(a.m_parserPayload), + m_name(a.m_name) { } @@ -76,7 +76,7 @@ class Action { } - const std::string *getName() { + const std::string *getName() const { return &m_name; } diff --git a/headers/modsecurity/audit_log.h b/headers/modsecurity/audit_log.h index 18862772..1278ecd9 100644 --- a/headers/modsecurity/audit_log.h +++ b/headers/modsecurity/audit_log.h @@ -170,9 +170,8 @@ class AuditLog { bool init(std::string *error); virtual bool close(); - bool saveIfRelevant(Transaction *transaction); - bool saveIfRelevant(Transaction *transaction, int parts); - bool isRelevant(int status); + bool saveIfRelevant(Transaction *transaction) const noexcept; + bool isRelevant(int status) const noexcept; static int addParts(int parts, const std::string& new_parts); static int removeParts(int parts, const std::string& new_parts); diff --git a/headers/modsecurity/rule_message.h b/headers/modsecurity/rule_message.h index ee778b6b..606ac8a7 100644 --- a/headers/modsecurity/rule_message.h +++ b/headers/modsecurity/rule_message.h @@ -130,6 +130,8 @@ class RuleMessage { std::string getUri() const; bool isDisruptive() const; + bool toBeAuditLog() const; + int m_severity; std::list m_tags; diff --git a/headers/modsecurity/transaction.h b/headers/modsecurity/transaction.h index 7d14639b..8bcc1383 100644 --- a/headers/modsecurity/transaction.h +++ b/headers/modsecurity/transaction.h @@ -322,8 +322,7 @@ class TransactionSecMarkerManagement { class TransactionRuleMessageManagement { public: explicit TransactionRuleMessageManagement(Transaction *t) - : m_transaction(t), - m_noAuditLog(false) { + : m_transaction(t) { messageNew(); }; @@ -332,22 +331,7 @@ class TransactionRuleMessageManagement { void logMatchLastRuleOnTheChain(RuleWithActions *rule); - void messageSetNoAuditLog(bool a) { - m_noAuditLog = a; - } - - bool messageSaveAuditLog() const { - return m_noAuditLog; - } - - std::list messageGetAll() { - std::list messages; - for (RuleMessage *a : m_rulesMessages) { - messages.push_back(a); - } - - return messages; - } + std::list messageGetAll(); void messageClear() { m_rulesMessages.clear(); @@ -362,7 +346,6 @@ class TransactionRuleMessageManagement { std::list m_rulesMessages; Transaction *m_transaction; - bool m_noAuditLog; }; diff --git a/src/actions/audit_log.cc b/src/actions/audit_log.cc index 153442e9..362b3f79 100644 --- a/src/actions/audit_log.cc +++ b/src/actions/audit_log.cc @@ -15,20 +15,10 @@ #include "src/actions/audit_log.h" -#include - -#include "modsecurity/transaction.h" - namespace modsecurity { namespace actions { -bool AuditLog::execute(Transaction *transaction) noexcept { - transaction->messageSetNoAuditLog(false); - return true; -} - - } // namespace actions } // namespace modsecurity diff --git a/src/actions/audit_log.h b/src/actions/audit_log.h index 7f48354d..d1851537 100644 --- a/src/actions/audit_log.h +++ b/src/actions/audit_log.h @@ -16,6 +16,10 @@ #include "src/actions/action_allowed_in_sec_default_action.h" +#include "src/actions/action_type_rule_metadata.h" +#include "src/actions/action_allowed_in_sec_default_action.h" + + #ifndef SRC_ACTIONS_AUDIT_LOG_H_ #define SRC_ACTIONS_AUDIT_LOG_H_ @@ -24,13 +28,16 @@ namespace modsecurity { namespace actions { -class AuditLog : public ActionAllowedAsSecDefaultAction { +class AuditLog : public ActionTypeRuleMetaData, + public ActionAllowedAsSecDefaultAction { public: AuditLog() : Action("auditLog") { } - bool execute(Transaction *transaction) noexcept override; + void configure(RuleWithActions *rule) override { + rule->setHasAuditLogAction(true); + } }; diff --git a/src/actions/no_audit_log.cc b/src/actions/no_audit_log.cc index 71a1d244..a8387d18 100644 --- a/src/actions/no_audit_log.cc +++ b/src/actions/no_audit_log.cc @@ -16,18 +16,10 @@ #include "src/actions/no_audit_log.h" -#include "modsecurity/transaction.h" - namespace modsecurity { namespace actions { -bool NoAuditLog::execute(Transaction *transaction) noexcept { - transaction->messageSetNoAuditLog(true); - return true; -} - - } // namespace actions } // namespace modsecurity diff --git a/src/actions/no_audit_log.h b/src/actions/no_audit_log.h index cd1818d4..8b0252bb 100644 --- a/src/actions/no_audit_log.h +++ b/src/actions/no_audit_log.h @@ -15,7 +15,8 @@ #include "modsecurity/actions/action.h" -#include "modsecurity/transaction.h" + +#include "src/actions/action_type_rule_metadata.h" #include "src/actions/action_allowed_in_sec_default_action.h" @@ -27,13 +28,16 @@ namespace modsecurity { namespace actions { -class NoAuditLog : public ActionAllowedAsSecDefaultAction { +class NoAuditLog : public ActionTypeRuleMetaData, + public ActionAllowedAsSecDefaultAction { public: NoAuditLog() : Action("noAuditLog") { } - bool execute(Transaction *transaction) noexcept override; + void configure(RuleWithActions *rule) override { + rule->setHasNoAuditLogAction(true); + } }; diff --git a/src/audit_log/audit_log.cc b/src/audit_log/audit_log.cc index e3c220d4..d6105b53 100644 --- a/src/audit_log/audit_log.cc +++ b/src/audit_log/audit_log.cc @@ -266,14 +266,13 @@ bool AuditLog::init(std::string *error) { } -bool AuditLog::isRelevant(int status) { +bool AuditLog::isRelevant(int status) const noexcept { std::string sstatus = std::to_string(status); if (m_relevant.empty()) { return false; } - if (sstatus.empty()) { return true; } @@ -283,45 +282,34 @@ bool AuditLog::isRelevant(int status) { } -bool AuditLog::saveIfRelevant(Transaction *transaction) { - return saveIfRelevant(transaction, -1); -} - - -bool AuditLog::saveIfRelevant(Transaction *transaction, int parts) { - bool saveAnyway = false; +bool AuditLog::saveIfRelevant(Transaction *transaction) const noexcept { if (m_status == OffAuditLogStatus || m_status == NotSetLogStatus) { ms_dbg_a(transaction, 5, "Audit log engine was not set."); - return true; + return false; } - saveAnyway = transaction->messageSaveAuditLog(); - if ((m_status == RelevantOnlyAuditLogStatus - && this->isRelevant(transaction->m_httpCodeReturned) == false) - && saveAnyway == false) { + && isRelevant(transaction->m_httpCodeReturned) == false)) { ms_dbg_a(transaction, 9, "Return code `" + std::to_string(transaction->m_httpCodeReturned) + "'" \ " is not interesting to audit logs, relevant code(s): `" + m_relevant + "'."); - return false; } - if (parts == -1) { - parts = m_parts; - } ms_dbg_a(transaction, 5, "Saving this request as part " \ "of the audit logs."); + if (m_writer == NULL) { ms_dbg_a(transaction, 1, "Internal error, audit log writer is null"); - } else { - std::string error; - bool a = m_writer->write(transaction, parts, &error); - if (a == false) { - ms_dbg_a(transaction, 1, "Cannot save the audit log: " + error); - return false; - } + return false; + } + + std::string error; + bool a = m_writer->write(transaction, transaction->m_auditLogParts, &error); + if (a == false) { + ms_dbg_a(transaction, 1, "Cannot save the audit log: " + error); + return false; } return true; diff --git a/src/parser/driver.cc b/src/parser/driver.cc index 66d44b4b..15f6615f 100644 --- a/src/parser/driver.cc +++ b/src/parser/driver.cc @@ -157,6 +157,12 @@ int Driver::addSecRule(std::unique_ptr r) { firstRule->getChainedParent()->setHasLogAction( firstRule->hasNoLogAction() ); + firstRule->getChainedParent()->setHasAuditLogAction( + firstRule->hasAuditLogAction() + ); + firstRule->getChainedParent()->setHasNoAuditLogAction( + firstRule->hasNoAuditLogAction() + ); firstRule = firstRule->getChainedParent(); } } diff --git a/src/rule_message.cc b/src/rule_message.cc index 3c29789c..0cf95cc1 100644 --- a/src/rule_message.cc +++ b/src/rule_message.cc @@ -176,6 +176,14 @@ int RuleMessage::getAccuracy() const { } +bool RuleMessage::toBeAuditLog() const { + if (m_rule) { + return m_rule->isItToBeAuditLogged(); + } + return false; +} + + std::string RuleMessage::getClientIpAddress() const { if (m_transaction) { return *m_transaction->m_clientIpAddress.get(); diff --git a/src/rule_with_actions.cc b/src/rule_with_actions.cc index 46f7cf54..98e211db 100644 --- a/src/rule_with_actions.cc +++ b/src/rule_with_actions.cc @@ -90,6 +90,8 @@ RuleWithActions::RuleWithActions( m_containsCaptureAction(false), m_containsLogAction(false), m_containsNoLogAction(false), + m_containsAuditLogAction(false), + m_containsNoAuditLogAction(false), m_containsMultiMatchAction(false), m_containsStaticBlockAction(false), m_defaultSeverity(SEVERITY_NOT_SET), @@ -100,6 +102,8 @@ RuleWithActions::RuleWithActions( m_defaultContainsCaptureAction(false), m_defaultContainsLogAction(false), m_defaultContainsNoLogAction(false), + m_defaultContainsAuditLogAction(false), + m_defaultContainsNoAuditLogAction(false), m_defaultContainsMultiMatchAction(false), m_defaultContainsStaticBlockAction(false), m_isChained(false) { diff --git a/src/rule_with_actions.h b/src/rule_with_actions.h index b81c2092..edde2981 100644 --- a/src/rule_with_actions.h +++ b/src/rule_with_actions.h @@ -146,6 +146,8 @@ class RuleWithActions : public Rule { m_containsCaptureAction(r.m_containsCaptureAction), m_containsLogAction(r.m_containsLogAction), m_containsNoLogAction(r.m_containsNoLogAction), + m_containsAuditLogAction(r.m_containsAuditLogAction), + m_containsNoAuditLogAction(r.m_containsNoAuditLogAction), m_containsMultiMatchAction(r.m_containsMultiMatchAction), m_containsStaticBlockAction(r.m_containsStaticBlockAction), m_defaultSeverity(r.m_defaultSeverity), @@ -156,6 +158,8 @@ class RuleWithActions : public Rule { m_defaultContainsCaptureAction(r.m_defaultContainsCaptureAction), m_defaultContainsLogAction(r.m_defaultContainsLogAction), m_defaultContainsNoLogAction(r.m_defaultContainsNoLogAction), + m_defaultContainsAuditLogAction(r.m_defaultContainsAuditLogAction), + m_defaultContainsNoAuditLogAction(r.m_defaultContainsNoAuditLogAction), m_defaultContainsMultiMatchAction(r.m_defaultContainsMultiMatchAction), m_defaultContainsStaticBlockAction(r.m_defaultContainsStaticBlockAction), m_isChained(r.m_isChained) { @@ -190,6 +194,8 @@ class RuleWithActions : public Rule { m_containsCaptureAction = r.m_containsCaptureAction; m_containsLogAction = r.m_containsLogAction; m_containsNoLogAction = r.m_containsNoLogAction; + m_containsAuditLogAction = r.m_containsAuditLogAction; + m_containsNoAuditLogAction = r.m_containsNoAuditLogAction; m_containsMultiMatchAction = r.m_containsMultiMatchAction; m_containsStaticBlockAction = r.m_containsStaticBlockAction; m_defaultSeverity = r.m_defaultSeverity; @@ -200,6 +206,8 @@ class RuleWithActions : public Rule { m_defaultContainsCaptureAction = r.m_defaultContainsCaptureAction; m_defaultContainsLogAction = r.m_defaultContainsLogAction; m_defaultContainsNoLogAction = r.m_defaultContainsNoLogAction; + m_defaultContainsAuditLogAction = r.m_defaultContainsAuditLogAction; + m_defaultContainsNoAuditLogAction = r.m_defaultContainsNoAuditLogAction; m_defaultContainsMultiMatchAction = r.m_defaultContainsMultiMatchAction; m_defaultContainsStaticBlockAction = r.m_defaultContainsStaticBlockAction; m_isChained = r.m_isChained; @@ -358,11 +366,48 @@ class RuleWithActions : public Rule { inline void setHasMultimatchAction(bool b) { m_containsMultiMatchAction = b; } inline bool hasMultimatchAction() const { return m_containsMultiMatchAction || m_defaultContainsMultiMatchAction; } + inline bool hasAuditLogAction() const { return m_containsAuditLogAction == true; } + inline void setHasAuditLogAction(bool b) { m_containsAuditLogAction = b; } + inline bool hasNoAuditLogAction() const { return m_containsNoAuditLogAction == true; } + inline void setHasNoAuditLogAction(bool b) { m_containsNoAuditLogAction = b; } + inline bool hasLogAction() const { return m_containsLogAction == true; } inline void setHasLogAction(bool b) { m_containsLogAction = b; } inline bool hasNoLogAction() const { return m_containsNoLogAction == true; } inline void setHasNoLogAction(bool b) { m_containsNoLogAction = b; } + + inline bool isItToBeLogged() const noexcept { + if (m_containsNoLogAction) { + return false; + } + + if (m_defaultContainsNoLogAction && !m_containsLogAction) { + return false; + } + + return true; + } + + + inline bool isItToBeAuditLogged() const noexcept { + if (!isItToBeLogged() && !m_containsAuditLogAction + && !m_defaultContainsAuditLogAction) { + return false; + } + + if (m_containsNoAuditLogAction) { + return false; + } + + if (m_defaultContainsNoLogAction && !m_containsAuditLogAction) { + return false; + } + + return true; + } + + inline bool hasLogDataAction() const { return m_logData != nullptr || m_defaultActionLogData != nullptr; } inline std::shared_ptr getLogDataAction() const { return m_logData; } std::string getLogData(/*const */Transaction *t) const; @@ -543,6 +588,8 @@ class RuleWithActions : public Rule { bool m_containsCaptureAction:1; bool m_containsLogAction:1; bool m_containsNoLogAction:1; + bool m_containsAuditLogAction:1; + bool m_containsNoAuditLogAction:1; bool m_containsMultiMatchAction:1; bool m_containsStaticBlockAction:1; @@ -555,6 +602,8 @@ class RuleWithActions : public Rule { bool m_defaultContainsCaptureAction:1; bool m_defaultContainsLogAction:1; bool m_defaultContainsNoLogAction:1; + bool m_defaultContainsAuditLogAction:1; + bool m_defaultContainsNoAuditLogAction:1; bool m_defaultContainsMultiMatchAction:1; bool m_defaultContainsStaticBlockAction:1; diff --git a/src/transaction.cc b/src/transaction.cc index be1b856e..b68d0085 100644 --- a/src/transaction.cc +++ b/src/transaction.cc @@ -72,12 +72,15 @@ void TransactionRuleMessageManagement::logMatchLastRuleOnTheChain(RuleWithAction rm->setRule(rule); - if (rule->hasDisruptiveAction() && + if (rule->hasDisruptiveAction() && rule->isItToBeLogged() && (m_transaction->getRuleEngineState() == RulesSet::DetectionOnlyRuleEngine)) { /* error */ // The error goes over the disruptive massage. We don't need it here. //m_transaction->serverLog(rm); - } else if (rule->hasBlockAction() && (!rule->hasNoLogAction()) || rule->hasLogAction()) { + } else if (rule->hasBlockAction() && rule->isItToBeLogged()) { + /* Log as warning. */ + m_transaction->serverLog(rm); + } else if (rule->isItToBeLogged()) { /* Log as warning. */ m_transaction->serverLog(rm); messageNew(); @@ -88,6 +91,15 @@ void TransactionRuleMessageManagement::messageNew() { m_rulesMessages.push_back(new RuleMessage(m_transaction)); } +std::list TransactionRuleMessageManagement::messageGetAll() { + std::list messages; + for (RuleMessage *a : m_rulesMessages) { + messages.push_back(a); + } + return messages; +} + + /** * @name Transaction @@ -272,7 +284,7 @@ Transaction::Transaction(ModSecurity *ms, RulesSet *rules, char *id, void *logCb ms_dbg(4, "Initializing transaction"); if (m_rules != NULL && m_rules->m_auditLog != NULL) { - m_auditLogParts = this->m_rules->m_auditLog->getParts(); + m_auditLogParts = m_rules->m_auditLog->getParts(); } intervention::clean(&m_it); @@ -1418,8 +1430,7 @@ int Transaction::processLogging() { ms_dbg(8, "Checking if this request is suitable to be " \ "saved as an audit log."); - // FIXME: m_auditLogParts can be accessed via Transaction. - bool saved = this->m_rules->m_auditLog->saveIfRelevant(this, m_auditLogParts); + bool saved = m_rules->m_auditLog->saveIfRelevant(this); if (saved) { ms_dbg(8, "Request was relevant to be saved. Parts: " + std::to_string(m_auditLogParts)); @@ -1605,6 +1616,9 @@ std::string Transaction::toOldAuditLogFormat(int parts, if (parts & audit_log::AuditLog::HAuditLogPart) { audit_log << "--" << trailer << "-" << "H--" << std::endl; for (auto a : messageGetAll()) { + if (!a->toBeAuditLog()) { + continue; + } audit_log << a->log(0, m_httpCodeReturned) << std::endl; } audit_log << std::endl; @@ -1768,6 +1782,10 @@ std::string Transaction::toJSON(int parts) { strlen("messages")); yajl_gen_array_open(g); for (auto a : messageGetAll()) { + if (!a->toBeAuditLog()) { + continue; + } + yajl_gen_map_open(g); LOGFY_ADD("message", a->m_message.c_str()); yajl_gen_string(g, diff --git a/test/cppcheck_suppressions.txt b/test/cppcheck_suppressions.txt index dbbaa62d..2e2874f7 100644 --- a/test/cppcheck_suppressions.txt +++ b/test/cppcheck_suppressions.txt @@ -42,7 +42,7 @@ redundantAssignment:src/operators/pm.cc:94 functionStatic:src/operators/geo_lookup.h:39 useInitializationList:src/utils/shared_files.h:87 unmatchedSuppression:src/utils/msc_tree.cc -functionStatic:headers/modsecurity/transaction.h:454 +functionStatic:headers/modsecurity/transaction.h:437 duplicateBranch:src/audit_log/audit_log.cc:223 unreadVariable:src/request_body_processor/multipart.cc:435 stlcstrParam:src/audit_log/writer/parallel.cc:145 diff --git a/test/test-cases/regression/auditlog.json b/test/test-cases/regression/auditlog.json index 33b61101..e872bbaf 100644 --- a/test/test-cases/regression/auditlog.json +++ b/test/test-cases/regression/auditlog.json @@ -3,7 +3,7 @@ "enabled": 1, "version_min": 300000, "version_max": 0, - "title": "auditlog : basic parser test - Parallel", + "title": "auditlog : basic parser test - Parallel 1", "client": { "ip": "200.249.12.31", "port": 2313 @@ -40,7 +40,7 @@ }, "expected": { "audit_log": "", - "debug_log": "\\[9\\] T \\(0\\) t:trim: \"test", + "debug_log": "Saving this request as part of the audit logs", "error_log": "", "http_code": 403 }, @@ -60,7 +60,7 @@ "enabled": 1, "version_min": 300000, "version_max": 0, - "title": "auditlog : basic parser test - Serial", + "title": "auditlog : basic parser test - Serial 1", "client": { "ip": "200.249.12.31", "port": 2313 @@ -96,8 +96,8 @@ ] }, "expected": { - "audit_log": "", - "debug_log": "\\[9\\] T \\(0\\) t:trim: \"test", + "audit_log": "Access denied with code 403", + "debug_log": "", "error_log": "", "http_code": 403 }, @@ -118,7 +118,7 @@ "enabled": 1, "version_min": 300000, "version_max": 0, - "title": "auditlog : basic parser test - Parallel", + "title": "auditlog : basic parser test - Parallel 2", "client": { "ip": "200.249.12.31", "port": 2313 @@ -154,8 +154,8 @@ ] }, "expected": { - "audit_log": "", - "debug_log": "\\[9\\] T \\(0\\) t:trim: \"test", + "audit_log": "www.modsecurity.org 200.249.12.31", + "debug_log": "Saving this request as part of the audit logs", "error_log": "", "http_code": 403 }, @@ -220,5 +220,55 @@ "SecAuditLogType Serial", "SecAuditLogRelevantStatus \"^(?:5|4(?!04))\"" ] + }, + { + "enabled": 1, + "version_min": 300000, + "version_max": 0, + "title": "auditlog : messages verification - nolog,noauditlog", + "client": { + "ip": "200.249.12.31", + "port": 2313 + }, + "server": { + "ip": "200.249.12.31", + "port": 80 + }, + "request": { + "headers": { + "Host": "www.modsecurity.org", + "User-Agent": "Mozilla\/5.0 (Windows; U; Windows NT 6.1; en-US; rv:1.9.1.5) Gecko\/20091102 Firefox\/3.5.5 (.NET CLR 3.5.30729)", + "Accept": "text\/html,application\/xhtml+xml,application\/xml;q=0.9,*\/*;q=0.8", + "Accept-Language": "en-us,en;q=0.5", + "Accept-Encoding": "gzip,deflate", + "Accept-Charset": "ISO-8859-1,utf-8;q=0.7,*;q=0.7", + "Keep-Alive": "300", + "Connection": "keep-alive", + "Pragma": "no-cache", + "Cache-Control": "no-cache" + }, + "uri": "\/test.pl?param1=test¶m2=test2", + "method": "GET", + "http_version": 1.1, + "body": "" + }, + "expected": { + "audit_log": "1556", + "error_log": "", + "http_code": 403 + }, + "rules": [ + "SecRuleEngine On", + "SecDefaultAction \"phase:1,nolog,auditlog,status:403,pass\"", + "SecRule ARGS \"@contains test\" \"id:1555,phase:1,log,noauditlog\"", + "SecRule ARGS \"@contains test\" \"id:1556,phase:1,deny,auditlog\"", + "SecAuditEngine RelevantOnly", + "SecAuditLogParts ABCFHZ", + "SecAuditLog /tmp/test/modsec_audit_auditlog_1.log", + "SecAuditLogDirMode 0766", + "SecAuditLogFileMode 0666", + "SecAuditLogType Serial", + "SecAuditLogRelevantStatus \"^(?:5|4(?!04))\"" + ] } ] diff --git a/test/test-cases/regression/config-secdefaultaction.json b/test/test-cases/regression/config-secdefaultaction.json index d4f71fed..ee9066ab 100644 --- a/test/test-cases/regression/config-secdefaultaction.json +++ b/test/test-cases/regression/config-secdefaultaction.json @@ -272,11 +272,14 @@ }, "expected":{ "audit_log":"", - "debug_log":"Request was relevant to be saved.", + "debug_log":"Saving this request as part of the audit logs", "http_code": 302 }, "rules":[ "SecRuleEngine On", + "SecAuditEngine On", + "SecAuditLogStorageDir /tmp/test", + "SecAuditLogType Parallel", "SecDefaultAction \"phase:2,log,auditlog,status:302,redirect:'http://www.google.com'\"", "SecRule REQUEST_HEADERS \"@contains PHPSESSID\" \"phase:2,id:1,block\"", "SecRule TX \"@contains to_test\" \"id:2,t:lowercase,t:none,block\""