feat(interp): wire RemediationMode into file-target output redirections [PR D]#526
Merged
Merged
Conversation
…n series) The sandbox previously rejected every non-O_RDONLY open as a blanket defense. This change replaces that with a precise flag-mask check: only bits in allowedOpenFlags (O_RDONLY|O_WRONLY|O_RDWR|O_APPEND| O_CREATE|O_EXCL|O_TRUNC) are accepted; unknown bits still return ErrPermission. Write opens through os.Root are now allowed for paths inside the allowlist. TOCTOU defense: the cross-root symlink fallback continues to be read-only. A symlink that escapes its os.Root is followed for reads but rejected for writes, preventing a malicious link target from being swapped between resolution and open. Nothing in the interpreter uses write opens yet — output redirections are still blocked at parse time. This is a pure capability layer change; wiring is PR C. Co-Authored-By: Claude Sonnet 4.6 <[email protected]>
…ion-mode-redirects
…ns [PR C] In remediation mode, output redirections (>, >>, 2>, &>, &>>) open through the AllowedPaths sandbox instead of being blocked at parse time. /dev/null is still short-circuited to io.Discard. <> stays blocked in all modes. - interp/validate.go: validateNode/validateRedirect accept remediationMode bool; file-target output redirect cases return nil (pass) when true - interp/api.go: pass r.remediationMode to validateNode - interp/runner_redir.go: in remediation mode, open file targets via the sandbox with O_WRONLY|O_CREATE|O_TRUNC (>) or O_WRONLY|O_CREATE|O_APPEND (>>); add rejectNonRegularRedirectTarget guard to prevent hangs on FIFOs - analysis/symbols_interp.go: add os.O_APPEND, O_CREATE, O_TRUNC, O_WRONLY and io/fs.ModeType to the allowed symbol list - tests/scenarios_test.go: add remediation_mode field to input struct - tests/scenarios/shell/redirections/file_target/: 8 new scenarios covering truncate, overwrite, append, 2>, &>, &>>, sandbox-blocked, and > file 2>&1 Co-Authored-By: Claude Sonnet 4.6 <[email protected]>
3 tasks
julesmcrt
marked this pull request as ready for review
June 12, 2026 11:53
julesmcrt
requested review from
AlexandreYang,
astuyve,
matt-dz,
thieman and
val06
as code owners
June 12, 2026 11:53
…ionMode() with Mode() Makes the Go API mirror the CLI's --mode flag: callers now write interp.Mode(interp.ModeRemediation) instead of the asymmetric interp.RemediationMode(). - type Mode string → type ExecutionMode string (frees "Mode" as a function name) - RemediationMode() RunnerOption → Mode(m ExecutionMode) RunnerOption (validates the value, rejects unknown modes) - cmd/rshell/main.go: interp.Mode(mode) type-conv → interp.ExecutionMode(mode); always pass interp.Mode(opts.mode) so read-only is explicit too - tests/scenarios_test.go: updated call site Co-Authored-By: Claude Sonnet 4.6 <[email protected]>
Proves the mode gate (not just the sandbox) blocks writes: AllowedPaths is configured so the sandbox would permit writes, but without remediation mode the validator rejects >, >>, and &> at parse time (exit 2). Co-Authored-By: Claude Sonnet 4.6 <[email protected]>
julesmcrt
force-pushed
the
jules.macret/sandbox-write-capability
branch
2 times, most recently
from
June 15, 2026 14:02
3f9f2cb to
db47ea4
Compare
julesmcrt
changed the title
Resolve 5 conflicts: - allowedpaths/sandbox.go: take PR C's readOnly default + SetWritable() + tightened allowedOpenFlags (removes O_RDWR/O_EXCL) - allowedpaths/sandbox_test.go: take PR C's TestSandboxDefaultReadOnly and updated SetWritable-gated write tests - analysis/symbols_allowedpaths.go: drop O_EXCL/O_RDWR (removed from sandbox) - interp/api.go: keep PR D's Mode(ExecutionMode) API; add SetWritable() call in New() when remediationMode is true - README.md: keep PR D's AllowedPaths paragraph (includes remediation mode docs) Co-Authored-By: Claude Sonnet 4.6 <[email protected]>
|
os.O_APPEND, os.O_CREATE, os.O_TRUNC, os.O_WRONLY, and io/fs.ModeType were added to the interpAllowedSymbols ceiling but omitted from interpPerModeSymbols["read-only"]. runner_redir.go (not a _remediation.go file) uses all five, so TestInterpPerModeSymbols failed. Add them to the "read-only" list. The runtime guard (r.remediationMode) still prevents actual writes in read-only mode; these are flag constants whose capability gate is allowedpaths.Sandbox.Open. Co-Authored-By: Claude Sonnet 4.6 <[email protected]>
Move write-capable code (openWriteRedirect, openWriteAllRedirect, rejectNonRegularRedirectTarget) out of runner_redir.go and into a new _remediation.go file, so the write-flag symbols (os.O_WRONLY, os.O_CREATE, os.O_TRUNC, os.O_APPEND, io/fs.ModeType, syntax.RedirOperator) can be listed in interpPerModeSymbols["remediation"] only — not in "read-only". This restores the static guarantee that write-capable symbols cannot appear in non-remediation interp/ files. Co-Authored-By: Claude Sonnet 4.6 <[email protected]>
rejectNonRegularRedirectTarget previously returned nil when r.sandbox == nil, silently skipping the check. Opening a FIFO with O_WRONLY blocks until a reader connects, so a sandboxless remediation-mode runner would hang for up to 30s on any named-pipe redirect target. Extract a statFileMode helper that uses r.sandbox.Stat when a sandbox is present, and falls back to os.Stat otherwise. The check now always runs, regardless of whether AllowedPaths is configured. Co-Authored-By: Claude Sonnet 4.6 <[email protected]>
Covers the one file-target redirect operator that had no dedicated test: stderr append (2>>). Follows the same pattern as append.yaml and stderr_redirect.yaml — appends stderr twice and verifies both lines appear. Co-Authored-By: Claude Sonnet 4.6 <[email protected]>
Extract the repeated `redirectTargetIsDevNull(rd) || remediationMode` condition into a redirectAllowed helper, removing the duplication across all four write-redirect cases in validateRedirect (suggested by AlexandreYang). Add clobber.yaml to cover the >| operator, the one file-target redirect with no dedicated scenario. Co-Authored-By: Claude Sonnet 4.6 <[email protected]>
On Windows, Go's filepath treats /tmp/evil.txt as the relative path tmp\evil.txt rather than an absolute path. The open therefore fails with "no such file or directory" before the sandbox can reject it with "permission denied". Add stderr_contains_windows to accept the Windows-specific error while keeping the stricter check on Linux/macOS. Co-Authored-By: Claude Sonnet 4.6 <[email protected]>
SHELL_FEATURES.md still referenced interp.RemediationMode() which was renamed to interp.Mode(interp.ModeRemediation) in this PR. Fix the doc. sandbox.Open() returns ErrPermission immediately when called on a nil receiver (sandbox.go:368-370), so r.open() always fails before touching a FIFO when r.sandbox == nil. The os.Stat fallback in statFileMode was therefore dead code — a nil sandbox already prevents any blocking open. Replace it with an early return of os.ErrNotExist so the FIFO type-check is skipped (the subsequent open failure surfaces the real error), and remove os.Stat from the symbol allowlists. Co-Authored-By: Claude Sonnet 4.6 <[email protected]>
PR #524 reviewer explicitly asked for type Mode with ModeReadOnly and ModeRemediation constants. PR #526 had renamed the type to ExecutionMode to avoid a naming conflict with the Mode() RunnerOption function, but that went against the prior review feedback. Restore type Mode string (as requested in PR #524) and rename the RunnerOption from Mode() to WithMode(), which reads naturally at the call site (interp.WithMode(interp.ModeRemediation)) and eliminates the awkward inversion where the setter and the type had swapped names. Co-Authored-By: Claude Sonnet 4.6 <[email protected]>
Member
|
@codex make comprehensive code and security review |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 9bad06fd86
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Comment on lines
+46
to
+50
| mode, err := r.statFileMode(path) | ||
| if err != nil { | ||
| return nil // ENOENT or other: let Open surface the real error | ||
| } | ||
| if mode&fs.ModeType == 0 { |
There was a problem hiding this comment.
When an allowed directory is writable by another local process, the target can be swapped from a regular file to a FIFO/socket after this Stat check but before openWriteRedirect calls OpenFile with blocking O_WRONLY; the runner can then hang in the open syscall before context cancellation is observed. The comment says this guard prevents FIFO hangs, so the type check needs to be tied to the open itself (for example open nonblocking and fstat/reject non-regular targets) rather than relying on a pre-open stat.
Useful? React with 👍 / 👎.
Collaborator
Author
There was a problem hiding this comment.
The TOCTOU window is documented in the code comment. Plugging it properly would require O_NONBLOCK on the open, but that constant isn't portable across Go's stdlib (no os.O_NONBLOCK). The remaining risk requires a coordinated race from another process that already has write access to the AllowedPaths directory — i.e. the directory the operator trusted enough to configure. Happy to revisit if you feel strongly, but I'd rather not add syscall-level platform ifdefs for this.
matt-dz
approved these changes
Jun 18, 2026
matt-dz
left a comment
matt-dz
left a comment
Collaborator
There was a problem hiding this comment.
overall looks good -- i think some more tests should be added for robustness
- Fix validate.go: reject unsupported fd numbers (3>, 0>) for output redirects before redirectAllowed, so 3>$(cmd) cannot execute the substitution before the fd check fires (exit 2) - Rename scenario field remediation_mode: true to mode: remediation to match the CLI --mode flag and WithMode(ModeRemediation) API - Restructure README.md AllowedPaths paragraph into bullet points - Restructure SHELL_FEATURES.md redirections section into a mode table - Add validate_test.go unit tests for validateRedirect/validateNode - Add runner_redir_remediation_test.go unit tests for write-redirect helpers - Add runner_redir_test.go unit tests for read-only enforcement and fd check - Add unsupported_fd_blocked.yaml scenario covering the fd validation fix Co-Authored-By: Claude Sonnet 4.6 <[email protected]>
Co-Authored-By: Claude Sonnet 4.6 <[email protected]>
AlexandreYang
approved these changes
Jun 19, 2026
gh-worker-dd-mergequeue-cf854d Bot
pushed a commit
to DataDog/datadog-agent
that referenced
this pull request
Jun 22, 2026
## What does this PR do? Adds a new Private Action Runner action **`com.datadoghq.remoteaction.rshell.runRemediationCommand`**, a sibling of the existing `runCommand`. It runs the restricted shell ([rshell](https://github.com/DataDog/rshell)) in **remediation mode** (`interp.ModeRemediation`) instead of read-only mode. Remediation mode (introduced in rshell [#526](DataDog/rshell#526) / [#532](DataDog/rshell#532)) permits: - file-target output redirections — `>`, `>>`, `2>`, `&>`, `&>>` - write-oriented builtins such as `truncate` all still confined to the configured `AllowedPaths` sandbox. ### Changes - Bump `github.com/DataDog/rshell` `v0.0.20` → `v0.0.21`. - Refactor `RunCommandHandler` to be parameterized by `interp.Mode`; `runCommand` keeps read-only behavior, `runRemediationCommand` uses remediation mode. The two actions share all sandboxing/allowlist-intersection logic. - Register `runRemediationCommand` in the rshell bundle. - Unit tests + parallel E2E tests + release note. ## Motivation Enable operator-authorized remediation workflows (writing/truncating files within the sandbox) while keeping `runCommand` strictly read-only. ## Describe how you validated your changes - `dda inv test --targets=./pkg/privateactionrunner/bundles/remoteaction/rshell/` — 126 unit tests pass, including new coverage: - both actions register with the expected mode - remediation: in-sandbox redirect writes the file - read-only: same redirect is rejected, no file created - remediation: out-of-sandbox redirect is still rejected - E2E `tests/privateactionrunner` package compiles; added `TestRshellRemediationWriteFile` (write + read-back) and `TestRshellRunCommandBlocksWrite`. ## Security / rollout notes -⚠️ **Not default-enabled.** `runRemediationCommand` is intentionally left out of `defaultCommonActionFQNs`. Being write-capable, it must be explicitly added to a runner's `actions_allowlist`. - Operators already using a wildcard allowlist entry (e.g. `com.datadoghq.remoteaction.rshell.*` or `com.datadoghq.remoteaction.*`) would pick this action up automatically — worth a security reviewer's attention. - Path/command allowlists (`restricted_shell.allowed_paths` / `allowed_commands`) are **shared** with `runCommand`. A follow-up could add a separate, narrower write-allowlist if desired. ## Possible drawbacks / trade-offs - Shared allowlist means operators can't currently grant broad read paths but narrow write paths in one config. ## Additional Notes Draft — pending review of the default-enablement decision and whether a dedicated remediation allowlist is wanted. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-authored-by: jules.macret <[email protected]>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
PR D of the remediation mode series. Stacks on PR A (#524, merged), PR B (#525, merged), and PR C (#528, merged).
interp/validate.go:validateNode/validateRedirectnow accept aremediationMode bool; file-target output redirect cases (>,>>,&>,&>>) pass validation whentrue.<>stays blocked in all modes.interp/api.go: passesr.remediationModetovalidateNodeinterp/runner_redir.go: in remediation mode, opens file targets through theAllowedPathssandbox withO_WRONLY|O_CREATE|O_TRUNC(for>) orO_WRONLY|O_CREATE|O_APPEND(for>>); addsrejectNonRegularRedirectTargetto prevent hangs on FIFOs;/dev/nullremains short-circuited toio.Discardin all modes; read-only mode defense-in-depth check preservedanalysis/symbols_interp.go: addsos.O_APPEND,os.O_CREATE,os.O_TRUNC,os.O_WRONLY,io/fs.ModeTypeto the allowed symbol listtests/scenarios_test.go: addsremediation_mode: boolfield to theinputstruct; wiresinterp.RemediationMode()when settests/scenarios/shell/redirections/file_target/: 8 new scenarios — truncate, overwrite, append (>>), stderr redirect (2>), both-stream redirect (&>), both-stream append (&>>), sandbox-blocked (target outsideAllowedPaths), and stdout/stderr merge (> file 2>&1)Security properties
AllowedPathsfail withpermission denied(exit 1);/dev/nullalways accepted;<>always blockedTest plan
go test ./interp/... ./tests/... -timeout 180spasses locallytests/scenarios/shell/redirections/file_target/scenarios passblocked_redirects/anddevnull/scenarios unchanged (read-only still works)RSHELL_BASH_TEST=1 go test ./tests/ -run TestShellScenariosAgainstBash -timeout 120sin CI (Docker required)🤖 Generated with Claude Code