From f3a499bc44637e6e6f034fbf904b3e0cf06c2e03 Mon Sep 17 00:00:00 2001 From: Tim Gross Date: Thu, 22 Oct 2020 16:26:52 -0400 Subject: [PATCH] artifact/template: make destination path absolute inside taskdir (0.12.7) (#9150) 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. --- CHANGELOG.md | 17 ++++ .../allocrunner/taskrunner/getter/getter.go | 9 +- .../taskrunner/template/template.go | 18 ++-- helper/funcs.go | 19 ++-- helper/funcs_test.go | 89 +++++++++++-------- 5 files changed, 97 insertions(+), 55 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 33423300b04..3780a3f6ec1 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,3 +1,9 @@ +## 0.12.7 (October 23, 2020) + +BUG FIXES: + * artifact: Fixed a regression in 0.12.6 where if the artifact `destination` field is an absolute path it is not appended to the task working directory, breaking the use of `NOMAD_SECRETS_DIR` as part of the destination path. [[GH-9148](https://github.com/hashicorp/nomad/issues/9148)] + * template: Fixed a regression in 0.12.6 where if the template `destination` field is an absolute path it is not appended to the task working directory, breaking the use of `NOMAD_SECRETS_DIR` as part of the destination path. [[GH-9148](https://github.com/hashicorp/nomad/issues/9148)] + ## 0.12.6 (October 21, 2020) SECURITY: @@ -167,6 +173,12 @@ BUG FIXES: * ui: The task group detail page no longer makes excessive requests to the allocation and stats endpoints. [[GH-8216](https://github.com/hashicorp/nomad/issues/8216)] * ui: Polling endpoints that have yet to be fetched normally works as expected (regression from 0.11.3). [[GH-8207](https://github.com/hashicorp/nomad/issues/8207)] +## 0.11.6 (October 23, 2020) + +BUG FIXES: + * artifact: _Backport from v0.12.7_ - Fixed a regression in 0.11.5 where if the artifact `destination` field is an absolute path it is not appended to the task working directory, breaking the use of `NOMAD_SECRETS_DIR` as part of the destination path. [[GH-9148](https://github.com/hashicorp/nomad/issues/9148)] + * template: _Backport from v0.12.7_ - Fixed a regression in 0.11.5 where if the template `destination` field is an absolute path it is not appended to the task working directory, breaking the use of `NOMAD_SECRETS_DIR` as part of the destination path. [[GH-9148](https://github.com/hashicorp/nomad/issues/9148)] + ## 0.11.5 (October 21, 2020) SECURITY: @@ -319,6 +331,11 @@ BUG FIXES: * scheduler: Fixed a bug where changes to task group `shutdown_delay` were not persisted or displayed in plan output [[GH-7618](https://github.com/hashicorp/nomad/issues/7618)] * ui: Fixed handling of multi-byte unicode characters in allocation log view [[GH-7470](https://github.com/hashicorp/nomad/issues/7470)] [[GH-7551](https://github.com/hashicorp/nomad/pull/7551)] +## 0.10.7 (October 23, 2020) + +BUG FIXES: + * artifact: _Backport from v0.12.7_ - Fixed a regression in 0.10.6 where if the artifact `destination` field is an absolute path it is not appended to the task working directory, breaking the use of `NOMAD_SECRETS_DIR` as part of the destination path. [[GH-9148](https://github.com/hashicorp/nomad/issues/9148)] + * template: _Backport from v0.12.7_ - Fixed a regression in 0.10.6 where if the template `destination` field is an absolute path it is not appended to the task working directory, breaking the use of `NOMAD_SECRETS_DIR` as part of the destination path. [[GH-9148](https://github.com/hashicorp/nomad/issues/9148)] ## 0.10.6 (October 21, 2020) diff --git a/client/allocrunner/taskrunner/getter/getter.go b/client/allocrunner/taskrunner/getter/getter.go index f40d4f306ba..a1abb456a9c 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 e871300a86f..d20209777c2 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/helper/funcs.go b/helper/funcs.go index 153850c22ce..d0277fa9f79 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 e209e9647f1..4e32681866b 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: false, + }, + { + name: "ok unjoined absolute path traversal constrained to sandbox", + 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: "/../alloc/safe", dir: "/alloc", - expected: "/alloc/safe", + expected: false, + }, + { + name: "fail joined relative path traverses outside sandbox", + path: filepath.Join("/alloc", "../../../unsafe"), + dir: "/alloc", + expected: true, }, { - name: "fail absolute path 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 relative path traverses outside sandbox", - path: "../../../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 absolute path tries to transverse outside sandbox", - path: "/alloc/../unsafe", - dir: "/alloc", - expected: "/unsafe", - expectedErr: "\"/unsafe\" escapes sandbox directory", + 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) }) }