From 6f1bb3e7df674deec596a2a636e82c8f5d9f12b1 Mon Sep 17 00:00:00 2001 From: Gusted Date: Tue, 4 Jan 2022 05:37:18 +0100 Subject: [PATCH 1/8] Use actually good salt - 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). --- models/user/user.go | 44 ++++++++++++++++++++++++++++----------- modules/util/util.go | 21 +++++++++++++++++-- modules/util/util_test.go | 18 ++++++++++++++++ 3 files changed, 69 insertions(+), 14 deletions(-) diff --git a/models/user/user.go b/models/user/user.go index 06cacd85feb9..5b2c98e4e7d4 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,29 @@ func (u *User) NewGitSig() *git.Signature { } } -func hashPassword(passwd, salt, algo string) string { +func hashPassword(passwd, salt, algo string) (string, error) { var tempPasswd []byte - + // Salt is encoded as hex, because certain bytes won't translates well into + // it's string correlation. + 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 +396,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 @@ -506,8 +516,18 @@ func IsUserExist(uid int64, name string) (bool, error) { } // GetUserSalt returns a random user salt token. +// +// Note on the used size here. As of begin of 2022, it's recommended to use +// 64 bits of salt, certain recommended like NIST are already recommending to use +// 128 bits of salt. Given it's more leaning to 128 bits of salt, we're using this +// and taking a security margin. Again, why 16? 16 bytes = 16 * 8 = 128 bits. func GetUserSalt() (string, error) { - return util.RandomString(10) + rBytes, err := util.RandomBytes(16) + 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 cbc6eb4f8a01..e6bb1ba01904 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,20 @@ func RandomString(length int64) (string, error) { } return string(bytes), nil } + +// RandomBytes generates `len` bytes +// This differ from RandomString, as RandomString is limited to each byte only +// having a maximum value of 63 instead of 255(max byte size) and thus decrease the +// security implications of it. +func RandomBytes(length int64) ([]byte, error) { + bytes := make([]byte, length) + limit := int64(^uint8(0)) + for i := range bytes { + num, err := RandomInt(limit) + if err != nil { + return []byte{}, err + } + bytes[i] = uint8(num) + } + return bytes, nil +} diff --git a/modules/util/util_test.go b/modules/util/util_test.go index 39cf07c85543..f42b1a930f24 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")) From b00a4ba17f2cd4155e0edbcdd5612aa9c2b3e269 Mon Sep 17 00:00:00 2001 From: Gusted Date: Tue, 4 Jan 2022 10:58:34 +0100 Subject: [PATCH 2/8] Add migration --- models/migrations/migrations.go | 2 ++ models/migrations/v205.go | 41 +++++++++++++++++++++++++++++++++ 2 files changed, 43 insertions(+) create mode 100644 models/migrations/v205.go diff --git a/models/migrations/migrations.go b/models/migrations/migrations.go index cc72ba99abf4..4b720c3f02a4 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 000000000000..50bfc236061c --- /dev/null +++ b/models/migrations/v205.go @@ -0,0 +1,41 @@ +// 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 + } + + // type User struct { + // Rands string `xorm:"VARCHAR(32)"` + // Salt string `xorm:"VARCHAR(32)"` + // } + + if err := modifyColumn(x, "user", &schemas.Column{ + Name: "rands", + SQLType: schemas.SQLType{ + Name: "VARCHAR", + }, + Length: 32, + }); err != nil { + return err + } + + return modifyColumn(x, "user", &schemas.Column{ + Name: "salt", + SQLType: schemas.SQLType{ + Name: "VARCHAR", + }, + Length: 32, + }) +} From 4ac0c4b5f7af691a637630219a2df517885be012 Mon Sep 17 00:00:00 2001 From: Gusted Date: Tue, 4 Jan 2022 11:47:26 +0100 Subject: [PATCH 3/8] Migrate passwords on-demand - 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. --- models/user/user.go | 18 ++++++++++++++---- services/auth/source/db/authenticate.go | 4 +++- 2 files changed, 17 insertions(+), 5 deletions(-) diff --git a/models/user/user.go b/models/user/user.go index 5b2c98e4e7d4..a4a3c67b626b 100644 --- a/models/user/user.go +++ b/models/user/user.go @@ -360,12 +360,22 @@ func (u *User) NewGitSig() *git.Signature { func hashPassword(passwd, salt, algo string) (string, error) { var tempPasswd []byte + var saltBytes []byte + // Salt is encoded as hex, because certain bytes won't translates well into - // it's string correlation. - saltBytes, err := hex.DecodeString(salt) - if err != nil { - return "", err + // it's string correlation. But only decode it when salt has a length of 32. + // Because we should tolerate hashing a password when a user has the old format + // of salt. + if len(salt) == 32 { + var err error + saltBytes, err = hex.DecodeString(salt) + if err != nil { + return "", err + } + } else { + saltBytes = []byte(salt) } + switch algo { case algoBcrypt: tempPasswd, _ = bcrypt.GenerateFromPassword([]byte(passwd), bcrypt.DefaultCost) diff --git a/services/auth/source/db/authenticate.go b/services/auth/source/db/authenticate.go index e0e439c2fe8f..71c6dd410905 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 has a length of 10, this in order + // to migrate user's salts to a more secure salt. + if user.PasswdHashAlgo != setting.PasswordHashAlgo || len(user.Salt) == 10 { if err := user.SetPassword(password); err != nil { return nil, err } From 8076dbabceefb0f12fd0fe788b42c63f2e955b7f Mon Sep 17 00:00:00 2001 From: Gusted Date: Tue, 4 Jan 2022 12:15:42 +0100 Subject: [PATCH 4/8] Reword comment in `hashPassword` --- models/user/user.go | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-) diff --git a/models/user/user.go b/models/user/user.go index a4a3c67b626b..f5f356e0b17b 100644 --- a/models/user/user.go +++ b/models/user/user.go @@ -362,10 +362,11 @@ func hashPassword(passwd, salt, algo string) (string, error) { var tempPasswd []byte var saltBytes []byte - // Salt is encoded as hex, because certain bytes won't translates well into - // it's string correlation. But only decode it when salt has a length of 32. - // Because we should tolerate hashing a password when a user has the old format - // of salt. + // 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) == 32 { var err error saltBytes, err = hex.DecodeString(salt) From 0fd2657838d6390448289547d207cf841326ef3f Mon Sep 17 00:00:00 2001 From: Gusted Date: Tue, 4 Jan 2022 12:37:06 +0100 Subject: [PATCH 5/8] Refactor comments --- models/migrations/v205.go | 5 ----- models/user/user.go | 9 ++++----- modules/util/util.go | 7 +++---- 3 files changed, 7 insertions(+), 14 deletions(-) diff --git a/models/migrations/v205.go b/models/migrations/v205.go index 50bfc236061c..22b4bac78d48 100644 --- a/models/migrations/v205.go +++ b/models/migrations/v205.go @@ -16,11 +16,6 @@ func migrateUserPasswordSalt(x *xorm.Engine) error { return nil } - // type User struct { - // Rands string `xorm:"VARCHAR(32)"` - // Salt string `xorm:"VARCHAR(32)"` - // } - if err := modifyColumn(x, "user", &schemas.Column{ Name: "rands", SQLType: schemas.SQLType{ diff --git a/models/user/user.go b/models/user/user.go index f5f356e0b17b..6e42ffdf6b53 100644 --- a/models/user/user.go +++ b/models/user/user.go @@ -365,7 +365,7 @@ func hashPassword(passwd, salt, algo string) (string, error) { // 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 + // We have to tolerate both here but Authenticate should // regenerate the Salt following a successful validation. if len(salt) == 32 { var err error @@ -528,10 +528,9 @@ func IsUserExist(uid int64, name string) (bool, error) { // GetUserSalt returns a random user salt token. // -// Note on the used size here. As of begin of 2022, it's recommended to use -// 64 bits of salt, certain recommended like NIST are already recommending to use -// 128 bits of salt. Given it's more leaning to 128 bits of salt, we're using this -// and taking a security margin. Again, why 16? 16 bytes = 16 * 8 = 128 bits. +// 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) func GetUserSalt() (string, error) { rBytes, err := util.RandomBytes(16) if err != nil { diff --git a/modules/util/util.go b/modules/util/util.go index e6bb1ba01904..83b26ee8bd86 100644 --- a/modules/util/util.go +++ b/modules/util/util.go @@ -162,10 +162,9 @@ func RandomString(length int64) (string, error) { return string(bytes), nil } -// RandomBytes generates `len` bytes -// This differ from RandomString, as RandomString is limited to each byte only -// having a maximum value of 63 instead of 255(max byte size) and thus decrease the -// security implications of it. +// 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) limit := int64(^uint8(0)) From 359ac9ec942414df98374f88fba292b89aa14800 Mon Sep 17 00:00:00 2001 From: Gusted Date: Tue, 4 Jan 2022 12:47:13 +0100 Subject: [PATCH 6/8] Simplify `RandomBytes` function --- modules/util/util.go | 11 ++--------- 1 file changed, 2 insertions(+), 9 deletions(-) diff --git a/modules/util/util.go b/modules/util/util.go index 83b26ee8bd86..c2117a6525b3 100644 --- a/modules/util/util.go +++ b/modules/util/util.go @@ -167,13 +167,6 @@ func RandomString(length int64) (string, error) { // a maximum value of 63 instead of 255(max byte size) func RandomBytes(length int64) ([]byte, error) { bytes := make([]byte, length) - limit := int64(^uint8(0)) - for i := range bytes { - num, err := RandomInt(limit) - if err != nil { - return []byte{}, err - } - bytes[i] = uint8(num) - } - return bytes, nil + _, err := rand.Read(bytes) + return bytes, err } From 491309de81d745074a0aa8a234ffbba7f7a45860 Mon Sep 17 00:00:00 2001 From: Gusted Date: Tue, 4 Jan 2022 13:35:59 +0100 Subject: [PATCH 7/8] Make MySQL happy --- models/migrations/v205.go | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/models/migrations/v205.go b/models/migrations/v205.go index 22b4bac78d48..755cb1024583 100644 --- a/models/migrations/v205.go +++ b/models/migrations/v205.go @@ -22,6 +22,8 @@ func migrateUserPasswordSalt(x *xorm.Engine) error { Name: "VARCHAR", }, Length: 32, + // MySQL will like us again. + Nullable: true, }); err != nil { return err } @@ -31,6 +33,7 @@ func migrateUserPasswordSalt(x *xorm.Engine) error { SQLType: schemas.SQLType{ Name: "VARCHAR", }, - Length: 32, + Length: 32, + Nullable: true, }) } From dcdc72dcc99d0c4ac4e61d3126b0a073f1f818da Mon Sep 17 00:00:00 2001 From: Gusted Date: Tue, 4 Jan 2022 14:46:35 +0100 Subject: [PATCH 8/8] Future-proof Co-authored-by: lafriks --- models/user/user.go | 15 ++++++++------- services/auth/source/db/authenticate.go | 6 +++--- 2 files changed, 11 insertions(+), 10 deletions(-) diff --git a/models/user/user.go b/models/user/user.go index 6e42ffdf6b53..8efd51c9be28 100644 --- a/models/user/user.go +++ b/models/user/user.go @@ -363,18 +363,18 @@ func hashPassword(passwd, salt, algo string) (string, error) { var saltBytes []byte // There are two formats for the Salt value: - // * The new format is a 32-byte hex-encoded string + // * 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) == 32 { + if len(salt) == 10 { + saltBytes = []byte(salt) + } else { var err error saltBytes, err = hex.DecodeString(salt) if err != nil { return "", err } - } else { - saltBytes = []byte(salt) } switch algo { @@ -526,13 +526,14 @@ func IsUserExist(uid int64, name string) (bool, error) { return isUserExist(db.GetEngine(db.DefaultContext), uid, name) } -// GetUserSalt returns a random user salt token. -// // 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) { - rBytes, err := util.RandomBytes(16) + rBytes, err := util.RandomBytes(SaltByteLength) if err != nil { return "", err } diff --git a/services/auth/source/db/authenticate.go b/services/auth/source/db/authenticate.go index 71c6dd410905..f062f66ae039 100644 --- a/services/auth/source/db/authenticate.go +++ b/services/auth/source/db/authenticate.go @@ -21,9 +21,9 @@ func Authenticate(user *user_model.User, login, password string) (*user_model.Us } // Update password hash if server password hash algorithm have changed - // Or update the password when the salt has a length of 10, this in order - // to migrate user's salts to a more secure salt. - if user.PasswdHashAlgo != setting.PasswordHashAlgo || len(user.Salt) == 10 { + // 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 }