Changes the behavior of the default sec actions

Fix #1629

Author: Felipe Zimmerle

CHANGES

@@ -1,6 +1,8 @@
 v3.0.3 - YYYY-MMM-DD (to be released)
 -------------------------------------
 
+ - Changes the behavior of the default sec actions
+   [Issue #1629 - @mirkodziadzka-avi, @zimmerle, @victorhora]
  - Refactoring on {global,ip,resources,session,tx,user} collections
    [Issue #1754, #1778 - @LeeShan87, @zimmerle, @victorhora, @wwd5613,
                          @sobigboy]

src/rule.cc

@@ -241,15 +241,14 @@ void Rule::updateRulesVariable(Transaction *trans) {
 
 
 void Rule::executeActionsIndependentOfChainedRuleResult(Transaction *trans,
-    bool *containsDisruptive, std::shared_ptr<RuleMessage> ruleMessage) {
+    bool *containsBlock, std::shared_ptr<RuleMessage> ruleMessage) {
     for (Action *a : this->m_actionsRuntimePos) {
         if (a->isDisruptive() == true) {
-            if (a->m_name == "pass") {
+            if (a->m_name == "block") {
 #ifndef NO_LOGS
-                trans->debug(9, "Rule contains a `pass' action");
+                trans->debug(9, "Rule contains a `block' action");
+                *containsBlock = true;
 #endif
-            } else {
-                *containsDisruptive = true;
             }
         } else {
             if (a->m_name == "setvar" || a->m_name == "msg"
@@ -661,7 +660,7 @@ std::vector<std::unique_ptr<VariableValue>> Rule::getFinalVars(
 
 
 void Rule::executeActionsAfterFullMatch(Transaction *trans,
-    bool containsDisruptive, std::shared_ptr<RuleMessage> ruleMessage) {
+    bool containsBlock, std::shared_ptr<RuleMessage> ruleMessage) {
 
     for (Action *a : trans->m_rules->m_defaultActions[this->m_phase]) {
         if (a->action_kind != actions::Action::RunTimeOnlyIfMatchKind) {
@@ -677,11 +676,11 @@ void Rule::executeActionsAfterFullMatch(Transaction *trans,
             continue;
         }
 
-        if (containsDisruptive) {
+        if (!containsBlock) {
 #ifndef NO_LOGS
             trans->debug(4, "(SecDefaultAction) ignoring " \
                 "action: " + a->m_name + \
-                " (rule contains a disruptive action)");
+                " (rule does not cotains block)");
 #endif
             continue;
         }
@@ -690,15 +689,15 @@ void Rule::executeActionsAfterFullMatch(Transaction *trans,
 #ifndef NO_LOGS
             trans->debug(4, "(SecDefaultAction) " \
                 "Running action: " + a->m_name + \
-                " (rule does not contain a disruptive action)");
+                ".");
 #endif
             a->evaluate(this, trans, ruleMessage);
             continue;
         }
 
 #ifndef NO_LOGS
         trans->debug(4, "(SecDefaultAction) Not running action: " \
-                + a->m_name + ". Rule does not contain a disruptive action,"\
+                + a->m_name + ". Rule contains 'block',"\
                 + " but SecRuleEngine is not On.");
 #endif
     }
@@ -736,7 +735,7 @@ bool Rule::evaluate(Transaction *trans,
     bool globalRet = false;
     std::vector<Variable *> *variables = this->m_variables;
     bool recursiveGlobalRet;
-    bool containsDisruptive = false;
+    bool containsBlock = false;
     std::vector<std::unique_ptr<VariableValue>> finalVars;
     std::string eparam;
 
@@ -756,7 +755,7 @@ bool Rule::evaluate(Transaction *trans,
             + ") Executing unconditional rule...");
 #endif
         executeActionsIndependentOfChainedRuleResult(trans,
-            &containsDisruptive, ruleMessage);
+            &containsBlock, ruleMessage);
         goto end_exec;
     }
 
@@ -827,7 +826,7 @@ bool Rule::evaluate(Transaction *trans,
                 ruleMessage->m_reference.append(*valueTemp.second);
                 updateMatchedVars(trans, key, value);
                 executeActionsIndependentOfChainedRuleResult(trans,
-                    &containsDisruptive, ruleMessage);
+                    &containsBlock, ruleMessage);
                 globalRet = true;
             }
         }
@@ -870,7 +869,7 @@ bool Rule::evaluate(Transaction *trans,
     return false;
 
 end_exec:
-    executeActionsAfterFullMatch(trans, containsDisruptive, ruleMessage);
+    executeActionsAfterFullMatch(trans, containsBlock, ruleMessage);
     if (m_ruleId != 0 && ruleMessage->m_saveMessage != false) {
         trans->serverLog(ruleMessage);
         trans->m_rulesMessages.push_back(*ruleMessage);

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

@@ -4,27 +4,27 @@
     "version_min":300000,
     "title":"Testing Disruptive actions (1/n)",
     "expected":{
-      "debug_log": " Running action: deny",
+      "debug_log": "Running action deny",
       "http_code":403
     },
     "rules":[
       "SecRuleEngine On",
       "SecDefaultAction \"phase:2,deny,status:404\"",
-      "SecAction \"id:'900001',phase:request,nolog,status:403,t:none\""
+      "SecAction \"id:'900001',phase:request,nolog,status:403,t:none,block\""
     ]
   },
   {
     "enabled":1,
     "version_min":300000,
     "title":"Testing Disruptive actions (2/n)",
     "expected":{
-      "debug_log": " Running action: deny",
+      "debug_log": "Running action deny",
       "http_code":404
     },
     "rules":[
       "SecRuleEngine On",
       "SecDefaultAction \"phase:2,deny,status:404\"",
-      "SecAction \"id:'1',phase:request,nolog,t:none\""
+      "SecAction \"id:'1',phase:request,nolog,t:none,block\""
     ]
   },
   {

test/test-cases/regression/config-secdefaultaction.json

@@ -278,8 +278,8 @@
     "rules":[
       "SecRuleEngine On",
       "SecDefaultAction \"phase:2,log,auditlog,status:302,redirect:'http://www.google.com'\"",
-      "SecRule REQUEST_HEADERS \"@contains PHPSESSID\" \"phase:2,id:1\"",
-      "SecRule TX \"@contains to_test\" \"id:2,t:lowercase,t:none\""
+      "SecRule REQUEST_HEADERS \"@contains PHPSESSID\" \"phase:2,id:1,block\"",
+      "SecRule TX \"@contains to_test\" \"id:2,t:lowercase,t:none,block\""
     ]
   }
 ]

test/test-cases/regression/secruleengine.json

@@ -4,14 +4,14 @@
     "version_min":300000,
     "title":"Testing Disruptive actions (1/n)",
     "expected":{
-      "debug_log": " Running action: deny",
+      "debug_log": " Running action deny",
       "http_code":403
     },
     "rules":[
       "SecRuleEngine On",
       "SecRuleEngine On",
       "SecDefaultAction \"phase:2,deny,status:404\"",
-      "SecAction \"id:'900001',phase:request,nolog,status:403,t:none\""
+      "SecAction \"id:'900001',phase:request,nolog,status:403,t:none,block\""
     ]
   },
   {
@@ -26,7 +26,7 @@
       "SecRuleEngine On",
       "SecRuleEngine Off",
       "SecDefaultAction \"phase:2,deny,status:404\"",
-      "SecAction \"id:'1',phase:request,nolog,t:none\""
+      "SecAction \"id:'1',phase:request,nolog,t:none,block\""
     ]
   },
   {
@@ -41,7 +41,7 @@
       "SecRuleEngine On",
       "SecRuleEngine DetectionOnly",
       "SecDefaultAction \"phase:2,deny,status:404\"",
-      "SecAction \"id:'1',phase:request,nolog,nolog,block,t:none\""
+      "SecAction \"id:'1',phase:request,nolog,nolog,block,t:none,block\""
     ]
   },
   {
@@ -56,7 +56,7 @@
       "SecRuleEngine On",
       "SecRuleEngine Off",
       "SecDefaultAction \"phase:2,deny,status:404\"",
-      "SecAction \"id:'1',phase:request,nolog,t:none\""
+      "SecAction \"id:'1',phase:request,nolog,t:none,block\""
     ]
   },
   {
@@ -71,7 +71,7 @@
       "SecRuleEngine On",
       "SecRuleEngine Off",
       "SecDefaultAction \"phase:2,deny,status:404\"",
-      "SecAction \"id:'1',phase:request,nolog,block,t:none\""
+      "SecAction \"id:'1',phase:request,nolog,block,t:none,block\""
     ]
   }
 ]