From 2b7d0b16ed14663bbc39165514cd1d8665c49a4b Mon Sep 17 00:00:00 2001 From: Nick Craig-Wood Date: Tue, 25 Aug 2026 10:13:39 +0100 Subject: [PATCH] lib/sanitize: factor untrusted path sanitization out of archive extract Move the archive entry name validation added for CVE-2026-59732 from cmd/archive/extract into a new lib/sanitize package as sanitize.Path, so the same check can be shared with the archive backend which mounts archives as a filesystem. sanitize.Path keeps the extract semantics - reject any name with a ".." path component, treating both "/" and "\" as separators - and additionally cleans the name with path.Clean. This corrects two edge cases in extract: a repeated "./" prefix ("././file.txt") is now fully stripped rather than only the first, and a bare "." entry is now treated as the archive root and skipped. Add sanitize.Leaf, which rejects a name that is empty, ".", ".." or contains a "/", for checking a single directory entry name read from an archive. The names handled are rclone remote paths, in which "/" is the only separator and "\" an ordinary character, so Leaf does not reject a backslash: making a name safe for its storage is the destination backend's job (the local backend encodes "\" on Windows and refuses paths which escape its root). Path's rejection of ".." between backslashes is kept as defence in depth for extract. --- cmd/archive/extract/extract.go | 28 ++--- cmd/archive/extract/extract_test.go | 41 +------ lib/sanitize/sanitize.go | 59 ++++++++++ lib/sanitize/sanitize_test.go | 169 ++++++++++++++++++++++++++++ 4 files changed, 241 insertions(+), 56 deletions(-) create mode 100644 lib/sanitize/sanitize.go create mode 100644 lib/sanitize/sanitize_test.go 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) + }) + } +}