From 1e6fa57acbe3c05cb996b789e8c2d381c953826f Mon Sep 17 00:00:00 2001
From: silverwind <me@silverwind.io>
Date: Mon, 10 May 2021 08:45:17 +0200
Subject: [PATCH] Use single shared random string generation function (#15741)

* Use single shared random string generation function

- Replace 3 functions that do the same with 1 shared one
- Use crypto/rand over math/rand for a stronger RNG
- Output only alphanumerical for URL compatibilty

Fixes: #15536

* use const string method

* Update modules/avatar/avatar.go

Co-authored-by: a1012112796 <1012112796@qq.com>

Co-authored-by: a1012112796 <1012112796@qq.com>
---
 models/migrations/v71.go          |   4 +-
 models/migrations/v85.go          |   4 +-
 models/token.go                   |   4 +-
 models/twofactor.go               |   6 +-
 models/user.go                    |   3 +-
 modules/avatar/avatar.go          |  10 +--
 modules/avatar/avatar_test.go     |  15 +++--
 modules/context/secret.go         | 100 ------------------------------
 modules/generate/generate.go      |  32 +---------
 modules/generate/generate_test.go |  24 -------
 modules/secret/secret.go          |  19 ++----
 modules/secret/secret_test.go     |   2 +-
 modules/util/util.go              |  27 ++++++++
 modules/util/util_test.go         |  38 ++++++++++++
 routers/user/auth_openid.go       |   4 +-
 15 files changed, 100 insertions(+), 192 deletions(-)
 delete mode 100644 modules/context/secret.go
 delete mode 100644 modules/generate/generate_test.go

diff --git a/models/migrations/v71.go b/models/migrations/v71.go
index 3012dd94f..e4ed46a21 100644
--- a/models/migrations/v71.go
+++ b/models/migrations/v71.go
@@ -8,8 +8,8 @@ import (
 	"crypto/sha256"
 	"fmt"
 
-	"code.gitea.io/gitea/modules/generate"
 	"code.gitea.io/gitea/modules/timeutil"
+	"code.gitea.io/gitea/modules/util"
 
 	"golang.org/x/crypto/pbkdf2"
 	"xorm.io/xorm"
@@ -53,7 +53,7 @@ func addScratchHash(x *xorm.Engine) error {
 
 		for _, tfa := range tfas {
 			// generate salt
-			salt, err := generate.GetRandomString(10)
+			salt, err := util.RandomString(10)
 			if err != nil {
 				return err
 			}
diff --git a/models/migrations/v85.go b/models/migrations/v85.go
index 8c92f10b6..bdbcebeb0 100644
--- a/models/migrations/v85.go
+++ b/models/migrations/v85.go
@@ -7,9 +7,9 @@ package migrations
 import (
 	"fmt"
 
-	"code.gitea.io/gitea/modules/generate"
 	"code.gitea.io/gitea/modules/log"
 	"code.gitea.io/gitea/modules/timeutil"
+	"code.gitea.io/gitea/modules/util"
 
 	"xorm.io/xorm"
 )
@@ -65,7 +65,7 @@ func hashAppToken(x *xorm.Engine) error {
 
 		for _, token := range tokens {
 			// generate salt
-			salt, err := generate.GetRandomString(10)
+			salt, err := util.RandomString(10)
 			if err != nil {
 				return err
 			}
diff --git a/models/token.go b/models/token.go
index 1245098df..4737dddda 100644
--- a/models/token.go
+++ b/models/token.go
@@ -10,8 +10,8 @@ import (
 	"time"
 
 	"code.gitea.io/gitea/modules/base"
-	"code.gitea.io/gitea/modules/generate"
 	"code.gitea.io/gitea/modules/timeutil"
+	"code.gitea.io/gitea/modules/util"
 
 	gouuid "github.com/google/uuid"
 )
@@ -40,7 +40,7 @@ func (t *AccessToken) AfterLoad() {
 
 // NewAccessToken creates new access token.
 func NewAccessToken(t *AccessToken) error {
-	salt, err := generate.GetRandomString(10)
+	salt, err := util.RandomString(10)
 	if err != nil {
 		return err
 	}
diff --git a/models/twofactor.go b/models/twofactor.go
index a84da8cdb..c19c5d120 100644
--- a/models/twofactor.go
+++ b/models/twofactor.go
@@ -11,10 +11,10 @@ import (
 	"encoding/base64"
 	"fmt"
 
-	"code.gitea.io/gitea/modules/generate"
 	"code.gitea.io/gitea/modules/secret"
 	"code.gitea.io/gitea/modules/setting"
 	"code.gitea.io/gitea/modules/timeutil"
+	"code.gitea.io/gitea/modules/util"
 
 	"github.com/pquerna/otp/totp"
 	"golang.org/x/crypto/pbkdf2"
@@ -34,11 +34,11 @@ type TwoFactor struct {
 
 // GenerateScratchToken recreates the scratch token the user is using.
 func (t *TwoFactor) GenerateScratchToken() (string, error) {
-	token, err := generate.GetRandomString(8)
+	token, err := util.RandomString(8)
 	if err != nil {
 		return "", err
 	}
-	t.ScratchSalt, _ = generate.GetRandomString(10)
+	t.ScratchSalt, _ = util.RandomString(10)
 	t.ScratchHash = hashToken(token, t.ScratchSalt)
 	return token, nil
 }
diff --git a/models/user.go b/models/user.go
index 26cfc0804..02ccfea47 100644
--- a/models/user.go
+++ b/models/user.go
@@ -22,7 +22,6 @@ import (
 	"unicode/utf8"
 
 	"code.gitea.io/gitea/modules/base"
-	"code.gitea.io/gitea/modules/generate"
 	"code.gitea.io/gitea/modules/git"
 	"code.gitea.io/gitea/modules/log"
 	"code.gitea.io/gitea/modules/setting"
@@ -746,7 +745,7 @@ func IsUserExist(uid int64, name string) (bool, error) {
 
 // GetUserSalt returns a random user salt token.
 func GetUserSalt() (string, error) {
-	return generate.GetRandomString(10)
+	return util.RandomString(10)
 }
 
 // NewGhostUser creates and returns a fake user for someone has deleted his/her account.
diff --git a/modules/avatar/avatar.go b/modules/avatar/avatar.go
index 44b56c26c..bb9c2e953 100644
--- a/modules/avatar/avatar.go
+++ b/modules/avatar/avatar.go
@@ -12,10 +12,9 @@ import (
 
 	// Enable PNG support:
 	_ "image/png"
-	"math/rand"
-	"time"
 
 	"code.gitea.io/gitea/modules/setting"
+	"code.gitea.io/gitea/modules/util"
 
 	"github.com/issue9/identicon"
 	"github.com/nfnt/resize"
@@ -29,8 +28,11 @@ const AvatarSize = 290
 // in custom size (height and width).
 func RandomImageSize(size int, data []byte) (image.Image, error) {
 	randExtent := len(palette.WebSafe) - 32
-	rand.Seed(time.Now().UnixNano())
-	colorIndex := rand.Intn(randExtent)
+	integer, err := util.RandomInt(int64(randExtent))
+	if err != nil {
+		return nil, fmt.Errorf("util.RandomInt: %v", err)
+	}
+	colorIndex := int(integer)
 	backColorIndex := colorIndex - 1
 	if backColorIndex < 0 {
 		backColorIndex = randExtent - 1
diff --git a/modules/avatar/avatar_test.go b/modules/avatar/avatar_test.go
index 853560565..f48266c85 100644
--- a/modules/avatar/avatar_test.go
+++ b/modules/avatar/avatar_test.go
@@ -13,12 +13,17 @@ import (
 	"github.com/stretchr/testify/assert"
 )
 
-func Test_RandomImage(t *testing.T) {
-	_, err := RandomImage([]byte("gogs@local"))
-	assert.NoError(t, err)
-
-	_, err = RandomImageSize(0, []byte("gogs@local"))
+func Test_RandomImageSize(t *testing.T) {
+	_, err := RandomImageSize(0, []byte("gitea@local"))
 	assert.Error(t, err)
+
+	_, err = RandomImageSize(64, []byte("gitea@local"))
+	assert.NoError(t, err)
+}
+
+func Test_RandomImage(t *testing.T) {
+	_, err := RandomImage([]byte("gitea@local"))
+	assert.NoError(t, err)
 }
 
 func Test_PrepareWithPNG(t *testing.T) {
diff --git a/modules/context/secret.go b/modules/context/secret.go
deleted file mode 100644
index fcb488d21..000000000
--- a/modules/context/secret.go
+++ /dev/null
@@ -1,100 +0,0 @@
-// Copyright 2019 The Gitea Authors. All rights reserved.
-// Use of this source code is governed by a MIT-style
-// license that can be found in the LICENSE file.
-
-package context
-
-import (
-	"crypto/aes"
-	"crypto/cipher"
-	"crypto/rand"
-	"crypto/sha256"
-	"encoding/base64"
-	"errors"
-	"io"
-)
-
-// NewSecret creates a new secret
-func NewSecret() (string, error) {
-	return NewSecretWithLength(32)
-}
-
-// NewSecretWithLength creates a new secret for a given length
-func NewSecretWithLength(length int64) (string, error) {
-	return randomString(length)
-}
-
-func randomBytes(len int64) ([]byte, error) {
-	b := make([]byte, len)
-	if _, err := rand.Read(b); err != nil {
-		return nil, err
-	}
-	return b, nil
-}
-
-func randomString(len int64) (string, error) {
-	b, err := randomBytes(len)
-	return base64.URLEncoding.EncodeToString(b), err
-}
-
-// AesEncrypt encrypts text and given key with AES.
-func AesEncrypt(key, text []byte) ([]byte, error) {
-	block, err := aes.NewCipher(key)
-	if err != nil {
-		return nil, err
-	}
-	b := base64.StdEncoding.EncodeToString(text)
-	ciphertext := make([]byte, aes.BlockSize+len(b))
-	iv := ciphertext[:aes.BlockSize]
-	if _, err := io.ReadFull(rand.Reader, iv); err != nil {
-		return nil, err
-	}
-	cfb := cipher.NewCFBEncrypter(block, iv)
-	cfb.XORKeyStream(ciphertext[aes.BlockSize:], []byte(b))
-	return ciphertext, nil
-}
-
-// AesDecrypt decrypts text and given key with AES.
-func AesDecrypt(key, text []byte) ([]byte, error) {
-	block, err := aes.NewCipher(key)
-	if err != nil {
-		return nil, err
-	}
-	if len(text) < aes.BlockSize {
-		return nil, errors.New("ciphertext too short")
-	}
-	iv := text[:aes.BlockSize]
-	text = text[aes.BlockSize:]
-	cfb := cipher.NewCFBDecrypter(block, iv)
-	cfb.XORKeyStream(text, text)
-	data, err := base64.StdEncoding.DecodeString(string(text))
-	if err != nil {
-		return nil, err
-	}
-	return data, nil
-}
-
-// EncryptSecret encrypts a string with given key into a hex string
-func EncryptSecret(key string, str string) (string, error) {
-	keyHash := sha256.Sum256([]byte(key))
-	plaintext := []byte(str)
-	ciphertext, err := AesEncrypt(keyHash[:], plaintext)
-	if err != nil {
-		return "", err
-	}
-	return base64.StdEncoding.EncodeToString(ciphertext), nil
-}
-
-// DecryptSecret decrypts a previously encrypted hex string
-func DecryptSecret(key string, cipherhex string) (string, error) {
-	keyHash := sha256.Sum256([]byte(key))
-	ciphertext, err := base64.StdEncoding.DecodeString(cipherhex)
-	if err != nil {
-		return "", err
-	}
-	plaintext, err := AesDecrypt(keyHash[:], ciphertext)
-	if err != nil {
-		return "", err
-	}
-	return string(plaintext), nil
-}
diff --git a/modules/generate/generate.go b/modules/generate/generate.go
index 304ad87f2..96589d3fb 100644
--- a/modules/generate/generate.go
+++ b/modules/generate/generate.go
@@ -9,31 +9,12 @@ import (
 	"crypto/rand"
 	"encoding/base64"
 	"io"
-	"math/big"
 	"time"
 
+	"code.gitea.io/gitea/modules/util"
 	"github.com/dgrijalva/jwt-go"
 )
 
-// GetRandomString generate random string by specify chars.
-func GetRandomString(n int) (string, error) {
-	const alphanum = "0123456789ABCDEFGHIJKLMNOPQRSTUVWXYZabcdefghijklmnopqrstuvwxyz"
-
-	buffer := make([]byte, n)
-	max := big.NewInt(int64(len(alphanum)))
-
-	for i := 0; i < n; i++ {
-		index, err := randomInt(max)
-		if err != nil {
-			return "", err
-		}
-
-		buffer[i] = alphanum[index]
-	}
-
-	return string(buffer), nil
-}
-
 // NewInternalToken generate a new value intended to be used by INTERNAL_TOKEN.
 func NewInternalToken() (string, error) {
 	secretBytes := make([]byte, 32)
@@ -69,19 +50,10 @@ func NewJwtSecret() (string, error) {
 
 // NewSecretKey generate a new value intended to be used by SECRET_KEY.
 func NewSecretKey() (string, error) {
-	secretKey, err := GetRandomString(64)
+	secretKey, err := util.RandomString(64)
 	if err != nil {
 		return "", err
 	}
 
 	return secretKey, nil
 }
-
-func randomInt(max *big.Int) (int, error) {
-	rand, err := rand.Int(rand.Reader, max)
-	if err != nil {
-		return 0, err
-	}
-
-	return int(rand.Int64()), nil
-}
diff --git a/modules/generate/generate_test.go b/modules/generate/generate_test.go
deleted file mode 100644
index 1cacfe668..000000000
--- a/modules/generate/generate_test.go
+++ /dev/null
@@ -1,24 +0,0 @@
-// Copyright 2020 The Gitea Authors. All rights reserved.
-// Use of this source code is governed by a MIT-style
-// license that can be found in the LICENSE file.
-
-package generate
-
-import (
-	"os"
-	"testing"
-
-	"github.com/stretchr/testify/assert"
-)
-
-func TestMain(m *testing.M) {
-	retVal := m.Run()
-
-	os.Exit(retVal)
-}
-
-func TestGetRandomString(t *testing.T) {
-	randomString, err := GetRandomString(4)
-	assert.NoError(t, err)
-	assert.Len(t, randomString, 4)
-}
diff --git a/modules/secret/secret.go b/modules/secret/secret.go
index 2b6e22cc6..7a06730c3 100644
--- a/modules/secret/secret.go
+++ b/modules/secret/secret.go
@@ -13,29 +13,18 @@ import (
 	"encoding/hex"
 	"errors"
 	"io"
+
+	"code.gitea.io/gitea/modules/util"
 )
 
 // New creats a new secret
 func New() (string, error) {
-	return NewWithLength(32)
+	return NewWithLength(44)
 }
 
 // NewWithLength creates a new secret for a given length
 func NewWithLength(length int64) (string, error) {
-	return randomString(length)
-}
-
-func randomBytes(len int64) ([]byte, error) {
-	b := make([]byte, len)
-	if _, err := rand.Read(b); err != nil {
-		return nil, err
-	}
-	return b, nil
-}
-
-func randomString(len int64) (string, error) {
-	b, err := randomBytes(len)
-	return base64.URLEncoding.EncodeToString(b), err
+	return util.RandomString(length)
 }
 
 // AesEncrypt encrypts text and given key with AES.
diff --git a/modules/secret/secret_test.go b/modules/secret/secret_test.go
index 6531ffbeb..f3a88eecc 100644
--- a/modules/secret/secret_test.go
+++ b/modules/secret/secret_test.go
@@ -13,7 +13,7 @@ import (
 func TestNew(t *testing.T) {
 	result, err := New()
 	assert.NoError(t, err)
-	assert.True(t, len(result) > 32)
+	assert.True(t, len(result) == 44)
 
 	result2, err := New()
 	assert.NoError(t, err)
diff --git a/modules/util/util.go b/modules/util/util.go
index 9de1710ac..d26e6f13e 100644
--- a/modules/util/util.go
+++ b/modules/util/util.go
@@ -6,7 +6,9 @@ package util
 
 import (
 	"bytes"
+	"crypto/rand"
 	"errors"
+	"math/big"
 	"strings"
 )
 
@@ -124,3 +126,28 @@ 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) {
+	int, err := rand.Int(rand.Reader, big.NewInt(limit))
+	if err != nil {
+		return 0, err
+	}
+	return int.Int64(), nil
+}
+
+const letters = "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)
+		if err != nil {
+			return "", err
+		}
+		bytes[i] = letters[num]
+	}
+	return string(bytes), nil
+}
diff --git a/modules/util/util_test.go b/modules/util/util_test.go
index 1d4f23de9..f82671787 100644
--- a/modules/util/util_test.go
+++ b/modules/util/util_test.go
@@ -5,6 +5,7 @@
 package util
 
 import (
+	"regexp"
 	"strings"
 	"testing"
 
@@ -118,3 +119,40 @@ func Test_NormalizeEOL(t *testing.T) {
 
 	assert.Equal(t, []byte("mix\nand\nmatch\n."), NormalizeEOL([]byte("mix\r\nand\rmatch\n.")))
 }
+
+func Test_RandomInt(t *testing.T) {
+	int, err := RandomInt(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)
+	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)
+	assert.NoError(t, err)
+	matches, err = regexp.MatchString(`^[a-zA-Z0-9]{32}$`, str1)
+	assert.NoError(t, err)
+	assert.True(t, matches)
+
+	assert.NotEqual(t, str1, str2)
+
+	str3, err := RandomString(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)
+	assert.NoError(t, err)
+	matches, err = regexp.MatchString(`^[a-zA-Z0-9]{256}$`, str4)
+	assert.NoError(t, err)
+	assert.True(t, matches)
+
+	assert.NotEqual(t, str3, str4)
+}
diff --git a/routers/user/auth_openid.go b/routers/user/auth_openid.go
index 45405320e..b1dfc6ada 100644
--- a/routers/user/auth_openid.go
+++ b/routers/user/auth_openid.go
@@ -13,11 +13,11 @@ import (
 	"code.gitea.io/gitea/modules/auth/openid"
 	"code.gitea.io/gitea/modules/base"
 	"code.gitea.io/gitea/modules/context"
-	"code.gitea.io/gitea/modules/generate"
 	"code.gitea.io/gitea/modules/hcaptcha"
 	"code.gitea.io/gitea/modules/log"
 	"code.gitea.io/gitea/modules/recaptcha"
 	"code.gitea.io/gitea/modules/setting"
+	"code.gitea.io/gitea/modules/util"
 	"code.gitea.io/gitea/modules/web"
 	"code.gitea.io/gitea/modules/web/middleware"
 	"code.gitea.io/gitea/services/forms"
@@ -411,7 +411,7 @@ func RegisterOpenIDPost(ctx *context.Context) {
 	if length < 256 {
 		length = 256
 	}
-	password, err := generate.GetRandomString(length)
+	password, err := util.RandomString(int64(length))
 	if err != nil {
 		ctx.RenderWithErr(err.Error(), tplSignUpOID, form)
 		return