From 935197b06220e83a2ef71d60f98cc2991b1f5de1 Mon Sep 17 00:00:00 2001 From: Nick Craig-Wood Date: Wed, 19 Aug 2026 16:37:59 +0100 Subject: [PATCH] fs: confine directory listing entries that escape the root GHSA-3vxh-3pcx-9m8q GHSA-38xv-hf3p-h7mq CVE-PENDING The rclone core does not sanitise ".." in an object's Remote(). Such a name can arrive from a malicious or buggy backend - an object store permits keys containing ".." or a leading "/" - and, if acted on, lets a listing or transfer escape the configured root. A source object named "../../other/x" is copied to "other/x" outside the destination root, and a crafted listing name surfaces outside the directory being listed. Add list.RemoteEscapesRoot, which reports whether a Remote climbs above the root when joined onto it, and list.RemoveEscaping, which drops and logs such entries. Apply RemoveEscaping unconditionally - independent of the include/exclude filters - at the three per-entry filtering points every listing passes through: filterDir, walk.listR and walk.walkRDirTree (recursive ListR). operations.StatJSON calls List and NewObject directly, bypassing those, so it rejects an escaping remote up front. This confines every backend at once, so no per-backend change is needed. --- fs/list/list.go | 35 +++++++++++++++++++++++++ fs/list/list_test.go | 51 ++++++++++++++++++++++++++++++++++++ fs/operations/lsjson.go | 9 +++++++ fs/operations/lsjson_test.go | 33 +++++++++++++++++++++++ fs/walk/walk.go | 2 ++ fs/walk/walk_test.go | 33 +++++++++++++++++++++++ 6 files changed, 163 insertions(+) diff --git a/fs/list/list.go b/fs/list/list.go index c534b3363..992beef8c 100644 --- a/fs/list/list.go +++ b/fs/list/list.go @@ -4,6 +4,7 @@ package list import ( "context" "fmt" + "path" "sort" "strings" @@ -13,6 +14,39 @@ import ( "github.com/rclone/rclone/lib/bucket" ) +// RemoteEscapesRoot reports whether remote, taken as a path relative to an Fs +// root, climbs above that root when joined onto it. +// +// It mirrors what the backends actually do - path.Join(root, remote) - by +// joining remote onto a sentinel root and checking whether the result is still +// under the sentinel. +// +// A well behaved backend never produces such a name; one can arise from a +// malicious or buggy server (an object store permits keys containing ".."), +// and acting on it would let a listing or transfer escape the configured root. +func RemoteEscapesRoot(remote string) bool { + const sentinel = "\x00rootsentinel" + joined := path.Join(sentinel, remote) + return joined != sentinel && !strings.HasPrefix(joined, sentinel+"/") +} + +// RemoveEscaping drops - and logs - any entries whose Remote escapes the Fs +// root (see RemoteEscapesRoot), filtering the slice in place. It is applied to +// every listing unconditionally, independent of the include/exclude filters, so +// that names which cannot be safely confined are never surfaced to any +// operation. +func RemoveEscaping(entries fs.DirEntries) fs.DirEntries { + kept := entries[:0] + for _, entry := range entries { + if RemoteEscapesRoot(entry.Remote()) { + fs.Errorf(entry, "Entry %q escapes the root - ignoring", entry.Remote()) + continue + } + kept = append(kept, entry) + } + return kept +} + // DirSorted reads Object and *Dir into entries for the given Fs. // // dir is the start directory, "" for root @@ -99,6 +133,7 @@ func DirSortedFn(ctx context.Context, f fs.Fs, includeAll bool, dir string, call func filterDir(ctx context.Context, entries fs.DirEntries, includeAll bool, dir string, IncludeObject func(ctx context.Context, o fs.Object) bool, IncludeDirectory func(remote string) (bool, error)) (newEntries fs.DirEntries, err error) { + entries = RemoveEscaping(entries) newEntries = entries[:0] // in place filter prefix := "" if dir != "" { diff --git a/fs/list/list_test.go b/fs/list/list_test.go index 94fa7a6cc..e6a937a46 100644 --- a/fs/list/list_test.go +++ b/fs/list/list_test.go @@ -15,6 +15,57 @@ import ( // NB integration tests for DirSorted are in // fs/operations/listdirsorted_test.go +func TestRemoteEscapesRoot(t *testing.T) { + for _, test := range []struct { + in string + escape bool + }{ + // non-escaping + {"", false}, + {".", false}, + {"a", false}, + {"a/b", false}, + {"a/../b", false}, // cleans to "b" - does not climb + {"foo/..", false}, // cleans back to root + {"..foo", false}, // not a ".." segment + {"a/..bar/c", false}, + {"...", false}, // three dots is an ordinary name + {"/", false}, // absolute, but does not climb + {"/a/b", false}, + {"//a", false}, + // climbing with a relative prefix + {"..", true}, + {"../b", true}, + {"a/../../b", true}, // cleans to "../b" + {"../../etc/passwd", true}, + // climbing hidden behind a leading slash - path.Clean would anchor + // these as absolute and miss them, but path.Join(root, ...) escapes + {"/..", true}, + {"/../x", true}, + {"//../../x", true}, + {"/../../etc/passwd", true}, + {"/./../x", true}, + } { + assert.Equal(t, test.escape, RemoteEscapesRoot(test.in), test.in) + } +} + +func TestFilterAndSortConfinement(t *testing.T) { + ok := mockobject.Object("ok.txt") + dotdot := mockobject.Object("..") // bare ".." - missed by the belongs-in-dir check + up := mockobject.Object("../escape.txt") // climbs one level + upDir := mockdir.New("../evildir") // climbing directory + deepUp := mockobject.Object("a/../../x") // cleans to "../x" + entries := fs.DirEntries{ok, dotdot, up, upDir, deepUp} + includeObject := func(ctx context.Context, o fs.Object) bool { return true } + includeDirectory := func(remote string) (bool, error) { return true, nil } + + // Even with includeAll, entries that escape the root are dropped. + newEntries, err := filterAndSortDir(context.Background(), entries, true, "", includeObject, includeDirectory) + require.NoError(t, err) + assert.Equal(t, fs.DirEntries{ok}, newEntries) +} + func TestFilterAndSortIncludeAll(t *testing.T) { da := mockdir.New("a") oA := mockobject.Object("A") diff --git a/fs/operations/lsjson.go b/fs/operations/lsjson.go index 0b2bd5a16..ac9811ee1 100644 --- a/fs/operations/lsjson.go +++ b/fs/operations/lsjson.go @@ -12,6 +12,7 @@ import ( "github.com/rclone/rclone/fs" "github.com/rclone/rclone/fs/accounting" "github.com/rclone/rclone/fs/hash" + "github.com/rclone/rclone/fs/list" "github.com/rclone/rclone/fs/walk" ) @@ -277,6 +278,14 @@ func StatJSON(ctx context.Context, fsrc fs.Fs, remote string, opt *ListJSONOpt) return nil, err } + // A remote that climbs above the Fs root can never be a valid item. + // StatJSON calls List/NewObject directly, bypassing the confinement in + // fs/list and fs/walk, so treat it as not found here rather than stat + // something outside the configured root. + if list.RemoteEscapesRoot(remote) { + return nil, nil + } + // Root is always a directory. When we have a NewDirEntry // primitive we need to call it, but for now this will do. if remote == "" { diff --git a/fs/operations/lsjson_test.go b/fs/operations/lsjson_test.go index e32aeefd4..0926320f1 100644 --- a/fs/operations/lsjson_test.go +++ b/fs/operations/lsjson_test.go @@ -12,6 +12,8 @@ import ( "github.com/rclone/rclone/fs/accounting" "github.com/rclone/rclone/fs/operations" "github.com/rclone/rclone/fstest" + "github.com/rclone/rclone/fstest/mockfs" + "github.com/rclone/rclone/fstest/mockobject" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" ) @@ -462,3 +464,34 @@ func TestStatJSONMemory(t *testing.T) { assert.Nil(t, got) }) } + +// TestStatJSONConfinement checks that StatJSON never returns an item whose +// remote escapes the Fs root. StatJSON calls List/NewObject directly and so +// bypasses the confinement in fs/list and fs/walk - the escaping objects added +// below would be returned without the guard in StatJSON. +func TestStatJSONConfinement(t *testing.T) { + ctx := context.Background() + ff, err := mockfs.NewFs(ctx, "mock", "/", nil) + require.NoError(t, err) + f := ff.(*mockfs.Fs) + f.AddObject(mockobject.Object("ok")) + f.AddObject(mockobject.Object("..")) + f.AddObject(mockobject.Object("../evil")) + f.AddObject(mockobject.Object("/../slashevil")) + f.AddObject(mockobject.Object("//../../slashevil2")) + f.AddObject(mockobject.Object("/../../etc/passwd")) + + // Escaping remotes are treated as not found, including the leading-slash + // variants that path.Clean would anchor as absolute and miss. + for _, remote := range []string{"..", "../evil", "/../slashevil", "//../../slashevil2", "/../../etc/passwd"} { + got, err := operations.StatJSON(ctx, f, remote, &operations.ListJSONOpt{}) + require.NoError(t, err, remote) + assert.Nil(t, got, remote) + } + + // A legitimate remote is still returned. + got, err := operations.StatJSON(ctx, f, "ok", &operations.ListJSONOpt{}) + require.NoError(t, err) + require.NotNil(t, got) + assert.Equal(t, "ok", got.Path) +} diff --git a/fs/walk/walk.go b/fs/walk/walk.go index e0916654c..4cbef9079 100644 --- a/fs/walk/walk.go +++ b/fs/walk/walk.go @@ -305,6 +305,7 @@ func listR(ctx context.Context, f fs.Fs, path string, includeAll bool, listType } } listType.Filter(&entries) + entries = list.RemoveEscaping(entries) if !includeAll { filteredEntries := entries[:0] for _, entry := range entries { @@ -473,6 +474,7 @@ func walkRDirTree(ctx context.Context, f fs.Fs, startPath string, includeAll boo var mu sync.Mutex err := listR(ctx, startPath, func(entries fs.DirEntries) error { accounting.Stats(ctx).Listed(int64(len(entries))) + entries = list.RemoveEscaping(entries) mu.Lock() defer mu.Unlock() for _, entry := range entries { diff --git a/fs/walk/walk_test.go b/fs/walk/walk_test.go index 7ba0e59da..1906d16d3 100644 --- a/fs/walk/walk_test.go +++ b/fs/walk/walk_test.go @@ -788,6 +788,39 @@ func TestListType(t *testing.T) { assert.Equal(t, dirEntries, got) } +func TestListRConfinement(t *testing.T) { + ctx := context.Background() + f, err := mockfs.NewFs(ctx, "mock", "/", nil) + require.NoError(t, err) + objects := fs.DirEntries{ + mockobject.Object("ok"), + mockobject.Object(".."), + mockobject.Object("../escape"), + mockdir.New("../evildir"), + // leading-slash climbers: filterDir doesn't guard the ListR path, so + // these reach RemoveEscaping directly and must still be dropped. + mockobject.Object("/../slashescape"), + mockobject.Object("//../../slashescape2"), + mockobject.Object("dir/deep"), + } + var got []string + callback := func(entries fs.DirEntries) error { + for _, entry := range entries { + got = append(got, entry.Remote()) + } + return nil + } + doListR := func(ctx context.Context, dir string, callback fs.ListRCallback) error { + return callback(objects) + } + // includeAll = true exercises the unconditional confinement - the + // include/exclude filter block is skipped in this mode, so an escaping + // entry would otherwise pass straight through. + err = listR(ctx, f, "", true, ListAll, callback, doListR, false) + require.NoError(t, err) + require.Equal(t, []string{"ok", "dir/deep"}, got) +} + func TestListR(t *testing.T) { ctx := context.Background() objects := fs.DirEntries{