Fix disruptive action flow while RuleEngine is in DetectionOnly

headers/modsecurity/intervention.h

@@ -25,6 +25,7 @@ typedef struct ModSecurityIntervention_t {
     int pause;
     const char *url;
     const char *log;
+    int disruptive;
 } ModSecurityIntervention;
 
 #ifdef __cplusplus

src/actions/block.cc

@@ -20,6 +20,7 @@
 
 #include "modsecurity/assay.h"
 #include "src/rule.h"
+#include "modsecurity/intervention.h"
 
 namespace ModSecurity {
 namespace actions {
@@ -32,6 +33,7 @@ Block::Block(std::string action)
 
 
 bool Block::evaluate(Rule *rule, Assay *assay) {
+    assay->debug(8, "Running action block");
     for (Action *a : rule->actions_runtime_pos) {
         if (a->isDisruptive() == true) {
             assay->actions.push_back(a);
@@ -41,7 +43,7 @@ bool Block::evaluate(Rule *rule, Assay *assay) {
 }
 
 void Block::fill_intervention(ModSecurityIntervention *i) {
-
+    i->disruptive = true;
 }
 
 }  // namespace actions

src/actions/deny.cc

@@ -31,13 +31,17 @@ Deny::Deny(std::string action)
 
 
 bool Deny::evaluate(Rule *rule, Assay *assay) {
+    assay->debug(8, "Running action deny");
     assay->actions.push_back(this);
     return true;
 }
 
 void Deny::fill_intervention(ModSecurityIntervention *i) {
-    i->status = 403;
+    if (i->status == 200) {
+        i->status = 403;
+    }
     i->log = "Deny action";
+    i->disruptive = true;
 }
 
 }  // namespace actions

src/assay.cc

@@ -1048,7 +1048,6 @@ void Assay::cleanup() {
  *
  */
 bool Assay::intervention(ModSecurityIntervention *it) {
-    bool ret = false;
     it->status = 200;
     it->url = NULL;
     if (actions.size() > 0) {
@@ -1061,9 +1060,8 @@ bool Assay::intervention(ModSecurityIntervention *it) {
             }
         }
         actions.clear();
-        ret = true;
     }
-    return ret;
+    return it->disruptive;
 }
 
 

src/rule.cc

@@ -30,6 +30,7 @@
 #include "actions/transformations/none.h"
 #include "variables/variations/exclusion.h"
 #include "src/utils.h"
+#include "modsecurity/rules.h"
 #include "src/macro_expansion.h"
 
 using ModSecurity::Variables::Variations::Exclusion;
@@ -128,6 +129,7 @@ Rule::Rule(Operator *_op,
 
 bool Rule::evaluateActions(Assay *assay) {
     int none = 0;
+    bool containsDisruptive = false;
     int transformations = 0;
     for (Action *a : this->actions_runtime_pre) {
         None *z = dynamic_cast<None *>(a);
@@ -168,17 +170,54 @@ bool Rule::evaluateActions(Assay *assay) {
         }
     }
 
-    for (Action *a : assay->m_rules->defaultActions[this->phase]) {
+    for (Action *a : this->actions_runtime_pos) {
-        if (a->action_kind == actions::Action::RunTimeOnlyIfMatchKind) {
+        if (a->isDisruptive() == false) {
-            assay->debug(4, "(SecDefaultAction) Running action: " + a->action);
+            assay->debug(4, "Running (_non_ disruptive) action: " + a->action);
             a->evaluate(this, assay);
+        } else {
+            containsDisruptive = true;
         }
     }
 
-    for (Action *a :
+    for (Action *a : assay->m_rules->defaultActions[this->phase]) {
-        this->actions_runtime_pos) {
+        if (a->action_kind == actions::Action::RunTimeOnlyIfMatchKind) {
-        assay->debug(4, "Running action: " + a->action);
+            if (a->isDisruptive()) {
-        a->evaluate(this, assay);
+                if (containsDisruptive) {
+                    assay->debug(4, "(SecDefaultAction) " \
+                        "_ignoring_ action: " + a->action + \
+                        " (rule contains a disruptive action)");
+                } else {
+                    if (assay->m_rules->secRuleEngine
+                        == Rules::EnabledRuleEngine) {
+                        assay->debug(4, "(SecDefaultAction) " \
+                            "Running action: " + a->action + \
+                            " (rule _does not_ contains a " \
+                            "disruptive action)");
+                            a->evaluate(this, assay);
+                    } else {
+                        assay->debug(4, "(SecDefaultAction) " \
+                            "_Not_ running action: " + a->action + \
+                            ". Rule _does not_ contains a " \
+                            "disruptive action, but SecRuleEngine is not On.");
+                    }
+                }
+            } else {
+                assay->debug(4, "(SecDefaultAction) Running action: " + \
+                    a->action);
+                    a->evaluate(this, assay);
+            }
+        }
+    }
+
+    for (Action *a : this->actions_runtime_pos) {
+        if (a->isDisruptive()
+            && assay->m_rules->secRuleEngine == Rules::EnabledRuleEngine) {
+            assay->debug(4, "Running (disruptive) action: " + a->action);
+            a->evaluate(this, assay);
+        } else if (a->isDisruptive()) {
+            assay->debug(4, "Not running disruptive action: " + \
+                a->action + ". SecRuleEngine is not On");
+        }
     }
 
     return true;
@@ -343,24 +382,39 @@ bool Rule::evaluate(Assay *assay) {
                                         "_ignoring_ action: " + a->action + \
                                         " (rule contains a disruptive action)");
                                 } else {
-                                    assay->debug(4, "(SecDefaultAction) " \
+                                    if (assay->m_rules->secRuleEngine
-                                        "Running action: " + a->action + \
+                                        == Rules::EnabledRuleEngine) {
-                                        " (rule _does not_ contains a " \
+                                        assay->debug(4, "(SecDefaultAction) " \
-                                        "disruptive action)");
+                                            "Running action: " + a->action + \
-                                    a->evaluate(this, assay);
+                                            " (rule _does not_ contains a " \
+                                            "disruptive action)");
+                                        a->evaluate(this, assay);
+                                    } else {
+                                        assay->debug(4, "(SecDefaultAction) " \
+                                            "_Not_ running action: " + a->action + \
+                                            ". Rule _does not_ contains a " \
+                                            "disruptive action, but SecRuleEngine is not On.");
+                                    }
                                 }
                             } else {
                                 assay->debug(4, "(SecDefaultAction) Running " \
-                                    "action: " + a->action);
+                                    "action: " + a->action + "!!" + std::to_string(a->isDisruptive()));
                                 a->evaluate(this, assay);
                             }
                         }
                     }
                     for (Action *a :
                         this->actions_runtime_pos) {
-                        if (a->isDisruptive()) {
+                        if (a->isDisruptive()
-                            assay->debug(4, "Running (disruptive) action: " + a->action);
+                            && assay->m_rules->secRuleEngine
+                                == Rules::EnabledRuleEngine) {
+                            assay->debug(4, "Running (disruptive) action: " + \
+                                a->action);
                             a->evaluate(this, assay);
+                        } else if (a->isDisruptive()) {
+                            assay->debug(4,
+                                "Not running disruptive action: " + \
+                                a->action + ". SecRuleEngine is not On");
                         }
                     }
                 }

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

@@ -5,7 +5,7 @@
     "title":"Testing Disruptive actions (1/n)",
     "expected":{
       "debug_log": " Running action: deny",
-      "http_code":403
+      "http_code":404
     },
     "rules":[
       "SecRuleEngine On",
@@ -34,7 +34,7 @@
     "version_min":300000,
     "title":"Testing Disruptive actions (3/n)",
     "expected":{
-      "debug_log": " Running action: deny",
+      "debug_log": "Running action block",
       "http_code":404
     },
     "rules":[

test/test-cases/regression/actions.json

@@ -242,14 +242,14 @@
       "SecRuleEngine On",
       "SecDebugLog \/tmp\/modsec_debug.log",
       "SecDebugLogLevel 9",
-      "SecRule ARGS \"@contains test\" \"id:1,t:trim,status:500\""
+      "SecRule ARGS \"@contains test\" \"id:1,t:trim,deny,status:500\""
     ]
   },
          {
     "enabled": 1,
     "version_min": 300000,
     "version_max": 0,
-    "title": "actions :: phase:1,trim,status:500",
+    "title": "actions :: phase:1,trim,status:500,deny",
     "client": {
       "ip": "200.249.12.31",
       "port": 2313
@@ -303,14 +303,14 @@
       "SecRuleEngine On",
       "SecDebugLog \/tmp\/modsec_debug.log",
       "SecDebugLogLevel 9",
-      "SecRule ARGS \"@contains test\" \"id:1,phase:1,t:trim,status:500\""
+      "SecRule ARGS \"@contains test\" \"id:1,phase:1,t:trim,status:500,deny\""
     ]
   },
   {
     "enabled": 1,
     "version_min": 300000,
     "version_max": 0,
-    "title": "actions :: phase:4,trim,status:500",
+    "title": "actions :: phase:4,trim,status:500,deny",
     "client": {
       "ip": "200.249.12.31",
       "port": 2313
@@ -364,7 +364,7 @@
       "SecRuleEngine On",
       "SecDebugLog \/tmp\/modsec_debug.log",
       "SecDebugLogLevel 9",
-      "SecRule ARGS \"@contains test\" \"id:1,phase:4,t:trim,status:500\""
+      "SecRule ARGS \"@contains test\" \"id:1,phase:4,t:trim,status:500,deny\""
     ]
   }
 ]

test/test-cases/regression/secruleengine.json

@@ -0,0 +1,50 @@
+[
+  {
+    "enabled":1,
+    "version_min":300000,
+    "title":"Testing Disruptive actions (1/n)",
+    "expected":{
+      "debug_log": " Running action: deny",
+      "http_code":404
+    },
+    "rules":[
+      "SecRuleEngine On",
+      "SecDebugLog \/tmp\/modsec_debug.log",
+      "SecRuleEngine On",
+      "SecDefaultAction \"phase:2,deny,status:404\"",
+      "SecAction \"id:'900001',phase:request,nolog,status:403,t:none\""
+    ]
+  },
+  {
+    "enabled":1,
+    "version_min":300000,
+    "title":"Testing Disruptive actions (2/n)",
+    "expected":{
+      "debug_log": "_Not_ running action: deny. Rule _does not_ contains a disruptive action, but SecRuleEngine is not On.",
+      "http_code":200
+    },
+    "rules":[
+      "SecRuleEngine On",
+      "SecDebugLog \/tmp\/modsec_debug.log",
+      "SecRuleEngine Off",
+      "SecDefaultAction \"phase:2,deny,status:404\"",
+      "SecAction \"'id:'1',phase:request,nolog,t:none\""
+    ]
+  },
+  {
+    "enabled":1,
+    "version_min":300000,
+    "title":"Testing Disruptive actions (3/n)",
+    "expected":{
+      "debug_log": "Not running disruptive action: block. SecRuleEngine is not On",
+      "http_code":200
+    },
+    "rules":[
+      "SecRuleEngine On",
+      "SecDebugLog \/tmp\/modsec_debug.log",
+      "SecRuleEngine DetectionOnly",
+      "SecDefaultAction \"phase:2,deny,status:404\"",
+      "SecAction \"id:'1',phase:request,nolog,block,t:none\""
+    ]
+  }
+]