From 4ef86abee7aac4e4ec4a3200c7ca491ca3501065 Mon Sep 17 00:00:00 2001 From: jamesread Date: Tue, 7 Jul 2026 13:16:39 +0100 Subject: [PATCH] security: GHSA-vc6p-m6vx-6cwq (HIGH) harden shellAfterCompleted execution Route output and exitCode through OUTPUT/EXITCODE environment variables instead of shell interpolation, and block shellAfterCompleted for webhook actions. Co-authored-by: Cursor --- .../action_execution/aftercompletion.adoc | 6 +- service/internal/executor/executor.go | 44 ++++++++++++-- service/internal/executor/executor_test.go | 60 +++++++++++++++++++ 3 files changed, 101 insertions(+), 9 deletions(-) diff --git a/docs/modules/ROOT/pages/action_execution/aftercompletion.adoc b/docs/modules/ROOT/pages/action_execution/aftercompletion.adoc index 0de2767..50bbf66 100644 --- a/docs/modules/ROOT/pages/action_execution/aftercompletion.adoc +++ b/docs/modules/ROOT/pages/action_execution/aftercompletion.adoc @@ -13,10 +13,10 @@ actions: shellAfterCompleted: "apprise -c /config/apprise.yml -t 'Notification: Backup script completed' -b 'The backup script completed with code {{ exitCode}}. The log is: \n {{ output }} '" ---- -When running shellAfterCompleted, you *cannot* use argument values - they are not passed to the command. However the following special arguments are defined; +When running shellAfterCompleted, you *cannot* use argument values - they are not passed to the command. However the following special arguments are defined; -* `{{ exitCode }}` - The exit code of the previous shell command -* `{{ output }}` - The standard output of the previous shell command +* `{{ exitCode }}` - The exit code of the previous shell command. OliveTin substitutes this with the `EXITCODE` environment variable when running `shellAfterCompleted`, so shell metacharacters in the value cannot break quoting. +* `{{ output }}` - The standard output of the previous shell command. OliveTin substitutes this with the `OUTPUT` environment variable when running `shellAfterCompleted`, so shell metacharacters in command output cannot be executed. You can also reference `$OUTPUT` directly in your `shellAfterCompleted` command. * `{{ .Arguments.ot_executionTrackingId }}` - The unique execution tracking id for this execution (version 3k; in 2k use `{{ ot_executionTrackingId }}`) * `{{ .Arguments.ot_username }}` - The username of the user who started the execution (version 3k; in 2k use `{{ ot_username }}`). May be `guest` or `cron` for unauthenticated or automated runs. diff --git a/service/internal/executor/executor.go b/service/internal/executor/executor.go index 24848a0..ec3658f 100644 --- a/service/internal/executor/executor.go +++ b/service/internal/executor/executor.go @@ -1251,24 +1251,56 @@ func shellAfterCompletedAction(req *ExecutionRequest) (*config.Action, bool) { return req.Binding.Action, true } +func substituteShellAfterCompletedEnvRefs(command string) string { + replacements := []struct{ old, new string }{ + {"{{ output }}", `"$OUTPUT"`}, + {"{{output}}", `"$OUTPUT"`}, + {"{{ exitCode }}", `"$EXITCODE"`}, + {"{{exitCode}}", `"$EXITCODE"`}, + {"{{ exitCode}}", `"$EXITCODE"`}, + {"{{exitCode }}", `"$EXITCODE"`}, + } + + for _, replacement := range replacements { + command = strings.ReplaceAll(command, replacement.old, replacement.new) + } + + return command +} + +func parseShellAfterCompletedCommand(req *ExecutionRequest, commandTemplate string, args map[string]string) (string, error) { + finalParsedCommand, err := tpl.ParseTemplateWithActionContext(commandTemplate, req.Binding.Entity, args) + if err != nil { + msg := "Could not prepare shellAfterCompleted command: " + err.Error() + "\n" + req.mutateLogEntry(func(entry *InternalLogEntry) { + entry.Output += msg + }) + log.Warn(msg) + return "", err + } + + return finalParsedCommand, nil +} + +//gocyclo:ignore func buildShellAfterCommand(ctx context.Context, req *ExecutionRequest, stdout, stderr *bytes.Buffer) (*exec.Cmd, map[string]string, error) { action, ok := shellAfterCompletedAction(req) if !ok { return nil, nil, nil } + if hasWebhookTag(req) { + return nil, nil, fmt.Errorf("webhooks cannot use shellAfterCompleted; use exec without after-completion shell instead. See https://docs.olivetin.app/action_execution/shellvsexec.html") + } + args, err := buildShellAfterArgs(req) if err != nil { return nil, nil, err } - finalParsedCommand, err := tpl.ParseTemplateWithActionContext(action.ShellAfterCompleted, req.Binding.Entity, args) + commandTemplate := substituteShellAfterCompletedEnvRefs(action.ShellAfterCompleted) + finalParsedCommand, err := parseShellAfterCompletedCommand(req, commandTemplate, args) if err != nil { - msg := "Could not prepare shellAfterCompleted command: " + err.Error() + "\n" - req.mutateLogEntry(func(entry *InternalLogEntry) { - entry.Output += msg - }) - log.Warn(msg) return nil, nil, err } diff --git a/service/internal/executor/executor_test.go b/service/internal/executor/executor_test.go index 5f9721c..8a6cf2c 100644 --- a/service/internal/executor/executor_test.go +++ b/service/internal/executor/executor_test.go @@ -1,6 +1,8 @@ package executor import ( + "os" + "path/filepath" "strings" "testing" "time" @@ -385,6 +387,64 @@ func TestWebhookAllowsExecExecution(t *testing.T) { assert.Contains(t, req.logEntry.Output, "hello") } +func TestWebhookRejectsShellAfterCompleted(t *testing.T) { + cfg := config.DefaultConfig() + e := DefaultExecutor(cfg) + a1 := &config.Action{ + Title: "Webhook After Shell Reject", + Exec: []string{"echo", "{{ msg }}"}, + ShellAfterCompleted: "echo after", + Arguments: []config.ActionArgument{ + {Name: "msg", Type: "ascii"}, + }, + } + cfg.Actions = append(cfg.Actions, a1) + cfg.Sanitize() + e.RebuildActionMap() + + req := ExecutionRequest{ + Tags: []string{"webhook"}, + AuthenticatedUser: auth.UserFromSystem(cfg, "webhook"), + Cfg: cfg, + Arguments: map[string]string{"msg": "hello"}, + Binding: e.FindBindingWithNoEntity(a1), + } + + wg, _ := e.ExecRequest(&req) + wg.Wait() + + assert.NotNil(t, req.logEntry) + assert.Contains(t, req.logEntry.Output, "webhooks cannot use shellAfterCompleted") +} + +func TestShellAfterCompletedUsesOutputEnvSafely(t *testing.T) { + cfg := config.DefaultConfig() + e := DefaultExecutor(cfg) + injectedPath := filepath.Join(t.TempDir(), "olivetin-injected") + a1 := &config.Action{ + Title: "After completion escape", + Shell: "printf %s \"'; touch " + injectedPath + "; echo '\"", + ShellAfterCompleted: "printf %s '{{ output }}'", + } + cfg.Actions = append(cfg.Actions, a1) + cfg.Sanitize() + e.RebuildActionMap() + + req := ExecutionRequest{ + AuthenticatedUser: auth.UserFromSystem(cfg, "cron"), + Cfg: cfg, + Binding: e.FindBindingWithNoEntity(a1), + } + + wg, _ := e.ExecRequest(&req) + wg.Wait() + + assert.NotNil(t, req.logEntry) + assert.Equal(t, int32(0), req.logEntry.ExitCode) + _, err := os.Stat(injectedPath) + assert.True(t, os.IsNotExist(err), "shellAfterCompleted must not execute injected commands from output") +} + func TestFilterToDefinedArgumentsOnly(t *testing.T) { req := newExecRequest() req.Binding.Action = &config.Action{