From ffd1e9a0f5e5d6ca1e919b86347490a17e211e9a Mon Sep 17 00:00:00 2001 From: Mahmood Ali Date: Tue, 11 Dec 2018 11:02:45 -0500 Subject: [PATCH 1/2] drivers/docker: enforce volumes.enabled When volumes.enable flag is off in Docker driver, disable all mounts of paths outside alloc dir. --- drivers/docker/driver.go | 38 ++++----- drivers/docker/driver_test.go | 147 ++++++++++++++++++++++++++++++++-- drivers/docker/utils.go | 18 +++++ drivers/docker/utils_test.go | 35 ++++++++ 4 files changed, 206 insertions(+), 32 deletions(-) diff --git a/drivers/docker/driver.go b/drivers/docker/driver.go index 4b780b32b74f..bb94d34351b1 100644 --- a/drivers/docker/driver.go +++ b/drivers/docker/driver.go @@ -541,7 +541,9 @@ func (d *Driver) containerBinds(task *drivers.TaskConfig, driverConfig *TaskConf secretDirBind := fmt.Sprintf("%s:%s", task.TaskDir().SecretsDir, task.Env[taskenv.SecretsDir]) binds := []string{allocDirBind, taskLocalBind, secretDirBind} - if !d.config.Volumes.Enabled && driverConfig.VolumeDriver != "" { + localBindVolume := driverConfig.VolumeDriver == "" || driverConfig.VolumeDriver == "local" + + if !d.config.Volumes.Enabled && !localBindVolume { return nil, fmt.Errorf("volumes are not enabled; cannot use volume driver %q", driverConfig.VolumeDriver) } @@ -551,25 +553,15 @@ func (d *Driver) containerBinds(task *drivers.TaskConfig, driverConfig *TaskConf return nil, fmt.Errorf("invalid docker volume: %q", userbind) } - // Resolve dotted path segments - parts[0] = filepath.Clean(parts[0]) + // Paths inside task dir are always allowed, Relative paths are always allowed as they mount within a container + // When a VolumeDriver is set, we assume we receive a binding in the format volume-name:container-dest + // Otherwise, we assume we receive a relative path binding in the format relative/to/task:/also/in/container + if localBindVolume { + parts[0] = expandPath(task.TaskDir().Dir, parts[0]) - // Absolute paths aren't always supported - if filepath.IsAbs(parts[0]) { - if !d.config.Volumes.Enabled { - // Disallow mounting arbitrary absolute paths + if !d.config.Volumes.Enabled && !isParentPath(task.AllocDir, parts[0]) { return nil, fmt.Errorf("volumes are not enabled; cannot mount host paths: %+q", userbind) } - binds = append(binds, userbind) - continue - } - - // Relative paths are always allowed as they mount within a container - // When a VolumeDriver is set, we assume we receive a binding in the format volume-name:container-dest - // Otherwise, we assume we receive a relative path binding in the format relative/to/task:/also/in/container - if driverConfig.VolumeDriver == "" { - // Expand path relative to alloc dir - parts[0] = filepath.Join(task.TaskDir().Dir, parts[0]) } binds = append(binds, strings.Join(parts, ":")) @@ -742,13 +734,11 @@ func (d *Driver) createContainerConfig(task *drivers.TaskConfig, driverConfig *T } if hm.Type == "bind" { - if filepath.IsAbs(filepath.Clean(hm.Source)) { - if !d.config.Volumes.Enabled { - return c, fmt.Errorf("volumes are not enabled; cannot mount host path: %q", hm.Source) - } - } else { - // Relative paths are always allowed as they mount within a container, and treated as relative to task dir - hm.Source = filepath.Join(task.TaskDir().Dir, hm.Source) + hm.Source = expandPath(task.TaskDir().Dir, hm.Source) + + // paths inside alloc dir are always allowed as they mount within a container, and treated as relative to task dir + if !d.config.Volumes.Enabled && !isParentPath(task.AllocDir, hm.Source) { + return c, fmt.Errorf("volumes are not enabled; cannot mount host path: %q %q", hm.Source, task.AllocDir) } } diff --git a/drivers/docker/driver_test.go b/drivers/docker/driver_test.go index 1a03c1b98a60..58f8f2102e10 100644 --- a/drivers/docker/driver_test.go +++ b/drivers/docker/driver_test.go @@ -1149,7 +1149,6 @@ func TestDockerDriver_CreateContainerConfig(t *testing.T) { require.Equal(t, "org/repo:0.1", c.Config.Image) require.EqualValues(t, opt, c.HostConfig.StorageOpt) } - func TestDockerDriver_Capabilities(t *testing.T) { if !tu.IsTravis() { t.Parallel() @@ -1638,6 +1637,143 @@ func TestDockerDriver_VolumesDisabled(t *testing.T) { } +func TestDockerDriver_BindMountsHonorVolumesEnabledFlag(t *testing.T) { + t.Parallel() + + allocDir := "/tmp/nomad/alloc-dir" + + cases := []struct { + name string + requiresVolumes bool + + volumeDriver string + volumes []string + + expectedVolumes []string + }{ + { + name: "basic plugin", + requiresVolumes: true, + volumeDriver: "nfs", + volumes: []string{"test-path:/tmp/taskpath"}, + expectedVolumes: []string{"test-path:/tmp/taskpath"}, + }, + { + name: "absolute default driver", + requiresVolumes: true, + volumeDriver: "", + volumes: []string{"/abs/test-path:/tmp/taskpath"}, + expectedVolumes: []string{"/abs/test-path:/tmp/taskpath"}, + }, + { + name: "absolute local driver", + requiresVolumes: true, + volumeDriver: "local", + volumes: []string{"/abs/test-path:/tmp/taskpath"}, + expectedVolumes: []string{"/abs/test-path:/tmp/taskpath"}, + }, + { + name: "relative default driver", + requiresVolumes: false, + volumeDriver: "", + volumes: []string{"test-path:/tmp/taskpath"}, + expectedVolumes: []string{"/tmp/nomad/alloc-dir/demo/test-path:/tmp/taskpath"}, + }, + { + name: "relative local driver", + requiresVolumes: false, + volumeDriver: "local", + volumes: []string{"test-path:/tmp/taskpath"}, + expectedVolumes: []string{"/tmp/nomad/alloc-dir/demo/test-path:/tmp/taskpath"}, + }, + { + name: "relative outside task-dir default driver", + requiresVolumes: false, + volumeDriver: "", + volumes: []string{"../test-path:/tmp/taskpath"}, + expectedVolumes: []string{"/tmp/nomad/alloc-dir/test-path:/tmp/taskpath"}, + }, + { + name: "relative outside task-dir local driver", + requiresVolumes: false, + volumeDriver: "local", + volumes: []string{"../test-path:/tmp/taskpath"}, + expectedVolumes: []string{"/tmp/nomad/alloc-dir/test-path:/tmp/taskpath"}, + }, + { + name: "relative outside alloc-dir default driver", + requiresVolumes: true, + volumeDriver: "", + volumes: []string{"../../test-path:/tmp/taskpath"}, + expectedVolumes: []string{"/tmp/nomad/test-path:/tmp/taskpath"}, + }, + { + name: "relative outside task-dir local driver", + requiresVolumes: true, + volumeDriver: "local", + volumes: []string{"../../test-path:/tmp/taskpath"}, + expectedVolumes: []string{"/tmp/nomad/test-path:/tmp/taskpath"}, + }, + } + + t.Run("with volumes enabled", func(t *testing.T) { + dh := dockerDriverHarness(t, nil) + driver := dh.Impl().(*Driver) + driver.config.Volumes.Enabled = true + + for _, c := range cases { + t.Run(c.name, func(t *testing.T) { + task, cfg, _ := dockerTask(t) + cfg.VolumeDriver = c.volumeDriver + cfg.Volumes = c.volumes + + task.AllocDir = allocDir + task.Name = "demo" + + require.NoError(t, task.EncodeConcreteDriverConfig(cfg)) + + cc, err := driver.createContainerConfig(task, cfg, "org/repo:0.1") + require.NoError(t, err) + + for _, v := range c.expectedVolumes { + require.Contains(t, cc.HostConfig.Binds, v) + } + }) + } + }) + + t.Run("with volumes disabled", func(t *testing.T) { + dh := dockerDriverHarness(t, nil) + driver := dh.Impl().(*Driver) + driver.config.Volumes.Enabled = false + + for _, c := range cases { + t.Run(c.name, func(t *testing.T) { + task, cfg, _ := dockerTask(t) + cfg.VolumeDriver = c.volumeDriver + cfg.Volumes = c.volumes + + task.AllocDir = allocDir + task.Name = "demo" + + require.NoError(t, task.EncodeConcreteDriverConfig(cfg)) + + cc, err := driver.createContainerConfig(task, cfg, "org/repo:0.1") + if c.requiresVolumes { + require.Error(t, err, "volumes are not enabled") + } else { + require.NoError(t, err) + + for _, v := range c.expectedVolumes { + require.Contains(t, cc.HostConfig.Binds, v) + } + } + }) + } + }) + +} + func TestDockerDriver_VolumesEnabled(t *testing.T) { if !tu.IsTravis() { t.Parallel() @@ -1810,13 +1946,8 @@ func TestDockerDriver_MountsSerialization(t *testing.T) { }, }, { - - // FIXME: This needs to be true but we have a bug with security implications. - // The relative paths check should restrict access to alloc-dir subtree - // documenting existing behavior in test here and need to follow up in another commit - requiresVolumes: false, - - name: "bind relative outside", + name: "bind relative outside", + requiresVolumes: true, passedMounts: []DockerMount{ { Type: "bind", diff --git a/drivers/docker/utils.go b/drivers/docker/utils.go index 7b482e11ad01..3b0b4a98446f 100644 --- a/drivers/docker/utils.go +++ b/drivers/docker/utils.go @@ -5,6 +5,7 @@ import ( "fmt" "os" "os/exec" + "path/filepath" "strings" "github.com/docker/cli/cli/config/configfile" @@ -200,3 +201,20 @@ func validateCgroupPermission(s string) bool { return true } + +// expandPath returns the absolute path of dir, relative to base if dir is relative path. +// base is expected to be an absolute path +func expandPath(base, dir string) string { + if filepath.IsAbs(dir) { + return filepath.Clean(dir) + } + + return filepath.Clean(filepath.Join(base, dir)) +} + +// isParentPath returns true if path is a child or decendant of parent path. +// Both inputes need to be absolute paths. +func isParentPath(parent, path string) bool { + rel, err := filepath.Rel(parent, path) + return err == nil && !strings.HasPrefix(rel, "..") +} diff --git a/drivers/docker/utils_test.go b/drivers/docker/utils_test.go index 6598d8313fcc..edc10dd0f62d 100644 --- a/drivers/docker/utils_test.go +++ b/drivers/docker/utils_test.go @@ -35,3 +35,38 @@ func TestValidateCgroupPermission(t *testing.T) { } } + +func TestExpandPath(t *testing.T) { + cases := []struct { + base string + target string + expected string + }{ + {"/tmp/alloc/task", "/home/user", "/home/user"}, + {"/tmp/alloc/task", "/home/user/..", "/home"}, + + {"/tmp/alloc/task", ".", "/tmp/alloc/task"}, + {"/tmp/alloc/task", "..", "/tmp/alloc"}, + + {"/tmp/alloc/task", "d1/d2", "/tmp/alloc/task/d1/d2"}, + {"/tmp/alloc/task", "../d1/d2", "/tmp/alloc/d1/d2"}, + {"/tmp/alloc/task", "../../d1/d2", "/tmp/d1/d2"}, + } + + for _, c := range cases { + t.Run(c.expected, func(t *testing.T) { + require.Equal(t, c.expected, expandPath(c.base, c.target)) + }) + } +} + +func TestIsParentPath(t *testing.T) { + require.True(t, isParentPath("/a/b/c", "/a/b/c")) + require.True(t, isParentPath("/a/b/c", "/a/b/c/d")) + require.True(t, isParentPath("/a/b/c", "/a/b/c/d/e")) + + require.False(t, isParentPath("/a/b/c", "/a/b/d")) + require.False(t, isParentPath("/a/b/c", "/a/b/cd")) + require.False(t, isParentPath("/a/b/c", "/a/d/c")) + require.False(t, isParentPath("/a/b/c", "/d/e/c")) +} From fa9cd5e25a474b490ef8d6e38ef3034e5dd720b5 Mon Sep 17 00:00:00 2001 From: Mahmood Ali Date: Tue, 11 Dec 2018 14:16:53 -0500 Subject: [PATCH 2/2] fix comment typos --- drivers/docker/utils.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/drivers/docker/utils.go b/drivers/docker/utils.go index 3b0b4a98446f..fc20b30b7872 100644 --- a/drivers/docker/utils.go +++ b/drivers/docker/utils.go @@ -212,8 +212,8 @@ func expandPath(base, dir string) string { return filepath.Clean(filepath.Join(base, dir)) } -// isParentPath returns true if path is a child or decendant of parent path. -// Both inputes need to be absolute paths. +// isParentPath returns true if path is a child or a descendant of parent path. +// Both inputs need to be absolute paths. func isParentPath(parent, path string) bool { rel, err := filepath.Rel(parent, path) return err == nil && !strings.HasPrefix(rel, "..")