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

Add isolated PCRE match limits as a layer of ReDoS defense

Author: martinhsv

Diff for: headers/modsecurity/rules_set_properties.h

@@ -379,6 +379,7 @@ class RulesSetProperties {
                                     from->m_responseBodyLimitAction,
                                     PropertyNotSetBodyLimitAction);
 
+        to->m_pcreMatchLimit.merge(&from->m_pcreMatchLimit);
         to->m_uploadFileLimit.merge(&from->m_uploadFileLimit);
         to->m_uploadFileMode.merge(&from->m_uploadFileMode);
         to->m_uploadDirectory.merge(&from->m_uploadDirectory);
@@ -470,6 +471,7 @@ class RulesSetProperties {
     ConfigDouble m_requestBodyLimit;
     ConfigDouble m_requestBodyNoFilesLimit;
     ConfigDouble m_responseBodyLimit;
+    ConfigInt m_pcreMatchLimit;
     ConfigInt m_uploadFileLimit;
     ConfigInt m_uploadFileMode;
     DebugLog *m_debugLog;

Diff for: headers/modsecurity/transaction.h

@@ -134,6 +134,8 @@ class TransactionAnchoredVariables {
         m_variableInboundDataError(t, "INBOUND_DATA_ERROR"),
         m_variableMatchedVar(t, "MATCHED_VAR"),
         m_variableMatchedVarName(t, "MATCHED_VAR_NAME"),
+        m_variableMscPcreError(t, "MSC_PCRE_ERROR"),
+        m_variableMscPcreLimitsExceeded(t, "MSC_PCRE_LIMITS_EXCEEDED"),
         m_variableMultipartBoundaryQuoted(t, "MULTIPART_BOUNDARY_QUOTED"),
         m_variableMultipartBoundaryWhiteSpace(t,
             "MULTIPART_BOUNDARY_WHITESPACE"),
@@ -219,6 +221,8 @@ class TransactionAnchoredVariables {
     AnchoredVariable m_variableInboundDataError;
     AnchoredVariable m_variableMatchedVar;
     AnchoredVariable m_variableMatchedVarName;
+    AnchoredVariable m_variableMscPcreError;
+    AnchoredVariable m_variableMscPcreLimitsExceeded;
     AnchoredVariable m_variableMultipartBoundaryQuoted;
     AnchoredVariable m_variableMultipartBoundaryWhiteSpace;
     AnchoredVariable m_variableMultipartCrlfLFLines;

Diff for: 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) {

Diff for: 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) {

Diff for: 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++
 

Diff for: 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".