Adds full support to UpdateActionById.

Issue #1800

CHANGES

@@ -1,6 +1,8 @@
 v3.0.3 - YYYY-MMM-DD (to be released)
 -------------------------------------
 
+ - Adds support to UpdateActionById.
+   [Issue #1800 - @zimmerle, @victorhora, @NisariAIT]
  - Add correct C function prototypes for msc_init and msc_create_rule_set
    [Issue #1922 - @steven-j-wojcik]
  - Allow LuaJIT 2.1 to be used

Makefile.am

@@ -183,6 +183,7 @@ TESTS+=test/test-cases/regression/variable-RESPONSE_BODY.json
 TESTS+=test/test-cases/regression/config-calling_phases_by_name.json
 TESTS+=test/test-cases/regression/variable-FILES_COMBINED_SIZE.json
 TESTS+=test/test-cases/regression/variable-FULL_REQUEST_LENGTH.json
+TESTS+=test/test-cases/regression/config-update-action-by-id.json
 TESTS+=test/test-cases/regression/config-update-target-by-id.json
 TESTS+=test/test-cases/regression/variable-REQUEST_HEADERS.json
 TESTS+=test/test-cases/regression/misc.json

headers/modsecurity/rule.h

@@ -124,36 +124,29 @@ class Rule {
         std::shared_ptr<std::string> transStr,
         int nth);
 
-    std::vector<actions::Action *> m_actionsRuntimePos;
-    std::vector<actions::Action *> m_actionsRuntimePre;
+    actions::Action *m_theDisruptiveAction;
+    actions::LogData *m_logData;
+    actions::Msg *m_msg;
+    actions::Severity *m_severity;
     bool m_chained;
-    Rule *m_chainedRule;
-    std::string m_fileName;
-    int m_lineNumber;
-    std::string m_marker;
-    operators::Operator *m_op;
-    bool m_secMarker;
-    modsecurity::Variables::Variables *m_variables;
-
-
-    int64_t m_ruleId;
-    std::string m_rev;
-    // msg ?
-    std::string m_ver;
-    //std::string m_logData;
-
-    //if (child->severity != NOT_SET) merged->severity = child->severity;
-    int m_accuracy;
-    int m_maturity;
-    int m_phase;
-
-    bool m_containsStaticDisruptiveAction;
     bool m_containsCaptureAction;
     bool m_containsMultiMatchAction;
     bool m_containsStaticBlockAction;
-    actions::Severity *m_severity;
-    actions::LogData *m_logData;
-    actions::Msg *m_msg;
+    bool m_secMarker;
+    int64_t m_ruleId;
+    int m_accuracy;
+    int m_lineNumber;
+    int m_maturity;
+    int m_phase;
+    modsecurity::Variables::Variables *m_variables;
+    operators::Operator *m_op;
+    Rule *m_chainedRule;
+    std::string m_fileName;
+    std::string m_marker;
+    std::string m_rev;
+    std::string m_ver;
+    std::vector<actions::Action *> m_actionsRuntimePos;
+    std::vector<actions::Action *> m_actionsRuntimePre;
     std::vector<actions::SetVar *> m_actionsSetVar;
     std::vector<actions::Tag *> m_actionsTag;
  private:

src/Makefile.am

@@ -106,6 +106,7 @@ ACTIONS = \
 	actions/accuracy.cc \
 	actions/action.cc \
 	actions/audit_log.cc \
+	actions/block.cc \
 	actions/capture.cc \
 	actions/chain.cc \
 	actions/ctl/audit_log_parts.cc \
@@ -119,7 +120,6 @@ ACTIONS = \
 	actions/ctl/rule_remove_by_tag.cc \
 	actions/ctl/request_body_access.cc\
 	actions/disruptive/allow.cc \
-	actions/disruptive/block.cc \
 	actions/disruptive/deny.cc \
 	actions/disruptive/redirect.cc \
 	actions/disruptive/pass.cc \

src/actions/action.cc

@@ -22,7 +22,7 @@
 #include "modsecurity/rule.h"
 #include "src/utils/string.h"
 
-#include "src/actions/disruptive/block.h"
+#include "src/actions/block.h"
 #include "src/actions/chain.h"
 #include "src/actions/disruptive/deny.h"
 #include "src/actions/disruptive/redirect.h"

src/actions/block.cc

@@ -13,7 +13,7 @@
  *
  */
 
-#include "src/actions/disruptive/block.h"
+#include "src/actions/block.h"
 
 #include <iostream>
 #include <string>
@@ -27,7 +27,6 @@
 
 namespace modsecurity {
 namespace actions {
-namespace disruptive {
 
 
 bool Block::evaluate(Rule *rule, Transaction *transaction,
@@ -37,8 +36,7 @@ bool Block::evaluate(Rule *rule, Transaction *transaction,
 #endif
 
     for (Action *a : transaction->m_rules->m_defaultActions[rule->m_phase]) {
-        if (a->isDisruptive() == false
-            || dynamic_cast<actions::disruptive::Block *>(a) != NULL) {
+        if (a->isDisruptive() == false) {
             continue;
         }
         a->evaluate(rule, transaction, rm);
@@ -48,6 +46,5 @@ bool Block::evaluate(Rule *rule, Transaction *transaction,
 }
 
 
-}  // namespace disruptive
 }  // namespace actions
 }  // namespace modsecurity

src/actions/block.h

@@ -29,7 +29,6 @@ namespace modsecurity {
 class Transaction;
 
 namespace actions {
-namespace disruptive {
 
 
 class Block : public Action {
@@ -38,11 +37,9 @@ class Block : public Action {
 
     bool evaluate(Rule *rule, Transaction *transaction,
         std::shared_ptr<RuleMessage> rm) override;
-    bool isDisruptive() override { return true; }
 };
 
 
-}  // namespace disruptive
 }  // namespace actions
 }  // namespace modsecurity
 #endif

src/parser/driver.cc

@@ -81,7 +81,7 @@ int Driver::addSecRule(Rule *rule) {
         if (lastRule->m_chainedRule == NULL) {
             rule->m_phase = lastRule->m_phase;
             lastRule->m_chainedRule = rule;
-            if (rule->m_containsStaticDisruptiveAction) {
+            if (rule->m_theDisruptiveAction) {
                 m_parserError << "Disruptive actions can only be specified by";
                 m_parserError << " chain starter rules.";
                 return false;
@@ -94,7 +94,7 @@ int Driver::addSecRule(Rule *rule) {
             }
             if (a->m_chained && a->m_chainedRule == NULL) {
                 a->m_chainedRule = rule;
-                if (a->m_containsStaticDisruptiveAction) {
+                if (a->m_theDisruptiveAction) {
                     m_parserError << "Disruptive actions can only be ";
                     m_parserError << "specified by chain starter rules.";
                     return false;

src/parser/seclang-parser.hh

@@ -55,6 +55,7 @@ class Driver;
 
 #include "src/actions/accuracy.h"
 #include "src/actions/audit_log.h"
+#include "src/actions/block.h"
 #include "src/actions/capture.h"
 #include "src/actions/chain.h"
 #include "src/actions/ctl/audit_log_parts.h"
@@ -69,7 +70,6 @@ class Driver;
 #include "src/actions/ctl/rule_remove_target_by_tag.h"
 #include "src/actions/data/status.h"
 #include "src/actions/disruptive/allow.h"
-#include "src/actions/disruptive/block.h"
 #include "src/actions/disruptive/deny.h"
 #include "src/actions/disruptive/pass.h"
 #include "src/actions/disruptive/redirect.h"

src/parser/seclang-parser.yy

@@ -21,6 +21,7 @@ class Driver;
 
 #include "src/actions/accuracy.h"
 #include "src/actions/audit_log.h"
+#include "src/actions/block.h"
 #include "src/actions/capture.h"
 #include "src/actions/chain.h"
 #include "src/actions/ctl/audit_log_parts.h"
@@ -35,7 +36,6 @@ class Driver;
 #include "src/actions/ctl/rule_remove_target_by_tag.h"
 #include "src/actions/data/status.h"
 #include "src/actions/disruptive/allow.h"
-#include "src/actions/disruptive/block.h"
 #include "src/actions/disruptive/deny.h"
 #include "src/actions/disruptive/pass.h"
 #include "src/actions/disruptive/redirect.h"
@@ -1188,7 +1188,7 @@ expression:
         int secRuleDefinedPhase = -1;
         for (actions::Action *a : *actions) {
             actions::Phase *phase = dynamic_cast<actions::Phase *>(a);
-            if (a->isDisruptive() == true && dynamic_cast<actions::disruptive::Block *>(a) == NULL) {
+            if (a->isDisruptive() == true && dynamic_cast<actions::Block *>(a) == NULL) {
                 hasDisruptive = true;
             }
             if (phase != NULL) {
@@ -1522,8 +1522,6 @@ expression:
             driver.error(@0, ss.str());
             YYERROR;
         }
-
-        YYERROR;
       }
     /* Debug log: start */
     | CONFIG_DIR_DEBUG_LVL
@@ -2561,7 +2559,7 @@ act:
       }
     | ACTION_BLOCK
       {
-        ACTION_CONTAINER($$, new actions::disruptive::Block($1));
+        ACTION_CONTAINER($$, new actions::Block($1));
       }
     | ACTION_CAPTURE
       {

src/rule.cc

@@ -38,7 +38,7 @@
 #include "src/actions/capture.h"
 #include "src/actions/multi_match.h"
 #include "src/actions/set_var.h"
-#include "src/actions/disruptive/block.h"
+#include "src/actions/block.h"
 #include "src/variables/variable.h"
 
 
@@ -71,7 +71,7 @@ Rule::Rule(std::string marker)
     m_ver(""),
     m_unconditional(false),
     m_referenceCount(1),
-    m_containsStaticDisruptiveAction(false),
+    m_theDisruptiveAction(nullptr),
     m_containsStaticBlockAction(false),
     m_containsCaptureAction(false),
     m_containsMultiMatchAction(false),
@@ -105,7 +105,7 @@ Rule::Rule(Operator *_op,
     m_ver(""),
     m_unconditional(false),
     m_referenceCount(1),
-    m_containsStaticDisruptiveAction(false),
+    m_theDisruptiveAction(nullptr),
     m_containsStaticBlockAction(false),
     m_containsCaptureAction(false),
     m_containsMultiMatchAction(false),
@@ -180,12 +180,15 @@ void Rule::organizeActions(std::vector<Action *> *actions) {
                     dynamic_cast<actions::SetVar *>(a));
             } else if (dynamic_cast<actions::Tag *>(a)) {
                 m_actionsTag.push_back(dynamic_cast<actions::Tag *>(a));
-            } else if (dynamic_cast<actions::disruptive::Block *>(a)) {
+            } else if (dynamic_cast<actions::Block *>(a)) {
                 m_actionsRuntimePos.push_back(a);
                 m_containsStaticBlockAction = true;
             } else if (a->isDisruptive() == true) {
-                m_actionsRuntimePos.push_back(a);
-                m_containsStaticDisruptiveAction = true;
+                if (m_theDisruptiveAction != nullptr) {
+                    delete m_theDisruptiveAction;
+                    m_theDisruptiveAction = nullptr;
+                }
+                m_theDisruptiveAction = a;
             } else {
                 m_actionsRuntimePos.push_back(a);
             }
@@ -231,6 +234,10 @@ void Rule::cleanUpActions() {
         m_actionsTag.pop_back();
         delete a;
     }
+    if (m_theDisruptiveAction != nullptr) {
+        delete m_theDisruptiveAction;
+        m_theDisruptiveAction = nullptr;
+    }
 }
 
 
@@ -599,12 +606,15 @@ void Rule::executeAction(Transaction *trans,
 
 void Rule::executeActionsAfterFullMatch(Transaction *trans,
     bool containsBlock, std::shared_ptr<RuleMessage> ruleMessage) {
+    bool disruptiveAlreadyExecuted = false;
 
     for (Action *a : trans->m_rules->m_defaultActions[this->m_phase]) {
         if (a->action_kind != actions::Action::RunTimeOnlyIfMatchKind) {
             continue;
         }
-        executeAction(trans, containsBlock, ruleMessage, a, true);
+        if (!a->isDisruptive()) {
+            executeAction(trans, containsBlock, ruleMessage, a, true);
+        }
     }
 
     for (actions::Tag *a : this->m_actionsTag) {
@@ -615,9 +625,6 @@ void Rule::executeActionsAfterFullMatch(Transaction *trans,
         a->evaluate(this, trans, ruleMessage);
     }
 
-    for (Action *a : this->m_actionsRuntimePos) {
-        executeAction(trans, containsBlock, ruleMessage, a, false);
-    }
     for (auto &b :
         trans->m_rules->m_exceptions.m_action_pos_update_target_by_id) {
         if (m_ruleId != b.first) {
@@ -625,6 +632,18 @@ void Rule::executeActionsAfterFullMatch(Transaction *trans,
         }
         actions::Action *a = dynamic_cast<actions::Action*>(b.second.get());
         executeAction(trans, containsBlock, ruleMessage, a, false);
+        disruptiveAlreadyExecuted = true;
+    }
+    for (Action *a : this->m_actionsRuntimePos) {
+        if (!a->isDisruptive()
+                && !(disruptiveAlreadyExecuted
+                && dynamic_cast<actions::Block *>(a))) {
+            executeAction(trans, containsBlock, ruleMessage, a, false);
+        }
+    }
+    if (!disruptiveAlreadyExecuted && m_theDisruptiveAction != nullptr) {
+        executeAction(trans, containsBlock, ruleMessage,
+            m_theDisruptiveAction, false);
     }
 }
 

src/rules_exceptions.cc

@@ -238,6 +238,20 @@ bool RulesExceptions::merge(RulesExceptions *from) {
                     std::move(p.second)));
     }
 
+    for (auto &p : from->m_action_pos_update_target_by_id) {
+        m_action_pos_update_target_by_id.emplace(
+            std::pair<double,
+                std::unique_ptr<actions::Action>>(p.first,
+                    std::move(p.second)));
+    }
+
+    for (auto &p : from->m_action_pre_update_target_by_id) {
+        m_action_pre_update_target_by_id.emplace(
+            std::pair<double,
+                std::unique_ptr<actions::Action>>(p.first,
+                    std::move(p.second)));
+    }
+
     for (auto &p : from->m_remove_rule_by_msg) {
         m_remove_rule_by_msg.push_back(p);
     }

test/test-cases/regression/action-disruptive.json

@@ -32,7 +32,6 @@
     "version_min":300000,
     "title":"Testing Disruptive actions (3/n)",
     "expected":{
-      "debug_log": "Running .disruptive.     action: block",
       "http_code":404
     },
     "rules":[

test/test-cases/regression/config-update-action-by-id.json

@@ -0,0 +1,227 @@
+[
+  {
+    "enabled":1,
+    "version_min":300000,
+    "title":"SecRuleUpdateActionById (1/n)",
+    "client":{
+      "ip":"200.249.12.31",
+      "port":123
+    },
+    "server":{
+      "ip":"200.249.12.31",
+      "port":80
+    },
+    "request":{
+      "headers":{
+        "Host":"localhost",
+        "User-Agent":"curl/7.38.0",
+        "Accept":"*/*",
+        "Content-Length":"330",
+        "Content-Type":"application/lhebs",
+        "Expect":"100-continue"
+      },
+      "uri":"/a=urlencoded?param1=value1",
+      "method":"GET"
+    },
+    "response":{
+      "headers":{
+        "Date":"Mon, 13 Jul 2015 20:02:41 GMT",
+        "Last-Modified":"Sun, 26 Oct 2014 22:33:37 GMT",
+        "Content-Type":"text/html"
+      },
+      "body":[
+        "no need."
+      ]
+    },
+    "expected":{
+      "http_code": 200,
+      "debug_log": "Skipped rule id '200005'"
+    },
+    "rules":[
+        "SecRuleEngine On",
+        "SecRuleUpdateActionById 200004 \"allow\"",
+        "SecRule ARGS \"@contains value1\" \"phase:3,id:200004,deny\"",
+        "SecRule ARGS \"@contains value1\" \"phase:3,id:200005,log\""
+    ]
+  },
+  {
+    "enabled":1,
+    "version_min":300000,
+    "title":"SecRuleUpdateActionById (2/n)",
+    "client":{
+      "ip":"200.249.12.31",
+      "port":123
+    },
+    "server":{
+      "ip":"200.249.12.31",
+      "port":80
+    },
+    "request":{
+      "headers":{
+        "Host":"localhost",
+        "User-Agent":"curl/7.38.0",
+        "Accept":"*/*",
+        "Content-Length":"330",
+        "Content-Type":"application/lhebs",
+        "Expect":"100-continue"
+      },
+      "uri":"/a=urlencoded?param1=value1",
+      "method":"GET"
+    },
+    "response":{
+      "headers":{
+        "Date":"Mon, 13 Jul 2015 20:02:41 GMT",
+        "Last-Modified":"Sun, 26 Oct 2014 22:33:37 GMT",
+        "Content-Type":"text/html"
+      },
+      "body":[
+        "no need."
+      ]
+    },
+    "expected":{
+      "http_code": 403
+    },
+    "rules":[
+        "SecRuleEngine On",
+        "SecRule ARGS \"@contains value1\" \"phase:3,id:200004,deny\""
+    ]
+  },
+  {
+    "enabled":1,
+    "version_min":300000,
+    "title":"SecRuleUpdateActionById (3/n)",
+    "client":{
+      "ip":"200.249.12.31",
+      "port":123
+    },
+    "server":{
+      "ip":"200.249.12.31",
+      "port":80
+    },
+    "request":{
+      "headers":{
+        "Host":"localhost",
+        "User-Agent":"curl/7.38.0",
+        "Accept":"*/*",
+        "Content-Length":"330",
+        "Content-Type":"application/lhebs",
+        "Expect":"100-continue"
+      },
+      "uri":"/a=urlencoded?param1=value1",
+      "method":"GET"
+    },
+    "response":{
+      "headers":{
+        "Date":"Mon, 13 Jul 2015 20:02:41 GMT",
+        "Last-Modified":"Sun, 26 Oct 2014 22:33:37 GMT",
+        "Content-Type":"text/html"
+      },
+      "body":[
+        "no need."
+      ]
+    },
+    "expected":{
+      "http_code": 200,
+      "debug_log": "Running action: log"
+    },
+    "rules":[
+        "SecRuleEngine On",
+        "SecRuleUpdateActionById 200004 \"pass\"",
+        "SecRule ARGS \"@contains value1\" \"phase:3,id:200004,deny\"",
+        "SecRule ARGS \"@contains value1\" \"phase:3,id:200005,log\""
+    ]
+  },
+  {
+    "enabled":1,
+    "version_min":300000,
+    "title":"SecRuleUpdateActionById (4/n)",
+    "client":{
+      "ip":"200.249.12.31",
+      "port":123
+    },
+    "server":{
+      "ip":"200.249.12.31",
+      "port":80
+    },
+    "request":{
+      "headers":{
+        "Host":"localhost",
+        "User-Agent":"curl/7.38.0",
+        "Accept":"*/*",
+        "Content-Length":"330",
+        "Content-Type":"application/lhebs",
+        "Expect":"100-continue"
+      },
+      "uri":"/a=urlencoded?param1=value1",
+      "method":"GET"
+    },
+    "response":{
+      "headers":{
+        "Date":"Mon, 13 Jul 2015 20:02:41 GMT",
+        "Last-Modified":"Sun, 26 Oct 2014 22:33:37 GMT",
+        "Content-Type":"text/html"
+      },
+      "body":[
+        "no need."
+      ]
+    },
+    "expected":{
+      "http_code": 200,
+      "debug_log": "Running action: log"
+    },
+    "rules":[
+        "SecRuleEngine On",
+        "SecRuleUpdateActionById 200004 \"pass\"",
+        "SecDefaultAction \"phase:3,deny\"",
+        "SecRule ARGS \"@contains value1\" \"phase:3,id:200004,block\"",
+        "SecRule ARGS \"@contains value1\" \"phase:3,id:200005,log\""
+    ]
+  },
+  {
+    "enabled":1,
+    "version_min":300000,
+    "title":"SecRuleUpdateActionById (5/n)",
+    "client":{
+      "ip":"200.249.12.31",
+      "port":123
+    },
+    "server":{
+      "ip":"200.249.12.31",
+      "port":80
+    },
+    "request":{
+      "headers":{
+        "Host":"localhost",
+        "User-Agent":"curl/7.38.0",
+        "Accept":"*/*",
+        "Content-Length":"330",
+        "Content-Type":"application/lhebs",
+        "Expect":"100-continue"
+      },
+      "uri":"/a=urlencoded?param1=value1",
+      "method":"GET"
+    },
+    "response":{
+      "headers":{
+        "Date":"Mon, 13 Jul 2015 20:02:41 GMT",
+        "Last-Modified":"Sun, 26 Oct 2014 22:33:37 GMT",
+        "Content-Type":"text/html"
+      },
+      "body":[
+        "no need."
+      ]
+    },
+    "expected":{
+      "http_code": 200,
+      "debug_log": "Dropping the evaluation of upcoming rules in favor of"
+    },
+    "rules":[
+        "SecRuleEngine On",
+        "SecRuleUpdateActionById 200004 \"allow\"",
+        "SecDefaultAction \"phase:3,deny\"",
+        "SecRule ARGS \"@contains value1\" \"phase:3,id:200004,block\"",
+        "SecRule ARGS \"@contains value1\" \"phase:3,id:200005,log\""
+    ]
+  }
+]
+

test/test-cases/regression/issue-960.json

@@ -69,8 +69,8 @@
   },
     "expected": {
       "audit_log": "",
-      "debug_log": "\\(Rule: 960032\\) .* Rule returned 1.",
-      "error_log": ""
+      "error_log": "",
+      "http_code": 418
     },
   "rules": [
     "SecDefaultAction \"phase:1,log,deny,status:418,tag:'Host: %{request_headers.host}'\"",
@@ -109,8 +109,8 @@
   },
     "expected": {
       "audit_log": "",
-      "debug_log": "\\(Rule: 960032\\) .* Rule returned 1.",
-      "error_log": ""
+      "error_log": "",
+      "http_code": 418
     },
   "rules": [
     "SecDefaultAction \"phase:1,log,deny,status:418,tag:'Host: %{request_headers.host}'\"",

test/test-cases/regression/secruleengine.json

@@ -34,8 +34,7 @@
     "version_min":300000,
     "title":"Testing Disruptive actions (3/n)",
     "expected":{
-      "debug_log": "Not running disruptive action: block. SecRuleEngine is not On",
-      "http_code":200
+      "http_code":404
     },
     "rules":[
       "SecRuleEngine On",