From 3471ec2c4b6473831c72ee889cb3c1a6849a1fb1 Mon Sep 17 00:00:00 2001 From: Henrique Dias Date: Thu, 4 Jun 2026 11:02:18 +0200 Subject: [PATCH] fix: incomplete fix for symlinked directories let scopes users and public-share recipients read and write files outside of scope --- files/file.go | 23 ++--- files/file_test.go | 50 +++++++++++ http/public_symlink_test.go | 166 ++++++++++++++++++++++++++++++++++++ http/public_test.go | 8 +- http/raw.go | 4 + http/tus_handlers.go | 10 +++ http/tus_symlink_test.go | 118 +++++++++++++++++++++++++ 7 files changed, 368 insertions(+), 11 deletions(-) create mode 100644 http/public_symlink_test.go create mode 100644 http/tus_symlink_test.go diff --git a/files/file.go b/files/file.go index dc227a6c..a034abc0 100644 --- a/files/file.go +++ b/files/file.go @@ -128,22 +128,18 @@ func stat(opts *FileOptions) (*FileInfo, error) { } } - // regular file - if file != nil && !file.IsSymlink { - return file, nil - } - - // The path is a symlink. Refuse to follow it if its on-disk target escapes - // the user's scoped root; otherwise a symlink that lives lexically inside - // the scope but points outside it would let a restricted user read, write, - // or share files beyond their boundary. - if file != nil && file.IsSymlink { + if file != nil { ok, scopeErr := WithinScope(opts.Fs, opts.Path) if scopeErr != nil || !ok { return nil, os.ErrPermission } } + // regular file + if file != nil && !file.IsSymlink { + return file, nil + } + // fs doesn't support afero.Lstater interface or the file is a symlink info, err := opts.Fs.Stat(opts.Path) if err != nil { @@ -479,6 +475,13 @@ func (i *FileInfo) readListing(checker rules.Checker, readHeader bool, calcImgRe isSymlink, isInvalidLink := false, false if IsSymlink(f.Mode()) { isSymlink = true + // A symlink whose on-disk target escapes the scoped root must not be + // followed, otherwise the listing would leak the target's metadata + // (and downstream access) for files outside the user's scope or the + // shared subtree. + if ok, scopeErr := WithinScope(i.Fs, fPath); scopeErr != nil || !ok { + continue + } // It's a symbolic link. We try to follow it. If it doesn't work, // we stay with the link information instead of the target's. info, err := i.Fs.Stat(fPath) diff --git a/files/file_test.go b/files/file_test.go index 0f89f572..d1f647f3 100644 --- a/files/file_test.go +++ b/files/file_test.go @@ -80,4 +80,54 @@ func TestWithinScope(t *testing.T) { t.Fatal("expected escaping symlink to a sibling directory to be rejected") } }) + + t.Run("symlink whose target stays within scope is allowed", func(t *testing.T) { + scope := t.TempDir() + if err := os.MkdirAll(filepath.Join(scope, "real"), 0o755); err != nil { + t.Fatal(err) + } + if err := os.WriteFile(filepath.Join(scope, "real", "f.txt"), []byte("x"), 0o644); err != nil { + t.Fatal(err) + } + if err := os.Symlink(filepath.Join(scope, "real"), filepath.Join(scope, "link")); err != nil { + t.Skipf("cannot create symlink: %v", err) + } + bfs := afero.NewBasePathFs(afero.NewOsFs(), scope) + + ok, err := WithinScope(bfs, "/link/f.txt") + if err != nil || !ok { + t.Fatalf("expected (true, nil) for an in-scope symlink target, got (%v, %v)", ok, err) + } + }) +} + +// stat must reject a regular file reached through a symlinked ancestor that +// escapes the scope (GHSA-hf77-9m7w-fq8q), while still serving in-scope files. +func TestStatRejectsLinkedAncestorEscape(t *testing.T) { + scope := t.TempDir() + if err := os.MkdirAll(filepath.Join(scope, "shared"), 0o755); err != nil { + t.Fatal(err) + } + if err := os.MkdirAll(filepath.Join(scope, "private"), 0o755); err != nil { + t.Fatal(err) + } + if err := os.WriteFile(filepath.Join(scope, "private", "secret.txt"), []byte("secret"), 0o600); err != nil { + t.Fatal(err) + } + if err := os.WriteFile(filepath.Join(scope, "shared", "ok.txt"), []byte("ok"), 0o600); err != nil { + t.Fatal(err) + } + if err := os.Symlink(filepath.Join(scope, "private"), filepath.Join(scope, "shared", "link")); err != nil { + t.Skipf("cannot create symlink: %v", err) + } + + // Filesystem scoped to the shared directory, as a public share would be. + bfs := afero.NewBasePathFs(afero.NewOsFs(), filepath.Join(scope, "shared")) + + if _, err := stat(&FileOptions{Fs: bfs, Path: "/link/secret.txt"}); !os.IsPermission(err) { + t.Fatalf("expected permission error for linked-ancestor escape, got %v", err) + } + if _, err := stat(&FileOptions{Fs: bfs, Path: "/ok.txt"}); err != nil { + t.Fatalf("expected in-scope file to be served, got %v", err) + } } diff --git a/http/public_symlink_test.go b/http/public_symlink_test.go new file mode 100644 index 00000000..89c3bbd3 --- /dev/null +++ b/http/public_symlink_test.go @@ -0,0 +1,166 @@ +package fbhttp + +import ( + "archive/zip" + "bytes" + "io" + "net/http" + "net/http/httptest" + "os" + "path/filepath" + "strings" + "testing" + + "github.com/asdine/storm/v3" + "github.com/spf13/afero" + + "github.com/filebrowser/filebrowser/v2/settings" + "github.com/filebrowser/filebrowser/v2/share" + "github.com/filebrowser/filebrowser/v2/storage" + "github.com/filebrowser/filebrowser/v2/storage/bolt" + "github.com/filebrowser/filebrowser/v2/users" +) + +// symlinkShareStorage builds a storage whose single user is rooted at a real +// on-disk scope containing a public share "/shared" with a symlinked +// descendant "link -> ../private". Skips the test if symlinks are unavailable. +func symlinkShareStorage(t *testing.T) *storage.Storage { + t.Helper() + scope := t.TempDir() + if err := os.MkdirAll(filepath.Join(scope, "shared"), 0o755); err != nil { + t.Fatal(err) + } + if err := os.MkdirAll(filepath.Join(scope, "private"), 0o755); err != nil { + t.Fatal(err) + } + if err := os.WriteFile(filepath.Join(scope, "private", "secret.txt"), []byte("symlink-secret"), 0o600); err != nil { + t.Fatal(err) + } + if err := os.Symlink(filepath.Join(scope, "private"), filepath.Join(scope, "shared", "link")); err != nil { + t.Skipf("cannot create symlink on this platform: %v", err) + } + + 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.Share.Save(&share.Link{Hash: "h", UserID: 1, Path: "/shared"}); err != nil { + t.Fatalf("failed to save share: %v", err) + } + if err := st.Users.Save(&users.User{ + Username: "username", + Password: "pw", + Perm: users.Permissions{Share: true, Download: true}, + }); err != nil { + t.Fatalf("failed to save user: %v", err) + } + if err := st.Settings.Save(&settings.Settings{Key: []byte("key")}); err != nil { + t.Fatalf("failed to save settings: %v", err) + } + st.Users = &customFSUser{ + Store: st.Users, + fs: afero.NewBasePathFs(afero.NewOsFs(), scope), + } + return st +} + +// Reproduces GHSA-hf77-9m7w-fq8q: a public directory share whose subtree +// contains a symlink to a directory outside the share. Requesting a regular +// file behind that linked ancestor must NOT disclose its contents. +func TestPublicShareSymlinkDescendantDisclosure(t *testing.T) { + cases := map[string]struct { + handler handleFunc + path string + }{ + "direct file download via dl handler": {handler: publicDlHandler, path: "h/link/secret.txt"}, + "share info via share handler": {handler: publicShareHandler, path: "h/link/secret.txt"}, + "listing of linked dir": {handler: publicShareHandler, path: "h/link/"}, + } + + for name, tc := range cases { + tc := tc + t.Run(name, func(t *testing.T) { + st := symlinkShareStorage(t) + + req := newHTTPRequest(t, func(r *http.Request) { r.URL.Path = tc.path }) + recorder := httptest.NewRecorder() + handler := handle(tc.handler, "", st, &settings.Server{}) + handler.ServeHTTP(recorder, req) + + result := recorder.Result() + defer result.Body.Close() + body, _ := io.ReadAll(result.Body) + + t.Logf("status=%d body=%q", result.StatusCode, string(body)) + if result.StatusCode == http.StatusOK { + t.Errorf("VULNERABLE: leaked path outside share (status 200, body=%q)", string(body)) + } + }) + } +} + +// The listing of the public share root must omit the escaping symlink "link" +// entirely (no target metadata leak). +func TestPublicShareSymlinkListingOmitsEscapingLink(t *testing.T) { + st := symlinkShareStorage(t) + + req := newHTTPRequest(t, func(r *http.Request) { r.URL.Path = "h/" }) + recorder := httptest.NewRecorder() + handler := handle(publicShareHandler, "", st, &settings.Server{}) + handler.ServeHTTP(recorder, req) + + result := recorder.Result() + defer result.Body.Close() + body, _ := io.ReadAll(result.Body) + if result.StatusCode != http.StatusOK { + t.Fatalf("share root listing failed: status=%d body=%q", result.StatusCode, string(body)) + } + if strings.Contains(string(body), "\"link\"") { + t.Errorf("VULNERABLE: listing exposes escaping symlink: %s", string(body)) + } +} + +// Reproduces the archive variant of GHSA-hf77-9m7w-fq8q: downloading the whole +// public share as a zip must not pull in files reached through a symlinked +// descendant. +func TestPublicShareSymlinkArchiveDisclosure(t *testing.T) { + st := symlinkShareStorage(t) + + // Request the whole share root as an archive. + req := newHTTPRequest(t, func(r *http.Request) { r.URL.Path = "h/" }) + recorder := httptest.NewRecorder() + handler := handle(publicDlHandler, "", st, &settings.Server{}) + handler.ServeHTTP(recorder, req) + + result := recorder.Result() + defer result.Body.Close() + body, _ := io.ReadAll(result.Body) + if result.StatusCode != http.StatusOK { + t.Fatalf("archive request failed: status=%d body=%q", result.StatusCode, string(body)) + } + + zr, err := zip.NewReader(bytes.NewReader(body), int64(len(body))) + if err != nil { + t.Fatalf("failed to read zip: %v", err) + } + for _, f := range zr.File { + if strings.Contains(f.Name, "secret.txt") { + t.Errorf("VULNERABLE: archive includes file behind symlinked descendant: %q", f.Name) + } + rc, err := f.Open() + if err != nil { + continue + } + content, _ := io.ReadAll(rc) + rc.Close() + if bytes.Contains(content, []byte("symlink-secret")) { + t.Errorf("VULNERABLE: archive entry %q leaks out-of-scope content", f.Name) + } + } +} diff --git a/http/public_test.go b/http/public_test.go index bc57a9ca..947ee049 100644 --- a/http/public_test.go +++ b/http/public_test.go @@ -220,7 +220,13 @@ func TestPublicShareHandlerRules(t *testing.T) { t.Fatalf("failed to save settings: %v", err) } - fs := afero.NewMemMapFs() + fs := afero.NewBasePathFs(afero.NewOsFs(), t.TempDir()) + if err := fs.MkdirAll("/projects/private", 0o755); err != nil { + t.Fatalf("failed to create private dir: %v", err) + } + if err := fs.MkdirAll("/projects/public", 0o755); err != nil { + t.Fatalf("failed to create public dir: %v", err) + } if err := afero.WriteFile(fs, "/projects/private/secret.txt", []byte("top secret"), 0o600); err != nil { t.Fatalf("failed to write secret file: %v", err) } diff --git a/http/raw.go b/http/raw.go index f0b12455..ab01eed6 100644 --- a/http/raw.go +++ b/http/raw.go @@ -115,6 +115,10 @@ func getFiles(d *data, path, commonPath string) ([]archives.FileInfo, error) { return nil, nil } + if ok, err := files.WithinScope(d.user.Fs, path); err != nil || !ok { + return nil, nil + } + info, err := d.user.Fs.Stat(path) if err != nil { return nil, err diff --git a/http/tus_handlers.go b/http/tus_handlers.go index 31245a34..8a68161d 100644 --- a/http/tus_handlers.go +++ b/http/tus_handlers.go @@ -44,6 +44,11 @@ func tusPostHandler(cache UploadCache) handleFunc { if !d.user.Perm.Create || !d.Check(r.URL.Path) { return http.StatusForbidden, nil } + + if ok, scopeErr := files.WithinScope(d.user.Fs, r.URL.Path); scopeErr != nil || !ok { + return http.StatusForbidden, nil + } + file, err := files.NewFileInfo(&files.FileOptions{ Fs: d.user.Fs, Path: r.URL.Path, @@ -161,6 +166,11 @@ func tusPatchHandler(cache UploadCache) handleFunc { if r.Header.Get("Content-Type") != "application/offset+octet-stream" { return http.StatusUnsupportedMediaType, nil } + // Defense in depth: refuse to write through a symlink that escapes the + // scope, in case the target path is (or sits behind) an escaping link. + if ok, scopeErr := files.WithinScope(d.user.Fs, r.URL.Path); scopeErr != nil || !ok { + return http.StatusForbidden, nil + } uploadOffset, err := getUploadOffset(r) if err != nil { diff --git a/http/tus_symlink_test.go b/http/tus_symlink_test.go new file mode 100644 index 00000000..ac0b8c3d --- /dev/null +++ b/http/tus_symlink_test.go @@ -0,0 +1,118 @@ +package fbhttp + +import ( + "net/http" + "net/http/httptest" + "os" + "path/filepath" + "testing" + "time" + + "github.com/asdine/storm/v3" + "github.com/golang-jwt/jwt/v5" + "github.com/spf13/afero" + + "github.com/filebrowser/filebrowser/v2/settings" + "github.com/filebrowser/filebrowser/v2/storage/bolt" + "github.com/filebrowser/filebrowser/v2/users" +) + +// Reproduces the TUS write vector of GHSA-v9g6-9pp4-3w22: a scoped user must +// not be able to create or write files through a symlinked directory that +// escapes their scope. +func TestTusHandlersRejectSymlinkScopeEscape(t *testing.T) { + root := t.TempDir() + userScope := filepath.Join(root, "user") + outside := filepath.Join(root, "otheruser") + for _, d := range []string{userScope, outside} { + if err := os.MkdirAll(d, 0o755); err != nil { + t.Fatal(err) + } + } + // A directory symlink inside the user's scope pointing outside it. + if err := os.Symlink(outside, filepath.Join(userScope, "escape_link")); err != nil { + t.Skipf("cannot create symlink: %v", err) + } + + key := []byte("test-signing-key") + + 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.Users.Save(&users.User{ + Username: "u", + Password: "pw", + Perm: users.Permissions{Create: true, Modify: true}, + }); err != nil { + t.Fatalf("failed to save user: %v", err) + } + if err := st.Settings.Save(&settings.Settings{Key: key}); err != nil { + t.Fatalf("failed to save settings: %v", err) + } + st.Users = &customFSUser{ + Store: st.Users, + fs: afero.NewBasePathFs(afero.NewOsFs(), userScope), + } + + // Forge a valid auth token for user ID 1. + claims := &authToken{ + User: userInfo{ID: 1, Username: "u", Perm: users.Permissions{Create: true, Modify: true}}, + RegisteredClaims: jwt.RegisteredClaims{ + IssuedAt: jwt.NewNumericDate(time.Now().Add(-time.Minute)), + ExpiresAt: jwt.NewNumericDate(time.Now().Add(time.Hour)), + }, + } + signed, err := jwt.NewWithClaims(jwt.SigningMethodHS256, claims).SignedString(key) + if err != nil { + t.Fatalf("failed to sign token: %v", err) + } + + cases := map[string]struct { + method string + handler handleFunc + headers map[string]string + }{ + "POST create through symlinked dir": { + method: http.MethodPost, + handler: tusPostHandler(newMemoryUploadCache()), + headers: map[string]string{"Upload-Length": "20"}, + }, + "PATCH write through symlinked dir": { + method: http.MethodPatch, + handler: tusPatchHandler(newMemoryUploadCache()), + headers: map[string]string{"Content-Type": "application/offset+octet-stream", "Upload-Offset": "0"}, + }, + } + + for name, tc := range cases { + tc := tc + t.Run(name, func(t *testing.T) { + req, err := http.NewRequest(tc.method, "escape_link/injected.txt", http.NoBody) + if err != nil { + t.Fatal(err) + } + req.Header.Set("X-Auth", signed) + for k, v := range tc.headers { + req.Header.Set(k, v) + } + + recorder := httptest.NewRecorder() + handler := handle(tc.handler, "", st, &settings.Server{}) + handler.ServeHTTP(recorder, req) + + if recorder.Code != http.StatusForbidden { + t.Errorf("expected 403, got %d", recorder.Code) + } + if _, statErr := os.Stat(filepath.Join(outside, "injected.txt")); statErr == nil { + t.Errorf("VULNERABLE: file was created outside the user's scope") + } + }) + } +}