diff --git a/client/allocrunner/taskrunner/getter/getter.go b/client/allocrunner/taskrunner/getter/getter.go index 45b8056eac94..d5737ff1af7a 100644 --- a/client/allocrunner/taskrunner/getter/getter.go +++ b/client/allocrunner/taskrunner/getter/getter.go @@ -1,6 +1,7 @@ package getter import ( + "errors" "fmt" "net/http" "net/url" @@ -31,7 +32,6 @@ const ( // is usually satisfied by taskenv.TaskEnv. type EnvReplacer interface { ReplaceEnv(string) string - ReplaceEnvClient(string) string ClientPath(string) (string, bool) } @@ -140,8 +140,7 @@ func GetArtifact(taskEnv EnvReplacer, artifact *structs.TaskArtifact) error { // Verify the destination is still in the task sandbox after interpolation if escapes { return newGetError(artifact.RelativeDest, - fmt.Errorf("artifact escapes the alloc directory rawDest=%s dest=%s", artifact.RelativeDest, dest), - // errors.New("artifact destination path escapes the alloc directory"), + errors.New("artifact destination path escapes the alloc directory"), false) } diff --git a/client/allocrunner/taskrunner/getter/getter_test.go b/client/allocrunner/taskrunner/getter/getter_test.go index e2ec27f9bf4b..02d27cb8f39c 100644 --- a/client/allocrunner/taskrunner/getter/getter_test.go +++ b/client/allocrunner/taskrunner/getter/getter_test.go @@ -26,28 +26,23 @@ type noopReplacer struct { taskDir string } -func (noopReplacer) ReplaceEnv(s string) string { - return s -} - -func (r noopReplacer) ReplaceEnvClient(s string) string { - return r.ReplaceEnv(s) +func clientPath(taskDir, path string) (string, bool) { + if !filepath.IsAbs(path) { + path = filepath.Join(taskDir, path) + } + path = filepath.Clean(path) + if taskDir != "" && !helper.PathEscapesSandbox(taskDir, path) { + return path, false + } + return path, true } -func checkEscape(taskDir, testPath string) bool { - if taskDir != "" && !helper.PathEscapesSandbox(taskDir, testPath) { - return false - } - return true +func (noopReplacer) ReplaceEnv(s string) string { + return s } func (r noopReplacer) ClientPath(p string) (string, bool) { - path := r.ReplaceEnv(p) - if !filepath.IsAbs(path) { - path = filepath.Join(r.taskDir, path) - } - path = filepath.Clean(path) - escapes := checkEscape(r.taskDir, path) + path, escapes := clientPath(r.taskDir, r.ReplaceEnv(p)) return path, escapes } @@ -67,16 +62,8 @@ func (upperReplacer) ReplaceEnv(s string) string { return strings.ToUpper(s) } -func (u upperReplacer) ReplaceEnvClient(s string) string { - return u.ReplaceEnv(s) -} - func (u upperReplacer) ClientPath(p string) (string, bool) { - path := u.ReplaceEnv(p) - if !filepath.IsAbs(path) { - path = filepath.Join(u.taskDir, path) - } - escapes := checkEscape(u.taskDir, path) + path, escapes := clientPath(u.taskDir, u.ReplaceEnv(p)) return path, escapes } diff --git a/client/allocrunner/taskrunner/template/template.go b/client/allocrunner/taskrunner/template/template.go index 0c8988dd486b..ad56761230f8 100644 --- a/client/allocrunner/taskrunner/template/template.go +++ b/client/allocrunner/taskrunner/template/template.go @@ -6,7 +6,6 @@ import ( "fmt" "math/rand" "os" - "path/filepath" "sort" "strconv" "strings" @@ -725,10 +724,8 @@ func loadTemplateEnv(tmpls []*structs.Template, taskDir string, taskEnv *taskenv continue } - dest := taskEnv.ReplaceEnvClient(t.DestPath) - if !filepath.IsAbs(dest) { - dest = filepath.Join(taskDir, dest) - } + // we checked escape before we rendered the file + dest, _ := taskEnv.ClientPath(t.DestPath) f, err := os.Open(dest) if err != nil { return nil, fmt.Errorf("error opening env template: %v", err) diff --git a/client/taskenv/env.go b/client/taskenv/env.go index 3348f24e8be5..c5751b1e09aa 100644 --- a/client/taskenv/env.go +++ b/client/taskenv/env.go @@ -306,7 +306,7 @@ func (t *TaskEnv) ReplaceEnv(arg string) string { return hargs.ReplaceEnv(arg, t.EnvMap, t.NodeAttrs) } -// ReplaceEnvClient takes an arg and replaces all occurrences of client-specific +// replaceEnvClient takes an arg and replaces all occurrences of client-specific // environment variables and Nomad variables. If the variable is found in the // passed map it is replaced, otherwise the original string is returned. // The difference from ReplaceEnv client is potentially difference values for @@ -314,8 +314,10 @@ func (t *TaskEnv) ReplaceEnv(arg string) string { // * NOMAD_ALLOC_DIR // * NOMAD_TASK_DIR // * NOMAD_SECRETS_DIR +// and anything that was interpolated using them. +// // See https://github.com/hashicorp/nomad/pull/9668 -func (t *TaskEnv) ReplaceEnvClient(arg string) string { +func (t *TaskEnv) replaceEnvClient(arg string) string { return hargs.ReplaceEnv(arg, t.EnvMapClient, t.NodeAttrs) } @@ -331,8 +333,16 @@ func (t *TaskEnv) checkEscape(testPath string) bool { return true } +// ClientPath interpolates the argument as a path, using the +// environment variables with client-relative directories. The +// result is an absolute path on the client filesystem. +// +// If the interpolated result is a relative path, it is made absolute +// wrt to the task working directory. +// The result is checked to see whether it escapes both the task working +// directory and the shared allocation directory. func (t *TaskEnv) ClientPath(rawPath string) (string, bool) { - path := t.ReplaceEnvClient(rawPath) + path := t.replaceEnvClient(rawPath) if !filepath.IsAbs(path) { path = filepath.Join(t.clientTaskDir, path) }