diff --git a/files/file_test.go b/files/file_test.go index b5a8b37f..45b114f6 100644 --- a/files/file_test.go +++ b/files/file_test.go @@ -92,6 +92,125 @@ func TestScopedFs(t *testing.T) { t.Fatalf("expected in-scope symlink target to be accessible, got %v", err) } }) + + // Regression for the dangling-symlink write escape (GHSA-8wc8-hf36-mjh9 / + // GHSA-fh54-6rfh-r8f3): a symlink whose target does not exist yet must not be + // followed for writes. Previously within() validated the link's in-scope + // parent directory, so OpenFile(O_CREATE) dereferenced the link and created + // the file at its out-of-scope target. + t.Run("write through a dangling escaping symlink is rejected", func(t *testing.T) { + base := t.TempDir() + scope := filepath.Join(base, "scope") + outside := filepath.Join(base, "outside") + for _, d := range []string{scope, outside} { + if err := os.MkdirAll(d, 0o755); err != nil { + t.Fatal(err) + } + } + outsideTarget := filepath.Join(outside, "created.txt") // does not exist yet + if err := os.Symlink(outsideTarget, filepath.Join(scope, "evil")); err != nil { + t.Skipf("cannot create symlink: %v", err) + } + fs := NewScopedFs(afero.NewOsFs(), scope) + + f, err := fs.OpenFile("/evil", os.O_RDWR|os.O_CREATE|os.O_TRUNC, 0o644) + if err == nil { + _ = f.Close() + t.Fatal("VULNERABLE: write through a dangling escaping symlink was allowed") + } + if !os.IsPermission(err) { + t.Fatalf("expected permission error, got %v", err) + } + if _, statErr := os.Stat(outsideTarget); statErr == nil { + t.Fatal("VULNERABLE: file was created outside the scope") + } + }) + + // A dangling *relative* symlink that lives under an escaping directory + // symlink must be resolved against the link's real directory, not its lexical + // parent. Otherwise the symlinked ancestor can shift the computed target back + // into scope while the real OS write lands outside it. + t.Run("write through a dangling relative symlink under a symlinked dir is rejected", func(t *testing.T) { + base := t.TempDir() + scope := filepath.Join(base, "scope") + outside := filepath.Join(base, "outside") + for _, d := range []string{scope, outside} { + if err := os.MkdirAll(d, 0o755); err != nil { + t.Fatal(err) + } + } + // An escaping directory symlink inside the scope: /scope/m -> /base/outside. + if err := os.Symlink(outside, filepath.Join(scope, "m")); err != nil { + t.Skipf("cannot create symlink: %v", err) + } + // A relative dangling symlink inside the escaping dir whose target, + // resolved against the real directory (/base/outside), is /base/escaped — + // outside the scope. + if err := os.Symlink("../escaped", filepath.Join(outside, "evil")); err != nil { + t.Skipf("cannot create symlink: %v", err) + } + fs := NewScopedFs(afero.NewOsFs(), scope) + + f, err := fs.OpenFile("/m/evil", os.O_RDWR|os.O_CREATE|os.O_TRUNC, 0o644) + if err == nil { + _ = f.Close() + t.Fatal("VULNERABLE: write through a dangling relative symlink under a symlinked dir was allowed") + } + if !os.IsPermission(err) { + t.Fatalf("expected permission error, got %v", err) + } + if _, statErr := os.Stat(filepath.Join(base, "escaped")); statErr == nil { + t.Fatal("VULNERABLE: file was created outside the scope") + } + }) + + // Regression for the symlink-following delete escape (GHSA-hq4g-mpch-f9vp / + // GHSA-fmm7-x4gx-8jhr): Remove/RemoveAll used to skip guard(), so RemoveAll + // followed a symlinked ancestor escaping the scope and deleted an + // out-of-scope file. + t.Run("RemoveAll through an escaping symlink is rejected", func(t *testing.T) { + base := t.TempDir() + scope := filepath.Join(base, "scope") + outside := filepath.Join(base, "outside") + for _, d := range []string{scope, outside} { + if err := os.MkdirAll(d, 0o755); err != nil { + t.Fatal(err) + } + } + victim := filepath.Join(outside, "victim.txt") + if err := os.WriteFile(victim, []byte("keep"), 0o644); err != nil { + t.Fatal(err) + } + if err := os.Symlink(outside, filepath.Join(scope, "link")); err != nil { + t.Skipf("cannot create symlink: %v", err) + } + fs := NewScopedFs(afero.NewOsFs(), scope) + + if err := fs.RemoveAll("/link/victim.txt"); !os.IsPermission(err) { + t.Fatalf("expected RemoveAll through escaping symlink to be rejected, got %v", err) + } + if _, statErr := os.Stat(victim); statErr != nil { + t.Fatalf("VULNERABLE: out-of-scope victim file was deleted: %v", statErr) + } + }) + + // The guard added for the delete escape must not break legitimate deletes of + // in-scope files. + t.Run("RemoveAll of an in-scope file is allowed", func(t *testing.T) { + scope := t.TempDir() + target := filepath.Join(scope, "deleteme.txt") + if err := os.WriteFile(target, []byte("x"), 0o644); err != nil { + t.Fatal(err) + } + fs := NewScopedFs(afero.NewOsFs(), scope) + + if err := fs.RemoveAll("/deleteme.txt"); err != nil { + t.Fatalf("expected in-scope RemoveAll to succeed, got %v", err) + } + if _, statErr := os.Stat(target); statErr == nil { + t.Fatal("expected in-scope file to be deleted") + } + }) } // stat must reject a regular file reached through a symlinked ancestor that diff --git a/files/scoped.go b/files/scoped.go index eef6378b..dcc27a12 100644 --- a/files/scoped.go +++ b/files/scoped.go @@ -25,6 +25,11 @@ var ( _ afero.Lstater = (*ScopedFs)(nil) ) +// maxSymlinkHops bounds how many dangling symlinks within() will follow before +// giving up, so a pathological chain cannot loop forever. It mirrors the kernel +// MAXSYMLINKS limit; the operation is rejected once the bound is exceeded. +const maxSymlinkHops = 255 + func NewScopedFs(source afero.Fs, path string) *ScopedFs { if s, ok := source.(*ScopedFs); ok { source = s.base @@ -61,13 +66,10 @@ func (s *ScopedFs) guard(name string) error { // // Paths that do not exist yet (e.g. a brand-new file being created) are // validated against their nearest existing ancestor, so legitimate new files -// are always allowed. -// -// Note: a dangling symlink whose target does not yet exist resolves to its -// containing directory and is therefore allowed; writing through such a link -// could still create a file outside the scope. This is treated as best-effort -// and relies on rejecting existing escaping symlinks, which covers the -// disclosure and overwrite vectors. +// are always allowed. A dangling symlink — a link whose target does not exist +// yet — is the exception: it is followed to where it points and validated +// there, so a write cannot dereference the link to create a file outside the +// scope. func (s *ScopedFs) within(p string) (bool, error) { root, err := filepath.EvalSymlinks(afero.FullBaseFsPath(s.base, "/")) if err != nil { @@ -76,12 +78,43 @@ func (s *ScopedFs) within(p string) (bool, error) { target := afero.FullBaseFsPath(s.base, p) resolved, err := filepath.EvalSymlinks(target) - for errors.Is(err, fs.ErrNotExist) { - parent := filepath.Dir(target) - if parent == target { - break + // When target does not resolve, work out where the operation would actually + // land. A non-existent regular path resolves to the file that would be + // created inside its containing directory, so walk up to the nearest + // existing ancestor and validate that. But when target itself is a dangling + // symlink, follow it one level instead: validating its lexical parent would + // wrongly accept a link pointing outside the scope, letting a write follow + // the link and create the file out of bounds. + for hops := 0; errors.Is(err, fs.ErrNotExist); { + if fi, lerr := os.Lstat(target); lerr == nil && fi.Mode()&os.ModeSymlink != 0 { + hops++ + if hops > maxSymlinkHops { + return false, os.ErrPermission + } + dest, rerr := os.Readlink(target) + if rerr != nil { + return false, rerr + } + if !filepath.IsAbs(dest) { + // Resolve the link relative to the directory that really contains + // it, not its lexical parent: a symlinked ancestor could otherwise + // shift the computed target back into scope while the real write + // lands outside it. The parent is guaranteed to resolve here + // because os.Lstat above already traversed it. + base, berr := filepath.EvalSymlinks(filepath.Dir(target)) + if berr != nil { + return false, berr + } + dest = filepath.Join(base, dest) + } + target = filepath.Clean(dest) + } else { + parent := filepath.Dir(target) + if parent == target { + break + } + target = parent } - target = parent resolved, err = filepath.EvalSymlinks(target) } if err != nil { @@ -136,10 +169,16 @@ func (s *ScopedFs) OpenFile(name string, flag int, perm os.FileMode) (afero.File } func (s *ScopedFs) Remove(name string) error { + if err := s.guard(name); err != nil { + return err + } return s.base.Remove(name) } func (s *ScopedFs) RemoveAll(path string) error { + if err := s.guard(path); err != nil { + return err + } return s.base.RemoveAll(path) } diff --git a/http/resource_test.go b/http/resource_test.go index 7081206f..e22d152b 100644 --- a/http/resource_test.go +++ b/http/resource_test.go @@ -5,6 +5,7 @@ import ( "net/http/httptest" "os" "path/filepath" + "strings" "testing" "time" @@ -14,6 +15,7 @@ import ( "github.com/filebrowser/filebrowser/v2/diskcache" "github.com/filebrowser/filebrowser/v2/settings" + "github.com/filebrowser/filebrowser/v2/storage" "github.com/filebrowser/filebrowser/v2/storage/bolt" "github.com/filebrowser/filebrowser/v2/users" ) @@ -115,3 +117,108 @@ func signToken(t *testing.T, perm users.Permissions, key []byte) string { } return signed } + +// scopedUserStorage returns a storage whose single user (ID 1) is scoped to +// userScope through a symlink-confining ScopedFs (via customFSUser), mirroring +// production. Used by the symlink scope-escape regression tests below. +func scopedUserStorage(t *testing.T, userScope string, perm users.Permissions, key []byte) *storage.Storage { + t.Helper() + 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: perm}); 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), + } + return st +} + +// Regression for the dangling-symlink write escape (GHSA-8wc8-hf36-mjh9 / +// GHSA-fh54-6rfh-r8f3): POSTing to an in-scope dangling symlink whose target is +// outside the scope must not dereference the link to create the out-of-scope +// file. +func TestResourcePostRejectsDanglingSymlinkWriteEscape(t *testing.T) { + root := t.TempDir() + userScope := filepath.Join(root, "user") + outside := filepath.Join(root, "outside") + for _, d := range []string{userScope, outside} { + if err := os.MkdirAll(d, 0o755); err != nil { + t.Fatal(err) + } + } + // A dangling symlink inside the scope pointing at a not-yet-existing file + // outside it (planted out-of-band, per the advisory preconditions). + outsideTarget := filepath.Join(outside, "created.txt") + if err := os.Symlink(outsideTarget, filepath.Join(userScope, "evil")); err != nil { + t.Skipf("cannot create symlink: %v", err) + } + + key := []byte("test-signing-key") + perm := users.Permissions{Create: true, Modify: true} + st := scopedUserStorage(t, userScope, perm, key) + signed := signToken(t, perm, key) + + req, _ := http.NewRequest(http.MethodPost, "/evil?override=true", strings.NewReader("http-outside")) + req.Header.Set("X-Auth", signed) + rec := httptest.NewRecorder() + handle(resourcePostHandler(diskcache.NewNoOp()), "", st, &settings.Server{}).ServeHTTP(rec, req) + + if _, statErr := os.Stat(outsideTarget); statErr == nil { + data, _ := os.ReadFile(outsideTarget) + t.Fatalf("VULNERABLE: out-of-scope file created via dangling symlink (status=%d, content=%q)", rec.Code, string(data)) + } + if rec.Code != http.StatusForbidden { + t.Errorf("expected 403, got %d body=%q", rec.Code, rec.Body.String()) + } +} + +// Regression for the symlink-following delete escape (GHSA-hq4g-mpch-f9vp / +// GHSA-fmm7-x4gx-8jhr): a Create-only user POSTing to a child of an escaping +// symlinked directory must not delete the out-of-scope target through the +// failed-upload cleanup RemoveAll. +func TestResourcePostCleanupDoesNotDeleteThroughSymlink(t *testing.T) { + root := t.TempDir() + userScope := filepath.Join(root, "user") + outside := filepath.Join(root, "outside") + for _, d := range []string{userScope, outside} { + if err := os.MkdirAll(d, 0o755); err != nil { + t.Fatal(err) + } + } + victim := filepath.Join(outside, "victim.txt") + if err := os.WriteFile(victim, []byte("keep"), 0o644); err != nil { + t.Fatal(err) + } + // An escaping directory symlink inside the scope (planted out-of-band). + if err := os.Symlink(outside, filepath.Join(userScope, "link")); err != nil { + t.Skipf("cannot create symlink: %v", err) + } + + key := []byte("test-signing-key") + // Create-only: Perm.Delete is deliberately false — the bug must not need it. + perm := users.Permissions{Create: true} + st := scopedUserStorage(t, userScope, perm, key) + signed := signToken(t, perm, key) + + req, _ := http.NewRequest(http.MethodPost, "/link/victim.txt", strings.NewReader("x")) + req.Header.Set("X-Auth", signed) + rec := httptest.NewRecorder() + handle(resourcePostHandler(diskcache.NewNoOp()), "", st, &settings.Server{}).ServeHTTP(rec, req) + + if _, statErr := os.Stat(victim); statErr != nil { + t.Fatalf("VULNERABLE: out-of-scope victim.txt deleted by cleanup RemoveAll (status=%d): %v", rec.Code, statErr) + } +}