Merge pull request #2736 from brandonpayton/add-regex-match-limits-and-error-reporting

Add isolated PCRE match limits as a layer of ReDoS defense

src/operators/rx.cc

@@ -51,12 +51,36 @@ bool Rx::evaluate(Transaction *transaction, RuleWithActions *rule,
         re = m_re;
     }
 
-    std::vector<Utils::SMatchCapture> captures;
     if (re->hasError()) {
         ms_dbg_a(transaction, 3, "Error with regular expression: \"" + re->pattern + "\"");
         return false;
     }
-    re->searchOneMatch(input, captures);
+
+    Utils::RegexResult regex_result;
+    std::vector<Utils::SMatchCapture> captures;
+
+    if (transaction && transaction->m_rules->m_pcreMatchLimit.m_set) {
+        unsigned long match_limit = transaction->m_rules->m_pcreMatchLimit.m_value;
+        regex_result = re->searchOneMatch(input, captures, match_limit);
+    } else {
+        regex_result = re->searchOneMatch(input, captures);
+    }
+
+    // FIXME: DRY regex error reporting. This logic is currently duplicated in other operators.
+    if (regex_result != Utils::RegexResult::Ok) {
+        transaction->m_variableMscPcreError.set("1", transaction->m_variableOffset);
+
+        std::string regex_error_str = "OTHER";
+        if (regex_result == Utils::RegexResult::ErrorMatchLimit) {
+            regex_error_str = "MATCH_LIMIT";
+            transaction->m_variableMscPcreLimitsExceeded.set("1", transaction->m_variableOffset);
+        }
+
+        ms_dbg_a(transaction, 1, "rx: regex error '" + regex_error_str + "' for pattern '" + re->pattern + "'");
+
+
+        return false;
+    }
 
     if (rule && rule->hasCaptureAction() && transaction) {
         for (const Utils::SMatchCapture& capture : captures) {

src/operators/rx_global.cc

@@ -51,8 +51,30 @@ bool RxGlobal::evaluate(Transaction *transaction, RuleWithActions *rule,
         re = m_re;
     }
 
+    Utils::RegexResult regex_result;
     std::vector<Utils::SMatchCapture> captures;
-    re->searchGlobal(input, captures);
+    if (transaction && transaction->m_rules->m_pcreMatchLimit.m_set) {
+        unsigned long match_limit = transaction->m_rules->m_pcreMatchLimit.m_value;
+        regex_result = re->searchGlobal(input, captures, match_limit);
+    } else {
+        regex_result = re->searchGlobal(input, captures);
+    }
+
+    // FIXME: DRY regex error reporting. This logic is currently duplicated in other operators.
+    if (regex_result != Utils::RegexResult::Ok) {
+        transaction->m_variableMscPcreError.set("1", transaction->m_variableOffset);
+
+        std::string regex_error_str = "OTHER";
+        if (regex_result == Utils::RegexResult::ErrorMatchLimit) {
+            regex_error_str = "MATCH_LIMIT";
+            transaction->m_variableMscPcreLimitsExceeded.set("1", transaction->m_variableOffset);
+        }
+
+        ms_dbg_a(transaction, 1, "rxGlobal: regex error '" + regex_error_str + "' for pattern '" + re->pattern + "'");
+
+        return false;
+    }
+
 
     if (rule && rule->hasCaptureAction() && transaction) {
         for (const Utils::SMatchCapture& capture : captures) {

src/parser/location.hh

@@ -1,4 +1,4 @@
-// A Bison parser, made by GNU Bison 3.7.6.
+// A Bison parser, made by GNU Bison 3.8.2.
 
 // Locations for Bison parsers in C++
 

src/parser/position.hh

@@ -1,4 +1,4 @@
-// A Bison parser, made by GNU Bison 3.7.6.
+// A Bison parser, made by GNU Bison 3.8.2.
 
 // Starting with Bison 3.2, this file is useless: the structure it
 // used to define is now defined in "location.hh".

src/parser/seclang-parser.yy

@@ -184,6 +184,8 @@ class Driver;
 #include "src/variables/matched_vars.h"
 #include "src/variables/matched_vars_names.h"
 #include "src/variables/modsec_build.h"
+#include "src/variables/msc_pcre_error.h"
+#include "src/variables/msc_pcre_limits_exceeded.h"
 #include "src/variables/multipart_boundary_quoted.h"
 #include "src/variables/multipart_boundary_whitespace.h"
 #include "src/variables/multipart_crlf_lf_lines.h"
@@ -368,6 +370,8 @@ using namespace modsecurity::operators;
   VARIABLE_INBOUND_DATA_ERROR   "INBOUND_DATA_ERROR"
   VARIABLE_MATCHED_VAR          "MATCHED_VAR"
   VARIABLE_MATCHED_VAR_NAME     "MATCHED_VAR_NAME"
+  VARIABLE_MSC_PCRE_ERROR           "MSC_PCRE_ERROR"
+  VARIABLE_MSC_PCRE_LIMITS_EXCEEDED "MSC_PCRE_LIMITS_EXCEEDED"
   VARIABLE_MULTIPART_BOUNDARY_QUOTED
   VARIABLE_MULTIPART_BOUNDARY_WHITESPACE
   VARIABLE_MULTIPART_CRLF_LF_LINES    "MULTIPART_CRLF_LF_LINES"
@@ -1648,10 +1652,10 @@ expression:
         YYERROR;
 */
     | CONFIG_DIR_PCRE_MATCH_LIMIT
-/* Parser error disabled to avoid breaking default installations with modsecurity.conf-recommended
-        driver.error(@0, "SecPcreMatchLimit is not currently supported. Default PCRE values are being used for now");
-        YYERROR;
-*/
+      {
+        driver.m_pcreMatchLimit.m_set = true;
+        driver.m_pcreMatchLimit.m_value = atoi($1.c_str());
+      }
     | CONGIG_DIR_RESPONSE_BODY_MP
       {
         std::istringstream buf($1);
@@ -2321,6 +2325,14 @@ var:
       {
         VARIABLE_CONTAINER($$, new variables::MatchedVarName());
       }
+    | VARIABLE_MSC_PCRE_ERROR
+      {
+        VARIABLE_CONTAINER($$, new variables::MscPcreError());
+      }
+    | VARIABLE_MSC_PCRE_LIMITS_EXCEEDED
+      {
+        VARIABLE_CONTAINER($$, new variables::MscPcreLimitsExceeded());
+      }
     | VARIABLE_MULTIPART_BOUNDARY_QUOTED
       {
         VARIABLE_CONTAINER($$, new variables::MultipartBoundaryQuoted());

src/parser/seclang-scanner.ll

@@ -186,6 +186,8 @@ VARIABLE_GLOBAL                           (?i:GLOBAL)
 VARIABLE_INBOUND_DATA_ERROR               (?i:INBOUND_DATA_ERROR)
 VARIABLE_MATCHED_VAR                      (?i:MATCHED_VAR)
 VARIABLE_MATCHED_VAR_NAME                 (?i:MATCHED_VAR_NAME)
+VARIABLE_MSC_PCRE_ERROR                   (?i:MSC_PCRE_ERROR)
+VARIABLE_MSC_PCRE_LIMITS_EXCEEDED         (?i:MSC_PCRE_LIMITS_EXCEEDED)
 VARIABLE_MULTIPART_BOUNDARY_QUOTED        (?i:MULTIPART_BOUNDARY_QUOTED)
 VARIABLE_MULTIPART_BOUNDARY_WHITESPACE    (?i:MULTIPART_BOUNDARY_WHITESPACE)
 VARIABLE_MULTIPART_CRLF_LF_LINES          (?i:MULTIPART_CRLF_LF_LINES)
@@ -910,6 +912,8 @@ EQUALS_MINUS                            (?i:=\-)
 {VARIABLE_INBOUND_DATA_ERROR}               { return p::make_VARIABLE_INBOUND_DATA_ERROR(*driver.loc.back()); }
 {VARIABLE_MATCHED_VAR_NAME}                 { return p::make_VARIABLE_MATCHED_VAR_NAME(*driver.loc.back()); }
 {VARIABLE_MATCHED_VAR}                      { return p::make_VARIABLE_MATCHED_VAR(*driver.loc.back()); }
+{VARIABLE_MSC_PCRE_ERROR}                   { return p::make_VARIABLE_MSC_PCRE_ERROR(*driver.loc.back()); }
+{VARIABLE_MSC_PCRE_LIMITS_EXCEEDED}         { return p::make_VARIABLE_MSC_PCRE_LIMITS_EXCEEDED(*driver.loc.back()); }
 {VARIABLE_MULTIPART_BOUNDARY_QUOTED}        { return p::make_VARIABLE_MULTIPART_BOUNDARY_QUOTED(*driver.loc.back()); }
 {VARIABLE_MULTIPART_BOUNDARY_WHITESPACE}    { return p::make_VARIABLE_MULTIPART_BOUNDARY_WHITESPACE(*driver.loc.back()); }
 {VARIABLE_MULTIPART_CRLF_LF_LINES}          { return p::make_VARIABLE_MULTIPART_CRLF_LF_LINES(*driver.loc.back()); }

src/parser/stack.hh

@@ -1,4 +1,4 @@
-// A Bison parser, made by GNU Bison 3.7.6.
+// A Bison parser, made by GNU Bison 3.8.2.
 
 // Starting with Bison 3.2, this file is useless: the structure it
 // used to define is now defined with the parser itself.

src/transaction.cc

@@ -167,6 +167,8 @@ Transaction::Transaction(ModSecurity *ms, RulesSet *rules, void *logCbData)
         + std::to_string(modsecurity::utils::generate_transaction_unique_id())));
 
     m_variableUrlEncodedError.set("0", 0);
+		m_variableMscPcreError.set("0", 0);
+		m_variableMscPcreLimitsExceeded.set("0", 0);
 
     ms_dbg(4, "Initializing transaction");
 
@@ -238,6 +240,8 @@ Transaction::Transaction(ModSecurity *ms, RulesSet *rules, char *id, void *logCb
     TransactionAnchoredVariables(this) {
 
     m_variableUrlEncodedError.set("0", 0);
+		m_variableMscPcreError.set("0", 0);
+		m_variableMscPcreLimitsExceeded.set("0", 0);
 
     ms_dbg(4, "Initializing transaction");
 

src/utils/regex.cc

@@ -25,12 +25,38 @@
 
 #ifndef WITH_PCRE2
 #if PCRE_HAVE_JIT
-#define pcre_study_opt PCRE_STUDY_JIT_COMPILE
+// NOTE: Add PCRE_STUDY_EXTRA_NEEDED so studying always yields a pcre_extra strucure
+// and we can selectively override match limits using a copy of that structure at runtime.
+#define pcre_study_opt PCRE_STUDY_JIT_COMPILE | PCRE_STUDY_EXTRA_NEEDED
 #else
-#define pcre_study_opt 0
+// NOTE: Add PCRE_STUDY_EXTRA_NEEDED so studying always yields a pcre_extra strucure
+// and we can selectively override match limits using a copy of that structure at runtime.
+#define pcre_study_opt PCRE_STUDY_EXTRA_NEEDED
 #endif
 #endif
 
+#ifdef WITH_PCRE2
+class Pcre2MatchContextPtr {
+ public:
+    Pcre2MatchContextPtr()
+        : m_match_context(pcre2_match_context_create(NULL)) {}
+
+		Pcre2MatchContextPtr(const Pcre2MatchContextPtr&) = delete;
+		Pcre2MatchContextPtr& operator=(const Pcre2MatchContextPtr&) = delete;
+
+    ~Pcre2MatchContextPtr() {
+        pcre2_match_context_free(m_match_context);
+    }
+
+    operator pcre2_match_context*() const {
+        return m_match_context;
+    }
+
+ private:
+    pcre2_match_context *m_match_context;
+};
+#endif
+
 namespace modsecurity {
 namespace Utils {
 
@@ -163,24 +189,39 @@ std::list<SMatch> Regex::searchAll(const std::string& s) const {
     return retList;
 }
 
-bool Regex::searchOneMatch(const std::string& s, std::vector<SMatchCapture>& captures) const {
+RegexResult Regex::searchOneMatch(const std::string& s, std::vector<SMatchCapture>& captures, unsigned long match_limit) const {
 #ifdef WITH_PCRE2
+    Pcre2MatchContextPtr match_context;
+    if (match_limit > 0) {
+        // TODO: What if setting the match limit fails?
+        pcre2_set_match_limit(match_context, match_limit);
+    }
+
     PCRE2_SPTR pcre2_s = reinterpret_cast<PCRE2_SPTR>(s.c_str());
     pcre2_match_data *match_data = pcre2_match_data_create_from_pattern(m_pc, NULL);
     int rc = 0;
     if (m_pcje == 0) {
-        rc = pcre2_jit_match(m_pc, pcre2_s, s.length(), 0, 0, match_data, NULL);
+        rc = pcre2_jit_match(m_pc, pcre2_s, s.length(), 0, 0, match_data, match_context);
     } 
     
     if (m_pcje != 0 || rc == PCRE2_ERROR_JIT_STACKLIMIT) {
-        rc = pcre2_match(m_pc, pcre2_s, s.length(), 0, PCRE2_NO_JIT, match_data, NULL);
+        rc = pcre2_match(m_pc, pcre2_s, s.length(), 0, PCRE2_NO_JIT, match_data, match_context);
     }
     PCRE2_SIZE *ovector = pcre2_get_ovector_pointer(match_data);
 #else
     const char *subject = s.c_str();
     int ovector[OVECCOUNT];
+    pcre_extra local_pce;
+    pcre_extra *pce = m_pce;
 
-    int rc = pcre_exec(m_pc, m_pce, subject, s.size(), 0, 0, ovector, OVECCOUNT);
+    if (m_pce != NULL && match_limit > 0) {
+        local_pce = *m_pce;
+        local_pce.match_limit = match_limit;
+        local_pce.flags |= PCRE_EXTRA_MATCH_LIMIT;
+        pce = &local_pce;
+    }
+
+    int rc = pcre_exec(m_pc, pce, subject, s.size(), 0, 0, ovector, OVECCOUNT);
 #endif
 
     for (int i = 0; i < rc; i++) {
@@ -197,12 +238,18 @@ bool Regex::searchOneMatch(const std::string& s, std::vector<SMatchCapture>& cap
 #ifdef WITH_PCRE2
     pcre2_match_data_free(match_data);
 #endif
-    return (rc > 0);
+    return to_regex_result(rc);
 }
 
-bool Regex::searchGlobal(const std::string& s, std::vector<SMatchCapture>& captures) const {
+RegexResult Regex::searchGlobal(const std::string& s, std::vector<SMatchCapture>& captures, unsigned long match_limit) const {
     bool prev_match_zero_length = false;
 #ifdef WITH_PCRE2
+    Pcre2MatchContextPtr match_context;
+    if (match_limit > 0) {
+        // TODO: What if setting the match limit fails?
+        pcre2_set_match_limit(match_context, match_limit);
+    }
+
     PCRE2_SPTR pcre2_s = reinterpret_cast<PCRE2_SPTR>(s.c_str());
     PCRE2_SIZE startOffset = 0;
 
@@ -213,11 +260,21 @@ bool Regex::searchGlobal(const std::string& s, std::vector<SMatchCapture>& captu
             pcre2_options = PCRE2_NOTEMPTY_ATSTART | PCRE2_ANCHORED;
         }
         int rc = pcre2_match(m_pc, pcre2_s, s.length(),
-                            startOffset, pcre2_options, match_data, NULL);
+                            startOffset, pcre2_options, match_data, match_context);
         PCRE2_SIZE *ovector = pcre2_get_ovector_pointer(match_data);
 
 #else
     const char *subject = s.c_str();
+    pcre_extra local_pce;
+    pcre_extra *pce = m_pce;
+
+    if (m_pce != NULL && match_limit > 0) {
+        local_pce = *m_pce;
+        local_pce.match_limit = match_limit;
+        local_pce.flags |= PCRE_EXTRA_MATCH_LIMIT;
+        pce = &local_pce;
+    }
+
     int startOffset = 0;
 
     while (startOffset <= s.length()) {
@@ -226,7 +283,12 @@ bool Regex::searchGlobal(const std::string& s, std::vector<SMatchCapture>& captu
         if (prev_match_zero_length) {
             pcre_options = PCRE_NOTEMPTY_ATSTART | PCRE_ANCHORED;
         }
-        int rc = pcre_exec(m_pc, m_pce, subject, s.length(), startOffset, pcre_options, ovector, OVECCOUNT);
+        int rc = pcre_exec(m_pc, pce, subject, s.length(), startOffset, pcre_options, ovector, OVECCOUNT);
+
+        RegexResult regex_result = to_regex_result(rc);
+        if (regex_result != RegexResult::Ok) {
+            return regex_result;
+        }
 
 #endif
         if (rc > 0) {
@@ -278,7 +340,7 @@ bool Regex::searchGlobal(const std::string& s, std::vector<SMatchCapture>& captu
 #ifdef WITH_PCRE2
     pcre2_match_data_free(match_data);
 #endif
-    return (captures.size() > 0);
+    return RegexResult::Ok;
 }
 
 int Regex::search(const std::string& s, SMatch *match) const {
@@ -340,5 +402,30 @@ int Regex::search(const std::string& s) const {
 #endif
 }
 
+RegexResult Regex::to_regex_result(int pcre_exec_result) const {
+    if (
+        pcre_exec_result > 0 ||
+#ifdef WITH_PCRE2
+        pcre_exec_result == PCRE2_ERROR_NOMATCH
+#else
+        pcre_exec_result == PCRE_ERROR_NOMATCH
+#endif
+    ) {
+        return RegexResult::Ok;
+    } else if(
+#ifdef WITH_PCRE2
+        pcre_exec_result == PCRE2_ERROR_MATCHLIMIT
+#else
+        pcre_exec_result == PCRE_ERROR_MATCHLIMIT
+#endif
+    ) {
+        return RegexResult::ErrorMatchLimit;
+    } else {
+        // Note that this can include the case where the PCRE result was zero.
+        // Zero is returned if the offset vector is not large enough and can be considered an error.
+        return RegexResult::ErrorOther;
+    }
+}
+
 }  // namespace Utils
 }  // namespace modsecurity

src/utils/regex.h

@@ -34,6 +34,12 @@ namespace Utils {
 
 #define OVECCOUNT 900
 
+enum class RegexResult {
+    Ok,
+    ErrorMatchLimit,
+    ErrorOther,
+};
+
 class SMatch {
  public:
     SMatch() :
@@ -76,13 +82,15 @@ class Regex {
         return (m_pc == NULL);
     }
     std::list<SMatch> searchAll(const std::string& s) const;
-    bool searchOneMatch(const std::string& s, std::vector<SMatchCapture>& captures) const;
-    bool searchGlobal(const std::string& s, std::vector<SMatchCapture>& captures) const;
+    RegexResult searchOneMatch(const std::string& s, std::vector<SMatchCapture>& captures, unsigned long match_limit = 0) const;
+    RegexResult searchGlobal(const std::string& s, std::vector<SMatchCapture>& captures, unsigned long match_limit = 0) const;
     int search(const std::string &s, SMatch *match) const;
     int search(const std::string &s) const;
 
     const std::string pattern;
  private:
+    RegexResult to_regex_result(int pcre_exec_result) const;
+
 #if WITH_PCRE2
     pcre2_code *m_pc;
     int m_pcje;

src/variables/msc_pcre_error.h

@@ -0,0 +1,39 @@
+/*
+ * ModSecurity, http://www.modsecurity.org/
+ * Copyright (c) 2015 - 2022 Trustwave Holdings, Inc. (http://www.trustwave.com/)
+ *
+ * You may not use this file except in compliance with
+ * the License.  You may obtain a copy of the License at
+ *
+ *     http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * If any of the files related to licensing are missing or if you have any
+ * other questions related to licensing please contact Trustwave Holdings, Inc.
+ * directly using the email address security@modsecurity.org.
+ *
+ */
+
+#include <iostream>
+#include <string>
+#include <vector>
+#include <list>
+#include <utility>
+
+#ifndef SRC_VARIABLES_MSC_PCRE_ERROR_H_
+#define SRC_VARIABLES_MSC_PCRE_ERROR_H_
+
+#include "src/variables/variable.h"
+
+namespace modsecurity {
+
+class Transaction;
+namespace variables {
+
+
+DEFINE_VARIABLE(MscPcreError, MSC_PCRE_ERROR, m_variableMscPcreError)
+
+
+}  // namespace variables
+}  // namespace modsecurity
+
+#endif  // SRC_VARIABLES_MSC_PCRE_ERROR_H_

src/variables/msc_pcre_limits_exceeded.h

@@ -0,0 +1,39 @@
+/*
+ * ModSecurity, http://www.modsecurity.org/
+ * Copyright (c) 2015 - 2022 Trustwave Holdings, Inc. (http://www.trustwave.com/)
+ *
+ * You may not use this file except in compliance with
+ * the License.  You may obtain a copy of the License at
+ *
+ *     http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * If any of the files related to licensing are missing or if you have any
+ * other questions related to licensing please contact Trustwave Holdings, Inc.
+ * directly using the email address security@modsecurity.org.
+ *
+ */
+
+#include <iostream>
+#include <string>
+#include <vector>
+#include <list>
+#include <utility>
+
+#ifndef SRC_VARIABLES_MSC_PCRE_LIMITS_EXCEEDED_H_
+#define SRC_VARIABLES_MSC_PCRE_LIMITS_EXCEEDED_H_
+
+#include "src/variables/variable.h"
+
+namespace modsecurity {
+
+class Transaction;
+namespace variables {
+
+
+DEFINE_VARIABLE(MscPcreLimitsExceeded, MSC_PCRE_LIMITS_EXCEEDED, m_variableMscPcreLimitsExceeded)
+
+
+}  // namespace variables
+}  // namespace modsecurity
+
+#endif  // SRC_VARIABLES_MSC_PCRE_LIMITS_EXCEEDED_H_

src/variables/variable.h

@@ -202,6 +202,10 @@ class VariableMonkeyResolution {
                 t->m_variableMatchedVar.evaluate(l);
             } else if (comp(variable, "MATCHED_VAR_NAME")) {
                 t->m_variableMatchedVarName.evaluate(l);
+            } else if (comp(variable, "MSC_PCRE_ERROR")) {
+                t->m_variableMscPcreError.evaluate(l);
+            } else if (comp(variable, "MSC_PCRE_LIMITS_EXCEEDED")) {
+                t->m_variableMscPcreLimitsExceeded.evaluate(l);
             } else if (comp(variable, "MULTIPART_CRLF_LF_LINES")) {
                 t->m_variableMultipartCrlfLFLines.evaluate(l);
             } else if (comp(variable, "MULTIPART_DATA_AFTER")) {
@@ -365,6 +369,10 @@ class VariableMonkeyResolution {
                 vv = t->m_variableMatchedVar.resolveFirst();
             } else if (comp(variable, "MATCHED_VAR_NAME")) {
                 vv = t->m_variableMatchedVarName.resolveFirst();
+            } else if (comp(variable, "MSC_PCRE_ERROR")) {
+                vv = t->m_variableMscPcreError.resolveFirst();
+            } else if (comp(variable, "MSC_PCRE_LIMITS_EXCEEDED")) {
+                vv = t->m_variableMscPcreLimitsExceeded.resolveFirst();
             } else if (comp(variable, "MULTIPART_CRLF_LF_LINES")) {
                 vv = t->m_variableMultipartCrlfLFLines.resolveFirst();
             } else if (comp(variable, "MULTIPART_DATA_AFTER")) {