From 2520b87982b7045fb064e76a347cb478cc538380 Mon Sep 17 00:00:00 2001 From: boban Date: Mon, 15 Jun 2026 19:43:40 +0200 Subject: [PATCH] fix(security): flatten reset-timing residuals from Codex review MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Follow-up hardening on top of 2171895 (which v0.9.2 shipped). That first pass introduced a cost-12 bcrypt dummy that was *slower* than the real verify path — a reverse timing oracle plus CPU-DoS amplification. Replace it with work that mirrors the real path instead of exceeding it, and close the remaining query / exception asymmetries: - resetPassword(): resolve 2FA with a uniform lookup — always exactly one webauthn-existence query (even for a TOTP user, and for a missing account) so its presence/absence can't tell accounts apart. Every path now does one users lookup + one webauthn query + one Google2FA HMAC + one locked recovery-code transaction (real data when present, throwaway data otherwise). - Split burnVerificationTime() into dummyTotpVerify() (HMAC) and dummyRecoveryLookup() (locked SELECT in a transaction); drop the bcrypt Hash::check entirely. A webauthn-only account spends an equivalent throwaway HMAC so its crypto cost matches a TOTP account. - dummyRecoveryLookup() no longer catches DB errors: a DB failure must surface identically to the real useRecoveryCode(), else an outage is an error-response oracle (generic for unknown, 500 for a real account). - sendResetLink(): report() the swallowed Throwable so a broken queue/token store is detectable instead of silently "succeeding" for every address. Tests: ForgotPasswordTimingTest now asserts an identical (users, webauthn_credentials) query fingerprint across TOTP, no-2FA, webauthn-only, and unknown-email branches, plus queued-mail and rate-limiter parity. Codex re-review verdict: clean (residual sub-microsecond crypto differences are below the network/DB noise floor for this self-hosted single-admin threat model). Co-Authored-By: Claude Opus 4.8 (1M context) --- app/Livewire/Auth/ForgotPassword.php | 64 ++++++++++++++++------ tests/Feature/ForgotPasswordTimingTest.php | 62 ++++++++++++--------- 2 files changed, 82 insertions(+), 44 deletions(-) diff --git a/app/Livewire/Auth/ForgotPassword.php b/app/Livewire/Auth/ForgotPassword.php index 08b42ef..d4728a7 100644 --- a/app/Livewire/Auth/ForgotPassword.php +++ b/app/Livewire/Auth/ForgotPassword.php @@ -4,6 +4,8 @@ namespace App\Livewire\Auth; use App\Models\AuditEvent; use App\Models\User; +use App\Models\WebauthnCredential; +use Illuminate\Support\Facades\DB; use Illuminate\Support\Facades\Hash; use Illuminate\Support\Facades\RateLimiter; use Illuminate\Support\Str; @@ -19,9 +21,6 @@ class ForgotPassword extends Component /** Fixed throwaway base32 secret for the dummy TOTP verify on the no-user/no-2FA branch. */ private const DUMMY_TOTP_SECRET = 'ABCDEFGHIJKLMNOP'; - /** Fixed throwaway bcrypt hash (cost 12) for the dummy password comparison on that branch. */ - private const DUMMY_PASSWORD_HASH = '$2y$12$d9GooWgw17GrZ703OLFEDuuCXPvJs0vc/YLflA2JA70Mm6.I.3f.W'; - public string $email = ''; public string $code = ''; @@ -53,8 +52,11 @@ class ForgotPassword extends Component // latency nor an exception can distinguish a known address from an unknown one. try { \Illuminate\Support\Facades\Password::sendResetLink(['email' => $this->email]); - } catch (\Throwable) { - // Intentionally ignored — the response is identical regardless (auth.reset_link_sent). + } catch (\Throwable $e) { + // The client response stays identical regardless — but surface the failure to ops + // (report() → logs), otherwise a broken queue/token store silently "succeeds" for + // every address and a broken reset flow goes unnoticed. + report($e); } // Generic — never reveal whether the email exists. @@ -79,13 +81,29 @@ class ForgotPassword extends Component $user = User::where('email', $this->email)->first(); - if ($user && $user->hasTwoFactorEnabled()) { + // Resolve 2FA status with a *uniform* lookup pattern: always exactly one + // webauthn-existence query — for a TOTP user that wouldn't otherwise need it, and for a + // missing account — so the presence/absence of that query can't tell accounts apart. + $hasWebauthn = $user + ? $user->hasWebauthnCredentials() + : WebauthnCredential::whereKey(0)->exists(); + $hasTotp = (bool) $user?->hasTotp(); + $has2fa = $hasTotp || $hasWebauthn; + + if ($user && $has2fa) { + // Keep the TOTP HMAC uniform: a webauthn-only account has no TOTP secret, so + // verifyTotp() returns without hashing — spend an equivalent throwaway HMAC first so + // this branch costs the same as a TOTP account would. + if (! $hasTotp) { + $this->dummyTotpVerify(); + } $ok = $user->verifyTotp($this->code) || $user->useRecoveryCode($this->code); } else { - // No account, or one without 2FA: run equivalent dummy crypto so this branch costs - // about as much as a real wrong-code attempt — the verification path must not be a - // timing oracle that distinguishes a known address. - $this->burnVerificationTime(); + // No account, or one without 2FA: run the same HMAC + locked recovery lookup a real + // wrong-code attempt does, so the verification path is not a timing/query oracle that + // distinguishes a known address. + $this->dummyTotpVerify(); + $this->dummyRecoveryLookup(); $ok = false; } @@ -117,21 +135,33 @@ class ForgotPassword extends Component } /** - * Constant dummy verification for the no-account / no-2FA branch: mirrors the crypto work a - * real wrong-code attempt does (a Google2FA HMAC verify + a bcrypt comparison) so response - * time can't be used to enumerate accounts. The results are deliberately discarded. + * Stand-in for User::verifyTotp() — one throwaway Google2FA HMAC against a fixed dummy secret, + * so a request with no (or no TOTP) account spends the same crypto as a real wrong-code + * attempt. It must NOT do heavier work than the real path (a cost-12 bcrypt here would make + * this slower, re-opening the oracle in reverse and amplifying CPU DoS). Result is discarded. */ - private function burnVerificationTime(): void + private function dummyTotpVerify(): void { - // Stand-in for User::verifyTotp() — a throwaway HMAC against a fixed dummy secret. try { (new Google2FA)->verifyKey(self::DUMMY_TOTP_SECRET, preg_replace('/\s+/', '', $this->code) ?? ''); } catch (\Throwable) { // ignore — verifyTotp swallows the same failure } + } - // Stand-in for the recovery-code path's cost — a bcrypt compare against a fixed hash. - Hash::check($this->code, self::DUMMY_PASSWORD_HASH); + /** + * Stand-in for User::useRecoveryCode() — a locked SELECT in a transaction + an in_array scan, + * matching the dominant DB cost of the recovery path without touching a real row (id 0 never + * exists, so nothing is locked). Exceptions are deliberately NOT caught: a DB failure must + * surface here exactly as it would in the real path, otherwise an outage becomes an + * error-response oracle (generic for an unknown address, 500 for a real account). + */ + private function dummyRecoveryLookup(): void + { + DB::transaction(function (): void { + $row = User::whereKey(0)->lockForUpdate()->first(); + in_array($this->code, $row?->recoveryCodes() ?? array_fill(0, 8, ''), true); + }); } public function render() diff --git a/tests/Feature/ForgotPasswordTimingTest.php b/tests/Feature/ForgotPasswordTimingTest.php index c71382d..11cbb14 100644 --- a/tests/Feature/ForgotPasswordTimingTest.php +++ b/tests/Feature/ForgotPasswordTimingTest.php @@ -4,10 +4,11 @@ namespace Tests\Feature; use App\Livewire\Auth\ForgotPassword; use App\Models\User; +use App\Models\WebauthnCredential; use App\Notifications\QueuedResetPassword; use Illuminate\Contracts\Queue\ShouldQueue; use Illuminate\Foundation\Testing\RefreshDatabase; -use Illuminate\Support\Facades\Hash; +use Illuminate\Support\Facades\DB; use Illuminate\Support\Facades\Notification; use Illuminate\Support\Facades\RateLimiter; use Livewire\Features\SupportTesting\Testable; @@ -77,39 +78,46 @@ class ForgotPasswordTimingTest extends TestCase ->assertDispatched('notify', message: __('auth.reset_link_sent')); } - // ── (b) the verification branch does constant dummy work ───────────── - public function test_unknown_email_invalid_branch_burns_verification_time(): void + // ── (b) every invalid branch issues an identical query fingerprint ─── + public function test_invalid_branches_have_identical_query_fingerprints(): void { - Hash::spy(); + // A real 2FA account with a wrong code runs a uniform webauthn-existence probe + verifyTotp + // (HMAC) + useRecoveryCode (a locked SELECT in a transaction). The no-account and no-2FA + // branches must produce the SAME query fingerprint (users + webauthn_credentials counts) — + // proving none of them returns early (faster), does heavier work like a cost-12 bcrypt + // (slower), nor skips the webauthn lookup (an extra account-enumeration tell). + $this->twoFactorUser('totp@clusev.local'); + User::factory()->create(['email' => 'plain@clusev.local']); // registered, no 2FA - $this->attemptReset('ghost@clusev.local')->assertHasErrors('code'); + // registered with a security key only (no TOTP) + $keyUser = User::factory()->create(['email' => 'key@clusev.local']); + WebauthnCredential::create(['user_id' => $keyUser->id, 'name' => 'k', 'credential_id' => 'cid', 'public_key' => '{}', 'sign_count' => 0]); - // The no-account branch must still run a bcrypt comparison (dummy work), matching the - // crypto cost of a real wrong-code attempt — otherwise it returns measurably faster. - Hash::shouldHaveReceived('check')->atLeast()->once(); + $totp = $this->queryFingerprint(fn () => $this->attemptReset('totp@clusev.local', '000000')->assertHasErrors('code')); + $plain = $this->queryFingerprint(fn () => $this->attemptReset('plain@clusev.local')->assertHasErrors('code')); + $keyOnly = $this->queryFingerprint(fn () => $this->attemptReset('key@clusev.local')->assertHasErrors('code')); + $unknown = $this->queryFingerprint(fn () => $this->attemptReset('ghost@clusev.local')->assertHasErrors('code')); + + $this->assertGreaterThan(1, $unknown['users'], 'no-account branch must do dummy verification work, not return early'); + $this->assertSame(1, $unknown['webauthn'], 'no-account branch must still issue the webauthn existence probe'); + $this->assertSame($totp, $unknown, 'unknown email must look identical to a real TOTP wrong-code attempt'); + $this->assertSame($totp, $plain, 'a no-2FA account must look identical to a real TOTP wrong-code attempt'); + $this->assertSame($totp, $keyOnly, 'a webauthn-only account must look identical to a real TOTP wrong-code attempt'); } - public function test_no_2fa_user_invalid_branch_burns_verification_time(): void + /** Count queries per table issued while running $action (one DB round-trip = one timing tell). */ + private function queryFingerprint(callable $action): array { - $user = User::factory()->create(['email' => 'plain@clusev.local']); - $this->assertFalse($user->hasTwoFactorEnabled()); + DB::flushQueryLog(); + DB::enableQueryLog(); + $action(); + $queries = collect(DB::getQueryLog())->map(fn ($entry) => strtolower((string) $entry['query'])); + DB::disableQueryLog(); - Hash::spy(); - - $this->attemptReset('plain@clusev.local')->assertHasErrors('code'); - - Hash::shouldHaveReceived('check')->atLeast()->once(); - } - - public function test_two_factor_user_wrong_code_does_not_burn_dummy_time(): void - { - // The real-verification branch uses verifyTotp/useRecoveryCode, NOT the dummy Hash::check. - $this->twoFactorUser(); - Hash::spy(); - - $this->attemptReset('admin@clusev.local', '000000')->assertHasErrors('code'); - - Hash::shouldNotHaveReceived('check'); + return [ + 'users' => $queries->filter(fn ($q) => str_contains($q, '"users"'))->count(), + 'webauthn' => $queries->filter(fn ($q) => str_contains($q, '"webauthn_credentials"'))->count(), + ]; } // ── structural equivalence of the invalid branches ──────────────────