fix(auth): equalise signin response timing across rejection paths - #12
Conversation
Every failing path on POST /email-password/authorize now pays the same bcrypt cost: the disabled-signin/break-glass gate and the unknown-user rejection compare the submitted password against a precomputed dummy hash (cost = SALT_ROUNDS) before throwing, so measuring response time no longer distinguishes allowlisted emails, existing accounts, or unknown users. Follow-up to the allowlist oracle fix in PR #11, per its review finding on the residual timing channel. A unit test pins the dummy hash format and cost factor so a future regeneration cannot silently desync it from real password hashing.
|
Warning Review limit reachedNext included review available in 25 minutes. View limit detailsLimit details: You’ve used the included review currently available. You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. Review configuration: ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Advanced Run ID: 📒 Files selected for processing (3)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a precomputed timing-safe dummy password hash to equalize the CPU-heavy bcrypt comparison cost on early-rejected sign-in attempts, helping to prevent timing attacks. However, the review highlights that significant timing discrepancies still remain due to database I/O operations—specifically, audit log write latency and database query latency differences—as well as an early return in the legacy service account check. Mitigations such as logging failed attempts for passwordless users, making audit log writes non-blocking, and simulating database latency are recommended to fully address these timing vulnerabilities.
| if (!user || !user.password) { | ||
| // Equalise timing with the real compare below: unknown users and | ||
| // passwordless accounts must not be distinguishable by response time. | ||
| await compare(password, TIMING_SAFE_DUMMY_PASSWORD_HASH); | ||
|
|
||
| throw new AppError(AuthenticationErrorCode.InvalidCredentials, { | ||
| message: 'Invalid email or password', | ||
| }); |
There was a problem hiding this comment.
While this PR successfully equalises the CPU-heavy bcrypt comparison cost, two timing discrepancies remain due to database I/O operations:\n\n1. Audit Log Write Latency: When an existing user with a password enters an incorrect password, the application performs and awaits a database write (prisma.userSecurityAuditLog.create on line 138), adding database insertion latency (typically 5–50ms) to the response time. However, when a user does not exist, or is a passwordless/SSO-only user (entering this !user || !user.password block), no audit log is written. This allows an attacker to distinguish between existing accounts with passwords and non-existent or passwordless accounts.\n\n2. Database Query Latency: In the disabled-signin path (lines 105-113), the application performs a dummy comparison and throws immediately without querying the database. In contrast, the normal sign-in path queries the database (prisma.user.findFirst on line 119). This difference in database query latency (~5-50ms) can allow an attacker to distinguish whether an email is on the break-glass allowlist.\n\n### Suggested Mitigation\n- Log failed attempts for passwordless users: If user exists but has no password, write a SIGN_IN_FAIL audit log for them before throwing.\n- Make audit log writes non-blocking: Consider not awaiting the prisma.userSecurityAuditLog.create promise directly in the main request-response cycle, or use a background task runner/ctx.executionCtx.waitUntil if running in a serverless environment.\n- Simulate DB latency or query anyway: For the disabled-signin path, consider performing a dummy database query or always querying the database to ensure database query latency is uniform across all paths.\n\nAdditionally, note that the legacy/deleted service account check at lines 115-117 returns a FORBIDDEN 403 response immediately without performing any bcrypt comparison. This creates a massive timing difference (a few milliseconds vs ~250ms) and a different status code, making these service accounts easily identifiable.
1 participant
Why
Follow-up to PR #11, addressing the residual timing channel its reviewer flagged: the disabled-signin/break-glass gate and the unknown-user rejection threw before any bcrypt work, while a wrong password for an existing account paid a full cost-12 bcrypt compare (~hundreds of ms). Measuring response time could therefore distinguish allowlisted emails and existing accounts even after the error oracle was closed.
What
TIMING_SAFE_DUMMY_PASSWORD_HASH(precomputed bcrypt hash, cost = SALT_ROUNDS = 12) inpackages/lib/constants/auth.ts.POST /email-password/authorize: both early-rejection paths (disabled signin + non-allowlisted email; unknown user or user without password) nowawait compare(password, dummy)before throwing the same INVALID_CREDENTIALS error, so every failing path pays equivalent bcrypt cost.SALT_ROUNDSso a future regeneration cannot silently desync it.Verification