From 49dd9067535538771ef13623ed1dd9698a4a2151 Mon Sep 17 00:00:00 2001
From: wxiaoguang <wxiaoguang@gmail.com>
Date: Wed, 26 Jan 2022 12:10:10 +0800
Subject: [PATCH] Use base32 for 2FA scratch token (#18384)
* Use base32 for 2FA scratch token
* rename Secure* to Crypto*, add comments
---
models/auth/twofactor.go | 8 ++++++--
models/migrations/v71.go | 2 +-
models/migrations/v85.go | 2 +-
models/token.go | 2 +-
models/user/user.go | 2 +-
modules/generate/generate.go | 2 +-
modules/secret/secret.go | 2 +-
modules/util/util.go | 36 ++++++++++++++++++------------------
modules/util/util_test.go | 18 +++++++++---------
routers/web/auth/openid.go | 2 +-
routers/web/repo/setting.go | 2 +-
11 files changed, 41 insertions(+), 37 deletions(-)
diff --git a/models/auth/twofactor.go b/models/auth/twofactor.go
index 883e6ce01c..c5bd972f91 100644
--- a/models/auth/twofactor.go
+++ b/models/auth/twofactor.go
@@ -8,6 +8,7 @@ import (
"crypto/md5"
"crypto/sha256"
"crypto/subtle"
+ "encoding/base32"
"encoding/base64"
"fmt"
@@ -58,11 +59,14 @@ func init() {
// GenerateScratchToken recreates the scratch token the user is using.
func (t *TwoFactor) GenerateScratchToken() (string, error) {
- token, err := util.RandomString(8)
+ tokenBytes, err := util.CryptoRandomBytes(6)
if err != nil {
return "", err
}
- t.ScratchSalt, _ = util.RandomString(10)
+ // these chars are specially chosen, avoid ambiguous chars like `0`, `O`, `1`, `I`.
+ const base32Chars = "ABCDEFGHJKLMNPQRSTUVWXYZ23456789"
+ token := base32.NewEncoding(base32Chars).WithPadding(base32.NoPadding).EncodeToString(tokenBytes)
+ t.ScratchSalt, _ = util.CryptoRandomString(10)
t.ScratchHash = HashToken(token, t.ScratchSalt)
return token, nil
}
diff --git a/models/migrations/v71.go b/models/migrations/v71.go
index e4ed46a21a..163ec3ee5f 100644
--- a/models/migrations/v71.go
+++ b/models/migrations/v71.go
@@ -53,7 +53,7 @@ func addScratchHash(x *xorm.Engine) error {
for _, tfa := range tfas {
// generate salt
- salt, err := util.RandomString(10)
+ salt, err := util.CryptoRandomString(10)
if err != nil {
return err
}
diff --git a/models/migrations/v85.go b/models/migrations/v85.go
index bdbcebeb00..9611d6e72a 100644
--- a/models/migrations/v85.go
+++ b/models/migrations/v85.go
@@ -65,7 +65,7 @@ func hashAppToken(x *xorm.Engine) error {
for _, token := range tokens {
// generate salt
- salt, err := util.RandomString(10)
+ salt, err := util.CryptoRandomString(10)
if err != nil {
return err
}
diff --git a/models/token.go b/models/token.go
index 44428a0809..b89514309c 100644
--- a/models/token.go
+++ b/models/token.go
@@ -62,7 +62,7 @@ func init() {
// NewAccessToken creates new access token.
func NewAccessToken(t *AccessToken) error {
- salt, err := util.RandomString(10)
+ salt, err := util.CryptoRandomString(10)
if err != nil {
return err
}
diff --git a/models/user/user.go b/models/user/user.go
index 57a7fcadfa..38352fe5e2 100644
--- a/models/user/user.go
+++ b/models/user/user.go
@@ -533,7 +533,7 @@ const SaltByteLength = 16
// GetUserSalt returns a random user salt token.
func GetUserSalt() (string, error) {
- rBytes, err := util.RandomBytes(SaltByteLength)
+ rBytes, err := util.CryptoRandomBytes(SaltByteLength)
if err != nil {
return "", err
}
diff --git a/modules/generate/generate.go b/modules/generate/generate.go
index ae9aeee18b..326fe8036b 100644
--- a/modules/generate/generate.go
+++ b/modules/generate/generate.go
@@ -60,7 +60,7 @@ func NewJwtSecretBase64() (string, error) {
// NewSecretKey generate a new value intended to be used by SECRET_KEY.
func NewSecretKey() (string, error) {
- secretKey, err := util.RandomString(64)
+ secretKey, err := util.CryptoRandomString(64)
if err != nil {
return "", err
}
diff --git a/modules/secret/secret.go b/modules/secret/secret.go
index 6a5024b729..6b410f2381 100644
--- a/modules/secret/secret.go
+++ b/modules/secret/secret.go
@@ -24,7 +24,7 @@ func New() (string, error) {
// NewWithLength creates a new secret for a given length
func NewWithLength(length int64) (string, error) {
- return util.RandomString(length)
+ return util.CryptoRandomString(length)
}
// AesEncrypt encrypts text and given key with AES.
diff --git a/modules/util/util.go b/modules/util/util.go
index c2117a6525..90d0eca15c 100644
--- a/modules/util/util.go
+++ b/modules/util/util.go
@@ -137,8 +137,8 @@ func MergeInto(dict map[string]interface{}, values ...interface{}) (map[string]i
return dict, nil
}
-// RandomInt returns a random integer between 0 and limit, inclusive
-func RandomInt(limit int64) (int64, error) {
+// CryptoRandomInt returns a crypto random integer between 0 and limit, inclusive
+func CryptoRandomInt(limit int64) (int64, error) {
rInt, err := rand.Int(rand.Reader, big.NewInt(limit))
if err != nil {
return 0, err
@@ -146,27 +146,27 @@ func RandomInt(limit int64) (int64, error) {
return rInt.Int64(), nil
}
-const letters = "abcdefghijklmnopqrstuvwxyzABCDEFGHIJKLMNOPQRSTUVWXYZ0123456789"
+const alphanumericalChars = "abcdefghijklmnopqrstuvwxyzABCDEFGHIJKLMNOPQRSTUVWXYZ0123456789"
-// RandomString generates a random alphanumerical string
-func RandomString(length int64) (string, error) {
- bytes := make([]byte, length)
- limit := int64(len(letters))
- for i := range bytes {
- num, err := RandomInt(limit)
+// CryptoRandomString generates a crypto random alphanumerical string, each byte is generated by [0,61] range
+func CryptoRandomString(length int64) (string, error) {
+ buf := make([]byte, length)
+ limit := int64(len(alphanumericalChars))
+ for i := range buf {
+ num, err := CryptoRandomInt(limit)
if err != nil {
return "", err
}
- bytes[i] = letters[num]
+ buf[i] = alphanumericalChars[num]
}
- return string(bytes), nil
+ return string(buf), nil
}
-// RandomBytes generates `length` bytes
-// This differs from RandomString, as RandomString is limits each byte to have
-// a maximum value of 63 instead of 255(max byte size)
-func RandomBytes(length int64) ([]byte, error) {
- bytes := make([]byte, length)
- _, err := rand.Read(bytes)
- return bytes, err
+// CryptoRandomBytes generates `length` crypto bytes
+// This differs from CryptoRandomString, as each byte in CryptoRandomString is generated by [0,61] range
+// This function generates totally random bytes, each byte is generated by [0,255] range
+func CryptoRandomBytes(length int64) ([]byte, error) {
+ buf := make([]byte, length)
+ _, err := rand.Read(buf)
+ return buf, err
}
diff --git a/modules/util/util_test.go b/modules/util/util_test.go
index e2e26b2627..b32cec23d9 100644
--- a/modules/util/util_test.go
+++ b/modules/util/util_test.go
@@ -120,20 +120,20 @@ func Test_NormalizeEOL(t *testing.T) {
}
func Test_RandomInt(t *testing.T) {
- int, err := RandomInt(255)
+ int, err := CryptoRandomInt(255)
assert.True(t, int >= 0)
assert.True(t, int <= 255)
assert.NoError(t, err)
}
func Test_RandomString(t *testing.T) {
- str1, err := RandomString(32)
+ str1, err := CryptoRandomString(32)
assert.NoError(t, err)
matches, err := regexp.MatchString(`^[a-zA-Z0-9]{32}$`, str1)
assert.NoError(t, err)
assert.True(t, matches)
- str2, err := RandomString(32)
+ str2, err := CryptoRandomString(32)
assert.NoError(t, err)
matches, err = regexp.MatchString(`^[a-zA-Z0-9]{32}$`, str1)
assert.NoError(t, err)
@@ -141,13 +141,13 @@ func Test_RandomString(t *testing.T) {
assert.NotEqual(t, str1, str2)
- str3, err := RandomString(256)
+ str3, err := CryptoRandomString(256)
assert.NoError(t, err)
matches, err = regexp.MatchString(`^[a-zA-Z0-9]{256}$`, str3)
assert.NoError(t, err)
assert.True(t, matches)
- str4, err := RandomString(256)
+ str4, err := CryptoRandomString(256)
assert.NoError(t, err)
matches, err = regexp.MatchString(`^[a-zA-Z0-9]{256}$`, str4)
assert.NoError(t, err)
@@ -157,18 +157,18 @@ func Test_RandomString(t *testing.T) {
}
func Test_RandomBytes(t *testing.T) {
- bytes1, err := RandomBytes(32)
+ bytes1, err := CryptoRandomBytes(32)
assert.NoError(t, err)
- bytes2, err := RandomBytes(32)
+ bytes2, err := CryptoRandomBytes(32)
assert.NoError(t, err)
assert.NotEqual(t, bytes1, bytes2)
- bytes3, err := RandomBytes(256)
+ bytes3, err := CryptoRandomBytes(256)
assert.NoError(t, err)
- bytes4, err := RandomBytes(256)
+ bytes4, err := CryptoRandomBytes(256)
assert.NoError(t, err)
assert.NotEqual(t, bytes3, bytes4)
diff --git a/routers/web/auth/openid.go b/routers/web/auth/openid.go
index e0c6069546..f3189887a5 100644
--- a/routers/web/auth/openid.go
+++ b/routers/web/auth/openid.go
@@ -416,7 +416,7 @@ func RegisterOpenIDPost(ctx *context.Context) {
if length < 256 {
length = 256
}
- password, err := util.RandomString(int64(length))
+ password, err := util.CryptoRandomString(int64(length))
if err != nil {
ctx.RenderWithErr(err.Error(), tplSignUpOID, form)
return
diff --git a/routers/web/repo/setting.go b/routers/web/repo/setting.go
index d1c03b59a6..8e249af55d 100644
--- a/routers/web/repo/setting.go
+++ b/routers/web/repo/setting.go
@@ -337,7 +337,7 @@ func SettingsPost(ctx *context.Context) {
return
}
- remoteSuffix, err := util.RandomString(10)
+ remoteSuffix, err := util.CryptoRandomString(10)
if err != nil {
ctx.ServerError("RandomString", err)
return