diff --git a/headers/modsecurity/intervention.h b/headers/modsecurity/intervention.h index ce8adf3d..645b2a58 100644 --- a/headers/modsecurity/intervention.h +++ b/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 diff --git a/src/actions/block.cc b/src/actions/block.cc index 59367669..346df07e 100644 --- a/src/actions/block.cc +++ b/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 diff --git a/src/actions/deny.cc b/src/actions/deny.cc index bf1178f5..f86ab992 100644 --- a/src/actions/deny.cc +++ b/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 diff --git a/src/assay.cc b/src/assay.cc index 9f06d194..5e857b08 100644 --- a/src/assay.cc +++ b/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; } diff --git a/src/rule.cc b/src/rule.cc index 13b91907..487ee350 100644 --- a/src/rule.cc +++ b/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(a); @@ -168,17 +170,54 @@ bool Rule::evaluateActions(Assay *assay) { } } - for (Action *a : assay->m_rules->defaultActions[this->phase]) { - if (a->action_kind == actions::Action::RunTimeOnlyIfMatchKind) { - assay->debug(4, "(SecDefaultAction) Running action: " + a->action); + for (Action *a : this->actions_runtime_pos) { + if (a->isDisruptive() == false) { + assay->debug(4, "Running (_non_ disruptive) action: " + a->action); a->evaluate(this, assay); + } else { + containsDisruptive = true; } } - for (Action *a : - this->actions_runtime_pos) { - assay->debug(4, "Running action: " + a->action); - a->evaluate(this, assay); + for (Action *a : assay->m_rules->defaultActions[this->phase]) { + if (a->action_kind == actions::Action::RunTimeOnlyIfMatchKind) { + if (a->isDisruptive()) { + 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) " \ - "Running action: " + a->action + \ - " (rule _does not_ contains a " \ - "disruptive action)"); - a->evaluate(this, assay); + 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); + "action: " + a->action + "!!" + std::to_string(a->isDisruptive())); a->evaluate(this, assay); } } } for (Action *a : this->actions_runtime_pos) { - if (a->isDisruptive()) { - assay->debug(4, "Running (disruptive) action: " + a->action); + 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"); } } } diff --git a/test/test-cases/regression/action-disruptive.json b/test/test-cases/regression/action-disruptive.json index 25e82be5..9ac4e711 100644 --- a/test/test-cases/regression/action-disruptive.json +++ b/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":[ diff --git a/test/test-cases/regression/actions.json b/test/test-cases/regression/actions.json index e293aa50..fa8a6cbe 100644 --- a/test/test-cases/regression/actions.json +++ b/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\"" ] } ] diff --git a/test/test-cases/regression/secruleengine.json b/test/test-cases/regression/secruleengine.json new file mode 100644 index 00000000..94e367db --- /dev/null +++ b/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\"" + ] + } +] \ No newline at end of file