From 356ea87e00fdf310ad5d107faadd58dad5f2b079 Mon Sep 17 00:00:00 2001 From: Piotr Kazmierczak <470696+pkazmierczak@users.noreply.github.com> Date: Fri, 28 Jun 2024 15:16:27 +0000 Subject: [PATCH] template: disable sandboxed rendering on Windows (#23432) Following #23443, we no longer need to sandbox template rendering on Windows. --- .changelog/23432.txt | 3 + .../taskrunner/template/template.go | 97 ----------- .../taskrunner/template/template_default.go | 100 ++++++++++- .../taskrunner/template/template_windows.go | 159 +----------------- .../template/template_windows_test.go | 103 ------------ 5 files changed, 99 insertions(+), 363 deletions(-) create mode 100644 .changelog/23432.txt delete mode 100644 client/allocrunner/taskrunner/template/template_windows_test.go diff --git a/.changelog/23432.txt b/.changelog/23432.txt new file mode 100644 index 000000000..2a7225352 --- /dev/null +++ b/.changelog/23432.txt @@ -0,0 +1,3 @@ +```release-note:bug +template: Fix template rendering on Windows +``` diff --git a/client/allocrunner/taskrunner/template/template.go b/client/allocrunner/taskrunner/template/template.go index 85dbdc9a5..a5cd317f9 100644 --- a/client/allocrunner/taskrunner/template/template.go +++ b/client/allocrunner/taskrunner/template/template.go @@ -7,7 +7,6 @@ import ( "context" "errors" "fmt" - "log" "math/rand" "os" "path/filepath" @@ -19,17 +18,14 @@ import ( ctconf "github.com/hashicorp/consul-template/config" "github.com/hashicorp/consul-template/manager" - "github.com/hashicorp/consul-template/renderer" "github.com/hashicorp/consul-template/signals" envparse "github.com/hashicorp/go-envparse" "github.com/hashicorp/go-hclog" multierror "github.com/hashicorp/go-multierror" "github.com/hashicorp/nomad/client/allocrunner/taskrunner/interfaces" - trenderer "github.com/hashicorp/nomad/client/allocrunner/taskrunner/template/renderer" "github.com/hashicorp/nomad/client/config" "github.com/hashicorp/nomad/client/taskenv" "github.com/hashicorp/nomad/helper/pointer" - "github.com/hashicorp/nomad/helper/subproc" "github.com/hashicorp/nomad/nomad/structs" structsc "github.com/hashicorp/nomad/nomad/structs/config" ) @@ -194,13 +190,6 @@ func NewTaskTemplateManager(config *TaskTemplateManagerConfig) (*TaskTemplateMan tm.signals[tmpl.ChangeSignal] = sig } - // the platform sandbox needs to be created before we construct the runner - // so that reading the template is sandboxed - err := createPlatformSandbox(config) - if err != nil { - return nil, err - } - // Build the consul-template runner runner, lookup, err := templateRunner(config) if err != nil { @@ -229,8 +218,6 @@ func (tm *TaskTemplateManager) Stop() { if tm.runner != nil { tm.runner.Stop() } - - destroyPlatformSandbox(tm.config) } // SetDriverHandle sets the executor @@ -1014,90 +1001,6 @@ type sandboxConfig struct { contents []byte } -func ReaderFn(taskID, taskDir string, sandboxEnabled bool) func(string) ([]byte, error) { - if !sandboxEnabled { - return nil - } - thisBin := subproc.Self() - - return func(src string) ([]byte, error) { - - sandboxCfg := &sandboxConfig{ - thisBin: thisBin, - sandboxPath: taskDir, - sourcePath: src, - taskID: taskID, - } - - stdout, stderr, code, err := readTemplateFromSandbox(sandboxCfg) - if err != nil && code != 0 { - return nil, fmt.Errorf("%v: %s", err, string(stderr)) - } - - // this will get wrapped in CT log formatter - fmt.Fprintf(os.Stderr, "[DEBUG] %s", string(stderr)) - return stdout, nil - } -} - -func RenderFn(taskID, taskDir string, sandboxEnabled bool) func(*renderer.RenderInput) (*renderer.RenderResult, error) { - if !sandboxEnabled { - return nil - } - thisBin := subproc.Self() - - return func(i *renderer.RenderInput) (*renderer.RenderResult, error) { - wouldRender := false - didRender := false - - sandboxCfg := &sandboxConfig{ - thisBin: thisBin, - sandboxPath: taskDir, - destPath: i.Path, - perms: strconv.FormatUint(uint64(i.Perms), 8), - user: i.User, - group: i.Group, - taskID: taskID, - contents: i.Contents, - } - - logs, code, err := renderTemplateInSandbox(sandboxCfg) - if err != nil { - if len(logs) > 0 { - log.Printf("[ERROR] %v: %s", err, logs) - } else { - log.Printf("[ERROR] %v", err) - } - return &renderer.RenderResult{ - DidRender: false, - WouldRender: false, - Contents: []byte{}, - }, fmt.Errorf("template render subprocess failed: %w", err) - } - if code == trenderer.ExitWouldRenderButDidnt { - didRender = false - wouldRender = true - } else { - didRender = true - wouldRender = true - } - - // the subprocess emits logs matching the consul-template runner, but we - // CT doesn't support hclog, so we just print the whole output here to - // stderr the same way CT does so the results look seamless - if len(logs) > 0 { - log.Printf("[DEBUG] %s", logs) - } - - result := &renderer.RenderResult{ - DidRender: didRender, - WouldRender: wouldRender, - Contents: i.Contents, - } - return result, nil - } -} - // loadTemplateEnv loads task environment variables from all templates. func loadTemplateEnv(tmpls []*structs.Template, taskEnv *taskenv.TaskEnv) (map[string]string, error) { all := make(map[string]string, 50) diff --git a/client/allocrunner/taskrunner/template/template_default.go b/client/allocrunner/taskrunner/template/template_default.go index 8b10db0a2..016840adf 100644 --- a/client/allocrunner/taskrunner/template/template_default.go +++ b/client/allocrunner/taskrunner/template/template_default.go @@ -8,19 +8,19 @@ package template import ( "bytes" "context" + "fmt" "io" + "log" + "os" "os/exec" + "strconv" "time" - "github.com/hashicorp/nomad/client/allocrunner/taskrunner/template/renderer" + "github.com/hashicorp/consul-template/renderer" + trenderer "github.com/hashicorp/nomad/client/allocrunner/taskrunner/template/renderer" + "github.com/hashicorp/nomad/helper/subproc" ) -// createPlatformSandbox is a no-op outside of windows -func createPlatformSandbox(_ *TaskTemplateManagerConfig) error { return nil } - -// destroyPlatformSandbox is a no-op outside of windows -func destroyPlatformSandbox(_ *TaskTemplateManagerConfig) error { return nil } - // renderTemplateInSandbox runs the template-render command in a subprocess that // will chroot itself to prevent a task from swapping a directory between the // sandbox path and the destination with a symlink pointing to somewhere outside @@ -65,7 +65,7 @@ func renderTemplateInSandbox(cfg *sandboxConfig) (string, int, error) { out, err := cmd.CombinedOutput() code := cmd.ProcessState.ExitCode() - if code == renderer.ExitWouldRenderButDidnt { + if code == trenderer.ExitWouldRenderButDidnt { err = nil // erase the "exit code 117" error } @@ -104,3 +104,87 @@ func readTemplateFromSandbox(cfg *sandboxConfig) ([]byte, []byte, int, error) { stderr := errb.Bytes() return stdout, stderr, cmd.ProcessState.ExitCode(), err } + +func RenderFn(taskID, taskDir string, sandboxEnabled bool) func(*renderer.RenderInput) (*renderer.RenderResult, error) { + if !sandboxEnabled { + return nil + } + thisBin := subproc.Self() + + return func(i *renderer.RenderInput) (*renderer.RenderResult, error) { + wouldRender := false + didRender := false + + sandboxCfg := &sandboxConfig{ + thisBin: thisBin, + sandboxPath: taskDir, + destPath: i.Path, + perms: strconv.FormatUint(uint64(i.Perms), 8), + user: i.User, + group: i.Group, + taskID: taskID, + contents: i.Contents, + } + + logs, code, err := renderTemplateInSandbox(sandboxCfg) + if err != nil { + if len(logs) > 0 { + log.Printf("[ERROR] %v: %s", err, logs) + } else { + log.Printf("[ERROR] %v", err) + } + return &renderer.RenderResult{ + DidRender: false, + WouldRender: false, + Contents: []byte{}, + }, fmt.Errorf("template render subprocess failed: %w", err) + } + if code == trenderer.ExitWouldRenderButDidnt { + didRender = false + wouldRender = true + } else { + didRender = true + wouldRender = true + } + + // the subprocess emits logs matching the consul-template runner, but we + // CT doesn't support hclog, so we just print the whole output here to + // stderr the same way CT does so the results look seamless + if len(logs) > 0 { + log.Printf("[DEBUG] %s", logs) + } + + result := &renderer.RenderResult{ + DidRender: didRender, + WouldRender: wouldRender, + Contents: i.Contents, + } + return result, nil + } +} + +func ReaderFn(taskID, taskDir string, sandboxEnabled bool) func(string) ([]byte, error) { + if !sandboxEnabled { + return nil + } + thisBin := subproc.Self() + + return func(src string) ([]byte, error) { + + sandboxCfg := &sandboxConfig{ + thisBin: thisBin, + sandboxPath: taskDir, + sourcePath: src, + taskID: taskID, + } + + stdout, stderr, code, err := readTemplateFromSandbox(sandboxCfg) + if err != nil && code != 0 { + return nil, fmt.Errorf("%v: %s", err, string(stderr)) + } + + // this will get wrapped in CT log formatter + fmt.Fprintf(os.Stderr, "[DEBUG] %s", string(stderr)) + return stdout, nil + } +} diff --git a/client/allocrunner/taskrunner/template/template_windows.go b/client/allocrunner/taskrunner/template/template_windows.go index c84fddd90..78e389fa5 100644 --- a/client/allocrunner/taskrunner/template/template_windows.go +++ b/client/allocrunner/taskrunner/template/template_windows.go @@ -6,164 +6,13 @@ package template import ( - "bytes" - "context" - "errors" - "fmt" - "io" - "path/filepath" - "time" - - "github.com/hashicorp/go-hclog" - "github.com/hashicorp/nomad/client/allocrunner/taskrunner/template/renderer" - "github.com/hashicorp/nomad/helper/subproc" - "github.com/hashicorp/nomad/helper/winappcontainer" - "github.com/hashicorp/nomad/helper/winexec" + "github.com/hashicorp/consul-template/renderer" ) -const ExitCodeFatal int = 13 // typically this is going to be a bug in Nomad - -// createPlatformSandbox creates the AppContainer profile and sets DACLs on the -// files we want to grant access to. -func createPlatformSandbox(cfg *TaskTemplateManagerConfig) error { - - if !isSandboxEnabled(cfg) { - return nil - } - thisBin := subproc.Self() - - containerCfg := &winappcontainer.AppContainerConfig{ - Name: cfg.TaskID, - AllowedPaths: []string{ - thisBin, - filepath.Dir(cfg.TaskDir), // give access to the whole alloc working directory - }, - } - if cfg.Logger == nil { - cfg.Logger = hclog.Default() // prevents panics in tests - } - - err := winappcontainer.CreateAppContainer(cfg.Logger, containerCfg) - if err != nil { - // if Nomad is running as an unprivileged user, we might not be able to - // create the sandbox, but in that case we're not vulnerable to the - // attacks this is intended to prevent anyways - if errors.Is(err, winappcontainer.ErrAccessDeniedToCreateSandbox) { - cfg.Logger.Debug("could not create platform sandbox", "error", err) - return nil - } - return fmt.Errorf("could not create platform sandbox: %w", err) - } - +func RenderFn(taskID, taskDir string, sandboxEnabled bool) func(*renderer.RenderInput) (*renderer.RenderResult, error) { return nil } -// destroyPlatformSandbox deletes the AppContainer profile. -func destroyPlatformSandbox(cfg *TaskTemplateManagerConfig) error { - - if cfg.ClientConfig.TemplateConfig.DisableSandbox { - return nil - } - - if cfg.Logger == nil { - cfg.Logger = hclog.Default() - } - - err := winappcontainer.DeleteAppContainer(cfg.Logger, cfg.TaskID) - if err != nil { - cfg.Logger.Warn("could not destroy platform sandbox", "error", err) - } - return err -} - -// renderTemplateInSandbox runs the template-render command in an AppContainer to -// prevent a task from swapping a directory between the sandbox path and the -// destination with a symlink pointing to somewhere outside the sandbox. -// -// See renderer/ subdirectory for implementation. -func renderTemplateInSandbox(cfg *sandboxConfig) (string, int, error) { - procThreadAttrs, cleanup, err := winappcontainer.CreateProcThreadAttributes(cfg.taskID) - if err != nil { - return "", ExitCodeFatal, fmt.Errorf("could not create proc attributes: %v", err) - } - defer cleanup() - - // Safe to inject user input as command arguments since winexec.Command - // does not invoke a shell. - args := []string{ - "template-render", - "write", - "-sandbox-path", cfg.sandboxPath, - "-dest-path", cfg.destPath, - "-perms", cfg.perms, - } - if cfg.user != "" { - args = append(args, "-user") - args = append(args, cfg.user) - } - if cfg.group != "" { - args = append(args, "-group") - args = append(args, cfg.group) - } - - ctx, cancel := context.WithTimeout(context.Background(), 60*time.Second) - defer cancel() - - cmd := winexec.CommandContext(ctx, cfg.thisBin, args...) - cmd.ProcThreadAttributes = procThreadAttrs - - stdin, err := cmd.StdinPipe() - if err != nil { - return "", 1, err - } - - go func() { - defer stdin.Close() - io.Copy(stdin, bytes.NewReader(cfg.contents)) - }() - - out, err := cmd.CombinedOutput() - code := cmd.ProcessState.ExitCode() - if code == renderer.ExitWouldRenderButDidnt { - err = nil // erase the "exit code 117" error - } - - return string(out), code, err -} - -// readTemplateFromSandbox runs the template-render command in a subprocess that -// will chroot itself to prevent a task from swapping a directory between the -// sandbox path and the source with a symlink pointing to somewhere outside -// the sandbox. -func readTemplateFromSandbox(cfg *sandboxConfig) ([]byte, []byte, int, error) { - procThreadAttrs, cleanup, err := winappcontainer.CreateProcThreadAttributes(cfg.taskID) - if err != nil { - return nil, nil, ExitCodeFatal, fmt.Errorf("could not create proc attributes: %v", err) - } - defer cleanup() - - // Safe to inject user input as command arguments since winexec.Command - // does not invoke a shell. Also, the only user-controlled argument here is - // the source path which we've already verified is at least a valid path in - // the caller. - args := []string{ - "template-render", - "read", - "-sandbox-path", cfg.sandboxPath, - "-source-path", cfg.sourcePath, - } - - ctx, cancel := context.WithTimeout(context.Background(), 60*time.Second) - defer cancel() - - cmd := winexec.CommandContext(ctx, cfg.thisBin, args...) - cmd.ProcThreadAttributes = procThreadAttrs - var outb, errb bytes.Buffer - cmd.Stdout = &outb - cmd.Stderr = &errb - - err = cmd.Run() - stdout := outb.Bytes() - stderr := errb.Bytes() - return stdout, stderr, cmd.ProcessState.ExitCode(), err +func ReaderFn(taskID, taskDir string, sandboxEnabled bool) func(string) ([]byte, error) { + return nil } diff --git a/client/allocrunner/taskrunner/template/template_windows_test.go b/client/allocrunner/taskrunner/template/template_windows_test.go deleted file mode 100644 index 4a1b14c6d..000000000 --- a/client/allocrunner/taskrunner/template/template_windows_test.go +++ /dev/null @@ -1,103 +0,0 @@ -// Copyright (c) HashiCorp, Inc. -// SPDX-License-Identifier: BUSL-1.1 - -//go:build windows - -package template - -import ( - "fmt" - "os" - "path/filepath" - "testing" - - "github.com/hashicorp/nomad/ci" - "github.com/hashicorp/nomad/client/taskenv" - clienttestutil "github.com/hashicorp/nomad/client/testutil" - "github.com/hashicorp/nomad/nomad/mock" - "github.com/hashicorp/nomad/nomad/structs" - "github.com/shoenig/test/must" -) - -// TestTaskTemplateManager_SymlinkEscapeSource verifies that a malicious or -// compromised task cannot use a symlink parent directory to cause reads to -// escape the sandbox -func TestTaskTemplateManager_SymlinkEscapeSource(t *testing.T) { - ci.Parallel(t) - clienttestutil.RequireAdministrator(t) // making symlinks is privileged on Windows - - // Create a set of "sensitive" files outside the task dir that the task - // should not be able to read or write to, despite filesystem permissions - sensitiveDir := t.TempDir() - sensitiveFile := filepath.Join(sensitiveDir, "sensitive.txt") - os.WriteFile(sensitiveFile, []byte("very-secret-stuff"), 0755) - - a := mock.Alloc() - task := a.Job.TaskGroups[0].Tasks[0] - task.Name = TestTaskName - template := &structs.Template{ChangeMode: structs.TemplateChangeModeNoop} - - // Build a new task environment with a valid DestPath - harness := newTestHarness(t, []*structs.Template{template}, false, false) - harness.envBuilder = taskenv.NewBuilder(harness.node, a, task, "global") - harness.envBuilder.SetClientTaskRoot(harness.taskDir) - os.MkdirAll(filepath.Join(harness.taskDir, "local"), 0755) - harness.templates[0].DestPath = filepath.Join("local", "dest.tmpl") - - // "Attack" the SourcePath by creating a symlink from the sensitive file to - // the task dir; this simulates what happens when the client restarts and - // the task attacks while the client is down, which is the easiest case to - // reproduce - must.NoError(t, os.Symlink(sensitiveDir, filepath.Join(harness.taskDir, "local", "pwned"))) - harness.templates[0].SourcePath = filepath.Join("local", "pwned", "sensitive.txt") - fullSrcPath := filepath.Join(harness.taskDir, harness.templates[0].SourcePath) - - err := harness.startWithErr() - t.Cleanup(harness.stop) - - must.EqError(t, err, fmt.Sprintf( - "failed to read template: failed to open source file %q: open %s: Access is denied.\n", fullSrcPath, fullSrcPath)) -} - -// TestTaskTemplateManager_SymlinkEscapeDest verifies that a malicious or -// compromised task cannot use a symlink parent directory to cause writes to -// escape the sandbox -func TestTaskTemplateManager_SymlinkEscapeDest(t *testing.T) { - ci.Parallel(t) - clienttestutil.RequireAdministrator(t) // making symlinks is privileged on Windows - - // Create a set of "sensitive" files outside the task dir that the task - // should not be able to read or write to, despite filesystem permissions - sensitiveDir := t.TempDir() - sensitiveFile := filepath.Join(sensitiveDir, "sensitive.txt") - os.WriteFile(sensitiveFile, []byte("very-secret-stuff"), 0755) - - a := mock.Alloc() - task := a.Job.TaskGroups[0].Tasks[0] - task.Name = TestTaskName - template := &structs.Template{ChangeMode: structs.TemplateChangeModeNoop} - - // Build a task environment with a valid SourcePath - harness := newTestHarness(t, []*structs.Template{template}, false, false) - harness.envBuilder = taskenv.NewBuilder(harness.node, a, task, "global") - harness.envBuilder.SetClientTaskRoot(harness.taskDir) - os.MkdirAll(filepath.Join(harness.taskDir, "local"), 0755) - - harness.templates[0].SourcePath = filepath.Join("local", "source.tmpl") - must.NoError(t, os.WriteFile( - filepath.Join(harness.taskDir, harness.templates[0].SourcePath), - []byte("hacked!"), 0755)) - - // "Attack" the DestPath by creating a symlink from the sensitive file to - // the task dir - must.NoError(t, os.Symlink(sensitiveDir, filepath.Join(harness.taskDir, "local", "pwned"))) - harness.templates[0].DestPath = filepath.Join("local", "pwned", "sensitive.txt") - - err := harness.startWithErr() - t.Cleanup(harness.stop) - - // Ensure we haven't written despite the error - b, err := os.ReadFile(sensitiveFile) - must.NoError(t, err) - must.Eq(t, "very-secret-stuff", string(b)) -}