From df013eb9b12d3dd231def73b3122a5972c3891ae Mon Sep 17 00:00:00 2001 From: AJ Stuyvenberg Date: Sat, 31 Oct 2026 17:47:02 -0400 Subject: [PATCH] fix(interp): refuse help in Check for RemediationOnly builtins Run refuses `help ` for a RemediationOnly builtin (rm, tee, truncate, logrotate, systemctl) outside remediation mode, but Check returned early for `help` without looking at its operand, so it approved scripts Run then refused. Check now applies the same remediationOnlyRefusal predicate dispatch uses and reports CheckRemediationRequired. help also checks the remediation gate before printing a command's static Help text, so a RemediationOnly builtin with static help can't print it outside remediation mode either. Found by Jacob Hayes with patina (https://github.com/JacobHayes/patina), with a Check-vs-Run oracle. Environment: Datadog workspace Co-Authored-By: Claude Opus 5.5 --- builtins/help/help.go | 23 ++++++++++++----------- interp/check_commands.go | 13 ++++++++++++- interp/check_test.go | 35 +++++++++++++++++++++++++++++++++++ 3 files changed, 59 insertions(+), 12 deletions(-) diff --git a/builtins/help/help.go b/builtins/help/help.go index 927c63813..92cd07042 100644 --- a/builtins/help/help.go +++ b/builtins/help/help.go @@ -81,18 +81,12 @@ func registerFlags(fs *builtins.FlagSet) builtins.HandlerFunc { return builtins.Result{Code: 1} } - // Use static Help text if available (for commands that don't - // handle --help, like echo, true, false). - if meta.Help != "" { - callCtx.Outf("%s\n", meta.Help) - return builtins.Result{} - } - // RemediationOnly builtins must never have their handler invoked - // outside remediation mode, even via `help --help`-style - // lookup below. This mirrors the dispatch-level gate in interp so - // a builtin that forgets its own remediation check cannot still - // run its handler (and leak whatever it writes) through help. + // outside remediation mode, even via static Help text or the + // `help --help`-style lookup below. This mirrors the + // dispatch-level gate in interp so a builtin that forgets its own + // remediation check cannot still run its handler (and leak + // whatever it writes) through help. if meta.RemediationOnly && !callCtx.RemediationMode { msg := meta.RemediationDeniedMessage if msg == "" { @@ -102,6 +96,13 @@ func registerFlags(fs *builtins.FlagSet) builtins.HandlerFunc { return builtins.Result{Code: 1} } + // Use static Help text if available (for commands that don't + // handle --help, like echo, true, false). + if meta.Help != "" { + callCtx.Outf("%s\n", meta.Help) + return builtins.Result{} + } + // Otherwise, invoke the command with --help and capture the output. if handler, ok := builtins.Lookup(name); ok && handler != nil { var buf bytes.Buffer diff --git a/interp/check_commands.go b/interp/check_commands.go index 221e77963..b8038cb53 100644 --- a/interp/check_commands.go +++ b/interp/check_commands.go @@ -58,7 +58,18 @@ func (c *scriptChecker) arguments(entry *CommandCheck, args []string, elevated b operation := allowedpaths.PathRead stdinDash := true switch entry.Command { - case "echo", "printf", "true", "false", "exit", "break", "continue", "read", "tr", "uname", "help": + case "help": + // `help ` refuses a RemediationOnly command outside + // remediation mode (builtins/help), the same gate dispatch applies. + if len(operands) == 1 { + if _, isFeature := builtins.Feature(operands[0]); !isFeature { + if message, denied := remediationOnlyRefusal(operands[0], c.policy.remediationMode); denied { + entry.issue(CheckRemediationRequired, message) + } + } + } + return + case "echo", "printf", "true", "false", "exit", "break", "continue", "read", "tr", "uname": return case "df", "free", "ip", "lsof", "pmap", "ps", "ss", "uptime", "vmstat", "ping", "ntfs-du": // These commands use fixed/trusted host capabilities, not user-selected diff --git a/interp/check_test.go b/interp/check_test.go index 8d5e97aca..2adb1e947 100644 --- a/interp/check_test.go +++ b/interp/check_test.go @@ -17,6 +17,7 @@ import ( "testing" "time" + "github.com/DataDog/rshell/builtins" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" "mvdan.cc/sh/v3/expand" @@ -452,3 +453,37 @@ func TestCheckElevatedXargsChildRequiresGrant(t *testing.T) { }) } } + +// TestCheckHelpOfRemediationOnlyBuiltinMatchesRun asserts Check never approves +// `help ` for a RemediationOnly builtin outside remediation mode when Run +// refuses it. The builtins are enumerated from the registry so new ones are +// covered. +func TestCheckHelpOfRemediationOnlyBuiltinMatchesRun(t *testing.T) { + checkRunner(t) // registers the builtins + var names []string + for _, name := range builtins.Names() { + if meta, ok := builtins.Meta(name); ok && meta.RemediationOnly { + names = append(names, name) + } + } + require.NotEmpty(t, names) + for _, name := range names { + for _, mode := range []Mode{ModeReadOnly, ModeRemediation} { + t.Run(name+"/"+string(mode), func(t *testing.T) { + script := "help " + name + runner := checkRunner(t, allowAllCommandsOpt(), WithMode(mode)) + result := checkScript(t, runner, script) + program, err := ParseScript(script, "") + require.NoError(t, err) + err = runner.Run(context.Background(), program) + assert.Equal(t, err == nil, result.Allowed, "%+v", result) + if mode == ModeReadOnly { + assert.False(t, result.Allowed, "%+v", result) + assert.True(t, checkHasIssue(result, CheckRemediationRequired), "%+v", result) + } else { + assert.True(t, result.Allowed, "%+v", result) + } + }) + } + } +}