[wip]

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, fromMount, err := lookupTaskBin(command)
-
+	taskPath, hostPath, err := lookupTaskBin(command)
 	if err != nil {
 		return nil, err
 	}
-	path := absPath
-
-	if !fromMount {
-		if err := makeExecutable(absPath); err != nil {
-			return nil, err
-		}
-
-		// 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
+	if err := makeExecutable(hostPath); err != nil {
+		return nil, err
 	}
 
-	combined := append([]string{path}, command.Args...)
+	combined := append([]string{taskPath}, command.Args...)
 	stdout, err := command.Stdout()
 	if err != nil {
 		return nil, err
@@ -805,61 +788,116 @@ 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, bool, 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 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, false, nil
+	taskPath, hostPath, err := getPathInDir(command.TaskDir, local, 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, false, nil
+	taskPath, hostPath, err = getPathInDir(command.TaskDir, command.TaskDir, bin)
+	if err == nil {
+		return taskPath, hostPath, nil
 	}
 
 	// Check in our mounts
 	for _, mount := range command.Mounts {
-		inMount := filepath.Join(mount.HostPath, bin)
-		if _, err := os.Stat(inMount); err == nil {
-			return filepath.Join(mount.TaskPath, bin), true, nil
+		taskPath, hostPath, err = getPathInDir(mount.TaskPath, mount.HostPath, 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 "", false, 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)
 
-	pathFromPath, err := lookPathIn(path, taskDir, bin)
-	return pathFromPath, false, err
-}
+	// 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 = getPathInDir(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)
+}
+
+// // lookPathIn looks for a file with a fake lookup PATH environment variable,
+// // 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)
+// func lookPathIn(lookupPathEnvVar string, root string, bin string) (string, error) {
+// 	for _, dir := range filepath.SplitList(lookupPathEnvVar) {
+// 		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
+// 		}
+// 	}
+// 	return "", fmt.Errorf("file %s not found under path %s", bin, root)
+// }
+
+func getPathInDir(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 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 "/" + rel, hostPath, nil
 }
 
 func newSetCPUSetCgroupHook(cgroupPath string) lconfigs.Hook {

drivers/shared/executor/executor_linux_test.go

@@ -430,8 +430,9 @@ func TestUniversalExecutor_LookupTaskBin(t *testing.T) {
 
 	// Lookout with an absolute path to the binary
 	cmd.Cmd = "/foo/tmp.txt"
-	_, _, err = lookupTaskBin(cmd)
+	path, _, err := lookupTaskBin(cmd)
 	require.NoError(err)
+	fmt.Println("path:", path)
 
 	// Write a file under local subdir
 	os.MkdirAll(filepath.Join(tmpDir, "local"), 0700)
@@ -440,13 +441,20 @@ func TestUniversalExecutor_LookupTaskBin(t *testing.T) {
 
 	// Lookup with file name, should find the one we wrote above
 	cmd.Cmd = "tmp.txt"
-	_, _, err = lookupTaskBin(cmd)
+	path, _, err = lookupTaskBin(cmd)
 	require.NoError(err)
+	fmt.Println("path:", path)
 
-	// Lookup a host absolute path
+	// Lookup a host absolute path outside the taskdir, should error
 	cmd.Cmd = "/bin/sh"
 	_, _, err = lookupTaskBin(cmd)
 	require.Error(err)
+
+	// TODO: this looks wrong but apparently works out ok?
+	// Lookup a relative path outside the sandbox, should error
+	cmd.Cmd = "../../../bin/kill"
+	_, _, err = lookupTaskBin(cmd)
+	require.Error(err)
 }
 
 // Exec Launch looks for the binary only inside the chroot