From cc59c28882bad9732696bf03422cbb655602c509 Mon Sep 17 00:00:00 2001 From: alexandergull Date: Fri, 23 Oct 2026 15:11:49 +0500 Subject: [PATCH 1/5] Fix. 2FA. Flow updates. --- inc/spbc-auth.php | 395 +++++++-- .../SpbctWP/G2FA/GoogleAuthenticator.php | 8 +- .../SpbcRateLimit/SpbcUserRateLimiter.php | 57 ++ security-malware-firewall.php | 3 + tests/Inc/TwoFactorAuthTest.php | 793 ++++++++++++++++++ .../SpbctWP/G2FA/TestGoogleAuthenticator.php | 38 + .../RateLimiter/TestSpbcUserRateLimiter.php | 125 +++ 7 files changed, 1365 insertions(+), 54 deletions(-) create mode 100644 lib/CleantalkSP/SpbctWP/SpbcRateLimit/SpbcUserRateLimiter.php create mode 100644 tests/Inc/TwoFactorAuthTest.php create mode 100644 tests/lib/CleantalkSP/SpbctWP/G2FA/TestGoogleAuthenticator.php create mode 100644 tests/lib/CleantalkSP/SpbctWP/RateLimiter/TestSpbcUserRateLimiter.php diff --git a/inc/spbc-auth.php b/inc/spbc-auth.php index 1c60b3a99..4cdcaace6 100644 --- a/inc/spbc-auth.php +++ b/inc/spbc-auth.php @@ -1,12 +1,14 @@ settings['2fa__enable'] ) { - if (Server::inUriCanonical('wp-login.php') && !empty($_POST)) { + // Registered by the POST field, not by URI: the login page could be renamed + if (isset($_POST['spbc_2fa'])) { add_action('init', 'spbc_2fa_rate_limit', 10, 1); } add_action('login_form_login', 'spbc_2fa__authenticate', 1); // Authenticate with Code add_action('login_form', 'spbc_2fa__show_field', 10); add_action('after_password_reset', 'spbc_2fa__2fa_app_replace_meta', 10, 1); + add_action('after_password_reset', 'spbc_2fa__user_rate_limiter_reset', 10, 1); // Profile page hook add_action('show_user_profile', 'spbc_2fa__SelfUserProfileEdit', 1); add_action('edit_user_profile', 'spbc_2fa__SelfUserProfileEdit', 1); @@ -74,7 +78,213 @@ */ function spbc_2fa__2fa_app_replace_meta(\WP_User $user) { - return delete_user_meta($user->ID, 'spbc_2fa_type') && delete_user_meta($user->ID, 'spbc_2fa_app_token'); + delete_user_meta($user->ID, 'spbc_2fa_type'); + delete_user_meta($user->ID, 'spbc_2fa_app_token'); + delete_user_meta($user->ID, 'spbc_2fa_last_step'); + + return get_user_meta($user->ID, 'spbc_2fa_type', true) === '' + && get_user_meta($user->ID, 'spbc_2fa_app_token', true) === ''; +} + +/** + * Issue a one-time token proving that the first factor (password) has been passed. + * Stored hashed in user meta (one record per user, overwritten on each login) and sent to the browser as HttpOnly cookie. + * + * @param \WP_User $user + * + * @return void + */ +function spbc_2fa__step_token_issue(\WP_User $user) +{ + $token = bin2hex(random_bytes(32)); + $expires = time() + SPBC_2FA_KEY_TTL; + + update_user_meta( + $user->ID, + 'spbc_2fa_step', + array( + 'hash' => hash('sha256', $token), + 'expires' => $expires, + 'fails' => 0, + ) + ); + + spbc_2fa__step_cookie_set($token, $expires); +} + +/** + * @param string $value + * @param int $expires + * + * @return void + */ +function spbc_2fa__step_cookie_set($value, $expires) +{ + if ( headers_sent() ) { + return; + } + + /** + * COOKIE_DOMAIN is false by default in WordPress, despite Psalm's string type + * @psalm-suppress RedundantCast + */ + $domain = defined('COOKIE_DOMAIN') ? (string) COOKIE_DOMAIN : ''; + + \CleantalkSP\Variables\Cookie::set( + 'spbc_2fa_step', + $value, + $expires, + '/', + $domain, + is_ssl(), + true, + 'Lax' + ); +} + +/** + * @return string Token from the request or empty string + */ +function spbc_2fa__step_token_get_from_request() +{ + /** + * $_COOKIE value may be an array (spbc_2fa_step[]=...), despite Psalm's string type + * @psalm-suppress RedundantCondition + */ + $token = isset($_COOKIE['spbc_2fa_step']) && is_string($_COOKIE['spbc_2fa_step']) + ? $_COOKIE['spbc_2fa_step'] + : ''; + + return preg_match('/^[a-f0-9]{64}$/', $token) ? $token : ''; +} + +/** + * Check that the current request carries a valid 2FA step token for the user. + * + * @param \WP_User $user + * + * @return bool + */ +function spbc_2fa__step_token_is_valid(\WP_User $user) +{ + $token = spbc_2fa__step_token_get_from_request(); + if ( $token === '' ) { + return false; + } + + $step = get_user_meta($user->ID, 'spbc_2fa_step', true); + if ( ! is_array($step) || empty($step['hash']) || empty($step['expires']) ) { + return false; + } + + if ( (int) $step['expires'] < time() || (int) $step['fails'] >= SPBC_2FA_MAX_ATTEMPTS ) { + delete_user_meta($user->ID, 'spbc_2fa_step'); + return false; + } + + return hash_equals((string) $step['hash'], hash('sha256', $token)); +} + +/** + * Count a failed code attempt. Destroys the step token when attempts are exhausted. + * + * @param \WP_User $user + * + * @return bool True if the step token is still usable + */ +function spbc_2fa__step_token_register_fail(\WP_User $user) +{ + global $spbc; + + $step = get_user_meta($user->ID, 'spbc_2fa_step', true); + if ( ! is_array($step) ) { + return false; + } + + $step['fails'] = (int) $step['fails'] + 1; + if ( $step['fails'] >= SPBC_2FA_MAX_ATTEMPTS ) { + spbc_2fa__step_token_destroy($user); + // E-mail code is dropped too, so the password has to be entered again to get a new one + if ( isset($spbc->data['2fa_keys'][ $user->user_login ]) ) { + unset($spbc->data['2fa_keys'][ $user->user_login ]); + $spbc->save('data'); + } + return false; + } + + update_user_meta($user->ID, 'spbc_2fa_step', $step); + + return true; +} + +/** + * @param \WP_User $user + * + * @return void + */ +function spbc_2fa__step_token_destroy(\WP_User $user) +{ + delete_user_meta($user->ID, 'spbc_2fa_step'); + spbc_2fa__step_cookie_set('', time() - 3600); +} + +/** + * Per-user limiter of 2FA code attempts. Not reset by a new password login and not bound to IP. + * + * @param \WP_User $user + * + * @return SpbcUserRateLimiter + */ +function spbc_2fa__user_rate_limiter(\WP_User $user) +{ + return new SpbcUserRateLimiter( + new RateLimiterConfig('2fa_user_attempts', SPBC_2FA_USER_MAX_ATTEMPTS, SPBC_2FA_USER_LOCK_PERIOD), + $user->ID + ); +} + +/** + * Unlock 2FA step after the password reset: the user has proved access to the account e-mail. + * + * @param \WP_User $user + * + * @return void + */ +function spbc_2fa__user_rate_limiter_reset(\WP_User $user) +{ + spbc_2fa__user_rate_limiter($user)->reset(); +} + +/** + * Check 2FA application (TOTP) code with replay protection. + * + * @param \WP_User $user + * @param string $secret + * @param string $code + * @param bool $is_reused Set to true if the code is correct but its time step was already used + * + * @return bool + */ +function spbc_2fa__app_code_check(\WP_User $user, $secret, $code, &$is_reused = false) +{ + $ga = new GoogleAuthenticator(); + $time_step = (int) floor(time() / 30); + $is_reused = false; + + if ( ! hash_equals((string) $ga->getCode($secret, $time_step), (string) $code) ) { + return false; + } + + // Reject the code if it (or a newer one) was already used + $last_step = (int) get_user_meta($user->ID, 'spbc_2fa_last_step', true); + if ( $time_step <= $last_step ) { + $is_reused = true; + return false; + } + + update_user_meta($user->ID, 'spbc_2fa_last_step', $time_step); + + return true; } /** @@ -223,6 +433,7 @@ function spbc_authenticate($user, $username) if ( $type2fa !== '2fa_application' ) { spbc_2fa__send_mail($user); } + spbc_2fa__step_token_issue($user); $woocommerce_additional_query_string = ''; if ( spbc_is_plugin_active('woocommerce/woocommerce.php') && @@ -867,7 +1078,8 @@ function spbc_2fa__show_field() if ( $spbc_2fa_user ) { $user = spbc_get_user_by('login', rawurldecode($spbc_2fa_user)); - if ( ! $user ) { + // The code form is shown only to the one who has just passed the password check + if ( ! $user || ! spbc_2fa__step_token_is_valid($user) ) { return; } @@ -887,7 +1099,30 @@ function spbc_2fa__show_field() } // Error displaying - if ( isset($_GET['spbc_2fa_error']) ) { + if ( Get::getString('spbc_2fa_error') === '2' && $type2fa === '2fa_application' ) { + // Correct code, but its time step is already used + $json_flags = JSON_HEX_TAG | JSON_HEX_AMP | JSON_HEX_APOS | JSON_HEX_QUOT; + echo TextPlateStatic::render( + ' + + ', + array( + 'title' => wp_json_encode(__('CODE ALREADY USED: ', 'security-malware-firewall'), $json_flags), + 'text' => wp_json_encode(__('Please, wait for the next code in your 2FA Code application and try again in 30 seconds.', 'security-malware-firewall'), $json_flags), + ) + ); + } elseif ( isset($_GET['spbc_2fa_error']) ) { echo '')); + $this->assertStringContainsString('textContent', $error_script); + $this->assertStringNotContainsString('innerHTML', $error_script); + } + + public function testReusedCodeMessageIsNotShownForEmailCode() + { + $_GET['spbc_2fa_user'] = $this->email_user->user_login; + $_GET['spbc_2fa_error'] = '2'; + $this->giveStepToken($this->email_user); + + list(, $output) = $this->captureOutput('spbc_2fa__show_field'); + + $this->assertStringNotContainsString('CODE ALREADY USED', $output); + } + + // ---------------------------------------------------------------- 2FA application setup / removal + + public function testReplaceMetaRemovesSecretEvenWithoutType() + { + delete_user_meta($this->app_user->ID, 'spbc_2fa_type'); + update_user_meta($this->app_user->ID, 'spbc_2fa_last_step', 123); + + $this->assertTrue(spbc_2fa__2fa_app_replace_meta($this->app_user)); + $this->assertSame('', get_user_meta($this->app_user->ID, 'spbc_2fa_app_token', true)); + $this->assertSame('', get_user_meta($this->app_user->ID, 'spbc_2fa_last_step', true)); + } + + public function testQrEndpointDoesNotExposeActivatedSecret() + { + $this->asAjaxUser($this->app_user); + + list($result, $output) = $this->captureOutput('spbc_2fa__Get2FAAppQrCode'); + + $this->assertSame('die', $result[0]); + $response = json_decode($output, true); + $this->assertFalse($response['success']); + $this->assertStringNotContainsString(self::SECRET, $output); + $this->assertSame(self::SECRET, get_user_meta($this->app_user->ID, 'spbc_2fa_app_token', true)); + } + + public function testQrEndpointIssuesFreshSecretOnEachSetup() + { + delete_user_meta($this->email_user->ID, 'spbc_2fa_type'); + $this->asAjaxUser($this->email_user); + + $secrets = array(); + for ( $i = 0; $i < 2; $i++ ) { + list(, $output) = $this->captureOutput('spbc_2fa__Get2FAAppQrCode'); + $response = json_decode($output, true); + + $this->assertTrue($response['success']); + $this->assertRegExp('/^[A-Z2-7]{32}$/', $response['data']['code']); + $this->assertSame($response['data']['code'], get_user_meta($this->email_user->ID, 'spbc_2fa_app_token', true)); + $secrets[] = $response['data']['code']; + } + + $this->assertNotSame($secrets[0], $secrets[1]); + } + + public function testQrEndpointRequiresNonce() + { + $this->asAjaxUser($this->email_user); + $_REQUEST['security'] = 'wrong'; + + list($result) = $this->captureOutput('spbc_2fa__Get2FAAppQrCode'); + + $this->assertSame(array('die', 403), $result); + $this->assertSame('', get_user_meta($this->email_user->ID, 'spbc_2fa_app_token', true)); + } + + public function testActivationCodeMarksTimeStepAsUsed() + { + $this->inOneTimeStep(function ($code) { + delete_user_meta($this->app_user->ID, 'spbc_2fa_type'); + $this->asAjaxUser($this->app_user); + $_POST['code'] = $code; + + list(, $output) = $this->captureOutput('spbc_2fa__Check2FAAppCode'); + + $this->assertTrue(json_decode($output, true)['success']); + $this->assertSame('2fa_application', get_user_meta($this->app_user->ID, 'spbc_2fa_type', true)); + $this->assertSame( + $this->timeStep(), + (int) get_user_meta($this->app_user->ID, 'spbc_2fa_last_step', true) + ); + + // The code used for activation can't be used to log in + $this->giveStepToken($this->app_user); + $this->assertCodeError($this->postCode($this->app_user->user_login, $code), 2, $this->app_user); + }); + } + + public function testEmailCodeIsSixDigits() + { + global $spbc; + + for ( $i = 0; $i < 20; $i++ ) { + spbc_2fa__key_generate_and_store($this->email_user); + $this->assertRegExp( + '/^[1-9]\d{5}$/', + (string) $spbc->data['2fa_keys'][$this->email_user->user_login]['val'] + ); + } + } +} diff --git a/tests/lib/CleantalkSP/SpbctWP/G2FA/TestGoogleAuthenticator.php b/tests/lib/CleantalkSP/SpbctWP/G2FA/TestGoogleAuthenticator.php new file mode 100644 index 000000000..0c031a28f --- /dev/null +++ b/tests/lib/CleantalkSP/SpbctWP/G2FA/TestGoogleAuthenticator.php @@ -0,0 +1,38 @@ +assertSame('287082', $ga->getCode('GEZDGNBVGY3TQOJQGEZDGNBVGY3TQOJQ', 1)); + } + + public function testCodeIsSixDigits() + { + $ga = new GoogleAuthenticator(); + + $this->assertRegExp('/^\d{6}$/', $ga->getCode($ga->generateSecret())); + } + + public function testSecretIs160BitBase32() + { + $ga = new GoogleAuthenticator(); + + $secrets = array(); + for ( $i = 0; $i < 50; $i++ ) { + $secret = $ga->generateSecret(); + $this->assertRegExp('/^[A-Z2-7]{32}$/', $secret); + $secrets[] = $secret; + } + + $this->assertCount(50, array_unique($secrets)); + } +} diff --git a/tests/lib/CleantalkSP/SpbctWP/RateLimiter/TestSpbcUserRateLimiter.php b/tests/lib/CleantalkSP/SpbctWP/RateLimiter/TestSpbcUserRateLimiter.php new file mode 100644 index 000000000..6eaa280b8 --- /dev/null +++ b/tests/lib/CleantalkSP/SpbctWP/RateLimiter/TestSpbcUserRateLimiter.php @@ -0,0 +1,125 @@ +createTable(SPBC_TBL_RATE_LIMITS); + $this->deleteRows(); + + $this->saved_server = $_SERVER; + $this->setRequest('10.0.0.1', 'PHPUnit'); + } + + protected function tearDown(): void + { + $this->deleteRows(); + $_SERVER = $this->saved_server; + $this->resetRequestCache(); + + parent::tearDown(); + } + + private function deleteRows() + { + global $wpdb; + $wpdb->query($wpdb->prepare('DELETE FROM ' . SPBC_TBL_RATE_LIMITS . ' WHERE type = %s', self::TYPE)); + } + + private function resetRequestCache() + { + Server::getInstance()->variables = array(); + \CleantalkSP\SpbctWP\Helpers\IP::getInstance()->ips_stored = array(); + } + + private function setRequest($ip, $ua) + { + $_SERVER['REMOTE_ADDR'] = $ip; + $_SERVER['HTTP_USER_AGENT'] = $ua; + $this->resetRequestCache(); + } + + private function limiter($user_id, $limit = 2) + { + return new SpbcUserRateLimiter(new RateLimiterConfig(self::TYPE, $limit, 60), $user_id); + } + + public function testLocksAfterLimit() + { + $this->assertTrue($this->limiter(1)->checkPassed()); + $this->assertTrue($this->limiter(1)->checkPassed()); + + $limiter = $this->limiter(1); + $this->assertFalse($limiter->checkPassed()); + $this->assertTrue($limiter->process_ok); + } + + public function testCounterIsPerUser() + { + for ( $i = 0; $i < 3; $i++ ) { + $this->limiter(1)->checkPassed(); + } + + $this->assertFalse($this->limiter(1)->checkPassed()); + $this->assertTrue($this->limiter(2)->checkPassed()); + } + + public function testIpChangeDoesNotResetCounter() + { + for ( $i = 1; $i <= 2; $i++ ) { + $this->setRequest('10.0.0.' . $i, 'PHPUnit'); + $this->assertTrue($this->limiter(1)->checkPassed()); + } + + $this->setRequest('10.0.0.3', 'PHPUnit'); + $this->assertFalse($this->limiter(1)->checkPassed()); + } + + public function testResetUnlocksOnlyGivenUser() + { + for ( $i = 0; $i < 3; $i++ ) { + $this->limiter(1)->checkPassed(); + $this->limiter(2)->checkPassed(); + } + + $this->limiter(1)->reset(); + + $this->assertTrue($this->limiter(1)->checkPassed()); + $this->assertFalse($this->limiter(2)->checkPassed()); + } + + /** + * With strict SQL mode an over-long UA fails the insert and the limiter would fail open. + */ + public function testLongUserAgentDoesNotBreakCountingInStrictMode() + { + global $wpdb; + + $sql_mode = $wpdb->get_var('SELECT @@SESSION.sql_mode'); + $wpdb->query("SET SESSION sql_mode = 'STRICT_ALL_TABLES'"); + + try { + $this->setRequest('10.0.0.1', str_repeat('A', 1000)); + + $this->assertTrue($this->limiter(1, 1)->checkPassed()); + + $limiter = $this->limiter(1, 1); + $this->assertFalse($limiter->checkPassed(), 'limiter must not fail open on a long UA'); + $this->assertTrue($limiter->process_ok); + } finally { + $wpdb->query($wpdb->prepare('SET SESSION sql_mode = %s', $sql_mode)); + } + } +} From 394ac7c54bc52b0a6991ea500a731ba935e7b534 Mon Sep 17 00:00:00 2001 From: alexandergull Date: Fri, 23 Oct 2026 15:40:31 +0500 Subject: [PATCH 2/5] Fix. 2FA. Keep user rate limiter UA charset-safe. Truncating UA by bytes could split a multibyte character; wpdb rejects queries with invalid text and the limiter fails open, so the per-user 2FA attempts cap could be bypassed. Keep only printable ASCII. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- .../SpbcRateLimit/SpbcUserRateLimiter.php | 6 ++- .../RateLimiter/TestSpbcUserRateLimiter.php | 44 +++++++++++++++---- 2 files changed, 39 insertions(+), 11 deletions(-) diff --git a/lib/CleantalkSP/SpbctWP/SpbcRateLimit/SpbcUserRateLimiter.php b/lib/CleantalkSP/SpbctWP/SpbcRateLimit/SpbcUserRateLimiter.php index b1d87a827..99c06835b 100644 --- a/lib/CleantalkSP/SpbctWP/SpbcRateLimit/SpbcUserRateLimiter.php +++ b/lib/CleantalkSP/SpbctWP/SpbcRateLimit/SpbcUserRateLimiter.php @@ -33,12 +33,14 @@ protected function setUID(): void } /** - * Fit the column, so a long attacker-controlled UA can't break the insert (limiter fails open on DB errors). + * UA is informational here (the limiter is keyed by user ID), but an attacker-controlled value + * must not break the insert: the limiter fails open on DB errors, and wpdb rejects queries with + * text invalid for the table charset. Printable ASCII fits any charset and is truncated safely. */ protected function setUA(): void { parent::setUA(); - $this->ua = substr($this->ua, 0, 200); + $this->ua = substr((string) preg_replace('/[^\x20-\x7E]/', '', $this->ua), 0, 200); } /** diff --git a/tests/lib/CleantalkSP/SpbctWP/RateLimiter/TestSpbcUserRateLimiter.php b/tests/lib/CleantalkSP/SpbctWP/RateLimiter/TestSpbcUserRateLimiter.php index 6eaa280b8..b3cd41007 100644 --- a/tests/lib/CleantalkSP/SpbctWP/RateLimiter/TestSpbcUserRateLimiter.php +++ b/tests/lib/CleantalkSP/SpbctWP/RateLimiter/TestSpbcUserRateLimiter.php @@ -100,10 +100,31 @@ public function testResetUnlocksOnlyGivenUser() $this->assertFalse($this->limiter(2)->checkPassed()); } + public function hostileUserAgentProvider() + { + return array( + 'long ascii' => array(str_repeat('A', 1000)), + 'multibyte char split at 200' => array(str_repeat('A', 199) . "\xC3\xA9"), + 'invalid utf-8 byte' => array('Mozilla' . "\xFF\xFE"), + '4-byte utf-8 char' => array('Mozilla ' . "\xF0\x9F\x98\x80"), + 'long multibyte' => array(str_repeat("\xD0\x96", 500)), + ); + } + + /** + * An attacker-controlled UA must not fail the insert: the limiter fails open on DB errors. + * + * @dataProvider hostileUserAgentProvider + */ + public function testHostileUserAgentDoesNotBreakCounting($ua) + { + $this->assertLockedAfterOneAttempt($ua); + } + /** - * With strict SQL mode an over-long UA fails the insert and the limiter would fail open. + * @dataProvider hostileUserAgentProvider */ - public function testLongUserAgentDoesNotBreakCountingInStrictMode() + public function testHostileUserAgentDoesNotBreakCountingInStrictMode($ua) { global $wpdb; @@ -111,15 +132,20 @@ public function testLongUserAgentDoesNotBreakCountingInStrictMode() $wpdb->query("SET SESSION sql_mode = 'STRICT_ALL_TABLES'"); try { - $this->setRequest('10.0.0.1', str_repeat('A', 1000)); - - $this->assertTrue($this->limiter(1, 1)->checkPassed()); - - $limiter = $this->limiter(1, 1); - $this->assertFalse($limiter->checkPassed(), 'limiter must not fail open on a long UA'); - $this->assertTrue($limiter->process_ok); + $this->assertLockedAfterOneAttempt($ua); } finally { $wpdb->query($wpdb->prepare('SET SESSION sql_mode = %s', $sql_mode)); } } + + private function assertLockedAfterOneAttempt($ua) + { + $this->setRequest('10.0.0.1', $ua); + + $this->assertTrue($this->limiter(1, 1)->checkPassed()); + + $limiter = $this->limiter(1, 1); + $this->assertFalse($limiter->checkPassed(), 'limiter must not fail open on a hostile UA'); + $this->assertTrue($limiter->process_ok); + } } From d39e1c4f7a889fa87dc4cfae915b867040238ba2 Mon Sep 17 00:00:00 2001 From: alexandergull Date: Fri, 23 Oct 2026 15:44:36 +0500 Subject: [PATCH 3/5] Fix. 2FA. Atomic step token attempts and claim. Concurrent requests with the same step token could overwrite each other's fail counter and both pass the TOTP replay check. Now each attempt is reserved by compare-and-swap of the token record before the code check, and a correct code logs in only if this request deletes the reserved record. The TOTP time step is marked used after the claim. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- inc/spbc-auth.php | 141 +++++++++++++++++++++++++++----- tests/Inc/TwoFactorAuthTest.php | 114 ++++++++++++++++++++++++-- 2 files changed, 228 insertions(+), 27 deletions(-) diff --git a/inc/spbc-auth.php b/inc/spbc-auth.php index 4cdcaace6..5b1a3334e 100644 --- a/inc/spbc-auth.php +++ b/inc/spbc-auth.php @@ -186,35 +186,102 @@ function spbc_2fa__step_token_is_valid(\WP_User $user) } /** - * Count a failed code attempt. Destroys the step token when attempts are exhausted. + * Atomically replace (or delete) the stored step token record, only if it still equals $old. + * Of concurrent requests holding the same record exactly one succeeds. * - * @param \WP_User $user + * @param \WP_User $user + * @param array $old Record read by this request + * @param array|null $new New record or null to delete * - * @return bool True if the step token is still usable + * @return bool */ -function spbc_2fa__step_token_register_fail(\WP_User $user) +function spbc_2fa__step_token_swap(\WP_User $user, array $old, $new = null) { - global $spbc; + global $wpdb; + + if ( $new === null ) { + $result = $wpdb->query( + $wpdb->prepare( + "DELETE FROM {$wpdb->usermeta} WHERE user_id = %d AND meta_key = %s AND meta_value = %s", + $user->ID, + 'spbc_2fa_step', + maybe_serialize($old) + ) + ); + } else { + $result = $wpdb->query( + $wpdb->prepare( + "UPDATE {$wpdb->usermeta} SET meta_value = %s WHERE user_id = %d AND meta_key = %s AND meta_value = %s", + maybe_serialize($new), + $user->ID, + 'spbc_2fa_step', + maybe_serialize($old) + ) + ); + } + + wp_cache_delete($user->ID, 'user_meta'); + + return (int) $result > 0; +} +/** + * Atomically count a code attempt before the code check, so parallel requests can't exceed the per-token limit. + * + * @param \WP_User $user + * + * @return array|false Reserved record or false if the token was changed by a concurrent request + */ +function spbc_2fa__step_token_reserve_attempt(\WP_User $user) +{ $step = get_user_meta($user->ID, 'spbc_2fa_step', true); if ( ! is_array($step) ) { return false; } - $step['fails'] = (int) $step['fails'] + 1; - if ( $step['fails'] >= SPBC_2FA_MAX_ATTEMPTS ) { - spbc_2fa__step_token_destroy($user); - // E-mail code is dropped too, so the password has to be entered again to get a new one - if ( isset($spbc->data['2fa_keys'][ $user->user_login ]) ) { - unset($spbc->data['2fa_keys'][ $user->user_login ]); - $spbc->save('data'); - } - return false; + $reserved = $step; + $reserved['fails'] = (int) $step['fails'] + 1; + + return spbc_2fa__step_token_swap($user, $step, $reserved) ? $reserved : false; +} + +/** + * Atomically consume the token on the correct code. Only one of concurrent requests may log in. + * + * @param \WP_User $user + * @param array $reserved Record returned by spbc_2fa__step_token_reserve_attempt() + * + * @return bool + */ +function spbc_2fa__step_token_claim(\WP_User $user, array $reserved) +{ + return spbc_2fa__step_token_swap($user, $reserved, null); +} + +/** + * Handle a wrong code. Destroys the step token when attempts are exhausted. + * + * @param \WP_User $user + * @param array $reserved Record returned by spbc_2fa__step_token_reserve_attempt() + * + * @return bool True if the step token is still usable + */ +function spbc_2fa__step_token_after_fail(\WP_User $user, array $reserved) +{ + global $spbc; + + if ( (int) $reserved['fails'] < SPBC_2FA_MAX_ATTEMPTS ) { + return true; } - update_user_meta($user->ID, 'spbc_2fa_step', $step); + spbc_2fa__step_token_destroy($user); + // E-mail code is dropped too, so the password has to be entered again to get a new one + if ( isset($spbc->data['2fa_keys'][ $user->user_login ]) ) { + unset($spbc->data['2fa_keys'][ $user->user_login ]); + $spbc->save('data'); + } - return true; + return false; } /** @@ -262,10 +329,12 @@ function spbc_2fa__user_rate_limiter_reset(\WP_User $user) * @param string $secret * @param string $code * @param bool $is_reused Set to true if the code is correct but its time step was already used + * @param bool $mark_used Mark the time step as used right away. Pass false to call spbc_2fa__app_code_mark_used() + * later, after the step token is claimed * * @return bool */ -function spbc_2fa__app_code_check(\WP_User $user, $secret, $code, &$is_reused = false) +function spbc_2fa__app_code_check(\WP_User $user, $secret, $code, &$is_reused = false, $mark_used = true) { $ga = new GoogleAuthenticator(); $time_step = (int) floor(time() / 30); @@ -282,11 +351,26 @@ function spbc_2fa__app_code_check(\WP_User $user, $secret, $code, &$is_reused = return false; } - update_user_meta($user->ID, 'spbc_2fa_last_step', $time_step); + if ( $mark_used ) { + spbc_2fa__app_code_mark_used($user); + } return true; } +/** + * Mark the current time step as used. If the step has changed since the check, the newer one is marked, + * which rejects the checked code as well. + * + * @param \WP_User $user + * + * @return void + */ +function spbc_2fa__app_code_mark_used(\WP_User $user) +{ + update_user_meta($user->ID, 'spbc_2fa_last_step', (int) floor(time() / 30)); +} + /** * Adding notification to registration form * @@ -1272,6 +1356,12 @@ function spbc_2fa__authenticate() spbc_2fa__locked($user); } + // Per-token attempt is reserved atomically too; a concurrent request holding the same token loses + $step = spbc_2fa__step_token_reserve_attempt($user); + if ( ! $step ) { + spbc_2fa__failed($user, true); + } + spbc_2fa__key_remove_old(); $type2fa = get_user_meta($user->ID, 'spbc_2fa_type', true); @@ -1284,6 +1374,11 @@ function spbc_2fa__authenticate() isset($spbc->data['2fa_keys'][ $user->user_login ]) && hash_equals((string) $spbc->data['2fa_keys'][ $user->user_login ]['val'], $code) ) { + // Only one of concurrent requests with the same token may log in + if ( ! spbc_2fa__step_token_claim($user, $step) ) { + spbc_2fa__failed($user, true); + } + // Delete it so no one could login via this code unset($spbc->data['2fa_keys'][ $user->user_login ]); $spbc->save('data'); @@ -1294,15 +1389,21 @@ function spbc_2fa__authenticate() } elseif ( $type2fa === '2fa_application' && $_2fa_app_token && - spbc_2fa__app_code_check($user, $_2fa_app_token, $code, $is_code_reused) + spbc_2fa__app_code_check($user, $_2fa_app_token, $code, $is_code_reused, false) ) { + // Only one of concurrent requests with the same token may log in + if ( ! spbc_2fa__step_token_claim($user, $step) ) { + spbc_2fa__failed($user, true); + } + + spbc_2fa__app_code_mark_used($user); $user_rate_limiter->reset(); spbc_authenticate__write_log_login($user); spbc_2fa__success($user); } } - spbc_2fa__failed($user, ! spbc_2fa__step_token_register_fail($user), $is_code_reused); + spbc_2fa__failed($user, ! spbc_2fa__step_token_after_fail($user, $step), $is_code_reused); } } } diff --git a/tests/Inc/TwoFactorAuthTest.php b/tests/Inc/TwoFactorAuthTest.php index 57d2ba200..ba0116e55 100644 --- a/tests/Inc/TwoFactorAuthTest.php +++ b/tests/Inc/TwoFactorAuthTest.php @@ -42,6 +42,8 @@ class TwoFactorAuthTest extends \SpbcTestCase private $email_user; private $subscriber; private $saved_server; + private $parallel_hook; + private $parallel_results = array(); /** * Users are created once per class: wp_insert_user()/wp_delete_user() are the slowest part of the test. @@ -120,6 +122,11 @@ private function resetState() $this->resetRequestCache(); remove_filter('wp_doing_ajax', '__return_true'); + if ( $this->parallel_hook ) { + remove_filter('query', $this->parallel_hook); + $this->parallel_hook = null; + } + $this->parallel_results = array(); wp_set_current_user(0); $wpdb->query('DELETE FROM ' . SPBC_TBL_RATE_LIMITS); @@ -252,6 +259,22 @@ private function inOneTimeStep(callable $scenario) } } + /** + * Emulates a concurrent request: runs $parallel right before the first query matching $pattern is executed. + */ + private function runParallelBeforeQuery($pattern, callable $parallel) + { + $triggered = false; + $this->parallel_hook = function ($query) use ($pattern, $parallel, &$triggered) { + if ( ! $triggered && preg_match($pattern, $query) ) { + $triggered = true; + $this->parallel_results[] = $parallel(); + } + return $query; + }; + add_filter('query', $this->parallel_hook); + } + /** * @return array [type, value]: 'auth' => user id, 'redirect' => location, 'die' => status code, 'none' => null */ @@ -358,7 +381,7 @@ public function testExhaustedStepTokenIsRejectedAndDeleted() $this->assertSame('', get_user_meta($this->app_user->ID, 'spbc_2fa_step', true)); } - public function testRegisterFailDestroysTokenAndEmailKeyOnLastAttempt() + public function testReserveAttemptCountsFailsAndAfterFailDestroysTokenAndEmailKeyOnLastAttempt() { global $spbc; @@ -366,19 +389,40 @@ public function testRegisterFailDestroysTokenAndEmailKeyOnLastAttempt() $spbc->data['2fa_keys'][$this->email_user->user_login] = array('generated' => time(), 'val' => '123456'); for ( $i = 1; $i < SPBC_2FA_MAX_ATTEMPTS; $i++ ) { - $this->assertTrue(spbc_2fa__step_token_register_fail($this->email_user)); + $reserved = spbc_2fa__step_token_reserve_attempt($this->email_user); + $this->assertSame($i, $reserved['fails']); + $this->assertSame($reserved, get_user_meta($this->email_user->ID, 'spbc_2fa_step', true)); + $this->assertTrue(spbc_2fa__step_token_after_fail($this->email_user, $reserved)); } - $step = get_user_meta($this->email_user->ID, 'spbc_2fa_step', true); - $this->assertSame(SPBC_2FA_MAX_ATTEMPTS - 1, $step['fails']); - $this->assertFalse(spbc_2fa__step_token_register_fail($this->email_user)); + $reserved = spbc_2fa__step_token_reserve_attempt($this->email_user); + $this->assertFalse(spbc_2fa__step_token_after_fail($this->email_user, $reserved)); $this->assertSame('', get_user_meta($this->email_user->ID, 'spbc_2fa_step', true)); $this->assertArrayNotHasKey($this->email_user->user_login, $spbc->data['2fa_keys']); } - public function testRegisterFailWithoutTokenReturnsFalse() + public function testReserveAttemptWithoutTokenReturnsFalse() { - $this->assertFalse(spbc_2fa__step_token_register_fail($this->app_user)); + $this->assertFalse(spbc_2fa__step_token_reserve_attempt($this->app_user)); + } + + /** + * Two requests which read the same record: only the first swap succeeds. + */ + public function testSwapSucceedsOnlyForActualRecord() + { + $this->giveStepToken($this->app_user); + $read_by_both = get_user_meta($this->app_user->ID, 'spbc_2fa_step', true); + + $first = array_merge($read_by_both, array('fails' => 1)); + $this->assertTrue(spbc_2fa__step_token_swap($this->app_user, $read_by_both, $first)); + $this->assertFalse(spbc_2fa__step_token_swap($this->app_user, $read_by_both, array_merge($read_by_both, array('fails' => 7)))); + $this->assertSame($first, get_user_meta($this->app_user->ID, 'spbc_2fa_step', true)); + + $this->assertFalse(spbc_2fa__step_token_claim($this->app_user, $read_by_both)); + $this->assertTrue(spbc_2fa__step_token_claim($this->app_user, $first)); + $this->assertFalse(spbc_2fa__step_token_claim($this->app_user, $first), 'token can be claimed only once'); + $this->assertSame('', get_user_meta($this->app_user->ID, 'spbc_2fa_step', true)); } // ---------------------------------------------------------------- TOTP check @@ -495,6 +539,62 @@ public function testReusedCodeIsRejectedWithSeparateError() }); } + /** + * Two simultaneous submissions of the same token and code: only one may log in. + */ + public function testConcurrentSubmissionOfSameCodeLogsInOnce() + { + $this->inOneTimeStep(function ($code) { + $this->giveStepToken($this->app_user); + $login = $this->app_user->user_login; + + // The parallel request runs when the first one is about to consume the token + $this->runParallelBeforeQuery( + "/^DELETE FROM \\S*usermeta .*'spbc_2fa_step'/", + function () use ($login, $code) { + return $this->postCode($login, $code); + } + ); + $first = $this->postCode($login, $code); + + $this->assertCount(1, $this->parallel_results, 'parallel request was not emulated'); + $results = array($first, $this->parallel_results[0]); + $this->assertCount( + 1, + array_filter($results, function ($result) { + return $result[0] === 'auth'; + }), + 'Exactly one of concurrent requests must log in: ' . var_export($results, true) + ); + }); + } + + /** + * Simultaneous wrong codes with the same token: each attempt is counted, the loser has to restart. + */ + public function testConcurrentWrongCodesAreAllCounted() + { + $this->giveStepToken($this->app_user); + $login = $this->app_user->user_login; + $wrong = $this->wrongCode(); + + // The parallel request runs when the first one is about to reserve the attempt + $this->runParallelBeforeQuery( + "/^UPDATE \\S*usermeta SET meta_value .*'spbc_2fa_step'/", + function () use ($login, $wrong) { + return $this->postCode($login, $wrong); + } + ); + $first = $this->postCode($login, $wrong); + + $this->assertCount(1, $this->parallel_results, 'parallel request was not emulated'); + $this->assertCodeError($this->parallel_results[0], 1, $this->app_user); + $this->assertRestart($first); + + $step = get_user_meta($this->app_user->ID, 'spbc_2fa_step', true); + $this->assertSame(1, $step['fails']); + } + public function testUserOutsideOf2faRolesCannotLogInByCode() { global $spbc; From efcdf5b515408252823b892e1e797767e64e01dc Mon Sep 17 00:00:00 2001 From: alexandergull Date: Fri, 23 Oct 2026 16:27:20 +0500 Subject: [PATCH 4/5] Fix. 2FA. Revoke pending challenge on password reset. A step token and e-mail code issued for the old password could still complete the login after the password reset. Revoke both on after_password_reset. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- inc/spbc-auth.php | 37 +++++++++++++++++++++------------ tests/Inc/TwoFactorAuthTest.php | 18 ++++++++++++++++ 2 files changed, 42 insertions(+), 13 deletions(-) diff --git a/inc/spbc-auth.php b/inc/spbc-auth.php index 5b1a3334e..38f51652c 100644 --- a/inc/spbc-auth.php +++ b/inc/spbc-auth.php @@ -60,6 +60,8 @@ add_action('login_form', 'spbc_2fa__show_field', 10); add_action('after_password_reset', 'spbc_2fa__2fa_app_replace_meta', 10, 1); add_action('after_password_reset', 'spbc_2fa__user_rate_limiter_reset', 10, 1); + // A challenge issued for the old password must not complete the login + add_action('after_password_reset', 'spbc_2fa__challenge_revoke', 10, 1); // Profile page hook add_action('show_user_profile', 'spbc_2fa__SelfUserProfileEdit', 1); add_action('edit_user_profile', 'spbc_2fa__SelfUserProfileEdit', 1); @@ -268,20 +270,34 @@ function spbc_2fa__step_token_claim(\WP_User $user, array $reserved) */ function spbc_2fa__step_token_after_fail(\WP_User $user, array $reserved) { - global $spbc; - if ( (int) $reserved['fails'] < SPBC_2FA_MAX_ATTEMPTS ) { return true; } - spbc_2fa__step_token_destroy($user); - // E-mail code is dropped too, so the password has to be entered again to get a new one + spbc_2fa__challenge_revoke($user); + spbc_2fa__step_cookie_set('', time() - 3600); + + return false; +} + +/** + * Revoke the pending 2FA challenge: the step token and the e-mail code. + * The password has to be entered again to get a new one. + * + * @param \WP_User $user + * + * @return void + */ +function spbc_2fa__challenge_revoke(\WP_User $user) +{ + global $spbc; + + delete_user_meta($user->ID, 'spbc_2fa_step'); + if ( isset($spbc->data['2fa_keys'][ $user->user_login ]) ) { unset($spbc->data['2fa_keys'][ $user->user_login ]); $spbc->save('data'); } - - return false; } /** @@ -1489,8 +1505,6 @@ function spbc_2fa__success(\WP_User $user) */ function spbc_2fa__locked(\WP_User $user) { - global $spbc; - $type2fa = get_user_meta($user->ID, 'spbc_2fa_type', true); spbc_auth_log( @@ -1503,11 +1517,8 @@ function spbc_2fa__locked(\WP_User $user) ) ); - spbc_2fa__step_token_destroy($user); - if ( isset($spbc->data['2fa_keys'][ $user->user_login ]) ) { - unset($spbc->data['2fa_keys'][ $user->user_login ]); - $spbc->save('data'); - } + spbc_2fa__challenge_revoke($user); + spbc_2fa__step_cookie_set('', time() - 3600); wp_die( esc_html__('Too many wrong authorization codes. Please try again later. Protected - Security by CleanTalk.', 'security-malware-firewall'), diff --git a/tests/Inc/TwoFactorAuthTest.php b/tests/Inc/TwoFactorAuthTest.php index ba0116e55..f2420fb61 100644 --- a/tests/Inc/TwoFactorAuthTest.php +++ b/tests/Inc/TwoFactorAuthTest.php @@ -681,6 +681,24 @@ public function testPerUserLimitSurvivesNewPasswordLoginsAndIpChange() $this->assertCodeError($this->postCode($this->email_user->user_login, '000000'), 1, $this->email_user); } + /** + * A challenge issued for the old password (step cookie + e-mail code) must not complete the login after a reset. + */ + public function testPasswordResetRevokesPendingChallenge() + { + global $spbc; + + $login = $this->email_user->user_login; + $this->giveStepToken($this->email_user); + $spbc->data['2fa_keys'][$login] = array('generated' => time(), 'val' => '123456'); + + spbc_2fa__challenge_revoke($this->email_user); + + $this->assertSame('', get_user_meta($this->email_user->ID, 'spbc_2fa_step', true)); + $this->assertArrayNotHasKey($login, $spbc->data['2fa_keys']); + $this->assertRestart($this->postCode($login, '123456')); + } + public function testPerUserLimitIsResetByPasswordReset() { $this->inOneTimeStep(function ($code) { From 0d2545e94a1687eb0a400b989e3a8e51c21ccdc3 Mon Sep 17 00:00:00 2001 From: alexandergull Date: Fri, 23 Oct 2026 16:29:24 +0500 Subject: [PATCH 5/5] Fix. 2FA. Keep concurrent login's fresh step token on success. The step record is already consumed by the atomic claim, so deleting user meta again on success could revoke a fresh challenge issued to a password login in another browser. Clear only this browser's cookie. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- inc/spbc-auth.php | 15 +++------------ tests/Inc/TwoFactorAuthTest.php | 27 +++++++++++++++++++++++++++ 2 files changed, 30 insertions(+), 12 deletions(-) diff --git a/inc/spbc-auth.php b/inc/spbc-auth.php index 38f51652c..9ecb2e58b 100644 --- a/inc/spbc-auth.php +++ b/inc/spbc-auth.php @@ -300,17 +300,6 @@ function spbc_2fa__challenge_revoke(\WP_User $user) } } -/** - * @param \WP_User $user - * - * @return void - */ -function spbc_2fa__step_token_destroy(\WP_User $user) -{ - delete_user_meta($user->ID, 'spbc_2fa_step'); - spbc_2fa__step_cookie_set('', time() - 3600); -} - /** * Per-user limiter of 2FA code attempts. Not reset by a new password login and not bound to IP. * @@ -1429,7 +1418,9 @@ function spbc_2fa__success(\WP_User $user) { global $spbc; - spbc_2fa__step_token_destroy($user); + // The step record is already consumed by spbc_2fa__step_token_claim(). Don't touch user meta here: + // it may already hold a fresh token of a concurrent password login in another browser. + spbc_2fa__step_cookie_set('', time() - 3600); // Check if password is leaked after 2FA success if (UsersPassCheckHandler::isUserPassLeaked($user->ID)) { diff --git a/tests/Inc/TwoFactorAuthTest.php b/tests/Inc/TwoFactorAuthTest.php index f2420fb61..bc69cca7b 100644 --- a/tests/Inc/TwoFactorAuthTest.php +++ b/tests/Inc/TwoFactorAuthTest.php @@ -506,6 +506,33 @@ public function testValidTotpWithStepTokenLogsIn() }); } + /** + * Successful login must not revoke a fresh challenge issued meanwhile to a password login in another browser. + */ + public function testSuccessKeepsFreshTokenOfConcurrentLogin() + { + $this->inOneTimeStep(function ($code) { + $this->giveStepToken($this->app_user); + $login = $this->app_user->user_login; + $fresh = null; + + // Another browser passes the password right after this request consumed its token + $this->runParallelBeforeQuery( + "/^DELETE FROM \\S*spbc_rate_limits/", + function () use (&$fresh) { + spbc_2fa__step_token_issue($this->app_user); + $fresh = get_user_meta($this->app_user->ID, 'spbc_2fa_step', true); + return true; + } + ); + + $this->assertSame(array('auth', $this->app_user->ID), $this->postCode($login, $code)); + $this->assertCount(1, $this->parallel_results, 'parallel login was not emulated'); + $this->assertIsArray($fresh); + $this->assertSame($fresh, get_user_meta($this->app_user->ID, 'spbc_2fa_step', true)); + }); + } + public function testLoginByEmailWorksWithStepToken() { $this->inOneTimeStep(function ($code) {