diff --git a/lib/CleantalkSP/SpbctWP/Firewall/WAF.php b/lib/CleantalkSP/SpbctWP/Firewall/WAF.php index 1f48a859e..db38b6780 100644 --- a/lib/CleantalkSP/SpbctWP/Firewall/WAF.php +++ b/lib/CleantalkSP/SpbctWP/Firewall/WAF.php @@ -409,6 +409,11 @@ private static function hasSignature($data, $signature) $result_signature_body = false; $result_additionally_params = false; + // Skip catch-all regex without headers/URL (e.g. #.*# with empty waf_headers) — mass FP + if ( self::isUnsafeCatchAllSignature($signature_body, $what_to_check_additionally) ) { + return false; + } + $is_regexp = Helper::isRegexp($signature_body); if ( $is_regexp && preg_match($signature_body, $data) ) { @@ -456,6 +461,33 @@ private static function hasSignature($data, $signature) return false; } + /** + * Detect catch-all regex WAF rules that have no headers/URL constraints. + * Such rules (e.g. body "#.*#" with empty waf_headers) match every request + * and cause mass false positives. + * + * @param string $signature_body + * @param string $what_to_check_additionally + * + * @return bool + */ + private static function isUnsafeCatchAllSignature($signature_body, $what_to_check_additionally) + { + if ( $what_to_check_additionally !== 'nothing' ) { + return false; + } + + if ( ! is_string($signature_body) || ! Helper::isRegexp($signature_body) ) { + return false; + } + + // Matches empty string and an arbitrary probe → pattern accepts any input + $probe = 'spbct_waf_probe_catchall_check'; + + return @preg_match($signature_body, '') === 1 + && @preg_match($signature_body, $probe) === 1; + } + /** * Compare signature headers and request headers * diff --git a/tests/lib/CleantalkSP/SpbctWP/Firewall/WAFHasSignatureTest.php b/tests/lib/CleantalkSP/SpbctWP/Firewall/WAFHasSignatureTest.php new file mode 100644 index 000000000..bb60a360a --- /dev/null +++ b/tests/lib/CleantalkSP/SpbctWP/Firewall/WAFHasSignatureTest.php @@ -0,0 +1,183 @@ +hasSignature = new ReflectionMethod(WAF::class, 'hasSignature'); + $this->hasSignature->setAccessible(true); + + $this->isUnsafeCatchAllSignature = new ReflectionMethod(WAF::class, 'isUnsafeCatchAllSignature'); + $this->isUnsafeCatchAllSignature->setAccessible(true); + + // Reset cached HTTP headers between tests + HTTP::getInstance()->http_headers = array(); + } + + protected function tearDown(): void + { + HTTP::getInstance()->http_headers = array(); + unset($_SERVER['HTTP_USER_AGENT']); + parent::tearDown(); + } + + /** + * @param string $data + * @param array $signature + * + * @return bool + */ + private function callHasSignature($data, array $signature) + { + return $this->hasSignature->invoke(null, $data, $signature); + } + + /** + * Incident #4471: catch-all body without headers must not match. + */ + public function testCatchAllWithoutHeadersDoesNotMatch() + { + $signature = array( + 'body' => '#.*#', + 'waf_headers' => null, + 'waf_url' => null, + ); + + $this->assertFalse($this->callHasSignature('', $signature)); + $this->assertFalse($this->callHasSignature('anything', $signature)); + $this->assertFalse($this->callHasSignature('?page=1', $signature)); + } + + /** + * Empty string headers are treated the same as NULL. + */ + public function testCatchAllWithEmptyHeadersDoesNotMatch() + { + $signature = array( + 'body' => '#.*#', + 'waf_headers' => '', + 'waf_url' => '', + ); + + $this->assertFalse($this->callHasSignature('query', $signature)); + } + + /** + * Catch-all with modifiers must also be rejected without constraints. + */ + public function testCatchAllWithModifiersWithoutHeadersDoesNotMatch() + { + $signature = array( + 'body' => '#.*#i', + 'waf_headers' => null, + 'waf_url' => null, + ); + + $this->assertFalse($this->callHasSignature('query', $signature)); + } + + /** + * Catch-all is valid when constrained by matching request headers. + */ + public function testCatchAllWithMatchingHeadersDoesMatch() + { + $_SERVER['HTTP_USER_AGENT'] = 'WPSCAN'; + HTTP::getInstance()->http_headers = array(); + + $signature = array( + 'body' => '#.*#', + 'waf_headers' => json_encode(array('User-Agent' => 'WPSCAN')), + 'waf_url' => null, + ); + + $this->assertTrue($this->callHasSignature('', $signature)); + $this->assertTrue($this->callHasSignature('any-query', $signature)); + } + + /** + * Catch-all with headers that do not match the request must not fire. + */ + public function testCatchAllWithNonMatchingHeadersDoesNotMatch() + { + $_SERVER['HTTP_USER_AGENT'] = 'Mozilla/5.0'; + HTTP::getInstance()->http_headers = array(); + + $signature = array( + 'body' => '#.*#', + 'waf_headers' => json_encode(array('User-Agent' => 'WPSCAN')), + 'waf_url' => null, + ); + + $this->assertFalse($this->callHasSignature('any-query', $signature)); + } + + /** + * Narrow regexp without headers must still work (not a catch-all). + */ + public function testNarrowRegexpWithoutHeadersStillMatches() + { + $signature = array( + 'body' => '#wp-config\.php#', + 'waf_headers' => null, + 'waf_url' => null, + ); + + $this->assertTrue($this->callHasSignature('download=wp-config.php', $signature)); + $this->assertFalse($this->callHasSignature('page=home', $signature)); + $this->assertFalse($this->callHasSignature('', $signature)); + } + + /** + * Literal (non-regexp) body without headers must still work. + */ + public function testLiteralBodyWithoutHeadersStillMatches() + { + $signature = array( + 'body' => 'eval(', + 'waf_headers' => null, + 'waf_url' => null, + ); + + $this->assertTrue($this->callHasSignature('x=eval(base64_decode(1))', $signature)); + $this->assertFalse($this->callHasSignature('x=safe', $signature)); + } + + /** + * Direct unit coverage for the catch-all detector. + */ + public function testIsUnsafeCatchAllSignatureDetector() + { + $this->assertTrue( + $this->isUnsafeCatchAllSignature->invoke(null, '#.*#', 'nothing') + ); + $this->assertTrue( + $this->isUnsafeCatchAllSignature->invoke(null, '#^.*$#', 'nothing') + ); + $this->assertFalse( + $this->isUnsafeCatchAllSignature->invoke(null, '#.*#', 'only_headers') + ); + $this->assertFalse( + $this->isUnsafeCatchAllSignature->invoke(null, '#wp-config#', 'nothing') + ); + $this->assertFalse( + $this->isUnsafeCatchAllSignature->invoke(null, 'eval(', 'nothing') + ); + } +}