artifact/template: make destination path absolute inside taskdir (0.1…

…2.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.
hashicorp · Oct 22, 2020 · f3a499b · f3a499b
1 parent 5f47b57
commit f3a499b
Show file tree

Hide file tree

Showing 5 changed files with 97 additions and 55 deletions.
diff --git 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
@@ -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
@@ -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
@@ -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
@@ -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)
 		})
 	}