From 49f9ded2a3085350a0ec877721a27e2a0ce4e063 Mon Sep 17 00:00:00 2001 From: Ervin Hegedus Date: Mon, 26 Oct 2026 21:21:12 +0100 Subject: [PATCH 1/2] fix: handle regex errors of @validateHash like @rx Follow-up to #3709, as promised in its review: - @validateHash returned 0 on a compile error of the macro-expanded pattern and on exceeded PCRE limits, which a negated rule turned into a match. A compile error was also not logged and not flagged, so a non-negated rule skipped the hash check silently. Both paths now return what re.c turns into "no match", and a compile error is logged and sets TX:MSC_PCRE_ERROR. Other execution errors set TX:MSC_PCRE_ERROR too, as in @rx. - With legacy PCRE and JIT, pcre_exec() returns PCRE_ERROR_JITSTACKLIMIT without retrying without JIT, as the PCRE2 path does. Handle it like the match limit in @rx and @validateHash. - Replace the TX flag blocks in re_operators.c with one helper that takes the flag name. --- apache2/re_operators.c | 86 +++++++++++++------------ tests/regression/misc/10-pcre-negated.t | 65 ++++++++++++++++++- 2 files changed, 106 insertions(+), 45 deletions(-) diff --git a/apache2/re_operators.c b/apache2/re_operators.c index c5c02649b..0386e0072 100644 --- a/apache2/re_operators.c +++ b/apache2/re_operators.c @@ -773,6 +773,22 @@ static int msre_op_validateHash_param_init(msre_rule *rule, char **error_msg) { return 1; /* OK */ } +/* Sets a TX:MSC_* flag to 1, where TX:/^MSC_/ rules see it. */ +static int msre_op_set_tx_flag(modsec_rec *msr, const char *name) { + msc_string *s = (msc_string *)apr_pcalloc(msr->mp, sizeof(msc_string)); + + if (s == NULL) return -1; + s->name = apr_pstrdup(msr->mp, name); + if (s->name == NULL) return -1; + s->name_len = strlen(s->name); + s->value = apr_pstrdup(msr->mp, "1"); + if (s->value == NULL) return -1; + s->value_len = 1; + apr_table_setn(msr->tx_vars, s->name, (void *)s); + + return 1; +} + /** * \brief Execute function to validateHash * @@ -845,7 +861,12 @@ static int msre_op_validateHash_execute(modsec_rec *msr, msre_rule *rule, msre_v if (regex == NULL) { *error_msg = apr_psprintf(msr->mp, "Error compiling pattern (offset %d): %s", erroffset, errptr); - return 0; + msr_log(msr, 3, "%s.", *error_msg); + if (msre_op_set_tx_flag(msr, "MSC_PCRE_ERROR") < 0) return -1; + + /* A regex error is neither a match nor a mismatch: return + * what the negation in re.c turns into "no match". */ + return rule->op_negated; } #ifdef WITH_PCRE_STUDY @@ -892,18 +913,15 @@ static int msre_op_validateHash_execute(modsec_rec *msr, msre_rule *rule, msre_v #ifndef WITH_PCRE if ((rc == PCRE2_ERROR_MATCHLIMIT) || (rc == PCRE2_ERROR_RECURSIONLIMIT)) { #else - if ((rc == PCRE_ERROR_MATCHLIMIT) || (rc == PCRE_ERROR_RECURSIONLIMIT)) { + if ((rc == PCRE_ERROR_MATCHLIMIT) || (rc == PCRE_ERROR_RECURSIONLIMIT) +#ifdef PCRE_ERROR_JITSTACKLIMIT + /* Unlike msc_regexec_ex() with PCRE2, pcre_exec() does not retry + * without JIT when the JIT stack is full. */ + || (rc == PCRE_ERROR_JITSTACKLIMIT) #endif - msc_string *s = (msc_string *)apr_pcalloc(msr->mp, sizeof(msc_string)); - - if (s == NULL) return -1; - s->name = apr_pstrdup(msr->mp, "MSC_PCRE_LIMITS_EXCEEDED"); - if (s->name == NULL) return -1; - s->name_len = strlen(s->name); - s->value = apr_pstrdup(msr->mp, "1"); - if (s->value == NULL) return -1; - s->value_len = 1; - apr_table_setn(msr->tx_vars, s->name, (void *)s); + ) { +#endif + if (msre_op_set_tx_flag(msr, "MSC_PCRE_LIMITS_EXCEEDED") < 0) return -1; *error_msg = apr_psprintf(msr->mp, "Rule %pp [id \"%s\"][file \"%s\"][line \"%d\"] - " @@ -915,11 +933,14 @@ static int msre_op_validateHash_execute(modsec_rec *msr, msre_rule *rule, msre_v msr_log(msr, 3, "%s.", *error_msg); - return 0; /* No match. */ + /* A regex error is neither a match nor a mismatch: return + * what the negation in re.c turns into "no match". */ + return rule->op_negated; } else if (rc < -1) { *error_msg = apr_psprintf(msr->mp, "Regex execution failed (%d): %s", rc, my_error_msg); + msre_op_set_tx_flag(msr, "MSC_PCRE_ERROR"); return -1; } @@ -1050,22 +1071,6 @@ static int msre_op_rx_param_init(msre_rule *rule, char **error_msg) { return 1; /* OK */ } -/* Flags a regex error in TX:MSC_PCRE_ERROR, where TX:/^MSC_/ rules see it. */ -static int msre_op_rx_set_error_flag(modsec_rec *msr) { - msc_string *s = (msc_string *)apr_pcalloc(msr->mp, sizeof(msc_string)); - - if (s == NULL) return -1; - s->name = apr_pstrdup(msr->mp, "MSC_PCRE_ERROR"); - if (s->name == NULL) return -1; - s->name_len = strlen(s->name); - s->value = apr_pstrdup(msr->mp, "1"); - if (s->value == NULL) return -1; - s->value_len = 1; - apr_table_setn(msr->tx_vars, s->name, (void *)s); - - return 1; -} - static int msre_op_rx_execute(modsec_rec *msr, msre_rule *rule, msre_var *var, char **error_msg) { assert(msr != NULL); assert(rule != NULL); @@ -1131,7 +1136,7 @@ static int msre_op_rx_execute(modsec_rec *msr, msre_rule *rule, msre_var *var, c *error_msg = apr_psprintf(msr->mp, "Error compiling pattern (offset %d): %s", erroffset, errptr); msr_log(msr, 3, "%s.", *error_msg); - if (msre_op_rx_set_error_flag(msr) < 0) return -1; + if (msre_op_set_tx_flag(msr, "MSC_PCRE_ERROR") < 0) return -1; /* A regex error is neither a match nor a mismatch: return * what the negation in re.c turns into "no match". */ @@ -1212,18 +1217,15 @@ static int msre_op_rx_execute(modsec_rec *msr, msre_rule *rule, msre_var *var, c #ifndef WITH_PCRE if ((rc == PCRE2_ERROR_MATCHLIMIT) || (rc == PCRE2_ERROR_RECURSIONLIMIT)) { #else - if ((rc == PCRE_ERROR_MATCHLIMIT) || (rc == PCRE_ERROR_RECURSIONLIMIT)) { + if ((rc == PCRE_ERROR_MATCHLIMIT) || (rc == PCRE_ERROR_RECURSIONLIMIT) +#ifdef PCRE_ERROR_JITSTACKLIMIT + /* Unlike msc_regexec_ex() with PCRE2, pcre_exec() does not retry + * without JIT when the JIT stack is full. */ + || (rc == PCRE_ERROR_JITSTACKLIMIT) #endif - msc_string *s = (msc_string *)apr_pcalloc(msr->mp, sizeof(msc_string)); - - if (s == NULL) return -1; - s->name = apr_pstrdup(msr->mp, "MSC_PCRE_LIMITS_EXCEEDED"); - if (s->name == NULL) return -1; - s->name_len = strlen(s->name); - s->value = apr_pstrdup(msr->mp, "1"); - if (s->value == NULL) return -1; - s->value_len = 1; - apr_table_setn(msr->tx_vars, s->name, (void *)s); + ) { +#endif + if (msre_op_set_tx_flag(msr, "MSC_PCRE_LIMITS_EXCEEDED") < 0) return -1; *error_msg = apr_psprintf(msr->mp, "Rule %pp [id \"%s\"][file \"%s\"][line \"%d\"] - " @@ -1242,7 +1244,7 @@ static int msre_op_rx_execute(modsec_rec *msr, msre_rule *rule, msre_var *var, c else if (rc < -1) { *error_msg = apr_psprintf(msr->mp, "Regex execution failed (%d): %s", rc, my_error_msg); - msre_op_rx_set_error_flag(msr); + msre_op_set_tx_flag(msr, "MSC_PCRE_ERROR"); return -1; } diff --git a/tests/regression/misc/10-pcre-negated.t b/tests/regression/misc/10-pcre-negated.t index d1da67827..0b78cf8b3 100644 --- a/tests/regression/misc/10-pcre-negated.t +++ b/tests/regression/misc/10-pcre-negated.t @@ -1,7 +1,7 @@ -### Regex errors with negated @rx +### Regex errors with negated @rx and @validateHash -# Neither @rx nor !@rx must match when the regex fails. A failure must -# be visible through the TX:MSC_* flags instead. +# Neither the operator nor its negation must match when the regex fails. +# A failure must be visible through the TX:MSC_* flags instead. { type => "misc", @@ -147,3 +147,62 @@ GET => "http://$ENV{SERVER_NAME}:$ENV{SERVER_PORT}/index.html?x=a&patt=x%28", ), }, +{ + type => "misc", + comment => "Invalid expanded pattern - negated validateHash does not match, MSC_PCRE_ERROR is set", + conf => qq( + SecRuleEngine On + SecDebugLog $ENV{DEBUG_LOG} + SecDebugLogLevel 9 + SecHashEngine On + + SecRule ARGS:x "!\@validateHash %{ARGS.patt}" "phase:2,deny,id:500068" + SecRule TX:MSC_PCRE_ERROR "\@streq 1" "phase:2,pass,log,msg:'PCRE error flagged',id:500069" + ), + match_log => { + debug => [ qr/Error compiling pattern/, 1 ], + error => [ qr/PCRE error flagged/, 1 ], + -error => [ qr/Access denied/, 1 ], + }, + match_response => { + status => qr/^200$/, + }, + request => new HTTP::Request( + GET => "http://$ENV{SERVER_NAME}:$ENV{SERVER_PORT}/index.html?x=a&patt=%2A", + ), +}, +{ + type => "misc", + comment => "PCRE limits exceeded - negated validateHash does not match", + conf => qq( + SecRuleEngine On + SecDebugLog $ENV{DEBUG_LOG} + SecDebugLogLevel 9 + SecHashEngine On + + SecRequestBodyAccess On + SecRequestBodyLimit 1000000 + SecRequestBodyInMemoryLimit 1000000 + + SecPcreMatchLimit 100 + SecPcreMatchLimitRecursion 100 + + SecRule ARGS "!\@validateHash (?:(.{2,})\\1{32,})" "phase:2,deny,id:500070" + SecRule TX:MSC_PCRE_LIMITS_EXCEEDED "\@streq 1" "phase:2,pass,log,msg:'PCRE limits flagged',id:500071" + ), + match_log => { + debug => [ qr/PCRE limits exceeded/, 1 ], + error => [ qr/PCRE limits flagged/, 1 ], + -error => [ qr/Access denied/, 1 ], + }, + match_response => { + status => qr/^200$/, + }, + request => new HTTP::Request( + POST => "http://$ENV{SERVER_NAME}:$ENV{SERVER_PORT}/index.html", + [ + "Content-Type" => "application/x-www-form-urlencoded", + ], + "test=" . (("a" x 64) . "b") x 64 . "d", + ), +}, From eeeb294c4cfc5959ad302dd43c4b6ce81089b376 Mon Sep 17 00:00:00 2001 From: Ervin Hegedus Date: Tue, 27 Oct 2026 19:12:10 +0100 Subject: [PATCH 2/2] fix: set TX:MSC_PCRE_ERROR on exceeded PCRE limits too TX:MSC_PCRE_ERROR now means a regex failed for any reason, as the standalone variable of the same name in v3 does, and TX:MSC_PCRE_LIMITS_EXCEEDED names one of the causes. On exceeded limits, @rx and @validateHash set both flags, MSC_PCRE_LIMITS_EXCEEDED first. tx_vars is iterated in insertion order, so a TX:/^MSC_/ rule names the more specific flag. Move the limit handling, which was identical in both operators, into two helpers: msre_op_pcre_limits_exceeded() for the condition and msre_op_pcre_limits_error() for the flags and the log message. --- apache2/re_operators.c | 84 ++++++++++++------------- tests/regression/misc/10-pcre-negated.t | 67 ++++++++++++++++++++ 2 files changed, 107 insertions(+), 44 deletions(-) diff --git a/apache2/re_operators.c b/apache2/re_operators.c index 0386e0072..d0486118f 100644 --- a/apache2/re_operators.c +++ b/apache2/re_operators.c @@ -789,6 +789,42 @@ static int msre_op_set_tx_flag(modsec_rec *msr, const char *name) { return 1; } +/* Tells whether a regex execution failed on a PCRE limit. */ +static int msre_op_pcre_limits_exceeded(int rc) { +#ifndef WITH_PCRE + return (rc == PCRE2_ERROR_MATCHLIMIT) || (rc == PCRE2_ERROR_RECURSIONLIMIT); +#else + return (rc == PCRE_ERROR_MATCHLIMIT) || (rc == PCRE_ERROR_RECURSIONLIMIT) +#ifdef PCRE_ERROR_JITSTACKLIMIT + /* Unlike msc_regexec_ex() with PCRE2, pcre_exec() does not retry + * without JIT when the JIT stack is full. */ + || (rc == PCRE_ERROR_JITSTACKLIMIT) +#endif + ; +#endif +} + +/* Flags and logs a regex execution that failed on a PCRE limit. A limit + * error is a regex error too, so both TX:MSC_PCRE_LIMITS_EXCEEDED and + * TX:MSC_PCRE_ERROR are set, the more specific one first. */ +static int msre_op_pcre_limits_error(modsec_rec *msr, msre_rule *rule, int rc, + const char *my_error_msg, char **error_msg) { + if (msre_op_set_tx_flag(msr, "MSC_PCRE_LIMITS_EXCEEDED") < 0) return -1; + if (msre_op_set_tx_flag(msr, "MSC_PCRE_ERROR") < 0) return -1; + + *error_msg = apr_psprintf(msr->mp, + "Rule %pp [id \"%s\"][file \"%s\"][line \"%d\"] - " + "Execution error - " + "PCRE limits exceeded (%d): %s", + rule,((rule->actionset != NULL)&&(rule->actionset->id != NULL)) ? rule->actionset->id : "-", + rule->filename != NULL ? rule->filename : "-", + rule->line_num,rc, my_error_msg); + + msr_log(msr, 3, "%s.", *error_msg); + + return 1; +} + /** * \brief Execute function to validateHash * @@ -910,28 +946,8 @@ static int msre_op_validateHash_execute(modsec_rec *msr, msre_rule *rule, msre_v * and no memory has to be allocated for any backreferences. */ rc = msc_regexec_capture(regex, target, target_length, ovector, 30, &my_error_msg); -#ifndef WITH_PCRE - if ((rc == PCRE2_ERROR_MATCHLIMIT) || (rc == PCRE2_ERROR_RECURSIONLIMIT)) { -#else - if ((rc == PCRE_ERROR_MATCHLIMIT) || (rc == PCRE_ERROR_RECURSIONLIMIT) -#ifdef PCRE_ERROR_JITSTACKLIMIT - /* Unlike msc_regexec_ex() with PCRE2, pcre_exec() does not retry - * without JIT when the JIT stack is full. */ - || (rc == PCRE_ERROR_JITSTACKLIMIT) -#endif - ) { -#endif - if (msre_op_set_tx_flag(msr, "MSC_PCRE_LIMITS_EXCEEDED") < 0) return -1; - - *error_msg = apr_psprintf(msr->mp, - "Rule %pp [id \"%s\"][file \"%s\"][line \"%d\"] - " - "Execution error - " - "PCRE limits exceeded (%d): %s", - rule,((rule->actionset != NULL)&&(rule->actionset->id != NULL)) ? rule->actionset->id : "-", - rule->filename != NULL ? rule->filename : "-", - rule->line_num,rc, my_error_msg); - - msr_log(msr, 3, "%s.", *error_msg); + if (msre_op_pcre_limits_exceeded(rc)) { + if (msre_op_pcre_limits_error(msr, rule, rc, my_error_msg, error_msg) < 0) return -1; /* A regex error is neither a match nor a mismatch: return * what the negation in re.c turns into "no match". */ @@ -1214,28 +1230,8 @@ static int msre_op_rx_execute(modsec_rec *msr, msre_rule *rule, msre_var *var, c * and no memory has to be allocated for any backreferences. */ rc = msc_regexec_capture(regex, target, target_length, ovector, 30, &my_error_msg); -#ifndef WITH_PCRE - if ((rc == PCRE2_ERROR_MATCHLIMIT) || (rc == PCRE2_ERROR_RECURSIONLIMIT)) { -#else - if ((rc == PCRE_ERROR_MATCHLIMIT) || (rc == PCRE_ERROR_RECURSIONLIMIT) -#ifdef PCRE_ERROR_JITSTACKLIMIT - /* Unlike msc_regexec_ex() with PCRE2, pcre_exec() does not retry - * without JIT when the JIT stack is full. */ - || (rc == PCRE_ERROR_JITSTACKLIMIT) -#endif - ) { -#endif - if (msre_op_set_tx_flag(msr, "MSC_PCRE_LIMITS_EXCEEDED") < 0) return -1; - - *error_msg = apr_psprintf(msr->mp, - "Rule %pp [id \"%s\"][file \"%s\"][line \"%d\"] - " - "Execution error - " - "PCRE limits exceeded (%d): %s", - rule,((rule->actionset != NULL)&&(rule->actionset->id != NULL)) ? rule->actionset->id : "-", - rule->filename != NULL ? rule->filename : "-", - rule->line_num,rc, my_error_msg); - - msr_log(msr, 3, "%s.", *error_msg); + if (msre_op_pcre_limits_exceeded(rc)) { + if (msre_op_pcre_limits_error(msr, rule, rc, my_error_msg, error_msg) < 0) return -1; /* A regex error is neither a match nor a mismatch: return * what the negation in re.c turns into "no match". */ diff --git a/tests/regression/misc/10-pcre-negated.t b/tests/regression/misc/10-pcre-negated.t index 0b78cf8b3..1c6bed395 100644 --- a/tests/regression/misc/10-pcre-negated.t +++ b/tests/regression/misc/10-pcre-negated.t @@ -206,3 +206,70 @@ "test=" . (("a" x 64) . "b") x 64 . "d", ), }, +{ + type => "misc", + comment => "PCRE limits exceeded - MSC_PCRE_ERROR is set too", + conf => qq( + SecRuleEngine On + SecDebugLog $ENV{DEBUG_LOG} + SecDebugLogLevel 9 + + SecRequestBodyAccess On + SecRequestBodyLimit 1000000 + SecRequestBodyInMemoryLimit 1000000 + + SecPcreMatchLimit 100 + SecPcreMatchLimitRecursion 100 + + SecRule ARGS "!\@rx (?:(.{2,})\\1{32,})" "phase:2,deny,id:500072" + SecRule TX:MSC_PCRE_ERROR "\@streq 1" "phase:2,pass,log,msg:'PCRE error flagged',id:500073" + ), + match_log => { + debug => [ qr/PCRE limits exceeded/, 1 ], + error => [ qr/PCRE error flagged/, 1 ], + -error => [ qr/Access denied/, 1 ], + }, + match_response => { + status => qr/^200$/, + }, + request => new HTTP::Request( + POST => "http://$ENV{SERVER_NAME}:$ENV{SERVER_PORT}/index.html", + [ + "Content-Type" => "application/x-www-form-urlencoded", + ], + "test=" . (("a" x 64) . "b") x 64 . "d", + ), +}, +{ + type => "misc", + comment => "PCRE limits exceeded - TX:/^MSC_/ rule names MSC_PCRE_LIMITS_EXCEEDED", + conf => qq( + SecRuleEngine On + SecDebugLog $ENV{DEBUG_LOG} + SecDebugLogLevel 9 + + SecRequestBodyAccess On + SecRequestBodyLimit 1000000 + SecRequestBodyInMemoryLimit 1000000 + + SecPcreMatchLimit 100 + SecPcreMatchLimitRecursion 100 + + SecRule ARGS "!\@rx (?:(.{2,})\\1{32,})" "phase:2,deny,id:500074" + SecRule TX:/^MSC_/ "!\@streq 0" "phase:2,t:none,log,deny,msg:'ModSecurity internal error flagged: %{MATCHED_VAR_NAME}',id:500075" + ), + match_log => { + error => [ qr/internal error flagged: TX:MSC_PCRE_LIMITS_EXCEEDED/, 1 ], + -error => [ qr/against "ARGS:test" required/, 1 ], + }, + match_response => { + status => qr/^403$/, + }, + request => new HTTP::Request( + POST => "http://$ENV{SERVER_NAME}:$ENV{SERVER_PORT}/index.html", + [ + "Content-Type" => "application/x-www-form-urlencoded", + ], + "test=" . (("a" x 64) . "b") x 64 . "d", + ), +},