Increase entropy for MFA secret (#14290)

Co-authored-by: mattermod <mattermod@users.noreply.github.com>

model/utils.go

@@ -155,15 +155,20 @@ func NewRandomTeamName() string {
 	return teamName
 }
 
+// NewRandomString returns a random string of the given length.
+// The resulting entropy will be (5 * length) bits.
 func NewRandomString(length int) string {
-	var b bytes.Buffer
+	data := make([]byte, 1+(length*5/8))
-	str := make([]byte, length+8)
+	rand.Read(data)
-	rand.Read(str)
+	return encoding.EncodeToString(data)[:length]
-	encoder := base32.NewEncoder(encoding, &b)
+}
-	encoder.Write(str)
+
-	encoder.Close()
+// NewRandomBase32String returns a base32 encoded string of a random slice
-	b.Truncate(length) // removes the '==' padding
+// of bytes of the given size. The resulting entropy will be (8 * size) bits.
-	return b.String()
+func NewRandomBase32String(size int) string {
+	data := make([]byte, size)
+	rand.Read(data)
+	return base32.StdEncoding.EncodeToString(data)
 }
 
 // GetMillis is a convenience method to get milliseconds since epoch.

model/utils_test.go

@@ -5,6 +5,7 @@ package model
 
 import (
 	"bytes"
+	"encoding/base32"
 	"fmt"
 	"net/http"
 	"reflect"
@@ -25,8 +26,16 @@ func TestNewId(t *testing.T) {
 
 func TestRandomString(t *testing.T) {
 	for i := 0; i < 1000; i++ {
-		r := NewRandomString(32)
+		str := NewRandomString(i)
-		require.Len(t, r, 32)
+		require.Len(t, str, i)
+		require.NotContains(t, str, "=")
+	}
+}
+
+func TestRandomBase32String(t *testing.T) {
+	for i := 0; i < 1000; i++ {
+		str := NewRandomBase32String(i)
+		require.Len(t, str, base32.StdEncoding.EncodedLen(i))
 	}
 }
 

services/mfa/mfa.go

@@ -4,7 +4,6 @@
 package mfa
 
 import (
-	b32 "encoding/base32"
 	"fmt"
 	"net/http"
 	"net/url"
@@ -18,6 +17,7 @@ import (
 )
 
 const (
+	// This will result in 160 bits of entropy (base32 encoded), as recommended by rfc4226.
 	MFA_SECRET_SIZE = 20
 )
 
@@ -58,7 +58,7 @@ func (m *Mfa) GenerateSecret(user *model.User) (string, []byte, *model.AppError)
 
 	issuer := getIssuerFromUrl(*m.ConfigService.Config().ServiceSettings.SiteURL)
 
-	secret := b32.StdEncoding.EncodeToString([]byte(model.NewRandomString(MFA_SECRET_SIZE)))
+	secret := model.NewRandomBase32String(MFA_SECRET_SIZE)
 
 	authLink := fmt.Sprintf("otpauth://totp/%s:%s?secret=%s&issuer=%s", issuer, user.Email, secret, issuer)
 

services/mfa/mfa_test.go

@@ -4,7 +4,6 @@
 package mfa
 
 import (
-	b32 "encoding/base32"
 	"fmt"
 	"net/http"
 	"net/url"
@@ -37,7 +36,7 @@ func TestGenerateSecret(t *testing.T) {
 		mfa := New(wrongConfigService, nil)
 		_, _, err := mfa.GenerateSecret(user)
 		require.NotNil(t, err)
-		require.Equal(t, err.Id, "mfa.mfa_disabled.app_error")
+		require.Equal(t, "mfa.mfa_disabled.app_error", err.Id)
 	})
 
 	t.Run("fail on store action fail", func(t *testing.T) {
@@ -51,7 +50,7 @@ func TestGenerateSecret(t *testing.T) {
 		mfa := New(configService, &storeMock)
 		_, _, err := mfa.GenerateSecret(user)
 		require.NotNil(t, err)
-		require.Equal(t, err.Id, "mfa.generate_qr_code.save_secret.app_error")
+		require.Equal(t, "mfa.generate_qr_code.save_secret.app_error", err.Id)
 	})
 
 	t.Run("Successful generate secret", func(t *testing.T) {
@@ -94,7 +93,8 @@ func TestGetIssuerFromUrl(t *testing.T) {
 
 func TestActivate(t *testing.T) {
 	user := &model.User{Id: model.NewId(), Roles: "system_user"}
-	user.MfaSecret = b32.StdEncoding.EncodeToString([]byte(model.NewRandomString(MFA_SECRET_SIZE)))
+	user.MfaSecret = model.NewRandomBase32String(MFA_SECRET_SIZE)
+
 	token := dgoogauth.ComputeCode(user.MfaSecret, time.Now().UTC().Unix()/30)
 
 	config := model.Config{}
@@ -110,21 +110,21 @@ func TestActivate(t *testing.T) {
 		mfa := New(wrongConfigService, nil)
 		err := mfa.Activate(user, "not-important")
 		require.NotNil(t, err)
-		require.Equal(t, err.Id, "mfa.mfa_disabled.app_error")
+		require.Equal(t, "mfa.mfa_disabled.app_error", err.Id)
 	})
 
 	t.Run("fail on wrongly formatted token", func(t *testing.T) {
 		mfa := New(configService, nil)
 		err := mfa.Activate(user, "invalid-token")
 		require.NotNil(t, err)
-		require.Equal(t, err.Id, "mfa.activate.authenticate.app_error")
+		require.Equal(t, "mfa.activate.authenticate.app_error", err.Id)
 	})
 
 	t.Run("fail on invalid token", func(t *testing.T) {
 		mfa := New(configService, nil)
 		err := mfa.Activate(user, "000000")
 		require.NotNil(t, err)
-		require.Equal(t, err.Id, "mfa.activate.bad_token.app_error")
+		require.Equal(t, "mfa.activate.bad_token.app_error", err.Id)
 	})
 
 	t.Run("fail on store action fail", func(t *testing.T) {
@@ -138,7 +138,7 @@ func TestActivate(t *testing.T) {
 		mfa := New(configService, &storeMock)
 		err := mfa.Activate(user, fmt.Sprintf("%06d", token))
 		require.NotNil(t, err)
-		require.Equal(t, err.Id, "mfa.activate.save_active.app_error")
+		require.Equal(t, "mfa.activate.save_active.app_error", err.Id)
 	})
 
 	t.Run("Successful activate", func(t *testing.T) {
@@ -171,7 +171,7 @@ func TestDeactivate(t *testing.T) {
 		mfa := New(wrongConfigService, nil)
 		err := mfa.Deactivate(user.Id)
 		require.NotNil(t, err)
-		require.Equal(t, err.Id, "mfa.mfa_disabled.app_error")
+		require.Equal(t, "mfa.mfa_disabled.app_error", err.Id)
 	})
 
 	t.Run("fail on store UpdateMfaActive action fail", func(t *testing.T) {
@@ -188,7 +188,7 @@ func TestDeactivate(t *testing.T) {
 		mfa := New(configService, &storeMock)
 		err := mfa.Deactivate(user.Id)
 		require.NotNil(t, err)
-		require.Equal(t, err.Id, "mfa.deactivate.save_active.app_error")
+		require.Equal(t, "mfa.deactivate.save_active.app_error", err.Id)
 	})
 
 	t.Run("fail on store UpdateMfaSecret action fail", func(t *testing.T) {
@@ -205,7 +205,7 @@ func TestDeactivate(t *testing.T) {
 		mfa := New(configService, &storeMock)
 		err := mfa.Deactivate(user.Id)
 		require.NotNil(t, err)
-		require.Equal(t, err.Id, "mfa.deactivate.save_secret.app_error")
+		require.Equal(t, "mfa.deactivate.save_secret.app_error", err.Id)
 	})
 
 	t.Run("Successful deactivate", func(t *testing.T) {
@@ -226,7 +226,7 @@ func TestDeactivate(t *testing.T) {
 }
 
 func TestValidateToken(t *testing.T) {
-	secret := b32.StdEncoding.EncodeToString([]byte(model.NewRandomString(MFA_SECRET_SIZE)))
+	secret := model.NewRandomBase32String(MFA_SECRET_SIZE)
 	token := dgoogauth.ComputeCode(secret, time.Now().UTC().Unix()/30)
 
 	config := model.Config{}
@@ -243,7 +243,7 @@ func TestValidateToken(t *testing.T) {
 		ok, err := mfa.ValidateToken(secret, fmt.Sprintf("%06d", token))
 		require.NotNil(t, err)
 		require.False(t, ok)
-		require.Equal(t, err.Id, "mfa.mfa_disabled.app_error")
+		require.Equal(t, "mfa.mfa_disabled.app_error", err.Id)
 	})
 
 	t.Run("fail on wrongly formatted token", func(t *testing.T) {
@@ -251,7 +251,7 @@ func TestValidateToken(t *testing.T) {
 		ok, err := mfa.ValidateToken(secret, "invalid-token")
 		require.NotNil(t, err)
 		require.False(t, ok)
-		require.Equal(t, err.Id, "mfa.validate_token.authenticate.app_error")
+		require.Equal(t, "mfa.validate_token.authenticate.app_error", err.Id)
 	})
 
 	t.Run("fail on invalid token", func(t *testing.T) {