From 74a0481586b7035bbe7a82f6e7e275cdd87d382a Mon Sep 17 00:00:00 2001 From: 6543 <6543@obermui.de> Date: Sun, 10 Jan 2021 19:05:18 +0100 Subject: [PATCH] [Refactor] Passwort Hash/Set (#14282) * move SaltGeneration into HashPasswort and rename it to what it does * Migration: Where Password is Valid with Empty String delete it * prohibit empty password hash * let SetPassword("") unset pwd stuff --- cmd/admin.go | 5 +- models/login_source.go | 6 +- models/migrations/migrations.go | 2 + models/migrations/v166.go | 115 ++++++++++++++++++++++++++++++++ models/user.go | 22 ++++-- models/user_test.go | 21 ++---- routers/admin/users.go | 5 +- routers/api/v1/admin/user.go | 5 +- routers/user/auth.go | 6 +- routers/user/setting/account.go | 3 +- 10 files changed, 158 insertions(+), 32 deletions(-) create mode 100644 models/migrations/v166.go diff --git a/cmd/admin.go b/cmd/admin.go index b4e0c3cb5..3ddf8eddf 100644 --- a/cmd/admin.go +++ b/cmd/admin.go @@ -349,12 +349,11 @@ func runChangePassword(c *cli.Context) error { if err != nil { return err } - if user.Salt, err = models.GetUserSalt(); err != nil { + if err = user.SetPassword(c.String("password")); err != nil { return err } - user.HashPassword(c.String("password")) - if err := models.UpdateUserCols(user, "passwd", "passwd_hash_algo", "salt"); err != nil { + if err = models.UpdateUserCols(user, "passwd", "passwd_hash_algo", "salt"); err != nil { return err } diff --git a/models/login_source.go b/models/login_source.go index f1941c3e7..d351f1286 100644 --- a/models/login_source.go +++ b/models/login_source.go @@ -771,8 +771,10 @@ func UserSignIn(username, password string) (*User, error) { // Update password hash if server password hash algorithm have changed if user.PasswdHashAlgo != setting.PasswordHashAlgo { - user.HashPassword(password) - if err := UpdateUserCols(user, "passwd", "passwd_hash_algo"); err != nil { + if err = user.SetPassword(password); err != nil { + return nil, err + } + if err = UpdateUserCols(user, "passwd", "passwd_hash_algo", "salt"); err != nil { return nil, err } } diff --git a/models/migrations/migrations.go b/models/migrations/migrations.go index 4f3d823b9..f62bba2a7 100644 --- a/models/migrations/migrations.go +++ b/models/migrations/migrations.go @@ -277,6 +277,8 @@ var migrations = []Migration{ NewMigration("Add scope and nonce columns to oauth2_grant table", addScopeAndNonceColumnsToOAuth2Grant), // v165 -> v166 NewMigration("Convert hook task type from char(16) to varchar(16) and trim the column", convertHookTaskTypeToVarcharAndTrim), + // v166 -> v167 + NewMigration("Where Password is Valid with Empty String delete it", recalculateUserEmptyPWD), } // GetCurrentDBVersion returns the current db version diff --git a/models/migrations/v166.go b/models/migrations/v166.go new file mode 100644 index 000000000..3d6cf4f57 --- /dev/null +++ b/models/migrations/v166.go @@ -0,0 +1,115 @@ +// Copyright 2021 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 ( + "crypto/sha256" + "fmt" + + "golang.org/x/crypto/argon2" + "golang.org/x/crypto/bcrypt" + "golang.org/x/crypto/pbkdf2" + "golang.org/x/crypto/scrypt" + "xorm.io/builder" + "xorm.io/xorm" +) + +func recalculateUserEmptyPWD(x *xorm.Engine) (err error) { + const ( + algoBcrypt = "bcrypt" + algoScrypt = "scrypt" + algoArgon2 = "argon2" + algoPbkdf2 = "pbkdf2" + ) + + type User struct { + ID int64 `xorm:"pk autoincr"` + Passwd string `xorm:"NOT NULL"` + PasswdHashAlgo string `xorm:"NOT NULL DEFAULT 'argon2'"` + MustChangePassword bool `xorm:"NOT NULL DEFAULT false"` + LoginType int + LoginName string + Type int + Salt string `xorm:"VARCHAR(10)"` + } + + // hashPassword hash password based on algo and salt + // state 461406070c + hashPassword := func(passwd, salt, algo string) string { + var tempPasswd []byte + + switch algo { + case algoBcrypt: + tempPasswd, _ = bcrypt.GenerateFromPassword([]byte(passwd), bcrypt.DefaultCost) + return string(tempPasswd) + case algoScrypt: + tempPasswd, _ = scrypt.Key([]byte(passwd), []byte(salt), 65536, 16, 2, 50) + case algoArgon2: + tempPasswd = argon2.IDKey([]byte(passwd), []byte(salt), 2, 65536, 8, 50) + case algoPbkdf2: + fallthrough + default: + tempPasswd = pbkdf2.Key([]byte(passwd), []byte(salt), 10000, 50, sha256.New) + } + + return fmt.Sprintf("%x", tempPasswd) + } + + // ValidatePassword checks if given password matches the one belongs to the user. + // state 461406070c, changed since it's not necessary to be time constant + ValidatePassword := func(u *User, passwd string) bool { + tempHash := hashPassword(passwd, u.Salt, u.PasswdHashAlgo) + + if u.PasswdHashAlgo != algoBcrypt && u.Passwd == tempHash { + return true + } + if u.PasswdHashAlgo == algoBcrypt && bcrypt.CompareHashAndPassword([]byte(u.Passwd), []byte(passwd)) == nil { + return true + } + return false + } + + sess := x.NewSession() + defer sess.Close() + + const batchSize = 100 + + for start := 0; ; start += batchSize { + users := make([]*User, 0, batchSize) + if err = sess.Limit(batchSize, start).Where(builder.Neq{"passwd": ""}, 0).Find(&users); err != nil { + return + } + if len(users) == 0 { + break + } + + if err = sess.Begin(); err != nil { + return + } + + for _, user := range users { + if ValidatePassword(user, "") { + user.Passwd = "" + user.Salt = "" + user.PasswdHashAlgo = "" + if _, err = sess.ID(user.ID).Cols("passwd", "salt", "passwd_hash_algo").Update(user); err != nil { + return err + } + } + } + + if err = sess.Commit(); err != nil { + return + } + } + + // delete salt and algo where password is empty + if _, err = sess.Where(builder.Eq{"passwd": ""}.And(builder.Neq{"salt": ""}.Or(builder.Neq{"passwd_hash_algo": ""}))). + Cols("salt", "passwd_hash_algo").Update(&User{}); err != nil { + return err + } + + return sess.Commit() +} diff --git a/models/user.go b/models/user.go index d3f1b16c2..dbd2372fc 100644 --- a/models/user.go +++ b/models/user.go @@ -395,10 +395,23 @@ func hashPassword(passwd, salt, algo string) string { return fmt.Sprintf("%x", tempPasswd) } -// HashPassword hashes a password using the algorithm defined in the config value of PASSWORD_HASH_ALGO. -func (u *User) HashPassword(passwd string) { +// SetPassword hashes a password using the algorithm defined in the config value of PASSWORD_HASH_ALGO +// change passwd, salt and passwd_hash_algo fields +func (u *User) SetPassword(passwd string) (err error) { + if len(passwd) == 0 { + u.Passwd = "" + u.Salt = "" + u.PasswdHashAlgo = "" + return nil + } + + if u.Salt, err = GetUserSalt(); 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. @@ -416,7 +429,7 @@ func (u *User) ValidatePassword(passwd string) bool { // IsPasswordSet checks if the password is set or left empty func (u *User) IsPasswordSet() bool { - return !u.ValidatePassword("") + return len(u.Passwd) != 0 } // IsOrganization returns true if user is actually a organization. @@ -826,10 +839,9 @@ func CreateUser(u *User) (err error) { if u.Rands, err = GetUserSalt(); err != nil { return err } - if u.Salt, err = GetUserSalt(); err != nil { + if err = u.SetPassword(u.Passwd); err != nil { return err } - u.HashPassword(u.Passwd) u.AllowCreateOrganization = setting.Service.DefaultAllowCreateOrganization && !setting.Admin.DisableRegularOrgCreation u.EmailNotificationsPreference = setting.Admin.DefaultEmailNotification u.MaxRepoCreation = -1 diff --git a/models/user_test.go b/models/user_test.go index 224acd5f3..69cb21b97 100644 --- a/models/user_test.go +++ b/models/user_test.go @@ -220,8 +220,7 @@ func TestEmailNotificationPreferences(t *testing.T) { func TestHashPasswordDeterministic(t *testing.T) { b := make([]byte, 16) - rand.Read(b) - u := &User{Salt: string(b)} + u := &User{} algos := []string{"argon2", "pbkdf2", "scrypt", "bcrypt"} for j := 0; j < len(algos); j++ { u.PasswdHashAlgo = algos[j] @@ -231,19 +230,15 @@ func TestHashPasswordDeterministic(t *testing.T) { pass := string(b) // save the current password in the user - hash it and store the result - u.HashPassword(pass) + u.SetPassword(pass) r1 := u.Passwd // run again - u.HashPassword(pass) + u.SetPassword(pass) r2 := u.Passwd - // assert equal (given the same salt+pass, the same result is produced) except bcrypt - if u.PasswdHashAlgo == "bcrypt" { - assert.NotEqual(t, r1, r2) - } else { - assert.Equal(t, r1, r2) - } + assert.NotEqual(t, r1, r2) + assert.True(t, u.ValidatePassword(pass)) } } } @@ -252,12 +247,10 @@ func BenchmarkHashPassword(b *testing.B) { // BenchmarkHashPassword ensures that it takes a reasonable amount of time // to hash a password - in order to protect from brute-force attacks. pass := "password1337" - bs := make([]byte, 16) - rand.Read(bs) - u := &User{Salt: string(bs), Passwd: pass} + u := &User{Passwd: pass} b.ResetTimer() for i := 0; i < b.N; i++ { - u.HashPassword(pass) + u.SetPassword(pass) } } diff --git a/routers/admin/users.go b/routers/admin/users.go index 8a848b1f9..9fe5c3ef7 100644 --- a/routers/admin/users.go +++ b/routers/admin/users.go @@ -267,7 +267,10 @@ func EditUserPost(ctx *context.Context, form auth.AdminEditUserForm) { ctx.ServerError("UpdateUser", err) return } - u.HashPassword(form.Password) + if err = u.SetPassword(form.Password); err != nil { + ctx.InternalServerError(err) + return + } } if len(form.UserName) != 0 && u.Name != form.UserName { diff --git a/routers/api/v1/admin/user.go b/routers/api/v1/admin/user.go index 93b28c217..670baf020 100644 --- a/routers/api/v1/admin/user.go +++ b/routers/api/v1/admin/user.go @@ -174,7 +174,10 @@ func EditUser(ctx *context.APIContext, form api.EditUserOption) { ctx.Error(http.StatusInternalServerError, "UpdateUser", err) return } - u.HashPassword(form.Password) + if err = u.SetPassword(form.Password); err != nil { + ctx.InternalServerError(err) + return + } } if form.MustChangePassword != nil { diff --git a/routers/user/auth.go b/routers/user/auth.go index 540a0d2f1..bce801847 100644 --- a/routers/user/auth.go +++ b/routers/user/auth.go @@ -1517,11 +1517,10 @@ func ResetPasswdPost(ctx *context.Context) { ctx.ServerError("UpdateUser", err) return } - if u.Salt, err = models.GetUserSalt(); err != nil { + if err = u.SetPassword(passwd); err != nil { ctx.ServerError("UpdateUser", err) return } - u.HashPassword(passwd) u.MustChangePassword = false if err := models.UpdateUserCols(u, "must_change_password", "passwd", "passwd_hash_algo", "rands", "salt"); err != nil { ctx.ServerError("UpdateUser", err) @@ -1591,12 +1590,11 @@ func MustChangePasswordPost(ctx *context.Context, cpt *captcha.Captcha, form aut } var err error - if u.Salt, err = models.GetUserSalt(); err != nil { + if err = u.SetPassword(form.Password); err != nil { ctx.ServerError("UpdateUser", err) return } - u.HashPassword(form.Password) u.MustChangePassword = false if err := models.UpdateUserCols(u, "must_change_password", "passwd", "passwd_hash_algo", "salt"); err != nil { diff --git a/routers/user/setting/account.go b/routers/user/setting/account.go index 4fb2e4be4..ca9b5b3c3 100644 --- a/routers/user/setting/account.go +++ b/routers/user/setting/account.go @@ -63,11 +63,10 @@ func AccountPost(ctx *context.Context, form auth.ChangePasswordForm) { ctx.Flash.Error(errMsg) } else { var err error - if ctx.User.Salt, err = models.GetUserSalt(); err != nil { + if err = ctx.User.SetPassword(form.Password); err != nil { ctx.ServerError("UpdateUser", err) return } - ctx.User.HashPassword(form.Password) if err := models.UpdateUserCols(ctx.User, "salt", "passwd_hash_algo", "passwd"); err != nil { ctx.ServerError("UpdateUser", err) return