diff --git a/auth/proxy_test.go b/auth/proxy_test.go index df520a2c..9b9ef3a7 100644 --- a/auth/proxy_test.go +++ b/auth/proxy_test.go @@ -22,6 +22,7 @@ func (m *mockUserStore) Get(_ string, _ bool, id interface{}) (*users.User, erro return nil, fberrors.ErrNotExist } +func (m *mockUserStore) GetByScope(_ string) (*users.User, error) { return nil, fberrors.ErrNotExist } func (m *mockUserStore) Gets(_ string, _ bool) ([]*users.User, error) { return nil, nil } func (m *mockUserStore) Update(_ *users.User, _ ...string) error { return nil } func (m *mockUserStore) Save(user *users.User) error { diff --git a/http/auth.go b/http/auth.go index 137824d5..b96819b8 100644 --- a/http/auth.go +++ b/http/auth.go @@ -201,6 +201,22 @@ var signupHandler = func(w http.ResponseWriter, r *http.Request, d *data) (int, return http.StatusInternalServerError, err } user.Scope = userHome + + // When home directories are created from the username, distinct usernames + // can normalize to the same scope (cleanUsername is many-to-one), which would + // silently hand the new user another user's home directory. Reject the signup + // if the derived scope is already taken. When CreateUserDir is off, all + // signups intentionally share the configured default scope, so this check + // does not apply. + if d.settings.CreateUserDir { + switch _, err := d.store.Users.GetByScope(user.Scope); { + case err == nil: + return http.StatusConflict, fberrors.ErrExist + case !errors.Is(err, fberrors.ErrNotExist): + return http.StatusInternalServerError, err + } + } + log.Printf("new user: %s, home dir: [%s].", user.Username, userHome) err = d.store.Users.Save(user) diff --git a/http/auth_test.go b/http/auth_test.go new file mode 100644 index 00000000..bacff284 --- /dev/null +++ b/http/auth_test.go @@ -0,0 +1,71 @@ +package fbhttp + +import ( + "net/http" + "net/http/httptest" + "path/filepath" + "strings" + "testing" + + "github.com/asdine/storm/v3" + + "github.com/filebrowser/filebrowser/v2/settings" + "github.com/filebrowser/filebrowser/v2/storage/bolt" +) + +// Regression for the username-normalization home-directory collision +// (GHSA-7rc3-g7h6-22m7): with Signup and CreateUserDir enabled, two distinct +// usernames that cleanUsername() normalizes to the same directory must not be +// handed the same home directory. The second registration is rejected. +func TestSignupRejectsCollidingNormalizedScope(t *testing.T) { + root := t.TempDir() + + db, err := storm.Open(filepath.Join(t.TempDir(), "db")) + if err != nil { + t.Fatalf("failed to open db: %v", err) + } + t.Cleanup(func() { _ = db.Close() }) + + st, err := bolt.NewStorage(db) + if err != nil { + t.Fatalf("failed to get storage: %v", err) + } + if err := st.Settings.Save(&settings.Settings{ + Key: []byte("test-signing-key"), + Signup: true, + CreateUserDir: true, + UserHomeBasePath: "/users", + MinimumPasswordLength: 1, + }); err != nil { + t.Fatalf("failed to save settings: %v", err) + } + + server := &settings.Server{Root: root} + + signup := func(username string) *httptest.ResponseRecorder { + body := `{"username":"` + username + `","password":"CollidePw12345!"}` + req, _ := http.NewRequest(http.MethodPost, "/signup", strings.NewReader(body)) + rec := httptest.NewRecorder() + handle(signupHandler, "", st, server).ServeHTTP(rec, req) + return rec + } + + // Victim registers first and gets /users/teamone-x. + if rec := signup("teamone-x"); rec.Code != http.StatusOK { + t.Fatalf("first signup: expected 200, got %d body=%q", rec.Code, rec.Body.String()) + } + + // Attacker picks a distinct username that normalizes to the same scope. + if rec := signup("teamone/x"); rec.Code != http.StatusConflict { + t.Fatalf("VULNERABLE: colliding signup expected 409, got %d body=%q", rec.Code, rec.Body.String()) + } + + // The shared scope must still be owned solely by the first user. + owner, err := st.Users.GetByScope("/users/teamone-x") + if err != nil { + t.Fatalf("expected first user to own the scope: %v", err) + } + if owner.Username != "teamone-x" { + t.Fatalf("scope owner = %q, want teamone-x", owner.Username) + } +} diff --git a/storage/bolt/users.go b/storage/bolt/users.go index 6686d941..33f67abb 100644 --- a/storage/bolt/users.go +++ b/storage/bolt/users.go @@ -6,6 +6,7 @@ import ( "reflect" "github.com/asdine/storm/v3" + "github.com/asdine/storm/v3/q" bolt "go.etcd.io/bbolt" fberrors "github.com/filebrowser/filebrowser/v2/errors" @@ -41,6 +42,19 @@ func (st usersBackend) GetBy(i interface{}) (user *users.User, err error) { return } +func (st usersBackend) GetByScope(scope string) (*users.User, error) { + user := &users.User{} + err := st.db.Select(q.Eq("Scope", scope)).First(user) + if err != nil { + if errors.Is(err, storm.ErrNotFound) { + return nil, fberrors.ErrNotExist + } + return nil, err + } + + return user, nil +} + func (st usersBackend) Gets() ([]*users.User, error) { var allUsers []*users.User err := st.db.All(&allUsers) diff --git a/users/storage.go b/users/storage.go index 32f10e4d..ff10aeab 100644 --- a/users/storage.go +++ b/users/storage.go @@ -10,6 +10,7 @@ import ( // StorageBackend is the interface to implement for a users storage. type StorageBackend interface { GetBy(interface{}) (*User, error) + GetByScope(scope string) (*User, error) Gets() ([]*User, error) Save(u *User) error Update(u *User, fields ...string) error @@ -20,6 +21,7 @@ type StorageBackend interface { type Store interface { Get(baseScope string, followExternalSymlinks bool, id interface{}) (user *User, err error) + GetByScope(scope string) (*User, error) Gets(baseScope string, followExternalSymlinks bool) ([]*User, error) Update(user *User, fields ...string) error Save(user *User) error @@ -56,6 +58,13 @@ func (s *Storage) Get(baseScope string, followExternalSymlinks bool, id interfac return } +// GetByScope returns the first user whose scope matches the given one, or +// ErrNotExist if none does. The user is returned as stored, without setting up +// its filesystem, as it is meant for existence checks rather than serving. +func (s *Storage) GetByScope(scope string) (*User, error) { + return s.back.GetByScope(scope) +} + // Gets gets a list of all users. func (s *Storage) Gets(baseScope string, followExternalSymlinks bool) ([]*User, error) { users, err := s.back.Gets()