archive: fix zip slip path traversal in untrusted zip files GHSA-66hp-wgxq-6f5q CVE-PENDING

The zip backend mounts a zip file as a browsable Fs. Go's archive/zip
does not sanitize entry names, and readZip applied path.Clean but did
not reject a cleaned name that still pointed outside the archive. A
crafted zip could make rclone copy/sync attempt writes outside the
intended destination.

Sanitize entry names with sanitize.Path - the same check used by
rclone archive extract - skipping any entry with a ".." path
component, whether separated by "/" or "\". A backslash is otherwise
kept as an ordinary character in the name, as archive extract does. It
is up to the destination backend to make names safe for its storage.

Skipped entries are logged as a single count per archive so a crafted
archive with many escaping entries cannot flood the log.

backend/archive/zip/zip.go

@@ -18,6 +18,7 @@ import (
 	"github.com/rclone/rclone/fs/hash"
 	"github.com/rclone/rclone/fs/log"
 	"github.com/rclone/rclone/lib/readers"
+	"github.com/rclone/rclone/lib/sanitize"
 	"github.com/rclone/rclone/vfs"
 	"github.com/rclone/rclone/vfs/vfscommon"
 )
@@ -135,10 +136,13 @@ func (f *Fs) readZip() (singleObject bool, err error) {
 		return singleObject, fmt.Errorf("failed to read zip file: %w", err)
 	}
 	dt := dirtree.New()
+	skipped := 0
 	for _, file := range zr.File {
-		remote := strings.Trim(path.Clean(file.Name), "/")
-		if remote == "." {
-			remote = ""
+		// Skip entries whose name escapes the archive's own namespace
+		remote, err := sanitize.Path(file.Name)
+		if err != nil {
+			skipped++
+			continue
 		}
 		remote = path.Join(f.prefix, remote)
 		if f.root != "" {
@@ -173,6 +177,9 @@ func (f *Fs) readZip() (singleObject bool, err error) {
 			}
 		}
 	}
+	if skipped > 0 {
+		fs.Logf(f, "Skipped %d zip entries which escape the archive", skipped)
+	}
 	dt.CheckParents("")
 	dt.Sort()
 	f.dt = dt

backend/archive/zip/zip_internal_test.go

@@ -0,0 +1,85 @@
+package zip
+
+import (
+	"archive/zip"
+	"bytes"
+	"context"
+	"os"
+	"path/filepath"
+	"sort"
+	"strings"
+	"testing"
+
+	_ "github.com/rclone/rclone/backend/local"
+	"github.com/rclone/rclone/fs"
+	"github.com/rclone/rclone/fs/cache"
+	"github.com/stretchr/testify/assert"
+	"github.com/stretchr/testify/require"
+)
+
+// writeZip builds a zip file with the given entry names (a name ending
+// in "/" is a directory) in dir and returns its leaf name.
+func writeZip(t *testing.T, dir, name string, names ...string) string {
+	var buf bytes.Buffer
+	zw := zip.NewWriter(&buf)
+	for _, n := range names {
+		w, err := zw.Create(n)
+		require.NoError(t, err)
+		if !strings.HasSuffix(n, "/") {
+			_, err = w.Write([]byte("data for " + n))
+			require.NoError(t, err)
+		}
+	}
+	require.NoError(t, zw.Close())
+	require.NoError(t, os.WriteFile(filepath.Join(dir, name), buf.Bytes(), 0600))
+	return name
+}
+
+// allRemotes returns every Object remote in the mounted archive's dirtree.
+func allRemotes(t *testing.T, f fs.Fs) []string {
+	zf, ok := f.(*Fs)
+	require.True(t, ok)
+	var remotes []string
+	for _, entries := range zf.dt {
+		for _, entry := range entries {
+			if o, ok := entry.(*Object); ok {
+				remotes = append(remotes, o.Remote())
+			}
+		}
+	}
+	// dt iteration order is nondeterministic, so sort for stable comparison.
+	sort.Strings(remotes)
+	return remotes
+}
+
+// A malicious zip whose entry names escape the archive's own namespace
+// must not be exposed by the zip backend (Zip Slip).
+func TestReadZipSlip(t *testing.T) {
+	ctx := context.Background()
+	dir := t.TempDir()
+	name := writeZip(t, dir, "evil.zip",
+		"good.txt",
+		"../../etc/cron.d/evil",
+		"../escape.txt",
+		"sub/../../above.txt",
+		`..\..\evil.exe`,
+	)
+
+	localFs, err := cache.Get(ctx, dir)
+	require.NoError(t, err)
+
+	f, err := New(ctx, localFs, name, "", "")
+	require.NoError(t, err)
+
+	remotes := allRemotes(t, f)
+
+	// The one benign entry must survive.
+	assert.Contains(t, remotes, "good.txt")
+
+	// No escaping entry may be exposed.
+	for _, remote := range remotes {
+		assert.False(t, remote == ".." || strings.HasPrefix(remote, "../"),
+			"escaping remote exposed: %q", remote)
+	}
+	assert.Equal(t, []string{"good.txt"}, remotes)
+}