account: stop rotating the SRP challenge on every getPassword read
GetPassword minted a brand-new random SRP server secret and B on every call while only ever assigning SRPID once (when zero). Two account.getPassword calls in a row -- e.g. a settings screen refreshing state, then the transfer- ownership dialog's own cloudPassword().reload() moments later -- silently invalidated each other's B with no signal the client could detect (SRPID unchanged), so a password check built from the first response's B failed with PASSWORD_HASH_INVALID even though the typed password was correct. The challenge now stays stable across reads and only rotates when it's missing entirely; UpdatePasswordSettings/RecoverPassword already mint their own fresh challenge whenever the password actually changes. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
This commit is contained in:
parent
1f6f25074a
commit
6435406690
2 changed files with 62 additions and 6 deletions
|
|
@ -271,18 +271,26 @@ func (s *Service) GetPassword(ctx context.Context, userID int64) (domain.Passwor
|
|||
return defaultPasswordSettings(), nil
|
||||
}
|
||||
settings = normalizePasswordSettings(settings)
|
||||
if settings.HasPassword {
|
||||
// Only mint a fresh SRP challenge (secret/B/SRPID together) when none is
|
||||
// outstanding yet. Regenerating B on every read while leaving SRPID
|
||||
// untouched let any two account.getPassword calls silently invalidate each
|
||||
// other's B without a signal the client could detect (SRPID unchanged) --
|
||||
// a client that fetched the password state, had a second screen/dialog
|
||||
// refresh it again, then submitted against the first B it saw got a false
|
||||
// PASSWORD_HASH_INVALID even with the correct password. The challenge must
|
||||
// stay stable across reads and only rotate when it's actually consumed by
|
||||
// a password change (UpdatePasswordSettings/RecoverPassword already mint
|
||||
// their own fresh challenge there).
|
||||
if settings.HasPassword && (len(settings.SRPBSecret) == 0 || len(settings.SRPB) == 0 || settings.SRPID == 0) {
|
||||
secret, b, err := makeSRPChallenge(settings.SRPVerifier)
|
||||
if err != nil {
|
||||
return domain.PasswordSettings{}, err
|
||||
}
|
||||
settings.SRPBSecret = secret
|
||||
settings.SRPB = b
|
||||
if settings.SRPID == 0 {
|
||||
settings.SRPID, err = randomInt64()
|
||||
if err != nil {
|
||||
return domain.PasswordSettings{}, err
|
||||
}
|
||||
settings.SRPID, err = randomInt64()
|
||||
if err != nil {
|
||||
return domain.PasswordSettings{}, err
|
||||
}
|
||||
if err := s.passwords.Save(ctx, userID, settings); err != nil {
|
||||
return domain.PasswordSettings{}, err
|
||||
|
|
|
|||
|
|
@ -68,6 +68,54 @@ func TestPasswordSRPRoundTrip(t *testing.T) {
|
|||
}
|
||||
}
|
||||
|
||||
// TestGetPasswordChallengeStableAcrossReads guards against a real production
|
||||
// bug: GetPassword used to mint a brand-new random SRP server secret/B on
|
||||
// every single call while leaving SRPID untouched. Two account.getPassword
|
||||
// calls in a row (e.g. a settings screen and, moments later, the transfer-
|
||||
// ownership dialog's own cloudPassword().reload()) would silently invalidate
|
||||
// each other's B with no signal the client could detect (SRPID unchanged),
|
||||
// so a password check built from the first response's B failed with
|
||||
// PASSWORD_HASH_INVALID even though the typed password was correct. The
|
||||
// challenge must stay identical across reads until a real password change
|
||||
// consumes it.
|
||||
func TestGetPasswordChallengeStableAcrossReads(t *testing.T) {
|
||||
ctx := context.Background()
|
||||
const userID int64 = 1003
|
||||
svc := NewService(memory.NewPasswordStore())
|
||||
|
||||
initial, err := svc.GetPassword(ctx, userID)
|
||||
if err != nil {
|
||||
t.Fatalf("GetPassword initial: %v", err)
|
||||
}
|
||||
algo := initial.NewAlgo
|
||||
algo.Salt1 = append(append([]byte(nil), algo.Salt1...), bytes.Repeat([]byte{0x11}, 32)...)
|
||||
if err := svc.UpdatePasswordSettings(ctx, userID, domain.PasswordCheck{Empty: true}, domain.PasswordInputSettings{
|
||||
NewAlgo: &algo,
|
||||
NewPasswordHash: verifierForPassword(algo, []byte("correct horse")),
|
||||
}); err != nil {
|
||||
t.Fatalf("UpdatePasswordSettings set password: %v", err)
|
||||
}
|
||||
|
||||
first, err := svc.GetPassword(ctx, userID)
|
||||
if err != nil {
|
||||
t.Fatalf("GetPassword first: %v", err)
|
||||
}
|
||||
// Simulate a second, unrelated screen/dialog refreshing the same cloud
|
||||
// password state before the user submits their check.
|
||||
second, err := svc.GetPassword(ctx, userID)
|
||||
if err != nil {
|
||||
t.Fatalf("GetPassword second: %v", err)
|
||||
}
|
||||
if first.SRPID != second.SRPID || !bytes.Equal(first.SRPB, second.SRPB) {
|
||||
t.Fatalf("challenge changed across reads: first=%+v second=%+v, want identical SRPID/SRPB", first, second)
|
||||
}
|
||||
|
||||
check := clientPasswordCheck(t, first, []byte("correct horse"))
|
||||
if err := svc.CheckPassword(ctx, userID, check); err != nil {
|
||||
t.Fatalf("CheckPassword against first-read challenge after an intervening read: %v", err)
|
||||
}
|
||||
}
|
||||
|
||||
func TestRecoverPasswordClearsTwoFactorPassword(t *testing.T) {
|
||||
ctx := context.Background()
|
||||
const userID int64 = 1002
|
||||
|
|
|
|||
Loading…
Add table
Add a link
Reference in a new issue