template/artifact: prevent file sandbox escapes (0.11.5+oss) (#466)

Ensure that the client honors the client configuration for the
`template.disable_file_sandbox` field when validating the jobspec's
`template.source` parameter, and not just with consul-template's own `file`
function.

Prevent interpolated `template.source`, `template.destination`, and
`artifact.destination` fields from escaping file sandbox.

.circleci/config/workflows/build-test.yml

@@ -15,8 +15,6 @@ jobs:
             - /^docs-.*/
             - stable-website
 
-  - build-darwin-binaries:
-      filters: *backend_branches_filter
   - test-windows:
       filters: *backend_branches_filter
 

CHANGELOG.md

@@ -1,3 +1,11 @@
+## 0.11.5 (October 21, 2020)
+
+SECURITY:
+
+ * artifact: Fixed a bug where interpolation can be used in the artifact `destination` field to write artifact payloads outside the allocation directory. CVE-2020-27195 [[GH-9129](https://github.com/hashicorp/nomad/issues/9129)]
+ * template: Fixed a bug where interpolation can be used in the template `source` and `destination` fields to read or write files outside the allocation directory even when `disable_file_sandbox` was set to `false` (the default). CVE-2020-27195 [[GH-9129](https://github.com/hashicorp/nomad/issues/9129)]
+ * template: Fixed a bug where the `disable_file_sandbox` configuration was only respected for the template `file` function and not the template `source` and `destination` fields. CVE-2020-27195 [[GH-9129](https://github.com/hashicorp/nomad/issues/9129)]
+
 ## 0.11.4 (August 7, 2020)
 
 SECURITY:
@@ -140,6 +148,14 @@ 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.6 (October 21, 2020)
+
+SECURITY:
+
+ * artifact: _Backport from v0.12.6_ - Fixed a bug where interpolation can be used in the artifact `destination` field to write artifact payloads outside the allocation directory. CVE-2020-27195 [[GH-9129](https://github.com/hashicorp/nomad/issues/9129)]
+ * template: _Backport from v0.12.6_ - Fixed a bug where interpolation can be used in the template `source` and `destination` fields to read or write files outside the allocation directory even when `disable_file_sandbox` was set to `false` (the default). CVE-2020-27195 [[GH-9129](https://github.com/hashicorp/nomad/issues/9129)]
+ * template: _Backport from v0.12.6_ - Fixed a bug where the `disable_file_sandbox` configuration was only respected for the template `file` function and not the template `source` and `destination` fields. CVE-2020-27195 [[GH-9129](https://github.com/hashicorp/nomad/issues/9129)]
+
 ## 0.10.5 (March 24, 2020)
 
 SECURITY:

client/allocrunner/taskrunner/getter/getter.go

@@ -1,13 +1,14 @@
 package getter
 
 import (
+	"errors"
 	"fmt"
 	"net/url"
-	"path/filepath"
 	"strings"
 	"sync"
 
 	gg "github.com/hashicorp/go-getter"
+	"github.com/hashicorp/nomad/helper"
 	"github.com/hashicorp/nomad/nomad/structs"
 )
 
@@ -96,8 +97,12 @@ func GetArtifact(taskEnv EnvReplacer, artifact *structs.TaskArtifact, taskDir st
 		return newGetError(artifact.GetterSource, err, false)
 	}
 
-	// Download the artifact
-	dest := filepath.Join(taskDir, artifact.RelativeDest)
+	// Verify the destination is still in the task sandbox after interpolation
+	dest, err := helper.GetPathInSandbox(taskDir, artifact.RelativeDest)
+	if err != nil {
+		return newGetError(artifact.RelativeDest,
+			errors.New("artifact destination path escapes the alloc directory"), false)
+	}
 
 	// Convert from string getter mode to go-getter const
 	mode := gg.ClientModeAny

client/allocrunner/taskrunner/getter/getter_test.go

@@ -93,6 +93,36 @@ func TestGetArtifact_File_RelativeDest(t *testing.T) {
 	}
 }
 
+func TestGetArtifact_File_EscapeDest(t *testing.T) {
+	// Create the test server hosting the file to download
+	ts := httptest.NewServer(http.FileServer(http.Dir(filepath.Dir("./test-fixtures/"))))
+	defer ts.Close()
+
+	// Create a temp directory to download into
+	taskDir, err := ioutil.TempDir("", "nomad-test")
+	if err != nil {
+		t.Fatalf("failed to make temp directory: %v", err)
+	}
+	defer os.RemoveAll(taskDir)
+
+	// Create the artifact
+	file := "test.sh"
+	relative := "../../../../foo/"
+	artifact := &structs.TaskArtifact{
+		GetterSource: fmt.Sprintf("%s/%s", ts.URL, file),
+		GetterOptions: map[string]string{
+			"checksum": "md5:bce963762aa2dbfed13caf492a45fb72",
+		},
+		RelativeDest: relative,
+	}
+
+	// attempt to download the artifact
+	err = GetArtifact(taskEnv, artifact, taskDir)
+	if err == nil || !strings.Contains(err.Error(), "escapes") {
+		t.Fatalf("expected GetArtifact to disallow sandbox escape: %v", err)
+	}
+}
+
 func TestGetGetterUrl_Interpolation(t *testing.T) {
 	// Create the artifact
 	artifact := &structs.TaskArtifact{

client/allocrunner/taskrunner/template/template.go

@@ -28,10 +28,6 @@ const (
 	// consulTemplateSourceName is the source name when using the TaskHooks.
 	consulTemplateSourceName = "Template"
 
-	// hostSrcOption is the Client option that determines whether the template
-	// source may be from the host
-	hostSrcOption = "template.allow_host_source"
-
 	// missingDepEventLimit is the number of missing dependencies that will be
 	// logged before we switch to showing just the number of missing
 	// dependencies.
@@ -546,25 +542,27 @@ func maskProcessEnv(env map[string]string) map[string]string {
 // parseTemplateConfigs converts the tasks templates in the config into
 // consul-templates
 func parseTemplateConfigs(config *TaskTemplateManagerConfig) (map[*ctconf.TemplateConfig]*structs.Template, error) {
-	allowAbs := config.ClientConfig.ReadBoolDefault(hostSrcOption, true)
+	sandboxEnabled := !config.ClientConfig.TemplateConfig.DisableSandbox
 	taskEnv := config.EnvBuilder.Build()
 
 	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 != "" {
-			if filepath.IsAbs(tmpl.SourcePath) {
-				if !allowAbs {
-					return nil, fmt.Errorf("Specifying absolute template paths disallowed by client config: %q", tmpl.SourcePath)
-				}
-
-				src = tmpl.SourcePath
-			} else {
-				src = filepath.Join(config.TaskDir, taskEnv.ReplaceEnv(tmpl.SourcePath))
+			src = taskEnv.ReplaceEnv(tmpl.SourcePath)
+			src, err = helper.GetPathInSandbox(config.TaskDir, src)
+			if err != nil && sandboxEnabled {
+				return nil, fmt.Errorf("template source path escapes alloc directory")
 			}
 		}
+
 		if tmpl.DestPath != "" {
-			dest = filepath.Join(config.TaskDir, taskEnv.ReplaceEnv(tmpl.DestPath))
+			dest = taskEnv.ReplaceEnv(tmpl.DestPath)
+			dest, err = helper.GetPathInSandbox(config.TaskDir, dest)
+			if err != nil && sandboxEnabled {
+				return nil, fmt.Errorf("template destination path escapes alloc directory")
+			}
 		}
 
 		ct := ctconf.DefaultTemplateConfig()
@@ -574,7 +572,7 @@ func parseTemplateConfigs(config *TaskTemplateManagerConfig) (map[*ctconf.Templa
 		ct.LeftDelim = &tmpl.LeftDelim
 		ct.RightDelim = &tmpl.RightDelim
 		ct.FunctionBlacklist = config.ClientConfig.TemplateConfig.FunctionBlacklist
-		if !config.ClientConfig.TemplateConfig.DisableSandbox {
+		if sandboxEnabled {
 			ct.SandboxPath = &config.TaskDir
 		}
 

client/allocrunner/taskrunner/template/template_test.go

@@ -380,7 +380,11 @@ func TestTaskTemplateManager_HostPath(t *testing.T) {
 	}
 
 	harness := newTestHarness(t, []*structs.Template{template}, false, false)
-	harness.start(t)
+	harness.config.TemplateConfig.DisableSandbox = true
+	err = harness.startWithErr()
+	if err != nil {
+		t.Fatalf("couldn't setup initial harness: %v", err)
+	}
 	defer harness.stop()
 
 	// Wait for the unblock
@@ -403,12 +407,46 @@ func TestTaskTemplateManager_HostPath(t *testing.T) {
 
 	// Change the config to disallow host sources
 	harness = newTestHarness(t, []*structs.Template{template}, false, false)
-	harness.config.Options = map[string]string{
-		hostSrcOption: "false",
+	err = harness.startWithErr()
+	if err == nil || !strings.Contains(err.Error(), "escapes alloc directory") {
+		t.Fatalf("Expected absolute template path disallowed for %q: %v",
+			template.SourcePath, err)
+	}
+
+	template.SourcePath = "../../../../../../" + file
+	harness = newTestHarness(t, []*structs.Template{template}, false, false)
+	err = harness.startWithErr()
+	if err == nil || !strings.Contains(err.Error(), "escapes alloc directory") {
+		t.Fatalf("Expected directory traversal out of %q disallowed for %q: %v",
+			harness.taskDir, template.SourcePath, err)
+	}
+
+	// Build a new task environment
+	a := mock.Alloc()
+	task := a.Job.TaskGroups[0].Tasks[0]
+	task.Name = TestTaskName
+	task.Meta = map[string]string{"ESCAPE": "../"}
+
+	template.SourcePath = "${NOMAD_META_ESCAPE}${NOMAD_META_ESCAPE}${NOMAD_META_ESCAPE}${NOMAD_META_ESCAPE}${NOMAD_META_ESCAPE}${NOMAD_META_ESCAPE}" + file
+	harness = newTestHarness(t, []*structs.Template{template}, false, false)
+	harness.envBuilder = taskenv.NewBuilder(harness.node, a, task, "global")
+	err = harness.startWithErr()
+	if err == nil || !strings.Contains(err.Error(), "escapes alloc directory") {
+		t.Fatalf("Expected directory traversal out of %q via interpolation disallowed for %q: %v",
+			harness.taskDir, template.SourcePath, err)
 	}
-	if err := harness.startWithErr(); err == nil || !strings.Contains(err.Error(), "absolute") {
-		t.Fatalf("Expected absolute template path disallowed: %v", err)
+
+	// Test with desination too
+	template.SourcePath = f.Name()
+	template.DestPath = "../../../../../../" + file
+	harness = newTestHarness(t, []*structs.Template{template}, false, false)
+	harness.envBuilder = taskenv.NewBuilder(harness.node, a, task, "global")
+	err = harness.startWithErr()
+	if err == nil || !strings.Contains(err.Error(), "escapes alloc directory") {
+		t.Fatalf("Expected directory traversal out of %q via interpolation disallowed for %q: %v",
+			harness.taskDir, template.SourcePath, err)
 	}
+
 }
 
 func TestTaskTemplateManager_Unblock_Static(t *testing.T) {

helper/funcs.go

@@ -3,6 +3,7 @@ package helper
 import (
 	"crypto/sha512"
 	"fmt"
+	"path/filepath"
 	"reflect"
 	"regexp"
 	"strings"
@@ -461,3 +462,22 @@ func RemoveEqualFold(xs *[]string, search string) {
 		}
 	}
 }
+
+// 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)
+	rel, err := filepath.Rel(sandboxDir, path)
+	if err != nil {
+		return path, err
+	}
+	if strings.HasPrefix(rel, "..") {
+		return path, fmt.Errorf("%q escapes sandbox directory", path)
+	}
+	return path, nil
+}