Computes auditlog during rules load time

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-]

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;
     }
 

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);

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<std::string> m_tags;
 

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<RuleMessage *> messageGetAll() {
-        std::list<RuleMessage *> messages;
-        for (RuleMessage *a : m_rulesMessages) {
-            messages.push_back(a);
-        }
-
-        return messages;
-    }
+    std::list<RuleMessage *> messageGetAll();
 
     void messageClear() {
         m_rulesMessages.clear();
@@ -362,7 +346,6 @@ class TransactionRuleMessageManagement {
     std::list<RuleMessage *> m_rulesMessages;
 
     Transaction *m_transaction;
-    bool m_noAuditLog;
 };
 
 

src/actions/audit_log.cc

@@ -15,20 +15,10 @@
 
 #include "src/actions/audit_log.h"
 
-#include <string>
-
-#include "modsecurity/transaction.h"
-
 
 namespace modsecurity {
 namespace actions {
 
 
-bool AuditLog::execute(Transaction *transaction) noexcept {
-    transaction->messageSetNoAuditLog(false);
-    return true;
-}
-
-
 }  // namespace actions
 }  // namespace modsecurity

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);
+    }
 };
 
 

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

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);
+    }
 };
 
 

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;

src/parser/driver.cc

@@ -157,6 +157,12 @@ int Driver::addSecRule(std::unique_ptr<RuleWithActions> r) {
                 firstRule->getChainedParent()->setHasLogAction(
                     firstRule->hasNoLogAction()
                 );
+                firstRule->getChainedParent()->setHasAuditLogAction(
+                    firstRule->hasAuditLogAction()
+                );
+                firstRule->getChainedParent()->setHasNoAuditLogAction(
+                    firstRule->hasNoAuditLogAction()
+                );
                 firstRule = firstRule->getChainedParent();
             }
         }

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();

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) {

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<actions::LogData> 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;
 

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<RuleMessage *> TransactionRuleMessageManagement::messageGetAll() {
+    std::list<RuleMessage *> 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,

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

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&param2=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))\""
+    ]
   }
 ]

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\""