From 3d1d0514fd17018b1cced4629a5d94a418915e2f Mon Sep 17 00:00:00 2001 From: Felipe Zimmerle Date: Fri, 1 Jul 2016 11:01:51 -0300 Subject: [PATCH] Fix pass action behaviour: now only ingore actions within the same rule More details on issue #1152 --- src/actions/pass.cc | 1 - src/rule.cc | 22 ++++- test/test-cases/regression/issue-1152.json | 106 ++++++++++++++++++++- 3 files changed, 124 insertions(+), 5 deletions(-) diff --git a/src/actions/pass.cc b/src/actions/pass.cc index 135e06c9..d75bf810 100644 --- a/src/actions/pass.cc +++ b/src/actions/pass.cc @@ -26,7 +26,6 @@ namespace actions { bool Pass::evaluate(Rule *rule, Transaction *transaction) { - transaction->m_actions.clear(); return true; } diff --git a/src/rule.cc b/src/rule.cc index 12b47b7a..ec6d1152 100644 --- a/src/rule.cc +++ b/src/rule.cc @@ -201,7 +201,7 @@ bool Rule::evaluateActions(Transaction *trasn) { if (a->isDisruptive() == false) { #ifndef NO_LOGS trasn->debug(4, "Running (_non_ disruptive) action: " + - a->m_name); + a->m_name + "."); #endif a->evaluate(this, trasn); } else { @@ -395,6 +395,7 @@ bool Rule::evaluate(Transaction *trasn) { if (ret) { bool containsDisruptive = false; bool chainResult = false; + bool containsPassAction = false; ruleMessage->m_match = "Operator `" + this->op->op + "' with parameter `" + this->op->param + "' against" \ @@ -415,6 +416,10 @@ bool Rule::evaluate(Transaction *trasn) { this->actions_runtime_pos) { if (a->isDisruptive() == true) { containsDisruptive = true; + if (a->m_name == "pass") { + containsPassAction = true; + trasn->debug(4, "Rule contains a `pass' action"); + } } } @@ -477,19 +482,30 @@ bool Rule::evaluate(Transaction *trasn) { this->actions_runtime_pos) { if (a->isDisruptive() && trasn->m_rules->secRuleEngine - == Rules::EnabledRuleEngine) { + == Rules::EnabledRuleEngine + && containsPassAction == false) { #ifndef NO_LOGS trasn->debug(4, "Running (disruptive) " \ "action: " + a->m_name); #endif a->evaluate(this, trasn); - } else if (a->isDisruptive()) { + } else if (a->isDisruptive() + && containsPassAction == false) { #ifndef NO_LOGS trasn->debug(4, "Not running disruptive action: " + \ a->m_name + ". SecRuleEngine " + \ "is not On"); #endif + } else if (a->isDisruptive() && + containsPassAction == true) { + if (a->m_name != "pass") { +#ifndef NO_LOGS + trasn->debug(4, "Not running disruptive " \ + "action: " + a->m_name + ". It was " \ + "silenced by an `pass' action."); +#endif + } } else if (!a->isDisruptive()) { #ifndef NO_LOGS trasn->debug(4, "Running (_non_ disruptive) " \ diff --git a/test/test-cases/regression/issue-1152.json b/test/test-cases/regression/issue-1152.json index 5d4c8ec3..54c78f79 100644 --- a/test/test-cases/regression/issue-1152.json +++ b/test/test-cases/regression/issue-1152.json @@ -39,7 +39,7 @@ } }, "expected": { - "http_code": 200 + "http_code": 403 }, "rules": [ "SecRuleEngine On", @@ -96,5 +96,109 @@ "SecRule ARGS:foo \"bar\" \"id:'900017',phase:1,t:none,deny,nolog,msg:'foo = bar'\"", "SecRule &TX:REAL_IP \"@eq 0\" \"id:'900021',phase:1,t:none,initcol:global=global,initcol:ip=%{remote_addr}_%{tx.ua_hash},setvar:tx.real_ip=%{remote_addr},nolog\"" ] + }, + { + "enabled": 1, + "version_min": 209000, + "version_max": -1, + "title": "Should libmodsec pass action clear m_actions?", + "url": "https:\/\/github.com\/SpiderLabs\/ModSecurity\/issues\/1152", + "client": { + "ip": "200.249.12.31", + "port": 2313 + }, + "server": { + "ip": "200.249.12.31", + "port": 80 + }, + "request": { + "headers": { + "Host": "net.tutsplus.com", + "User-Agent": "Mozilla\/5.0 (Windows; U; Windows NT 6.1; en-US; rv:1.9.1.5) Gecko\/20091102 Firefox\/3.5.5 (.NET CLR 3.5.30729)", + "Accept": "text\/html,application\/xhtml+xml,application\/xml;q=0.9,*\/*;q=0.8", + "Accept-Language": "en-us,en;q=0.5", + "Accept-Encoding": "gzip,deflate", + "Accept-Charset": "ISO-8859-1,utf-8;q=0.7,*;q=0.7", + "Keep-Alive": "300", + "Connection": "keep-alive", + "Cookie": "PHPSESSID=r2t5uvjq435r4q7ib3vtdjq120", + "Pragma": "no-cache", + "Cache-Control": "no-cache" + }, + "uri": "\/test.pl?foo=bar", + "method": "GET", + "http_version": 1.1, + "body": "" + }, + "response": { + "headers": { + "Content-Type": "text\/xml; charset=utf-8\n\r", + "Content-Length": "length\n\r" + }, + "body": [ + ] + }, + "expected": { + "http_code": 200 + }, + "rules": [ + "SecRuleEngine On", + "SecRule ARGS:foo \"bar\" \"id:'900017',phase:1,t:none,nolog,msg:'foo = bar'\"", + "SecRule &TX:REAL_IP \"@eq 0\" \"id:'900021',phase:1,t:none,initcol:global=global,initcol:ip=%{remote_addr}_%{tx.ua_hash},setvar:tx.real_ip=%{remote_addr},nolog,deny,pass\"" + ] + }, + { + "enabled": 1, + "version_min": 209000, + "version_max": -1, + "title": "Should libmodsec pass action clear m_actions?", + "url": "https:\/\/github.com\/SpiderLabs\/ModSecurity\/issues\/1152", + "client": { + "ip": "200.249.12.31", + "port": 2313 + }, + "server": { + "ip": "200.249.12.31", + "port": 80 + }, + "request": { + "headers": { + "Host": "net.tutsplus.com", + "User-Agent": "Mozilla\/5.0 (Windows; U; Windows NT 6.1; en-US; rv:1.9.1.5) Gecko\/20091102 Firefox\/3.5.5 (.NET CLR 3.5.30729)", + "Accept": "text\/html,application\/xhtml+xml,application\/xml;q=0.9,*\/*;q=0.8", + "Accept-Language": "en-us,en;q=0.5", + "Accept-Encoding": "gzip,deflate", + "Accept-Charset": "ISO-8859-1,utf-8;q=0.7,*;q=0.7", + "Keep-Alive": "300", + "Connection": "keep-alive", + "Cookie": "PHPSESSID=r2t5uvjq435r4q7ib3vtdjq120", + "Pragma": "no-cache", + "Cache-Control": "no-cache" + }, + "uri": "\/test.pl?a=test&b=test&c=test&d=test", + "method": "GET", + "http_version": 1.1, + "body": "" + }, + "response": { + "headers": { + "Content-Type": "text\/xml; charset=utf-8\n\r", + "Content-Length": "length\n\r" + }, + "body": [ + ] + }, + "expected": { + "http_code": 200, + "debug_log": "Target value: \"4\" \\(Variable: TX:test\\)" + }, + "rules": [ + "SecRuleEngine On", + "# Set TX.test to zero", + "SecAction \"phase:2,nolog,pass,setvar:TX.test=0,id:123\"", + "# Increment TX.test for every request parameter", + "SecRule ARGS \"test\" \"phase:2,log,pass,setvar:TX.test=+1,id:124\"", + "SecRule TX:test \"test\" \"phase:2,log,pass,id:125\"" + ] } ]