fix(api): clear bound passkeys on password change to close a takeover foothold
handleChangePassword revoked other sessions but never cleared webauthn_credentials, and enrollment needs no step-up. A passkey planted through a transiently-hijacked session needs no password, so it survived the reset + session-revoke as a standing login foothold. Add DeleteAllPasskeyCredentialsForUser and call it in the change-password remediation so every passkey is unbound alongside the session revoke. Removing zero rows is a successful no-op. Email-OTP remains the fallback factor, so this never locks anyone out; the user re-enrolls a passkey afterward if they want one.
This commit is contained in:
5 files changed
+45
No files matched your search
@@ -359,6 +359,17 @@ func (f *fakeRepo) DeletePasskeyCredential(_ context.Context, userID, id string)
|
|||||||
return ErrNotFound
|
return ErrNotFound
|
||||||
}
|
}
|
||||||
|
|
||||||
|
// DeleteAllPasskeyCredentialsForUser mirrors PGRepo: unbind every passkey the user holds,
|
||||||
|
// and removing zero is a successful no-op (never ErrNotFound).
|
||||||
|
func (f *fakeRepo) DeleteAllPasskeyCredentialsForUser(_ context.Context, userID string) error {
|
||||||
|
for id, c := range f.passkeyCreds {
|
||||||
|
if c.UserID == userID {
|
||||||
|
delete(f.passkeyCreds, id)
|
||||||
|
}
|
||||||
|
}
|
||||||
|
return nil
|
||||||
|
}
|
||||||
|
|
||||||
// fakePasskeyVerifier is the hermetic PasskeyVerifier: it performs no real attestation
|
// fakePasskeyVerifier is the hermetic PasskeyVerifier: it performs no real attestation
|
||||||
// crypto, so it exercises the enrollment STATE MACHINE (challenge persistence, consume,
|
// crypto, so it exercises the enrollment STATE MACHINE (challenge persistence, consume,
|
||||||
// conflict, audit) without go-webauthn. BeginRegistration returns a fixed options blob
|
// conflict, audit) without go-webauthn. BeginRegistration returns a fixed options blob
|
||||||
|
|||||||
@@ -218,6 +218,16 @@ func (a *API) handleChangePassword(w http.ResponseWriter, r *http.Request) {
|
|||||||
return
|
return
|
||||||
}
|
}
|
||||||
|
|
||||||
|
// Revoking sessions is not enough: a passkey needs no password, so one planted
|
||||||
|
// through a transiently-hijacked session would outlive the reset as a standing login
|
||||||
|
// foothold. A password change is a possible-compromise signal, so unbind every passkey
|
||||||
|
// as part of the same remediation. The user re-enrolls afterward if they want one; the
|
||||||
|
// email-OTP factor stays available in the meantime, so this never locks anyone out.
|
||||||
|
if err := a.Repo.DeleteAllPasskeyCredentialsForUser(r.Context(), u.ID); err != nil {
|
||||||
|
writeError(w, r, err)
|
||||||
|
return
|
||||||
|
}
|
||||||
|
|
||||||
a.audit(r, u.Username, "auth.password_change", "")
|
a.audit(r, u.Username, "auth.password_change", "")
|
||||||
writeJSON(w, http.StatusOK, map[string]any{"ok": true})
|
writeJSON(w, http.StatusOK, map[string]any{"ok": true})
|
||||||
}
|
}
|
||||||
|
|||||||
@@ -244,6 +244,10 @@ func TestHandleChangePasswordSuccess(t *testing.T) {
|
|||||||
// no felis_session cookie, so keep="" and every session of u1 is revoked — the
|
// no felis_session cookie, so keep="" and every session of u1 is revoked — the
|
||||||
// safe direction the handler documents.
|
// safe direction the handler documents.
|
||||||
repo.sessions["other-device"] = &fakeSession{userID: "u1", expiresAt: api.now().Add(time.Hour)}
|
repo.sessions["other-device"] = &fakeSession{userID: "u1", expiresAt: api.now().Add(time.Hour)}
|
||||||
|
// A bound passkey for u1: the change must unbind it too. A passkey planted through a
|
||||||
|
// hijacked session needs no password, so it would otherwise survive the reset as a
|
||||||
|
// standing login foothold.
|
||||||
|
repo.passkeyCreds["pk1"] = PasskeyCredential{ID: "pk1", UserID: "u1", CredentialID: "cred-1", PublicKey: "k"}
|
||||||
|
|
||||||
w := do(api.ExternalHandler(), "POST", "/api/v1/auth/change-password",
|
w := do(api.ExternalHandler(), "POST", "/api/v1/auth/change-password",
|
||||||
`{"current_password":"old-password","new_password":"brand-new-password"}`, jsonHeader)
|
`{"current_password":"old-password","new_password":"brand-new-password"}`, jsonHeader)
|
||||||
@@ -261,6 +265,9 @@ func TestHandleChangePasswordSuccess(t *testing.T) {
|
|||||||
if !repo.sessions["other-device"].revoked {
|
if !repo.sessions["other-device"].revoked {
|
||||||
t.Fatalf("other sessions should be revoked on a password change")
|
t.Fatalf("other sessions should be revoked on a password change")
|
||||||
}
|
}
|
||||||
|
if len(repo.passkeyCreds) != 0 {
|
||||||
|
t.Fatalf("password change left %d passkeys, want 0 — a planted passkey must not survive remediation", len(repo.passkeyCreds))
|
||||||
|
}
|
||||||
}
|
}
|
||||||
|
|
||||||
// TestHandleChangePasswordContentTypeGuard pins the defense-in-depth guard on the
|
// TestHandleChangePasswordContentTypeGuard pins the defense-in-depth guard on the
|
||||||
|
|||||||
@@ -997,3 +997,14 @@ func (p *PGRepo) DeletePasskeyCredential(ctx context.Context, userID, id string)
|
|||||||
}
|
}
|
||||||
return nil
|
return nil
|
||||||
}
|
}
|
||||||
|
|
||||||
|
// DeleteAllPasskeyCredentialsForUser unbinds every passkey a user holds. Unlike the
|
||||||
|
// single-credential delete this does NOT report ErrNotFound on zero rows: removing all of
|
||||||
|
// a user's passkeys when they have none is a successful no-op, since "the user holds no
|
||||||
|
// passkeys" is exactly the intended post-condition. The change-password flow calls it so a
|
||||||
|
// passkey planted through a transiently-hijacked session cannot survive the remediation.
|
||||||
|
func (p *PGRepo) DeleteAllPasskeyCredentialsForUser(ctx context.Context, userID string) error {
|
||||||
|
_, err := p.db.ExecContext(ctx,
|
||||||
|
`DELETE FROM webauthn_credentials WHERE user_id = $1`, userID)
|
||||||
|
return err
|
||||||
|
}
|
||||||
@@ -291,6 +291,12 @@ type Repo interface {
|
|||||||
// can only unbind their OWN credential. No matching (user, id) row → ErrNotFound,
|
// can only unbind their OWN credential. No matching (user, id) row → ErrNotFound,
|
||||||
// so a stale or cross-user id cannot silently no-op as success.
|
// so a stale or cross-user id cannot silently no-op as success.
|
||||||
DeletePasskeyCredential(ctx context.Context, userID, id string) error
|
DeletePasskeyCredential(ctx context.Context, userID, id string) error
|
||||||
|
// DeleteAllPasskeyCredentialsForUser unbinds every passkey a user holds. The
|
||||||
|
// change-password flow calls it so a passkey planted via a transiently-hijacked
|
||||||
|
// session does not survive the remediation (password reset + session revoke) as a
|
||||||
|
// standing login foothold. Removing zero rows is success, not an error — an account
|
||||||
|
// with no passkeys is the intended post-condition either way.
|
||||||
|
DeleteAllPasskeyCredentialsForUser(ctx context.Context, userID string) error
|
||||||
|
|
||||||
// ---- player game-login: username-collision reclaim (spec §B3) ----
|
// ---- player game-login: username-collision reclaim (spec §B3) ----
|
||||||
|
|
||||||
|
|||||||
Reference in new issue
Block a user