From df80753bcb07824e91d37104527135d9a0c2cf7e Mon Sep 17 00:00:00 2001 From: Bartosz Leper Date: Wed, 14 Feb 2024 12:19:32 +0100 Subject: [PATCH 1/2] Always verify the old password when changing it --- lib/auth/methods.go | 5 +++++ lib/auth/password.go | 6 ++---- lib/auth/password_test.go | 45 +++++++++++++++++++++++++++++++++++++++ 3 files changed, 52 insertions(+), 4 deletions(-) diff --git a/lib/auth/methods.go b/lib/auth/methods.go index 4925b790d68e6..0d0ac0013b46b 100644 --- a/lib/auth/methods.go +++ b/lib/auth/methods.go @@ -371,6 +371,11 @@ func (a *Server) authenticateUserInternal(ctx context.Context, req AuthenticateU authErr = authenticateHeadlessError case req.Webauthn != nil: authenticateFn = func() (*types.MFADevice, error) { + if req.Pass != nil { + if err = a.checkPasswordWOToken(user, req.Pass.Password); err != nil { + return nil, trace.Wrap(err) + } + } mfaResponse := &proto.MFAAuthenticateResponse{ Response: &proto.MFAAuthenticateResponse_Webauthn{ Webauthn: wantypes.CredentialAssertionResponseToProto(req.Webauthn), diff --git a/lib/auth/password.go b/lib/auth/password.go index cd71bf064b6c7..6501a88b28c3e 100644 --- a/lib/auth/password.go +++ b/lib/auth/password.go @@ -129,10 +129,8 @@ func (a *Server) ChangePassword(ctx context.Context, req *proto.ChangePasswordRe Username: user, Webauthn: wantypes.CredentialAssertionResponseFromProto(req.Webauthn), } - if len(req.OldPassword) > 0 { - authReq.Pass = &PassCreds{ - Password: req.OldPassword, - } + authReq.Pass = &PassCreds{ + Password: req.OldPassword, } if req.SecondFactorToken != "" { authReq.OTP = &OTPCreds{ diff --git a/lib/auth/password_test.go b/lib/auth/password_test.go index 96c9d89945bee..458bbd76e8a59 100644 --- a/lib/auth/password_test.go +++ b/lib/auth/password_test.go @@ -35,6 +35,7 @@ import ( "github.com/gravitational/teleport" "github.com/gravitational/teleport/api/client/proto" "github.com/gravitational/teleport/api/constants" + mfav1 "github.com/gravitational/teleport/api/gen/proto/go/teleport/mfa/v1" "github.com/gravitational/teleport/api/types" apievents "github.com/gravitational/teleport/api/types/events" wanpb "github.com/gravitational/teleport/api/types/webauthn" @@ -294,6 +295,50 @@ func TestServer_ChangePassword(t *testing.T) { } } +// This test asserts that an attacker is unable to change password without +// providing the old one if they take over a user's web session and use a +// different type of WebAuthn challenge that would be normally requested by the +// Web UI. This is a regression test for +// https://github.com/gravitational/teleport-private/issues/1369. +func TestServer_ChangePassword_FailsWithoutOldPassword(t *testing.T) { + t.Parallel() + + srv := newTestTLSServer(t) + mfa := configureForMFA(t, srv) + authServer := srv.Auth() + ctx := context.Background() + + username := mfa.User + newPass := []byte("capybarasarecool123") + + clt, err := srv.NewClient(TestUser(username)) + require.NoError(t, err) + + // Acquire and solve an MFA challenge. + mfaChallenge, err := clt.CreateAuthenticateChallenge(ctx, &proto.CreateAuthenticateChallengeRequest{ + Request: &proto.CreateAuthenticateChallengeRequest_ContextUser{ + ContextUser: &proto.ContextUser{}, + }, + ChallengeExtensions: &mfav1.ChallengeExtensions{ + AllowReuse: mfav1.ChallengeAllowReuse_CHALLENGE_ALLOW_REUSE_NO, + }, + }) + require.NoError(t, err, "creating challenge") + mfaResp, err := mfa.WebDev.SolveAuthn(mfaChallenge) + require.NoError(t, err, "solving challenge with device") + + // Change password. + req := &proto.ChangePasswordRequest{ + User: username, + NewPassword: newPass, + Webauthn: mfaResp.GetWebauthn(), + } + require.Error(t, authServer.ChangePassword(ctx, req), "changing password") + + // Did the password change take effect? + require.Error(t, authServer.checkPasswordWOToken(username, newPass), "password was changed") +} + func TestChangeUserAuthentication(t *testing.T) { t.Parallel() From d793b7700c53e036fe003adf0a4866efc3404d54 Mon Sep 17 00:00:00 2001 From: Bartosz Leper Date: Thu, 15 Feb 2024 12:11:46 +0100 Subject: [PATCH 2/2] review --- lib/auth/password_test.go | 20 +++++++++++++------- 1 file changed, 13 insertions(+), 7 deletions(-) diff --git a/lib/auth/password_test.go b/lib/auth/password_test.go index 458bbd76e8a59..4da9cd15f15e6 100644 --- a/lib/auth/password_test.go +++ b/lib/auth/password_test.go @@ -29,6 +29,7 @@ import ( "github.com/gravitational/trace" "github.com/jonboulle/clockwork" "github.com/pquerna/otp/totp" + "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" "golang.org/x/crypto/bcrypt" @@ -303,19 +304,20 @@ func TestServer_ChangePassword(t *testing.T) { func TestServer_ChangePassword_FailsWithoutOldPassword(t *testing.T) { t.Parallel() - srv := newTestTLSServer(t) - mfa := configureForMFA(t, srv) - authServer := srv.Auth() + server := newTestTLSServer(t) + mfa := configureForMFA(t, server) + authServer := server.Auth() ctx := context.Background() username := mfa.User newPass := []byte("capybarasarecool123") - clt, err := srv.NewClient(TestUser(username)) + userClient, err := server.NewClient(TestUser(username)) require.NoError(t, err) + defer userClient.Close() // Acquire and solve an MFA challenge. - mfaChallenge, err := clt.CreateAuthenticateChallenge(ctx, &proto.CreateAuthenticateChallengeRequest{ + mfaChallenge, err := userClient.CreateAuthenticateChallenge(ctx, &proto.CreateAuthenticateChallengeRequest{ Request: &proto.CreateAuthenticateChallengeRequest_ContextUser{ ContextUser: &proto.ContextUser{}, }, @@ -333,10 +335,14 @@ func TestServer_ChangePassword_FailsWithoutOldPassword(t *testing.T) { NewPassword: newPass, Webauthn: mfaResp.GetWebauthn(), } - require.Error(t, authServer.ChangePassword(ctx, req), "changing password") + err = authServer.ChangePassword(ctx, req) + assert.True(t, + trace.IsAccessDenied(err), + "ChangePassword error mismatch, want=AccessDenied, got=%v (%T)", + err, trace.Unwrap(err)) // Did the password change take effect? - require.Error(t, authServer.checkPasswordWOToken(username, newPass), "password was changed") + assert.Error(t, authServer.checkPasswordWOToken(username, newPass), "password was changed") } func TestChangeUserAuthentication(t *testing.T) {