feat(interp): route file-target output redirects through AllowedPaths

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.
 

SHELL_FEATURES.md

@@ -72,17 +72,13 @@ Blocked features are rejected before execution with exit code 2.
 - ✅ `<` — input redirection (read-only, within AllowedPaths)
 - ✅ `<<DELIM` — heredoc
 - ✅ `<<-DELIM` — heredoc with tab stripping
-- ✅ `>/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

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.

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)
@@ -219,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 {
@@ -241,31 +281,57 @@ 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
 
 	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
+		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
+		}
+		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
+		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
+		}
+		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 {

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 ---

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 ---