From 8a90b7eb161a151875ca82050d2f028aa80c904a Mon Sep 17 00:00:00 2001 From: Tim Gross Date: Thu, 22 Oct 2020 15:47:49 -0400 Subject: [PATCH] artifact/template: make destination path absolute inside taskdir (#9149) Prior to Nomad 0.12.5, you could use `${NOMAD_SECRETS_DIR}/mysecret.txt` as the `artifact.destination` and `template.destination` because we would always append the destination to the task working directory. In the recent security patch we treated the `destination` absolute path as valid if it didn't escape the working directory, but this breaks backwards compatibility and interpolation of `destination` fields. This changeset partially reverts the behavior so that we always append the destination, but we also perform the escape check on that new destination after interpolation so the security hole is closed. Also, ConsulTemplate test should exercise interpolation --- .../allocrunner/taskrunner/getter/getter.go | 9 +- .../taskrunner/template/template.go | 18 ++-- e2e/consultemplate/input/templating.nomad | 8 +- helper/funcs.go | 19 ++-- helper/funcs_test.go | 89 +++++++++++-------- 5 files changed, 84 insertions(+), 59 deletions(-) diff --git a/client/allocrunner/taskrunner/getter/getter.go b/client/allocrunner/taskrunner/getter/getter.go index f40d4f306..a1abb456a 100644 --- a/client/allocrunner/taskrunner/getter/getter.go +++ b/client/allocrunner/taskrunner/getter/getter.go @@ -4,6 +4,7 @@ import ( "errors" "fmt" "net/url" + "path/filepath" "strings" "sync" @@ -98,8 +99,12 @@ func GetArtifact(taskEnv EnvReplacer, artifact *structs.TaskArtifact, taskDir st } // Verify the destination is still in the task sandbox after interpolation - dest, err := helper.GetPathInSandbox(taskDir, artifact.RelativeDest) - if err != nil { + // Note: we *always* join here even if we get passed an absolute path so + // that $NOMAD_SECRETS_DIR and friends can be used and always fall inside + // the task working directory + dest := filepath.Join(taskDir, artifact.RelativeDest) + escapes := helper.PathEscapesSandbox(taskDir, dest) + if escapes { return newGetError(artifact.RelativeDest, errors.New("artifact destination path escapes the alloc directory"), false) } diff --git a/client/allocrunner/taskrunner/template/template.go b/client/allocrunner/taskrunner/template/template.go index ee73c83d3..2bf11400c 100644 --- a/client/allocrunner/taskrunner/template/template.go +++ b/client/allocrunner/taskrunner/template/template.go @@ -551,19 +551,27 @@ func parseTemplateConfigs(config *TaskTemplateManagerConfig) (map[*ctconf.Templa 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 != "" { src = taskEnv.ReplaceEnv(tmpl.SourcePath) - src, err = helper.GetPathInSandbox(config.TaskDir, src) - if err != nil && sandboxEnabled { + if !filepath.IsAbs(src) { + src = filepath.Join(config.TaskDir, src) + } else { + src = filepath.Clean(src) + } + escapes := helper.PathEscapesSandbox(config.TaskDir, src) + if escapes && sandboxEnabled { return nil, fmt.Errorf("template source path escapes alloc directory") } } if tmpl.DestPath != "" { dest = taskEnv.ReplaceEnv(tmpl.DestPath) - dest, err = helper.GetPathInSandbox(config.TaskDir, dest) - if err != nil && sandboxEnabled { + // Note: we *always* join here even if we get passed an absolute + // path so that $NOMAD_SECRETS_DIR and friends can be used and + // always fall inside the task working directory + dest = filepath.Join(config.TaskDir, dest) + escapes := helper.PathEscapesSandbox(config.TaskDir, dest) + if escapes && sandboxEnabled { return nil, fmt.Errorf("template destination path escapes alloc directory") } } diff --git a/e2e/consultemplate/input/templating.nomad b/e2e/consultemplate/input/templating.nomad index 7ae21af0a..39ff3d8c8 100644 --- a/e2e/consultemplate/input/templating.nomad +++ b/e2e/consultemplate/input/templating.nomad @@ -24,7 +24,7 @@ job "templating" { server {{ .Name }} {{ .Address }}:{{ .Port }}{{ end }} EOT - destination = "local/services.conf" + destination = "${NOMAD_TASK_DIR}/services.conf" change_mode = "noop" } @@ -35,7 +35,7 @@ key: {{ key "consultemplatetest" }} job: {{ env "NOMAD_JOB_NAME" }} EOT - destination = "local/kv.yml" + destination = "${NOMAD_TASK_DIR}/kv.yml" change_mode = "restart" } @@ -63,7 +63,7 @@ EOT server {{ .Name }} {{ .Address }}:{{ .Port }}{{ end }} EOT - destination = "local/services.conf" + destination = "${NOMAD_TASK_DIR}/services.conf" change_mode = "noop" } @@ -73,7 +73,7 @@ EOT key: {{ key "consultemplatetest" }} job: {{ env "NOMAD_JOB_NAME" }} EOT - destination = "local/kv.yml" + destination = "${NOMAD_TASK_DIR}/kv.yml" change_mode = "restart" } diff --git a/helper/funcs.go b/helper/funcs.go index 153850c22..d0277fa9f 100644 --- a/helper/funcs.go +++ b/helper/funcs.go @@ -514,21 +514,16 @@ 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) +// PathEscapesSandbox returns whether previously cleaned path inside the +// sandbox directory (typically this will be the allocation directory) +// escapes. +func PathEscapesSandbox(sandboxDir, path string) bool { rel, err := filepath.Rel(sandboxDir, path) if err != nil { - return path, err + return true } if strings.HasPrefix(rel, "..") { - return path, fmt.Errorf("%q escapes sandbox directory", path) + return true } - return path, nil + return false } diff --git a/helper/funcs_test.go b/helper/funcs_test.go index e209e9647..4e3268186 100644 --- a/helper/funcs_test.go +++ b/helper/funcs_test.go @@ -2,6 +2,7 @@ package helper import ( "fmt" + "path/filepath" "reflect" "sort" "testing" @@ -245,70 +246,86 @@ func TestCheckNamespaceScope(t *testing.T) { } } -func TestGetPathInSandbox(t *testing.T) { +func TestPathEscapesSandbox(t *testing.T) { cases := []struct { - name string - path string - dir string - expected string - expectedErr string + name string + path string + dir string + expected bool }{ { - name: "ok absolute path inside sandbox", - path: "/alloc/safe", + // this is the ${NOMAD_SECRETS_DIR} case + name: "ok joined absolute path inside sandbox", + path: filepath.Join("/alloc", "/secrets"), dir: "/alloc", - expected: "/alloc/safe", + expected: false, }, { - name: "ok relative path inside sandbox", + name: "fail unjoined absolute path outside sandbox", + path: "/secrets", + dir: "/alloc", + expected: true, + }, + { + name: "ok joined relative path inside sandbox", + path: filepath.Join("/alloc", "./safe"), + dir: "/alloc", + expected: false, + }, + { + name: "fail unjoined relative path outside sandbox", path: "./safe", dir: "/alloc", - expected: "/alloc/safe", + expected: true, }, { name: "ok relative path traversal constrained to sandbox", - path: "../../alloc/safe", + path: filepath.Join("/alloc", "../../alloc/safe"), dir: "/alloc", - expected: "/alloc/safe", + expected: false, }, { - name: "ok absolute path traversal constrained to sandbox", + name: "ok unjoined absolute path traversal constrained to sandbox", + path: filepath.Join("/alloc", "/../alloc/safe"), + dir: "/alloc", + expected: false, + }, + { + name: "ok unjoined absolute path traversal constrained to sandbox", path: "/../alloc/safe", dir: "/alloc", - expected: "/alloc/safe", + expected: false, }, { - name: "fail absolute path outside sandbox", - path: "/unsafe", - dir: "/alloc", - expected: "/unsafe", - expectedErr: "\"/unsafe\" escapes sandbox directory", + name: "fail joined relative path traverses outside sandbox", + path: filepath.Join("/alloc", "../../../unsafe"), + dir: "/alloc", + expected: true, }, { - name: "fail relative path traverses outside sandbox", - path: "../../../unsafe", - dir: "/alloc", - expected: "/unsafe", - expectedErr: "\"/unsafe\" escapes sandbox directory", + name: "fail unjoined relative path traverses outside sandbox", + path: "../../../unsafe", + dir: "/alloc", + expected: true, }, { - name: "fail absolute path tries to transverse outside sandbox", - path: "/alloc/../unsafe", - dir: "/alloc", - expected: "/unsafe", - expectedErr: "\"/unsafe\" escapes sandbox directory", + name: "fail joined absolute path tries to transverse outside sandbox", + path: filepath.Join("/alloc", "/alloc/../../unsafe"), + dir: "/alloc", + expected: true, + }, + { + name: "fail unjoined absolute path tries to transverse outside sandbox", + path: "/alloc/../../unsafe", + dir: "/alloc", + expected: true, }, } 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) - } + escapes := PathEscapesSandbox(tc.dir, tc.path) require.Equal(t, tc.expected, escapes, caseMsg) }) }