From db5b299187d1e0c0d00ffc192f9efa6e4bad64e4 Mon Sep 17 00:00:00 2001 From: Jules Macret Date: Thu, 30 Apr 2026 14:48:07 +0200 Subject: [PATCH 1/5] feat(interp): route file-target output redirects through AllowedPaths Stop rejecting >, >>, 2>, 2>>, &>, &>> at validation. The runtime now opens the redirect target via r.open(), which is backed by the AllowedPaths sandbox added in PR #206. Targets inside AllowedPaths are created/truncated/appended; targets outside fail with "permission denied" and exit 1. The literal "/dev/null" continues to short-circuit to io.Discard so operators don't need to whitelist it. The opened file is returned as the redir Closer so the caller flushes and closes it before the next command runs. <> (read-write open) stays rejected; bash quirks (ambiguous-redirect, noclobber) remain out of scope. Co-Authored-By: Claude Opus 4.7 (1M context) --- README.md | 4 +- SHELL_FEATURES.md | 12 +- interp/runner_redir.go | 44 +++-- interp/tests/cmdsubst_pentest_test.go | 6 +- interp/tests/redir_devnull_pentest_test.go | 63 +++++--- interp/tests/redir_devnull_test.go | 49 +++--- interp/tests/redir_file_target_test.go | 153 ++++++++++++++++++ interp/validate.go | 49 +----- .../shell/blocked_redirects/append_all.yaml | 8 +- .../append_redirect_blocked.yaml | 8 +- .../blocked_after_valid.yaml | 12 +- .../output_redirect_variable.yaml | 8 +- .../shell/blocked_redirects/stderr_write.yaml | 8 +- .../variable_redirect_target.yaml | 11 +- .../shell/blocked_redirects/write_all.yaml | 8 +- .../shell/blocked_redirects/write_append.yaml | 8 +- .../blocked_redirects/write_clobber.yaml | 8 +- .../blocked_redirects/write_truncate.yaml | 8 +- .../devnull_path_traversal_blocked.yaml | 8 +- .../redirect_to_file_still_blocked.yaml | 8 +- .../stderr_redirect_to_file_blocked.yaml | 8 +- .../redirections/file_target/append.yaml | 16 ++ .../file_target/both_streams_append.yaml | 16 ++ .../file_target/both_streams_truncate.yaml | 11 ++ .../file_target/heredoc_to_file.yaml | 15 ++ .../file_target/outside_allowed_paths.yaml | 16 ++ .../file_target/path_traversal_blocked.yaml | 16 ++ .../redirections/file_target/stderr_only.yaml | 13 ++ .../stderr_to_stdout_then_file.yaml | 11 ++ .../redirections/file_target/truncate.yaml | 11 ++ .../file_target/truncate_overwrites.yaml | 15 ++ 31 files changed, 463 insertions(+), 168 deletions(-) create mode 100644 interp/tests/redir_file_target_test.go create mode 100644 tests/scenarios/shell/redirections/file_target/append.yaml create mode 100644 tests/scenarios/shell/redirections/file_target/both_streams_append.yaml create mode 100644 tests/scenarios/shell/redirections/file_target/both_streams_truncate.yaml create mode 100644 tests/scenarios/shell/redirections/file_target/heredoc_to_file.yaml create mode 100644 tests/scenarios/shell/redirections/file_target/outside_allowed_paths.yaml create mode 100644 tests/scenarios/shell/redirections/file_target/path_traversal_blocked.yaml create mode 100644 tests/scenarios/shell/redirections/file_target/stderr_only.yaml create mode 100644 tests/scenarios/shell/redirections/file_target/stderr_to_stdout_then_file.yaml create mode 100644 tests/scenarios/shell/redirections/file_target/truncate.yaml create mode 100644 tests/scenarios/shell/redirections/file_target/truncate_overwrites.yaml diff --git a/README.md b/README.md index 157c1aa3..04370c1c 100644 --- a/README.md +++ b/README.md @@ -62,11 +62,11 @@ Every access path is default-deny: | External commands | Blocked (exit code 127) | Provide an `ExecHandler` | | Filesystem access | Blocked | Configure `AllowedPaths` with directory list | | Environment variables| Empty (no host env inherited) | Pass variables via the `Env` option | -| Output redirections | Only `/dev/null` allowed (exit code 2 for other targets) | `>/dev/null`, `2>/dev/null`, `&>/dev/null`, `2>&1` | +| Output redirections | File-target redirects routed through `AllowedPaths` (writes outside fail with `permission denied` and exit 1) | `>FILE`, `>>FILE`, `2>FILE`, `&>FILE`, `&>>FILE`, `>/dev/null`, `2>&1` | **AllowedCommands** restricts which commands (builtins or external) the interpreter may execute. Commands must be specified with the `rshell:` namespace prefix (e.g. `rshell:cat`, `rshell:echo`). If not set, no commands are allowed. -**AllowedPaths** restricts all file operations to specified directories using Go's `os.Root` API (`openat` syscalls), making it immune to symlink traversal, TOCTOU races, and `..` escape attacks. Both reads and writes are sandboxed by the same mechanism — files outside the allowlist cannot be opened, created, truncated, or appended to. The cross-root symlink fallback is read-only: a symlink that points outside its `os.Root` is followed for reads but never for writes (avoids a TOCTOU window where a malicious link target could be swapped between resolution and open). Whether a particular shell feature actually performs writes is layered on top — output redirections (`>`, `>>`) to file targets are still rejected at parse time, so the user-visible surface remains read-only until builtins or redirection layers opt in. Configured directories that cannot be opened (missing, not a directory, no permission) are skipped with a diagnostic message; by default these messages are flushed once to the runner's stderr at construction time. Callers that need to keep stderr clean of sandbox diagnostics can route them to a dedicated sink with `WarningsWriter(io.Writer)` or retrieve them programmatically via `Runner.Warnings()`. +**AllowedPaths** restricts all file operations to specified directories using Go's `os.Root` API (`openat` syscalls), making it immune to symlink traversal, TOCTOU races, and `..` escape attacks. Both reads and writes are sandboxed by the same mechanism — files outside the allowlist cannot be opened, created, truncated, or appended to. The cross-root symlink fallback is read-only: a symlink that points outside its `os.Root` is followed for reads but never for writes (avoids a TOCTOU window where a malicious link target could be swapped between resolution and open). File-target output redirections (`>`, `>>`, `2>`, `&>`, `&>>`) open through the same sandbox: writes inside `AllowedPaths` succeed, anything else fails with `permission denied` and exit 1. The literal target `/dev/null` is short-circuited to a discarded sink without going through the sandbox. Configured directories that cannot be opened (missing, not a directory, no permission) are skipped with a diagnostic message; by default these messages are flushed once to the runner's stderr at construction time. Callers that need to keep stderr clean of sandbox diagnostics can route them to a dedicated sink with `WarningsWriter(io.Writer)` or retrieve them programmatically via `Runner.Warnings()`. > **Note:** The `ss` and `ip route` builtins bypass `AllowedPaths` for their `/proc/net/*` reads. Both builtins open kernel pseudo-filesystem paths (e.g. `/proc/net/tcp`, `/proc/net/route`) directly with `os.Open` rather than going through the sandboxed opener. These paths are hardcoded in the implementation and are never derived from user input, so there is no sandbox-escape risk. However, operators cannot use `AllowedPaths` to block `ss` from enumerating local sockets or `ip route` from reading the routing table — these reads succeed regardless of the configured path policy. diff --git a/SHELL_FEATURES.md b/SHELL_FEATURES.md index 7e7028d1..5efef31b 100644 --- a/SHELL_FEATURES.md +++ b/SHELL_FEATURES.md @@ -72,17 +72,13 @@ Blocked features are rejected before execution with exit code 2. - ✅ `<` — input redirection (read-only, within AllowedPaths) - ✅ `</dev/null`, `2>/dev/null` — redirect stdout or stderr to /dev/null (output is discarded; only `/dev/null` is allowed as target) -- ✅ `&>/dev/null` — redirect both stdout and stderr to /dev/null -- ✅ `>>/dev/null`, `&>>/dev/null` — append redirect to /dev/null (same effect as truncate) +- ✅ `> FILE`, `>> FILE` — write/truncate or append to a regular file inside AllowedPaths; `/dev/null` is always accepted (output is discarded). Targets outside AllowedPaths return `permission denied` and exit 1. +- ✅ `2> FILE`, `2>> FILE` — same rules for the stderr stream. +- ✅ `&> FILE`, `&>> FILE` — redirect both stdout and stderr to the same file (truncate / append). - ✅ `2>&1`, `>&2` — file descriptor duplication between stdout (1) and stderr (2) - ❌ `|&` — pipe stdout and stderr (bash extension) - ❌ `<<<` — herestring (bash extension) -- ❌ `> FILE` — write/truncate to any file other than /dev/null -- ❌ `>> FILE` — append to any file other than /dev/null -- ❌ `&> FILE` — redirect all to any file other than /dev/null -- ❌ `&>> FILE` — append all to any file other than /dev/null -- ❌ `<>` — read-write +- ❌ `<>` — read-write open - ❌ `<&N` — input file descriptor duplication ## Quoting and Expansion diff --git a/interp/runner_redir.go b/interp/runner_redir.go index 15c8e927..502d81b7 100644 --- a/interp/runner_redir.go +++ b/interp/runner_redir.go @@ -246,26 +246,44 @@ func (r *Runner) redir(ctx context.Context, rd *syntax.Redirect) (io.Closer, err // done further below case syntax.RdrOut, syntax.ClbOut, syntax.AppOut: - // Output redirects are only allowed to /dev/null (enforced at validation). - // Re-check at runtime after variable expansion for defense-in-depth. - if !isDevNull(arg) { - r.errf("> %s: file redirection is only supported for /dev/null\n", arg) - return nil, fmt.Errorf("> %s: file redirection is only supported for /dev/null", arg) + // /dev/null is short-circuited to io.Discard. The sandbox does not + // add /dev/null to AllowedPaths automatically, so going through + // r.open would require operators to whitelist it explicitly. + if isDevNull(arg) { + *orig = io.Discard + return nil, nil } - *orig = io.Discard - return nil, nil + flags := os.O_WRONLY | os.O_CREATE | os.O_TRUNC + if rd.Op == syntax.AppOut { + flags = os.O_WRONLY | os.O_CREATE | os.O_APPEND + } + f, err := r.open(ctx, arg, flags, 0644, true) + if err != nil { + return nil, err + } + *orig = f + return f, nil case syntax.RdrAll, syntax.AppAll: // Note: these ops redirect both stdout and stderr, so they assign // r.stdout and r.stderr directly rather than going through *orig. // Bash does not allow an explicit fd prefix on &>/&>>. - if !isDevNull(arg) { - r.errf("&> %s: file redirection is only supported for /dev/null\n", arg) - return nil, fmt.Errorf("&> %s: file redirection is only supported for /dev/null", arg) + if isDevNull(arg) { + r.stdout = io.Discard + r.stderr = io.Discard + return nil, nil } - r.stdout = io.Discard - r.stderr = io.Discard - return nil, nil + flags := os.O_WRONLY | os.O_CREATE | os.O_TRUNC + if rd.Op == syntax.AppAll { + flags = os.O_WRONLY | os.O_CREATE | os.O_APPEND + } + f, err := r.open(ctx, arg, flags, 0644, true) + if err != nil { + return nil, err + } + r.stdout = f + r.stderr = f + return f, nil case syntax.DplOut: switch arg { diff --git a/interp/tests/cmdsubst_pentest_test.go b/interp/tests/cmdsubst_pentest_test.go index efc3f7c1..e75ceb4f 100644 --- a/interp/tests/cmdsubst_pentest_test.go +++ b/interp/tests/cmdsubst_pentest_test.go @@ -185,9 +185,11 @@ func TestCmdSubstPentestCatShortcutEmptyFile(t *testing.T) { func TestSubshellPentestRedirectOutBlocked(t *testing.T) { dir := t.TempDir() + // Subshell stdout redirect to a path outside AllowedPaths is rejected by + // the sandbox at runtime (exit 1, "permission denied"). _, stderr, code := subshellRun(t, `(echo data) > /tmp/evil.txt`, dir) - assert.Equal(t, 2, code) - assert.Contains(t, stderr, "file redirection is not supported") + assert.Equal(t, 1, code) + assert.Contains(t, stderr, "permission denied") } // --- Context cancellation --- diff --git a/interp/tests/redir_devnull_pentest_test.go b/interp/tests/redir_devnull_pentest_test.go index ff9f01db..2c07e9e0 100644 --- a/interp/tests/redir_devnull_pentest_test.go +++ b/interp/tests/redir_devnull_pentest_test.go @@ -71,6 +71,16 @@ func pentestRedirRunProg(ctx context.Context, t *testing.T, prog *syntax.File, d } // --- Path traversal attacks --- +// +// pentestRedirRun configures AllowedPaths to the test dir, so any traversal +// that resolves outside dir must fail at sandbox open time. The literal +// "/dev/null" string is short-circuited to io.Discard before the sandbox is +// consulted; any non-literal variant (extra slash, ".." segment, case +// variation, relative path) is opened through the sandbox and rejected. +// The exact error message ("permission denied" for absolute paths outside +// the sandbox roots, "no such file" for paths that resolve inside but to a +// missing component) varies; the important property is that the redirect +// fails with a non-zero exit and never silently writes a file. func TestPentestRedirPathTraversal(t *testing.T) { dir := t.TempDir() @@ -85,21 +95,23 @@ func TestPentestRedirPathTraversal(t *testing.T) { {"trailing slash", "echo hello > /dev/null/"}, {"case variation", "echo hello > /Dev/Null"}, {"relative devnull", "echo hello > dev/null"}, - {"bare null", "echo hello > null"}, } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { stdout, stderr, code := pentestRedirRun(t, tt.script, dir) assert.Equal(t, "", stdout, "should produce no stdout") - assert.NotEqual(t, 0, code, "should fail with non-zero exit") - // Should either be validation error (exit 2) or runtime error - assert.True(t, code == 2 || code == 1, "exit code should be 1 or 2, got %d", code) - _ = stderr // error message varies + assert.Equal(t, 1, code, "should fail at sandbox open with exit 1") + assert.NotEmpty(t, stderr, "expected an error message on stderr") }) } } -// --- Variable expansion attacks --- +// --- Variable expansion (now allowed) --- +// +// Variable expansion in the redirect target is permitted; the runtime opens +// the expanded path through the sandbox. /dev/null is special-cased and +// short-circuited to io.Discard, so a variable that resolves to /dev/null +// produces a successful, no-op redirect. func TestPentestRedirVariableExpansion(t *testing.T) { dir := t.TempDir() @@ -113,13 +125,19 @@ func TestPentestRedirVariableExpansion(t *testing.T) { } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { - _, _, code := pentestRedirRun(t, tt.script, dir) - assert.Equal(t, 2, code, "variable expansion in redirect target should be blocked at validation") + stdout, stderr, code := pentestRedirRun(t, tt.script, dir) + assert.Equal(t, 0, code, "variable expanding to /dev/null should succeed silently") + assert.Equal(t, "", stdout) + assert.Equal(t, "", stderr) }) } } -// --- Quoting attacks --- +// --- Quoted /dev/null (now accepted) --- +// +// Quoted "/dev/null" expands to the literal "/dev/null" string at runtime via +// expand.Literal, so the io.Discard short-circuit applies just like the +// unquoted form. func TestPentestRedirQuotedDevNull(t *testing.T) { dir := t.TempDir() @@ -132,21 +150,25 @@ func TestPentestRedirQuotedDevNull(t *testing.T) { } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { - _, _, code := pentestRedirRun(t, tt.script, dir) - // Quoted paths have different AST structure (SglQuoted/DblQuoted vs Lit) - // Our check requires a single Lit part, so quoted paths should be rejected - assert.Equal(t, 2, code, "quoted /dev/null in redirect should be blocked at validation") + stdout, stderr, code := pentestRedirRun(t, tt.script, dir) + assert.Equal(t, 0, code, "quoted /dev/null should succeed silently") + assert.Equal(t, "", stdout) + assert.Equal(t, "", stderr) }) } } -// --- Glob/wildcard attacks --- +// --- Glob/wildcard in target --- +// +// Redirect targets are not glob-expanded (expand.Literal, not expand.Fields), +// so the literal string "/dev/nul?" is passed through to the sandbox, which +// rejects it. func TestPentestRedirGlobInTarget(t *testing.T) { dir := t.TempDir() - // Glob characters in redirect targets - _, _, code := pentestRedirRun(t, "echo hello > /dev/nul?", dir) - assert.Equal(t, 2, code, "glob in redirect target should be rejected") + _, stderr, code := pentestRedirRun(t, "echo hello > /dev/nul?", dir) + assert.Equal(t, 1, code, "literal '?' in redirect target should fail at sandbox open") + assert.Contains(t, stderr, "permission denied") } // --- fd duplication attacks --- @@ -255,9 +277,10 @@ func TestPentestRedirHerestringBlocked(t *testing.T) { func TestPentestRedirMixedAllowedBlocked(t *testing.T) { dir := t.TempDir() - // First redirect is allowed, second is not - _, _, code := pentestRedirRun(t, "echo hello >/dev/null > /tmp/evil", dir) - assert.Equal(t, 2, code, "mixed redirects with blocked target should fail at validation") + // First redirect is allowed (/dev/null), second is rejected by the sandbox. + _, stderr, code := pentestRedirRun(t, "echo hello >/dev/null > /tmp/evil", dir) + assert.Equal(t, 1, code, "blocked second redirect should fail at sandbox open") + assert.Contains(t, stderr, "permission denied") } // --- Ensure /dev/null redirect doesn't create any files --- diff --git a/interp/tests/redir_devnull_test.go b/interp/tests/redir_devnull_test.go index f12ca6e6..2730dca0 100644 --- a/interp/tests/redir_devnull_test.go +++ b/interp/tests/redir_devnull_test.go @@ -174,39 +174,38 @@ func TestRedirDevNullPreservesFailureExitCode(t *testing.T) { assert.Equal(t, "1\n", stdout) } -// --- Blocked redirects (still rejected) --- +// --- Sandbox-blocked redirects (target outside AllowedPaths) --- -func TestRedirToFileStillBlocked(t *testing.T) { +func TestRedirToFileBlockedBySandbox(t *testing.T) { dir := t.TempDir() - // The validation should reject this stdout, stderr, code := redirRunNoAllowed(t, "echo hello > /tmp/output.txt", dir) - assert.Equal(t, 2, code) + assert.Equal(t, 1, code) assert.Equal(t, "", stdout) - assert.Contains(t, stderr, "file redirection is not supported") + assert.Contains(t, stderr, "permission denied") } -func TestRedirStderrToFileStillBlocked(t *testing.T) { +func TestRedirStderrToFileBlockedBySandbox(t *testing.T) { dir := t.TempDir() stdout, stderr, code := redirRunNoAllowed(t, "echo hello 2> /tmp/errors.txt", dir) - assert.Equal(t, 2, code) + assert.Equal(t, 1, code) assert.Equal(t, "", stdout) - assert.Contains(t, stderr, "file redirection is not supported") + assert.Contains(t, stderr, "permission denied") } -func TestRedirAppendToFileStillBlocked(t *testing.T) { +func TestRedirAppendToFileBlockedBySandbox(t *testing.T) { dir := t.TempDir() stdout, stderr, code := redirRunNoAllowed(t, "echo hello >> /tmp/output.txt", dir) - assert.Equal(t, 2, code) + assert.Equal(t, 1, code) assert.Equal(t, "", stdout) - assert.Contains(t, stderr, "file redirection is not supported") + assert.Contains(t, stderr, "permission denied") } -func TestRedirAllToFileStillBlocked(t *testing.T) { +func TestRedirAllToFileBlockedBySandbox(t *testing.T) { dir := t.TempDir() stdout, stderr, code := redirRunNoAllowed(t, "echo hello &> /tmp/output.txt", dir) - assert.Equal(t, 2, code) + assert.Equal(t, 1, code) assert.Equal(t, "", stdout) - assert.Contains(t, stderr, "file redirection is not supported") + assert.Contains(t, stderr, "permission denied") } // --- Path traversal via /dev/null --- @@ -214,17 +213,17 @@ func TestRedirAllToFileStillBlocked(t *testing.T) { func TestRedirDevNullPathTraversalBlocked(t *testing.T) { dir := t.TempDir() stdout, stderr, code := redirRunNoAllowed(t, "echo hello > /dev/null/../../../tmp/evil", dir) - assert.Equal(t, 2, code) + assert.Equal(t, 1, code) assert.Equal(t, "", stdout) - assert.Contains(t, stderr, "file redirection is not supported") + assert.Contains(t, stderr, "permission denied") } func TestRedirDevNullExtraSlashBlocked(t *testing.T) { dir := t.TempDir() stdout, stderr, code := redirRunNoAllowed(t, "echo hello > /dev//null", dir) - assert.Equal(t, 2, code) + assert.Equal(t, 1, code) assert.Equal(t, "", stdout) - assert.Contains(t, stderr, "file redirection is not supported") + assert.Contains(t, stderr, "permission denied") } // --- Unsupported fd numbers --- @@ -232,9 +231,9 @@ func TestRedirDevNullExtraSlashBlocked(t *testing.T) { func TestRedirFd3Blocked(t *testing.T) { dir := t.TempDir() stdout, stderr, code := redirRunNoAllowed(t, "echo hello 3>/dev/null", dir) - assert.Equal(t, 2, code) + assert.Equal(t, 1, code) assert.Equal(t, "", stdout) - assert.Contains(t, stderr, "file redirection is not supported") + assert.Contains(t, stderr, "unsupported fd") } func TestRedirDupFd3Blocked(t *testing.T) { @@ -269,13 +268,13 @@ func TestRedirMultipleDevNull(t *testing.T) { assert.Equal(t, "", stderr) } -// --- Variable in redirect target should be blocked --- +// --- Variable in redirect target --- -func TestRedirVariableTargetBlocked(t *testing.T) { +func TestRedirVariableTargetExpandsToDevNull(t *testing.T) { dir := t.TempDir() - // $TARGET in redirect word makes it non-literal, so validation rejects it + // $TARGET expands to /dev/null at runtime, which is short-circuited to io.Discard. stdout, stderr, code := redirRunNoAllowed(t, "TARGET=/dev/null; echo hello > $TARGET", dir) - assert.Equal(t, 2, code) + assert.Equal(t, 0, code) assert.Equal(t, "", stdout) - assert.Contains(t, stderr, "file redirection is not supported") + assert.Equal(t, "", stderr) } diff --git a/interp/tests/redir_file_target_test.go b/interp/tests/redir_file_target_test.go new file mode 100644 index 00000000..accf65a9 --- /dev/null +++ b/interp/tests/redir_file_target_test.go @@ -0,0 +1,153 @@ +// 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 tests_test + +import ( + "os" + "path/filepath" + "runtime" + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +// File-target output redirects (>, >>, 2>, &>, &>>) write through the +// AllowedPaths sandbox. These tests verify behaviour that scenario tests +// cannot express directly: that the file handle is closed before the next +// command runs (so reads from disk see the written bytes), that file mode +// is 0644, and that the sandbox boundary is enforced when the open fails. + +func TestRedirTruncateWritesAndCloses(t *testing.T) { + dir := t.TempDir() + stdout, stderr, code := redirRun(t, "echo hi > out.txt", dir) + require.Equal(t, 0, code) + assert.Empty(t, stdout) + assert.Empty(t, stderr) + + got, err := os.ReadFile(filepath.Join(dir, "out.txt")) + require.NoError(t, err) + assert.Equal(t, "hi\n", string(got)) +} + +func TestRedirAppendPreservesExisting(t *testing.T) { + dir := t.TempDir() + require.NoError(t, os.WriteFile(filepath.Join(dir, "out.txt"), []byte("first\n"), 0644)) + _, _, code := redirRun(t, "echo second >> out.txt", dir) + require.Equal(t, 0, code) + + got, err := os.ReadFile(filepath.Join(dir, "out.txt")) + require.NoError(t, err) + assert.Equal(t, "first\nsecond\n", string(got)) +} + +func TestRedirTruncateOverwritesExistingShorter(t *testing.T) { + dir := t.TempDir() + original := "this string is intentionally longer than the new contents" + require.NoError(t, os.WriteFile(filepath.Join(dir, "out.txt"), []byte(original), 0644)) + _, _, code := redirRun(t, "echo new > out.txt", dir) + require.Equal(t, 0, code) + + got, err := os.ReadFile(filepath.Join(dir, "out.txt")) + require.NoError(t, err) + assert.Equal(t, "new\n", string(got)) +} + +func TestRedirCreatesFileWithMode0644(t *testing.T) { + if runtime.GOOS == "windows" { + t.Skip("file mode bits are not honoured on Windows") + } + dir := t.TempDir() + _, _, code := redirRun(t, "echo hi > out.txt", dir) + require.Equal(t, 0, code) + + info, err := os.Stat(filepath.Join(dir, "out.txt")) + require.NoError(t, err) + // Permission bits depend on the test process umask. The interpreter + // passes 0644 to os.OpenFile; assert no executable or extra bits are + // set, which is the practical guarantee callers depend on. + mode := info.Mode().Perm() + assert.Zero(t, mode&0111, "file should not be executable, got %o", mode) + assert.NotZero(t, mode&0400, "owner should have read access, got %o", mode) + assert.NotZero(t, mode&0200, "owner should have write access, got %o", mode) +} + +func TestRedirHeredocAndFileTargetCombine(t *testing.T) { + dir := t.TempDir() + script := "cat < out.txt\nfoo\nbar\nEOF" + _, _, code := redirRun(t, script, dir) + require.Equal(t, 0, code) + + got, err := os.ReadFile(filepath.Join(dir, "out.txt")) + require.NoError(t, err) + assert.Equal(t, "foo\nbar\n", string(got)) +} + +func TestRedirStderrToFileSeparatesStreams(t *testing.T) { + dir := t.TempDir() + stdout, _, code := redirRun(t, "cat missing 2> err.log; echo ok", dir) + assert.Contains(t, stdout, "ok") + + got, err := os.ReadFile(filepath.Join(dir, "err.log")) + require.NoError(t, err) + // rshell's cat normalises errno messages via PortableErrMsg. + assert.Contains(t, string(got), "no such file or directory") + assert.Equal(t, 0, code, "trailing echo overrides cat's exit status") +} + +func TestRedirBothStreamsToSameFile(t *testing.T) { + dir := t.TempDir() + _, _, code := redirRun(t, "echo only-stdout &> combined.log", dir) + require.Equal(t, 0, code) + + got, err := os.ReadFile(filepath.Join(dir, "combined.log")) + require.NoError(t, err) + assert.Equal(t, "only-stdout\n", string(got)) +} + +func TestRedirOutsideAllowedPathsDoesNotCreateFile(t *testing.T) { + allowed := t.TempDir() + other := t.TempDir() + target := filepath.Join(other, "evil.txt") + + stdout, stderr, code := redirRun(t, "echo hi > "+target, allowed) + assert.Equal(t, 1, code) + assert.Empty(t, stdout) + assert.Contains(t, stderr, "permission denied") + + _, err := os.Stat(target) + assert.True(t, os.IsNotExist(err), "redirect must not create files outside AllowedPaths, got err=%v", err) +} + +func TestRedirFailureDoesNotRunCommand(t *testing.T) { + allowed := t.TempDir() + // The first redirect succeeds (creates ok.txt), but the second points + // outside the sandbox. The command should not run, and ok.txt should + // remain empty (created and closed) since the second redirect aborts + // before echo executes. + other := t.TempDir() + stderr := other + "/blocked.log" + stdout, errOut, code := redirRun(t, "echo hi > ok.txt 2> "+stderr, allowed) + assert.Equal(t, 1, code) + assert.Empty(t, stdout) + assert.Contains(t, errOut, "permission denied") + + got, err := os.ReadFile(filepath.Join(allowed, "ok.txt")) + require.NoError(t, err) + assert.Empty(t, string(got), "echo should not have written to ok.txt") +} + +func TestRedirSandboxBlockedNoFileCreated(t *testing.T) { + dir := t.TempDir() + // No allowed paths configured — every write must fail. + stdout, stderr, code := redirRunNoAllowed(t, "echo evil > evil.txt", dir) + assert.Equal(t, 1, code) + assert.Empty(t, stdout) + assert.Contains(t, stderr, "permission denied") + + _, err := os.Stat(filepath.Join(dir, "evil.txt")) + assert.True(t, os.IsNotExist(err), "no file should have been created, got err=%v", err) +} diff --git a/interp/validate.go b/interp/validate.go index e04cfcb3..62ba35c5 100644 --- a/interp/validate.go +++ b/interp/validate.go @@ -212,26 +212,11 @@ func validateRedirect(rd *syntax.Redirect) error { return fmt.Errorf("%s< input fd redirection is not supported", rd.N.Value) } return nil - case syntax.RdrOut, syntax.ClbOut: - if redirectTargetIsDevNull(rd) { - return nil - } - return fmt.Errorf("> file redirection is not supported") - case syntax.AppOut: - if redirectTargetIsDevNull(rd) { - return nil - } - return fmt.Errorf(">> file redirection is not supported") - case syntax.RdrAll: - if redirectTargetIsDevNull(rd) { - return nil - } - return fmt.Errorf("&> file redirection is not supported") - case syntax.AppAll: - if redirectTargetIsDevNull(rd) { - return nil - } - return fmt.Errorf("&>> file redirection is not supported") + case syntax.RdrOut, syntax.ClbOut, syntax.AppOut, syntax.RdrAll, syntax.AppAll: + // File-target output redirections are accepted at the parser level; + // the runtime opens the target via the AllowedPaths sandbox and + // rejects fds other than 1 (stdout) or 2 (stderr). + return nil case syntax.RdrInOut: return fmt.Errorf("<> file redirection is not supported") case syntax.DplOut: @@ -245,30 +230,6 @@ func validateRedirect(rd *syntax.Redirect) error { return nil } -// redirectTargetIsDevNull reports whether the redirect word is the literal -// path /dev/null (or os.DevNull on Windows). Only simple literal words are -// accepted — variable expansions, globs, and other dynamic forms are rejected -// so that the target cannot be manipulated at runtime. The source fd (rd.N) -// must also be a supported fd (1 or 2). -func redirectTargetIsDevNull(rd *syntax.Redirect) bool { - // Check source fd: only 1 (stdout) and 2 (stderr) are supported. - // rd.N is nil when no explicit fd is given (defaults to stdout). - // For RdrAll/AppAll (&>/&>>), rd.N is always nil since bash does - // not allow an explicit fd prefix on these ops, so this check is - // a no-op for them. - if rd.N != nil && rd.N.Value != "1" && rd.N.Value != "2" { - return false - } - if rd.Word == nil || len(rd.Word.Parts) != 1 { - return false - } - lit, ok := rd.Word.Parts[0].(*syntax.Lit) - if !ok { - return false - } - return isDevNull(lit.Value) -} - // redirectTargetIsFD reports whether the DplOut (>&N) redirect uses only // supported file descriptors (1 and 2 for stdout/stderr). Both the source // fd (rd.N, defaulting to 1) and target fd (rd.Word) must be 1 or 2. diff --git a/tests/scenarios/shell/blocked_redirects/append_all.yaml b/tests/scenarios/shell/blocked_redirects/append_all.yaml index a5cf0355..8840d230 100644 --- a/tests/scenarios/shell/blocked_redirects/append_all.yaml +++ b/tests/scenarios/shell/blocked_redirects/append_all.yaml @@ -1,11 +1,11 @@ -# skip: redirect type is intentionally blocked in the restricted shell +# skip: sandbox blocks writes to paths outside AllowedPaths; bash has no equivalent restriction skip_assert_against_bash: true -description: Append all (&>>) is not supported. +description: Append all (&>>) is blocked when target is outside AllowedPaths. input: script: |+ echo hello &>> output.txt expect: stdout: "" stderr: |+ - &>> file redirection is not supported - exit_code: 2 + open output.txt: permission denied + exit_code: 1 diff --git a/tests/scenarios/shell/blocked_redirects/append_redirect_blocked.yaml b/tests/scenarios/shell/blocked_redirects/append_redirect_blocked.yaml index 6f2416be..02987199 100644 --- a/tests/scenarios/shell/blocked_redirects/append_redirect_blocked.yaml +++ b/tests/scenarios/shell/blocked_redirects/append_redirect_blocked.yaml @@ -1,6 +1,6 @@ -# skip: file redirection is intentionally blocked in the restricted shell +# skip: sandbox blocks writes to paths outside AllowedPaths; bash has no equivalent restriction skip_assert_against_bash: true -description: Append redirection with variable filename is blocked. +description: Append redirection with variable filename is blocked when outside AllowedPaths. input: script: |+ F=file.txt @@ -8,5 +8,5 @@ input: expect: stdout: "" stderr: |+ - >> file redirection is not supported - exit_code: 2 + open file.txt: permission denied + exit_code: 1 diff --git a/tests/scenarios/shell/blocked_redirects/blocked_after_valid.yaml b/tests/scenarios/shell/blocked_redirects/blocked_after_valid.yaml index 6b8805e9..3228c04f 100644 --- a/tests/scenarios/shell/blocked_redirects/blocked_after_valid.yaml +++ b/tests/scenarios/shell/blocked_redirects/blocked_after_valid.yaml @@ -1,13 +1,15 @@ -# skip: redirect type is intentionally blocked in the restricted shell +# skip: sandbox redirect support is rshell-specific; bash has no AllowedPaths restriction skip_assert_against_bash: true -description: Write redirect after valid commands still causes rejection. +description: A blocked redirect fails the offending command but does not abort the script. input: script: |+ echo before echo data > output.txt echo after expect: - stdout: "" + stdout: |+ + before + after stderr: |+ - > file redirection is not supported - exit_code: 2 + open output.txt: permission denied + exit_code: 0 diff --git a/tests/scenarios/shell/blocked_redirects/output_redirect_variable.yaml b/tests/scenarios/shell/blocked_redirects/output_redirect_variable.yaml index b218c98d..c692b9da 100644 --- a/tests/scenarios/shell/blocked_redirects/output_redirect_variable.yaml +++ b/tests/scenarios/shell/blocked_redirects/output_redirect_variable.yaml @@ -1,6 +1,6 @@ -# skip: file redirection is intentionally blocked in the restricted shell +# skip: sandbox blocks writes to paths outside AllowedPaths; bash has no equivalent restriction skip_assert_against_bash: true -description: Output redirection with variable filename is blocked. +description: Output redirection with variable filename is blocked when target is outside AllowedPaths. input: script: |+ F=out.txt @@ -8,5 +8,5 @@ input: expect: stdout: "" stderr: |+ - > file redirection is not supported - exit_code: 2 + open out.txt: permission denied + exit_code: 1 diff --git a/tests/scenarios/shell/blocked_redirects/stderr_write.yaml b/tests/scenarios/shell/blocked_redirects/stderr_write.yaml index 70afe4c5..6389b100 100644 --- a/tests/scenarios/shell/blocked_redirects/stderr_write.yaml +++ b/tests/scenarios/shell/blocked_redirects/stderr_write.yaml @@ -1,11 +1,11 @@ -# skip: redirect type is intentionally blocked in the restricted shell +# skip: sandbox blocks writes to paths outside AllowedPaths; bash has no equivalent restriction skip_assert_against_bash: true -description: Stderr redirection (2>) is not supported. +description: Stderr redirection (2>) is blocked when target is outside AllowedPaths. input: script: |+ echo hello 2> errors.txt expect: stdout: "" stderr: |+ - > file redirection is not supported - exit_code: 2 + open errors.txt: permission denied + exit_code: 1 diff --git a/tests/scenarios/shell/blocked_redirects/variable_redirect_target.yaml b/tests/scenarios/shell/blocked_redirects/variable_redirect_target.yaml index d6f38c76..e1e9ab5a 100644 --- a/tests/scenarios/shell/blocked_redirects/variable_redirect_target.yaml +++ b/tests/scenarios/shell/blocked_redirects/variable_redirect_target.yaml @@ -1,11 +1,12 @@ -# skip: redirect type is intentionally blocked in the restricted shell +# skip: sandbox redirect support is rshell-specific; bash has no AllowedPaths restriction skip_assert_against_bash: true -description: Variable expansion in redirect target is blocked (even if it resolves to /dev/null). +description: Variable expansion in redirect target works at runtime; sandbox blocks writes outside AllowedPaths. input: script: |+ - TARGET=/dev/null; echo hello > $TARGET + TARGET=out.txt + echo hello > $TARGET expect: stdout: "" stderr: |+ - > file redirection is not supported - exit_code: 2 + open out.txt: permission denied + exit_code: 1 diff --git a/tests/scenarios/shell/blocked_redirects/write_all.yaml b/tests/scenarios/shell/blocked_redirects/write_all.yaml index 5b52ec70..825bbb09 100644 --- a/tests/scenarios/shell/blocked_redirects/write_all.yaml +++ b/tests/scenarios/shell/blocked_redirects/write_all.yaml @@ -1,11 +1,11 @@ -# skip: redirect type is intentionally blocked in the restricted shell +# skip: sandbox blocks writes to paths outside AllowedPaths; bash has no equivalent restriction skip_assert_against_bash: true -description: Redirect all (&>) is not supported. +description: Redirect all (&>) is blocked when target is outside AllowedPaths. input: script: |+ echo hello &> output.txt expect: stdout: "" stderr: |+ - &> file redirection is not supported - exit_code: 2 + open output.txt: permission denied + exit_code: 1 diff --git a/tests/scenarios/shell/blocked_redirects/write_append.yaml b/tests/scenarios/shell/blocked_redirects/write_append.yaml index 97adc6d9..1065c4bb 100644 --- a/tests/scenarios/shell/blocked_redirects/write_append.yaml +++ b/tests/scenarios/shell/blocked_redirects/write_append.yaml @@ -1,11 +1,11 @@ -# skip: redirect type is intentionally blocked in the restricted shell +# skip: sandbox blocks writes to paths outside AllowedPaths; bash has no equivalent restriction skip_assert_against_bash: true -description: Append redirection (>>) is not supported. +description: Append redirection (>>) is blocked when target is outside AllowedPaths. input: script: |+ echo hello >> output.txt expect: stdout: "" stderr: |+ - >> file redirection is not supported - exit_code: 2 + open output.txt: permission denied + exit_code: 1 diff --git a/tests/scenarios/shell/blocked_redirects/write_clobber.yaml b/tests/scenarios/shell/blocked_redirects/write_clobber.yaml index 60bc1f02..f00555cf 100644 --- a/tests/scenarios/shell/blocked_redirects/write_clobber.yaml +++ b/tests/scenarios/shell/blocked_redirects/write_clobber.yaml @@ -1,11 +1,11 @@ -# skip: redirect type is intentionally blocked in the restricted shell +# skip: sandbox blocks writes to paths outside AllowedPaths; bash has no equivalent restriction skip_assert_against_bash: true -description: Clobber redirection (>|) is not supported. +description: Clobber redirection (>|) is blocked when target is outside AllowedPaths. input: script: |+ echo hello >| output.txt expect: stdout: "" stderr: |+ - > file redirection is not supported - exit_code: 2 + open output.txt: permission denied + exit_code: 1 diff --git a/tests/scenarios/shell/blocked_redirects/write_truncate.yaml b/tests/scenarios/shell/blocked_redirects/write_truncate.yaml index 0498ccb4..071f0d0d 100644 --- a/tests/scenarios/shell/blocked_redirects/write_truncate.yaml +++ b/tests/scenarios/shell/blocked_redirects/write_truncate.yaml @@ -1,11 +1,11 @@ -# skip: redirect type is intentionally blocked in the restricted shell +# skip: sandbox blocks writes to paths outside AllowedPaths; bash has no equivalent restriction skip_assert_against_bash: true -description: Output redirection (>) is not supported. +description: Output redirection (>) is blocked when target is outside AllowedPaths. input: script: |+ echo hello > output.txt expect: stdout: "" stderr: |+ - > file redirection is not supported - exit_code: 2 + open output.txt: permission denied + exit_code: 1 diff --git a/tests/scenarios/shell/redirections/devnull/devnull_path_traversal_blocked.yaml b/tests/scenarios/shell/redirections/devnull/devnull_path_traversal_blocked.yaml index 23eb8ec0..6974c931 100644 --- a/tests/scenarios/shell/redirections/devnull/devnull_path_traversal_blocked.yaml +++ b/tests/scenarios/shell/redirections/devnull/devnull_path_traversal_blocked.yaml @@ -1,11 +1,11 @@ -# skip: redirect restrictions are an rshell-specific security feature +# skip: sandbox redirect support is rshell-specific; bash has no AllowedPaths restriction skip_assert_against_bash: true -description: Path traversal via /dev/null/../../etc is blocked (not literal /dev/null). +description: Path traversal via /dev/null/.. is not recognized as /dev/null and the sandbox blocks the open. input: script: |+ echo hello > /dev/null/../../../tmp/evil expect: stdout: "" stderr: |+ - > file redirection is not supported - exit_code: 2 + open /dev/null/../../../tmp/evil: permission denied + exit_code: 1 diff --git a/tests/scenarios/shell/redirections/devnull/redirect_to_file_still_blocked.yaml b/tests/scenarios/shell/redirections/devnull/redirect_to_file_still_blocked.yaml index d6bc61ae..503868cb 100644 --- a/tests/scenarios/shell/redirections/devnull/redirect_to_file_still_blocked.yaml +++ b/tests/scenarios/shell/redirections/devnull/redirect_to_file_still_blocked.yaml @@ -1,11 +1,11 @@ -# skip: redirect restrictions are an rshell-specific security feature +# skip: sandbox redirect support is rshell-specific; bash has no AllowedPaths restriction skip_assert_against_bash: true -description: Output redirection to a real file (not /dev/null) is still blocked. +description: Output redirection to a path outside AllowedPaths is blocked at runtime. input: script: |+ echo hello > /tmp/output.txt expect: stdout: "" stderr: |+ - > file redirection is not supported - exit_code: 2 + open /tmp/output.txt: permission denied + exit_code: 1 diff --git a/tests/scenarios/shell/redirections/devnull/stderr_redirect_to_file_blocked.yaml b/tests/scenarios/shell/redirections/devnull/stderr_redirect_to_file_blocked.yaml index 4ff81815..a84bd8ca 100644 --- a/tests/scenarios/shell/redirections/devnull/stderr_redirect_to_file_blocked.yaml +++ b/tests/scenarios/shell/redirections/devnull/stderr_redirect_to_file_blocked.yaml @@ -1,11 +1,11 @@ -# skip: redirect restrictions are an rshell-specific security feature +# skip: sandbox redirect support is rshell-specific; bash has no AllowedPaths restriction skip_assert_against_bash: true -description: Stderr redirection to a real file (not /dev/null) is still blocked. +description: Stderr redirection to a path outside AllowedPaths is blocked at runtime. input: script: |+ echo hello 2> /tmp/errors.txt expect: stdout: "" stderr: |+ - > file redirection is not supported - exit_code: 2 + open /tmp/errors.txt: permission denied + exit_code: 1 diff --git a/tests/scenarios/shell/redirections/file_target/append.yaml b/tests/scenarios/shell/redirections/file_target/append.yaml new file mode 100644 index 00000000..b1d10d38 --- /dev/null +++ b/tests/scenarios/shell/redirections/file_target/append.yaml @@ -0,0 +1,16 @@ +description: Append redirect (>>) appends to an existing file inside AllowedPaths. +setup: + files: + - path: out.txt + content: "first\n" +input: + allowed_paths: ["$DIR"] + script: |+ + echo second >> out.txt + cat out.txt +expect: + stdout: |+ + first + second + stderr: "" + exit_code: 0 diff --git a/tests/scenarios/shell/redirections/file_target/both_streams_append.yaml b/tests/scenarios/shell/redirections/file_target/both_streams_append.yaml new file mode 100644 index 00000000..57a1622a --- /dev/null +++ b/tests/scenarios/shell/redirections/file_target/both_streams_append.yaml @@ -0,0 +1,16 @@ +description: '&>> appends both stdout and stderr to the same file.' +setup: + files: + - path: combined.log + content: "header\n" +input: + allowed_paths: ["$DIR"] + script: |+ + echo body &>> combined.log + cat combined.log +expect: + stdout: |+ + header + body + stderr: "" + exit_code: 0 diff --git a/tests/scenarios/shell/redirections/file_target/both_streams_truncate.yaml b/tests/scenarios/shell/redirections/file_target/both_streams_truncate.yaml new file mode 100644 index 00000000..b68cf5c0 --- /dev/null +++ b/tests/scenarios/shell/redirections/file_target/both_streams_truncate.yaml @@ -0,0 +1,11 @@ +description: '&> sends both stdout and stderr to the same file.' +input: + allowed_paths: ["$DIR"] + script: |+ + echo on-stdout &> combined.log + cat combined.log +expect: + stdout: |+ + on-stdout + stderr: "" + exit_code: 0 diff --git a/tests/scenarios/shell/redirections/file_target/heredoc_to_file.yaml b/tests/scenarios/shell/redirections/file_target/heredoc_to_file.yaml new file mode 100644 index 00000000..7a21f652 --- /dev/null +++ b/tests/scenarios/shell/redirections/file_target/heredoc_to_file.yaml @@ -0,0 +1,15 @@ +description: Heredoc stdin combines with file-target stdout redirect on the same command. +input: + allowed_paths: ["$DIR"] + script: |+ + cat < greeting.txt + hello + world + EOF + cat greeting.txt +expect: + stdout: |+ + hello + world + stderr: "" + exit_code: 0 diff --git a/tests/scenarios/shell/redirections/file_target/outside_allowed_paths.yaml b/tests/scenarios/shell/redirections/file_target/outside_allowed_paths.yaml new file mode 100644 index 00000000..5996717d --- /dev/null +++ b/tests/scenarios/shell/redirections/file_target/outside_allowed_paths.yaml @@ -0,0 +1,16 @@ +# skip: sandbox restriction is rshell-specific; bash has no AllowedPaths concept +skip_assert_against_bash: true +description: Redirect target outside AllowedPaths is blocked at runtime by the sandbox. +setup: + files: + - path: inside/.keep + content: "" +input: + allowed_paths: ["inside"] + script: |+ + echo hi > outside.txt +expect: + stdout: "" + stderr: |+ + open outside.txt: permission denied + exit_code: 1 diff --git a/tests/scenarios/shell/redirections/file_target/path_traversal_blocked.yaml b/tests/scenarios/shell/redirections/file_target/path_traversal_blocked.yaml new file mode 100644 index 00000000..0893f5c8 --- /dev/null +++ b/tests/scenarios/shell/redirections/file_target/path_traversal_blocked.yaml @@ -0,0 +1,16 @@ +# skip: sandbox restriction is rshell-specific; bash has no AllowedPaths concept +skip_assert_against_bash: true +description: A redirect target that escapes AllowedPaths via .. is blocked. +setup: + files: + - path: inside/.keep + content: "" +input: + allowed_paths: ["inside"] + script: |+ + echo hi > inside/../../etc/foo +expect: + stdout: "" + stderr_contains: + - "permission denied" + exit_code: 1 diff --git a/tests/scenarios/shell/redirections/file_target/stderr_only.yaml b/tests/scenarios/shell/redirections/file_target/stderr_only.yaml new file mode 100644 index 00000000..0738207e --- /dev/null +++ b/tests/scenarios/shell/redirections/file_target/stderr_only.yaml @@ -0,0 +1,13 @@ +description: Stderr-only redirect (2>) captures stderr without touching stdout. +input: + allowed_paths: ["$DIR"] + script: |+ + cat missing 2> err.log + echo -- + cat err.log +expect: + stdout_contains: + - "--" + - "such file or directory" + stderr: "" + exit_code: 0 diff --git a/tests/scenarios/shell/redirections/file_target/stderr_to_stdout_then_file.yaml b/tests/scenarios/shell/redirections/file_target/stderr_to_stdout_then_file.yaml new file mode 100644 index 00000000..ced82669 --- /dev/null +++ b/tests/scenarios/shell/redirections/file_target/stderr_to_stdout_then_file.yaml @@ -0,0 +1,11 @@ +description: Combining > file with 2>&1 sends both streams into the same file. +input: + allowed_paths: ["$DIR"] + script: |+ + cat missing > combined.log 2>&1 + cat combined.log +expect: + stdout_contains: + - "such file or directory" + stderr: "" + exit_code: 0 diff --git a/tests/scenarios/shell/redirections/file_target/truncate.yaml b/tests/scenarios/shell/redirections/file_target/truncate.yaml new file mode 100644 index 00000000..e0b9d353 --- /dev/null +++ b/tests/scenarios/shell/redirections/file_target/truncate.yaml @@ -0,0 +1,11 @@ +description: Truncating output redirect (>) writes a file inside AllowedPaths. +input: + allowed_paths: ["$DIR"] + script: |+ + echo hi > out.txt + cat out.txt +expect: + stdout: |+ + hi + stderr: "" + exit_code: 0 diff --git a/tests/scenarios/shell/redirections/file_target/truncate_overwrites.yaml b/tests/scenarios/shell/redirections/file_target/truncate_overwrites.yaml new file mode 100644 index 00000000..129c6d4d --- /dev/null +++ b/tests/scenarios/shell/redirections/file_target/truncate_overwrites.yaml @@ -0,0 +1,15 @@ +description: Truncating output redirect overwrites an existing file. +setup: + files: + - path: out.txt + content: "old contents that are longer than the new ones" +input: + allowed_paths: ["$DIR"] + script: |+ + echo new > out.txt + cat out.txt +expect: + stdout: |+ + new + stderr: "" + exit_code: 0 From 8541506d2bedca7dee561130d4db2313b8bebcf3 Mon Sep 17 00:00:00 2001 From: Jules Macret Date: Thu, 30 Apr 2026 15:08:58 +0200 Subject: [PATCH 2/5] fix(interp): reject non-regular file-target redirects before opening MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A FIFO inside AllowedPaths would hang the script during redirect setup: opening a named pipe with O_WRONLY blocks until a reader connects, and the runner has no opportunity to observe context cancellation while it's stuck inside the open syscall. Pre-PR, validation rejected the entire class so this was unreachable. Stat the target through the sandbox (openat-based, never blocks) before opening for output. Reject non-regular files with a clear error; ENOENT and other Stat errors are ignored — the file will be created by O_CREATE or the open will surface the genuine failure. The same guard covers 2>file and &>file paths. Adds a unix-only test that bounds the redirect with a timeout so any future regression that reintroduces the hang fails fast instead of stalling the suite. Co-Authored-By: Claude Opus 4.7 (1M context) --- interp/runner_redir.go | 44 +++++++++++ interp/tests/redir_file_target_unix_test.go | 85 +++++++++++++++++++++ 2 files changed, 129 insertions(+) create mode 100644 interp/tests/redir_file_target_unix_test.go diff --git a/interp/runner_redir.go b/interp/runner_redir.go index 502d81b7..1fd54abe 100644 --- a/interp/runner_redir.go +++ b/interp/runner_redir.go @@ -200,6 +200,44 @@ func (r *Runner) hdocReader(ctx context.Context, rd *syntax.Redirect) (*os.File, return pr, nil } +// rejectNonRegularRedirectTarget guards against opening a non-regular +// redirect target (FIFO, socket, char/block device) for output. Opening +// a FIFO with O_WRONLY blocks until a reader connects, which would hang +// the script during redirect setup before the command runs and before +// context cancellation can fire. Sandbox.Stat is metadata-only (openat- +// based) and never blocks. +// +// /dev/null is handled by callers via the io.Discard fast path and never +// reaches this helper. ENOENT and other Stat errors are ignored — if the +// file does not exist, O_CREATE will create a regular file; any genuine +// failure (permission denied, etc.) will surface from the subsequent +// Open call. +// +// When a custom openHandler is installed (r.sandbox is nil), the caller +// is responsible for the file-type check; we skip the guard rather than +// silently misbehave. +// +// There is a TOCTOU window between Stat and Open. It is not a sandbox- +// escape concern: the sandbox enforces that the path stays within +// AllowedPaths, and an attacker who can swap a regular file for a FIFO +// already has write access to the parent directory. The check defends +// against accidental hangs on operator-created FIFOs in the common case. +func (r *Runner) rejectNonRegularRedirectTarget(path string) error { + if r.sandbox == nil { + return nil + } + info, err := r.sandbox.Stat(path, r.Dir) + if err != nil { + return nil + } + if info.Mode().IsRegular() { + return nil + } + werr := fmt.Errorf("open %s: not a regular file", path) + r.errf("%v\n", werr) + return werr +} + func (r *Runner) redir(ctx context.Context, rd *syntax.Redirect) (io.Closer, error) { if rd.Hdoc != nil { pr, err := r.hdocReader(ctx, rd) @@ -253,6 +291,9 @@ func (r *Runner) redir(ctx context.Context, rd *syntax.Redirect) (io.Closer, err *orig = io.Discard return nil, nil } + if err := r.rejectNonRegularRedirectTarget(arg); err != nil { + return nil, err + } flags := os.O_WRONLY | os.O_CREATE | os.O_TRUNC if rd.Op == syntax.AppOut { flags = os.O_WRONLY | os.O_CREATE | os.O_APPEND @@ -273,6 +314,9 @@ func (r *Runner) redir(ctx context.Context, rd *syntax.Redirect) (io.Closer, err r.stderr = io.Discard return nil, nil } + if err := r.rejectNonRegularRedirectTarget(arg); err != nil { + return nil, err + } flags := os.O_WRONLY | os.O_CREATE | os.O_TRUNC if rd.Op == syntax.AppAll { flags = os.O_WRONLY | os.O_CREATE | os.O_APPEND diff --git a/interp/tests/redir_file_target_unix_test.go b/interp/tests/redir_file_target_unix_test.go new file mode 100644 index 00000000..43c526d1 --- /dev/null +++ b/interp/tests/redir_file_target_unix_test.go @@ -0,0 +1,85 @@ +// 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. + +//go:build !windows + +package tests_test + +import ( + "path/filepath" + "syscall" + "testing" + "time" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +// Opening a FIFO with O_WRONLY blocks until a reader connects. Without the +// pre-open type guard, redirecting to a FIFO inside AllowedPaths would hang +// the script during redirect setup before any command runs and before the +// runner could observe context cancellation. Sandbox.Stat is openat-based +// and never blocks, so we use it to reject non-regular targets before the +// blocking open happens. + +func TestRedirToFifoRejectedFastForOutput(t *testing.T) { + dir := t.TempDir() + fifo := filepath.Join(dir, "fifo") + require.NoError(t, syscall.Mkfifo(fifo, 0644)) + + // Bound the test so a regression (a hang) fails loudly instead of + // stalling the whole suite. The check itself is openat-only and + // returns synchronously well within this budget. + done := make(chan struct{}) + var stdout, stderr string + var code int + go func() { + defer close(done) + stdout, stderr, code = redirRun(t, "echo hi > fifo", dir) + }() + select { + case <-done: + case <-time.After(2 * time.Second): + t.Fatal("redirect to FIFO hung; pre-open type guard regressed") + } + + assert.Equal(t, 1, code) + assert.Empty(t, stdout) + assert.Contains(t, stderr, "not a regular file") +} + +func TestRedirToFifoRejectedFastForBothStreams(t *testing.T) { + dir := t.TempDir() + fifo := filepath.Join(dir, "fifo") + require.NoError(t, syscall.Mkfifo(fifo, 0644)) + + done := make(chan struct{}) + var stdout, stderr string + var code int + go func() { + defer close(done) + stdout, stderr, code = redirRun(t, "echo hi &> fifo", dir) + }() + select { + case <-done: + case <-time.After(2 * time.Second): + t.Fatal("&> redirect to FIFO hung; pre-open type guard regressed") + } + + assert.Equal(t, 1, code) + assert.Empty(t, stdout) + assert.Contains(t, stderr, "not a regular file") +} + +// A baseline: when there is no FIFO on the path, the same code path must +// still work — i.e. the pre-open Stat must not gratuitously fail on a +// non-existent target. +func TestRedirToMissingFileStillWorks(t *testing.T) { + dir := t.TempDir() + stdout, stderr, code := redirRun(t, "echo hi > new.txt", dir) + assert.Equal(t, 0, code) + assert.Empty(t, stdout) + assert.Empty(t, stderr) +} From 3ad84658b0716d06dcb724c33c917ea7a7ebd61d Mon Sep 17 00:00:00 2001 From: Jules Macret Date: Thu, 30 Apr 2026 15:25:47 +0200 Subject: [PATCH 3/5] fix(interp): reject unsupported redirect fds before expanding the word MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Output redirects with an unsupported fd (anything other than 1 or 2) were rejected at runtime, but the rejection happened after r.literal had already expanded the redirect word. A command substitution in the target — e.g. 3>$(cmd) — would execute cmd for its side effects only to have its output discarded. Pre-PR validation rejected fd 3+ before any expansion ran, so this PR regressed that property. Move the fd-N check above r.literal. rd.N is a parser literal, not an expansion, so it can be inspected without driving any expansion side effects. Same guard applies to fd 0 on output ops. Co-Authored-By: Claude Opus 4.7 (1M context) --- interp/runner_redir.go | 8 ++++++-- interp/tests/redir_file_target_test.go | 27 ++++++++++++++++++++++++++ 2 files changed, 33 insertions(+), 2 deletions(-) diff --git a/interp/runner_redir.go b/interp/runner_redir.go index 1fd54abe..556fa9c8 100644 --- a/interp/runner_redir.go +++ b/interp/runner_redir.go @@ -257,9 +257,11 @@ func (r *Runner) redir(ctx context.Context, rd *syntax.Redirect) (io.Closer, err return pr, nil } - arg := r.literal(rd.Word) - // Determine which fd this redirect targets (default: stdout for output ops). + // This check runs BEFORE expanding the redirect word so that an unsupported + // fd rejects the redirect without triggering any command substitution or + // other expansion side effects in the target. rd.N is a parser literal, + // not an expansion, so this is safe to inspect early. orig := &r.stdout if rd.N != nil { switch rd.N.Value { @@ -279,6 +281,8 @@ func (r *Runner) redir(ctx context.Context, rd *syntax.Redirect) (io.Closer, err } } + arg := r.literal(rd.Word) + switch rd.Op { case syntax.RdrIn: // done further below diff --git a/interp/tests/redir_file_target_test.go b/interp/tests/redir_file_target_test.go index accf65a9..0bf93dd3 100644 --- a/interp/tests/redir_file_target_test.go +++ b/interp/tests/redir_file_target_test.go @@ -151,3 +151,30 @@ func TestRedirSandboxBlockedNoFileCreated(t *testing.T) { _, err := os.Stat(filepath.Join(dir, "evil.txt")) assert.True(t, os.IsNotExist(err), "no file should have been created, got err=%v", err) } + +// Unsupported fds (anything other than 1 or 2 for output, 0 for input) must +// be rejected before the redirect word is expanded, otherwise a command +// substitution in an invalid-fd redirect would execute its body for its +// side effects only to be discarded. +func TestRedirUnsupportedFdRejectedBeforeExpansion(t *testing.T) { + dir := t.TempDir() + // If fd 3 were rejected after expansion, the command substitution + // would run and write SIDE-EFFECT to stderr. We assert the opposite: + // nothing on stderr besides the unsupported-fd error. + stdout, stderr, code := redirRun(t, "echo x 3>$(echo SIDE-EFFECT >&2; echo out)", dir) + assert.Equal(t, 1, code) + assert.Empty(t, stdout) + assert.NotContains(t, stderr, "SIDE-EFFECT", "command substitution must not run for an unsupported fd") + assert.Contains(t, stderr, "3: unsupported fd") +} + +func TestRedirInputFdOnOutputRejectedBeforeExpansion(t *testing.T) { + dir := t.TempDir() + // fd 0 on an output op is rejected. The command substitution must not + // run before that rejection. + stdout, stderr, code := redirRun(t, "echo x 0>$(echo SIDE-EFFECT >&2; echo out)", dir) + assert.Equal(t, 1, code) + assert.Empty(t, stdout) + assert.NotContains(t, stderr, "SIDE-EFFECT") + assert.Contains(t, stderr, "0: unsupported fd") +} From 8ae8c0efe2a1907f92503e46da8c555e3e33509b Mon Sep 17 00:00:00 2001 From: Jules Macret Date: Thu, 30 Apr 2026 15:50:01 +0200 Subject: [PATCH 4/5] test(interp): shell-quote temp paths embedded in redirect test scripts MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit t.TempDir() returns backslash paths on Windows (C:\...), and concatenating them raw into a shell command leaves the backslashes for the parser to consume as escape characters — the redirect target ends up pointing at the wrong file. Single-quoting the path with the standard '\'' idiom defuses backslashes plus any other shell metacharacters that could appear in temp directory paths (spaces, $). Also replace a stray forward-slash concatenation in TestRedirFailureDoesNotRunCommand with filepath.Join, which was hiding the same Windows-correctness issue. docs/RULES.md requires cross-platform path handling in tests. Co-Authored-By: Claude Opus 4.7 (1M context) --- interp/tests/redir_file_target_test.go | 18 +++++++++++++++--- 1 file changed, 15 insertions(+), 3 deletions(-) diff --git a/interp/tests/redir_file_target_test.go b/interp/tests/redir_file_target_test.go index 0bf93dd3..8f1cc514 100644 --- a/interp/tests/redir_file_target_test.go +++ b/interp/tests/redir_file_target_test.go @@ -9,6 +9,7 @@ import ( "os" "path/filepath" "runtime" + "strings" "testing" "github.com/stretchr/testify/assert" @@ -21,6 +22,17 @@ import ( // command runs (so reads from disk see the written bytes), that file mode // is 0644, and that the sandbox boundary is enforced when the open fails. +// shQuote single-quotes a path for safe inclusion in a shell command, +// escaping any embedded single quotes with the standard '\” idiom. This +// is necessary on Windows where t.TempDir() returns paths with backslashes +// — without quoting, the shell parser would consume the backslashes as +// escape characters and the redirect target would not match the intended +// path. Single quotes also defuse any other shell metacharacters that +// could appear in paths (spaces, $, etc.). +func shQuote(s string) string { + return "'" + strings.ReplaceAll(s, "'", `'\''`) + "'" +} + func TestRedirTruncateWritesAndCloses(t *testing.T) { dir := t.TempDir() stdout, stderr, code := redirRun(t, "echo hi > out.txt", dir) @@ -113,7 +125,7 @@ func TestRedirOutsideAllowedPathsDoesNotCreateFile(t *testing.T) { other := t.TempDir() target := filepath.Join(other, "evil.txt") - stdout, stderr, code := redirRun(t, "echo hi > "+target, allowed) + stdout, stderr, code := redirRun(t, "echo hi > "+shQuote(target), allowed) assert.Equal(t, 1, code) assert.Empty(t, stdout) assert.Contains(t, stderr, "permission denied") @@ -129,8 +141,8 @@ func TestRedirFailureDoesNotRunCommand(t *testing.T) { // remain empty (created and closed) since the second redirect aborts // before echo executes. other := t.TempDir() - stderr := other + "/blocked.log" - stdout, errOut, code := redirRun(t, "echo hi > ok.txt 2> "+stderr, allowed) + blockedTarget := filepath.Join(other, "blocked.log") + stdout, errOut, code := redirRun(t, "echo hi > ok.txt 2> "+shQuote(blockedTarget), allowed) assert.Equal(t, 1, code) assert.Empty(t, stdout) assert.Contains(t, errOut, "permission denied") From 23c16a55edf4f5a61891e39010e503b80ec0614f Mon Sep 17 00:00:00 2001 From: Jules Macret Date: Thu, 30 Apr 2026 16:17:40 +0200 Subject: [PATCH 5/5] chore(analysis): allow os.O_WRONLY/O_CREATE/O_TRUNC/O_APPEND in interp MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The new file-target output redirect path uses these four flag constants in interp/runner_redir.go, but they were not in the interp allowlist — TestInterpAllowedSymbols and TestVerificationInterpCleanPass fail without this addition. The flags are pure integer constants. The actual write capability gate is allowedpaths.Sandbox.Open, which is already authorized; passing O_WRONLY|O_CREATE|O_TRUNC into a non-sandbox path would still be prevented by the lack of any direct os.OpenFile call in interp/. Co-Authored-By: Claude Opus 4.7 (1M context) --- analysis/symbols_interp.go | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/analysis/symbols_interp.go b/analysis/symbols_interp.go index 3da6c1a2..38acdac4 100644 --- a/analysis/symbols_interp.go +++ b/analysis/symbols_interp.go @@ -47,7 +47,11 @@ var interpAllowedSymbols = []string{ "os.File", // 🟠 file handle type; interpreter needs file I/O for redirects and pipes. "os.FileMode", // 🟢 file permission bits type; pure type. "os.Getwd", // 🟠 returns current working directory; read-only. + "os.O_APPEND", // 🟢 append-on-write flag constant; pure integer. Capability gate is allowedpaths.Sandbox.Open, not the flag itself. + "os.O_CREATE", // 🟢 create-if-missing flag constant; pure integer. Capability gate is allowedpaths.Sandbox.Open, not the flag itself. "os.O_RDONLY", // 🟢 read-only file flag constant; pure constant. + "os.O_TRUNC", // 🟢 truncate-on-open flag constant; pure integer. Capability gate is allowedpaths.Sandbox.Open, not the flag itself. + "os.O_WRONLY", // 🟢 write-only file flag constant; pure integer. Capability gate is allowedpaths.Sandbox.Open, not the flag itself. "os.PathError", // 🟢 error type wrapping path and operation; pure type. "os.Pipe", // 🟠 creates an OS pipe pair; needed for shell pipelines. "path/filepath.IsAbs", // 🟢 checks if path is absolute; pure function, no I/O.