artifact/template: make destination path absolute inside taskdir (0.10.7)

CHANGELOG.md

@@ -1,3 +1,9 @@
+## 0.10.7 (October 23, 2020)
+
+BUG FIXES:
+ * artifact: 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: 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)
 
 SECURITY:

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

client/allocrunner/taskrunner/template/template.go

@@ -548,19 +548,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")
 			}
 		}

helper/funcs.go

@@ -390,21 +390,16 @@ func CheckHCLKeys(node ast.Node, valid []string) error {
 	return result
 }
 
-// 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
 }

helper/funcs_test.go

@@ -2,6 +2,7 @@ package helper
 
 import (
 	"fmt"
+	"path/filepath"
 	"reflect"
 	"sort"
 	"testing"
@@ -160,70 +161,86 @@ func BenchmarkCleanEnvVar(b *testing.B) {
 	}
 }
 
-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)
 		})
 	}