diff --git a/files/file.go b/files/file.go index e84c7e2a..88990feb 100644 --- a/files/file.go +++ b/files/file.go @@ -475,10 +475,6 @@ 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 } diff --git a/fileutils/copy_test.go b/fileutils/copy_test.go new file mode 100644 index 00000000..f9cad6f6 --- /dev/null +++ b/fileutils/copy_test.go @@ -0,0 +1,74 @@ +package fileutils + +import ( + "os" + "path/filepath" + "testing" + + "github.com/spf13/afero" +) + +// Copying an in-scope directory that contains a symlink whose target escapes +// the user's scope must not dereference that symlink into the destination. +// Otherwise a scoped user could exfiltrate out-of-scope file content via the +// recursive copy path (GHSA-c2gv-wf5f-hjhh, an incomplete fix of +// GHSA-239w-m3h6-ch8v). +func TestCopyDoesNotDereferenceEscapingSymlink(t *testing.T) { + base := t.TempDir() + scope := filepath.Join(base, "scope") + if err := os.MkdirAll(filepath.Join(scope, "srcdir"), 0o755); err != nil { + t.Fatal(err) + } + + // A secret living outside the scope. + secret := filepath.Join(base, "secret.txt") + if err := os.WriteFile(secret, []byte("OUT-OF-SCOPE-SECRET"), 0o644); err != nil { + t.Fatal(err) + } + + // An escaping symlink planted inside the user's scope. + if err := os.Symlink(secret, filepath.Join(scope, "srcdir", "link.txt")); err != nil { + t.Skipf("cannot create symlink: %v", err) + } + + afs := afero.NewBasePathFs(afero.NewOsFs(), scope) + + err := Copy(afs, "/srcdir", "/dstdir", 0o644, 0o755) + if err == nil { + t.Fatal("expected copy of a directory containing an escaping symlink to fail") + } + + // The escaping symlink's target content must not have landed in scope. + if data, readErr := afero.ReadFile(afs, "/dstdir/link.txt"); readErr == nil { + t.Fatalf("escaping symlink was dereferenced into scope: got %q", string(data)) + } +} + +// A symlink whose target stays within scope is legitimate and must still be +// copied (dereferenced) so the fix does not over-block normal usage. +func TestCopyAllowsInScopeSymlink(t *testing.T) { + scope := t.TempDir() + if err := os.MkdirAll(filepath.Join(scope, "srcdir", "real"), 0o755); err != nil { + t.Fatal(err) + } + if err := os.WriteFile(filepath.Join(scope, "srcdir", "real", "f.txt"), []byte("in-scope"), 0o644); err != nil { + t.Fatal(err) + } + if err := os.Symlink(filepath.Join(scope, "srcdir", "real", "f.txt"), filepath.Join(scope, "srcdir", "link.txt")); err != nil { + t.Skipf("cannot create symlink: %v", err) + } + + afs := afero.NewBasePathFs(afero.NewOsFs(), scope) + + if err := Copy(afs, "/srcdir", "/dstdir", 0o644, 0o755); err != nil { + t.Fatalf("expected copy of an in-scope symlink to succeed, got: %v", err) + } + + data, err := afero.ReadFile(afs, "/dstdir/link.txt") + if err != nil { + t.Fatalf("expected in-scope symlink to be copied, got: %v", err) + } + if string(data) != "in-scope" { + t.Fatalf("unexpected copied content: %q", string(data)) + } +} diff --git a/fileutils/dir.go b/fileutils/dir.go index e0b049db..0b127c48 100644 --- a/fileutils/dir.go +++ b/fileutils/dir.go @@ -3,14 +3,24 @@ package fileutils import ( "errors" "io/fs" + "os" "github.com/spf13/afero" + + "github.com/filebrowser/filebrowser/v2/files" ) // CopyDir copies a directory from source to dest and all // of its sub-directories. It doesn't stop if it finds an error // during the copy. Returns an error if any. func CopyDir(afs afero.Fs, source, dest string, fileMode, dirMode fs.FileMode) error { + if ok, err := files.WithinScope(afs, source); err != nil || !ok { + if err != nil { + return err + } + return os.ErrPermission + } + // Get properties of source. srcinfo, err := afs.Stat(source) if err != nil { diff --git a/fileutils/file.go b/fileutils/file.go index 784f728f..ab8c7fff 100644 --- a/fileutils/file.go +++ b/fileutils/file.go @@ -8,6 +8,8 @@ import ( "path/filepath" "github.com/spf13/afero" + + "github.com/filebrowser/filebrowser/v2/files" ) // MoveFile moves file from src to dst. @@ -32,6 +34,13 @@ func MoveFile(afs afero.Fs, src, dst string, fileMode, dirMode fs.FileMode) erro // CopyFile copies a file from source to dest and returns // an error if any. func CopyFile(afs afero.Fs, source, dest string, fileMode, dirMode fs.FileMode) error { + if ok, err := files.WithinScope(afs, source); err != nil || !ok { + if err != nil { + return err + } + return os.ErrPermission + } + // Open the source file. src, err := afs.Open(source) if err != nil { diff --git a/http/resource.go b/http/resource.go index 5f06a7cd..27eb8b70 100644 --- a/http/resource.go +++ b/http/resource.go @@ -228,10 +228,6 @@ func resourcePatchHandler(fileCache FileCache) handleFunc { return http.StatusForbidden, nil } - // Refuse to copy/move through a symlink that escapes the user's scope, - // for either the source (read escape) or the destination (write - // escape). fileutils.Copy/MoveFile operate on the raw afero FS and - // follow symlinks, so they bypass the guards in stat()/writeFile(). for _, p := range []string{src, dst} { if ok, scopeErr := files.WithinScope(d.user.Fs, p); scopeErr != nil || !ok { if scopeErr != nil { @@ -308,9 +304,6 @@ func addVersionSuffix(source string, afs afero.Fs) string { } func writeFile(afs afero.Fs, dst string, in io.Reader, fileMode, dirMode fs.FileMode) (os.FileInfo, error) { - // Refuse to write through a symlink that escapes the user's scope, so an - // overwrite of an existing escaping symlink cannot modify a file outside - // the boundary. if ok, err := files.WithinScope(afs, dst); err != nil || !ok { return nil, os.ErrPermission } diff --git a/http/tus_handlers.go b/http/tus_handlers.go index 8a68161d..2629bd09 100644 --- a/http/tus_handlers.go +++ b/http/tus_handlers.go @@ -166,8 +166,7 @@ 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 }