From 7367ff75cd5ca8aa2ae12c363d3705e5b22584be Mon Sep 17 00:00:00 2001 From: Aleksey Fedorov <320380254+Dv2Corn@users.noreply.github.com> Date: Fri, 13 Nov 2026 09:13:13 +1000 Subject: [PATCH] fix(2fa): accept display forms of the TOTP secret and never panic on a bad one (#7113) gotp decodes the stored secret with strict base32 and panics when that fails, so a 2FA secret saved through the settings API in lower case, grouped by spaces, or not base32 at all turned every login and every attempt to turn 2FA off into an HTTP 500. An empty secret had the opposite problem: the login check accepted the code of an empty HMAC key, which anyone can compute. VerifyWithSkew now normalizes the secret and verifies nothing for an empty or non-base32 one, so secrets already stored in a display form work again. Turning 2FA on through the settings update stores the normalized secret and refuses one no authenticator could match. --- internal/util/totp/totp.go | 20 ++++++++- internal/util/totp/totp_test.go | 27 ++++++++++++ internal/web/service/panel/user_test.go | 36 ++++++++++++++++ internal/web/service/setting.go | 6 +++ internal/web/service/setting_security_test.go | 42 ++++++++++++++++++- 5 files changed, 128 insertions(+), 3 deletions(-) diff --git a/internal/util/totp/totp.go b/internal/util/totp/totp.go index 4a25dc42e..96aa09096 100644 --- a/internal/util/totp/totp.go +++ b/internal/util/totp/totp.go @@ -1,6 +1,8 @@ package totp import ( + "encoding/base32" + "strings" "time" "github.com/xlzd/gotp" @@ -10,9 +12,25 @@ import ( // Standard TOTP clock-drift tolerance, see MHSanaei/3x-ui#6535. const SkewWindows = 1 +// NormalizeSecret turns a base32 key as authenticator apps show it (lower case, +// grouped by spaces, padded) into the bare upper-case form gotp decodes. +func NormalizeSecret(secret string) string { + return strings.TrimRight(strings.ToUpper(strings.Join(strings.Fields(secret), "")), "=") +} + +// ValidSecret reports whether secret is a non-empty base32 key once normalized. +func ValidSecret(secret string) bool { + key, err := base32.StdEncoding.WithPadding(base32.NoPadding).DecodeString(NormalizeSecret(secret)) + return err == nil && len(key) > 0 +} + // VerifyWithSkew accepts the code for the current step plus/minus SkewWindows. +// An empty or non-base32 secret verifies nothing (gotp panics on the latter). func VerifyWithSkew(secret, code string, now time.Time) bool { - totp := gotp.NewDefaultTOTP(secret) + if !ValidSecret(secret) { + return false + } + totp := gotp.NewDefaultTOTP(NormalizeSecret(secret)) for i := -SkewWindows; i <= SkewWindows; i++ { if totp.AtTime(now.Add(time.Duration(i*30)*time.Second)) == code { return true diff --git a/internal/util/totp/totp_test.go b/internal/util/totp/totp_test.go index 1036af6f2..6161e7c56 100644 --- a/internal/util/totp/totp_test.go +++ b/internal/util/totp/totp_test.go @@ -32,3 +32,30 @@ func TestVerifyWithSkew(t *testing.T) { t.Fatal("wrong code should not verify") } } + +func TestVerifyWithSkewSecretForms(t *testing.T) { + const canonical = "JBSWY3DPEHPK3PXP" + now := time.Unix((time.Now().Unix()/30)*30+15, 0).UTC() + code := gotp.NewDefaultTOTP(canonical).AtTime(now) + + cases := []struct { + name string + secret string + want bool + }{ + {"canonical", canonical, true}, + {"lower case", "jbswy3dpehpk3pxp", true}, + {"grouped and padded", " jbsw y3dp ehpk 3pxp== ", true}, + {"empty", "", false}, + {"blank", " ", false}, + {"not base32", "totp-secret", false}, + {"base32 digits out of range", "JBSWY3DPEHPK3PX1", false}, + } + for _, c := range cases { + t.Run(c.name, func(t *testing.T) { + if got := VerifyWithSkew(c.secret, code, now); got != c.want { + t.Fatalf("VerifyWithSkew(%q) = %v, want %v", c.secret, got, c.want) + } + }) + } +} diff --git a/internal/web/service/panel/user_test.go b/internal/web/service/panel/user_test.go index 70e26f5b3..7171e53e9 100644 --- a/internal/web/service/panel/user_test.go +++ b/internal/web/service/panel/user_test.go @@ -4,6 +4,9 @@ import ( "path/filepath" "strings" "testing" + "time" + + "github.com/xlzd/gotp" "github.com/mhsanaei/3x-ui/v3/internal/database" "github.com/mhsanaei/3x-ui/v3/internal/database/dbtest" @@ -74,3 +77,36 @@ func TestUserSettersRefuseNamesTheLoginRouteRefuses(t *testing.T) { t.Fatalf("stored username = %q (%v), want it unchanged", stored.Username, err) } } + +// A stored 2FA secret in a form gotp cannot decode made every login panic (HTTP 500). +func TestCheckUserTwoFactorSecretForms(t *testing.T) { + cases := []struct { + name, stored, codeSecret string + want bool + }{ + {"canonical", "JBSWY3DPEHPK3PXP", "JBSWY3DPEHPK3PXP", true}, + {"lower case", "jbswy3dpehpk3pxp", "JBSWY3DPEHPK3PXP", true}, + {"not base32", "totp-secret", "JBSWY3DPEHPK3PXP", false}, + {"empty secret", "", "", false}, + } + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + dbtest.InitDB(t, filepath.Join(t.TempDir(), "x-ui.db")) + svc := &UserService{} + if err := svc.UpdateFirstUser("admin", "s3cret-Pass"); err != nil { + t.Fatalf("UpdateFirstUser: %v", err) + } + if err := svc.settingService.SetTwoFactorEnable(true); err != nil { + t.Fatal(err) + } + if err := svc.settingService.SetTwoFactorToken(tc.stored); err != nil { + t.Fatal(err) + } + code := gotp.NewDefaultTOTP(tc.codeSecret).AtTime(time.Now()) + user, err := svc.CheckUser("admin", "s3cret-Pass", code) + if got := err == nil && user != nil; got != tc.want { + t.Fatalf("CheckUser with stored secret %q: ok = %v (%v), want %v", tc.stored, got, err, tc.want) + } + }) + } +} diff --git a/internal/web/service/setting.go b/internal/web/service/setting.go index b7a9df205..14643202e 100644 --- a/internal/web/service/setting.go +++ b/internal/web/service/setting.go @@ -1683,6 +1683,12 @@ func (s *SettingService) UpdateAllSetting(allSetting *entity.AllSetting, clears if err := s.preserveRedactedSecrets(allSetting, clears); err != nil { return err } + if allSetting.TwoFactorEnable { + if !totp.ValidSecret(allSetting.TwoFactorToken) { + return errors.New("two-factor secret must be a non-empty base32 key") + } + allSetting.TwoFactorToken = totp.NormalizeSecret(allSetting.TwoFactorToken) + } if err := validateSettingsURLs(allSetting); err != nil { return err } diff --git a/internal/web/service/setting_security_test.go b/internal/web/service/setting_security_test.go index 1b0777303..2d06dae5d 100644 --- a/internal/web/service/setting_security_test.go +++ b/internal/web/service/setting_security_test.go @@ -102,7 +102,7 @@ func TestUpdateAllSettingPreservesRedactedSecrets(t *testing.T) { if err := s.saveSetting("twoFactorEnable", "true"); err != nil { t.Fatal(err) } - if err := s.saveSetting("twoFactorToken", "totp-secret"); err != nil { + if err := s.saveSetting("twoFactorToken", "JBSWY3DPEHPK3PXP"); err != nil { t.Fatal(err) } if err := s.saveSetting("smtpPassword", "smtp-secret"); err != nil { @@ -126,7 +126,7 @@ func TestUpdateAllSettingPreservesRedactedSecrets(t *testing.T) { if got, _ := s.GetLdapPassword(); got != "ldap-secret" { t.Fatalf("ldap password = %q, want preserved secret", got) } - if got, _ := s.GetTwoFactorToken(); got != "totp-secret" { + if got, _ := s.GetTwoFactorToken(); got != "JBSWY3DPEHPK3PXP" { t.Fatalf("2fa token = %q, want preserved secret", got) } if got, _ := s.GetSmtpPassword(); got != "smtp-secret" { @@ -137,6 +137,44 @@ func TestUpdateAllSettingPreservesRedactedSecrets(t *testing.T) { } } +// Turning 2FA on stores the secret in the form the TOTP decoder takes, and refuses +// one that no authenticator code could ever match. +func TestUpdateAllSettingTwoFactorSecret(t *testing.T) { + cases := []struct { + name, stored, submitted, want string + wantErr bool + }{ + {name: "canonical", submitted: "JBSWY3DPEHPK3PXP", want: "JBSWY3DPEHPK3PXP"}, + {name: "lower case and grouped", submitted: "jbsw y3dp ehpk 3pxp", want: "JBSWY3DPEHPK3PXP"}, + {name: "redacted keeps stored", stored: "ORIGINALSECRET234567", submitted: "", want: "ORIGINALSECRET234567"}, + {name: "not base32", stored: "ORIGINALSECRET234567", submitted: "totp-secret", want: "ORIGINALSECRET234567", wantErr: true}, + {name: "nothing stored", submitted: "", want: "", wantErr: true}, + } + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + setupSettingTestDB(t) + s := &SettingService{} + if err := s.SetTwoFactorToken(tc.stored); err != nil { + t.Fatal(err) + } + view, err := s.GetAllSettingView() + if err != nil { + t.Fatal(err) + } + settings := &view.AllSetting + settings.TwoFactorEnable = true + settings.TwoFactorToken = tc.submitted + err = s.UpdateAllSetting(settings, SecretClears{}) + if (err != nil) != tc.wantErr { + t.Fatalf("UpdateAllSetting error = %v, wantErr %v", err, tc.wantErr) + } + if got, _ := s.GetTwoFactorToken(); got != tc.want { + t.Fatalf("stored 2FA secret = %q, want %q", got, tc.want) + } + }) + } +} + func TestUpdateAllSettingClearsFlaggedSecrets(t *testing.T) { setupSettingTestDB(t) s := &SettingService{}