From 254901a51ee82fd3a647001e0691d766da3af31d Mon Sep 17 00:00:00 2001 From: Michael Schurter Date: Mon, 1 Apr 2019 14:17:42 -0700 Subject: [PATCH 1/4] executor/linux: make chroot binary paths absolute Avoid libcontainer.Process trying to lookup the binary via $PATH as the executor has already found where the binary is located. --- .../taskrunner/task_runner_test.go | 79 +++++++++++++++++++ .../allocrunner/taskrunner/testdata/noop.sh | 2 + drivers/shared/executor/executor_linux.go | 5 +- 3 files changed, 85 insertions(+), 1 deletion(-) create mode 100644 client/allocrunner/taskrunner/testdata/noop.sh diff --git a/client/allocrunner/taskrunner/task_runner_test.go b/client/allocrunner/taskrunner/task_runner_test.go index 997a217183e..99900b78509 100644 --- a/client/allocrunner/taskrunner/task_runner_test.go +++ b/client/allocrunner/taskrunner/task_runner_test.go @@ -1054,6 +1054,85 @@ func TestTaskRunner_DeriveToken_Unrecoverable(t *testing.T) { require.True(t, state.Events[2].FailsTask) } +// TestTaskRunner_Download_ChrootExec asserts that downloaded artifacts may be +// executed in a chroot. +func TestTaskRunner_Download_ChrootExec(t *testing.T) { + t.Parallel() + ctestutil.ExecCompatible(t) + + ts := httptest.NewServer(http.FileServer(http.Dir(filepath.Dir(".")))) + defer ts.Close() + + // Create a task that downloads a script and executes it. + alloc := mock.BatchAlloc() + alloc.Job.TaskGroups[0].RestartPolicy = &structs.RestartPolicy{} + task := alloc.Job.TaskGroups[0].Tasks[0] + task.Driver = "exec" + task.Config = map[string]interface{}{ + "command": "noop.sh", + } + task.Artifacts = []*structs.TaskArtifact{ + { + GetterSource: fmt.Sprintf("%s/testdata/noop.sh", ts.URL), + GetterMode: "file", + RelativeDest: "noop.sh", + }, + } + + tr, _, cleanup := runTestTaskRunner(t, alloc, task.Name) + defer cleanup() + + // Wait for task to run and exit + select { + case <-tr.WaitCh(): + case <-time.After(time.Duration(testutil.TestMultiplier()*15) * time.Second): + require.Fail(t, "timed out waiting for task runner to exit") + } + + state := tr.TaskState() + require.Equal(t, structs.TaskStateDead, state.State) + require.False(t, state.Failed) +} + +// TestTaskRunner_Download_Exec asserts that downloaded artifacts may be +// executed in a driver without filesystem isolation. +func TestTaskRunner_Download_RawExec(t *testing.T) { + t.Parallel() + + ts := httptest.NewServer(http.FileServer(http.Dir(filepath.Dir(".")))) + defer ts.Close() + + // Create a task that downloads a script and executes it. + alloc := mock.BatchAlloc() + alloc.Job.TaskGroups[0].RestartPolicy = &structs.RestartPolicy{} + task := alloc.Job.TaskGroups[0].Tasks[0] + task.Driver = "raw_exec" + task.Config = map[string]interface{}{ + "command": "noop.sh", + } + task.Artifacts = []*structs.TaskArtifact{ + { + GetterSource: fmt.Sprintf("%s/testdata/noop.sh", ts.URL), + GetterMode: "file", + RelativeDest: "noop.sh", + }, + } + + tr, _, cleanup := runTestTaskRunner(t, alloc, task.Name) + defer cleanup() + + // Wait for task to run and exit + select { + case <-tr.WaitCh(): + case <-time.After(time.Duration(testutil.TestMultiplier()*15) * time.Second): + require.Fail(t, "timed out waiting for task runner to exit") + } + + state := tr.TaskState() + require.Equal(t, structs.TaskStateDead, state.State) + require.False(t, state.Failed) +} + // TestTaskRunner_Download_List asserts that multiple artificats are downloaded // before a task is run. func TestTaskRunner_Download_List(t *testing.T) { diff --git a/client/allocrunner/taskrunner/testdata/noop.sh b/client/allocrunner/taskrunner/testdata/noop.sh new file mode 100644 index 00000000000..8832c26a2d1 --- /dev/null +++ b/client/allocrunner/taskrunner/testdata/noop.sh @@ -0,0 +1,2 @@ +#!/bin/sh +echo "ok" diff --git a/drivers/shared/executor/executor_linux.go b/drivers/shared/executor/executor_linux.go index 3ac4618aa5a..3cd8bfda589 100644 --- a/drivers/shared/executor/executor_linux.go +++ b/drivers/shared/executor/executor_linux.go @@ -161,7 +161,10 @@ func (l *LibcontainerExecutor) Launch(command *ExecCommand) (*ProcessState, erro if err != nil { return nil, fmt.Errorf("failed to determine relative path base=%q target=%q: %v", command.TaskDir, path, err) } - path = rel + + // Turn relative-to-chroot path into absolute path to avoid + // libcontainer trying to resolve the binary using $PATH + path = "/" + rel combined := append([]string{path}, command.Args...) stdout, err := command.Stdout() From cb36f4537e63d53b198c2a87d1e03880895631bd Mon Sep 17 00:00:00 2001 From: Michael Schurter Date: Tue, 2 Apr 2019 09:40:53 -0700 Subject: [PATCH 2/4] executor/linux: add defensive checks to binary path --- client/allocrunner/taskrunner/task_runner_test.go | 2 +- drivers/shared/executor/executor_linux.go | 8 +++++++- 2 files changed, 8 insertions(+), 2 deletions(-) diff --git a/client/allocrunner/taskrunner/task_runner_test.go b/client/allocrunner/taskrunner/task_runner_test.go index 99900b78509..45f5ed92a06 100644 --- a/client/allocrunner/taskrunner/task_runner_test.go +++ b/client/allocrunner/taskrunner/task_runner_test.go @@ -1085,7 +1085,7 @@ func TestTaskRunner_Download_ChrootExec(t *testing.T) { // Wait for task to run and exit select { case <-tr.WaitCh(): - case <-time.After(time.Duration(testutil.TestMultiplier()*15) * time.Second): + case <-time.After(time.Duration(testutil.TestMultiplier()*30) * time.Second): require.Fail(t, "timed out waiting for task runner to exit") } diff --git a/drivers/shared/executor/executor_linux.go b/drivers/shared/executor/executor_linux.go index 3cd8bfda589..7b700b641ba 100644 --- a/drivers/shared/executor/executor_linux.go +++ b/drivers/shared/executor/executor_linux.go @@ -162,9 +162,15 @@ func (l *LibcontainerExecutor) Launch(command *ExecCommand) (*ProcessState, erro return nil, fmt.Errorf("failed to determine relative path base=%q target=%q: %v", command.TaskDir, path, err) } + // filepath.Rel will only return leading dots if the path escapes the + // TaskDir + if strings.HasPrefix(rel, "..") { + return nil, fmt.Errorf("command path %q escapes task dir %q", path, command.TaskDir) + } + // Turn relative-to-chroot path into absolute path to avoid // libcontainer trying to resolve the binary using $PATH - path = "/" + rel + path = filepath.Join("/", rel) combined := append([]string{path}, command.Args...) stdout, err := command.Stdout() From 21e895e2e7e274dd9319acbd5c11fcd38d0fea8b Mon Sep 17 00:00:00 2001 From: Michael Schurter Date: Tue, 2 Apr 2019 11:17:12 -0700 Subject: [PATCH 3/4] Revert "executor/linux: add defensive checks to binary path" This reverts commit cb36f4537e63d53b198c2a87d1e03880895631bd. --- client/allocrunner/taskrunner/task_runner_test.go | 2 +- drivers/shared/executor/executor_linux.go | 8 +------- 2 files changed, 2 insertions(+), 8 deletions(-) diff --git a/client/allocrunner/taskrunner/task_runner_test.go b/client/allocrunner/taskrunner/task_runner_test.go index 45f5ed92a06..99900b78509 100644 --- a/client/allocrunner/taskrunner/task_runner_test.go +++ b/client/allocrunner/taskrunner/task_runner_test.go @@ -1085,7 +1085,7 @@ func TestTaskRunner_Download_ChrootExec(t *testing.T) { // Wait for task to run and exit select { case <-tr.WaitCh(): - case <-time.After(time.Duration(testutil.TestMultiplier()*30) * time.Second): + case <-time.After(time.Duration(testutil.TestMultiplier()*15) * time.Second): require.Fail(t, "timed out waiting for task runner to exit") } diff --git a/drivers/shared/executor/executor_linux.go b/drivers/shared/executor/executor_linux.go index 7b700b641ba..3cd8bfda589 100644 --- a/drivers/shared/executor/executor_linux.go +++ b/drivers/shared/executor/executor_linux.go @@ -162,15 +162,9 @@ func (l *LibcontainerExecutor) Launch(command *ExecCommand) (*ProcessState, erro return nil, fmt.Errorf("failed to determine relative path base=%q target=%q: %v", command.TaskDir, path, err) } - // filepath.Rel will only return leading dots if the path escapes the - // TaskDir - if strings.HasPrefix(rel, "..") { - return nil, fmt.Errorf("command path %q escapes task dir %q", path, command.TaskDir) - } - // Turn relative-to-chroot path into absolute path to avoid // libcontainer trying to resolve the binary using $PATH - path = filepath.Join("/", rel) + path = "/" + rel combined := append([]string{path}, command.Args...) stdout, err := command.Stdout() From 56048bda0a15abec37b680d15e945b95ef504e4b Mon Sep 17 00:00:00 2001 From: Michael Schurter Date: Tue, 2 Apr 2019 11:25:45 -0700 Subject: [PATCH 4/4] executor/linux: comment this bizarre code --- drivers/shared/executor/executor_linux.go | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/drivers/shared/executor/executor_linux.go b/drivers/shared/executor/executor_linux.go index 3cd8bfda589..6c978e78778 100644 --- a/drivers/shared/executor/executor_linux.go +++ b/drivers/shared/executor/executor_linux.go @@ -163,7 +163,10 @@ func (l *LibcontainerExecutor) Launch(command *ExecCommand) (*ProcessState, erro } // Turn relative-to-chroot path into absolute path to avoid - // libcontainer trying to resolve the binary using $PATH + // 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...)