diff --git a/cmd/archive/extract/extract.go b/cmd/archive/extract/extract.go index c518788cf..d49a689df 100644 --- a/cmd/archive/extract/extract.go +++ b/cmd/archive/extract/extract.go @@ -8,7 +8,6 @@ import ( "errors" "fmt" "path" - "slices" "strings" "github.com/mholt/archives" @@ -18,6 +17,7 @@ import ( "github.com/rclone/rclone/fs/accounting" "github.com/rclone/rclone/fs/filter" "github.com/rclone/rclone/fs/operations" + "github.com/rclone/rclone/lib/sanitize" "github.com/spf13/cobra" ) @@ -202,26 +202,20 @@ func ArchiveExtract(ctx context.Context, dst fs.Fs, dstDir string, src fs.Fs, sr // 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. +// Archive entry names are attacker controlled so they are sanitized with +// sanitize.Path before being joined onto dstDir. This cleans the name +// (stripping the "./" prefix which tar archives created with relative +// paths, e.g. "tar -czf archive.tar.gz .", put on their entries) and +// rejects any name with a ".." path component which would otherwise +// escape the destination directory (a path traversal, or "Zip Slip", +// attack). // // 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 == '\\' } - if slices.Contains(strings.FieldsFunc(remote, isSeparator), "..") { - return "", fmt.Errorf("refusing to extract archive entry %q with a %q path component", nameInArchive, "..") + remote, err := sanitize.Path(nameInArchive) + if err != nil { + return "", fmt.Errorf("refusing to extract archive entry: %w", err) } if remote == "" { return "", nil diff --git a/cmd/archive/extract/extract_test.go b/cmd/archive/extract/extract_test.go index 64c8bbc75..d2c402ddf 100644 --- a/cmd/archive/extract/extract_test.go +++ b/cmd/archive/extract/extract_test.go @@ -18,25 +18,10 @@ func TestDestPath(t *testing.T) { wantErr bool }{ { - name: "strip leading dot-slash from file", - input: "./file.txt", - expected: "file.txt", - }, - { - name: "strip leading dot-slash from nested path", + name: "sanitized name returned", input: "./subdir/file.txt", expected: "subdir/file.txt", }, - { - name: "no prefix unchanged", - input: "file.txt", - expected: "file.txt", - }, - { - name: "nested path unchanged", - input: "dir/file.txt", - expected: "dir/file.txt", - }, { name: "joined onto destination directory", input: "file.txt", @@ -48,44 +33,22 @@ func TestDestPath(t *testing.T) { input: "./", expected: "", }, - { - name: "only single leading dot-slash stripped", - 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", + name: "interior dot-dot rejected with destination", 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 { diff --git a/lib/sanitize/sanitize.go b/lib/sanitize/sanitize.go new file mode 100644 index 000000000..a03ba6788 --- /dev/null +++ b/lib/sanitize/sanitize.go @@ -0,0 +1,59 @@ +// Package sanitize cleans untrusted input before use. +// +// The names handled here are rclone remote paths, which use "/" as +// the only path separator. A "\" is an ordinary character in a remote +// path and is kept as such - it is up to each backend to make names +// safe for its own storage (the local backend, for example, encodes +// it on Windows and rejects any path which escapes its root). As +// defence in depth Path does however refuse names in which "\" would +// form a ".." component if it were a separator. +package sanitize + +import ( + "fmt" + "path" + "slices" + "strings" +) + +// Leaf checks that the untrusted name is safe to use as a single path +// component (a directory entry leaf), such as a name read from an +// archive's directory listing. +// +// It returns an error if the name is empty, ".", "..", or contains a +// "/". Such a name would otherwise fabricate hierarchy or escape its +// directory when joined onto a path. +func Leaf(name string) error { + switch name { + case "", ".", "..": + return fmt.Errorf("unsafe path component %q", name) + } + if strings.Contains(name, "/") { + return fmt.Errorf("path component %q contains a \"/\"", name) + } + return nil +} + +// Path sanitizes the untrusted "/"-separated path name, such as an +// archive entry name, so that it is safe to use as an rclone remote +// path relative to some root. +// +// It returns the name cleaned with path.Clean and with any leading +// and trailing "/" removed, or "" if the name refers to the root +// directory (e.g. "", "/" or "./"). +// +// It returns an error for any name with a ".." path component, which +// would otherwise escape the root when joined onto it (a path +// traversal, or "Zip Slip", attack). Both "/" and "\" are treated as +// separators when looking for ".." components. +func Path(name string) (string, error) { + isSeparator := func(r rune) bool { return r == '/' || r == '\\' } + if slices.Contains(strings.FieldsFunc(name, isSeparator), "..") { + return "", fmt.Errorf("path %q has a %q component", name, "..") + } + cleaned := strings.Trim(path.Clean(name), "/") + if cleaned == "." { + return "", nil + } + return cleaned, nil +} diff --git a/lib/sanitize/sanitize_test.go b/lib/sanitize/sanitize_test.go new file mode 100644 index 000000000..69f849be4 --- /dev/null +++ b/lib/sanitize/sanitize_test.go @@ -0,0 +1,169 @@ +package sanitize + +import ( + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +func TestPath(t *testing.T) { + tests := []struct { + name string + input string + expected string + wantErr bool + }{ + { + name: "plain file unchanged", + input: "file.txt", + expected: "file.txt", + }, + { + name: "nested path unchanged", + input: "dir/file.txt", + expected: "dir/file.txt", + }, + { + name: "strip leading dot-slash from file", + input: "./file.txt", + expected: "file.txt", + }, + { + name: "strip leading dot-slash from nested path", + input: "./subdir/file.txt", + expected: "subdir/file.txt", + }, + { + name: "strip repeated leading dot-slash", + input: "././file.txt", + expected: "file.txt", + }, + { + name: "strip interior dot component", + input: "dir/./file.txt", + expected: "dir/file.txt", + }, + { + name: "strip leading slash", + input: "/dir/file.txt", + expected: "dir/file.txt", + }, + { + name: "strip trailing slash from directory", + input: "dir/", + expected: "dir", + }, + { + name: "collapse doubled slashes", + input: "dir//file.txt", + expected: "dir/file.txt", + }, + { + name: "empty name is the root", + input: "", + expected: "", + }, + { + name: "dot-slash is the root", + input: "./", + expected: "", + }, + { + name: "dot is the root", + input: ".", + expected: "", + }, + { + name: "slash is the root", + input: "/", + expected: "", + }, + { + name: "three dots allowed", + input: "dir/...", + expected: "dir/...", + }, + { + name: "backslash kept in file name", + input: `dir/back\slash.txt`, + expected: `dir/back\slash.txt`, + }, + { + name: "leading dot-dot rejected", + input: "../etc/passwd", + wantErr: true, + }, + { + name: "interior dot-dot rejected", + input: "dir/../../escaped.txt", + wantErr: true, + }, + { + name: "trailing dot-dot rejected", + input: "dir/..", + wantErr: true, + }, + { + name: "bare dot-dot rejected", + input: "..", + wantErr: true, + }, + { + name: "backslash dot-dot rejected", + input: `..\escaped.txt`, + wantErr: true, + }, + { + name: "nested backslash dot-dot rejected", + input: `dir\..\..\escaped.txt`, + wantErr: true, + }, + { + name: "mixed separator dot-dot rejected", + input: `dir/..\escaped.txt`, + wantErr: true, + }, + } + + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + got, err := Path(tc.input) + if tc.wantErr { + require.Error(t, err) + return + } + require.NoError(t, err) + assert.Equal(t, tc.expected, got) + }) + } +} + +func TestLeaf(t *testing.T) { + tests := []struct { + name string + input string + wantErr bool + }{ + {name: "plain name", input: "file.txt"}, + {name: "name with dots", input: "..."}, + {name: "hidden name", input: ".hidden"}, + {name: "empty rejected", input: "", wantErr: true}, + {name: "dot rejected", input: ".", wantErr: true}, + {name: "dot-dot rejected", input: "..", wantErr: true}, + {name: "slash rejected", input: "a/b", wantErr: true}, + {name: "backslash allowed", input: `a\b`}, + {name: "leading slash rejected", input: "/etc", wantErr: true}, + } + + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + err := Leaf(tc.input) + if tc.wantErr { + require.Error(t, err) + return + } + require.NoError(t, err) + }) + } +}