diff --git a/backend/archive/zip/zip.go b/backend/archive/zip/zip.go index 17511b76b..ec9ba77d7 100644 --- a/backend/archive/zip/zip.go +++ b/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 diff --git a/backend/archive/zip/zip_internal_test.go b/backend/archive/zip/zip_internal_test.go new file mode 100644 index 000000000..f0bb3bb03 --- /dev/null +++ b/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) +}