[MM-59069] Make sure OTP are actual One Time Password (#28074)

Automatic Merge

server/platform/shared/mfa/mfa.go

@@ -11,6 +11,7 @@ import (
 	"strings"
 
 	"github.com/dgryski/dgoogauth"
+	"github.com/mattermost/mattermost/server/public/model"
 	"github.com/mattermost/rsc/qr"
 	"github.com/pkg/errors"
 )
@@ -26,6 +27,8 @@ const (
 type Store interface {
 	UpdateMfaActive(userId string, active bool) error
 	UpdateMfaSecret(userId, secret string) error
+	StoreMfaUsedTimestamps(userId string, ts []int) error
+	GetMfaUsedTimestamps(userId string) ([]int, error)
 }
 
 type MFA struct {
@@ -120,11 +123,17 @@ func (m *MFA) Deactivate(userId string) error {
 }
 
 // Validate the provide token using the secret provided
-func (m *MFA) ValidateToken(secret, token string) (bool, error) {
+func (m *MFA) ValidateToken(user *model.User, token string) (bool, error) {
+	usedTs, err := m.store.GetMfaUsedTimestamps(user.Id)
+	if err != nil {
+		return false, errors.Wrap(err, "unable to retrieve the DisallowReuse slice")
+	}
+
 	otpConfig := &dgoogauth.OTPConfig{
-		Secret:      secret,
-		WindowSize:  3,
-		HotpCounter: 0,
+		Secret:        user.MfaSecret,
+		WindowSize:    3,
+		HotpCounter:   0,
+		DisallowReuse: usedTs,
 	}
 
 	trimmedToken := strings.TrimSpace(token)
@@ -132,6 +141,14 @@ func (m *MFA) ValidateToken(secret, token string) (bool, error) {
 	if err != nil {
 		return false, errors.Wrap(err, "unable to parse the token")
 	}
+	if !ok {
+		return false, nil
+	}
 
-	return ok, nil
+	err = m.store.StoreMfaUsedTimestamps(user.Id, otpConfig.DisallowReuse)
+	if err != nil {
+		return true, errors.Wrap(err, "unable to store the DisallowReuse slice")
+	}
+
+	return true, nil
 }

server/platform/shared/mfa/mfa_test.go

@@ -15,6 +15,7 @@ import (
 	"github.com/stretchr/testify/assert"
 	"github.com/stretchr/testify/require"
 
+	"github.com/mattermost/mattermost/server/public/model"
 	"github.com/mattermost/mattermost/server/public/plugin/plugintest/mock"
 	"github.com/mattermost/mattermost/server/v8/channels/store/storetest/mocks"
 )
@@ -156,12 +157,49 @@ func TestDeactivate(t *testing.T) {
 
 func TestValidateToken(t *testing.T) {
 	t.Run("fail on wrongly formatted token", func(t *testing.T) {
+		id := model.NewId()
 		secret := newRandomBase32String(mfaSecretSize)
-		ok, err := New(nil).ValidateToken(secret, "invalid-token")
+		u := &model.User{Id: id, MfaSecret: secret}
+
+		usMock := mocks.UserStore{}
+		usMock.On("GetMfaUsedTimestamps", u.Id).Return([]int{}, nil).Once()
+		ok, err := New(&usMock).ValidateToken(u, "invalid-token")
 		require.Error(t, err)
 		require.False(t, ok)
 		require.Contains(t, err.Error(), "unable to parse the token")
 	})
+
+	t.Run("successful validation", func(t *testing.T) {
+		id := model.NewId()
+		secret := newRandomBase32String(mfaSecretSize)
+		u := &model.User{Id: id, MfaSecret: secret}
+
+		code := fmt.Sprintf("%06d", dgoogauth.ComputeCode(secret, time.Now().UTC().Unix()/30))
+
+		usMock := mocks.UserStore{}
+		usMock.On("GetMfaUsedTimestamps", u.Id).Return([]int{}, nil).Once()
+		usMock.On("StoreMfaUsedTimestamps", u.Id, mock.AnythingOfType("[]int")).Return(nil).Once()
+
+		ok, err := New(&usMock).ValidateToken(u, code)
+		require.NoError(t, err)
+		require.True(t, ok)
+	})
+
+	t.Run("disallow reuse of totp", func(t *testing.T) {
+		id := model.NewId()
+		secret := newRandomBase32String(mfaSecretSize)
+		u := &model.User{Id: id, MfaSecret: secret}
+
+		t0 := time.Now().UTC().Unix() / 30
+		code := fmt.Sprintf("%06d", dgoogauth.ComputeCode(secret, t0))
+
+		usMock := mocks.UserStore{}
+		usMock.On("GetMfaUsedTimestamps", u.Id).Return([]int{int(t0)}, nil).Once()
+
+		ok, err := New(&usMock).ValidateToken(u, code)
+		require.False(t, ok)
+		require.NoError(t, err)
+	})
 }
 
 func TestRandomBase32String(t *testing.T) {