Adds support to multiple ranges in ctl:ruleRemoveById

Issue #1956

CHANGES

@@ -1,6 +1,8 @@
 v3.0.4 - YYYY-MMM-DD (to be released)
 -------------------------------------
 
+ - Adds support to multiple ranges in ctl:ruleRemoveById
+   [Issue #1956 - @theseion, @victorhora, @zimmerle]
  - Rule variable interpolation broken
    [Issue #1961 - @soonum, @zimmerle] 
  - Make the boundary check less strict as per RFC2046

Makefile.am

@@ -95,6 +95,7 @@ TESTS+=test/test-cases/regression/issue-1850.json
 TESTS+=test/test-cases/regression/issue-1725.json
 TESTS+=test/test-cases/regression/issue-1941.json
 TESTS+=test/test-cases/regression/issue-1943.json
+TESTS+=test/test-cases/regression/issue-1956.json
 TESTS+=test/test-cases/regression/variable-RESPONSE_HEADERS.json
 TESTS+=test/test-cases/regression/config-include.json
 TESTS+=test/test-cases/regression/variable-WEBSERVER_ERROR_LOG.json

headers/modsecurity/transaction.h

@@ -461,6 +461,7 @@ class Transaction : public TransactionAnchoredVariables {
      *
      */
     std::list<int > m_ruleRemoveById;
+    std::list<std::pair<int, int> > m_ruleRemoveByIdRange;
 
     /**
      *

src/actions/ctl/rule_remove_by_id.cc

@@ -19,6 +19,7 @@
 #include <string>
 
 #include "modsecurity/transaction.h"
+#include "src/utils/string.h"
 
 namespace modsecurity {
 namespace actions {
@@ -27,20 +28,69 @@ namespace ctl {
 
 bool RuleRemoveById::init(std::string *error) {
     std::string what(m_parser_payload, 15, m_parser_payload.size() - 15);
+    bool added = false;
+    std::vector<std::string> toRemove = utils::string::ssplit(what, ' ');
+    for (std::string &a : toRemove) {
+        std::string b = modsecurity::utils::string::parserSanitizer(a);
+        if (b.size() == 0) {
+            continue;
+        }
 
+        size_t dash = b.find('-');
+        if (dash != std::string::npos) {
+            std::string n1s = std::string(b, 0, dash);
+            std::string n2s = std::string(b, dash + 1, b.size() - (dash + 1));
+            int n1n = 0;
+            int n2n = 0;
             try {
-        m_id = std::stoi(what);
+                n1n = std::stoi(n1s);
+                added = true;
             } catch (...) {
-        error->assign("Not able to convert '" + what +
-            "' into a number");
+                error->assign("Not a number: " + n1s);
+                return false;
+            }
+            try {
+                n2n = std::stoi(n2s);
+                added = true;
+            } catch (...) {
+                error->assign("Not a number: " + n2s);
                 return false;
             }
 
+            if (n1s > n2s) {
+                error->assign("Invalid range: " + b);
+                return false;
+            }
+            m_ranges.push_back(std::make_pair(n1n, n2n));
+            added = true;
+        } else {
+            try {
+                int num = std::stoi(b);
+                m_ids.push_back(num);
+                added = true;
+            } catch (...) {
+                error->assign("Not a number or range: " + b);
+                return false;
+            }
+        }
+    }
+
+    if (added) {
         return true;
     }
 
+    error->assign("Not a number or range: " + what);
+    return false;
+}
+
 bool RuleRemoveById::evaluate(Rule *rule, Transaction *transaction) {
-    transaction->m_ruleRemoveById.push_back(m_id);
+    for (auto &i : m_ids) {
+        transaction->m_ruleRemoveById.push_back(i);
+    }
+    for (auto &i : m_ranges) {
+        transaction->m_ruleRemoveByIdRange.push_back(i);
+    }
+
     return true;
 }
 

src/actions/ctl/rule_remove_by_id.h

@@ -30,13 +30,13 @@ namespace ctl {
 class RuleRemoveById : public Action {
  public:
     explicit RuleRemoveById(std::string action)
-        : Action(action, RunTimeOnlyIfMatchKind),
-        m_id(0) { }
+        : Action(action, RunTimeOnlyIfMatchKind) { }
 
     bool init(std::string *error) override;
     bool evaluate(Rule *rule, Transaction *transaction) override;
 
-    int m_id;
+    std::list<std::pair<int, int> > m_ranges;
+    std::list<int> m_ids;
 };
 
 

src/rule.cc

@@ -656,6 +656,14 @@ bool Rule::evaluate(Transaction *trans,
             " was skipped due to a ruleRemoveById action...");
         return true;
     }
+    for (auto &i : trans->m_ruleRemoveByIdRange) {
+        if (!(i.first <= m_ruleId && i.second >= m_ruleId)) {
+            continue;
+        }
+        ms_dbg_a(trans, 9, "Rule id: " + std::to_string(m_ruleId) +
+            " was skipped due to a ruleRemoveById action...");
+        return true;
+    }
 
     if (m_op->m_string) {
         eparam = m_op->m_string->evaluate(trans);

test/test-cases/regression/issue-1956.json

@@ -0,0 +1,192 @@
+[
+{
+  "enabled": 1,
+  "version_min": 209000,
+  "version_max": -1,
+  "title": "ctl:ruleRemoveById doesn't handle all ranges equally 1",
+  "url": "https:\/\/github.com\/SpiderLabs\/ModSecurity\/issues\/1956",
+  "gihub_issue": 1956,
+  "client": {
+    "ip": "200.249.12.31",
+    "port": 2313
+  },
+  "server": {
+    "ip": "200.249.12.31",
+    "port": 80
+  },
+  "request": {
+      "headers": {
+        "Host": "www.google.com"
+      },
+    "uri": "\/test.pl?param1=   test   &param2=<a href=\"javascript:alert(1)\">)",
+    "body": "",
+    "method": "GET",
+    "http_version": 1.1
+  },
+  "response": {
+    "headers": "",
+    "body": ""
+  },
+    "expected": {
+      "audit_log": "",
+      "debug_log": "Rule id: 913104 was skipped due to a ruleRemoveById",
+      "error_log": ""
+    },
+  "rules": [
+      "SecRule REQUEST_URI \"@beginsWith /test\" \"id:1001,phase:request,pass,nolog,t:none,ctl:ruleRemoveById=913103-913105\"",
+      "SecRule REQUEST_URI \"@beginsWith /test\" \"id:913104,phase:request,pass,nolog,t:none,msg:'whee'\""
+    ]
+  },
+  {
+  "enabled": 1,
+  "version_min": 209000,
+  "version_max": -1,
+  "title": "ctl:ruleRemoveById doesn't handle all ranges equally 2",
+  "url": "https:\/\/github.com\/SpiderLabs\/ModSecurity\/issues\/1956",
+  "gihub_issue": 1956,
+  "client": {
+    "ip": "200.249.12.31",
+    "port": 2313
+  },
+  "server": {
+    "ip": "200.249.12.31",
+    "port": 80
+  },
+  "request": {
+      "headers": {
+        "Host": "www.google.com"
+      },
+    "uri": "\/test.pl?param1=   test   &param2=<a href=\"javascript:alert(1)\">)",
+    "body": "",
+    "method": "GET",
+    "http_version": 1.1
+  },
+  "response": {
+    "headers": "",
+    "body": ""
+  },
+    "expected": {
+      "audit_log": "",
+      "debug_log": "Rule id: 913104 was skipped due to a ruleRemoveById",
+      "error_log": ""
+    },
+  "rules": [
+      "SecRule REQUEST_URI \"@beginsWith /test\" \"id:1001,phase:request,pass,nolog,t:none,ctl:ruleRemoveById=913104\"",
+      "SecRule REQUEST_URI \"@beginsWith /test\" \"id:913104,phase:request,pass,nolog,t:none,msg:'whee'\""
+    ]
+  },
+  {
+  "enabled": 1,
+  "version_min": 209000,
+  "version_max": -1,
+  "title": "ctl:ruleRemoveById doesn't handle all ranges equally 3",
+  "url": "https:\/\/github.com\/SpiderLabs\/ModSecurity\/issues\/1956",
+  "gihub_issue": 1956,
+  "client": {
+    "ip": "200.249.12.31",
+    "port": 2313
+  },
+  "server": {
+    "ip": "200.249.12.31",
+    "port": 80
+  },
+  "request": {
+      "headers": {
+        "Host": "www.google.com"
+      },
+    "uri": "\/test.pl?param1=   test   &param2=<a href=\"javascript:alert(1)\">)",
+    "body": "",
+    "method": "GET",
+    "http_version": 1.1
+  },
+  "response": {
+    "headers": "",
+    "body": ""
+  },
+    "expected": {
+      "audit_log": "",
+      "debug_log": "Rule id: 913103 was skipped due to a ruleRemoveById",
+      "error_log": ""
+    },
+  "rules": [
+      "SecRule REQUEST_URI \"@beginsWith /test\" \"id:1001,phase:request,pass,nolog,t:none,ctl:ruleRemoveById=913103-913105\"",
+      "SecRule REQUEST_URI \"@beginsWith /test\" \"id:913103,phase:request,pass,nolog,t:none,msg:'whee'\""
+    ]
+  },
+  {
+  "enabled": 1,
+  "version_min": 209000,
+  "version_max": -1,
+  "title": "ctl:ruleRemoveById doesn't handle all ranges equally 4",
+  "url": "https:\/\/github.com\/SpiderLabs\/ModSecurity\/issues\/1956",
+  "gihub_issue": 1956,
+  "client": {
+    "ip": "200.249.12.31",
+    "port": 2313
+  },
+  "server": {
+    "ip": "200.249.12.31",
+    "port": 80
+  },
+  "request": {
+      "headers": {
+        "Host": "www.google.com"
+      },
+    "uri": "\/test.pl?param1=   test   &param2=<a href=\"javascript:alert(1)\">)",
+    "body": "",
+    "method": "GET",
+    "http_version": 1.1
+  },
+  "response": {
+    "headers": "",
+    "body": ""
+  },
+    "expected": {
+      "audit_log": "",
+      "debug_log": "Rule id: 913105 was skipped due to a ruleRemoveById",
+      "error_log": ""
+    },
+  "rules": [
+      "SecRule REQUEST_URI \"@beginsWith /test\" \"id:1001,phase:request,pass,nolog,t:none,ctl:ruleRemoveById=913103-913105\"",
+      "SecRule REQUEST_URI \"@beginsWith /test\" \"id:913105,phase:request,pass,nolog,t:none,msg:'whee'\""
+    ]
+  },
+  {
+  "enabled": 1,
+  "version_min": 209000,
+  "version_max": -1,
+  "title": "ctl:ruleRemoveById doesn't handle all ranges equally 5",
+  "url": "https:\/\/github.com\/SpiderLabs\/ModSecurity\/issues\/1956",
+  "gihub_issue": 1956,
+  "client": {
+    "ip": "200.249.12.31",
+    "port": 2313
+  },
+  "server": {
+    "ip": "200.249.12.31",
+    "port": 80
+  },
+  "request": {
+      "headers": {
+        "Host": "www.google.com"
+      },
+    "uri": "\/test.pl?param1=   test   &param2=<a href=\"javascript:alert(1)\">)",
+    "body": "",
+    "method": "GET",
+    "http_version": 1.1
+  },
+  "response": {
+    "headers": "",
+    "body": ""
+  },
+    "expected": {
+      "audit_log": "",
+      "debug_log": "Rule: 913102. Executing operator",
+      "error_log": ""
+    },
+  "rules": [
+      "SecRule REQUEST_URI \"@beginsWith /test\" \"id:1001,phase:request,pass,nolog,t:none,ctl:ruleRemoveById=913103-913105\"",
+      "SecRule REQUEST_URI \"@beginsWith /test\" \"id:913102,phase:request,pass,nolog,t:none,msg:'whee'\""
+    ]
+  }
+]