From ebffd9f040f791208aee1db2e5a8aecd1e3e603d Mon Sep 17 00:00:00 2001 From: jamesread Date: Sun, 31 May 2026 22:41:16 +0100 Subject: [PATCH] security: GHSA-prj9-97mp-mwh2 (HIGH) Treat all ot_ system arguments as reserved, preventing RCE Ensure OliveTin-owned arguments are injected only after filtering and validate system values before command execution. --- docs/modules/ROOT/pages/args/types.adoc | 1 + frontend/resources/vue/views/ArgumentForm.vue | 2 +- service/internal/config/config.go | 3 + service/internal/config/sanitize.go | 28 +++ service/internal/config/sanitize_test.go | 53 +++++ service/internal/executor/arguments.go | 1 + service/internal/executor/arguments_test.go | 32 +++ service/internal/executor/executor.go | 102 ++++++--- service/internal/executor/executor_test.go | 210 +++++++++++++++++- 9 files changed, 402 insertions(+), 30 deletions(-) diff --git a/docs/modules/ROOT/pages/args/types.adoc b/docs/modules/ROOT/pages/args/types.adoc index fa10b99..ffdfed0 100644 --- a/docs/modules/ROOT/pages/args/types.adoc +++ b/docs/modules/ROOT/pages/args/types.adoc @@ -10,6 +10,7 @@ A full list of argument types are below; | (default) | xref:args/input.adoc[Textbox] | If a `type:` is not set, and `choices:` is empty, then ascii will be used, and a warning will be logged. It is recommended that you set the type explicitly, rather than relying on defaults. | ascii | xref:args/input.adoc[Textbox] | a-z (case insensitive), 0-9, but no spaces or punctuation | ascii_identifier | xref:args/input.adoc[Textbox] | Like a DNS name, a-Z (case insensitive), 0-9, `-`, `.`, and `_`. +| shell_safe_identifier | xref:args/input.adoc[Textbox] | Like an ascii identifier, but also allows `@` and `+`. Useful for shell-safe usernames and email-style identifiers. | ascii_sentence | xref:args/input.adoc[Textbox] | a-z (case insensitive), 0-9, with spaces, `.` and `,`. | unicode_identifier | xref:args/input.adoc[Textbox] | Like an ascii identifier, but allows unicode characters. This is useful for languages that use non-ascii characters, such as Chinese, Japanese, etc. | email | xref:args/input.adoc[Textbox] | An email address. diff --git a/frontend/resources/vue/views/ArgumentForm.vue b/frontend/resources/vue/views/ArgumentForm.vue index 0ba61ce..925c052 100644 --- a/frontend/resources/vue/views/ArgumentForm.vue +++ b/frontend/resources/vue/views/ArgumentForm.vue @@ -174,7 +174,7 @@ function getInputType(arg) { return 'checkbox' } - if (arg.type === 'ascii_identifier' || arg.type === 'ascii' || arg.type === 'ascii_sentence') { + if (arg.type === 'ascii_identifier' || arg.type === 'shell_safe_identifier' || arg.type === 'ascii' || arg.type === 'ascii_sentence') { return 'text' } diff --git a/service/internal/config/config.go b/service/internal/config/config.go index 9bc6077..b118afa 100644 --- a/service/internal/config/config.go +++ b/service/internal/config/config.go @@ -4,6 +4,9 @@ import ( "fmt" ) +// ReservedArgumentNamePrefix is reserved for OliveTin-injected system arguments. +const ReservedArgumentNamePrefix = "ot_" + // Action represents the core functionality of OliveTin - commands that show up // as buttons in the UI. type Action struct { diff --git a/service/internal/config/sanitize.go b/service/internal/config/sanitize.go index 674fba5..d22cc90 100644 --- a/service/internal/config/sanitize.go +++ b/service/internal/config/sanitize.go @@ -26,6 +26,34 @@ func (cfg *Config) Sanitize() { } cfg.sanitizeDashboardsForInlineActions() + + if err := cfg.validateReservedActionArgumentNames(); err != nil { + log.Fatalf("%v", err) + } +} + +func (cfg *Config) validateReservedActionArgumentNames() error { + for _, action := range cfg.Actions { + if err := action.validateReservedArgumentNames(); err != nil { + return err + } + } + + return nil +} + +func (action *Action) validateReservedArgumentNames() error { + if action == nil { + return nil + } + + for _, arg := range action.Arguments { + if strings.HasPrefix(arg.Name, ReservedArgumentNamePrefix) { + return fmt.Errorf("action %q argument %q uses reserved prefix %q", action.Title, arg.Name, ReservedArgumentNamePrefix) + } + } + + return nil } func (cfg *Config) sanitizeDashboardsForInlineActions() { diff --git a/service/internal/config/sanitize_test.go b/service/internal/config/sanitize_test.go index adba106..036d710 100644 --- a/service/internal/config/sanitize_test.go +++ b/service/internal/config/sanitize_test.go @@ -92,6 +92,59 @@ func TestSanitizeConfigInlineDashboardActions(t *testing.T) { } } +func TestValidateReservedActionArgumentNames(t *testing.T) { + c := DefaultConfig() + c.Actions = append(c.Actions, &Action{ + Title: "Reserved arg", + Arguments: []ActionArgument{ + {Name: "ot_custom", Type: "ascii"}, + }, + }) + + err := c.validateReservedActionArgumentNames() + + require.Error(t, err) + assert.Contains(t, err.Error(), `action "Reserved arg" argument "ot_custom" uses reserved prefix "ot_"`) +} + +func TestValidateReservedActionArgumentNamesAllowsNonReserved(t *testing.T) { + c := DefaultConfig() + c.Actions = append(c.Actions, &Action{ + Title: "Allowed arg", + Arguments: []ActionArgument{ + {Name: "target", Type: "ascii"}, + }, + }) + + require.NoError(t, c.validateReservedActionArgumentNames()) +} + +func TestValidateReservedActionArgumentNamesChecksInlineActions(t *testing.T) { + c := DefaultConfig() + c.Dashboards = []*DashboardComponent{ + { + Title: "Dashboard", + Contents: []*DashboardComponent{ + { + Title: "Inline reserved arg", + InlineAction: &Action{ + Shell: "echo test", + Arguments: []ActionArgument{ + {Name: "ot_custom", Type: "ascii"}, + }, + }, + }, + }, + }, + } + + c.sanitizeDashboardsForInlineActions() + err := c.validateReservedActionArgumentNames() + + require.Error(t, err) + assert.Contains(t, err.Error(), `action "Inline reserved arg" argument "ot_custom" uses reserved prefix "ot_"`) +} + func TestValidateUniqueLocalUserAPIKeys(t *testing.T) { t.Parallel() diff --git a/service/internal/executor/arguments.go b/service/internal/executor/arguments.go index a73a0d7..e499134 100644 --- a/service/internal/executor/arguments.go +++ b/service/internal/executor/arguments.go @@ -21,6 +21,7 @@ var ( "unicode_identifier": `^[\w\-\.\_\d]+$`, "ascii": `^[a-zA-Z0-9]+$`, "ascii_identifier": `^[a-zA-Z0-9\-\._]+$`, + "shell_safe_identifier": `^[a-zA-Z0-9@\.\_\+\-]+$`, "ascii_sentence": `^[a-zA-Z0-9\-\._, ]+$`, } ) diff --git a/service/internal/executor/arguments_test.go b/service/internal/executor/arguments_test.go index 8cf03a9..7d409f1 100644 --- a/service/internal/executor/arguments_test.go +++ b/service/internal/executor/arguments_test.go @@ -576,6 +576,38 @@ func TestTypeSafetyCheckAsciiIdentifier(t *testing.T) { } } +func TestTypeSafetyCheckShellSafeIdentifier(t *testing.T) { + tests := []struct { + name string + value string + hasError bool + }{ + {"Simple username", "alice123", false}, + {"Email username", "alice@example.com", false}, + {"Plus addressing", "alice+test@example.com", false}, + {"Hyphen underscore dot", "alice-test_user.example", false}, + {"Invalid space", "alice example", true}, + {"Invalid shell substitution", "$(whoami)", true}, + {"Invalid backtick", "`whoami`", true}, + {"Invalid semicolon", "alice;id", true}, + {"Invalid ampersand", "alice&id", true}, + {"Invalid pipe", "alice|id", true}, + {"Invalid quote", "alice'example", true}, + {"Invalid slash", "alice/example", true}, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + err := TypeSafetyCheck("username", tt.value, "shell_safe_identifier") + if tt.hasError { + assert.NotNil(t, err, "Expected error for value '%s'", tt.value) + } else { + assert.Nil(t, err, "Expected no error for value '%s', but got: %v", tt.value, err) + } + }) + } +} + func TestTypeSafetyCheckAsciiSentence(t *testing.T) { tests := []struct { name string diff --git a/service/internal/executor/executor.go b/service/internal/executor/executor.go index 670aeea..fbcaa68 100644 --- a/service/internal/executor/executor.go +++ b/service/internal/executor/executor.go @@ -666,13 +666,15 @@ func stepACLCheck(req *ExecutionRequest) bool { func stepParseArgs(req *ExecutionRequest) bool { ensureArgumentMap(req) - injectSystemArgs(req) if !hasBindingAndAction(req) { return fail(req, fmt.Errorf("cannot parse arguments: Binding or Action is nil")) } filterToDefinedArgumentsOnly(req) + if err := injectSystemArgs(req); err != nil { + return fail(req, err) + } mangleInvalidArgumentValues(req) if hasExec(req) { @@ -735,7 +737,7 @@ func filterToDefinedArgumentsOnly(req *ExecutionRequest) { func keepArgument(name string, definedNames map[string]struct{}) bool { _, ok := definedNames[name] - return ok || strings.HasPrefix(name, "ot_") + return ok } func hasWebhookTag(req *ExecutionRequest) bool { @@ -747,9 +749,38 @@ func hasWebhookTag(req *ExecutionRequest) bool { return false } -func injectSystemArgs(req *ExecutionRequest) { - req.Arguments["ot_executionTrackingId"] = req.TrackingID - req.Arguments["ot_username"] = req.AuthenticatedUser.Username +var systemArgumentDefinitions = []config.ActionArgument{ + {Name: "ot_executionTrackingId", Type: "ascii_identifier", RejectNull: true}, + {Name: "ot_username", Type: "shell_safe_identifier", RejectNull: true}, +} + +func injectSystemArgs(req *ExecutionRequest) error { + args, err := validatedSystemArgs(req) + if err != nil { + return err + } + + for name, value := range args { + req.Arguments[name] = value + } + + return nil +} + +func validatedSystemArgs(req *ExecutionRequest) (map[string]string, error) { + values := map[string]string{ + "ot_executionTrackingId": req.TrackingID, + "ot_username": req.AuthenticatedUser.Username, + } + + for i := range systemArgumentDefinitions { + arg := &systemArgumentDefinitions[i] + if err := ValidateArgument(arg, values[arg.Name], req.Binding.Action); err != nil { + return nil, fmt.Errorf("system argument %q failed validation: %w", arg.Name, err) + } + } + + return values, nil } func hasBindingAndAction(req *ExecutionRequest) bool { @@ -939,36 +970,20 @@ func prepareCommand(cmd *exec.Cmd, streamer *OutputStreamer, req *ExecutionReque } func stepExecAfter(req *ExecutionRequest) bool { - if req.Binding.Action.ShellAfterCompleted == "" { - return true - } - ctx, cancel := newTimeoutContext(context.Background(), time.Duration(req.Binding.Action.Timeout)*time.Second, req.executor) defer cancel() var stdout bytes.Buffer var stderr bytes.Buffer - args := map[string]string{ - "output": req.logEntry.Output, - "exitCode": fmt.Sprintf("%v", req.logEntry.ExitCode), - "ot_executionTrackingId": req.TrackingID, - "ot_username": req.AuthenticatedUser.Username, - } - - finalParsedCommand, err := tpl.ParseTemplateWithActionContext(req.Binding.Action.ShellAfterCompleted, req.Binding.Entity, args) - + cmd, args, err := buildShellAfterCommand(ctx, req, &stdout, &stderr) if err != nil { - msg := "Could not prepare shellAfterCompleted command: " + err.Error() + "\n" - req.logEntry.Output += msg - log.Warn(msg) + return fail(req, err) + } + if cmd == nil { return true } - cmd := wrapCommandInShell(ctx, finalParsedCommand) - cmd.Stdout = &stdout - cmd.Stderr = &stderr - cmd.Env = buildEnv(args) runerr := cmd.Start() @@ -998,6 +1013,43 @@ func stepExecAfter(req *ExecutionRequest) bool { return true } +func buildShellAfterCommand(ctx context.Context, req *ExecutionRequest, stdout, stderr *bytes.Buffer) (*exec.Cmd, map[string]string, error) { + if req.Binding.Action.ShellAfterCompleted == "" { + return nil, nil, nil + } + + args, err := buildShellAfterArgs(req) + if err != nil { + return nil, nil, err + } + + finalParsedCommand, err := tpl.ParseTemplateWithActionContext(req.Binding.Action.ShellAfterCompleted, req.Binding.Entity, args) + if err != nil { + msg := "Could not prepare shellAfterCompleted command: " + err.Error() + "\n" + req.logEntry.Output += msg + log.Warn(msg) + return nil, nil, nil + } + + cmd := wrapCommandInShell(ctx, finalParsedCommand) + cmd.Stdout = stdout + cmd.Stderr = stderr + + return cmd, args, nil +} + +func buildShellAfterArgs(req *ExecutionRequest) (map[string]string, error) { + args, err := validatedSystemArgs(req) + if err != nil { + return nil, err + } + + args["output"] = req.logEntry.Output + args["exitCode"] = fmt.Sprintf("%v", req.logEntry.ExitCode) + + return args, nil +} + //gocyclo:ignore func stepTrigger(req *ExecutionRequest) bool { if req.Binding.Action.Triggers == nil { diff --git a/service/internal/executor/executor_test.go b/service/internal/executor/executor_test.go index a36fc10..d79f014 100644 --- a/service/internal/executor/executor_test.go +++ b/service/internal/executor/executor_test.go @@ -1,6 +1,7 @@ package executor import ( + "strings" "testing" "time" @@ -37,7 +38,7 @@ func TestCreateExecutorAndExec(t *testing.T) { e, cfg := testingExecutor() req := ExecutionRequest{ - AuthenticatedUser: &authpublic.AuthenticatedUser{Username: "Mr Tickle"}, + AuthenticatedUser: &authpublic.AuthenticatedUser{Username: "MrTickle"}, Cfg: cfg, Arguments: map[string]string{ "person": "yourself", @@ -379,7 +380,7 @@ func TestFilterToDefinedArgumentsOnly(t *testing.T) { assert.Empty(t, req.Arguments["extra_undefined"]) } -func TestFilterToDefinedArgumentsPreservesSystemArgs(t *testing.T) { +func TestFilterToDefinedArgumentsDropsReservedPrefixArgs(t *testing.T) { req := newExecRequest() req.Binding.Action = &config.Action{ Title: "Filter test", @@ -393,8 +394,209 @@ func TestFilterToDefinedArgumentsPreservesSystemArgs(t *testing.T) { filterToDefinedArgumentsOnly(req) - assert.Equal(t, "track-123", req.Arguments["ot_executionTrackingId"]) - assert.Equal(t, "webhook", req.Arguments["ot_username"]) + assert.Empty(t, req.Arguments["ot_executionTrackingId"]) + assert.Empty(t, req.Arguments["ot_username"]) +} + +func TestStepParseArgsInjectsSystemArgsAfterFiltering(t *testing.T) { + req := newExecRequest() + req.TrackingID = "server-track-456" + req.AuthenticatedUser = &authpublic.AuthenticatedUser{Username: "alice"} + req.Binding.Action = &config.Action{ + Title: "Filter then inject", + Shell: "echo test", + Arguments: []config.ActionArgument{ + {Name: "name", Type: "ascii"}, + }, + } + req.Arguments = map[string]string{ + "name": "Alice", + "ot_executionTrackingId": "attacker-track", + "ot_username": "mallory", + "ot_custom": "polluted", + } + + assert.True(t, stepParseArgs(req)) + assert.Equal(t, "Alice", req.Arguments["name"]) + assert.Equal(t, "server-track-456", req.Arguments["ot_executionTrackingId"]) + assert.Equal(t, "alice", req.Arguments["ot_username"]) + assert.Empty(t, req.Arguments["ot_custom"]) +} + +func TestStepParseArgsDropsReservedPrefixArgsFromEnvironment(t *testing.T) { + req := newExecRequest() + req.TrackingID = "server-track-456" + req.AuthenticatedUser = &authpublic.AuthenticatedUser{Username: "alice@example.com"} + req.Binding.Action = &config.Action{ + Title: "No reserved prefix pollution", + Shell: "echo test", + Arguments: []config.ActionArgument{}, + } + req.Arguments = map[string]string{ + "ot_custom": "polluted", + } + + assert.True(t, stepParseArgs(req)) + env := buildEnv(req.Arguments) + + assert.False(t, containsEnvPrefix(env, "OT_CUSTOM=")) + assert.True(t, containsEnvPrefix(env, "OT_USERNAME=alice@example.com")) + assert.True(t, containsEnvPrefix(env, "OT_EXECUTIONTRACKINGID=server-track-456")) +} + +func TestSystemArgumentDefinitionsAreReservedAndShellSafe(t *testing.T) { + unsafeTypes := map[string]struct{}{ + "email": {}, + "password": {}, + "raw_string_multiline": {}, + "url": {}, + "very_dangerous_raw_string": {}, + } + seen := map[string]struct{}{} + + for _, arg := range systemArgumentDefinitions { + assert.True(t, strings.HasPrefix(arg.Name, config.ReservedArgumentNamePrefix)) + assert.NotEmpty(t, arg.Type) + assert.True(t, arg.RejectNull) + + _, duplicate := seen[arg.Name] + assert.False(t, duplicate, "duplicate system argument definition %q", arg.Name) + seen[arg.Name] = struct{}{} + + _, unsafe := unsafeTypes[arg.Type] + assert.False(t, unsafe, "system argument %q uses unsafe type %q", arg.Name, arg.Type) + } +} + +func TestValidatedSystemArgsMatchesSystemArgumentDefinitions(t *testing.T) { + req := newExecRequest() + req.TrackingID = "server-track-456" + req.AuthenticatedUser = &authpublic.AuthenticatedUser{Username: "alice@example.com"} + + args, err := validatedSystemArgs(req) + + assert.Nil(t, err) + assert.Len(t, args, len(systemArgumentDefinitions)) + for _, arg := range systemArgumentDefinitions { + assert.Contains(t, args, arg.Name) + } +} + +func TestBuildShellAfterArgsOnlyAddsExpectedNonSystemArgs(t *testing.T) { + req := newExecRequest() + req.logEntry = &InternalLogEntry{ + Output: "hello", + ExitCode: 7, + } + req.TrackingID = "server-track-456" + req.AuthenticatedUser = &authpublic.AuthenticatedUser{Username: "alice@example.com"} + req.Binding.Action = &config.Action{ShellAfterCompleted: "echo test"} + + args, err := buildShellAfterArgs(req) + + assert.Nil(t, err) + assert.Len(t, args, len(systemArgumentDefinitions)+2) + assert.Contains(t, args, "output") + assert.Contains(t, args, "exitCode") + for _, arg := range systemArgumentDefinitions { + assert.Contains(t, args, arg.Name) + } +} + +func TestStepParseArgsAllowsEmailUsernameSystemArg(t *testing.T) { + req := newExecRequest() + req.logEntry = &InternalLogEntry{} + req.TrackingID = "server-track-456" + req.AuthenticatedUser = &authpublic.AuthenticatedUser{Username: "alice@example.com"} + req.Binding.Action = &config.Action{ + Title: "Email username", + Shell: "echo test", + Arguments: []config.ActionArgument{}, + } + + assert.True(t, stepParseArgs(req)) + assert.Equal(t, "alice@example.com", req.Arguments["ot_username"]) +} + +func TestStepParseArgsFailsWhenUsernameSystemArgIsInvalid(t *testing.T) { + req := newExecRequest() + req.logEntry = &InternalLogEntry{} + req.TrackingID = "server-track-456" + req.AuthenticatedUser = &authpublic.AuthenticatedUser{Username: "alice;id"} + req.Binding.Action = &config.Action{ + Title: "Invalid system arg", + Shell: "echo test", + Arguments: []config.ActionArgument{}, + } + + assert.False(t, stepParseArgs(req)) + assert.Contains(t, req.logEntry.Output, `system argument "ot_username" failed validation`) + assert.Empty(t, req.Arguments["ot_username"]) +} + +func TestStepParseArgsFailsWhenTrackingIDSystemArgIsInvalid(t *testing.T) { + req := newExecRequest() + req.logEntry = &InternalLogEntry{} + req.TrackingID = "track/../../bad" + req.AuthenticatedUser = &authpublic.AuthenticatedUser{Username: "alice"} + req.Binding.Action = &config.Action{ + Title: "Invalid tracking ID", + Shell: "echo test", + Arguments: []config.ActionArgument{}, + } + + assert.False(t, stepParseArgs(req)) + assert.Contains(t, req.logEntry.Output, `system argument "ot_executionTrackingId" failed validation`) + assert.Empty(t, req.Arguments["ot_executionTrackingId"]) +} + +func TestBuildShellAfterArgsUsesValidatedSystemArgs(t *testing.T) { + req := newExecRequest() + req.logEntry = &InternalLogEntry{ + Output: "hello", + ExitCode: 7, + } + req.TrackingID = "server-track-456" + req.AuthenticatedUser = &authpublic.AuthenticatedUser{Username: "alice@example.com"} + req.Binding.Action = &config.Action{ + Title: "Shell after", + ShellAfterCompleted: "echo test", + } + + args, err := buildShellAfterArgs(req) + + assert.Nil(t, err) + assert.Equal(t, "alice@example.com", args["ot_username"]) + assert.Equal(t, "server-track-456", args["ot_executionTrackingId"]) + assert.Equal(t, "hello", args["output"]) + assert.Equal(t, "7", args["exitCode"]) +} + +func TestBuildShellAfterArgsFailsWhenSystemArgIsInvalid(t *testing.T) { + req := newExecRequest() + req.logEntry = &InternalLogEntry{} + req.TrackingID = "server-track-456" + req.AuthenticatedUser = &authpublic.AuthenticatedUser{Username: "alice;id"} + req.Binding.Action = &config.Action{ + Title: "Shell after invalid username", + ShellAfterCompleted: "echo test", + } + + args, err := buildShellAfterArgs(req) + + assert.Nil(t, args) + assert.NotNil(t, err) + assert.Contains(t, err.Error(), `system argument "ot_username" failed validation`) +} + +func containsEnvPrefix(env []string, prefix string) bool { + for _, item := range env { + if strings.HasPrefix(item, prefix) { + return true + } + } + + return false } func TestTriggerExecutesTriggeredAction(t *testing.T) {