vfs: fix serve nfs path traversal by refusing unsafe file names GHSA-44pm-2q5p-fm63 CVE-PENDING

Dir.Create, Dir.Mkdir and Dir.Rename joined whatever name they were
given onto the directory's path. A name of "..", "." or one containing
a "/" isn't a child of the directory, and at the VFS root
path.Join("", "..") preserves the ".." so the new node's path pointed
outside the root. Backends which join the Fs root with the remote then
resolved it to a location above the directory the VFS was created on.

serve nfs was exploitable through this. The go-nfs library refuses a
name of exactly "." or ".." in MKDIR only, doesn't check the name in
CREATE, RENAME or SYMLINK, and never refuses a name containing a "/".

serve webdav was exploitable in the same way and has its own fix. The
other serve front-ends rely on their own or their protocol library's
path cleaning to keep such names out of the VFS.

Reject such names with EINVAL in the VFS itself, so that every
front-end and mount is protected regardless of what the layer above it
checks. This is the fix for serve nfs and defence in depth for the
others. The serve webdav check is kept as the front-end is the trust
boundary and gives the client a clear error. The source name of a
rename is only used to look the node up so it isn't checked.

VFS.Mkdir of the root, which previously went through Dir.Mkdir with an
empty name and added a nameless child node to the root, now makes the
root directory on the backend directly, still honouring --read-only.
serve webdav and serve ftp made directories through Dir.Mkdir rather
than VFS.Mkdir, so they are switched to VFS.Mkdir to keep "MKCOL /" and
"MKD /" working.

cmd/serve/ftp/ftp.go

@@ -535,12 +535,7 @@ func (d *driver) MakeDir(sctx *ftp.Context, path string) (err error) {
 		return err
 	}
 	defer VFS.Shutdown()
-	dir, leaf, err := VFS.StatParent(path)
-	if err != nil {
-		return err
-	}
-	_, err = dir.Mkdir(leaf)
-	return err
+	return VFS.Mkdir(path, 0777)
 }
 
 // GetFile download a file

cmd/serve/ftp/ftp_test.go

@@ -130,6 +130,32 @@ func TestNewServerPerServerAuthProxy(t *testing.T) {
 	assert.Nil(t, d.provider.VFS(), "expected no fixed VFS when auth proxy is in use")
 }
 
