exec: allow running commands from host volume

The exec driver and other drivers derived from the shared executor check the
path of the command before handing off to libcontainer to ensure that the
command doesn't escape the sandbox. But we don't check any host volume mounts,
which should be safe to use as a source for executables if we're letting the
user mount them to the container in the first place.

Check the mount config to verify the executable lives in the mount's host path,
but then return an absolute path within the mount's task path so that we can hand
that off to libcontainer to run.

drivers/shared/executor/executor_linux.go

@@ -120,32 +120,15 @@ func (l *LibcontainerExecutor) Launch(command *ExecCommand) (*ProcessState, erro
 	l.container = container
 
 	// Look up the binary path and make it executable
-	absPath, err := lookupTaskBin(command)
-
+	taskPath, hostPath, err := lookupTaskBin(command)
 	if err != nil {
 		return nil, err
 	}
-
-	if err := makeExecutable(absPath); err != nil {
+	if err := makeExecutable(hostPath); err != nil {
 		return nil, err
 	}
 
-	path := absPath
-
-	// Ensure that the path is contained in the chroot, and find it relative to the container
-	rel, err := filepath.Rel(command.TaskDir, path)
-	if err != nil {
-		return nil, fmt.Errorf("failed to determine relative path base=%q target=%q: %v", command.TaskDir, path, err)
-	}
-
-	// Turn relative-to-chroot path into absolute path to avoid
-	// libcontainer trying to resolve the binary using $PATH.
-	// Do *not* use filepath.Join as it will translate ".."s returned by
-	// filepath.Rel. Prepending "/" will cause the path to be rooted in the
-	// chroot which is the desired behavior.
-	path = "/" + rel
-
-	combined := append([]string{path}, command.Args...)
+	combined := append([]string{taskPath}, command.Args...)
 	stdout, err := command.Stdout()
 	if err != nil {
 		return nil, err
@@ -805,52 +788,125 @@ func cmdMounts(mounts []*drivers.MountConfig) []*lconfigs.Mount {
 	return r
 }
 
-// lookupTaskBin finds the file `bin` in taskDir/local, taskDir in that order, then performs
-// a PATH search inside taskDir. It returns an absolute path. See also executor.lookupBin
-func lookupTaskBin(command *ExecCommand) (string, error) {
+// lookupTaskBin finds the file `bin` in taskDir/local, taskDir in that order,
+// then performs a PATH search inside taskDir. It returns an absolute path
+// inside the container that will get passed as arg[0] to the launched process,
+// and the absolute path to that binary as seen by the host (these will be
+// identical for binaries that don't come from mounts).
+//
+// See also executor.lookupBin for a version used by non-isolated drivers.
+func lookupTaskBin(command *ExecCommand) (string, string, error) {
 	taskDir := command.TaskDir
 	bin := command.Cmd
 
 	// Check in the local directory
 	localDir := filepath.Join(taskDir, allocdir.TaskLocal)
-	local := filepath.Join(localDir, bin)
-	if _, err := os.Stat(local); err == nil {
-		return local, nil
+	taskPath, hostPath, err := getPathInTaskDir(command.TaskDir, localDir, bin)
+	if err == nil {
+		return taskPath, hostPath, nil
 	}
 
 	// Check at the root of the task's directory
-	root := filepath.Join(taskDir, bin)
-	if _, err := os.Stat(root); err == nil {
-		return root, nil
+	taskPath, hostPath, err = getPathInTaskDir(command.TaskDir, command.TaskDir, bin)
+	if err == nil {
+		return taskPath, hostPath, nil
 	}
 
+	// Check in our mounts
+	for _, mount := range command.Mounts {
+		taskPath, hostPath, err = getPathInMount(mount.HostPath, mount.TaskPath, bin)
+		if err == nil {
+			return taskPath, hostPath, nil
+		}
+	}
+
+	// If there's a / in the binary's path, we can't fallback to a PATH search
 	if strings.Contains(bin, "/") {
-		return "", fmt.Errorf("file %s not found under path %s", bin, taskDir)
+		return "", "", fmt.Errorf("file %s not found under path %s", bin, taskDir)
 	}
 
-	path := "/usr/local/bin:/usr/bin:/bin"
+	// look for a file using a PATH-style lookup inside the directory
+	// root. Similar to the stdlib's exec.LookPath except:
+	//   - uses a restricted lookup PATH rather than the agent process's PATH env var.
+	//   - does not require that the file is already executable (this will be ensured
+	//     by the caller)
+	//   - does not prevent using relative path as added to exec.LookPath in go1.19
+	//     (this gets fixed-up in the caller)
 
-	return lookPathIn(path, taskDir, bin)
-}
+	// This is a fake PATH so that we're not using the agent's PATH
+	restrictedPaths := []string{"/usr/local/bin", "/usr/bin", "/bin"}
 
-// lookPathIn looks for a file with PATH inside the directory root. Like exec.LookPath
-func lookPathIn(path string, root string, bin string) (string, error) {
-	// exec.LookPath(file string)
-	for _, dir := range filepath.SplitList(path) {
-		if dir == "" {
-			// match unix shell behavior, empty path element == .
-			dir = "."
-		}
-		path := filepath.Join(root, dir, bin)
-		f, err := os.Stat(path)
-		if err != nil {
-			continue
-		}
-		if m := f.Mode(); !m.IsDir() {
-			return path, nil
+	for _, dir := range restrictedPaths {
+		pathDir := filepath.Join(command.TaskDir, dir)
+		taskPath, hostPath, err = getPathInTaskDir(command.TaskDir, pathDir, bin)
+		if err == nil {
+			return taskPath, hostPath, nil
 		}
 	}
-	return "", fmt.Errorf("file %s not found under path %s", bin, root)
+
+	return "", "", fmt.Errorf("file %s not found under path", bin)
+}
+
+// getPathInTaskDir searches for the binary in the task directory and nested
+// search directory. It returns the absolute path rooted inside the container
+// and the absolute path on the host.
+func getPathInTaskDir(taskDir, searchDir, bin string) (string, string, error) {
+
+	hostPath := filepath.Join(searchDir, bin)
+
+	f, err := os.Stat(hostPath)
+	if err != nil {
+		return "", "", err
+	}
+	if m := f.Mode(); m.IsDir() {
+		return "", "", fmt.Errorf("path was directory, not file")
+	}
+
+	// Find the path relative to the task directory
+	rel, err := filepath.Rel(taskDir, hostPath)
+	if rel == "" || err != nil {
+		return "", "", fmt.Errorf(
+			"failed to determine relative path base=%q target=%q: %v",
+			taskDir, hostPath, err)
+	}
+
+	// Turn relative-to-taskdir path into re-rooted absolute path to avoid
+	// libcontainer trying to resolve the binary using $PATH.
+	// Do *not* use filepath.Join as it will translate ".."s returned by
+	// filepath.Rel. Prepending "/" will cause the path to be rooted in the
+	// chroot which is the desired behavior.
+	return filepath.Clean("/" + rel), hostPath, nil
+}
+
+// getPathInMount for the binary in the mount's host path, constructing the path
+// considering that the bin path is rooted in the mount's task path and not its
+// host path. It returns the absolute path rooted inside the container and the
+// absolute path on the host.
+func getPathInMount(mountHostPath, mountTaskPath, bin string) (string, string, error) {
+
+	// Find the path relative to the mount point in the task so that we can
+	// trim off any shared prefix when we search on the host path
+	mountRel, err := filepath.Rel(mountTaskPath, bin)
+	if mountRel == "" || err != nil {
+		return "", "", fmt.Errorf("path was not relative to the mount task path")
+	}
+
+	hostPath := filepath.Join(mountHostPath, mountRel)
+
+	f, err := os.Stat(hostPath)
+	if err != nil {
+		return "", "", err
+	}
+	if m := f.Mode(); m.IsDir() {
+		return "", "", fmt.Errorf("path was directory, not file")
+	}
+
+	// Turn relative-to-taskdir path into re-rooted absolute path to avoid
+	// libcontainer trying to resolve the binary using $PATH.
+	// Do *not* use filepath.Join as it will translate ".."s returned by
+	// filepath.Rel. Prepending "/" will cause the path to be rooted in the
+	// chroot which is the desired behavior.
+	return filepath.Clean("/" + bin), hostPath, nil
 }
 
 func newSetCPUSetCgroupHook(cgroupPath string) lconfigs.Hook {

drivers/shared/executor/executor_linux_test.go

@@ -25,6 +25,8 @@ import (
 	"github.com/opencontainers/runc/libcontainer/cgroups"
 	lconfigs "github.com/opencontainers/runc/libcontainer/configs"
 	"github.com/opencontainers/runc/libcontainer/devices"
+	"github.com/shoenig/test"
+	"github.com/shoenig/test/must"
 	"github.com/stretchr/testify/require"
 	"golang.org/x/sys/unix"
 )
@@ -410,43 +412,121 @@ func TestExecutor_CgroupPathsAreDestroyed(t *testing.T) {
 	}
 }
 
-func TestUniversalExecutor_LookupTaskBin(t *testing.T) {
+func TestExecutor_LookupTaskBin(t *testing.T) {
 	ci.Parallel(t)
-	require := require.New(t)
 
 	// Create a temp dir
-	tmpDir := t.TempDir()
-
-	// Create the command
-	cmd := &ExecCommand{Env: []string{"PATH=/bin"}, TaskDir: tmpDir}
-
-	// Make a foo subdir
-	os.MkdirAll(filepath.Join(tmpDir, "foo"), 0700)
-
-	// Write a file under foo
-	filePath := filepath.Join(tmpDir, "foo", "tmp.txt")
-	err := ioutil.WriteFile(filePath, []byte{1, 2}, os.ModeAppend)
-	require.NoError(err)
+	taskDir := t.TempDir()
+	mountDir := t.TempDir()
 
-	// Lookout with an absolute path to the binary
-	cmd.Cmd = "/foo/tmp.txt"
-	_, err = lookupTaskBin(cmd)
-	require.NoError(err)
+	// Create the command with mounts
+	cmd := &ExecCommand{
+		Env:     []string{"PATH=/bin"},
+		TaskDir: taskDir,
+		Mounts:  []*drivers.MountConfig{{TaskPath: "/srv", HostPath: mountDir}},
+	}
 
-	// Write a file under local subdir
-	os.MkdirAll(filepath.Join(tmpDir, "local"), 0700)
-	filePath2 := filepath.Join(tmpDir, "local", "tmp.txt")
-	ioutil.WriteFile(filePath2, []byte{1, 2}, os.ModeAppend)
+	// Make a /foo /local/foo and /usr/local/bin subdirs under task dir
+	// and /bar under mountdir
+	must.NoError(t, os.MkdirAll(filepath.Join(taskDir, "foo"), 0700))
+	must.NoError(t, os.MkdirAll(filepath.Join(taskDir, "local/foo"), 0700))
+	must.NoError(t, os.MkdirAll(filepath.Join(taskDir, "usr/local/bin"), 0700))
+	must.NoError(t, os.MkdirAll(filepath.Join(mountDir, "bar"), 0700))
+
+	writeFile := func(paths ...string) {
+		t.Helper()
+		path := filepath.Join(paths...)
+		must.NoError(t, os.WriteFile(path, []byte("hello"), 0o700))
+	}
 
-	// Lookup with file name, should find the one we wrote above
-	cmd.Cmd = "tmp.txt"
-	_, err = lookupTaskBin(cmd)
-	require.NoError(err)
+	// Write some files
+	writeFile(taskDir, "usr/local/bin", "tmp0.txt") // under /usr/local/bin in taskdir
+	writeFile(taskDir, "foo", "tmp1.txt")           // under foo in taskdir
+	writeFile(taskDir, "local", "tmp2.txt")         // under root of task-local dir
+	writeFile(taskDir, "local/foo", "tmp3.txt")     // under foo in task-local dir
+	writeFile(mountDir, "tmp4.txt")                 // under root of mount dir
+	writeFile(mountDir, "bar/tmp5.txt")             // under bar in mount dir
+
+	testCases := []struct {
+		name           string
+		cmd            string
+		expectErr      string
+		expectTaskPath string
+		expectHostPath string
+	}{
+		{
+			name:           "lookup with file name in PATH",
+			cmd:            "tmp0.txt",
+			expectTaskPath: "/usr/local/bin/tmp0.txt",
+			expectHostPath: filepath.Join(taskDir, "usr/local/bin/tmp0.txt"),
+		},
+		{
+			name:           "lookup with absolute path to binary",
+			cmd:            "/foo/tmp1.txt",
+			expectTaskPath: "/foo/tmp1.txt",
+			expectHostPath: filepath.Join(taskDir, "foo/tmp1.txt"),
+		},
+		{
+			name:           "lookup in task local dir with absolute path to binary",
+			cmd:            "/local/tmp2.txt",
+			expectTaskPath: "/local/tmp2.txt",
+			expectHostPath: filepath.Join(taskDir, "local/tmp2.txt"),
+		},
+		{
+			name:           "lookup in task local dir with relative path to binary",
+			cmd:            "local/tmp2.txt",
+			expectTaskPath: "/local/tmp2.txt",
+			expectHostPath: filepath.Join(taskDir, "local/tmp2.txt"),
+		},
+		{
+			name:           "lookup in task local dir with file name",
+			cmd:            "tmp2.txt",
+			expectTaskPath: "/local/tmp2.txt",
+			expectHostPath: filepath.Join(taskDir, "local/tmp2.txt"),
+		},
+		{
+			name:           "lookup in task local subdir with absolute path to binary",
+			cmd:            "/local/foo/tmp3.txt",
+			expectTaskPath: "/local/foo/tmp3.txt",
+			expectHostPath: filepath.Join(taskDir, "local/foo/tmp3.txt"),
+		},
+		{
+			name:      "lookup host absolute path outside taskdir",
+			cmd:       "/bin/sh",
+			expectErr: "file /bin/sh not found under path " + taskDir,
+		},
+		{
+			name:           "lookup file from mount with absolute path",
+			cmd:            "/srv/tmp4.txt",
+			expectTaskPath: "/srv/tmp4.txt",
+			expectHostPath: filepath.Join(mountDir, "tmp4.txt"),
+		},
+		{
+			name:      "lookup file from mount with file name fails",
+			cmd:       "tmp4.txt",
+			expectErr: "file tmp4.txt not found under path",
+		},
+		{
+			name:           "lookup file from mount with subdir",
+			cmd:            "/srv/bar/tmp5.txt",
+			expectTaskPath: "/srv/bar/tmp5.txt",
+			expectHostPath: filepath.Join(mountDir, "bar/tmp5.txt"),
+		},
+	}
 
-	// Lookup a host absolute path
-	cmd.Cmd = "/bin/sh"
-	_, err = lookupTaskBin(cmd)
-	require.Error(err)
+	for _, tc := range testCases {
+		t.Run(tc.name, func(t *testing.T) {
+			cmd.Cmd = tc.cmd
+			taskPath, hostPath, err := lookupTaskBin(cmd)
+			if tc.expectErr == "" {
+				must.NoError(t, err)
+				test.Eq(t, tc.expectTaskPath, taskPath)
+				test.Eq(t, tc.expectHostPath, hostPath)
+			} else {
+				test.EqError(t, err, tc.expectErr)
+			}
+		})
+	}
 }
 
 // Exec Launch looks for the binary only inside the chroot