diff --git a/cmd/serve/docker/docker_test.go b/cmd/serve/docker/docker_test.go index f57382711..c695ae21e 100644 --- a/cmd/serve/docker/docker_test.go +++ b/cmd/serve/docker/docker_test.go @@ -204,6 +204,49 @@ func TestDockerPluginLogic(t *testing.T) { assert.NoError(t, err) } +// TestDockerPluginVolumeNameConfined checks that a crafted volume name +// containing ".." components cannot escape the base directory to create a +// mountpoint at an arbitrary host path. +func TestDockerPluginVolumeNameConfined(t *testing.T) { + ctx := context.Background() + oldCacheDir := config.GetCacheDir() + testDir, testFs := initialise(ctx, t) + err := config.SetCacheDir(testDir) + require.NoError(t, err) + defer func() { + _ = config.SetCacheDir(oldCacheDir) + if !t.Failed() { + fstest.Purge(testFs) + _ = os.RemoveAll(testDir) + } + }() + + drv, err := docker.NewDriver(ctx, testDir, nil, nil, true, true) + require.NoError(t, err) + require.NotNil(t, drv) + + // A volume name with ".." components resolves outside testDir, to a + // sibling path that does not exist so the test never touches a real + // system directory. + escape := filepath.Join("..", "docker-escape-target") + outside := filepath.Join(filepath.Dir(testDir), "docker-escape-target") + require.NoDirExists(t, outside) + volReq := &docker.CreateRequest{ + Name: escape, + Options: docker.VolOpts{"remote": testDir}, + } + err = drv.Create(volReq) + assertErrorContains(t, err, "resolves outside the base directory") + + // The escaping directory must not have been created. + _, statErr := os.Stat(outside) + assert.True(t, os.IsNotExist(statErr), "escaping mountpoint %q should not exist", outside) + + // The volume must not have been registered. + _, err = drv.Get(&docker.GetRequest{Name: escape}) + assert.Error(t, err) +} + const ( httpTimeout = 2 * time.Second tempDelay = 10 * time.Millisecond diff --git a/cmd/serve/docker/volume.go b/cmd/serve/docker/volume.go index 4f69cb627..813ab6320 100644 --- a/cmd/serve/docker/volume.go +++ b/cmd/serve/docker/volume.go @@ -8,6 +8,7 @@ import ( "path/filepath" "runtime" "sort" + "strings" "time" "github.com/rclone/rclone/cmd/mountlib" @@ -54,8 +55,24 @@ type VolInfo struct { Status map[string]any `json:",omitempty"` } +// volumeMountPath returns the mountpoint for a volume called name below +// root, together with whether that path stays confined within root. +// +// filepath.Join cleans its result, collapsing any ".." components in the +// volume name, so a crafted name could otherwise resolve to an arbitrary +// host path outside root. Callers must reject names for which confined is +// false rather than creating a directory and mounting there. +func volumeMountPath(root, name string) (path string, confined bool) { + path = filepath.Join(root, name) + root = filepath.Clean(root) + return path, path == root || strings.HasPrefix(path, root+string(filepath.Separator)) +} + func newVolume(ctx context.Context, name string, volOpt VolOpts, drv *Driver) (*Volume, error) { - path := filepath.Join(drv.root, name) + path, confined := volumeMountPath(drv.root, name) + if !confined { + return nil, fmt.Errorf("invalid volume name %q: resolves outside the base directory", name) + } mnt := &mountlib.MountPoint{ MountPoint: path, }