Fix bug in lock code

Author: aarondl

CHANGELOG.md

@@ -3,6 +3,19 @@
 The format is based on [Keep a Changelog](http://keepachangelog.com/en/1.0.0/)
 and this project adheres to [Semantic Versioning](http://semver.org/spec/v2.0.0.html).
 
+## [2.4.1] - 2020-05-18
+
+### Fixed
+
+Fix a security issue where a user could brute-force a password based on
+differing responses that are returned from the site when the incorrect password
+is entered versus the correct password.
+
+This comes with a slight change in behavior to minimize differences between the
+code paths of a correct vs incorrect password: The "attempt" time is always
+bumped in the DB no matter if it was the right or wrong password when being
+rejected for locking.
+
 ## [2.4.0] - 2020-02-07
 
 ### Added

lock/lock.go

@@ -39,22 +39,7 @@ func (l *Lock) Init(ab *authboss.Authboss) error {
 
 // BeforeAuth ensures the account is not locked.
 func (l *Lock) BeforeAuth(w http.ResponseWriter, r *http.Request, handled bool) (bool, error) {
-	user, err := l.Authboss.CurrentUser(r)
-	if err != nil {
-		return false, err
-	}
-
-	lu := authboss.MustBeLockable(user)
-	if !IsLocked(lu) {
-		return false, nil
-	}
-
-	ro := authboss.RedirectOptions{
-		Code:         http.StatusTemporaryRedirect,
-		Failure:      "Your account is locked. Please contact the administrator.",
-		RedirectPath: l.Authboss.Config.Paths.LockNotOK,
-	}
-	return true, l.Authboss.Config.Core.Redirector.Redirect(w, r, ro)
+	return l.updateLockedState(w, r, true)
 }
 
 // AfterAuthSuccess resets the attempt number field.
@@ -74,35 +59,41 @@ func (l *Lock) AfterAuthSuccess(w http.ResponseWriter, r *http.Request, handled
 // AfterAuthFail adjusts the attempt number and time negatively
 // and locks the user if they're beyond limits.
 func (l *Lock) AfterAuthFail(w http.ResponseWriter, r *http.Request, handled bool) (bool, error) {
+	return l.updateLockedState(w, r, false)
+}
+
+// updateLockedState exists to minimize any differences between a success and
+// a failure path in the case where a correct/incorrect password is entered
+func (l *Lock) updateLockedState(w http.ResponseWriter, r *http.Request, wasCorrectPassword bool) (bool, error) {
 	user, err := l.Authboss.CurrentUser(r)
 	if err != nil {
 		return false, err
 	}
 
+	// Fetch things
 	lu := authboss.MustBeLockable(user)
 	last := lu.GetLastAttempt()
 	attempts := lu.GetAttemptCount()
 	attempts++
 
-	nowLocked := false
+	if !wasCorrectPassword {
+		if time.Now().UTC().Sub(last) <= l.Modules.LockWindow {
+			if attempts >= l.Modules.LockAfter {
+				lu.PutLocked(time.Now().UTC().Add(l.Modules.LockDuration))
+			}
 
-	if time.Now().UTC().Sub(last) <= l.Modules.LockWindow {
-		if attempts >= l.Modules.LockAfter {
-			lu.PutLocked(time.Now().UTC().Add(l.Modules.LockDuration))
-			nowLocked = true
+			lu.PutAttemptCount(attempts)
+		} else {
+			lu.PutAttemptCount(1)
 		}
-
-		lu.PutAttemptCount(attempts)
-	} else {
-		lu.PutAttemptCount(1)
 	}
 	lu.PutLastAttempt(time.Now().UTC())
 
 	if err := l.Authboss.Config.Storage.Server.Save(r.Context(), lu); err != nil {
 		return false, err
 	}
 
-	if !nowLocked {
+	if !IsLocked(lu) {
 		return false, nil
 	}
 

lock/lock_test.go

@@ -72,8 +72,10 @@ func TestBeforeAuthAllow(t *testing.T) {
 	harness := testSetup()
 
 	user := &mocks.User{
+		Email:  "test@test.com",
 		Locked: time.Time{},
 	}
+	harness.storer.Users["test@test.com"] = user
 
 	r := mocks.Request("GET")
 	r = r.WithContext(context.WithValue(r.Context(), authboss.CTXKeyUser, user))
@@ -94,8 +96,10 @@ func TestBeforeAuthDisallow(t *testing.T) {
 	harness := testSetup()
 
 	user := &mocks.User{
+		Email:  "test@test.com",
 		Locked: time.Now().UTC().Add(time.Hour),
 	}
+	harness.storer.Users["test@test.com"] = user
 
 	r := mocks.Request("GET")
 	r = r.WithContext(context.WithValue(r.Context(), authboss.CTXKeyUser, user))