fix: dangling symlink, write, delete scope bugs

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

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) {
+	// 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
+		}
 		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)
 }
 

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)
+	}
+}