From adedd1ea4558876dd21a33de627afefed252d0f7 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Julio=20L=C3=B3pez?= <1953782+julio-lopez@users.noreply.github.com> Date: Fri, 7 Jun 2024 13:00:44 -0700 Subject: [PATCH] fix(general): avoid panic on computing password hash error (#3907) Rationale: this code path is primarily executed from the server. A potential error, say from a corrupt, unsupported or otherwise invalid user profile should not cause the server to panic (and crash). It is possible for `computePasswordHash` to return an error, not just an impossibility. Test refactoring: - use 'require' in user profile tests; - move test case to TestBadPasswordHashVersion; - update comments in test. --- internal/auth/authn_repo.go | 9 +++- internal/user/user_profile.go | 6 +-- internal/user/user_profile_pw_hash.go | 8 ++-- internal/user/user_profile_test.go | 68 +++++++++++++++++---------- 4 files changed, 57 insertions(+), 34 deletions(-) diff --git a/internal/auth/authn_repo.go b/internal/auth/authn_repo.go index 9df1373dd..7ac7ceae1 100644 --- a/internal/auth/authn_repo.go +++ b/internal/auth/authn_repo.go @@ -51,7 +51,14 @@ func (ac *repositoryUserAuthenticator) IsValid(ctx context.Context, rep repo.Rep // IsValidPassword can be safely called on nil and the call will take as much time as for a valid user // thus not revealing anything about whether the user exists. - return ac.userProfiles[username].IsValidPassword(password) + valid, err := ac.userProfiles[username].IsValidPassword(password) + if err != nil { + log(ctx).Debugf("password error for user '%s': %v", username, err) + + return false + } + + return valid } func (ac *repositoryUserAuthenticator) Refresh(ctx context.Context) error { diff --git a/internal/user/user_profile.go b/internal/user/user_profile.go index be0cedd56..150fa9ccc 100644 --- a/internal/user/user_profile.go +++ b/internal/user/user_profile.go @@ -38,7 +38,7 @@ func (p *Profile) SetPassword(password string) error { } // IsValidPassword determines whether the password is valid for a given user. -func (p *Profile) IsValidPassword(password string) bool { +func (p *Profile) IsValidPassword(password string) (bool, error) { var invalidProfile bool var passwordHashAlgorithm string @@ -59,9 +59,9 @@ func (p *Profile) IsValidPassword(password string) bool { // if the user profile is invalid, either a non-existing user name or password // hash version, then return false but use the same amount of time as when we // compare against valid user to avoid revealing whether the user account exists. - isValidPassword(password, dummyHashThatNeverMatchesAnyPassword, algorithms[rand.Intn(len(algorithms))]) //nolint:gosec + _, err := isValidPassword(password, dummyHashThatNeverMatchesAnyPassword, algorithms[rand.Intn(len(algorithms))]) //nolint:gosec - return false + return false, err } return isValidPassword(password, p.PasswordHash, passwordHashAlgorithm) diff --git a/internal/user/user_profile_pw_hash.go b/internal/user/user_profile_pw_hash.go index 78e8abe28..334e839c7 100644 --- a/internal/user/user_profile_pw_hash.go +++ b/internal/user/user_profile_pw_hash.go @@ -50,17 +50,17 @@ func computePasswordHash(password string, salt []byte, keyDerivationAlgorithm st return payload, nil } -func isValidPassword(password string, hashedPassword []byte, keyDerivationAlgorithm string) bool { +func isValidPassword(password string, hashedPassword []byte, keyDerivationAlgorithm string) (bool, error) { if len(hashedPassword) != passwordHashSaltLength+passwordHashLength { - return false + return false, nil } salt := hashedPassword[0:passwordHashSaltLength] h, err := computePasswordHash(password, salt, keyDerivationAlgorithm) if err != nil { - panic(err) + return false, err } - return subtle.ConstantTimeCompare(h, hashedPassword) != 0 + return subtle.ConstantTimeCompare(h, hashedPassword) != 0, nil } diff --git a/internal/user/user_profile_test.go b/internal/user/user_profile_test.go index 476189a0c..0f84e7826 100644 --- a/internal/user/user_profile_test.go +++ b/internal/user/user_profile_test.go @@ -3,6 +3,8 @@ import ( "testing" + "github.com/stretchr/testify/require" + "github.com/kopia/kopia/internal/user" ) @@ -11,25 +13,22 @@ func TestUserProfile(t *testing.T) { PasswordHashVersion: user.ScryptHashVersion, } - if p.IsValidPassword("bar") { - t.Fatalf("password unexpectedly valid!") - } + isValid, err := p.IsValidPassword("bar") + + require.False(t, isValid, "password unexpectedly valid!") + require.NoError(t, err) p.SetPassword("foo") - if !p.IsValidPassword("foo") { - t.Fatalf("password not valid!") - } + isValid, err = p.IsValidPassword("foo") - if p.IsValidPassword("bar") { - t.Fatalf("password unexpectedly valid!") - } + require.True(t, isValid, "password not valid!") + require.NoError(t, err) - // Different key derivation algorithm besides the original should fail - p.PasswordHashVersion = user.Pbkdf2HashVersion - if p.IsValidPassword("foo") { - t.Fatalf("password unexpectedly valid!") - } + isValid, err = p.IsValidPassword("bar") + + require.False(t, isValid, "password unexpectedly valid!") + require.NoError(t, err) } func TestBadPasswordHashVersion(t *testing.T) { @@ -37,21 +36,37 @@ func TestBadPasswordHashVersion(t *testing.T) { p := &user.Profile{ PasswordHashVersion: user.ScryptHashVersion, } + p.SetPassword("foo") - // Assume the key derivation algorithm is bad. This will cause - // a panic when validating + + isValid, err := p.IsValidPassword("foo") + + require.True(t, isValid, "password not valid!") + require.NoError(t, err) + + // A password hashing algorithm different from the original should fail + p.PasswordHashVersion = user.Pbkdf2HashVersion + isValid, err = p.IsValidPassword("foo") + + require.False(t, isValid, "password unexpectedly valid!") + require.NoError(t, err) + + // Invalid password hashing algorithm p.PasswordHashVersion = 0 - if p.IsValidPassword("foo") { - t.Fatalf("password unexpectedly valid!") - } + + isValid, err = p.IsValidPassword("foo") + + require.False(t, isValid, "password unexpectedly valid!") + require.NoError(t, err) } func TestNilUserProfile(t *testing.T) { var p *user.Profile - if p.IsValidPassword("bar") { - t.Fatalf("password unexpectedly valid!") - } + isValid, err := p.IsValidPassword("bar") + + require.False(t, isValid, "password unexpectedly valid!") + require.NoError(t, err) } func TestInvalidPasswordHash(t *testing.T) { @@ -63,10 +78,11 @@ func TestInvalidPasswordHash(t *testing.T) { for _, tc := range cases { p := &user.Profile{ PasswordHash: tc, - PasswordHashVersion: 1, - } - if p.IsValidPassword("some-password") { - t.Fatalf("password unexpectedly valid for %v", tc) + PasswordHashVersion: user.ScryptHashVersion, } + isValid, err := p.IsValidPassword("some-password") + + require.False(t, isValid, "password unexpectedly valid for %v", tc) + require.NoError(t, err) } }