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) }) }