From 623c93ff46798e86636dba24f5a48bb35e12b9a5 Mon Sep 17 00:00:00 2001 From: Gusted Date: Tue, 4 Jan 2022 15:13:52 +0000 Subject: [PATCH] Increase Salt randomness (#18179) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - 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 Co-authored-by: zeripath --- models/migrations/migrations.go | 2 + models/migrations/v205.go | 39 ++++++++++++++++++ models/user/user.go | 53 ++++++++++++++++++++----- modules/util/util.go | 13 +++++- modules/util/util_test.go | 18 +++++++++ services/auth/source/db/authenticate.go | 4 +- 6 files changed, 115 insertions(+), 14 deletions(-) create mode 100644 models/migrations/v205.go 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. diff --git a/modules/util/util.go b/modules/util/util.go index cbc6eb4f8a..c2117a6525 100644 --- a/modules/util/util.go +++ b/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 +} diff --git a/modules/util/util_test.go b/modules/util/util_test.go index 39cf07c855..f42b1a930f 100644 --- a/modules/util/util_test.go +++ b/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")) diff --git a/services/auth/source/db/authenticate.go b/services/auth/source/db/authenticate.go index e0e439c2fe..f062f66ae0 100644 --- a/services/auth/source/db/authenticate.go +++ b/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 }