From e1b0c090406c989dd0b62aeb58410f6e5c6522bd Mon Sep 17 00:00:00 2001 From: Nick Craig-Wood Date: Tue, 25 Aug 2026 12:46:11 +0100 Subject: [PATCH] archive: hide any archive entry which escapes the directory being listed GHSA-66hp-wgxq-6f5q Whether an archive entry name can escape the archive's namespace was left entirely to each archiver. Enforce it in the archive backend too. List only passes on direct children of the directory listed and NewObject only returns the object asked for, so a future archiver which forgets to validate names cannot expose a traversal to fs/sync and fs/operations. --- backend/archive/archive.go | 39 +++++++++++-- backend/archive/archive_internal_test.go | 72 ++++++++++++++++++++++++ backend/archive/archiver/archiver.go | 5 ++ 3 files changed, 112 insertions(+), 4 deletions(-) diff --git a/backend/archive/archive.go b/backend/archive/archive.go index ec0e6c36c..25e489581 100644 --- a/backend/archive/archive.go +++ b/backend/archive/archive.go @@ -495,24 +495,50 @@ func (f *Fs) List(ctx context.Context, dir string) (entries fs.DirEntries, err e return nil, err } - entries, err = subFs.List(ctx, dir) + subEntries, err := subFs.List(ctx, dir) if err != nil { return nil, err } - for i, entry := range entries { + entries = subEntries[:0] + skipped := 0 + for _, entry := range subEntries { + remote := entry.Remote() + // Only pass on direct children of dir - anything else + // could escape the archive's namespace + if !isDirectChild(dir, remote) { + fs.Debugf(f, "Skipping entry %q which is not in directory %q", remote, dir) + skipped++ + continue + } // Can only unarchive files if o, ok := entry.(fs.Object); ok { - remote := o.Remote() archive := f.findArchive(remote) if archive != nil { // Overwrite entry with directory - entries[i] = fs.NewDir(remote, o.ModTime(ctx)) + entry = fs.NewDir(remote, o.ModTime(ctx)) } } + entries = append(entries, entry) + } + if skipped > 0 { + fs.Logf(f, "Skipped %d entries in %q whose names escape the directory", skipped, dir) } return entries, nil } +// isDirectChild reports whether remote names an entry directly in dir +// ("" being the root), with no ".." or other components in between. +func isDirectChild(dir, remote string) bool { + if remote == "" || remote == "." || remote == ".." || strings.HasSuffix(remote, "/") { + return false + } + parent := path.Dir(remote) + if parent == "." || parent == "/" { + parent = "" + } + return parent == dir && path.Clean(remote) == remote +} + // NewObject creates a new remote archive file object func (f *Fs) NewObject(ctx context.Context, remote string) (fs.Object, error) { @@ -530,6 +556,11 @@ func (f *Fs) NewObject(ctx context.Context, remote string) (fs.Object, error) { if err != nil { return nil, err } + // Don't trust the archiver to have returned the object asked for + if o.Remote() != remote { + fs.Debugf(f, "Ignoring object %q returned for %q", o.Remote(), remote) + return nil, fs.ErrorObjectNotFound + } return o, nil } diff --git a/backend/archive/archive_internal_test.go b/backend/archive/archive_internal_test.go index f0ea6a253..75509e6f7 100644 --- a/backend/archive/archive_internal_test.go +++ b/backend/archive/archive_internal_test.go @@ -16,6 +16,7 @@ import ( "strings" "testing" + "github.com/rclone/rclone/backend/archive/archiver" _ "github.com/rclone/rclone/backend/local" "github.com/rclone/rclone/fs" "github.com/rclone/rclone/fs/cache" @@ -307,3 +308,74 @@ func TestArchiveUncleanRoot(t *testing.T) { }) } } + +// escapingObject is an object whose remote is not where it was asked for. +type escapingObject struct { + fs.Object + remote string +} + +func (o *escapingObject) Remote() string { return o.remote } +func (o *escapingObject) String() string { return o.remote } + +// escapingFs stands in for a badly behaved archiver which exposes entry +// names outside the directory being listed. +type escapingFs struct { + fs.Fs + prefix string +} + +func (f *escapingFs) Name() string { return "escaping" } +func (f *escapingFs) Root() string { return "" } +func (f *escapingFs) String() string { return "escaping" } +func (f *escapingFs) Features() *fs.Features { return &fs.Features{} } + +func (f *escapingFs) List(ctx context.Context, dir string) (fs.DirEntries, error) { + return fs.DirEntries{ + &escapingObject{remote: path.Join(dir, "good.txt")}, + fs.NewDir(path.Join(dir, "gooddir"), fstest.Time("2001-02-03T04:05:06.499999999Z")), + &escapingObject{remote: path.Join(dir, "../escape.txt")}, + &escapingObject{remote: "../../escape.txt"}, + &escapingObject{remote: path.Join(dir, "sub/notachild.txt")}, + fs.NewDir("../escapedir", fstest.Time("2001-02-03T04:05:06.499999999Z")), + }, nil +} + +func (f *escapingFs) NewObject(ctx context.Context, remote string) (fs.Object, error) { + return &escapingObject{remote: "../escape.txt"}, nil +} + +// TestArchiveEscapingArchiver checks that the archive backend does not +// pass on entries from an archiver which escape the directory being +// listed, whatever the archiver does. +func TestArchiveEscapingArchiver(t *testing.T) { + fstest.Initialise() + ctx := context.Background() + + archiver.Register(archiver.Archiver{ + New: func(ctx context.Context, f fs.Fs, remote, prefix, root string) (fs.Fs, error) { + return &escapingFs{prefix: prefix}, nil + }, + Extension: ".escaping", + }) + + dir := t.TempDir() + require.NoError(t, os.WriteFile(filepath.Join(dir, "test.escaping"), []byte("x"), 0600)) + f, err := cache.Get(ctx, ":archive:"+dir) + require.NoError(t, err) + + // Archives are discovered when their parent directory is listed + _, err = f.List(ctx, "") + require.NoError(t, err) + + entries, err := f.List(ctx, "test.escaping") + require.NoError(t, err) + var remotes []string + for _, entry := range entries { + remotes = append(remotes, entry.Remote()) + } + assert.ElementsMatch(t, []string{"test.escaping/good.txt", "test.escaping/gooddir"}, remotes) + + _, err = f.NewObject(ctx, "test.escaping/file.txt") + assert.ErrorIs(t, err, fs.ErrorObjectNotFound) +} diff --git a/backend/archive/archiver/archiver.go b/backend/archive/archiver/archiver.go index a065960fb..66043602c 100644 --- a/backend/archive/archiver/archiver.go +++ b/backend/archive/archiver/archiver.go @@ -8,6 +8,11 @@ import ( ) // Archiver describes an archive package +// +// Entry names inside an archive are attacker controlled. An archiver +// must not expose an entry whose remote escapes the archive's own +// namespace (for example one with a ".." component), typically by +// validating names with lib/sanitize. type Archiver struct { // New constructs an Fs from the (wrappedFs, remote) with the objects // prefix with prefix and rooted at root