fix(security): flatten reset-timing residuals from Codex review
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) <noreply@anthropic.com>
feat/v1-foundation
parent
db179ad98a
commit
2520b87982
|
|
@ -4,6 +4,8 @@ namespace App\Livewire\Auth;
|
||||||
|
|
||||||
use App\Models\AuditEvent;
|
use App\Models\AuditEvent;
|
||||||
use App\Models\User;
|
use App\Models\User;
|
||||||
|
use App\Models\WebauthnCredential;
|
||||||
|
use Illuminate\Support\Facades\DB;
|
||||||
use Illuminate\Support\Facades\Hash;
|
use Illuminate\Support\Facades\Hash;
|
||||||
use Illuminate\Support\Facades\RateLimiter;
|
use Illuminate\Support\Facades\RateLimiter;
|
||||||
use Illuminate\Support\Str;
|
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. */
|
/** Fixed throwaway base32 secret for the dummy TOTP verify on the no-user/no-2FA branch. */
|
||||||
private const DUMMY_TOTP_SECRET = 'ABCDEFGHIJKLMNOP';
|
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 $email = '';
|
||||||
|
|
||||||
public string $code = '';
|
public string $code = '';
|
||||||
|
|
@ -53,8 +52,11 @@ class ForgotPassword extends Component
|
||||||
// latency nor an exception can distinguish a known address from an unknown one.
|
// latency nor an exception can distinguish a known address from an unknown one.
|
||||||
try {
|
try {
|
||||||
\Illuminate\Support\Facades\Password::sendResetLink(['email' => $this->email]);
|
\Illuminate\Support\Facades\Password::sendResetLink(['email' => $this->email]);
|
||||||
} catch (\Throwable) {
|
} catch (\Throwable $e) {
|
||||||
// Intentionally ignored — the response is identical regardless (auth.reset_link_sent).
|
// 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.
|
// Generic — never reveal whether the email exists.
|
||||||
|
|
@ -79,13 +81,29 @@ class ForgotPassword extends Component
|
||||||
|
|
||||||
$user = User::where('email', $this->email)->first();
|
$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);
|
$ok = $user->verifyTotp($this->code) || $user->useRecoveryCode($this->code);
|
||||||
} else {
|
} else {
|
||||||
// No account, or one without 2FA: run equivalent dummy crypto so this branch costs
|
// No account, or one without 2FA: run the same HMAC + locked recovery lookup a real
|
||||||
// about as much as a real wrong-code attempt — the verification path must not be a
|
// wrong-code attempt does, so the verification path is not a timing/query oracle that
|
||||||
// timing oracle that distinguishes a known address.
|
// distinguishes a known address.
|
||||||
$this->burnVerificationTime();
|
$this->dummyTotpVerify();
|
||||||
|
$this->dummyRecoveryLookup();
|
||||||
$ok = false;
|
$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
|
* Stand-in for User::verifyTotp() — one throwaway Google2FA HMAC against a fixed dummy secret,
|
||||||
* real wrong-code attempt does (a Google2FA HMAC verify + a bcrypt comparison) so response
|
* so a request with no (or no TOTP) account spends the same crypto as a real wrong-code
|
||||||
* time can't be used to enumerate accounts. The results are deliberately discarded.
|
* 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 {
|
try {
|
||||||
(new Google2FA)->verifyKey(self::DUMMY_TOTP_SECRET, preg_replace('/\s+/', '', $this->code) ?? '');
|
(new Google2FA)->verifyKey(self::DUMMY_TOTP_SECRET, preg_replace('/\s+/', '', $this->code) ?? '');
|
||||||
} catch (\Throwable) {
|
} catch (\Throwable) {
|
||||||
// ignore — verifyTotp swallows the same failure
|
// 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()
|
public function render()
|
||||||
|
|
|
||||||
|
|
@ -4,10 +4,11 @@ namespace Tests\Feature;
|
||||||
|
|
||||||
use App\Livewire\Auth\ForgotPassword;
|
use App\Livewire\Auth\ForgotPassword;
|
||||||
use App\Models\User;
|
use App\Models\User;
|
||||||
|
use App\Models\WebauthnCredential;
|
||||||
use App\Notifications\QueuedResetPassword;
|
use App\Notifications\QueuedResetPassword;
|
||||||
use Illuminate\Contracts\Queue\ShouldQueue;
|
use Illuminate\Contracts\Queue\ShouldQueue;
|
||||||
use Illuminate\Foundation\Testing\RefreshDatabase;
|
use Illuminate\Foundation\Testing\RefreshDatabase;
|
||||||
use Illuminate\Support\Facades\Hash;
|
use Illuminate\Support\Facades\DB;
|
||||||
use Illuminate\Support\Facades\Notification;
|
use Illuminate\Support\Facades\Notification;
|
||||||
use Illuminate\Support\Facades\RateLimiter;
|
use Illuminate\Support\Facades\RateLimiter;
|
||||||
use Livewire\Features\SupportTesting\Testable;
|
use Livewire\Features\SupportTesting\Testable;
|
||||||
|
|
@ -77,39 +78,46 @@ class ForgotPasswordTimingTest extends TestCase
|
||||||
->assertDispatched('notify', message: __('auth.reset_link_sent'));
|
->assertDispatched('notify', message: __('auth.reset_link_sent'));
|
||||||
}
|
}
|
||||||
|
|
||||||
// ── (b) the verification branch does constant dummy work ─────────────
|
// ── (b) every invalid branch issues an identical query fingerprint ───
|
||||||
public function test_unknown_email_invalid_branch_burns_verification_time(): void
|
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
|
$totp = $this->queryFingerprint(fn () => $this->attemptReset('totp@clusev.local', '000000')->assertHasErrors('code'));
|
||||||
// crypto cost of a real wrong-code attempt — otherwise it returns measurably faster.
|
$plain = $this->queryFingerprint(fn () => $this->attemptReset('plain@clusev.local')->assertHasErrors('code'));
|
||||||
Hash::shouldHaveReceived('check')->atLeast()->once();
|
$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']);
|
DB::flushQueryLog();
|
||||||
$this->assertFalse($user->hasTwoFactorEnabled());
|
DB::enableQueryLog();
|
||||||
|
$action();
|
||||||
|
$queries = collect(DB::getQueryLog())->map(fn ($entry) => strtolower((string) $entry['query']));
|
||||||
|
DB::disableQueryLog();
|
||||||
|
|
||||||
Hash::spy();
|
return [
|
||||||
|
'users' => $queries->filter(fn ($q) => str_contains($q, '"users"'))->count(),
|
||||||
$this->attemptReset('plain@clusev.local')->assertHasErrors('code');
|
'webauthn' => $queries->filter(fn ($q) => str_contains($q, '"webauthn_credentials"'))->count(),
|
||||||
|
];
|
||||||
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');
|
|
||||||
}
|
}
|
||||||
|
|
||||||
// ── structural equivalence of the invalid branches ──────────────────
|
// ── structural equivalence of the invalid branches ──────────────────
|
||||||
|
|
|
||||||
Loading…
Reference in New Issue