Log a warning when a regex condition hits a PCRE error - #44
Open
lbajsarowicz wants to merge 1 commit into
Open
Conversation
preg_match() returns false (not 0) when PCRE fails to evaluate a pattern: a bad pattern, a backtrack or JIT stack limit hit on a large body, or invalid UTF-8 under /u. The regex branch cast that value to bool, so a PCRE failure was silently treated as "no match" while every other failure path in Rule::matches() already logs a warning. Log the PCRE error on the false branch only; the fast path is unchanged.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
In
Model/Rule.php::scalarValueMatchesCondition(), theregexbranch is:preg_match()can returnfalseon a PCRE error (invalid pattern,pcre.backtrack_limit/ JIT stack exhaustion on a large request body, invalid UTF-8 with/u). Castingfalsetoboolmakes it indistinguishable from a genuine non-match: the rule fails open silently and nothing is logged, so a rule that stops matching due to a PCRE error is invisible invar/log/sansec_shield.log.Change
logPcreFailure()helper. It is called only whenpreg_match()returnsfalse, and logs through the existing injected logger withpreg_last_error_msg()(PHP >= 8.0) falling back to the integerpreg_last_error()code on PHP 7.2/7.3, plus the condition's target and pattern.preg_match()call, one extra=== falsecomparison, zero extra work when the pattern evaluates normally. No new object allocation, no try/catch, no extra function calls unless PCRE actually failed.falseon a PCRE error, only now it also logs.Testing
Added
testRuleRegexLogsAndFailsOpenOnPcreError()inTest/Model/RuleTest.php.It triggers a genuine runtime PCRE failure rather than a pattern compile error:
ini_set('pcre.backtrack_limit', '1')plus a catastrophic-backtracking pattern ((a+)+$) against a non-matching body forcespreg_match()to returnfalseviaPREG_BACKTRACK_LIMIT_ERROR(or the JIT-stack equivalent). This path does not emit a PHPE_WARNING, unlike an invalid pattern (unbalanced parenthesis etc.), which would raise "preg_match(): Compilation failed" and fail PHPUnit's--fail-on-warningrun. The test assertsRule::matches()still returnsfalse(fail-open) and that the logger mock receives exactly onewarning()call, then restores the originalpcre.backtrack_limitin afinallyblock.Local test run: PHPUnit with
--fail-on-warningandxmllintonetc/*.xmlpassed (58 tests, 64 assertions).