From 1d365f242195652f1905e1925d01243b9b94b1e6 Mon Sep 17 00:00:00 2001 From: AJ Stuyvenberg Date: Sat, 31 Oct 2026 17:49:57 -0400 Subject: [PATCH 1/2] fix(interp): release builtins blocked on an idle stdin pipe at the run deadline MaxExecutionTime did not bound Run while a builtin waited on an *os.File stdin pipe that never sends, the setup the docs recommend: cat, head, tail, grep, wc, sort, uniq, tr, sed, cut, strings, xargs and jq hung forever. Only read, awk and sha256sum had their own way to wake up at the deadline. Run now binds the runner's stdin to the run's context once: when the context ends, a watchdog sets an expired read deadline on the file, which wakes any blocked Read with os.ErrDeadlineExceeded. The file is never closed and no bytes are read on the caller's behalf, and the deadline is cleared when Run returns. Files without read deadlines (regular files, /dev/null, anonymous pipes on Windows) are left alone. Found by Jacob Hayes with patina (https://github.com/JacobHayes/patina). Environment: Datadog workspace Co-Authored-By: Claude Opus 5.5 --- SHELL_FEATURES.md | 2 +- interp/api.go | 1 + interp/stdin_deadline.go | 44 +++++++++++++++ interp/stdin_pipe_deadline_test.go | 86 ++++++++++++++++++++++++++++++ 4 files changed, 132 insertions(+), 1 deletion(-) create mode 100644 interp/stdin_deadline.go create mode 100644 interp/stdin_pipe_deadline_test.go diff --git a/SHELL_FEATURES.md b/SHELL_FEATURES.md index cb7e9aec..930c590a 100644 --- a/SHELL_FEATURES.md +++ b/SHELL_FEATURES.md @@ -244,7 +244,7 @@ the substituted command. - ✅ AllowedSystemServices policy — one shared default-deny capability map for exact unit names with `read`, `clean`, `start`, `stop`, `reload`, `restart`, `enable`, and `disable` actions; the `*` action wildcard grants every action supported by the running and future rshell versions for its exact unit and is expanded in `help`; actionless grants are ignored, invalid names/actions are skipped, names are case-sensitive, are not normalized, and cannot contain `:` or globs; all valid unit types are accepted; `read` remains usable by bounded `journalctl` queries in read-only mode, every non-read action requires remediation mode, and the entire `systemctl` builtin separately requires remediation mode while `list-units` sees only exact `read` grants; allowing all commands does not bypass this policy; a `read` grant on a `.slice` unit is the one selector whose journal results are not bounded by the granted name, because `journalctl -u SLICE` matches `_SYSTEMD_SLICE` and therefore returns every entry logged by every unit in that slice (see the trusted systemd target exception in `docs/RULES.md`); configure grants through `interp.AllowedSystemServices` or CLI `--allowed-services UNIT:ACTION[+ACTION...]` - ✅ SystemdTargetConfig — systemd-aware builtins use standard local paths by default; explicit `JournalDirs`, `MachineIDPath`, `JournalControlSocket`, and `ManagerBusSocket` paths support container mount layouts without falling back to local endpoints; target paths are trusted configuration and bypass `AllowedPaths`; explicit mounts must all refer to the same host; manager operations use the public system D-Bus socket (default `/run/dbus/system_bus_socket`), pin its inode, authenticate, and verify the systemd manager peer's machine ID, while the private `/run/systemd/private` endpoint is unsupported - ✅ AllowedPaths filesystem sandboxing — restricts all file access (read and write) to specified directories. Entries may end with `:ro` or `:rw` to indicate read-only and read-write permissions, respectively; entries without a suffix default to read-only. A terminal `:ro`/`:rw` is always read as the access mode and never as part of the file name, on every platform, so a directory whose real name ends in `:ro` or `:rw` cannot be configured; suffix parsing never consults the filesystem, so files a script creates cannot change which roots a later runner opens. On Linux and macOS, when an entry's full text names an existing path, the runner emits a non-fatal `AllowedPaths: existing path "…" is not granted` warning (see [Security model](README.md#security-model)). In remediation mode, write operations are accepted only inside the most-specific matching `:rw` root. Write and remove targets are also refused when any directory they pass through is, by filesystem identity, a read-only root, so a different spelling of a nested `:ro` root (letter case on a case-insensitive volume, an NTFS 8.3 short name, a bind mount) cannot be written through a writable parent root. Cross-root symlink fallback is read-only to avoid TOCTOU on writes; on Unix, symlink components in write targets are rejected with `symlinks are not supported as write targets` via a no-follow `openat` walk. Containment is path-based and cannot see hard links, so every content-mutating write target is additionally checked on its open handle and rejected with `hard links are not supported as write targets` when its link count exceeds one; `rm` is deliberately exempt (unlinking one of several names changes no content). A link count that cannot be determined fails closed -- ✅ Whole-run execution timeout — callers can bound a `Run()` call via `context.Context`, `interp.MaxExecutionTime`, or the CLI `--timeout` flag; the deadline applies to the whole script. Cancellation is checked between bounded operations; a caller-owned stream blocked in `Read` or `Write` can delay return because rshell does not close borrowed streams +- ✅ Whole-run execution timeout — callers can bound a `Run()` call via `context.Context`, `interp.MaxExecutionTime`, or the CLI `--timeout` flag; the deadline applies to the whole script. Cancellation is checked between bounded operations; a caller-owned stream blocked in `Write` can delay return because rshell does not close borrowed streams. A builtin blocked in `Read` on a deadline-capable `*os.File` stdin (an `os.Pipe`, socket or TTY) is released when the deadline fires, without closing the file or consuming its bytes; where the platform gives that file no read deadlines (anonymous pipes on Windows) a blocked stdin `Read` can still delay return - ✅ ProcPath — overrides the proc filesystem path used by `ps` (default `/proc`; Linux-only; useful for testing/container environments); `ps` reads only bounded process metadata and counters, never `/proc//cmdline` or process environment data - ✅ RemediationMode — opt-in mode (`interp.WithMode(interp.ModeRemediation)` / `--mode remediation`) that enables file-target output redirections (`>`, `>>`, `2>`, `&>`, `&>>`) within `:rw` `AllowedPaths` and remediation-only builtins such as `truncate`, `tee`, `logrotate`, `rm`, and the entire restricted `systemctl` surface; targets outside the allowlist or inside read-only roots fail with `permission denied` (exit 1); symlinked write targets fail with `symlinks are not supported as write targets`; hard-linked write targets fail with `hard links are not supported as write targets` on Unix; `/dev/null` always accepted; `<>` remains blocked - ✅ Privileged helper isolation (Linux) — authenticated `runCommand` and `runRemediationCommand` Private Action Runner dispatches run in fresh same-binary workers with a Landlock ABI 3+ policy derived from the verified effective `AllowedPaths`, `AllowedCommands`, and `AllowedSystemServices`, followed by a TSYNC seccomp denylist; authorized `journalctl` and remediation-mode `systemctl` calls receive only the fixed local systemd paths their grants require; `runCommand` forces ordinary path grants to read-only and never grants journal deletion rights, verified roots are pinned before execution drops back to the service account, sandbox setup fails closed, external execution is denied, and only explicitly authorized commands use controlled `setresuid` elevation diff --git a/interp/api.go b/interp/api.go index 41c72d79..69734158 100644 --- a/interp/api.go +++ b/interp/api.go @@ -701,6 +701,7 @@ func (r *Runner) Run(ctx context.Context, node syntax.Node) (retErr error) { // per-command telemetry can detect pipe and redirect reassignments // without tripping on the Run-level limitWriter wrap. r.runStdin = r.stdin + defer bindStdinToContext(ctx, r.stdin)() r.runStdout = r.stdout r.startTime = time.Now() r.globReadDirCount = &atomic.Int64{} diff --git a/interp/stdin_deadline.go b/interp/stdin_deadline.go new file mode 100644 index 00000000..cd7b783e --- /dev/null +++ b/interp/stdin_deadline.go @@ -0,0 +1,44 @@ +// Unless explicitly stated otherwise all files in this repository are licensed +// under the Apache License Version 2.0. +// This product includes software developed at Datadog (https://www.datadoghq.com/). +// Copyright 2026-present Datadog, Inc. + +package interp + +import ( + "context" + "os" + "time" +) + +// bindStdinToContext makes a Read blocked on f return once ctx is done, so a +// builtin waiting on an idle stdin pipe cannot outlive MaxExecutionTime or a +// caller cancellation. When ctx finishes, a watchdog sets an already-expired +// read deadline on f, which wakes every blocked and future Read with +// [os.ErrDeadlineExceeded]. The returned function stops the watchdog and +// clears the deadline so the caller's file is left as it was found. +// +// f is never closed and no extra Read is issued, so no bytes are consumed on +// the caller's behalf. Files that do not support deadlines (regular files, +// /dev/null, and anonymous pipes on Windows) are left alone: regular files do +// not block, and on Windows a blocked pipe Read cannot be interrupted. +func bindStdinToContext(ctx context.Context, f *os.File) (release func()) { + if f == nil || ctx.Done() == nil || f.SetReadDeadline(time.Time{}) != nil { + return func() {} + } + stop := make(chan struct{}) + done := make(chan struct{}) + go func() { + defer close(done) + select { + case <-ctx.Done(): + _ = f.SetReadDeadline(time.Now()) + case <-stop: + } + }() + return func() { + close(stop) + <-done + _ = f.SetReadDeadline(time.Time{}) + } +} diff --git a/interp/stdin_pipe_deadline_test.go b/interp/stdin_pipe_deadline_test.go new file mode 100644 index 00000000..b23cfd9a --- /dev/null +++ b/interp/stdin_pipe_deadline_test.go @@ -0,0 +1,86 @@ +// Unless explicitly stated otherwise all files in this repository are licensed +// under the Apache License Version 2.0. +// This product includes software developed at Datadog (https://www.datadoghq.com/). +// Copyright 2026-present Datadog, Inc. + +package interp + +import ( + "context" + "io" + "os" + "testing" + "time" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +// TestMaxExecutionTimeBoundsBuiltinBlockedOnStdinPipe verifies that a builtin +// blocked reading a runner stdin *os.File pipe that never receives data is +// released when the run's deadline fires. +func TestMaxExecutionTimeBoundsBuiltinBlockedOnStdinPipe(t *testing.T) { + scripts := map[string]string{ + "cat": "cat", + "head": "head", + "tail": "tail", + "grep": "grep x", + "wc": "wc", + "sort": "sort", + "uniq": "uniq", + "tr": "tr a b", + "sed": "sed p", + "cut": "cut -c1", + "strings": "strings", + "xargs": "xargs", + "jq": "jq .", + "read": "read x", + "awk": "awk '{print}'", + "sha256sum": "sha256sum", + } + for name, script := range scripts { + t.Run(name, func(t *testing.T) { + pr, pw, err := os.Pipe() + require.NoError(t, err) + t.Cleanup(func() { _ = pr.Close(); _ = pw.Close() }) + + r, err := New(allowAllCommandsOpt(), StdIO(pr, io.Discard, io.Discard), MaxExecutionTime(200*time.Millisecond)) + require.NoError(t, err) + t.Cleanup(func() { _ = r.Close() }) + + done := make(chan error, 1) + go func() { done <- r.Run(context.Background(), parseScript(t, script)) }() + select { + case err := <-done: + assert.ErrorIs(t, err, context.DeadlineExceeded) + case <-time.After(5 * time.Second): + t.Fatalf("%s: Run did not return after MaxExecutionTime with an idle stdin pipe", name) + } + }) + } +} + +// TestStdinPipeDeadlineDoesNotLeakOrStealBytes verifies a normal run leaves +// the caller's pipe usable: no deadline is left behind and no bytes are eaten. +func TestStdinPipeDeadlineDoesNotLeakOrStealBytes(t *testing.T) { + pr, pw, err := os.Pipe() + require.NoError(t, err) + t.Cleanup(func() { _ = pr.Close(); _ = pw.Close() }) + + r, err := New(allowAllCommandsOpt(), StdIO(pr, io.Discard, io.Discard), MaxExecutionTime(5*time.Second)) + require.NoError(t, err) + t.Cleanup(func() { _ = r.Close() }) + + _, err = pw.WriteString("one\n") + require.NoError(t, err) + require.NoError(t, r.Run(context.Background(), parseScript(t, "read x"))) + + // The deadline must have been cleared after Run. + _, err = pw.WriteString("two\n") + require.NoError(t, err) + time.Sleep(50 * time.Millisecond) + buf := make([]byte, 16) + n, err := pr.Read(buf) + require.NoError(t, err) + assert.Equal(t, "two\n", string(buf[:n])) +} From 9e97c22f49fbe7ac7c4604f18fe57ea5cc8a31a0 Mon Sep 17 00:00:00 2001 From: AJ Stuyvenberg Date: Sat, 31 Oct 2026 20:00:18 -0400 Subject: [PATCH 2/2] test(interp): skip deadline-reliant idle-pipe cases on Windows Anonymous pipes on Windows have no read deadlines, the documented limit of this fix; read, awk and sha256sum still run there. Environment: Datadog workspace Co-Authored-By: Claude Opus 5.5 --- interp/stdin_pipe_deadline_test.go | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/interp/stdin_pipe_deadline_test.go b/interp/stdin_pipe_deadline_test.go index b23cfd9a..a8cfd5a5 100644 --- a/interp/stdin_pipe_deadline_test.go +++ b/interp/stdin_pipe_deadline_test.go @@ -9,6 +9,7 @@ import ( "context" "io" "os" + "runtime" "testing" "time" @@ -38,8 +39,14 @@ func TestMaxExecutionTimeBoundsBuiltinBlockedOnStdinPipe(t *testing.T) { "awk": "awk '{print}'", "sha256sum": "sha256sum", } + // read, awk and sha256sum wake at the deadline by themselves; the rest rely + // on a read deadline on stdin, which Windows anonymous pipes do not support. + selfBounded := map[string]bool{"read": true, "awk": true, "sha256sum": true} for name, script := range scripts { t.Run(name, func(t *testing.T) { + if runtime.GOOS == "windows" && !selfBounded[name] { + t.Skip("anonymous pipes on Windows have no read deadlines") + } pr, pw, err := os.Pipe() require.NoError(t, err) t.Cleanup(func() { _ = pr.Close(); _ = pw.Close() })