From d435c50df312a2590aca8713f11964399b3a9865 Mon Sep 17 00:00:00 2001 From: kolaente Date: Thu, 9 Apr 2026 17:16:15 +0200 Subject: [PATCH] fix(security): persist TOTP lockout across login rollback The failed-TOTP handler shared the login request's xorm session, and the login handler rolled that session back after a failed login. The status change to StatusAccountLocked was silently discarded, so the account was never locked regardless of how many failed TOTP attempts arrived. HandleFailedTOTPAuth now opens its own session and commits independently of the caller. The login handler rolls back its session before invoking the handler so the lockout write can acquire a write lock on SQLite shared-cache. Also handles the Redis keyvalue backend returning the attempt counter as a string instead of int64, which would have prevented the lockout path from ever running on Redis. See GHSA-fgfv-pv97-6cmj. --- pkg/routes/api/v1/login.go | 10 ++++-- pkg/user/totp.go | 73 ++++++++++++++++++++++++++------------ 2 files changed, 58 insertions(+), 25 deletions(-) diff --git a/pkg/routes/api/v1/login.go b/pkg/routes/api/v1/login.go index 7856534b9..e13074bb2 100644 --- a/pkg/routes/api/v1/login.go +++ b/pkg/routes/api/v1/login.go @@ -92,10 +92,14 @@ func Login(c *echo.Context) (err error) { Passcode: u.TOTPPasscode, }) if err != nil { - if user2.IsErrInvalidTOTPPasscode(err) { - user2.HandleFailedTOTPAuth(s, user) - } + // Rollback before HandleFailedTOTPAuth so its dedicated session + // can acquire a write lock on SQLite shared-cache. The lockout + // write is decoupled from this handler's transaction — see + // GHSA-fgfv-pv97-6cmj. _ = s.Rollback() + if user2.IsErrInvalidTOTPPasscode(err) { + user2.HandleFailedTOTPAuth(user) + } return err } } diff --git a/pkg/user/totp.go b/pkg/user/totp.go index bf50f5dc5..66abb813c 100644 --- a/pkg/user/totp.go +++ b/pkg/user/totp.go @@ -23,6 +23,7 @@ import ( "time" "code.vikunja.io/api/pkg/config" + "code.vikunja.io/api/pkg/db" "code.vikunja.io/api/pkg/log" "code.vikunja.io/api/pkg/modules/keyvalue" "code.vikunja.io/api/pkg/notifications" @@ -197,30 +198,53 @@ func GetTOTPQrCodeForUser(s *xorm.Session, user *User) (qrcode image.Image, err return key.Image(300, 300) } -// HandleFailedTOTPAuth handles informing the user of failed TOTP attempts and blocking the account after 10 attempts -func HandleFailedTOTPAuth(s *xorm.Session, user *User) { +// HandleFailedTOTPAuth records a failed TOTP attempt and locks the account +// after 10 consecutive failures. +// +// Must not share the caller's session: the login handler rolls its session +// back on auth failure, which would discard the lockout write +// (GHSA-fgfv-pv97-6cmj). Opens its own session and commits independently. +func HandleFailedTOTPAuth(user *User) { log.Errorf("Invalid TOTP credentials provided for user %d", user.ID) key := user.GetFailedTOTPAttemptsKey() - err := keyvalue.IncrBy(key, 1) - if err != nil { + if err := keyvalue.IncrBy(key, 1); err != nil { log.Errorf("Could not increase failed TOTP attempts for user %d: %s", user.ID, err) return } a, _, err := keyvalue.Get(key) if err != nil { - log.Errorf("Could get failed TOTP attempts for user %d: %s", user.ID, err) + log.Errorf("Could not get failed TOTP attempts for user %d: %s", user.ID, err) return } - attempts := a.(int64) - - if attempts == 3 { - err = notifications.Notify(user, &InvalidTOTPNotification{User: user}, s) - if err != nil { - log.Errorf("Could not send failed TOTP notification to user %d: %s", user.ID, err) + // Redis backend returns the counter as a string; in-memory as int64. + attempts, ok := a.(int64) + if !ok { + attemptsStr, ok := a.(string) + if !ok { + log.Errorf("Unexpected type for failed TOTP attempts for user %d: %T", user.ID, a) return } + attempts, err = strconv.ParseInt(attemptsStr, 10, 64) + if err != nil { + log.Errorf("Could not convert failed TOTP attempts to int64 for user %d: %v, value: %s", user.ID, err, attemptsStr) + return + } + } + + if attempts == 3 { + s := db.NewSession() + defer s.Close() + if err := notifications.Notify(user, &InvalidTOTPNotification{User: user}, s); err != nil { + log.Errorf("Could not send failed TOTP notification to user %d: %s", user.ID, err) + _ = s.Rollback() + return + } + if err := s.Commit(); err != nil { + log.Errorf("Could not commit failed TOTP notification for user %d: %s", user.ID, err) + } + return } if attempts < 10 { @@ -228,20 +252,25 @@ func HandleFailedTOTPAuth(s *xorm.Session, user *User) { } log.Infof("Blocking user account %d after 10 failed TOTP password attempts", user.ID) - err = RequestUserPasswordResetToken(s, user) - if err != nil { - log.Errorf("Could not reset password of user %d after 10 failed TOTP attempts: %s", user.ID, err) + s := db.NewSession() + defer s.Close() + + if err := RequestUserPasswordResetToken(s, user); err != nil { + log.Errorf("Could not issue password reset token for user %d after 10 failed TOTP attempts: %s", user.ID, err) + _ = s.Rollback() return } - err = notifications.Notify(user, &PasswordAccountLockedAfterInvalidTOTPNotification{ - User: user, - }, s) - if err != nil { - log.Errorf("Could send password information mail to user %d after 10 failed TOTP attempts: %s", user.ID, err) + if err := notifications.Notify(user, &PasswordAccountLockedAfterInvalidTOTPNotification{User: user}, s); err != nil { + log.Errorf("Could not send password information mail to user %d after 10 failed TOTP attempts: %s", user.ID, err) + _ = s.Rollback() return } - err = user.SetStatus(s, StatusAccountLocked) - if err != nil { - log.Errorf("Could not disable user %d: %s", user.ID, err) + if err := user.SetStatus(s, StatusAccountLocked); err != nil { + log.Errorf("Could not lock user %d: %s", user.ID, err) + _ = s.Rollback() + return + } + if err := s.Commit(); err != nil { + log.Errorf("Could not commit lockout for user %d: %s", user.ID, err) } }