diff --git a/internal/web/controller/csrf_log_test.go b/internal/web/controller/csrf_log_test.go index e436bcf39..90b3d2fdb 100644 --- a/internal/web/controller/csrf_log_test.go +++ b/internal/web/controller/csrf_log_test.go @@ -1,8 +1,11 @@ package controller import ( + "encoding/json" "net/http" "net/http/httptest" + "net/url" + "path/filepath" "strings" "testing" @@ -10,7 +13,11 @@ import ( "github.com/gin-contrib/sessions/cookie" "github.com/gin-gonic/gin" + "github.com/mhsanaei/3x-ui/v3/internal/database/dbtest" "github.com/mhsanaei/3x-ui/v3/internal/logger" + "github.com/mhsanaei/3x-ui/v3/internal/web/locale" + "github.com/mhsanaei/3x-ui/v3/internal/web/service/panel" + "github.com/mhsanaei/3x-ui/v3/internal/web/session" ) func TestLoginCSRFRejectionLogsPeerIPNotForwardedHeader(t *testing.T) { @@ -39,3 +46,49 @@ func TestLoginCSRFRejectionLogsPeerIPNotForwardedHeader(t *testing.T) { t.Fatalf("CSRF rejection log = %q, want the untrusted peer 198.51.100.9 and not the forwarded header", line) } } + +// A failed login keeps its username in the log buffer, the limiter table and the +// notifier queue; an unauthenticated caller must not be able to make that unbounded. +func TestLoginRefusesOversizedUsernameBeforeRecordingIt(t *testing.T) { + gin.SetMode(gin.TestMode) + dbtest.InitDB(t, filepath.Join(t.TempDir(), "x-ui.db")) + engine := gin.New() + engine.Use(sessions.Sessions("3x-ui", cookie.NewStore([]byte("login-length-test-secret")))) + engine.Use(func(c *gin.Context) { + c.Set("I18n", func(_ locale.I18nType, key string, _ ...string) string { return key }) + c.Next() + }) + NewIndexController(engine.Group("/")) + + tokenReq := httptest.NewRequest(http.MethodGet, "/csrf-token", nil) + tokenRec := httptest.NewRecorder() + engine.ServeHTTP(tokenRec, tokenReq) + var token struct{ Obj string } + if err := json.Unmarshal(tokenRec.Body.Bytes(), &token); err != nil || token.Obj == "" { + t.Fatalf("csrf token: %v %s", err, tokenRec.Body.String()) + } + username := strings.Repeat("u", panel.MaxUsernameLength+1) + form := url.Values{"username": {username}, "password": {"wrong"}} + req := httptest.NewRequest(http.MethodPost, "/login", strings.NewReader(form.Encode())) + req.RemoteAddr = "198.51.100.10:40000" + req.Header.Set("Content-Type", "application/x-www-form-urlencoded") + req.Header.Set(session.CSRFHeaderName, token.Obj) + for _, cookie := range tokenRec.Result().Cookies() { + req.AddCookie(cookie) + } + rec := httptest.NewRecorder() + engine.ServeHTTP(rec, req) + if rec.Code != http.StatusOK || !strings.Contains(rec.Body.String(), `"success":false`) { + t.Fatalf("login = %d %s, want the generic failure", rec.Code, rec.Body.String()) + } + for _, line := range logger.GetLogs(100, "DEBUG") { + if strings.Contains(line, username) { + t.Fatal("the oversized username reached the log buffer") + } + } + defaultLoginLimiter.mu.Lock() + defer defaultLoginLimiter.mu.Unlock() + if _, kept := defaultLoginLimiter.attempts[loginLimitKey("198.51.100.10", username)]; kept { + t.Fatal("the oversized username was kept in the limiter table") + } +} diff --git a/internal/web/controller/index.go b/internal/web/controller/index.go index 0433e2ed8..ba0bae364 100644 --- a/internal/web/controller/index.go +++ b/internal/web/controller/index.go @@ -47,7 +47,8 @@ func (a *IndexController) initRouter(g *gin.RouterGroup) { g.GET("/sponsors", a.sponsors) g.GET("/sponsors/logo/:name", a.sponsorLogo) - g.POST("/login", middleware.CSRFMiddleware(getRemoteIp), a.login) + // The route carries three short fields; anything longer is refused unread. + g.POST("/login", middleware.MaxBodyBytes(16<<10), middleware.CSRFMiddleware(getRemoteIp), a.login) g.POST("/logout", middleware.CSRFMiddleware(getRemoteIp), a.logout) g.POST("/getTwoFactorEnable", middleware.CSRFMiddleware(getRemoteIp), a.getTwoFactorEnable) } @@ -102,6 +103,12 @@ func (a *IndexController) login(c *gin.Context) { } remoteIP := getRemoteIp(c) + // No stored name is longer; the log buffer, the limiter and the notifiers keep what follows. + if len(form.Username) > panel.MaxUsernameLength { + logger.Warningf("failed login: IP=%q, reason=%q", remoteIP, "username too long") + pureJsonMsg(c, http.StatusOK, false, I18nWeb(c, "pages.login.toasts.wrongUsernameOrPassword")) + return + } safeUser := template.HTMLEscapeString(form.Username) timeStr := time.Now().Format("2006-01-02 15:04:05") if blockedUntil, ok := defaultLoginLimiter.allow(remoteIP, form.Username); !ok { diff --git a/internal/web/service/panel/user.go b/internal/web/service/panel/user.go index dd6970d86..8c0bf9e7d 100644 --- a/internal/web/service/panel/user.go +++ b/internal/web/service/panel/user.go @@ -2,6 +2,7 @@ package panel import ( "errors" + "fmt" "time" "gorm.io/gorm" @@ -128,6 +129,9 @@ func (s *UserService) BumpLoginEpoch() error { } func (s *UserService) UpdateUser(id int, username string, password string) error { + if len(username) > MaxUsernameLength { + return fmt.Errorf("username can not be longer than %d bytes", MaxUsernameLength) + } db := database.GetDB() hashedPassword, err := crypto.HashPasswordAsBcrypt(password) if err != nil { @@ -157,6 +161,8 @@ func (s *UserService) UpdateUser(id int, username string, password string) error func (s *UserService) UpdateFirstUser(username string, password string) error { if username == "" { return errors.New("username can not be empty") + } else if len(username) > MaxUsernameLength { + return fmt.Errorf("username can not be longer than %d bytes", MaxUsernameLength) } else if password == "" { return errors.New("password can not be empty") } @@ -181,3 +187,7 @@ func (s *UserService) UpdateFirstUser(username string, password string) error { user.LoginEpoch++ return db.Save(user).Error } + +// MaxUsernameLength bounds a panel username: the login route refuses longer names before +// logging or recording them, so no stored name may exceed it. +const MaxUsernameLength = 256 diff --git a/internal/web/service/panel/user_test.go b/internal/web/service/panel/user_test.go index ba79578d4..70e26f5b3 100644 --- a/internal/web/service/panel/user_test.go +++ b/internal/web/service/panel/user_test.go @@ -2,6 +2,7 @@ package panel import ( "path/filepath" + "strings" "testing" "github.com/mhsanaei/3x-ui/v3/internal/database" @@ -48,3 +49,28 @@ func TestHasDefaultCredential(t *testing.T) { }) } } + +// The login route refuses names over MaxUsernameLength unread, so storing one would lock +// the admin out of their own panel. +func TestUserSettersRefuseNamesTheLoginRouteRefuses(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) + } + var user model.User + if err := database.GetDB().First(&user).Error; err != nil { + t.Fatalf("load user: %v", err) + } + long := strings.Repeat("a", MaxUsernameLength+1) + if err := svc.UpdateFirstUser(long, "s3cret-Pass"); err == nil { + t.Fatal("UpdateFirstUser stored a name the login route refuses") + } + if err := svc.UpdateUser(user.Id, long, "s3cret-Pass"); err == nil { + t.Fatal("UpdateUser stored a name the login route refuses") + } + var stored model.User + if err := database.GetDB().First(&stored, user.Id).Error; err != nil || stored.Username != "admin" { + t.Fatalf("stored username = %q (%v), want it unchanged", stored.Username, err) + } +}