From 52926d906efa0d518c62ee84a382542ad3e80a82 Mon Sep 17 00:00:00 2001 From: Tim Gross Date: Fri, 7 Oct 2022 15:43:37 -0400 Subject: [PATCH] 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 | 154 ++++++++++++------ .../shared/executor/executor_linux_test.go | 140 ++++++++++++---- 2 files changed, 215 insertions(+), 79 deletions(-) diff --git a/drivers/shared/executor/executor_linux.go b/drivers/shared/executor/executor_linux.go index fc91fc367280..84e5f8a19375 100644 --- a/drivers/shared/executor/executor_linux.go +++ b/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 { diff --git a/drivers/shared/executor/executor_linux_test.go b/drivers/shared/executor/executor_linux_test.go index d6b2d137fa50..8827c63b2b09 100644 --- a/drivers/shared/executor/executor_linux_test.go +++ b/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