drivers/docker: enforce volumes.enabled (#4983)

When volumes.enable flag is off in Docker driver, disable all mounts of
paths outside alloc dir.

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)
 			}
 		}
 

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",

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 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, "..")
+}

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"))
+}