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
b565efe9d1
commit
ecdb14aaca
2 changed files with 62 additions and 6 deletions
|
|
@ -271,19 +271,27 @@ func (s *Service) GetPassword(ctx context.Context, userID int64) (domain.Passwor
|
||||||
return defaultPasswordSettings(), nil
|
return defaultPasswordSettings(), nil
|
||||||
}
|
}
|
||||||
settings = normalizePasswordSettings(settings)
|
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)
|
secret, b, err := makeSRPChallenge(settings.SRPVerifier)
|
||||||
if err != nil {
|
if err != nil {
|
||||||
return domain.PasswordSettings{}, err
|
return domain.PasswordSettings{}, err
|
||||||
}
|
}
|
||||||
settings.SRPBSecret = secret
|
settings.SRPBSecret = secret
|
||||||
settings.SRPB = b
|
settings.SRPB = b
|
||||||
if settings.SRPID == 0 {
|
|
||||||
settings.SRPID, err = randomInt64()
|
settings.SRPID, err = randomInt64()
|
||||||
if err != nil {
|
if err != nil {
|
||||||
return domain.PasswordSettings{}, err
|
return domain.PasswordSettings{}, err
|
||||||
}
|
}
|
||||||
}
|
|
||||||
if err := s.passwords.Save(ctx, userID, settings); err != nil {
|
if err := s.passwords.Save(ctx, userID, settings); err != nil {
|
||||||
return domain.PasswordSettings{}, err
|
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) {
|
func TestRecoverPasswordClearsTwoFactorPassword(t *testing.T) {
|
||||||
ctx := context.Background()
|
ctx := context.Background()
|
||||||
const userID int64 = 1002
|
const userID int64 = 1002
|
||||||
|
|
|
||||||
Loading…
Add table
Add a link
Reference in a new issue