Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

TX.partial_match returned when TX.partial requested #1818

Closed
michaelgranzow-avi opened this issue Jun 25, 2018 · 4 comments
Closed

TX.partial_match returned when TX.partial requested #1818

michaelgranzow-avi opened this issue Jun 25, 2018 · 4 comments
Assignees
Labels
3.x Related to ModSecurity version 3.x

Comments

@michaelgranzow-avi
Copy link
Contributor

"SecRuleEngine On",
"SecRule REMOTE_ADDR \"@unconditionalMatch\" \"id:1,deny,setvar:TX.partial_match=1,chain\"",
"SecRule TX.partial \"@gt 0\" \"id:2,t:lowercase,t:none,status:444\""

The second (chained) rule should never fire because there is no variable 'TX.partial', only 'TX.partial_match'. But it does fire:

[8] Saving variable: TX:partial_match with value: 1
[4] Rule returned 1.
[4] Executing chained rule.
[4] (Rule: 2) Executing operator "Gt" with param "0" against TX:partial.
[9] Target value: "1" (Variable: TX:partial)
[9] Matched vars updated.
[4] Rule returned 1.
@zimmerle
Copy link
Contributor

Hi @michaelgranzow-avi,

Thank you. We already saw your comments on victor`s pull request. It is on our queue to be fixed.

@zimmerle zimmerle self-assigned this Jun 25, 2018
@zimmerle zimmerle added the 3.x Related to ModSecurity version 3.x label Jun 25, 2018
@michaelgranzow-avi
Copy link
Contributor Author

michaelgranzow-avi commented Jun 26, 2018

This change fixes both issues (the partial match and the performance degradation) by going back to the original code:

commit ec90bad44e6bf27eeae98a9ca5474b303fec1110 (HEAD -> Issue-1818_partial-match_v3-master)
Author: michaelgranzow-avi <michael@avinetworks.com>
Date:   Mon Jun 25 21:43:33 2018 +0200

    Variable names must match fully, not partially; also revert to hash table lookup instead of linear search

diff --git a/src/collection/backend/in_memory-per_process.cc b/src/collection/backend/in_memory-per_process.cc
index 65e3371d..f627aaef 100644
--- a/src/collection/backend/in_memory-per_process.cc
+++ b/src/collection/backend/in_memory-per_process.cc
@@ -105,10 +105,9 @@ void InMemoryPerProcess::resolveMultiMatches(const std::string& var,
             l->insert(l->begin(), new VariableValue(&m_name, &i.first, &i.second));
         }
     } else {
-        for (auto &a : *this) {
-            if (a.first.compare(0, var.size(), var) == 0) {
-                l->insert(l->begin(), new VariableValue(&m_name, &var, &a.second));
-            }
+        auto range = this->equal_range(var);
+        for (auto it = range.first; it != range.second; ++it) {
+            l->insert(l->begin(), new VariableValue(&m_name, &var, &it->second));
         }
     }
 }
diff --git a/test/test-cases/regression/collection-tx.json b/test/test-cases/regression/collection-tx.json
index 127f701c..076d6b72 100644
--- a/test/test-cases/regression/collection-tx.json
+++ b/test/test-cases/regression/collection-tx.json
@@ -1,4 +1,43 @@
 [
+   {
+    "enabled": 1,
+    "version_min":300000,
+    "version_max":0,
+    "title":"AAAAA :: TX full vs partial match",
+    "client":{
+      "ip":"200.249.12.31",
+      "port":2313
+    },
+    "server":{
+      "ip":"200.249.12.31",
+      "port":80
+    },
+    "request":{
+      "headers":{
+        "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)"
+      },
+      "uri":"/",
+      "method":"GET",
+      "http_version":1.1,
+      "body":""
+    },
+    "response":{
+      "headers":{
+        "Content-Type":"text\/xml; charset=utf-8\n\r"
+      },
+      "body":[
+        "<?xml version=\"1.0\" encoding=\"utf-8\"?>\n"
+      ]
+    },
+    "expected":{
+       "http_code":200
+    },
+    "rules":[
+      "SecRuleEngine On",
+      "SecRule REMOTE_ADDR \"@unconditionalMatch\" \"id:1,deny,setvar:TX.partial_match=1,chain\"",
+      "SecRule TX.partial \"@gt 0\" \"id:2,t:lowercase,t:none,status:444\""
+    ]
+  },
   {
     "enabled":1,
     "version_min":300000,

michaelgranzow-avi added a commit to avinetworks/ModSecurity that referenced this issue Jun 26, 2018
…y; also revert to hash table lookup instead of linear search; add test case
@michaelgranzow-avi
Copy link
Contributor Author

Proposed fix: #1820 - this would make #1810 unnecessary.

zimmerle pushed a commit that referenced this issue Jun 26, 2018
… hash table lookup instead of linear search; add test case
@zimmerle
Copy link
Contributor

fixed by: d810de9

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
3.x Related to ModSecurity version 3.x
Projects
None yet
Development

No branches or pull requests

2 participants