From 4096c56768259c08687768ea8e64147845d2fe72 Mon Sep 17 00:00:00 2001 From: Ervin Hegedus Date: Mon, 26 Oct 2026 20:56:29 +0100 Subject: [PATCH 1/2] fix: flag regex errors of @rx/@rxGlobal in TX:MSC_PCRE_ERROR Since #3700, a negated @rx/@rxGlobal no longer matches on a regex error. Exceeded match limits are still caught by the TX:/^MSC_/ rule (200005) of the recommended configuration through TX:MSC_PCRE_LIMITS_EXCEEDED. Compile errors and other regex errors only set the standalone MSC_PCRE_ERROR variable, though, which TX:/^MSC_/ does not see, so such a request passed both the negated rule and rule 200005. Set TX:MSC_PCRE_ERROR as well, the same way as MSC_PCRE_LIMITS_EXCEEDED. Rule 200005 now blocks on any regex error without a configuration change, as it does in ModSecurity v2 (#3709). The standalone variable is kept. --- modsecurity.conf-recommended | 1 + src/operators/rx.cc | 4 ++ src/operators/rx_global.cc | 4 ++ test/test-cases/regression/operator-rx.json | 48 +++++++++++++++++++ .../regression/operator-rxGlobal.json | 48 +++++++++++++++++++ 5 files changed, 105 insertions(+) diff --git a/modsecurity.conf-recommended b/modsecurity.conf-recommended index bdc93ba67..70acc582b 100644 --- a/modsecurity.conf-recommended +++ b/modsecurity.conf-recommended @@ -155,6 +155,7 @@ SecPcreMatchLimitRecursion 1000 # All of these are prefixed with "MSC_". The following flags currently exist: # # MSC_PCRE_LIMITS_EXCEEDED: PCRE match limits were exceeded. +# MSC_PCRE_ERROR: a regular expression failed to compile or execute. # SecRule TX:/^MSC_/ "!@streq 0" \ "id:'200005',phase:2,t:none,log,deny,msg:'ModSecurity internal error flagged: %{MATCHED_VAR_NAME}'" diff --git a/src/operators/rx.cc b/src/operators/rx.cc index 24bde647a..da4224c20 100644 --- a/src/operators/rx.cc +++ b/src/operators/rx.cc @@ -68,6 +68,8 @@ bool Rx::evaluate(Transaction *transaction, RuleWithActions *rule, if (transaction) { ms_dbg_a(transaction, 1, "Error with regular expression: \"" + re->pattern + "\""); transaction->m_variableMscPcreError.set("1", transaction->m_variableOffset); + transaction->m_collections.m_tx_collection->storeOrUpdateFirst("MSC_PCRE_ERROR", "1"); + ms_dbg_a(transaction, 7, "Set TX.MSC_PCRE_ERROR to 1"); } /* A regex failure is neither a match nor a mismatch. Return the * value that the negation in Operator::evaluateInternal turns @@ -90,6 +92,8 @@ bool Rx::evaluate(Transaction *transaction, RuleWithActions *rule, if (regex_result != Utils::RegexResult::Ok) { if (transaction) { transaction->m_variableMscPcreError.set("1", transaction->m_variableOffset); + transaction->m_collections.m_tx_collection->storeOrUpdateFirst("MSC_PCRE_ERROR", "1"); + ms_dbg_a(transaction, 7, "Set TX.MSC_PCRE_ERROR to 1"); std::string regex_error_str = "OTHER"; if (regex_result == Utils::RegexResult::ErrorMatchLimit) { diff --git a/src/operators/rx_global.cc b/src/operators/rx_global.cc index d0792bce8..d2a23d5a7 100644 --- a/src/operators/rx_global.cc +++ b/src/operators/rx_global.cc @@ -68,6 +68,8 @@ bool RxGlobal::evaluate(Transaction *transaction, RuleWithActions *rule, if (transaction) { ms_dbg_a(transaction, 1, "Error with regular expression: \"" + re->pattern + "\""); transaction->m_variableMscPcreError.set("1", transaction->m_variableOffset); + transaction->m_collections.m_tx_collection->storeOrUpdateFirst("MSC_PCRE_ERROR", "1"); + ms_dbg_a(transaction, 7, "Set TX.MSC_PCRE_ERROR to 1"); } /* A regex failure is neither a match nor a mismatch. Return the * value that the negation in Operator::evaluateInternal turns @@ -89,6 +91,8 @@ bool RxGlobal::evaluate(Transaction *transaction, RuleWithActions *rule, if (regex_result != Utils::RegexResult::Ok) { if (transaction) { transaction->m_variableMscPcreError.set("1", transaction->m_variableOffset); + transaction->m_collections.m_tx_collection->storeOrUpdateFirst("MSC_PCRE_ERROR", "1"); + ms_dbg_a(transaction, 7, "Set TX.MSC_PCRE_ERROR to 1"); std::string regex_error_str = "OTHER"; if (regex_result == Utils::RegexResult::ErrorMatchLimit) { diff --git a/test/test-cases/regression/operator-rx.json b/test/test-cases/regression/operator-rx.json index 9ae8408da..755413b5b 100644 --- a/test/test-cases/regression/operator-rx.json +++ b/test/test-cases/regression/operator-rx.json @@ -565,5 +565,53 @@ "SecRule ARGS:subject \"!@rx %{ARGS:patt}\" \"id:4001,phase:2,deny,status:403\"", "SecRule MSC_PCRE_ERROR \"@streq 1\" \"id:4002,phase:2,pass,log\"" ] + }, + { + "enabled": 1, + "version_min": 300000, + "title": "Testing Operator :: !@rx with invalid macro-expanded pattern is flagged in TX for TX:/^MSC_/", + "client": { + "ip": "200.249.12.31", + "port": 123 + }, + "server": { + "ip": "200.249.12.31", + "port": 80 + }, + "request": { + "headers": { + "Host": "localhost", + "User-Agent": "curl/7.38.0", + "Accept": "*/*", + "Content-Length": "27", + "Content-Type": "application/x-www-form-urlencoded" + }, + "uri": "/?patt=%28", + "method": "POST", + "body": [ + "subject=wwww" + ] + }, + "response": { + "headers": { + "Date": "Mon, 13 Jul 2015 20:02:41 GMT", + "Last-Modified": "Sun, 26 Oct 2014 22:33:37 GMT", + "Content-Type": "text/html", + "Content-Length": "8" + }, + "body": [ + "no need." + ] + }, + "expected": { + "debug_log": "Set TX.MSC_PCRE_ERROR to 1", + "error_log": "ModSecurity internal error flagged: TX:MSC_PCRE_ERROR", + "http_code": 409 + }, + "rules": [ + "SecRuleEngine On", + "SecRule ARGS:subject \"!@rx %{ARGS:patt}\" \"id:4001,phase:2,deny,status:403\"", + "SecRule TX:/^MSC_/ \"!@streq 0\" \"id:4002,phase:2,t:none,log,deny,status:409,msg:'ModSecurity internal error flagged: %{MATCHED_VAR_NAME}'\"" + ] } ] diff --git a/test/test-cases/regression/operator-rxGlobal.json b/test/test-cases/regression/operator-rxGlobal.json index eb35c1f13..2baedae65 100644 --- a/test/test-cases/regression/operator-rxGlobal.json +++ b/test/test-cases/regression/operator-rxGlobal.json @@ -371,5 +371,53 @@ "SecRule ARGS:subject \"!@rxGlobal %{ARGS:patt}\" \"id:4001,phase:2,deny,status:403\"", "SecRule MSC_PCRE_ERROR \"@streq 1\" \"id:4002,phase:2,pass,log\"" ] + }, + { + "enabled": 1, + "version_min": 300000, + "title": "Testing Operator :: !@rxGlobal with invalid macro-expanded pattern is flagged in TX for TX:/^MSC_/", + "client": { + "ip": "200.249.12.31", + "port": 123 + }, + "server": { + "ip": "200.249.12.31", + "port": 80 + }, + "request": { + "headers": { + "Host": "localhost", + "User-Agent": "curl/7.38.0", + "Accept": "*/*", + "Content-Length": "27", + "Content-Type": "application/x-www-form-urlencoded" + }, + "uri": "/?patt=%28", + "method": "POST", + "body": [ + "subject=wwww" + ] + }, + "response": { + "headers": { + "Date": "Mon, 13 Jul 2015 20:02:41 GMT", + "Last-Modified": "Sun, 26 Oct 2014 22:33:37 GMT", + "Content-Type": "text/html", + "Content-Length": "8" + }, + "body": [ + "no need." + ] + }, + "expected": { + "debug_log": "Set TX.MSC_PCRE_ERROR to 1", + "error_log": "ModSecurity internal error flagged: TX:MSC_PCRE_ERROR", + "http_code": 409 + }, + "rules": [ + "SecRuleEngine On", + "SecRule ARGS:subject \"!@rxGlobal %{ARGS:patt}\" \"id:4001,phase:2,deny,status:403\"", + "SecRule TX:/^MSC_/ \"!@streq 0\" \"id:4002,phase:2,t:none,log,deny,status:409,msg:'ModSecurity internal error flagged: %{MATCHED_VAR_NAME}'\"" + ] } ] From 286f87b717663e39e1d8f1e671b971d2175d57be Mon Sep 17 00:00:00 2001 From: Ervin Hegedus Date: Tue, 27 Oct 2026 14:41:08 +0100 Subject: [PATCH 2/2] refactor: share the regex error reporting of @rx and @rxGlobal Move the duplicated error handling of both operators into two static helpers in Operator: reportRegexCompileError() for an invalid pattern, and reportRegexSearchError() for a failed search. They set the standalone MSC_PCRE_* variables and the TX:MSC_PCRE_* flags, and keep the debug log messages unchanged. This resolves the "DRY regex error reporting" FIXME. The helpers are kept out of Utils::Regex, which doesn't know about transactions and is also used outside of operators, and out of Transaction, which is part of the public API. No behavior change. The log message is now built without a temporary variable, so the cppcheck suppressions for rx.cc and rx_global.cc, which only covered that variable in NO_LOGS builds, are no longer needed. --- src/operators/operator.cc | 38 ++++++++++++++++++++++++++++++++++ src/operators/operator.h | 14 +++++++++++++ src/operators/rx.cc | 24 ++------------------- src/operators/rx_global.cc | 24 ++------------------- test/cppcheck_suppressions.txt | 6 ------ 5 files changed, 56 insertions(+), 50 deletions(-) diff --git a/src/operators/operator.cc b/src/operators/operator.cc index 935add621..d9d33be7b 100644 --- a/src/operators/operator.cc +++ b/src/operators/operator.cc @@ -21,6 +21,7 @@ #include "modsecurity/transaction.h" #include "src/run_time_string.h" +#include "src/utils/regex.h" #include "src/utils/string.h" #include "src/operators/begins_with.h" #include "src/operators/contains.h" @@ -70,6 +71,43 @@ namespace modsecurity { namespace operators { +void Operator::reportRegexCompileError(Transaction *transaction, + const Utils::Regex &re) { + if (transaction == nullptr) { + return; + } + + ms_dbg_a(transaction, 1, "Error with regular expression: \"" + re.pattern + "\""); + transaction->m_variableMscPcreError.set("1", transaction->m_variableOffset); + transaction->m_collections.m_tx_collection->storeOrUpdateFirst("MSC_PCRE_ERROR", "1"); + ms_dbg_a(transaction, 7, "Set TX.MSC_PCRE_ERROR to 1"); +} + + +void Operator::reportRegexSearchError(Transaction *transaction, + const Utils::Regex &re, Utils::RegexResult result, + const std::string &opName) { + if (transaction == nullptr) { + return; + } + + transaction->m_variableMscPcreError.set("1", transaction->m_variableOffset); + transaction->m_collections.m_tx_collection->storeOrUpdateFirst("MSC_PCRE_ERROR", "1"); + ms_dbg_a(transaction, 7, "Set TX.MSC_PCRE_ERROR to 1"); + + const bool match_limit = (result == Utils::RegexResult::ErrorMatchLimit); + if (match_limit) { + transaction->m_variableMscPcreLimitsExceeded.set("1", transaction->m_variableOffset); + transaction->m_collections.m_tx_collection->storeOrUpdateFirst("MSC_PCRE_LIMITS_EXCEEDED", "1"); + ms_dbg_a(transaction, 7, "Set TX.MSC_PCRE_LIMITS_EXCEEDED to 1"); + } + + ms_dbg_a(transaction, 1, opName + ": regex error '" + + (match_limit ? "MATCH_LIMIT" : "OTHER") + + "' for pattern '" + re.pattern + "'"); +} + + bool Operator::evaluateInternal(Transaction *transaction, RuleWithActions *rule, const std::string& a, RuleMessage &ruleMessage) { bool res = evaluate(transaction, rule, a, ruleMessage); diff --git a/src/operators/operator.h b/src/operators/operator.h index 636ad5e00..52099a510 100644 --- a/src/operators/operator.h +++ b/src/operators/operator.h @@ -26,6 +26,11 @@ #include "src/run_time_string.h" namespace modsecurity { +namespace Utils { +class Regex; +enum class RegexResult; +} // namespace Utils + namespace operators { class Operator { @@ -140,6 +145,15 @@ class Operator { std::string m_param; std::unique_ptr m_string; bool m_couldContainsMacro; + + protected: + /* Regex error reporting shared by @rx and @rxGlobal: sets the + * standalone MSC_PCRE_* variables and the TX:MSC_PCRE_* flags. */ + static void reportRegexCompileError(Transaction *transaction, + const Utils::Regex &re); + static void reportRegexSearchError(Transaction *transaction, + const Utils::Regex &re, Utils::RegexResult result, + const std::string &opName); }; } // namespace operators diff --git a/src/operators/rx.cc b/src/operators/rx.cc index da4224c20..9f93e1209 100644 --- a/src/operators/rx.cc +++ b/src/operators/rx.cc @@ -65,12 +65,7 @@ bool Rx::evaluate(Transaction *transaction, RuleWithActions *rule, } if (re->hasError()) { - if (transaction) { - ms_dbg_a(transaction, 1, "Error with regular expression: \"" + re->pattern + "\""); - transaction->m_variableMscPcreError.set("1", transaction->m_variableOffset); - transaction->m_collections.m_tx_collection->storeOrUpdateFirst("MSC_PCRE_ERROR", "1"); - ms_dbg_a(transaction, 7, "Set TX.MSC_PCRE_ERROR to 1"); - } + reportRegexCompileError(transaction, *re); /* A regex failure is neither a match nor a mismatch. Return the * value that the negation in Operator::evaluateInternal turns * into "no match", so that the negated form does not fire on @@ -88,23 +83,8 @@ bool Rx::evaluate(Transaction *transaction, RuleWithActions *rule, 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) { - if (transaction) { - transaction->m_variableMscPcreError.set("1", transaction->m_variableOffset); - transaction->m_collections.m_tx_collection->storeOrUpdateFirst("MSC_PCRE_ERROR", "1"); - ms_dbg_a(transaction, 7, "Set TX.MSC_PCRE_ERROR to 1"); - - 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); - transaction->m_collections.m_tx_collection->storeOrUpdateFirst("MSC_PCRE_LIMITS_EXCEEDED", "1"); - ms_dbg_a(transaction, 7, "Set TX.MSC_PCRE_LIMITS_EXCEEDED to 1"); - } - - ms_dbg_a(transaction, 1, "rx: regex error '" + regex_error_str + "' for pattern '" + re->pattern + "'"); - } + reportRegexSearchError(transaction, *re, regex_result, "rx"); /* A regex failure is neither a match nor a mismatch. Return the * value that the negation in Operator::evaluateInternal turns diff --git a/src/operators/rx_global.cc b/src/operators/rx_global.cc index d2a23d5a7..4459f887c 100644 --- a/src/operators/rx_global.cc +++ b/src/operators/rx_global.cc @@ -65,12 +65,7 @@ bool RxGlobal::evaluate(Transaction *transaction, RuleWithActions *rule, } if (re->hasError()) { - if (transaction) { - ms_dbg_a(transaction, 1, "Error with regular expression: \"" + re->pattern + "\""); - transaction->m_variableMscPcreError.set("1", transaction->m_variableOffset); - transaction->m_collections.m_tx_collection->storeOrUpdateFirst("MSC_PCRE_ERROR", "1"); - ms_dbg_a(transaction, 7, "Set TX.MSC_PCRE_ERROR to 1"); - } + reportRegexCompileError(transaction, *re); /* A regex failure is neither a match nor a mismatch. Return the * value that the negation in Operator::evaluateInternal turns * into "no match", so that the negated form does not fire on @@ -87,23 +82,8 @@ bool RxGlobal::evaluate(Transaction *transaction, RuleWithActions *rule, 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) { - if (transaction) { - transaction->m_variableMscPcreError.set("1", transaction->m_variableOffset); - transaction->m_collections.m_tx_collection->storeOrUpdateFirst("MSC_PCRE_ERROR", "1"); - ms_dbg_a(transaction, 7, "Set TX.MSC_PCRE_ERROR to 1"); - - 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); - transaction->m_collections.m_tx_collection->storeOrUpdateFirst("MSC_PCRE_LIMITS_EXCEEDED", "1"); - ms_dbg_a(transaction, 7, "Set TX.MSC_PCRE_LIMITS_EXCEEDED to 1"); - } - - ms_dbg_a(transaction, 1, "rxGlobal: regex error '" + regex_error_str + "' for pattern '" + re->pattern + "'"); - } + reportRegexSearchError(transaction, *re, regex_result, "rxGlobal"); /* A regex failure is neither a match nor a mismatch. Return the * value that the negation in Operator::evaluateInternal turns diff --git a/test/cppcheck_suppressions.txt b/test/cppcheck_suppressions.txt index 8b2f2da18..8bfa8a031 100644 --- a/test/cppcheck_suppressions.txt +++ b/test/cppcheck_suppressions.txt @@ -22,9 +22,6 @@ shiftNegative:src/utils/msc_tree.cc // // ModSecurity v3 code... // -variableScope:src/operators/rx.cc -variableScope:src/operators/rx_global.cc - noExplicitConstructor:seclang-parser.hh constParameter:seclang-parser.hh accessMoved:seclang-parser.hh @@ -34,9 +31,6 @@ constVariableReference:seclang-parser.hh uninitMemberVar:seclang-parser.hh -unreadVariable:src/operators/rx.cc -unreadVariable:src/operators/rx_global.cc - noExplicitConstructor:src/collection/backend/collection_data.h stlIfStrFind:src/collection/backend/collection_data.cc