Adds support to multiple ranges in ctl:ruleRemoveTargetById (owasp-modsecurity#2110)

Author: Hamza Zerhouni

CHANGES

@@ -30,14 +30,16 @@ v3.0.4 - YYYY-MMM-DD (to be released)
    [@rdiperri-yottaa, @danbiagini-work, @mmelo-yottaa, @zimmerle]
  - Missing throw in Operator::instantiate
    [Issue #2106 - @marduone]
+ - Adds support to multiple ranges in ctl:ruleRemoveTargetById
+   [Issue #2110 - @theMiddleBlue, @zimmerle]
  - Making block action execution dependent of the SecEngine status
    [Issue #2113, #2111 - @theMiddleBlue, @airween]
  - Making block action execution dependent of the SecEngine status
    [Issue #1960 - @theMiddleBlue, @zimmerle, @airween, @victorhora]
  - Having body limits to respect the rule engine state
    [@zimmerle]
  - Fix SecRuleUpdateTargetById does not match regular expressions
-   [Issue #1872 - @zimmerle, @anush-cr, @victorhora, @j0k2r]  
+   [Issue #1872 - @zimmerle, @anush-cr, @victorhora, @j0k2r]
  - Adds missing check for runtime ctl:ruleRemoveByTag
    [Issue #2102, #2099 - @airween]
  - Adds a new operator verifySVNR that checks for Austrian social
@@ -98,7 +100,7 @@ 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] 
+   [Issue #1961 - @soonum, @zimmerle]
  - Make the boundary check less strict as per RFC2046
    [Issue #1943 - @victorhora, @allanbomsft]
  - Fix buffer size for utf8toUnicode transformation
@@ -239,7 +241,7 @@ v3.0.3 - 2018-Nov-05
    [Issue #1739 - @p0pr0ck5]
  - Fix utils::string::ssplit() to handle delimiter in the end of string
    [Issue #1743, #1744 - @defanator]
- - Fix variable FILES_TMPNAMES 
+ - Fix variable FILES_TMPNAMES
    [Issue #1646, #1610 - @victorhora, @zimmerle, @defanator]
  - Fix memory leak in Collections
    [Issue #1729, #1730 - @defanator]
@@ -341,7 +343,7 @@ v3.0.0 - 2017-Dec-13
  - Adds support to WEBAPPID variable.
    [Issue #1027 - @zimmerle, @victorhora]
  - Adds support for SecWebAppId.
-   [Issue #1442 - @zimmerle, @victorhora] 
+   [Issue #1442 - @zimmerle, @victorhora]
  - Adds support for SecRuleRemoveByTag.
    [Issue #1476 - @zimmerle, @victorhora]
  - Adds support for update target by message.
@@ -404,4 +406,3 @@ v3.0.0-rc1 - 2017-Aug-28
 ------------------------
 
  Very first public version.
-

Makefile.am

@@ -134,6 +134,7 @@ TESTS+=test/test-cases/regression/config-update-target-by-tag.json
 TESTS+=test/test-cases/regression/config-xml_external_entity.json
 TESTS+=test/test-cases/regression/debug_log.json
 TESTS+=test/test-cases/regression/directive-sec_rule_script.json
+TESTS+=test/test-cases/regression/issue-2110.json
 TESTS+=test/test-cases/regression/issue-1152.json
 TESTS+=test/test-cases/regression/issue-1528.json
 TESTS+=test/test-cases/regression/issue-1565.json

headers/modsecurity/transaction.h

@@ -477,6 +477,7 @@ class Transaction : public TransactionAnchoredVariables {
      *
      */
     std::list< std::pair<int, std::string> > m_ruleRemoveTargetById;
+    std::list< std::pair<std::pair<int, int>, std::string> > m_ruleRemoveTargetByIdRange;
 
     /**
      *

src/actions/ctl/rule_remove_target_by_id.cc

@@ -33,28 +33,73 @@ bool RuleRemoveTargetById::init(std::string *error) {
     std::string what(m_parser_payload, 21, m_parser_payload.size() - 21);
     std::vector<std::string> param = utils::string::split(what, ';');
 
+    bool added = false;
+
     if (param.size() < 2) {
         error->assign(what + " is not a valid `ID;VARIABLE'");
         return false;
     }
 
-    try {
-        m_id = std::stoi(param[0]);
-    } catch(...) {
-        error->assign("Not able to convert '" + param[0] +
-            "' into a number");
-        return false;
+    size_t dash = param[0].find('-');
+    if (dash != std::string::npos) {
+        std::string n1s = std::string(param[0], 0, dash);
+        std::string n2s = std::string(param[0], dash + 1, param[0].size() - (dash + 1));
+        int n1n = 0;
+        int n2n = 0;
+        try {
+            n1n = std::stoi(n1s);
+            added = true;
+        } catch (...) {
+            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 (n1n > n2n) {
+            error->assign("Invalid range: " + param[0]);
+            return false;
+        }
+        m_ranges.push_back(std::make_pair(n1n, n2n));
+        added = true;
+    } else {
+        try {
+            int num = std::stoi(param[0]);
+            m_ids.push_back(num);
+            added = true;
+        } catch (...) {
+            error->assign("Not able to convert '" + param[0] +
+              "' into a number");
+            return false;
+        }
     }
 
-    m_target = param[1];
+    if (added) {
+      m_target = param[1];
+      return true;
+    }
 
-    return true;
+    error->assign("Not a number or range: " + what);
+    return false;
 }
 
 bool RuleRemoveTargetById::evaluate(Rule *rule, Transaction *transaction) {
+  for (auto &i : m_ids) {
     transaction->m_ruleRemoveTargetById.push_back(
-        std::make_pair(m_id, m_target));
-    return true;
+      std::make_pair(i, m_target));
+  }
+
+  for (auto &i : m_ranges) {
+      transaction->m_ruleRemoveTargetByIdRange.push_back(
+        std::make_pair(i, m_target));
+  }
+
+  return true;
 }
 
 

src/actions/ctl/rule_remove_target_by_id.h

@@ -30,14 +30,13 @@ namespace ctl {
 class RuleRemoveTargetById : public Action {
  public:
     explicit RuleRemoveTargetById(std::string action)
-        : Action(action, RunTimeOnlyIfMatchKind),
-        m_id(0),
-        m_target("") { }
+        : 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;
     std::string m_target;
 };
 

src/rule.cc

@@ -710,6 +710,23 @@ bool Rule::evaluate(Transaction *trans,
                 v = NULL;
                 continue;
             }
+
+	    if (exclusion.contains(v->getKey()) ||
+                std::find_if(trans->m_ruleRemoveTargetByIdRange.begin(),
+                    trans->m_ruleRemoveTargetByIdRange.end(),
+                    [&, v, this](std::pair<std::pair<int, int>, std::string> &m) -> bool {
+                      if (m.first.first <= m_ruleId && m.first.second >= m_ruleId) {
+                        return m.second == v->getKey();
+                      }
+
+		      return false;
+                    }) != trans->m_ruleRemoveTargetByIdRange.end()
+            ) {
+                delete v;
+                v = NULL;
+                continue;
+            }
+
             if (exclusion.contains(v->getKey()) ||
                 std::find_if(trans->m_ruleRemoveTargetByTag.begin(),
                     trans->m_ruleRemoveTargetByTag.end(),

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

@@ -0,0 +1,33 @@
+[
+  {
+    "enabled":1,
+    "version_min":300000,
+    "title":"Testing ctl:ruleRemoveTargetById - issue 1444",
+    "expected":{
+      "http_code":200
+    },
+    "client":{
+      "ip":"127.0.0.1",
+      "port":123
+    },
+    "request":{
+      "headers":{
+        "Host":"localhost",
+        "User-Agent":"curl/7.38.0",
+        "Accept":"*/*"
+      },
+      "uri":"index.php?foo=bar&z=xxx",
+      "method":"GET",
+      "body": ""
+    },
+    "server":{
+      "ip":"127.0.0.1",
+      "port":80
+    },
+    "rules":[
+        "SecRuleEngine On",
+        "SecRule ARGS:foo \"@rx ^bar$\" \"id:100,phase:1,ctl:ruleRemoveTargetById=1000-1999;ARGS:z\"",
+        "SecRule ARGS:z \"@rx ^xxx$\" \"id:1010,phase:1,deny,status:403\""
+    ]
+  }
+]