From ad68af10dfcf9d973349f89d025cb5729771a0bf Mon Sep 17 00:00:00 2001 From: Claudio Costa Date: Wed, 22 Apr 2020 18:46:16 +0200 Subject: [PATCH] Increase entropy for MFA secret (#14290) Co-authored-by: mattermod --- model/utils.go | 21 +++++++++++++-------- model/utils_test.go | 13 +++++++++++-- services/mfa/mfa.go | 4 ++-- services/mfa/mfa_test.go | 28 ++++++++++++++-------------- 4 files changed, 40 insertions(+), 26 deletions(-) diff --git a/model/utils.go b/model/utils.go index aee04a068e..17617752a0 100644 --- a/model/utils.go +++ b/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 - str := make([]byte, length+8) - rand.Read(str) - encoder := base32.NewEncoder(encoding, &b) - encoder.Write(str) - encoder.Close() - b.Truncate(length) // removes the '==' padding - return b.String() + data := make([]byte, 1+(length*5/8)) + rand.Read(data) + return encoding.EncodeToString(data)[:length] +} + +// NewRandomBase32String returns a base32 encoded string of a random slice +// of bytes of the given size. The resulting entropy will be (8 * size) bits. +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. diff --git a/model/utils_test.go b/model/utils_test.go index ae49acba9d..3fec8f8939 100644 --- a/model/utils_test.go +++ b/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) - require.Len(t, r, 32) + str := NewRandomString(i) + 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)) } } diff --git a/services/mfa/mfa.go b/services/mfa/mfa.go index eb7076bf28..326e965bc8 100644 --- a/services/mfa/mfa.go +++ b/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) diff --git a/services/mfa/mfa_test.go b/services/mfa/mfa_test.go index dbc21a488f..79c2cd28bb 100644 --- a/services/mfa/mfa_test.go +++ b/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) {