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

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.

Also, ConsulTemplate test should exercise interpolation

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)
+	// Note: we *always* join here even if we get passed an absolute path so
-	if err != nil {
+	// 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

@@ -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 !filepath.IsAbs(src) {
-			if err != nil && sandboxEnabled {
+				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)
+			// Note: we *always* join here even if we get passed an absolute
-			if err != nil && sandboxEnabled {
+			// 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")
 			}
 		}

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

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
+// PathEscapesSandbox returns whether previously cleaned path inside the
-// (typically this will be the allocation directory). Relative paths will be
+// sandbox directory (typically this will be the allocation directory)
-// joined to the sandbox directory. Returns an error if the path escapes the
+// escapes.
-// sandbox directory.
+func PathEscapesSandbox(sandboxDir, path string) bool {
-func GetPathInSandbox(sandboxDir, path string) (string, error) {
-	if !filepath.IsAbs(path) {
-		path = filepath.Join(sandboxDir, path)
-	}
-	path = filepath.Clean(path)
 	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"
@@ -245,70 +246,86 @@ func TestCheckNamespaceScope(t *testing.T) {
 	}
 }
 
-func TestGetPathInSandbox(t *testing.T) {
+func TestPathEscapesSandbox(t *testing.T) {
 	cases := []struct {
-		name        string
+		name     string
-		path        string
+		path     string
-		dir         string
+		dir      string
-		expected    string
+		expected bool
-		expectedErr string
 	}{
 		{
-			name:     "ok absolute path inside sandbox",
+			// this is the ${NOMAD_SECRETS_DIR} case
-			path:     "/alloc/safe",
+			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",
+			name:     "fail joined relative path traverses outside sandbox",
-			path:        "/unsafe",
+			path:     filepath.Join("/alloc", "../../../unsafe"),
-			dir:         "/alloc",
+			dir:      "/alloc",
-			expected:    "/unsafe",
+			expected: true,
-			expectedErr: "\"/unsafe\" escapes sandbox directory",
 		},
 		{
-			name:        "fail relative path traverses outside sandbox",
+			name:     "fail unjoined relative path traverses outside sandbox",
-			path:        "../../../unsafe",
+			path:     "../../../unsafe",
-			dir:         "/alloc",
+			dir:      "/alloc",
-			expected:    "/unsafe",
+			expected: true,
-			expectedErr: "\"/unsafe\" escapes sandbox directory",
 		},
 		{
-			name:        "fail absolute path tries to transverse outside sandbox",
+			name:     "fail joined absolute path tries to transverse outside sandbox",
-			path:        "/alloc/../unsafe",
+			path:     filepath.Join("/alloc", "/alloc/../../unsafe"),
-			dir:         "/alloc",
+			dir:      "/alloc",
-			expected:    "/unsafe",
+			expected: true,
-			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)
+			escapes := PathEscapesSandbox(tc.dir, tc.path)
-			if tc.expectedErr != "" {
-				require.EqualError(t, err, tc.expectedErr, caseMsg)
-			} else {
-				require.NoError(t, err, caseMsg)
-			}
 			require.Equal(t, tc.expected, escapes, caseMsg)
 		})
 	}