[MM-57942] Fix a panic on password is too long (#27449)

* return error from bcrypt, handle gracefully; remove dead code

* linting

* linting

* i18n

* fix test

* fill out translations
This commit is contained in:
Christopher Poile 2024-07-03 17:58:26 -04:00 committed by GitHub
parent 5d2bf1ea1c
commit cc5e87ae24
No known key found for this signature in database
GPG Key ID: B5690EEEBB952194
8 changed files with 54 additions and 30 deletions

View File

@ -1445,7 +1445,11 @@ func (a *App) UpdatePassword(rctx request.CTX, user *model.User, newPassword str
return model.NewAppError("UpdatePassword", "api.user.update_password.failed.app_error", nil, "", http.StatusInternalServerError)
}
hashedPassword := model.HashPassword(newPassword)
hashedPassword, err := model.HashPassword(newPassword)
if err != nil {
// can't be password length (checked in IsPasswordValid)
return model.NewAppError("UpdatePassword", "api.user.update_password.password_hash.app_error", nil, "user_id="+user.Id, http.StatusInternalServerError).Wrap(err)
}
if err := a.Srv().Store().User().UpdatePassword(user.Id, hashedPassword); err != nil {
return model.NewAppError("UpdatePassword", "api.user.update_password.failed.app_error", nil, "", http.StatusInternalServerError).Wrap(err)

View File

@ -20,16 +20,6 @@ func CheckUserPassword(user *model.User, password string) error {
return nil
}
// HashPassword generates a hash using the bcrypt.GenerateFromPassword
func HashPassword(password string) string {
hash, err := bcrypt.GenerateFromPassword([]byte(password), 10)
if err != nil {
panic(err)
}
return string(hash)
}
func ComparePassword(hash string, password string) error {
if password == "" || hash == "" {
return errors.New("empty password or hash")

View File

@ -13,13 +13,6 @@ import (
"github.com/mattermost/mattermost/server/public/model"
)
func TestComparePassword(t *testing.T) {
hash := HashPassword("Test")
assert.NoError(t, ComparePassword(hash, "Test"), "Passwords don't match")
assert.Error(t, ComparePassword(hash, "Test2"), "Passwords should not have matched")
}
func TestIsPasswordValidWithSettings(t *testing.T) {
for name, tc := range map[string]struct {
Password string

View File

@ -112,7 +112,9 @@ func (us SqlUserStore) Save(rctx request.CTX, user *model.User) (*model.User, er
return nil, store.NewErrInvalidInput("User", "id", user.Id)
}
user.PreSave()
if err := user.PreSave(); err != nil {
return nil, err
}
if err := user.IsValid(); err != nil {
return nil, err
}

View File

@ -12,6 +12,8 @@ import (
"testing"
"time"
"golang.org/x/crypto/bcrypt"
"github.com/stretchr/testify/assert"
"github.com/stretchr/testify/require"
@ -2294,7 +2296,11 @@ func testUserStoreUpdatePassword(t *testing.T, rctx request.CTX, ss store.Store)
_, nErr := ss.Team().SaveMember(rctx, &model.TeamMember{TeamId: teamId, UserId: u1.Id}, -1)
require.NoError(t, nErr)
hashedPassword := model.HashPassword("newpwd")
_, err = model.HashPassword(strings.Repeat("1234567890", 8))
require.ErrorIs(t, err, bcrypt.ErrPasswordTooLong)
hashedPassword, err := model.HashPassword("newpwd")
require.NoError(t, err)
err = ss.User().UpdatePassword(u1.Id, hashedPassword)
require.NoError(t, err)

View File

@ -4146,6 +4146,10 @@
"id": "api.user.update_password.oauth.app_error",
"translation": "Update password failed because the user is logged in through an OAuth service."
},
{
"id": "api.user.update_password.password_hash.app_error",
"translation": "There was an internal error saving the password."
},
{
"id": "api.user.update_password.user_and_hashed.app_error",
"translation": "Only system administrators can set already-hashed passwords."
@ -9758,6 +9762,14 @@
"id": "model.user.is_valid.username.app_error",
"translation": "Username must begin with a letter, and contain between 3 to 22 lowercase characters made up of numbers, letters, and the symbols \".\", \"-\", and \"_\"."
},
{
"id": "model.user.pre_save.password_hash.app_error",
"translation": "There was an internal error saving the password."
},
{
"id": "model.user.pre_save.password_too_long.app_error",
"translation": "Your password must contain no more than 72 characters."
},
{
"id": "model.user_access_token.is_valid.description.app_error",
"translation": "Invalid description, must be 255 or less characters."

View File

@ -14,6 +14,8 @@ import (
"time"
"unicode/utf8"
"github.com/pkg/errors"
"golang.org/x/crypto/bcrypt"
"golang.org/x/text/language"
@ -378,10 +380,6 @@ func (u *User) IsValid() *AppError {
return InvalidUserError("auth_data_pwd", u.Id, *u.AuthData)
}
if len(u.Password) > UserPasswordMaxLength {
return InvalidUserError("password_limit", u.Id, "")
}
if !IsValidLocale(u.Locale) {
return InvalidUserError("locale", u.Id, u.Locale)
}
@ -430,7 +428,7 @@ func NormalizeEmail(email string) string {
// PreSave will set the Id and Username if missing. It will also fill
// in the CreateAt, UpdateAt times. It will also hash the password. It should
// be run before saving the user to the db.
func (u *User) PreSave() {
func (u *User) PreSave() *AppError {
if u.Id == "" {
u.Id = NewId()
}
@ -477,7 +475,15 @@ func (u *User) PreSave() {
}
if u.Password != "" {
u.Password = HashPassword(u.Password)
hashed, err := HashPassword(u.Password)
if errors.Is(err, bcrypt.ErrPasswordTooLong) {
return NewAppError("User.PreSave", "model.user.pre_save.password_too_long.app_error",
nil, "user_id="+u.Id, http.StatusBadRequest).Wrap(err)
} else if err != nil {
return NewAppError("User.PreSave", "model.user.pre_save.password_hash.app_error",
nil, "user_id="+u.Id, http.StatusBadRequest).Wrap(err)
}
u.Password = hashed
}
cs := u.GetCustomStatus()
@ -485,6 +491,8 @@ func (u *User) PreSave() {
cs.PreSave()
u.SetCustomStatus(cs)
}
return nil
}
// PreUpdate should be run before updating the user in the db.
@ -928,13 +936,13 @@ func (u *UserPatch) SetField(fieldName string, fieldValue string) {
}
// HashPassword generates a hash using the bcrypt.GenerateFromPassword
func HashPassword(password string) string {
func HashPassword(password string) (string, error) {
hash, err := bcrypt.GenerateFromPassword([]byte(password), 10)
if err != nil {
panic(err)
return "", err
}
return string(hash)
return string(hash), nil
}
var validUsernameChars = regexp.MustCompile(`^[a-z0-9\.\-_]+$`)

View File

@ -9,6 +9,8 @@ import (
"strings"
"testing"
"golang.org/x/crypto/bcrypt"
"github.com/stretchr/testify/assert"
"github.com/stretchr/testify/require"
@ -47,12 +49,19 @@ func TestUserDeepCopy(t *testing.T) {
func TestUserPreSave(t *testing.T) {
user := User{Password: "test"}
user.PreSave()
err := user.PreSave()
require.Nil(t, err)
user.Etag(true, true)
assert.NotNil(t, user.Timezone, "Timezone is nil")
assert.Equal(t, user.Timezone["useAutomaticTimezone"], "true", "Timezone is not set to default")
}
func TestUserPreSavePwdTooLong(t *testing.T) {
user := User{Password: strings.Repeat("1234567890", 8)}
err := user.PreSave()
assert.ErrorIs(t, err, bcrypt.ErrPasswordTooLong)
}
func TestUserPreUpdate(t *testing.T) {
user := User{Password: "test"}
user.PreUpdate()