From 2f1761e3fe059d18ea40541df80778a5cccf2319 Mon Sep 17 00:00:00 2001 From: wxiaoguang Date: Mon, 12 Oct 2026 16:44:52 +0800 Subject: [PATCH] fix(auth): avoid "compromised account" false-alarm when login with remember-me token (#39686) 1. Remember the last token that has been rotated and skip its validation 2. Don't alarm when the token mismatch occurs in a short window --- modelmigration/migrations.go | 1 + modelmigration/v29/v358.go | 25 +++++++++++++++ models/auth/auth_token.go | 10 ++++-- routers/web/auth/auth.go | 52 ++++++++++++++++++-------------- routers/web/auth/auth_test.go | 20 ++++++++++++ services/auth/auth_token.go | 28 +++++++++++------ services/auth/auth_token_test.go | 18 ++++++++++- 7 files changed, 118 insertions(+), 36 deletions(-) create mode 100644 modelmigration/v29/v358.go diff --git a/modelmigration/migrations.go b/modelmigration/migrations.go index c90b3425ee3..98321e2de16 100644 --- a/modelmigration/migrations.go +++ b/modelmigration/migrations.go @@ -434,6 +434,7 @@ func prepareMigrationTasks() []*migration { newMigration(356, "Add index on action_run commit_sha", v29.AddActionRunCommitSHAIndex), newMigration(357, "Normalize legacy team authorize values", v29.NormalizeLegacyTeamAuthorize), + newMigration(358, "Track previous remember-me token hashes", v29.AddPreviousAuthTokenHash), } return preparedMigrations } diff --git a/modelmigration/v29/v358.go b/modelmigration/v29/v358.go new file mode 100644 index 00000000000..6295215e939 --- /dev/null +++ b/modelmigration/v29/v358.go @@ -0,0 +1,25 @@ +// Copyright 2026 The Gitea Authors. All rights reserved. +// SPDX-License-Identifier: MIT + +package v29 + +import ( + "context" + + "gitea.dev/modelmigration/base" + "gitea.dev/modules/timeutil" + + "xorm.io/xorm" +) + +func AddPreviousAuthTokenHash(_ context.Context, x base.EngineMigration) error { + type AuthToken struct { + PreviousTokenHash string + RotatedUnix timeutil.TimeStamp + } + _, err := x.SyncWithOptions(xorm.SyncOptions{ + IgnoreDropIndices: true, + IgnoreConstrains: true, + }, new(AuthToken)) + return err +} diff --git a/models/auth/auth_token.go b/models/auth/auth_token.go index 0eaf700c2e2..b570061e6e6 100644 --- a/models/auth/auth_token.go +++ b/models/auth/auth_token.go @@ -20,6 +20,9 @@ type AuthToken struct { //nolint:revive // export stutter TokenHash string UserID int64 `xorm:"INDEX"` ExpiresUnix timeutil.TimeStamp `xorm:"INDEX"` + + PreviousTokenHash string // Identifies stale cookies without authenticating them. + RotatedUnix timeutil.TimeStamp // Bounds the grace window for stale cookies. } func init() { @@ -44,9 +47,10 @@ func GetAuthTokenByID(ctx context.Context, id string) (*AuthToken, error) { return at, nil } -func UpdateAuthTokenByID(ctx context.Context, t *AuthToken) error { - _, err := db.GetEngine(ctx).ID(t.ID).Cols("token_hash", "expires_unix").Update(t) - return err +func UpdateAuthTokenByID(ctx context.Context, t *AuthToken, oldHash string) (bool, error) { + affected, err := db.GetEngine(ctx).ID(t.ID).Where("token_hash = ?", oldHash). + Cols("token_hash", "expires_unix", "previous_token_hash", "rotated_unix").Update(t) + return affected == 1, err } func DeleteAuthTokenByID(ctx context.Context, id string) error { diff --git a/routers/web/auth/auth.go b/routers/web/auth/auth.go index ae33c1a6c62..75d331f7487 100644 --- a/routers/web/auth/auth.go +++ b/routers/web/auth/auth.go @@ -78,49 +78,54 @@ func prepareCommonAuthPageData(ctx *context.Context, opt CommonAuthOptions) { // autoSignIn reads cookie and try to auto-login. func autoSignIn(ctx *context.Context) (bool, error) { - isSucceed := false - defer func() { - if !isSucceed { - ctx.DeleteSiteCookie(setting.CookieRememberName) - } - }() + cookieTokenValue := ctx.GetSiteCookie(setting.CookieRememberName) + if cookieTokenValue == "" { + return false, nil + } if err := auth.DeleteExpiredAuthTokens(ctx); err != nil { log.Error("Failed to delete expired auth tokens: %v", err) } - t, err := auth_service.CheckAuthToken(ctx, ctx.GetSiteCookie(setting.CookieRememberName)) - if err != nil { - switch err { - case auth_service.ErrAuthTokenInvalidFormat, auth_service.ErrAuthTokenExpired: - return false, nil + deleteCookie := true + defer func() { + if deleteCookie { + ctx.DeleteSiteCookie(setting.CookieRememberName) } - return false, err - } - if t == nil { + }() + + t, err := auth_service.CheckAuthToken(ctx, cookieTokenValue) + if errors.Is(err, auth_service.ErrAuthTokenStale) { + deleteCookie = false return false, nil + } else if errors.Is(err, auth_service.ErrAuthTokenInvalidFormat) || errors.Is(err, auth_service.ErrAuthTokenExpired) { + return false, nil + } else if err != nil { + return false, err } u, err := user_model.GetUserByID(ctx, t.UserID) - if err != nil { - if !user_model.IsErrUserNotExist(err) { - return false, fmt.Errorf("GetUserByID: %w", err) - } + if user_model.IsErrUserNotExist(err) { return false, nil + } else if err != nil { + return false, fmt.Errorf("GetUserByID: %w", err) } + userHasTwoFactorAuth, err := auth.HasTwoFactorOrWebAuthn(ctx, u.ID) if err != nil { return false, fmt.Errorf("HasTwoFactorOrWebAuthn: %w", err) } - isSucceed = true - nt, token, err := auth_service.RegenerateAuthToken(ctx, t) - if err != nil { + if errors.Is(err, auth_service.ErrAuthTokenStale) { + deleteCookie = false + return false, nil + } else if err != nil { return false, err } ctx.SetSiteCookie(setting.CookieRememberName, nt.ID+":"+token, setting.LogInRememberDays*timeutil.Day) + deleteCookie = false if err := regenerateSession(ctx, map[string]any{ session.KeyUID: u.ID, @@ -130,7 +135,7 @@ func autoSignIn(ctx *context.Context) (bool, error) { } if err := resetLocale(ctx, u); err != nil { - return false, err + log.Error("Failed to reset locale for user %d: %v", u.ID, err) } return true, nil @@ -374,6 +379,9 @@ func handleSignIn(ctx *context.Context, u *user_model.User, remember bool) { func handleSignInFull(ctx *context.Context, u *user_model.User, remember bool) { if remember { + if err := auth.DeleteExpiredAuthTokens(ctx); err != nil { + log.Error("Failed to delete expired auth tokens: %v", err) + } nt, token, err := auth_service.CreateAuthTokenForUserID(ctx, u.ID) if err != nil { ctx.ServerError("CreateAuthTokenForUserID", err) diff --git a/routers/web/auth/auth_test.go b/routers/web/auth/auth_test.go index 0ae06bb715e..26e6df153dc 100644 --- a/routers/web/auth/auth_test.go +++ b/routers/web/auth/auth_test.go @@ -18,6 +18,7 @@ import ( "gitea.dev/modules/test" "gitea.dev/modules/util" "gitea.dev/modules/web" + auth_service "gitea.dev/services/auth" "gitea.dev/services/auth/source/oauth2" "gitea.dev/services/contexttest" "gitea.dev/services/forms" @@ -216,3 +217,22 @@ func TestRegisterOpenIDPostRejectsWrongCaptcha(t *testing.T) { assert.Equal(t, true, ctx.Data["Err_Captcha"]) unittest.AssertNotExistsBean(t, &user_model.User{LowerName: "openid-captcha-user"}) } + +func TestAutoSignInStaleCookie(t *testing.T) { + require.NoError(t, unittest.PrepareTestDatabase()) + at, token, err := auth_service.CreateAuthTokenForUserID(t.Context(), 2) + require.NoError(t, err) + current, currentToken, err := auth_service.RegenerateAuthToken(t.Context(), at) + require.NoError(t, err) + + ctx, resp := contexttest.MockContext(t, "/user/login") + ctx.Req.AddCookie(&http.Cookie{Name: setting.CookieRememberName, Value: at.ID + ":" + token}) + succeeded, err := autoSignIn(ctx) + require.NoError(t, err) + require.False(t, succeeded) + for _, cookie := range resp.Result().Cookies() { + assert.NotEqual(t, setting.CookieRememberName, cookie.Name) + } + _, err = auth_service.CheckAuthToken(t.Context(), current.ID+":"+currentToken) + require.NoError(t, err) +} diff --git a/services/auth/auth_token.go b/services/auth/auth_token.go index 809dff5b261..6671e10e8ff 100644 --- a/services/auth/auth_token.go +++ b/services/auth/auth_token.go @@ -25,15 +25,12 @@ import ( var ( ErrAuthTokenInvalidFormat = util.NewInvalidArgumentErrorf("auth token has an invalid format") + ErrAuthTokenStale = util.NewInvalidArgumentErrorf("auth token has been rotated") ErrAuthTokenExpired = util.NewInvalidArgumentErrorf("auth token has expired") ErrAuthTokenInvalidHash = util.NewInvalidArgumentErrorf("auth token is invalid") ) func CheckAuthToken(ctx context.Context, value string) (*auth_model.AuthToken, error) { - if len(value) == 0 { - return nil, nil //nolint:nilnil // the auth method is not applicable - } - parts := strings.SplitN(value, ":", 2) if len(parts) != 2 { return nil, ErrAuthTokenInvalidFormat @@ -53,7 +50,12 @@ func CheckAuthToken(ctx context.Context, value string) (*auth_model.AuthToken, e hashedToken := sha256.Sum256([]byte(parts[1])) - if !util.CryptoConstTimeEqual(t.TokenHash, hex.EncodeToString(hashedToken[:])) { + hash := hex.EncodeToString(hashedToken[:]) + if !util.CryptoConstTimeEqual(t.TokenHash, hash) { + // Recently rotated cookies can still be in flight, but must not authenticate. + if t.RotatedUnix.AddDuration(time.Minute) >= timeutil.TimeStampNow() && util.CryptoConstTimeEqual(t.PreviousTokenHash, hash) { + return nil, ErrAuthTokenStale + } // If an attacker steals a token and uses the token to create a new session the hash gets updated. // When the victim uses the old token the hashes don't match anymore and the victim should be notified about the compromised token. // Revoke the token so the attacker's rotated token (which shares this ID) can no longer be used. @@ -70,15 +72,21 @@ func RegenerateAuthToken(ctx context.Context, t *auth_model.AuthToken) (*auth_mo token, hash := generateTokenAndHash() newToken := &auth_model.AuthToken{ - ID: t.ID, - TokenHash: hash, - UserID: t.UserID, - ExpiresUnix: timeutil.TimeStampNow().AddDuration(time.Duration(setting.LogInRememberDays*24) * time.Hour), + ID: t.ID, + TokenHash: hash, + PreviousTokenHash: t.TokenHash, + RotatedUnix: timeutil.TimeStampNow(), + UserID: t.UserID, + ExpiresUnix: timeutil.TimeStampNow().AddDuration(time.Duration(setting.LogInRememberDays*24) * time.Hour), } - if err := auth_model.UpdateAuthTokenByID(ctx, newToken); err != nil { + updated, err := auth_model.UpdateAuthTokenByID(ctx, newToken, t.TokenHash) + if err != nil { return nil, "", err } + if !updated { + return nil, "", ErrAuthTokenStale + } return newToken, token, nil } diff --git a/services/auth/auth_token_test.go b/services/auth/auth_token_test.go index ab24381d0a9..a1fe0c2746b 100644 --- a/services/auth/auth_token_test.go +++ b/services/auth/auth_token_test.go @@ -13,6 +13,7 @@ import ( "gitea.dev/modules/util" "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" ) func TestCheckAuthToken(t *testing.T) { @@ -20,7 +21,7 @@ func TestCheckAuthToken(t *testing.T) { t.Run("Empty", func(t *testing.T) { token, err := CheckAuthToken(t.Context(), "") - assert.NoError(t, err) + assert.ErrorIs(t, err, ErrAuthTokenInvalidFormat) assert.Nil(t, token) }) @@ -105,5 +106,20 @@ func TestRegenerateAuthToken(t *testing.T) { assert.NotEqual(t, token, token2) assert.NotEqual(t, at.ExpiresUnix, at2.ExpiresUnix) + // A second request validated the same cookie before the first rotation. + _, _, err = RegenerateAuthToken(t.Context(), at) + require.ErrorIs(t, err, ErrAuthTokenStale) + + _, err = CheckAuthToken(t.Context(), at.ID+":"+token) + require.ErrorIs(t, err, ErrAuthTokenStale) + current, err := CheckAuthToken(t.Context(), at2.ID+":"+token2) + require.NoError(t, err) + require.Equal(t, at2.TokenHash, current.TokenHash) + + timeutil.MockSet(time.Date(2023, 1, 1, 0, 1, 2, 0, time.UTC)) + _, err = CheckAuthToken(t.Context(), at.ID+":"+token) + require.ErrorIs(t, err, ErrAuthTokenInvalidHash) + _, err = auth_model.GetAuthTokenByID(t.Context(), at.ID) + require.ErrorIs(t, err, util.ErrNotExist) assert.NoError(t, auth_model.DeleteAuthTokenByID(t.Context(), at.ID)) }