Mercurial > gemma
changeset 535:da5f47a0941c
Password reset: Reduce the risk of timing attacks and being a user oracle when requesting resets.
author | Sascha L. Teichmann <sascha.teichmann@intevation.de> |
---|---|
date | Wed, 29 Aug 2018 10:34:46 +0200 |
parents | f96d18e53369 |
children | d9dbb6139760 |
files | pkg/controllers/pwreset.go |
diffstat | 1 files changed, 44 insertions(+), 39 deletions(-) [+] |
line wrap: on
line diff
--- a/pkg/controllers/pwreset.go Tue Aug 28 14:38:18 2018 +0200 +++ b/pkg/controllers/pwreset.go Wed Aug 29 10:34:46 2018 +0200 @@ -5,6 +5,7 @@ "context" "database/sql" "encoding/hex" + "errors" "log" "net/http" "os/exec" @@ -60,6 +61,13 @@ const pwResetRole = "sys_admin" var ( + errTooMuchPasswordResets = errors.New("Too many password resets") + errTooMuchPasswordResetsPerUser = errors.New("Too many password resets per user") + errNoSuchUser = errors.New("User does not exist") + errInvalidUser = errors.New("Invalid user") +) + +var ( passwordResetRequestMailTmpl = template.Must( template.New("request").Parse(`You or someone else has requested a password change for your account {{ .User }} on @@ -171,34 +179,17 @@ return common.RandomString(passwordLength) } -func passwordResetRequest( - input interface{}, - req *http.Request, - _ *sql.Conn, -) (jr JSONResult, err error) { - - user := input.(*models.PWResetUser) +func backgroundRequest(https, host string, user *models.PWResetUser) error { if user.User == "" { - err = JSONError{http.StatusBadRequest, "Invalid user name"} - return + return errInvalidUser } var hash, email string - ctx := req.Context() - - // FIXME, we need to always answer with a neutral messages - // to avoid becoming an oracle about which user exists to third parties. + ctx := context.Background() - // Error messages need to be logged instead of being send to the user. - // - // const neutralMessage = "If this account exists, a reset link will be mailed." - - // FIXME responding should be done it a goroutine of its own so its - // executing time is constant (to avoid becoming an oracle over the - // response time). - if err = auth.RunAs( + if err := auth.RunAs( pwResetRole, ctx, func(conn *sql.Conn) error { @@ -210,10 +201,7 @@ // Limit total number of password requests. if count >= maxPasswordResets { - return JSONError{ - Code: http.StatusServiceUnavailable, - Message: "Too many requests for the server, please notify the administrator.", - } + return errTooMuchPasswordResets } err := conn.QueryRowContext(ctx, userExistsSQL, user.User).Scan(&email) @@ -221,7 +209,7 @@ switch { case err == sql.ErrNoRows: //FIXME change to logging - return JSONError{http.StatusNotFound, "User does not exist."} + return errNoSuchUser case err != nil: //FIXME change to logging return err @@ -234,26 +222,43 @@ // Limit requests per user if count >= maxPasswordRequestsPerUser { - //FIXME change to logging - return JSONError{ - Code: http.StatusServiceUnavailable, - Message: "Too much password reset requests for user", - } + return errTooMuchPasswordResetsPerUser } hash = generateHash() _, err = conn.ExecContext(ctx, insertRequestSQL, hash, user.User) return err - }); err == nil { - body := requestMessageBody(useHTTPS(req), user.User, hash, req.Host) + }, + ); err != nil { + return err + } + + body := requestMessageBody(https, user.User, hash, host) + + return misc.SendMail(email, "Password Reset Link", body) +} + +func passwordResetRequest( + input interface{}, + req *http.Request, + _ *sql.Conn, +) (jr JSONResult, err error) { - if err = misc.SendMail(email, "Password Reset Link", body); err == nil { - //FIXME change to logging - jr.Result = &struct { - SendTo string `json:"send-to"` - }{email} + // We do the checks and the emailing in background + // no reduce the risks of timing attacks. + go func(https, host string, user *models.PWResetUser) { + if err := backgroundRequest(https, host, user); err != nil { + log.Printf("error: %v\n", err) } - } + }(useHTTPS(req), req.Host, input.(*models.PWResetUser)) + + // Send a neutral message to avoid being an user oracle. + const neutralMessage = "If this account exists, a reset link will be mailed." + + jr.Result = &struct { + Message string `json:"message"` + }{neutralMessage} + return }