From 202a15bea8e6e009bb5c03737d565d685f7db9aa Mon Sep 17 00:00:00 2001 From: Felipe Zimmerle Date: Thu, 31 May 2018 14:33:13 -0300 Subject: [PATCH] Changes the behavior of the default sec actions Fix #1629 --- CHANGES | 2 ++ src/rule.cc | 27 +++++++++---------- .../regression/action-disruptive.json | 8 +++--- .../regression/config-secdefaultaction.json | 4 +-- test/test-cases/regression/secruleengine.json | 12 ++++----- 5 files changed, 27 insertions(+), 26 deletions(-) diff --git a/CHANGES b/CHANGES index 587ef56e..64c49051 100644 --- a/CHANGES +++ b/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] diff --git a/src/rule.cc b/src/rule.cc index 1250fe93..1ecdf106 100644 --- a/src/rule.cc +++ b/src/rule.cc @@ -241,15 +241,14 @@ void Rule::updateRulesVariable(Transaction *trans) { void Rule::executeActionsIndependentOfChainedRuleResult(Transaction *trans, - bool *containsDisruptive, std::shared_ptr ruleMessage) { + bool *containsBlock, std::shared_ptr 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> Rule::getFinalVars( void Rule::executeActionsAfterFullMatch(Transaction *trans, - bool containsDisruptive, std::shared_ptr ruleMessage) { + bool containsBlock, std::shared_ptr 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,7 +689,7 @@ 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; @@ -698,7 +697,7 @@ void Rule::executeActionsAfterFullMatch(Transaction *trans, #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 *variables = this->m_variables; bool recursiveGlobalRet; - bool containsDisruptive = false; + bool containsBlock = false; std::vector> 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 @@ end_clean: 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); diff --git a/test/test-cases/regression/action-disruptive.json b/test/test-cases/regression/action-disruptive.json index 7772f84d..d889afd5 100644 --- a/test/test-cases/regression/action-disruptive.json +++ b/test/test-cases/regression/action-disruptive.json @@ -4,13 +4,13 @@ "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\"" ] }, { @@ -18,13 +18,13 @@ "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\"" ] }, { diff --git a/test/test-cases/regression/config-secdefaultaction.json b/test/test-cases/regression/config-secdefaultaction.json index 852e913c..bb3d7d81 100644 --- a/test/test-cases/regression/config-secdefaultaction.json +++ b/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\"" ] } ] diff --git a/test/test-cases/regression/secruleengine.json b/test/test-cases/regression/secruleengine.json index 31d74ffe..4e7b6055 100644 --- a/test/test-cases/regression/secruleengine.json +++ b/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\"" ] } ]