From 076db2ef6b581c7f7e055e6191bb5b1c5d03eeba Mon Sep 17 00:00:00 2001 From: Tim Gross Date: Fri, 16 Oct 2020 13:23:44 -0400 Subject: [PATCH] artifact/template: prevent file sandbox escapes Ensure that the client honors the client configuration for the `template.disable_file_sandbox` field when validating the jobspec's `template.source` parameter, and not just with consul-template's own `file` function. Prevent interpolated `template.source`, `template.destination`, and `artifact.destination` fields from escaping file sandbox. --- .../allocrunner/taskrunner/getter/getter.go | 11 ++- .../taskrunner/getter/getter_test.go | 30 ++++++++ .../taskrunner/template/template.go | 28 ++++---- .../taskrunner/template/template_test.go | 48 +++++++++++-- helper/funcs.go | 19 +++++ helper/funcs_test.go | 69 +++++++++++++++++++ 6 files changed, 182 insertions(+), 23 deletions(-) diff --git a/client/allocrunner/taskrunner/getter/getter.go b/client/allocrunner/taskrunner/getter/getter.go index c896d6f92..f40d4f306 100644 --- a/client/allocrunner/taskrunner/getter/getter.go +++ b/client/allocrunner/taskrunner/getter/getter.go @@ -1,13 +1,14 @@ package getter import ( + "errors" "fmt" "net/url" - "path/filepath" "strings" "sync" gg "github.com/hashicorp/go-getter" + "github.com/hashicorp/nomad/helper" "github.com/hashicorp/nomad/nomad/structs" ) @@ -96,8 +97,12 @@ func GetArtifact(taskEnv EnvReplacer, artifact *structs.TaskArtifact, taskDir st return newGetError(artifact.GetterSource, err, false) } - // Download the artifact - dest := filepath.Join(taskDir, artifact.RelativeDest) + // Verify the destination is still in the task sandbox after interpolation + dest, err := helper.GetPathInSandbox(taskDir, artifact.RelativeDest) + if err != nil { + return newGetError(artifact.RelativeDest, + errors.New("artifact destination path escapes the alloc directory"), false) + } // Convert from string getter mode to go-getter const mode := gg.ClientModeAny diff --git a/client/allocrunner/taskrunner/getter/getter_test.go b/client/allocrunner/taskrunner/getter/getter_test.go index 2816be10b..cfac7a010 100644 --- a/client/allocrunner/taskrunner/getter/getter_test.go +++ b/client/allocrunner/taskrunner/getter/getter_test.go @@ -93,6 +93,36 @@ func TestGetArtifact_File_RelativeDest(t *testing.T) { } } +func TestGetArtifact_File_EscapeDest(t *testing.T) { + // Create the test server hosting the file to download + ts := httptest.NewServer(http.FileServer(http.Dir(filepath.Dir("./test-fixtures/")))) + defer ts.Close() + + // Create a temp directory to download into + taskDir, err := ioutil.TempDir("", "nomad-test") + if err != nil { + t.Fatalf("failed to make temp directory: %v", err) + } + defer os.RemoveAll(taskDir) + + // Create the artifact + file := "test.sh" + relative := "../../../../foo/" + artifact := &structs.TaskArtifact{ + GetterSource: fmt.Sprintf("%s/%s", ts.URL, file), + GetterOptions: map[string]string{ + "checksum": "md5:bce963762aa2dbfed13caf492a45fb72", + }, + RelativeDest: relative, + } + + // attempt to download the artifact + err = GetArtifact(taskEnv, artifact, taskDir) + if err == nil || !strings.Contains(err.Error(), "escapes") { + t.Fatalf("expected GetArtifact to disallow sandbox escape: %v", err) + } +} + func TestGetGetterUrl_Interpolation(t *testing.T) { // Create the artifact artifact := &structs.TaskArtifact{ diff --git a/client/allocrunner/taskrunner/template/template.go b/client/allocrunner/taskrunner/template/template.go index 116a97406..ee73c83d3 100644 --- a/client/allocrunner/taskrunner/template/template.go +++ b/client/allocrunner/taskrunner/template/template.go @@ -28,10 +28,6 @@ const ( // consulTemplateSourceName is the source name when using the TaskHooks. consulTemplateSourceName = "Template" - // hostSrcOption is the Client option that determines whether the template - // source may be from the host - hostSrcOption = "template.allow_host_source" - // missingDepEventLimit is the number of missing dependencies that will be // logged before we switch to showing just the number of missing // dependencies. @@ -549,25 +545,27 @@ func maskProcessEnv(env map[string]string) map[string]string { // parseTemplateConfigs converts the tasks templates in the config into // consul-templates func parseTemplateConfigs(config *TaskTemplateManagerConfig) (map[*ctconf.TemplateConfig]*structs.Template, error) { - allowAbs := config.ClientConfig.ReadBoolDefault(hostSrcOption, true) + sandboxEnabled := !config.ClientConfig.TemplateConfig.DisableSandbox taskEnv := config.EnvBuilder.Build() ctmpls := make(map[*ctconf.TemplateConfig]*structs.Template, len(config.Templates)) for _, tmpl := range config.Templates { var src, dest string + var err error if tmpl.SourcePath != "" { - if filepath.IsAbs(tmpl.SourcePath) { - if !allowAbs { - return nil, fmt.Errorf("Specifying absolute template paths disallowed by client config: %q", tmpl.SourcePath) - } - - src = tmpl.SourcePath - } else { - src = filepath.Join(config.TaskDir, taskEnv.ReplaceEnv(tmpl.SourcePath)) + src = taskEnv.ReplaceEnv(tmpl.SourcePath) + src, err = helper.GetPathInSandbox(config.TaskDir, src) + if err != nil && sandboxEnabled { + return nil, fmt.Errorf("template source path escapes alloc directory") } } + if tmpl.DestPath != "" { - dest = filepath.Join(config.TaskDir, taskEnv.ReplaceEnv(tmpl.DestPath)) + dest = taskEnv.ReplaceEnv(tmpl.DestPath) + dest, err = helper.GetPathInSandbox(config.TaskDir, dest) + if err != nil && sandboxEnabled { + return nil, fmt.Errorf("template destination path escapes alloc directory") + } } ct := ctconf.DefaultTemplateConfig() @@ -577,7 +575,7 @@ func parseTemplateConfigs(config *TaskTemplateManagerConfig) (map[*ctconf.Templa ct.LeftDelim = &tmpl.LeftDelim ct.RightDelim = &tmpl.RightDelim ct.FunctionDenylist = config.ClientConfig.TemplateConfig.FunctionDenylist - if !config.ClientConfig.TemplateConfig.DisableSandbox { + if sandboxEnabled { ct.SandboxPath = &config.TaskDir } diff --git a/client/allocrunner/taskrunner/template/template_test.go b/client/allocrunner/taskrunner/template/template_test.go index af4962cbf..ba2340c84 100644 --- a/client/allocrunner/taskrunner/template/template_test.go +++ b/client/allocrunner/taskrunner/template/template_test.go @@ -382,7 +382,11 @@ func TestTaskTemplateManager_HostPath(t *testing.T) { } harness := newTestHarness(t, []*structs.Template{template}, false, false) - harness.start(t) + harness.config.TemplateConfig.DisableSandbox = true + err = harness.startWithErr() + if err != nil { + t.Fatalf("couldn't setup initial harness: %v", err) + } defer harness.stop() // Wait for the unblock @@ -405,12 +409,46 @@ func TestTaskTemplateManager_HostPath(t *testing.T) { // Change the config to disallow host sources harness = newTestHarness(t, []*structs.Template{template}, false, false) - harness.config.Options = map[string]string{ - hostSrcOption: "false", + err = harness.startWithErr() + if err == nil || !strings.Contains(err.Error(), "escapes alloc directory") { + t.Fatalf("Expected absolute template path disallowed for %q: %v", + template.SourcePath, err) } - if err := harness.startWithErr(); err == nil || !strings.Contains(err.Error(), "absolute") { - t.Fatalf("Expected absolute template path disallowed: %v", err) + + template.SourcePath = "../../../../../../" + file + harness = newTestHarness(t, []*structs.Template{template}, false, false) + err = harness.startWithErr() + if err == nil || !strings.Contains(err.Error(), "escapes alloc directory") { + t.Fatalf("Expected directory traversal out of %q disallowed for %q: %v", + harness.taskDir, template.SourcePath, err) } + + // Build a new task environment + a := mock.Alloc() + task := a.Job.TaskGroups[0].Tasks[0] + task.Name = TestTaskName + task.Meta = map[string]string{"ESCAPE": "../"} + + template.SourcePath = "${NOMAD_META_ESCAPE}${NOMAD_META_ESCAPE}${NOMAD_META_ESCAPE}${NOMAD_META_ESCAPE}${NOMAD_META_ESCAPE}${NOMAD_META_ESCAPE}" + file + harness = newTestHarness(t, []*structs.Template{template}, false, false) + harness.envBuilder = taskenv.NewBuilder(harness.node, a, task, "global") + err = harness.startWithErr() + if err == nil || !strings.Contains(err.Error(), "escapes alloc directory") { + t.Fatalf("Expected directory traversal out of %q via interpolation disallowed for %q: %v", + harness.taskDir, template.SourcePath, err) + } + + // Test with desination too + template.SourcePath = f.Name() + template.DestPath = "../../../../../../" + file + harness = newTestHarness(t, []*structs.Template{template}, false, false) + harness.envBuilder = taskenv.NewBuilder(harness.node, a, task, "global") + err = harness.startWithErr() + if err == nil || !strings.Contains(err.Error(), "escapes alloc directory") { + t.Fatalf("Expected directory traversal out of %q via interpolation disallowed for %q: %v", + harness.taskDir, template.SourcePath, err) + } + } func TestTaskTemplateManager_Unblock_Static(t *testing.T) { diff --git a/helper/funcs.go b/helper/funcs.go index 72b0ed69c..153850c22 100644 --- a/helper/funcs.go +++ b/helper/funcs.go @@ -513,3 +513,22 @@ func CheckNamespaceScope(provided string, requested []string) []string { } return nil } + +// GetPathInSandbox returns a cleaned path inside the sandbox directory +// (typically this will be the allocation directory). Relative paths will be +// joined to the sandbox directory. Returns an error if the path escapes the +// sandbox directory. +func GetPathInSandbox(sandboxDir, path string) (string, error) { + if !filepath.IsAbs(path) { + path = filepath.Join(sandboxDir, path) + } + path = filepath.Clean(path) + rel, err := filepath.Rel(sandboxDir, path) + if err != nil { + return path, err + } + if strings.HasPrefix(rel, "..") { + return path, fmt.Errorf("%q escapes sandbox directory", path) + } + return path, nil +} diff --git a/helper/funcs_test.go b/helper/funcs_test.go index 5c5fea94e..e209e9647 100644 --- a/helper/funcs_test.go +++ b/helper/funcs_test.go @@ -244,3 +244,72 @@ func TestCheckNamespaceScope(t *testing.T) { }) } } + +func TestGetPathInSandbox(t *testing.T) { + cases := []struct { + name string + path string + dir string + expected string + expectedErr string + }{ + { + name: "ok absolute path inside sandbox", + path: "/alloc/safe", + dir: "/alloc", + expected: "/alloc/safe", + }, + { + name: "ok relative path inside sandbox", + path: "./safe", + dir: "/alloc", + expected: "/alloc/safe", + }, + { + name: "ok relative path traversal constrained to sandbox", + path: "../../alloc/safe", + dir: "/alloc", + expected: "/alloc/safe", + }, + { + name: "ok absolute path traversal constrained to sandbox", + path: "/../alloc/safe", + dir: "/alloc", + expected: "/alloc/safe", + }, + { + name: "fail absolute path outside sandbox", + path: "/unsafe", + dir: "/alloc", + expected: "/unsafe", + expectedErr: "\"/unsafe\" escapes sandbox directory", + }, + { + name: "fail relative path traverses outside sandbox", + path: "../../../unsafe", + dir: "/alloc", + expected: "/unsafe", + expectedErr: "\"/unsafe\" escapes sandbox directory", + }, + { + name: "fail absolute path tries to transverse outside sandbox", + path: "/alloc/../unsafe", + dir: "/alloc", + expected: "/unsafe", + expectedErr: "\"/unsafe\" escapes sandbox directory", + }, + } + + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + caseMsg := fmt.Sprintf("path: %v\ndir: %v", tc.path, tc.dir) + escapes, err := GetPathInSandbox(tc.dir, tc.path) + if tc.expectedErr != "" { + require.EqualError(t, err, tc.expectedErr, caseMsg) + } else { + require.NoError(t, err, caseMsg) + } + require.Equal(t, tc.expected, escapes, caseMsg) + }) + } +}