From c8a3a2d723099a556e38a2bcb551282041b7437f Mon Sep 17 00:00:00 2001 From: Sanaei Date: Wed, 19 Aug 2026 19:54:15 +0200 Subject: [PATCH] fix(security): require a 2FA code to replace the stored TOTP secret The confirmation gate in updateSetting only covered the true -> false transition, so a settings save that kept twoFactorEnable=true while carrying a non-blank twoFactorToken silently rebound the authenticator. preserveRedactedSecrets restores the stored secret only when the submitted one is blank, so a non-blank value went straight through without any branch asking for a code. Not reachable pre-auth or cross-site (CSRFMiddleware rejects unsafe methods without the session token), but it matters after a session hijack or with an admin API token, which sets api_authed and short-circuits the CSRF check: the attacker gains persistence and locks the legitimate operator out of their own authenticator. Now a code is required whenever 2FA is currently on and the submitted secret differs from the stored one. Enabling from off is untouched, as no code exists yet to verify, and a blank secret still means "unchanged", so the panel's normal save path is unaffected. Reported by @n0ctal (GHSA-xqqw-jqqv-99h6). --- internal/web/controller/setting.go | 15 +++-- internal/web/controller/setting_test.go | 84 +++++++++++++++++++++++++ 2 files changed, 95 insertions(+), 4 deletions(-) diff --git a/internal/web/controller/setting.go b/internal/web/controller/setting.go index f93d693b6..74f352580 100644 --- a/internal/web/controller/setting.go +++ b/internal/web/controller/setting.go @@ -4,6 +4,7 @@ import ( "errors" "net/http" "strconv" + "strings" "time" "github.com/mhsanaei/3x-ui/v3/internal/logger" @@ -130,10 +131,16 @@ func (a *SettingController) updateSetting(c *gin.Context) { oldTgToken, _ := a.settingService.GetTgBotToken() oldTgChatId, _ := a.settingService.GetTgBotChatId() oldTgAPIServer, _ := a.settingService.GetTgBotAPIServer() - if twoFactorErr == nil && oldTwoFactor && !allSetting.TwoFactorEnable { - if err := a.settingService.VerifyTwoFactorCode(form.TwoFactorCode); err != nil { - jsonMsg(c, I18nWeb(c, "pages.settings.toasts.modifySettings"), err) - return + if twoFactorErr == nil && oldTwoFactor { + // Rebinding the authenticator is the same class of change as turning 2FA + // off, so both need a current code. Blank still means "unchanged". + submittedToken := strings.TrimSpace(allSetting.TwoFactorToken) + storedToken, _ := a.settingService.GetTwoFactorToken() + if !allSetting.TwoFactorEnable || (submittedToken != "" && submittedToken != storedToken) { + if err := a.settingService.VerifyTwoFactorCode(form.TwoFactorCode); err != nil { + jsonMsg(c, I18nWeb(c, "pages.settings.toasts.modifySettings"), err) + return + } } } err := a.settingService.UpdateAllSetting(allSetting, service.SecretClears{ diff --git a/internal/web/controller/setting_test.go b/internal/web/controller/setting_test.go index 51a23d426..88ae236e0 100644 --- a/internal/web/controller/setting_test.go +++ b/internal/web/controller/setting_test.go @@ -1,6 +1,7 @@ package controller import ( + "encoding/json" "net/http" "net/http/httptest" "path/filepath" @@ -13,6 +14,7 @@ import ( "github.com/mhsanaei/3x-ui/v3/internal/database" "github.com/mhsanaei/3x-ui/v3/internal/database/model" "github.com/mhsanaei/3x-ui/v3/internal/util/crypto" + "github.com/mhsanaei/3x-ui/v3/internal/web/service" ) func TestValidateRegex(t *testing.T) { @@ -86,3 +88,85 @@ func TestAPITokenMutationRoutesEnforceExpectedScope(t *testing.T) { t.Fatal("token was disabled by wrong scope") } } + +// GHSA-xqqw-jqqv-99h6: a save that keeps 2FA enabled must not be able to +// rebind the authenticator without presenting a current code. +func TestUpdateSettingRequiresCodeToReplaceTwoFactorToken(t *testing.T) { + t.Setenv("XUI_DB_FOLDER", t.TempDir()) + if err := database.InitDB(filepath.Join(t.TempDir(), "x-ui.db")); err != nil { + t.Fatalf("InitDB: %v", err) + } + t.Cleanup(func() { _ = database.CloseDB() }) + + settingService := service.SettingService{} + if err := settingService.SetTwoFactorToken("ORIGINALSECRET234567"); err != nil { + t.Fatalf("seed token: %v", err) + } + if err := settingService.SetTwoFactorEnable(true); err != nil { + t.Fatalf("seed enable: %v", err) + } + + post := func(t *testing.T, mutate func(map[string]any)) string { + t.Helper() + base, err := settingService.GetAllSetting() + if err != nil { + t.Fatalf("GetAllSetting: %v", err) + } + raw, err := json.Marshal(base) + if err != nil { + t.Fatalf("marshal: %v", err) + } + body := map[string]any{} + if err := json.Unmarshal(raw, &body); err != nil { + t.Fatalf("unmarshal: %v", err) + } + mutate(body) + payload, err := json.Marshal(body) + if err != nil { + t.Fatalf("marshal payload: %v", err) + } + + gin.SetMode(gin.TestMode) + router := gin.New() + NewSettingController(router.Group("/panel/api")) + req := httptest.NewRequest(http.MethodPost, "/panel/api/setting/update", strings.NewReader(string(payload))) + req.Header.Set("Content-Type", "application/json") + resp := httptest.NewRecorder() + router.ServeHTTP(resp, req) + return resp.Body.String() + } + + t.Run("rebind without code is rejected", func(t *testing.T) { + got := post(t, func(body map[string]any) { + body["twoFactorEnable"] = true + body["twoFactorToken"] = "ATTACKERSECRET567890" + }) + if !strings.Contains(got, `"success":false`) { + t.Fatalf("rebind without a 2FA code was accepted: %s", got) + } + stored, err := settingService.GetTwoFactorToken() + if err != nil { + t.Fatalf("GetTwoFactorToken: %v", err) + } + if stored != "ORIGINALSECRET234567" { + t.Fatalf("stored 2FA secret = %q, want it unchanged", stored) + } + }) + + t.Run("ordinary save with redacted token still succeeds", func(t *testing.T) { + got := post(t, func(body map[string]any) { + body["twoFactorEnable"] = true + body["twoFactorToken"] = "" + }) + if !strings.Contains(got, `"success":true`) { + t.Fatalf("normal settings save was rejected: %s", got) + } + stored, err := settingService.GetTwoFactorToken() + if err != nil { + t.Fatalf("GetTwoFactorToken: %v", err) + } + if stored != "ORIGINALSECRET234567" { + t.Fatalf("stored 2FA secret = %q, want it preserved", stored) + } + }) +}