+// TestMakeDirRoot checks that "MKD /" makes the directory being
+// served. The ftp backend never sends this so TestFTP doesn't cover it.
+func TestMakeDirRoot(t *testing.T) {
+	root := filepath.Join(t.TempDir(), "root")
+	f, err := fs.NewFs(context.Background(), root)
+	require.NoError(t, err)
+
+	opt := Opt
+	opt.ListenAddr = testHOST + ":" + testPORT
+	opt.PassivePorts = testPASSIVEPORTRANGE
+
+	d, err := newServer(context.Background(), f, &opt, &vfscommon.Opt, &proxy.Opt)
+	require.NoError(t, err)
+	defer d.provider.Shutdown()
+
+	require.NoError(t, d.MakeDir(nil, "/"))
+	fi, err := os.Stat(root)
+	require.NoError(t, err)
+	assert.True(t, fi.IsDir())
+
+	require.NoError(t, d.MakeDir(nil, "/dir"))
+	fi, err = os.Stat(filepath.Join(root, "dir"))
+	require.NoError(t, err)
+	assert.True(t, fi.IsDir())
+}
+
 func TestRc(t *testing.T) {
 	if israce.Enabled {
 		t.Skip("Skipping under race detector as underlying library is racy")

cmd/serve/nfs/filesystem_test.go

@@ -3,9 +3,15 @@
 package nfs
 
 import (
+	"io"
+	"os"
+	"strings"
 	"testing"
 
+	_ "github.com/rclone/rclone/backend/memory"
 	"github.com/rclone/rclone/fs"
+	"github.com/rclone/rclone/fs/operations"
+	"github.com/rclone/rclone/fstest"
 	"github.com/rclone/rclone/vfs"
 	"github.com/rclone/rclone/vfs/vfscommon"
 	"github.com/stretchr/testify/assert"
@@ -39,3 +45,56 @@ func TestChmodDanglingSymlink(t *testing.T) {
 	assert.ErrorIs(t, bfs.Chmod("missing", 0777), vfs.ENOENT)
 	assert.ErrorIs(t, bfs.Chown("missing", 1000, 1000), vfs.ENOENT)
 }
+
+// go-nfs joins the file name from the client onto the directory path
+// with Join without checking it for ".." elements or a "/", so such a
+// name must not reach outside the directory served.
+//
+// The memory backend is used because it resolves ".." by joining paths,
+// unlike the local backend which refuses paths outside its root.
+func TestPathTraversal(t *testing.T) {
+	ctx := t.Context()
+	modTime := fstest.Time("2001-02-03T04:05:06.499999999Z")
+
+	// outside is the parent of the directory served and holds an object
+	// which must stay unreachable through the server.
+	outside, err := fs.NewFs(ctx, ":memory:nfs-traversal-test")
+	require.NoError(t, err)
+	_, err = operations.Rcat(ctx, outside, "outside-secret.txt", io.NopCloser(strings.NewReader("SECRET")), modTime, nil)
+	require.NoError(t, err)
+
+	f, err := fs.NewFs(ctx, ":memory:nfs-traversal-test/served-root")
+	require.NoError(t, err)
+	_, err = operations.Rcat(ctx, f, "inside.txt", io.NopCloser(strings.NewReader("INSIDE")), modTime, nil)
+	require.NoError(t, err)
+
+	opt := vfscommon.Opt
+	opt.Links = true
+	opt.CacheMode = vfscommon.CacheModeWrites
+	v := vfs.New(ctx, f, &opt)
+	defer v.Shutdown()
+	bfs := &FS{vfs: v}
+
+	// The MkdirAll calls come first so that, should one succeed, the
+	// later calls are routed through the directory node it made.
+	assert.Error(t, bfs.MkdirAll(bfs.Join("", "../new-dir"), 0777))
+	assert.Error(t, bfs.MkdirAll(bfs.Join("", ".."), 0777))
+
+	_, err = bfs.Create(bfs.Join("", "../outside-write.txt"))
+	assert.Error(t, err)
+	_, err = bfs.OpenFile(bfs.Join("", "../outside-secret.txt"), os.O_WRONLY|os.O_TRUNC, 0666)
+	assert.Error(t, err)
+	_, err = bfs.Open(bfs.Join("", "../outside-secret.txt"))
+	assert.Error(t, err)
+	assert.Error(t, bfs.Symlink("target", bfs.Join("", "../outside-link")))
+	assert.Error(t, bfs.Rename("inside.txt", bfs.Join("", "../outside-move.txt")))
+	assert.Error(t, bfs.Rename(bfs.Join("", "../outside-secret.txt"), "stolen.txt"))
+	assert.Error(t, bfs.Remove(bfs.Join("", "../outside-secret.txt")))
+
+	// Nothing was created, modified, moved or removed, either outside
+	// the served root or inside it.
+	fstest.CheckListingWithPrecision(t, outside, []fstest.Item{
+		fstest.NewItem("outside-secret.txt", "SECRET", modTime),
+		fstest.NewItem("served-root/inside.txt", "INSIDE", modTime),
+	}, []string{"served-root"}, fs.GetModifyWindow(ctx, outside))
+}

cmd/serve/webdav/webdav.go

@@ -535,9 +535,9 @@ func (w *WebDAV) logRequest(r *http.Request, err error) {
 }
 
 // checkPath returns an error if the client supplied path name has a
-// "." or ".." element. The VFS joins the path onto the Fs root, so
-// such an element would otherwise address objects outside the
-// directory served.
+// "." or ".." element. Such an element can't name anything inside
+// the directory served, and the VFS refuses to make nodes with such
+// names, so the request is rejected up front with a clear error.
 //
 // The elements are checked individually rather than the path compared
 // with path.Clean because path.Clean can't resolve a leading ".." in
@@ -558,12 +558,7 @@ func (w *WebDAV) Mkdir(ctx context.Context, name string, perm os.FileMode) (err
 	if err != nil {
 		return err
 	}
-	dir, leaf, err := VFS.StatParent(name)
-	if err != nil {
-		return err
-	}
-	_, err = dir.Mkdir(leaf)
-	return err
+	return VFS.Mkdir(name, perm)
 }
 
 // OpenFile opens a file or a directory

vfs/dir.go

@@ -18,6 +18,7 @@ import (
 	"github.com/rclone/rclone/fs/object"
 	"github.com/rclone/rclone/fs/operations"
 	"github.com/rclone/rclone/fs/walk"
+	"github.com/rclone/rclone/lib/sanitize"
 	"github.com/rclone/rclone/vfs/vfscommon"
 	"golang.org/x/text/unicode/norm"
 )
@@ -1026,9 +1027,28 @@ func (d *Dir) Open(flags int) (fd Handle, err error) {
 	return newDirHandle(d), nil
 }
 
+// checkName returns EINVAL if name isn't a single safe path element,
+// that is it is empty, "." or "..", or contains a "/".
+//
+// Such a name can't be a child of the directory and would make a node
+// whose path is outside it (or outside the VFS root, since at the root
+// path.Join("", "..") preserves the "..").
+func (d *Dir) checkName(name string) error {
+	if err := sanitize.Leaf(name); err != nil {
+		fs.Errorf(d, "Refusing unsafe name: %v", err)
+		return EINVAL
+	}
+	return nil
+}
+
 // Create makes a new file node
+//
+// It returns EINVAL if name isn't a single safe path element.
 func (d *Dir) Create(name string, flags int) (*File, error) {
 	// fs.Debugf(path, "Dir.Create")
+	if err := d.checkName(name); err != nil {
+		return nil, err
+	}
 	// Return existing node if one exists
 	node, err := d.stat(name)
 	switch err {
@@ -1058,10 +1078,15 @@ func (d *Dir) Create(name string, flags int) (*File, error) {
 }
 
 // Mkdir creates a new directory
+//
+// It returns EINVAL if name isn't a single safe path element.
 func (d *Dir) Mkdir(name string) (*Dir, error) {
 	if d.vfs.Opt.ReadOnly {
 		return nil, EROFS
 	}
+	if err := d.checkName(name); err != nil {
+		return nil, err
+	}
 	path := path.Join(d.path, name)
 	node, err := d.stat(name)
 	switch err {
@@ -1170,11 +1195,16 @@ func (d *Dir) RemoveName(name string) error {
 }
 
 // Rename the file
+//
+// It returns EINVAL if newName isn't a single safe path element.
 func (d *Dir) Rename(oldName, newName string, destDir *Dir) error {
 	// fs.Debugf(d, "BEFORE\n%s", d.dump())
 	if d.vfs.Opt.ReadOnly {
 		return EROFS
 	}
+	if err := destDir.checkName(newName); err != nil {
+		return err
+	}
 	oldPath := path.Join(d.path, oldName)
 	newPath := path.Join(destDir.path, newName)
 	// fs.Debugf(oldPath, "Dir.Rename to %q", newPath)

vfs/dir_test.go

@@ -471,6 +471,48 @@ func TestDirMkdirSub(t *testing.T) {
 	assert.Equal(t, EROFS, err)
 }
 
+// TestDirUnsafeName checks that a name which isn't a single safe path
+// element is refused by everything which creates a node, so it can't
+// build a path outside the directory. At the root path.Join("", "..")
+// preserves the "..", so this would otherwise make a node whose path is
+// outside the VFS root.
+func TestDirUnsafeName(t *testing.T) {
+	r, vfs, dir, file1 := dirCreate(t)
+
+	root, err := vfs.Root()
+	require.NoError(t, err)
+
+	for _, d := range []*Dir{root, dir} {
+		for _, name := range []string{"", ".", "..", "sub/dir", "../escape", "escape/.."} {
+			t.Run(fmt.Sprintf("%q in %q", name, d.Path()), func(t *testing.T) {
+				_, err := d.Mkdir(name)
+				assert.Equal(t, EINVAL, err, "Mkdir")
+
+				_, err = d.Create(name, os.O_WRONLY|os.O_CREATE)
+				assert.Equal(t, EINVAL, err, "Create")
+
+				err = dir.Rename("file1", name, d)
+				assert.Equal(t, EINVAL, err, "Rename")
+
+				// The source name is only looked up so isn't found
+				err = d.Rename(name, "renamed", dir)
+				assert.Equal(t, ENOENT, err, "Rename source")
+			})
+		}
+	}
+
+	// such a name can't be looked up either
+	_, err = vfs.Stat("..")
+	assert.Equal(t, ENOENT, err)
+	_, err = vfs.Stat("../escape")
+	assert.Equal(t, ENOENT, err)
+
+	// check nothing was made in the vfs or the underlying r.Fremote
+	checkListing(t, root, []string{"dir,0,true"})
+	checkListing(t, dir, []string{"file1,14,false"})
+	r.CheckRemoteItems(t, file1)
+}
+
 func TestDirRemove(t *testing.T) {
 	r, vfs, dir, _ := dirCreate(t)
 

vfs/vfs.go

@@ -820,6 +820,19 @@ func (vfs *VFS) Chown(name string, uid, gid int) error {
 // mkdir creates a new directory with the specified name and permission bits
 // (before umask) returning the new directory node.
 func (vfs *VFS) mkdir(name string, perm os.FileMode) (*Dir, error) {
+	// The root directory node always exists, but the directory it
+	// represents may not, so make that on the backend.
+	if strings.Trim(name, "/") == "" {
+		if vfs.Opt.ReadOnly {
+			return nil, EROFS
+		}
+		err := vfs.f.Mkdir(vfs.ctx, "")
+		if err != nil {
+			fs.Errorf(vfs.root, "Dir.Mkdir failed to create directory: %v", err)
+			return nil, err
+		}
+		return vfs.root, nil
+	}
 	dir, leaf, err := vfs.StatParent(name)
 	if err != nil {
 		return nil, err

vfs/vfs_test.go

@@ -478,6 +478,12 @@ func TestVFSMkdir(t *testing.T) {
 	require.NoError(t, err)
 
 	r.CheckRemoteListing(t, nil, []string{"a", "b", "c", "d"})
+
+	vfs.Opt.ReadOnly = true
+	err = vfs.Mkdir("", 0777)
+	assert.Equal(t, EROFS, err)
+	err = vfs.Mkdir("e", 0777)
+	assert.Equal(t, EROFS, err)
 }
 
 func TestVFSMkdirAll(t *testing.T) {