diff --git a/backend/handlers/auth/auth_test.go b/backend/handlers/auth/auth_test.go index 74443db..0c62f23 100644 --- a/backend/handlers/auth/auth_test.go +++ b/backend/handlers/auth/auth_test.go @@ -1853,7 +1853,11 @@ func TestValidateUKPhoneNumber_RejectsMixedInjectionPayloads(t *testing.T) { // ============================================================================= // TestLogin_AccountLockout_After5Failures verifies that after 5 failed login -// attempts, the account is locked and the next login attempt returns HTTP 429. +// attempts, the account is locked and the next login attempt returns HTTP 401 — +// INDISTINGUISHABLE from a wrong password (F5.5): the old 429 lockout response +// revealed account existence and lockout state, so an attacker could probe +// whether their lockout DoS of a victim account was in effect. The DB-side +// lockout state (failed_attempts >= 5, locked_until set) is unchanged. func TestLogin_AccountLockout_After5Failures(t *testing.T) { ctx, tx := resetTestData(t) @@ -1882,8 +1886,8 @@ func TestLogin_AccountLockout_After5Failures(t *testing.T) { Password: "wrongpassword", } w := testutils.MakeRequestNoAuth(handler, "POST", "/api/login", body, ctx) - if w.Code != http.StatusTooManyRequests { - t.Errorf("expected 429 after 5 failures, got %d. body: %s", w.Code, w.Body.String()) + if w.Code != http.StatusUnauthorized { + t.Errorf("expected 401 on a locked account (F5.5 — indistinguishable from a wrong password), got %d. body: %s", w.Code, w.Body.String()) } var failedAttempts int @@ -1945,11 +1949,12 @@ func TestLogin_AccountLockout_ResetsOnSuccess(t *testing.T) { } } -// TestLogin_AccountLockout_CappedAt30Min verifies the LOW-6 fix: the lockout -// ceiling never exceeds 30 minutes no matter how many failed attempts pile up -// (previously 20+ failures locked the account for 2 hours — a persistent, +// TestLogin_AccountLockout_CappedAt60Min verifies the LOW-6 follow-up: the +// escalating lockout ceiling never exceeds 60 minutes no matter how many failed +// attempts pile up (the 15/30/60-minute tiers replaced the old flat 30-minute +// cap; the earliest pre-cap lockout locked 20+ failures for 2 hours — a // repeatedly-extendable DoS window for a guessing attacker). -func TestLogin_AccountLockout_CappedAt30Min(t *testing.T) { +func TestLogin_AccountLockout_CappedAt60Min(t *testing.T) { ctx, tx := resetTestData(t) handler := http.HandlerFunc(LoginHandler) @@ -1961,7 +1966,8 @@ func TestLogin_AccountLockout_CappedAt30Min(t *testing.T) { defer fixtures.DeleteUser(tx, userID) defer tx.Exec(ctx, "UPDATE users SET failed_attempts = 0, locked_until = NULL WHERE id = $1", userID) - // Simulate 20 prior failures — the old CASE locked for 2 hours here. + // Simulate 20 prior failures — the escalation CASE (15/30/60min tiers) + // caps the lock here instead of growing without bound. _, err = tx.Exec(ctx, "UPDATE users SET failed_attempts = 20 WHERE id = $1", userID) if err != nil { t.Fatalf("failed to seed failed_attempts: %v", err) @@ -1976,10 +1982,11 @@ func TestLogin_AccountLockout_CappedAt30Min(t *testing.T) { if w.Code != http.StatusUnauthorized { t.Fatalf("expected 401 on the lock-setting attempt, got %d. body: %s", w.Code, w.Body.String()) } - // Second attempt hits the active lock → 429. + // Second attempt hits the active lock → 401 (F5.5: indistinguishable from + // a wrong password; the old 429 leaked the lockout state). w = testutils.MakeRequestNoAuth(handler, "POST", "/api/login", body, ctx) - if w.Code != http.StatusTooManyRequests { - t.Fatalf("expected 429, got %d. body: %s", w.Code, w.Body.String()) + if w.Code != http.StatusUnauthorized { + t.Fatalf("expected 401 on the active lock, got %d. body: %s", w.Code, w.Body.String()) } var lockedUntil *time.Time @@ -1991,8 +1998,8 @@ func TestLogin_AccountLockout_CappedAt30Min(t *testing.T) { if lockedUntil == nil { t.Fatal("expected locked_until to be set") } - if until := lockedUntil.Sub(clock.Now()); until > 30*time.Minute { - t.Errorf("lockout must never exceed the 30-minute ceiling, got %v", until) + if until := lockedUntil.Sub(clock.Now()); until > 60*time.Minute { + t.Errorf("lockout must never exceed the 60-minute ceiling, got %v", until) } } diff --git a/backend/handlers/auth/local.go b/backend/handlers/auth/local.go index 136fc8e..81f55aa 100644 --- a/backend/handlers/auth/local.go +++ b/backend/handlers/auth/local.go @@ -480,13 +480,31 @@ func LoginHandler(w http.ResponseWriter, r *http.Request) { return } - // Check if account is locked + // Check if account is locked (F5.5): the response MUST be indistinguishable + // from a generic invalid-credentials failure — same status, same body, and + // the same constant-time bcrypt work — so an attacker can never tell + // "locked" from "wrong password". A distinguishable lockout (the old 429 + // "account is temporarily locked") is an account-existence oracle AND a + // lockout-probing signal: an attacker burning 5 wrong passwords to DoS a + // victim could then watch the victim's lockout state flip. The lockout + // itself is inherent to the 5-attempt progressive policy; hiding the state + // is what removes the oracle. The audit line stays server-side only. + // A locked-out user's recovery is the backend-only password-reset flow (see + // the success-path TODO below) or an operator clearing the columns at the DB. var failedAttempts int var lockedUntil *time.Time err = db.Conn.QueryRow(r.Context(), `SELECT failed_attempts, locked_until FROM users WHERE id = $1`, userID).Scan(&failedAttempts, &lockedUntil) if err == nil && lockedUntil != nil && clock.Now().Before(*lockedUntil) { - http.Error(w, "account is temporarily locked. try again later.", http.StatusTooManyRequests) + // Burn the same constant-time bcrypt compare a real login would, so + // the locked path's latency cannot distinguish it either. The compare + // result is discarded: a locked account stays locked (and the result + // could match if the attacker guessed the password — still no login). + if release, ok := acquireBcryptSlot(); ok { + _ = bcrypt.CompareHashAndPassword([]byte(passwordHash), []byte(req.Password)) + release() + } log.Printf("LOGIN_AUDIT: locked account attempt - user=%s ip=%s", userID, middleware.GetClientIP(r.Context())) + http.Error(w, "invalid credentials", http.StatusUnauthorized) return } @@ -554,6 +572,7 @@ func LoginHandler(w http.ResponseWriter, r *http.Request) { SET failed_attempts = failed_attempts + 1, locked_until = CASE WHEN failed_attempts + 1 >= 5 THEN NOW() + (CASE + WHEN failed_attempts + 1 >= 10 THEN INTERVAL '60 minutes' WHEN failed_attempts + 1 >= 7 THEN INTERVAL '30 minutes' ELSE INTERVAL '15 minutes' END) @@ -588,14 +607,19 @@ func LoginHandler(w http.ResponseWriter, r *http.Request) { // LOW 6 documented gap (finding 6): there is NO password-reset UI — the // backend-only reset flow (GenerateVerificationCodeHandler/VerifyCodeHandler) // has no frontend link, so a user locked out by a guessing attacker has no - // self-service recovery until locked_until lapses (capped at 30 minutes — - // see the failure path above); the operator can only intervene at the DB. - // The lockout counter stays keyed per-user (not per-(user,IP)) because this - // codebase deliberately rejects IP-in-the-key for account-level budgets (see - // the 2FA limiter note in main.go, B8): a client that rotates its source IP - // would mint a fresh bucket per IP and collapse the per-account budget. The - // 30-minute ceiling is the bounded-DoS compromise; successful 2FA verifies - // also clear the lockout (internal/twofa.Check). + // self-service recovery until locked_until lapses (15min at 5+ failures, + // 30min at 7+, 60min at 10+ — see the failure path above); the operator can + // only intervene at the DB. The escalating ceiling is the repeat-DoS + // mitigation: an attacker who keeps guessing past each unlock makes the lock + // LONGER (up to 60 minutes) instead of merely sustaining the 15-minute tier, + // raising the effort-per-DoS ratio while the response stays the uniform 401 + // (never distinguishable from a wrong password). The lockout counter stays + // keyed per-user (not per-(user,IP)) because this codebase deliberately + // rejects IP-in-the-key for account-level budgets (see the 2FA limiter note + // in main.go, B8): a client that rotates its source IP would mint a fresh + // bucket per IP and collapse the per-account budget. The 60-minute ceiling + // is the bounded-DoS compromise; successful 2FA verifies also clear the + // lockout (internal/twofa.Check). tx, err := db.Conn.Begin(r.Context()) if err != nil { log.Printf("Failed to begin transaction: %v", err) diff --git a/backend/handlers/auth/lockout_test.go b/backend/handlers/auth/lockout_test.go new file mode 100644 index 0000000..67f3432 --- /dev/null +++ b/backend/handlers/auth/lockout_test.go @@ -0,0 +1,111 @@ +//go:build test + +package auth + +// F5.5 regression: a locked account must respond EXACTLY like a generic +// invalid-credentials failure — same status, same body — so an attacker can +// never distinguish "locked" from "wrong password". The old distinguishable +// 429 lockout was an account-existence oracle and a lockout-probing signal. + +import ( + "net/http" + "testing" + + "crussell/testutils" + "crussell/testutils/fixtures" + + "github.com/stretchr/testify/require" +) + +// TestLogin_LockedAccount_IndistinguishableFromWrongPassword pins the F5.5 +// fix: a login attempt against a locked account returns the SAME status and +// byte-identical body as a wrong-password attempt against a normal account. +// Before the fix the locked account answered 429 "account is temporarily +// locked..." — distinguishable from the 401 "invalid credentials" a wrong +// password returns, revealing account existence and lockout state. +func TestLogin_LockedAccount_IndistinguishableFromWrongPassword(t *testing.T) { + ctx, tx := testutils.SetupTestTx(t) + handler := http.HandlerFunc(LoginHandler) + + normalID, err := fixtures.CreateTestUserWithEmail(tx, "normal@test.com", "verified_email") + require.NoError(t, err) + defer fixtures.DeleteUser(tx, normalID) + + lockedID, err := fixtures.CreateTestUserWithEmail(tx, "locked@test.com", "verified_email") + require.NoError(t, err) + defer fixtures.DeleteUser(tx, lockedID) + // Lock the account the way the failure path would after 5 wrong passwords. + _, err = tx.Exec(ctx, `UPDATE users SET failed_attempts = 5, locked_until = NOW() + INTERVAL '15 minutes' WHERE id = $1`, lockedID) + require.NoError(t, err) + + wWrong := testutils.MakeRequestNoAuth(handler, "POST", "/api/login", + LoginRequest{Email: "normal@test.com", Password: "wrong-password"}, ctx) + wLocked := testutils.MakeRequestNoAuth(handler, "POST", "/api/login", + LoginRequest{Email: "locked@test.com", Password: "wrong-password"}, ctx) + + require.Equal(t, http.StatusUnauthorized, wWrong.Code, "a wrong password must be 401") + require.Equal(t, http.StatusUnauthorized, wLocked.Code, "a locked account must be 401, not 429 (F5.5)") + require.Equal(t, wWrong.Body.String(), wLocked.Body.String(), + "the locked-account body must be byte-identical to the wrong-password body (no lockout oracle)") +} + +// TestLogin_LockedAccount_CorrectPasswordStillUniform verifies the locked path +// stays indistinguishable even when the attacker submits the CORRECT password: +// the account is still refused with the generic 401 — the compare result is +// deliberately discarded on the locked path. +func TestLogin_LockedAccount_CorrectPasswordStillUniform(t *testing.T) { + ctx, tx := testutils.SetupTestTx(t) + handler := http.HandlerFunc(LoginHandler) + + // fixtures.CreateTestUserWithEmail hashes "testpassword123". + lockedID, err := fixtures.CreateTestUserWithEmail(tx, "locked-correct@test.com", "verified_email") + require.NoError(t, err) + defer fixtures.DeleteUser(tx, lockedID) + _, err = tx.Exec(ctx, `UPDATE users SET failed_attempts = 5, locked_until = NOW() + INTERVAL '15 minutes' WHERE id = $1`, lockedID) + require.NoError(t, err) + + w := testutils.MakeRequestNoAuth(handler, "POST", "/api/login", + LoginRequest{Email: "locked-correct@test.com", Password: "testpassword123"}, ctx) + + require.Equal(t, http.StatusUnauthorized, w.Code, "a locked account must refuse even the correct password (F5.5)") + require.Equal(t, "invalid credentials\n", w.Body.String()) +} + +// TestLogin_LockoutEscalation pins the escalating lockout (LOW 6 follow-up): the +// lock duration grows with sustained failure rounds — 15min at 5-6 failures, +// 30min at 7-9, 60min at 10+ — so an attacker who keeps guessing past each +// unlock makes the lock LONGER instead of merely sustaining the 15-minute tier. +// Every round re-arms the account (locked_until reset to NULL, simulating the +// attacker retrying after each unlock); each attempt stays a uniform 401, so +// the escalation adds no distinguishable response (F5.5). +func TestLogin_LockoutEscalation(t *testing.T) { + ctx, tx := testutils.SetupTestTx(t) + handler := http.HandlerFunc(LoginHandler) + + userID, err := fixtures.CreateTestUserWithEmail(tx, "escalate@test.com", "verified_email") + require.NoError(t, err) + defer fixtures.DeleteUser(tx, userID) + + // lockSeconds re-arms the account with the given failure count, performs one + // wrong-password login, and returns how long the resulting lock lasts. + lockSeconds := func(failedAttempts int) float64 { + t.Helper() + _, err = tx.Exec(ctx, `UPDATE users SET failed_attempts = $1, locked_until = NULL WHERE id = $2`, failedAttempts, userID) + require.NoError(t, err) + + w := testutils.MakeRequestNoAuth(handler, "POST", "/api/login", + LoginRequest{Email: "escalate@test.com", Password: "wrong-password"}, ctx) + require.Equal(t, http.StatusUnauthorized, w.Code, "every lockout round stays a uniform 401 (F5.5)") + + var secs float64 + err = tx.QueryRow(ctx, `SELECT EXTRACT(EPOCH FROM (locked_until - NOW())) FROM users WHERE id = $1`, userID).Scan(&secs) + require.NoError(t, err) + return secs + } + + require.InDelta(t, 900, lockSeconds(4), 5, "5th failure locks for 15 minutes") + require.InDelta(t, 900, lockSeconds(5), 5, "6th failure stays on the 15-minute tier") + require.InDelta(t, 1800, lockSeconds(6), 5, "7th failure escalates to 30 minutes") + require.InDelta(t, 1800, lockSeconds(8), 5, "9th failure stays on the 30-minute tier") + require.InDelta(t, 3600, lockSeconds(9), 5, "10th failure escalates to the 60-minute ceiling") +} diff --git a/backend/internal/twofa/twofa.go b/backend/internal/twofa/twofa.go index 241b897..c79aeb7 100644 --- a/backend/internal/twofa/twofa.go +++ b/backend/internal/twofa/twofa.go @@ -475,16 +475,32 @@ func Check(ctx context.Context, userID string, st *AttemptState, reqCode string, // lifetime. The interactive setup/disable flows pass consume=false: // they clear the pending fields themselves on success (enableTwoFA / // disableTwoFA), so the code must stay valid through the whole - // verification handshake here. The write goes through the same - // context-routed connection as the rest of Check, so verification and - // consumption are one unit. - if _, err := db.Conn.Exec(ctx, ` + // verification handshake here. + // + // DB-ATOMIC (F5.5): the UPDATE is conditional on the exact digest this + // verify just matched (WHERE id=$1 AND two_factor_pending_code_hash=$2) + // and reports rows affected. The per-user mutex above only serializes + // attempts WITHIN one process; two concurrent verifications of the same + // code on different instances both read the same digest and both match + // it, but only the first conditional UPDATE can affect a row — the + // loser sees 0 rows and must fail (MissingOrExpired), so one code + // authorizes exactly ONE operation even across instances. + tag, err := db.Conn.Exec(ctx, ` UPDATE users SET two_factor_pending_code_hash = NULL, two_factor_pending_code_expires = NULL - WHERE id = $1 - `, userID); err != nil { + WHERE id = $1 AND two_factor_pending_code_hash = $2 + `, userID, pendingHash.String) + if err != nil { + // The DB is erroring — atomicity cannot be proven. Keep the prior + // fail-open behaviour (report the verify as OK) so the user is not + // stranded; the in-process mutex still serializes same-instance + // verifications, and a degraded DB is the only way to reach here. log.Printf("failed to consume 2FA pending code for user %s: %v", userID, err) + } else if tag.RowsAffected() == 0 { + // A concurrent verification consumed the code between this SELECT + // and this UPDATE. Single-use means this one must fail. + return MissingOrExpired, nil } } return OK, nil diff --git a/backend/internal/twofa/twofa_test.go b/backend/internal/twofa/twofa_test.go index a6cdac8..97b2e9b 100644 --- a/backend/internal/twofa/twofa_test.go +++ b/backend/internal/twofa/twofa_test.go @@ -11,6 +11,7 @@ package twofa import ( "context" "database/sql" + "sync" "testing" "time" @@ -116,6 +117,67 @@ func TestVerifyForUser_SuccessClearsLoginLockout(t *testing.T) { require.Nil(t, lockedUntil, "successful 2FA verify must clear locked_until") } +// TestVerifyForUser_DBAtomicConsume_Concurrent pins the DB-ATOMIC consume +// (F5.5): two concurrent verifications of the SAME code must result in EXACTLY +// one success. The per-user mutex (StateFor) only serializes verifications +// within one process, so this test races two checks that each hold their OWN +// attempt state — bypassing the mutex exactly like two processes behind the +// same DB would — and relies on the conditional UPDATE in Check's consume path +// (WHERE id=$1 AND two_factor_pending_code_hash=$2) to make the code single-use +// across instances. The concurrent checks race through the POOL (pgx.Tx is not +// concurrency-safe), so the user + pending code are seeded and cleaned up +// directly on the pool proxy instead of a rollback transaction. +func TestVerifyForUser_DBAtomicConsume_Concurrent(t *testing.T) { + userID, err := fixtures.CreateTestUser(db.Conn) + require.NoError(t, err) + t.Cleanup(func() { + _, _ = db.Conn.Exec(context.Background(), `DELETE FROM users WHERE id = $1`, userID) + }) + + _, err = db.Conn.Exec(context.Background(), ` + UPDATE users + SET two_factor_method = 'email', + two_factor_pending_code_hash = $2, + two_factor_pending_code_expires = $3 + WHERE id = $1`, userID, Hash("424242"), clock.Now().Add(10*time.Minute)) + require.NoError(t, err) + + const workers = 2 + type outcome struct { + result Result + err error + } + results := make(chan outcome, workers) + var wg sync.WaitGroup + for i := 0; i < workers; i++ { + wg.Add(1) + go func() { + defer wg.Done() + st := &AttemptState{} + st.SetLastActive(clock.Now()) + res, err := Check(context.Background(), userID, st, "424242", true) + results <- outcome{result: res, err: err} + }() + } + wg.Wait() + close(results) + + okCount, missingCount := 0, 0 + for r := range results { + require.NoError(t, r.err, "no DB failure may occur in a concurrent verify") + switch r.result { + case OK: + okCount++ + case MissingOrExpired: + missingCount++ + default: + t.Errorf("unexpected verify result %v", r.result) + } + } + require.Equal(t, 1, okCount, "exactly one of two concurrent verifies of the same code must succeed") + require.Equal(t, 1, missingCount, "the losing concurrent verify must observe the code consumed (DB-atomic single-use)") +} + // TestConsumePendingCode pins the MEDIUM-2 contract: ConsumePendingCode NULLs // the stored pending-code digest and expiry (idempotently), and is the ONLY // place a verified-but-unconsumed code dies on the saved-card charge path.