diff --git a/lib/auth/methods.go b/lib/auth/methods.go index d5b7c7a58d72f..69fa8a41bfb7d 100644 --- a/lib/auth/methods.go +++ b/lib/auth/methods.go @@ -326,6 +326,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 20ee2b1c9b0bc..152b651a4ec8f 100644 --- a/lib/auth/password.go +++ b/lib/auth/password.go @@ -125,10 +125,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 10edcb718ae4c..48cb1d44202b9 100644 --- a/lib/auth/password_test.go +++ b/lib/auth/password_test.go @@ -27,6 +27,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" "github.com/gravitational/teleport" @@ -258,6 +259,52 @@ 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() + + server := newTestTLSServer(t) + mfa := configureForMFA(t, server) + authServer := server.Auth() + ctx := context.Background() + + username := mfa.User + newPass := []byte("capybarasarecool123") + + userClient, err := server.NewClient(TestUser(username)) + require.NoError(t, err) + defer userClient.Close() + + // Acquire and solve an MFA challenge. + mfaChallenge, err := userClient.CreateAuthenticateChallenge(ctx, &proto.CreateAuthenticateChallengeRequest{ + Request: &proto.CreateAuthenticateChallengeRequest_ContextUser{ + ContextUser: &proto.ContextUser{}, + }, + }) + 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(), + } + 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? + assert.Error(t, authServer.checkPasswordWOToken(username, newPass), "password was changed") +} + func TestChangeUserAuthentication(t *testing.T) { t.Parallel()