From 1d1f480b855fbe04e4ac102cb6400c85540e9317 Mon Sep 17 00:00:00 2001 From: Jules Macret Date: Thu, 30 Apr 2026 14:00:55 +0200 Subject: [PATCH] feat(allowedpaths): permit writes through sandbox open MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Relax Sandbox.Open from read-only to read+write. The underlying os.Root.OpenFile already validates writes safely via openat, so the sandbox needed only to widen its flag check rather than build new machinery. The cross-root symlink fallback stays read-only — following a symlink that escapes its os.Root and then performing O_CREATE/O_TRUNC is the classic TOCTOU footgun, where the link target can be swapped between resolution and open. Writes therefore stay inside a single os.Root. Open flags are also tightened to an explicit allowlist (O_RDONLY|O_WRONLY|O_RDWR|O_APPEND|O_CREATE|O_EXCL|O_TRUNC) so unknown or platform-specific bits cannot slip through. This is the foundation of a demo-only stack — file-target redirects (>, >>) are still blocked at the parser, so no user-visible surface gains write capability in this commit. Co-Authored-By: Claude Opus 4.7 (1M context) --- README.md | 2 +- SHELL_FEATURES.md | 2 +- allowedpaths/sandbox.go | 31 ++++++- allowedpaths/sandbox_test.go | 139 +++++++++++++++++++++++++++---- analysis/symbols_allowedpaths.go | 6 ++ interp/api.go | 9 +- 6 files changed, 166 insertions(+), 23 deletions(-) diff --git a/README.md b/README.md index b8f75bf3..157c1aa3 100644 --- a/README.md +++ b/README.md @@ -66,7 +66,7 @@ Every access path is default-deny: **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. 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). 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()`. > **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 7c7a7aea..7e7028d1 100644 --- a/SHELL_FEATURES.md +++ b/SHELL_FEATURES.md @@ -99,7 +99,7 @@ Blocked features are rejected before execution with exit code 2. ## Execution - ✅ AllowedCommands — restricts which commands (builtins or external) may be executed; commands require the `rshell:` namespace prefix (e.g. `rshell:cat`); if not set, no commands are allowed -- ✅ AllowedPaths filesystem sandboxing — restricts all file access to specified directories +- ✅ AllowedPaths filesystem sandboxing — restricts all file access (read and write) to specified directories; cross-root symlink fallback is read-only to avoid TOCTOU on writes - ✅ Whole-run execution timeout — callers can bound a `Run()` call via `context.Context`, `interp.MaxExecutionTime`, or the CLI `--timeout` flag; the deadline applies to the entire script, not each individual command - ✅ ProcPath — overrides the proc filesystem path used by `ps` (default `/proc`; Linux-only; useful for testing/container environments) - ❌ External commands — blocked by default; requires an ExecHandler to be configured and the binary to be within AllowedPaths diff --git a/allowedpaths/sandbox.go b/allowedpaths/sandbox.go index d158918a..bf40b87e 100644 --- a/allowedpaths/sandbox.go +++ b/allowedpaths/sandbox.go @@ -291,11 +291,29 @@ func IsDevNull(path string) bool { return false } +// allowedOpenFlags is the set of os.OpenFile flag bits permitted by the +// sandbox. Anything outside this mask (O_DIRECTORY, O_NOFOLLOW, O_SYNC, +// O_NONBLOCK, etc.) is rejected as a defense-in-depth measure so unknown +// or platform-specific flags cannot slip through. +const allowedOpenFlags = os.O_RDONLY | os.O_WRONLY | os.O_RDWR | + os.O_APPEND | os.O_CREATE | os.O_EXCL | os.O_TRUNC + // Open implements the restricted file-open policy. The file is opened through -// os.Root for atomic path validation. Only read-only access is permitted; -// any write flags are rejected as a defense-in-depth measure. +// os.Root for atomic path validation, which uses openat under the hood and is +// immune to symlink and ".." traversal between path validation and open. +// +// Read and write opens are both permitted. Only the flag bits in +// allowedOpenFlags are accepted; anything else returns ErrPermission. +// +// The cross-root symlink fallback (resolveFollowingSymlinks) is read-only. +// Following a symlink that escapes its os.Root and then performing a write +// (O_CREATE/O_TRUNC/O_APPEND/O_WRONLY/O_RDWR) is the classic TOCTOU footgun: +// a malicious symlink could redirect a create or truncate to a target that +// has changed between resolution and open, defeating the sandbox. Writes +// must therefore stay within a single os.Root and never traverse the +// cross-root fallback. func (s *Sandbox) Open(path string, cwd string, flag int, perm os.FileMode) (io.ReadWriteCloser, error) { - if flag != os.O_RDONLY { + if flag&^allowedOpenFlags != 0 { return nil, &os.PathError{Op: "open", Path: path, Err: os.ErrPermission} } @@ -313,7 +331,12 @@ func (s *Sandbox) Open(path string, cwd string, flag int, perm os.FileMode) (io. if !isPathEscapeError(err) { return nil, PortablePathError(err) } - // Symlink escapes this root — resolve across all roots. + // Symlink escapes this root. Only fall back across roots for + // read-only opens — cross-root writes are TOCTOU-unsafe (see the + // function comment above). + if flag != os.O_RDONLY { + return nil, PortablePathError(err) + } r, rel, ok := s.resolveFollowingSymlinks(absPath, false) if !ok { return nil, PortablePathError(err) diff --git a/allowedpaths/sandbox_test.go b/allowedpaths/sandbox_test.go index 5e52340c..6ded6799 100644 --- a/allowedpaths/sandbox_test.go +++ b/allowedpaths/sandbox_test.go @@ -38,29 +38,136 @@ func (f fakeFileInfo) ModTime() time.Time { return time.Time{} } func (f fakeFileInfo) IsDir() bool { return false } func (f fakeFileInfo) Sys() any { return nil } -func TestSandboxOpenRejectsWriteFlags(t *testing.T) { +func TestSandboxWriteAllowedPath(t *testing.T) { dir := t.TempDir() - require.NoError(t, os.WriteFile(filepath.Join(dir, "test.txt"), []byte("data"), 0644)) sb, _, err := New([]string{dir}) require.NoError(t, err) defer sb.Close() - writeFlags := []int{ - os.O_WRONLY, - os.O_RDWR, - os.O_APPEND, - os.O_CREATE, - os.O_TRUNC, - os.O_WRONLY | os.O_CREATE | os.O_TRUNC, - } - for _, flag := range writeFlags { - f, err := sb.Open("test.txt", dir, flag, 0644) - assert.Nil(t, f, "open with flag %d should return nil", flag) - assert.ErrorIs(t, err, os.ErrPermission, "open with flag %d should be denied", flag) - } + // O_CREATE|O_WRONLY for a new file inside the allowlist should succeed + // and the file's contents should reflect the write. + f, err := sb.Open("created.txt", dir, os.O_WRONLY|os.O_CREATE|os.O_TRUNC, 0644) + require.NoError(t, err) + n, err := f.Write([]byte("hello")) + require.NoError(t, err) + assert.Equal(t, 5, n) + require.NoError(t, f.Close()) + + got, err := os.ReadFile(filepath.Join(dir, "created.txt")) + require.NoError(t, err) + assert.Equal(t, "hello", string(got)) +} + +func TestSandboxWriteOutsideAllowedPath(t *testing.T) { + allowed := t.TempDir() + outside := t.TempDir() + + sb, _, err := New([]string{allowed}) + require.NoError(t, err) + defer sb.Close() + + // Absolute path outside the allowlist must be rejected for writes. + target := filepath.Join(outside, "should-not-exist.txt") + f, err := sb.Open(target, allowed, os.O_WRONLY|os.O_CREATE|os.O_TRUNC, 0644) + assert.Nil(t, f) + assert.ErrorIs(t, err, os.ErrPermission) + + _, statErr := os.Stat(target) + assert.True(t, os.IsNotExist(statErr), "file outside allowlist must not be created") +} + +func TestSandboxAppend(t *testing.T) { + dir := t.TempDir() + path := filepath.Join(dir, "log.txt") + require.NoError(t, os.WriteFile(path, []byte("first\n"), 0644)) + + sb, _, err := New([]string{dir}) + require.NoError(t, err) + defer sb.Close() + + f, err := sb.Open("log.txt", dir, os.O_WRONLY|os.O_APPEND, 0) + require.NoError(t, err) + _, err = f.Write([]byte("second\n")) + require.NoError(t, err) + require.NoError(t, f.Close()) + + got, err := os.ReadFile(path) + require.NoError(t, err) + assert.Equal(t, "first\nsecond\n", string(got)) +} + +func TestSandboxTruncate(t *testing.T) { + dir := t.TempDir() + path := filepath.Join(dir, "data.txt") + require.NoError(t, os.WriteFile(path, []byte("original-content"), 0644)) + + sb, _, err := New([]string{dir}) + require.NoError(t, err) + defer sb.Close() + + f, err := sb.Open("data.txt", dir, os.O_WRONLY|os.O_TRUNC, 0) + require.NoError(t, err) + _, err = f.Write([]byte("short")) + require.NoError(t, err) + require.NoError(t, f.Close()) + + got, err := os.ReadFile(path) + require.NoError(t, err) + assert.Equal(t, "short", string(got), "O_TRUNC must replace, not append to, the original content") +} + +// TestSandboxWriteThroughSymlinkEscapeRejected ensures the cross-root +// symlink fallback is not used for write opens. Following a symlink that +// escapes its os.Root and then performing a create or truncate is the +// classic TOCTOU footgun: the link target can be swapped between +// resolution and open. Writes must stay inside a single os.Root. +func TestSandboxWriteThroughSymlinkEscapeRejected(t *testing.T) { + allowed := t.TempDir() + outside := t.TempDir() + + // Symlink inside the allowlist that points to a path *outside* the + // allowlist. os.Root will reject this with a path-escape error; + // the sandbox must then refuse to fall back for writes. + linkPath := filepath.Join(allowed, "escape") + require.NoError(t, os.Symlink(filepath.Join(outside, "target.txt"), linkPath)) + + sb, _, err := New([]string{allowed}) + require.NoError(t, err) + defer sb.Close() + + f, err := sb.Open("escape", allowed, os.O_WRONLY|os.O_CREATE|os.O_TRUNC, 0644) + assert.Nil(t, f) + assert.Error(t, err) + + _, statErr := os.Stat(filepath.Join(outside, "target.txt")) + assert.True(t, os.IsNotExist(statErr), "symlink target must not be created outside the sandbox") +} + +func TestSandboxWriteRejectsUnknownFlag(t *testing.T) { + dir := t.TempDir() + + sb, _, err := New([]string{dir}) + require.NoError(t, err) + defer sb.Close() + + // Pick a high bit that is not in allowedOpenFlags. We OR with + // O_WRONLY so the access mode itself is valid; only the unknown bit + // should trigger rejection. + const unknownFlag = 1 << 30 + f, err := sb.Open("x.txt", dir, os.O_WRONLY|os.O_CREATE|unknownFlag, 0644) + assert.Nil(t, f) + assert.ErrorIs(t, err, os.ErrPermission) +} + +func TestSandboxOpenReadStillWorks(t *testing.T) { + dir := t.TempDir() + require.NoError(t, os.WriteFile(filepath.Join(dir, "test.txt"), []byte("data"), 0644)) + + sb, _, err := New([]string{dir}) + require.NoError(t, err) + defer sb.Close() - // Read-only should still work. f, err := sb.Open("test.txt", dir, os.O_RDONLY, 0) require.NoError(t, err) f.Close() diff --git a/analysis/symbols_allowedpaths.go b/analysis/symbols_allowedpaths.go index 73cc7c7b..9d670d0d 100644 --- a/analysis/symbols_allowedpaths.go +++ b/analysis/symbols_allowedpaths.go @@ -41,7 +41,13 @@ var allowedpathsAllowedSymbols = []string{ "os.Getgid", // 🟠 returns the numeric group id of the caller; read-only syscall. "os.Getgroups", // 🟠 returns supplementary group ids; read-only syscall. "os.Getuid", // 🟠 returns the numeric user id of the caller; read-only syscall. + "os.O_APPEND", // 🟢 append-on-write file flag constant; pure constant. Part of the sandbox open-flag allowlist. + "os.O_CREATE", // 🟢 create-if-missing file flag constant; pure constant. Part of the sandbox open-flag allowlist. + "os.O_EXCL", // 🟢 exclusive-create file flag constant; pure constant. Part of the sandbox open-flag allowlist. "os.O_RDONLY", // 🟢 read-only file flag constant; pure constant. + "os.O_RDWR", // 🟢 read-write file flag constant; pure constant. Part of the sandbox open-flag allowlist. + "os.O_TRUNC", // 🟢 truncate-on-open file flag constant; pure constant. Part of the sandbox open-flag allowlist. + "os.O_WRONLY", // 🟢 write-only file flag constant; pure constant. Part of the sandbox open-flag allowlist. "os.OpenRoot", // 🟠 opens a directory as a root for sandboxed file access; needed for sandbox. "os.PathError", // 🟢 error type wrapping path and operation; pure type. "os.Root", // 🟠 sandboxed directory root type; core of the filesystem sandbox. diff --git a/interp/api.go b/interp/api.go index 691cea13..521288bc 100644 --- a/interp/api.go +++ b/interp/api.go @@ -695,7 +695,14 @@ func (r *Runner) Warnings() []string { // AllowedPaths restricts file and directory access to the specified directories. // Paths must be absolute directories that exist. When set, only files within -// these directories can be opened, read, or executed. +// these directories can be opened (for reading or writing), read, or executed. +// +// The sandbox itself permits both read and write opens through os.Root; +// whether a particular shell feature (a builtin, a redirection, etc.) +// actually performs writes is a separate, layered decision. The validate +// pass currently blocks file-target output redirections (>, >>) at parse +// time, so the user-visible surface remains read-only until those layers +// opt in. // // When not set (default), all file access is blocked. // An empty slice also blocks all file access.