Increase Salt randomness (#18179)

- The current implementation of `RandomString` doesn't give you a most-possible unique randomness. It gives you 6*`length` instead of the possible 8*`length` bits(or as `length`x bytes) randomness. This is because `RandomString` is being limited to a max value of 63, this in order to represent the random byte as a letter/digit.
- The recommendation of pbkdf2 is to use 64+ bit salt, which the `RandomString` doesn't give with a length of 10, instead of increasing 10 to a higher number, this patch adds a new function called `RandomBytes` which does give you the guarentee of 8*`length` randomness and thus corresponding of `length`x bytes randomness.
- Use hexadecimal to store the bytes value in the database, as mentioned, it doesn't play nice in order to convert it to a string. This will always be a length of 32(with `length` being 16).
- When we detect on `Authenticate`(source: db) that a user has the old format of salt, re-hash the password such that the user will have it's password hashed with increased salt.

Thanks to @zeripath for working out the rouge edges from my first commit 😄.

Co-authored-by: lafriks <lauris@nix.lv>
Co-authored-by: zeripath <art27@cantab.net>

3 files changed, 83 insertions, 11 deletions

diff --git a/models/migrations/migrations.go b/models/migrations/migrations.go
index cc72ba99ab..4b720c3f02 100644
--- a/models/migrations/migrations.go
+++ b/models/migrations/migrations.go
@@ -363,6 +363,8 @@ var migrations = []Migration{
 	NewMigration("Add Sorting to ProjectIssue table", addProjectIssueSorting),
 	// v204 -> v205
 	NewMigration("Add key is verified to ssh key", addSSHKeyIsVerified),
+	// v205 -> v206
+	NewMigration("Migrate to higher varchar on user struct", migrateUserPasswordSalt),
 }
 
 // GetCurrentDBVersion returns the current db version
diff --git a/models/migrations/v205.go b/models/migrations/v205.go
new file mode 100644
index 0000000000..755cb10245
--- /dev/null
+++ b/models/migrations/v205.go
@@ -0,0 +1,39 @@
+// Copyright 2022 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 migrations
+
+import (
+	"xorm.io/xorm"
+	"xorm.io/xorm/schemas"
+)
+
+func migrateUserPasswordSalt(x *xorm.Engine) error {
+	dbType := x.Dialect().URI().DBType
+	// For SQLITE, the max length doesn't matter.
+	if dbType == schemas.SQLITE {
+		return nil
+	}
+
+	if err := modifyColumn(x, "user", &schemas.Column{
+		Name: "rands",
+		SQLType: schemas.SQLType{
+			Name: "VARCHAR",
+		},
+		Length: 32,
+		// MySQL will like us again.
+		Nullable: true,
+	}); err != nil {
+		return err
+	}
+
+	return modifyColumn(x, "user", &schemas.Column{
+		Name: "salt",
+		SQLType: schemas.SQLType{
+			Name: "VARCHAR",
+		},
+		Length:   32,
+		Nullable: true,
+	})
+}
diff --git a/models/user/user.go b/models/user/user.go
index 06cacd85fe..8efd51c9be 100644
--- a/models/user/user.go
+++ b/models/user/user.go
@@ -95,8 +95,8 @@ type User struct {
 	Type        UserType
 	Location    string
 	Website     string
-	Rands       string `xorm:"VARCHAR(10)"`
-	Salt        string `xorm:"VARCHAR(10)"`
+	Rands       string `xorm:"VARCHAR(32)"`
+	Salt        string `xorm:"VARCHAR(32)"`
 	Language    string `xorm:"VARCHAR(5)"`
 	Description string
 
@@ -358,24 +358,40 @@ func (u *User) NewGitSig() *git.Signature {
 	}
 }
 
-func hashPassword(passwd, salt, algo string) string {
+func hashPassword(passwd, salt, algo string) (string, error) {
 	var tempPasswd []byte
+	var saltBytes []byte
+
+	// There are two formats for the Salt value:
+	// * The new format is a (32+)-byte hex-encoded string
+	// * The old format was a 10-byte binary format
+	// We have to tolerate both here but Authenticate should
+	// regenerate the Salt following a successful validation.
+	if len(salt) == 10 {
+		saltBytes = []byte(salt)
+	} else {
+		var err error
+		saltBytes, err = hex.DecodeString(salt)
+		if err != nil {
+			return "", err
+		}
+	}
 
 	switch algo {
 	case algoBcrypt:
 		tempPasswd, _ = bcrypt.GenerateFromPassword([]byte(passwd), bcrypt.DefaultCost)
-		return string(tempPasswd)
+		return string(tempPasswd), nil
 	case algoScrypt:
-		tempPasswd, _ = scrypt.Key([]byte(passwd), []byte(salt), 65536, 16, 2, 50)
+		tempPasswd, _ = scrypt.Key([]byte(passwd), saltBytes, 65536, 16, 2, 50)
 	case algoArgon2:
-		tempPasswd = argon2.IDKey([]byte(passwd), []byte(salt), 2, 65536, 8, 50)
+		tempPasswd = argon2.IDKey([]byte(passwd), saltBytes, 2, 65536, 8, 50)
 	case algoPbkdf2:
 		fallthrough
 	default:
-		tempPasswd = pbkdf2.Key([]byte(passwd), []byte(salt), 10000, 50, sha256.New)
+		tempPasswd = pbkdf2.Key([]byte(passwd), saltBytes, 10000, 50, sha256.New)
 	}
 
-	return fmt.Sprintf("%x", tempPasswd)
+	return fmt.Sprintf("%x", tempPasswd), nil
 }
 
 // SetPassword hashes a password using the algorithm defined in the config value of PASSWORD_HASH_ALGO
@@ -391,15 +407,20 @@ func (u *User) SetPassword(passwd string) (err error) {
 	if u.Salt, err = GetUserSalt(); err != nil {
 		return err
 	}
+	if u.Passwd, err = hashPassword(passwd, u.Salt, setting.PasswordHashAlgo); err != nil {
+		return err
+	}
 	u.PasswdHashAlgo = setting.PasswordHashAlgo
-	u.Passwd = hashPassword(passwd, u.Salt, setting.PasswordHashAlgo)
 
 	return nil
 }
 
 // ValidatePassword checks if given password matches the one belongs to the user.
 func (u *User) ValidatePassword(passwd string) bool {
-	tempHash := hashPassword(passwd, u.Salt, u.PasswdHashAlgo)
+	tempHash, err := hashPassword(passwd, u.Salt, u.PasswdHashAlgo)
+	if err != nil {
+		return false
+	}
 
 	if u.PasswdHashAlgo != algoBcrypt && subtle.ConstantTimeCompare([]byte(u.Passwd), []byte(tempHash)) == 1 {
 		return true
@@ -505,9 +526,19 @@ func IsUserExist(uid int64, name string) (bool, error) {
 	return isUserExist(db.GetEngine(db.DefaultContext), uid, name)
 }
 
+// Note: As of the beginning of 2022, it is recommended to use at least
+// 64 bits of salt, but NIST is already recommending to use to 128 bits.
+// (16 bytes = 16 * 8 = 128 bits)
+const SaltByteLength = 16
+
 // GetUserSalt returns a random user salt token.
 func GetUserSalt() (string, error) {
-	return util.RandomString(10)
+	rBytes, err := util.RandomBytes(SaltByteLength)
+	if err != nil {
+		return "", err
+	}
+	// Returns a 32 bytes long string.
+	return hex.EncodeToString(rBytes), nil
 }
 
 // NewGhostUser creates and returns a fake user for someone has deleted his/her account.