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>

Changes

6 changed files (+115/-14)

• 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
• models/migrations/v205.go (new)
  • @@ -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, + }) + }
• 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.
• modules/util/util.go
  • @@ -139,11 +139,11 @@ func MergeInto(dict map[string]interface{}, values ...interface{}) (map[string]i
    // 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)) + rInt, err := rand.Int(rand.Reader, big.NewInt(limit)) if err != nil { return 0, err } - return int.Int64(), nil + return rInt.Int64(), nil } const letters = "abcdefghijklmnopqrstuvwxyzABCDEFGHIJKLMNOPQRSTUVWXYZ0123456789"
  • @@ -161,3 +161,12 @@ func RandomString(length int64) (string, error) {
    } return string(bytes), 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 + }
• modules/util/util_test.go
  • @@ -157,6 +157,24 @@ func Test_RandomString(t *testing.T) {
    assert.NotEqual(t, str3, str4) } + func Test_RandomBytes(t *testing.T) { + bytes1, err := RandomBytes(32) + assert.NoError(t, err) + + bytes2, err := RandomBytes(32) + assert.NoError(t, err) + + assert.NotEqual(t, bytes1, bytes2) + + bytes3, err := RandomBytes(256) + assert.NoError(t, err) + + bytes4, err := RandomBytes(256) + assert.NoError(t, err) + + assert.NotEqual(t, bytes3, bytes4) + } + func Test_OptionalBool(t *testing.T) { assert.Equal(t, OptionalBoolNone, OptionalBoolParse("")) assert.Equal(t, OptionalBoolNone, OptionalBoolParse("x"))
• services/auth/source/db/authenticate.go
  • @@ -21,7 +21,9 @@ func Authenticate(user *user_model.User, login, password string) (*user_model.Us
    } // Update password hash if server password hash algorithm have changed - if user.PasswdHashAlgo != setting.PasswordHashAlgo { + // Or update the password when the salt length doesn't match the current + // recommended salt length, this in order to migrate user's salts to a more secure salt. + if user.PasswdHashAlgo != setting.PasswordHashAlgo || len(user.Salt) != user_model.SaltByteLength*2 { if err := user.SetPassword(password); err != nil { return nil, err }