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

@@ -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
 }
 

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

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