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