Issue-2423: Meta-actions like 'msg' should be applied at end of chain

owasp-modsecurity · Oct 29, 2020 · 1b7aa42 · 1b7aa42
1 parent 2672db1
commit 1b7aa42
Show file tree

Hide file tree

Showing 4 changed files with 141 additions and 11 deletions.
diff --git a/CHANGES b/CHANGES
@@ -1,6 +1,8 @@
 v3.x.y - YYYY-MMM-DD (to be released)
 -------------------------------------
 
+  - Fixed MatchedVar on chained rules
+    [Issue #2423, #2435, #2436 - @michaelgranzow-avi]
   - Add support for new operator rxGlobal
     [@martinhsv]
   - Fix maxminddb link on FreeBSD

diff --git a/Makefile.am b/Makefile.am
@@ -158,6 +158,7 @@ TESTS+=test/test-cases/regression/issue-2099.json
 TESTS+=test/test-cases/regression/issue-2000.json
 TESTS+=test/test-cases/regression/issue-2111.json
 TESTS+=test/test-cases/regression/issue-2196.json
+TESTS+=test/test-cases/regression/issue-2423-msg-in-chain.json
 TESTS+=test/test-cases/regression/issue-394.json
 TESTS+=test/test-cases/regression/issue-849.json
 TESTS+=test/test-cases/regression/issue-960.json

diff --git a/src/rule_with_actions.cc b/src/rule_with_actions.cc
@@ -215,17 +215,6 @@ void RuleWithActions::executeActionsIndependentOfChainedRuleResult(Transaction *
         }
     }
 
-    if (m_severity) {
-        m_severity->evaluate(this, trans, ruleMessage);
-    }
-
-    if (m_logData) {
-        m_logData->evaluate(this, trans, ruleMessage);
-    }
-
-    if (m_msg) {
-        m_msg->evaluate(this, trans, ruleMessage);
-    }
 }
 
 
@@ -257,6 +246,17 @@ void RuleWithActions::executeActionsAfterFullMatch(Transaction *trans,
         executeAction(trans, containsBlock, ruleMessage, a, false);
         disruptiveAlreadyExecuted = true;
     }
+    if (m_severity) {
+        m_severity->evaluate(this, trans, ruleMessage);
+    }
+
+    if (m_logData) {
+        m_logData->evaluate(this, trans, ruleMessage);
+    }
+
+    if (m_msg) {
+        m_msg->evaluate(this, trans, ruleMessage);
+    }
     for (Action *a : this->m_actionsRuntimePos) {
         if (!a->isDisruptive()
                 && !(disruptiveAlreadyExecuted

diff --git a/test/test-cases/regression/issue-2423-msg-in-chain.json b/test/test-cases/regression/issue-2423-msg-in-chain.json
@@ -0,0 +1,127 @@
+[
+  {
+    "enabled":1,
+    "version_min":300000,
+    "title":"Test match variable (1/n)",
+    "github_issue": 2423,
+    "expected":{
+        "http_code": 437,
+        "error_log": "against variable `REQUEST_HEADERS:Transfer-Encoding' .Value: `deflate'"
+    },
+    "client":{
+      "ip":"200.249.12.31",
+      "port":123
+    },
+    "request":{
+      "headers":{
+        "Host":"localhost",
+        "Transfer-Encoding": "deflate"
+      },
+      "uri":"/match-this",
+      "method":"GET"
+    },
+    "server":{
+      "ip":"200.249.12.31",
+      "port":80
+    },
+    "rules":[
+      "SecRuleEngine On",
+      "SecRule REQUEST_URI \"^.*$\" \"phase:2,deny,capture,id:1,msg:'MatchedVar On Msg: [%{MATCHED_VAR}]',logdata:'MatchedVar On LogData %{MATCHED_VAR}',chain\"",
+      "SecRule REQUEST_HEADERS \"^.*$\" \"status:437\""
+    ]
+  },
+  {
+    "enabled":1,
+    "version_min":300000,
+    "title":"Test match variable (2/n)",
+    "github_issue": 2423,
+    "expected":{
+        "http_code": 437,
+        "error_log": "MatchedVar On Msg: .deflate."
+    },
+    "client":{
+      "ip":"200.249.12.31",
+      "port":123
+    },
+    "request":{
+      "headers":{
+        "Host":"localhost",
+        "Transfer-Encoding": "deflate"
+      },
+      "uri":"/match-this",
+      "method":"GET"
+    },
+    "server":{
+      "ip":"200.249.12.31",
+      "port":80
+    },
+    "rules":[
+      "SecRuleEngine On",
+      "SecRule REQUEST_URI \"^.*$\" \"phase:2,deny,capture,id:1,msg:'MatchedVar On Msg: [%{MATCHED_VAR}]',logdata:'MatchedVar On LogData %{MATCHED_VAR}',chain\"",
+      "SecRule REQUEST_HEADERS \"^.*$\" \"status:437\""
+    ]
+  },
+  {
+    "enabled":1,
+    "version_min":300000,
+    "title":"Test match variable (3/n)",
+    "github_issue": 2423,
+    "expected":{
+        "http_code": 437,
+        "error_log": "MatchedVar On LogData: deflate"
+    },
+    "client":{
+      "ip":"200.249.12.31",
+      "port":123
+    },
+    "request":{
+      "headers":{
+        "Host":"localhost",
+        "Transfer-Encoding": "deflate"
+      },
+      "uri":"/match-this",
+      "method":"GET"
+    },
+    "server":{
+      "ip":"200.249.12.31",
+      "port":80
+    },
+    "rules":[
+      "SecRuleEngine On",
+      "SecRule REQUEST_URI \"^.*$\" \"phase:2,deny,capture,id:1,msg:'MatchedVar On Msg: [%{MATCHED_VAR}]',logdata:'MatchedVar On LogData: %{MATCHED_VAR}',chain\"",
+      "SecRule REQUEST_HEADERS \"^.*$\" \"status:437\""
+    ]
+  },
+  {
+    "enabled":1,
+    "version_min":300000,
+    "title":"Test match variable (4/n)",
+    "github_issue": 2423,
+    "expected":{
+        "http_code": 437,
+        "error_log": "msg \"Illegal header \\[/restricted/\\]\""
+    },
+    "client":{
+      "ip":"200.249.12.31",
+      "port":123
+    },
+    "request":{
+      "headers":{
+        "Host":"localhost",
+        "Restricted":"attack",
+	      "Other": "Value"
+      },
+      "uri":"/",
+      "method":"GET"
+    },
+    "server":{
+      "ip":"200.249.12.31",
+      "port":80
+    },
+    "rules":[
+      "SecRuleEngine On",
+      "SecRule REQUEST_HEADERS_NAMES \"^.*$\" \"phase:2,setvar:'tx.header_name_%{TX.0}=/%{TX.0}/',deny,t:lowercase,capture,id:500065,msg:'Illegal header [%{MATCHED_VAR}]',logdata:'Restricted header detected: %{MATCHED_VAR}',chain\"",
+      "SecRule TX:/^header_name_/ \"@within /name1/restricted/name3/\" \"status:437\""
+    ]
+  }
+]