From 89a425c4489b1c6cf7bd9d4e2ce121501f5e942c Mon Sep 17 00:00:00 2001
From: Bartosz Leper <bartosz.leper@goteleport.com>
Date: Wed, 6 Mar 2024 09:27:27 +0100
Subject: [PATCH] [v15] Always verify the old password when changing it
 (#38962)

* Always verify the old password when changing it

* review
---
 lib/auth/methods.go       |  5 ++++
 lib/auth/password.go      |  6 ++---
 lib/auth/password_test.go | 51 +++++++++++++++++++++++++++++++++++++++
 3 files changed, 58 insertions(+), 4 deletions(-)

diff --git a/lib/auth/methods.go b/lib/auth/methods.go
index 7df551d006203..55d50075da909 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..4da9cd15f15e6 100644
--- a/lib/auth/password_test.go
+++ b/lib/auth/password_test.go
@@ -29,12 +29,14 @@ 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"
 
 	"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 +296,55 @@ 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{},
+		},
+		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(),
+	}
+	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()