diff --git a/internal/app/account/service.go b/internal/app/account/service.go index e6c3dcfa..62486b6d 100644 --- a/internal/app/account/service.go +++ b/internal/app/account/service.go @@ -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 diff --git a/internal/app/account/service_test.go b/internal/app/account/service_test.go index 1689cf9e..f1eb7ef5 100644 --- a/internal/app/account/service_test.go +++ b/internal/app/account/service_test.go @@ -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