-
Notifications
You must be signed in to change notification settings - Fork 653
Password timing attack delays #669
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
65beae0
171a02b
4c7db1b
f639601
8061ef4
2fd66f1
d5b8f59
7dd5ba5
889a44f
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,79 @@ | ||
| import { expect, test, vi } from 'vitest' | ||
| import { | ||
| applyPasswordSubmissionDelay, | ||
| getPasswordSubmissionDelayMs, | ||
| } from '../password.server.ts' | ||
|
|
||
| test('getPasswordSubmissionDelayMs clamps non-positive maxMs to 0', () => { | ||
| const randomInt = vi.fn(() => 123) | ||
|
|
||
| expect(getPasswordSubmissionDelayMs({ maxMs: 0, randomInt })).toBe(0) | ||
| expect(getPasswordSubmissionDelayMs({ maxMs: -5, randomInt })).toBe(0) | ||
|
|
||
| expect(randomInt).not.toHaveBeenCalled() | ||
| }) | ||
|
|
||
| test('getPasswordSubmissionDelayMs treats non-finite maxMs as 0', () => { | ||
| const randomInt = vi.fn(() => 123) | ||
|
|
||
| expect(getPasswordSubmissionDelayMs({ maxMs: Number.NaN, randomInt })).toBe(0) | ||
| expect( | ||
| getPasswordSubmissionDelayMs({ | ||
| maxMs: Number.POSITIVE_INFINITY, | ||
| randomInt, | ||
| }), | ||
| ).toBe(0) | ||
|
|
||
| expect(randomInt).not.toHaveBeenCalled() | ||
| }) | ||
|
|
||
| test('getPasswordSubmissionDelayMs caps maxMs to crypto.randomInt range', () => { | ||
| const randomInt = vi.fn(() => 0) | ||
| void getPasswordSubmissionDelayMs({ maxMs: 2 ** 48, randomInt }) | ||
| expect(randomInt).toHaveBeenCalledWith(0, 2 ** 48 - 1) | ||
| }) | ||
|
|
||
| test('getPasswordSubmissionDelayMs uses an inclusive upper bound', () => { | ||
| const randomInt = vi.fn(() => 42) | ||
|
|
||
| expect(getPasswordSubmissionDelayMs({ maxMs: 250, randomInt })).toBe(42) | ||
| expect(randomInt).toHaveBeenCalledWith(0, 251) | ||
| }) | ||
|
|
||
| test('applyPasswordSubmissionDelay resolves immediately when maxMs is 0', async () => { | ||
| vi.useFakeTimers() | ||
| const setTimeoutSpy = vi.spyOn(globalThis, 'setTimeout') | ||
| setTimeoutSpy.mockClear() | ||
| try { | ||
| const randomInt = vi.fn() | ||
| await applyPasswordSubmissionDelay({ maxMs: 0, randomInt }) | ||
| expect(randomInt).not.toHaveBeenCalled() | ||
| expect(setTimeoutSpy).not.toHaveBeenCalled() | ||
| } finally { | ||
| setTimeoutSpy.mockRestore() | ||
| vi.useRealTimers() | ||
| } | ||
| }) | ||
|
|
||
| test('applyPasswordSubmissionDelay waits for the sampled delay', async () => { | ||
| vi.useFakeTimers() | ||
| try { | ||
| const randomInt = vi.fn(() => 40) | ||
| let resolved = false | ||
| const promise = applyPasswordSubmissionDelay({ maxMs: 250, randomInt }).then( | ||
| () => { | ||
| resolved = true | ||
| }, | ||
| ) | ||
|
|
||
| await vi.advanceTimersByTimeAsync(39) | ||
| expect(resolved).toBe(false) | ||
|
|
||
| await vi.advanceTimersByTimeAsync(1) | ||
| await promise | ||
| expect(resolved).toBe(true) | ||
| } finally { | ||
| vi.useRealTimers() | ||
| } | ||
| }) | ||
|
|
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -10,6 +10,46 @@ const PASSWORD_MIN_LENGTH = 8 | |
| // NOTE: bcrypt only uses the first 72 bytes of the password. | ||
| // Enforcing this avoids giving users a false sense of security. | ||
| const PASSWORD_MAX_BYTES = 72 | ||
| const PASSWORD_SUBMISSION_DELAY_MAX_MS = 250 | ||
| // `crypto.randomInt` requires (max - min) < 2**48 and both args are safe ints. | ||
| // We call `randomInt(0, safeMaxMs + 1)` (exclusive upper bound), so cap `safeMaxMs` | ||
| // to keep the exclusive max within range. | ||
| const MAX_PASSWORD_SUBMISSION_DELAY_MS = 2 ** 48 - 2 | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Near-identical constant names risk confusion and future bugsLow Severity
|
||
|
|
||
| type RandomIntFunction = (min: number, max: number) => number | ||
|
|
||
| function normalizeMaxDelayMs(maxMs: number) { | ||
| if (!Number.isFinite(maxMs)) return 0 | ||
| return Math.min( | ||
| MAX_PASSWORD_SUBMISSION_DELAY_MS, | ||
| Math.max(0, Math.floor(maxMs)), | ||
| ) | ||
| } | ||
|
|
||
| /** | ||
| * Adds random jitter to password submission handling to make timing attacks | ||
| * noisier. Keep the upper bound small to avoid noticeable UX regressions. | ||
| */ | ||
| export function getPasswordSubmissionDelayMs({ | ||
| maxMs = PASSWORD_SUBMISSION_DELAY_MAX_MS, | ||
| randomInt = crypto.randomInt, | ||
| }: { | ||
| maxMs?: number | ||
| randomInt?: RandomIntFunction | ||
| } = {}) { | ||
| const safeMaxMs = normalizeMaxDelayMs(maxMs) | ||
| // `crypto.randomInt` uses an exclusive upper bound. | ||
| return safeMaxMs === 0 ? 0 : randomInt(0, safeMaxMs + 1) | ||
| } | ||
|
|
||
| export async function applyPasswordSubmissionDelay(options?: { | ||
| maxMs?: number | ||
| randomInt?: RandomIntFunction | ||
| }) { | ||
| const delayMs = getPasswordSubmissionDelayMs(options) | ||
| if (delayMs <= 0) return | ||
| await new Promise<void>((resolve) => setTimeout(resolve, delayMs)) | ||
| } | ||
|
|
||
| export async function getPasswordHash(password: string) { | ||
| return bcrypt.hash(password, BCRYPT_COST) | ||
|
|
||


There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Delay fires on all action paths, not just the password-submission path.
applyPasswordSubmissionDelay()is called beforeactionIdis read, so thecancel,requestCode, andverifyCodebranches all incur a gratuitous 0–250 ms delay. Those paths don't touch a password, so there is no security benefit — only unnecessary UX latency. Only the implicitsignUpbranch (line 209+) that callsgetPasswordHashneeds this guard.🛡️ Proposed fix — move the delay after the early-return branches
export async function action({ request }: Route.ActionArgs) { - await applyPasswordSubmissionDelay() const loginInfoSession = await getLoginInfoSession(request) const requestText = await request.text() const form = new URLSearchParams(requestText) const actionId = form.get('actionId') if (actionId === actionIds.cancel) { // ... (early return) } if (actionId === actionIds.requestCode) { // ... (early return) } if (actionId === actionIds.verifyCode) { // ... (early return) } + // Only the signUp path reaches here — guard it with the timing-attack delay. + await applyPasswordSubmissionDelay() const signupEmail = loginInfoSession.getSignupEmail()🤖 Prompt for AI Agents