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{}