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/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 24bde647a..9f93e1209 100644 --- a/src/operators/rx.cc +++ b/src/operators/rx.cc @@ -65,10 +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); - } + 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 @@ -86,21 +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); - - 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 d0792bce8..4459f887c 100644 --- a/src/operators/rx_global.cc +++ b/src/operators/rx_global.cc @@ -65,10 +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); - } + 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 @@ -85,21 +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); - - 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 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}'\"" + ] } ]