diff --git a/CHANGELOG.md b/CHANGELOG.md index 756eede..54ec965 100644 --- a/CHANGELOG.md +++ b/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 diff --git a/lock/lock.go b/lock/lock.go index 0f8d5ec..86c115d 100644 --- a/lock/lock.go +++ b/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,27 +59,33 @@ 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()) @@ -102,7 +93,7 @@ func (l *Lock) AfterAuthFail(w http.ResponseWriter, r *http.Request, handled boo return false, err } - if !nowLocked { + if !IsLocked(lu) { return false, nil } diff --git a/lock/lock_test.go b/lock/lock_test.go index 9dcc55d..0d24324 100644 --- a/lock/lock_test.go +++ b/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))