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
 }