diff --git a/cmd/archive/archive_test.go b/cmd/archive/archive_test.go index e9678afef..edef3b880 100644 --- a/cmd/archive/archive_test.go +++ b/cmd/archive/archive_test.go @@ -1,6 +1,8 @@ package archive_test import ( + "archive/zip" + "bytes" "context" "strings" "testing" @@ -170,6 +172,60 @@ func TestIntegration(t *testing.T) { testArchive(t) } +// craftZip builds an in-memory zip archive with the given entries, returning +// it as a string. The entry names are written verbatim so they can include +// crafted path traversal sequences that the archive create command would +// never produce from real files. +func craftZip(t *testing.T, entries []struct{ name, content string }) string { + var buf bytes.Buffer + zw := zip.NewWriter(&buf) + for _, e := range entries { + w, err := zw.CreateHeader(&zip.FileHeader{Name: e.name, Method: zip.Store}) + require.NoError(t, err) + _, err = w.Write([]byte(e.content)) + require.NoError(t, err) + } + require.NoError(t, zw.Close()) + return buf.String() +} + +// TestExtractPathTraversal checks that a crafted archive whose entries try to +// escape the destination directory with ".." components is rejected and does +// not write anything outside the destination (GHSA-4vr5-p2gc-h23p). +func TestExtractPathTraversal(t *testing.T) { + ctx := context.Background() + + for _, tc := range []struct { + name string + entry string + }{ + {name: "forward slash", entry: "../escaped.txt"}, + {name: "backslash", entry: `..\escaped.txt`}, + } { + t.Run(tc.name, func(t *testing.T) { + r := fstest.NewRun(t) + + zipData := craftZip(t, []struct{ name, content string }{ + {"safe.txt", "safe content"}, + {tc.entry, "escaped content"}, + }) + r.WriteObject(ctx, "malicious.zip", zipData, t1) + + // Extract into a subdirectory so a successful escape would land + // in "safe/escaped.txt", a sibling of the "safe/prefix" target. + err := extract.ArchiveExtract(ctx, r.Flocal, "safe/prefix", r.Fremote, "malicious.zip") + require.Error(t, err) + assert.Contains(t, err.Error(), "..") + + // Nothing should have escaped the destination directory. + for _, escaped := range []string{"safe/escaped.txt", "escaped.txt"} { + _, err := r.Flocal.NewObject(ctx, escaped) + assert.ErrorIs(t, err, fs.ErrorObjectNotFound, escaped) + } + }) + } +} + func TestMemory(t *testing.T) { if *fstest.RemoteName != "" { t.Skip("skipping as -remote is set") diff --git a/cmd/archive/extract/extract.go b/cmd/archive/extract/extract.go index ce7c981e2..0ddbc7ed7 100644 --- a/cmd/archive/extract/extract.go +++ b/cmd/archive/extract/extract.go @@ -150,19 +150,13 @@ func ArchiveExtract(ctx context.Context, dst fs.Fs, dstDir string, src fs.Fs, sr } // extract files err = ex.Extract(ctx, in, func(ctx context.Context, f archives.FileInfo) error { - remote := f.NameInArchive - // Strip leading "./" from archive paths. Tar files created with - // relative paths (e.g. "tar -czf archive.tar.gz .") use "./" prefixed - // entries. Without stripping, rclone encodes the "." as a full-width - // dot character creating a spurious directory. We only strip "./" - // specifically to avoid enabling path traversal attacks via "../". - remote = strings.TrimPrefix(remote, "./") - // If the entry was exactly "./" (the root dir), skip it - if remote == "" && f.IsDir() { - return nil + remote, err := destPath(f.NameInArchive, dstDir) + if err != nil { + return err } - if dstDir != "" { - remote = path.Join(dstDir, remote) + // Skip the archive root entry ("./") which has no name of its own. + if remote == "" { + return nil } // check if file should be extracted if !fi.Include(remote, f.Size(), f.ModTime(), fs.Metadata{}) { @@ -203,3 +197,38 @@ func ArchiveExtract(ctx context.Context, dst fs.Fs, dstDir string, src fs.Fs, sr return err } + +// destPath maps an archive entry name onto its destination remote within +// dstDir, returning an error if the name is unsafe. +// +// Archive entry names are attacker controlled. A leading "./" is stripped: +// tar archives created with relative paths (e.g. "tar -czf archive.tar.gz .") +// use "./" prefixed entries and, without stripping, rclone would encode the +// "." as a full-width dot character creating a spurious directory. +// +// Any entry with a ".." path component is then rejected to prevent a path +// traversal ("Zip Slip") attack: path.Join collapses "..", so an entry such +// as "../escaped.txt" joined onto "dir" would resolve to "escaped.txt" and be +// written outside the selected destination directory. Both "/" and "\" are +// treated as separators when looking for ".." segments: archive names should +// use "/", but a crafted archive may use "\", which the local backend treats +// as a path separator on Windows. +// +// The returned remote is empty for the archive root entry ("./"), which the +// caller should skip. +func destPath(nameInArchive, dstDir string) (string, error) { + remote := strings.TrimPrefix(nameInArchive, "./") + isSeparator := func(r rune) bool { return r == '/' || r == '\\' } + for _, segment := range strings.FieldsFunc(remote, isSeparator) { + if segment == ".." { + return "", fmt.Errorf("refusing to extract archive entry %q with a %q path component", nameInArchive, "..") + } + } + if remote == "" { + return "", nil + } + if dstDir != "" { + remote = path.Join(dstDir, remote) + } + return remote, nil +} diff --git a/cmd/archive/extract/extract_test.go b/cmd/archive/extract/extract_test.go index 867e3c358..64c8bbc75 100644 --- a/cmd/archive/extract/extract_test.go +++ b/cmd/archive/extract/extract_test.go @@ -3,17 +3,19 @@ package extract import ( - "strings" "testing" "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" ) -func TestStripDotSlashPrefix(t *testing.T) { +func TestDestPath(t *testing.T) { tests := []struct { name string input string + dstDir string expected string + wantErr bool }{ { name: "strip leading dot-slash from file", @@ -36,12 +38,13 @@ func TestStripDotSlashPrefix(t *testing.T) { expected: "dir/file.txt", }, { - name: "dot-dot-slash NOT stripped (path traversal safety)", - input: "../etc/passwd", - expected: "../etc/passwd", + name: "joined onto destination directory", + input: "file.txt", + dstDir: "safe/prefix", + expected: "safe/prefix/file.txt", }, { - name: "dot-slash directory entry becomes empty", + name: "archive root entry skipped", input: "./", expected: "", }, @@ -50,12 +53,49 @@ func TestStripDotSlashPrefix(t *testing.T) { input: "././file.txt", expected: "./file.txt", }, + { + name: "leading dot-dot rejected", + input: "../etc/passwd", + wantErr: true, + }, + { + name: "leading dot-dot rejected with destination", + input: "../escaped.txt", + dstDir: "safe/prefix", + wantErr: true, + }, + { + name: "interior dot-dot rejected", + input: "dir/../../escaped.txt", + dstDir: "safe/prefix", + wantErr: true, + }, + { + name: "trailing dot-dot rejected", + input: "dir/..", + wantErr: true, + }, + { + name: "backslash dot-dot rejected", + input: `..\escaped.txt`, + wantErr: true, + }, + { + name: "nested backslash dot-dot rejected", + input: `dir\..\..\escaped.txt`, + dstDir: "safe/prefix", + wantErr: true, + }, } for _, tc := range tests { t.Run(tc.name, func(t *testing.T) { - // This mirrors the stripping logic in ArchiveExtract - got := strings.TrimPrefix(tc.input, "./") + got, err := destPath(tc.input, tc.dstDir) + if tc.wantErr { + require.Error(t, err) + return + } + require.NoError(t, err) assert.Equal(t, tc.expected, got) }) }