From e2e204165d63c1f998568788d8e95ec49a629dc8 Mon Sep 17 00:00:00 2001 From: Glomberg Date: Mon, 26 Oct 2026 14:53:17 +0300 Subject: [PATCH] Fix. SFW. AC/AF pass cookie value fixed. --- cleantalk.php | 7 +- lib/Cleantalk/ApbctWP/Firewall/SFW.php | 76 ++++++++++++++++---- tests/ApbctWP/Firewall/TestSFW.php | 99 +++++++++++++++++++------- 3 files changed, 142 insertions(+), 40 deletions(-) diff --git a/cleantalk.php b/cleantalk.php index e7b61a6e1..392872abc 100644 --- a/cleantalk.php +++ b/cleantalk.php @@ -913,7 +913,12 @@ function apbct_sfw__check() ); Cookie::set( 'ct_sfw_pass_key', - md5(Server::get('REMOTE_ADDR') . $apbct->api_key . $apbct->data['salt']), + SFW::makePassKey( + TT::toString(Server::get('REMOTE_ADDR')), + TT::toString($apbct->api_key), + TT::toString($apbct->data['salt']), + '0' + ), time() + 1200, '/', '' diff --git a/lib/Cleantalk/ApbctWP/Firewall/SFW.php b/lib/Cleantalk/ApbctWP/Firewall/SFW.php index 638c20675..d4596df0a 100644 --- a/lib/Cleantalk/ApbctWP/Firewall/SFW.php +++ b/lib/Cleantalk/ApbctWP/Firewall/SFW.php @@ -124,7 +124,6 @@ public function check() global $apbct; $results = array(); - $status = 0; if (empty($apbct->key_is_ok) || empty($apbct->api_key)) { return $results; @@ -143,12 +142,16 @@ public function check() } // Skip by cookie + $pass_key = Cookie::getString('ct_sfw_pass_key'); foreach ($this->ip_array as $current_ip) { - if ( - TT::toString(Cookie::get('ct_sfw_pass_key')) - && strpos(TT::toString(Cookie::get('ct_sfw_pass_key')), md5($current_ip . $this->api_key . $apbct->data['salt'])) === 0 - ) { - if (Cookie::get('ct_sfw_passed')) { + $status = self::getPassKeyStatus( + $pass_key, + TT::toString($current_ip), + TT::toString($this->api_key), + TT::toString($apbct->data['salt']) + ); + if ($status !== null) { + if (Cookie::getString('ct_sfw_passed')) { if ( ! headers_sent()) { Cookie::set( 'ct_sfw_passed', @@ -176,11 +179,7 @@ public function check() } } - if (strlen(TT::toString(Cookie::get('ct_sfw_pass_key'))) > 32) { - $status = substr(TT::toString(Cookie::get('ct_sfw_pass_key')), -1); - } - - if ($status) { + if ($status === '1') { $results[] = array( 'ip' => $current_ip, 'is_personal' => false, @@ -277,6 +276,47 @@ public function check() return $results; } + /** + * Builds the ct_sfw_pass_key cookie value. The status is signed together with the IP. + * + * @param string $ip + * @param string $api_key + * @param string $salt + * @param string $status '1' - whitelisted, '0' - passed + * + * @return string + */ + public static function makePassKey($ip, $api_key, $salt, $status) + { + $status = $status === '1' ? '1' : '0'; + + return md5($ip . $api_key . $salt . $status) . $status; + } + + /** + * Validates the ct_sfw_pass_key cookie value. + * + * @param string $pass_key + * @param string $ip + * @param string $api_key + * @param string $salt + * + * @return string|null '1' or '0' for a valid key, null otherwise + */ + public static function getPassKeyStatus($pass_key, $ip, $api_key, $salt) + { + if (strlen($pass_key) !== 33) { + return null; + } + + $status = substr($pass_key, -1); + if ($status !== '0' && $status !== '1') { + return null; + } + + return hash_equals(self::makePassKey($ip, $api_key, $salt, $status), $pass_key) ? $status : null; + } + /** * Add entry to SFW log. * Writes to database. @@ -360,7 +400,12 @@ public function actionsForPassed($result) global $apbct; if ($this->data__cookies_type === 'native' && ! headers_sent()) { $status = $result['status'] === 'PASS_SFW__BY_WHITELIST' ? '1' : '0'; - $cookie_val = md5($result['ip'] . $this->api_key . $apbct->data['salt']) . $status; + $cookie_val = self::makePassKey( + TT::toString($result['ip']), + TT::toString($this->api_key), + TT::toString($apbct->data['salt']), + $status + ); Cookie::setNativeCookie( 'ct_sfw_pass_key', $cookie_val, @@ -399,7 +444,12 @@ public function diePage($result) $net_count = $apbct->stats['sfw']['entries']; $status = $result['status'] === 'PASS_SFW__BY_WHITELIST' ? '1' : '0'; - $cookie_val = md5($result['ip'] . $this->api_key . $apbct->data['salt']) . $status; + $cookie_val = self::makePassKey( + TT::toString($result['ip']), + TT::toString($this->api_key), + TT::toString($apbct->data['salt']), + $status + ); $block_message = sprintf( esc_html__('SpamFireWall is checking your browser and IP %s for spam bots', 'cleantalk-spam-protect'), diff --git a/tests/ApbctWP/Firewall/TestSFW.php b/tests/ApbctWP/Firewall/TestSFW.php index b04d63281..0db500d7a 100644 --- a/tests/ApbctWP/Firewall/TestSFW.php +++ b/tests/ApbctWP/Firewall/TestSFW.php @@ -119,8 +119,8 @@ public function testCookieWithSaltIsAccepted() $api_key = 'test_api_key'; $salt = 'test_salt_value'; - // New-style cookie: md5(ip + api_key + salt) - $new_cookie = md5($ip . $api_key . $salt) . '0'; + // New-style cookie: md5(ip + api_key + salt + status) + status + $new_cookie = SFW::makePassKey($ip, $api_key, $salt, '0'); $_COOKIE['ct_sfw_pass_key'] = $new_cookie; $_COOKIE['ct_sfw_passed'] = '1'; @@ -164,8 +164,8 @@ public function testCookieWithSaltAndWhitelistStatus() $api_key = 'test_api_key'; $salt = 'test_salt_value'; - // New-style cookie with whitelist status '1' - $new_cookie = md5($ip . $api_key . $salt) . '1'; + // Whitelist cookie issued by the plugin, status '1' is signed + $new_cookie = SFW::makePassKey($ip, $api_key, $salt, '1'); $_COOKIE['ct_sfw_pass_key'] = $new_cookie; $sfw = new SFW( @@ -200,7 +200,7 @@ public function testCookieWithWrongSaltIsRejected() $api_key = 'test_api_key'; // Cookie made with wrong salt - $wrong_salt_cookie = md5($ip . $api_key . 'wrong_salt') . '0'; + $wrong_salt_cookie = SFW::makePassKey($ip, $api_key, 'wrong_salt', '0'); $_COOKIE['ct_sfw_pass_key'] = $wrong_salt_cookie; $sfw = new SFW( @@ -228,15 +228,72 @@ public function testCookieWithWrongSaltIsRejected() /** * @test - * actionsForPassed() sets cookie value using salt. + * makePassKey() signs the status: keys for '0' and '1' differ in the hash part, not only the suffix. */ - public function testActionsForPassedUsesSaltInCookie() + public function testMakePassKeySignsStatus() { - global $apbct; + $ip = '203.0.113.50'; + $api_key = 'test_api_key'; + $salt = 'test_salt_value'; + + $passed = SFW::makePassKey($ip, $api_key, $salt, '0'); + $whitelisted = SFW::makePassKey($ip, $api_key, $salt, '1'); + $this->assertSame(md5($ip . $api_key . $salt . '0') . '0', $passed); + $this->assertSame(md5($ip . $api_key . $salt . '1') . '1', $whitelisted); + $this->assertSame(33, strlen($passed)); + $this->assertNotSame(substr($passed, 0, 32), substr($whitelisted, 0, 32)); + $this->assertSame($passed, SFW::makePassKey($ip, $api_key, $salt, 'x'), 'Unknown status falls back to 0'); + } + + /** + * @test + * getPassKeyStatus() accepts only exact, correctly signed 33-char keys with status 0 or 1. + * + * @dataProvider passKeyProvider + */ + public function testGetPassKeyStatus($pass_key, $expected) + { + $this->assertSame( + $expected, + SFW::getPassKeyStatus($pass_key, '203.0.113.50', 'test_api_key', 'test_salt_value') + ); + } + + public function passKeyProvider() + { $ip = '203.0.113.50'; $api_key = 'test_api_key'; $salt = 'test_salt_value'; + $passed = SFW::makePassKey($ip, $api_key, $salt, '0'); + $whitelisted = SFW::makePassKey($ip, $api_key, $salt, '1'); + + return array( + 'valid passed' => array($passed, '0'), + 'valid whitelisted' => array($whitelisted, '1'), + 'status flipped 0 to 1' => array(substr($passed, 0, 32) . '1', null), + 'status flipped 1 to 0' => array(substr($whitelisted, 0, 32) . '0', null), + 'old unsigned status format' => array(md5($ip . $api_key . $salt) . '1', null), + 'status other than 0 or 1' => array(substr($passed, 0, 32) . 'a', null), + 'hash only, no status' => array(substr($passed, 0, 32), null), + 'extra trailing chars' => array($passed . '1', null), + 'other IP' => array(SFW::makePassKey('198.51.100.1', $api_key, $salt, '1'), null), + 'empty' => array('', null), + ); + } + + /** + * @test + * A passed (status 0) cookie with its last char flipped to '1' must not give PASS_SFW__BY_WHITELIST. + */ + public function testForgedWhitelistStatusIsRejected() + { + $ip = '10.0.0.1'; + $api_key = 'test_api_key'; + $salt = 'test_salt_value'; + + $forged = substr(SFW::makePassKey($ip, $api_key, $salt, '0'), 0, 32) . '1'; + $_COOKIE['ct_sfw_pass_key'] = $forged; $sfw = new SFW( 'test_log_table', @@ -247,26 +304,16 @@ public function testActionsForPassedUsesSaltInCookie() 'sfw_common_table_name' => 'test_data_table', ) ); + $sfw->setIpArray(array('real' => $ip)); + $sfw->setDb($this->createDbStub(array())); - $result = array( - 'ip' => $ip, - 'status' => 'PASS_SFW', - ); - - // actionsForPassed sets a cookie - we can't easily verify the cookie value - // in CLI, but we can verify the hash computation is correct - $expected_hash = md5($ip . $api_key . $salt) . '0'; + $results = $sfw->check(); - // Verify that the hash formula includes salt - $this->assertNotEquals( - md5($ip . $api_key) . '0', - $expected_hash, - 'Hash with salt must differ from hash without salt' - ); - $this->assertEquals( - md5($ip . $api_key . $salt) . '0', - $expected_hash - ); + $this->assertNotEmpty($results, 'Forged cookie must fall through to the regular DB check'); + foreach ($results as $result) { + $this->assertNotEquals('PASS_SFW__BY_WHITELIST', $result['status']); + $this->assertNotEquals('PASS_SFW__BY_COOKIE', $result['status']); + } } // =========================================================================