Use base32 for 2FA scratch token (#18384)

* Use base32 for 2FA scratch token
* rename Secure* to Crypto*, add comments

diff --git a/models/auth/twofactor.go b/models/auth/twofactor.go
index 883e6ce01c09695e749e48ef69186a396d3711a4..c5bd972f91641a1c74bca16b3239776dc2e536c4 100644 (file)
--- 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 e4ed46a21a5b1341a9b85af1cd898bb8e293e853..163ec3ee5f9884897dd76b52cc69e8b8c53b261e 100644 (file)
--- 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 bdbcebeb00f24b9bd7cf5d26b563cf7bf10e7a31..9611d6e72ac28552590e9907f2696c6adc131e37 100644 (file)
--- 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 44428a08092733ab0a43b0faa4457ecf06ad2109..b89514309c492d09f83683807e7b8c03da9a4139 100644 (file)
--- 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 57a7fcadfa96fa2adbb3d0c352745c377b1c37e1..38352fe5e24c3a4f54d987f67341e8b6aacf21d8 100644 (file)
--- 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 ae9aeee18b67427d9c1fce859e6845e90e1387fe..326fe8036b445a2c9af878e9a8c7ff16a65184f3 100644 (file)
--- 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 6a5024b7298d24f116b98c386eea267675ac7fb1..6b410f238197bdbf8d11f481eeaeefd98711c1a4 100644 (file)
--- 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 c2117a6525b34416525b85d47fb522d5ee497388..90d0eca15c1e8f73ed63d15ebbc57a90400f0c84 100644 (file)
--- 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 e2e26b26274c473d3a1d2f238fb27a443016e834..b32cec23d9bccded0a9c32b598f087b3cf67a7cf 100644 (file)
--- 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 e0c60695463bfcbb9be5277758e8c33d597ba649..f3189887a530873ff23f3789a0baab5ca7daab55 100644 (file)
--- 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 d1c03b59a6b854e1b56a4e1ab25cf0b9eaa74208..8e249af55d0729355914840221b2190c758596c4 100644 (file)
--- 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