Fix session persistence security hole in totp/sms

- Reorder the lookups to ensure CurrentUser is always looked up before
  any temporary pending PIDs.
- See changelog for more details

CHANGELOG.md

@@ -3,6 +3,16 @@
 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.1.1] - 2018-12-10
+
+### Security
+
+- Fix a bug with the 2fa code where a client that failed to log in to a user
+  account got SessionTOTPPendingPID set to that user's pid. That user's pid
+  was used as lookup for verify() method in totp/sms methods before current
+  user was looked at meaning the logged in user could remove 2fa from the
+  other user's account because of the lookup order.
+
 ## [2.1.0] - 2018-10-28
 
 ### Added

otp/twofactor/sms2fa/sms.go

@@ -263,19 +263,21 @@ func (s *SMSValidator) Get(w http.ResponseWriter, r *http.Request) error {
 // Post receives a code in the body and validates it, if the code is
 // missing then it sends the code to the user (rate-limited).
 func (s *SMSValidator) Post(w http.ResponseWriter, r *http.Request) error {
-	var abUser authboss.User
-	var err error
-
 	// Get the user, they're either logged in and CurrentUser works, or they're
 	// in the middle of logging in and SMSPendingPID is set.
-	if pid, ok := authboss.GetSession(r, SessionSMSPendingPID); ok && len(pid) != 0 {
-		abUser, err = s.Authboss.Config.Storage.Server.Load(r.Context(), pid)
-	} else {
-		abUser, err = s.CurrentUser(r)
+	// Ensure we always look up CurrentUser first or session persistence
+	// attacks can be performed.
+	abUser, err := s.Authboss.CurrentUser(r)
+	if err == authboss.ErrUserNotFound {
+		pid, ok := authboss.GetSession(r, SessionSMSPendingPID)
+		if ok && len(pid) != 0 {
+			abUser, err = s.Authboss.Config.Storage.Server.Load(r.Context(), pid)
+		}
 	}
 	if err != nil {
 		return err
 	}
+
 	user := abUser.(User)
 
 	validator, err := s.Authboss.Config.Core.BodyReader.Read(s.Page, r)

otp/twofactor/totp2fa/totp.go

@@ -359,13 +359,16 @@ func (t *TOTP) PostValidate(w http.ResponseWriter, r *http.Request) error {
 
 func (t *TOTP) validate(r *http.Request) (User, bool, error) {
 	logger := t.RequestLogger(r)
-	var abUser authboss.User
-	var err error
 
-	if pid, ok := authboss.GetSession(r, SessionTOTPPendingPID); ok && len(pid) != 0 {
-		abUser, err = t.Authboss.Config.Storage.Server.Load(r.Context(), pid)
-	} else {
-		abUser, err = t.CurrentUser(r)
+	// Look up CurrentUser first, otherwise session persistence can allow
+	// a previous login attempt's user to be recalled here by a logged in
+	// user for 2fa removal and verification.
+	abUser, err := t.CurrentUser(r)
+	if err == authboss.ErrUserNotFound {
+		pid, ok := authboss.GetSession(r, SessionTOTPPendingPID)
+		if ok && len(pid) != 0 {
+			abUser, err = t.Authboss.Config.Storage.Server.Load(r.Context(), pid)
+		}
 	}
 	if err != nil {
 		return nil, false, err