diff --git a/docs/guides/dev/cli-internals.md b/docs/guides/dev/cli-internals.md index cabc46c5f5..e45f694b0a 100644 --- a/docs/guides/dev/cli-internals.md +++ b/docs/guides/dev/cli-internals.md @@ -271,7 +271,7 @@ Per-repo mode does not use the layer stack — it runs the same phases inline in │ ▼ │ │ ┌──────────────────┐ │ │ │ Extract output │ SafeDownload() with sanitization: │ -│ │ │ - Remove symlinks (sandbox escape) │ +│ │ │ - Remove dangerous symlinks (sandbox escape) │ │ │ │ - Remove .git/hooks/ (hook injection) │ │ └──────┬───────────┘ │ │ ▼ │ @@ -316,7 +316,7 @@ SandboxClaudeConfig = "/tmp/claude-config" | `ExecStreamReader()` | `openshell sandbox exec ...` | Streaming stdout reader | | `Upload()` | `openshell sandbox upload ...` | Copy files into sandbox | | `Download()` | `openshell sandbox download ...` | Copy files out of sandbox | -| `SafeDownload()` | Download + sanitize | Remove symlinks, .git/hooks | +| `SafeDownload()` | Download + sanitize | Remove dangerous symlinks (absolute or repo-escaping), .git/hooks | | `CollectLogs()` | Download logs dir | Extract sandbox logs | | `ExtractTranscripts()` | Download transcripts | Extract conversation transcripts | | `Delete()` | `openshell sandbox delete` | Destroy container | @@ -324,7 +324,7 @@ SandboxClaudeConfig = "/tmp/claude-config" ### Security: sanitizeDownload() After downloading files from the sandbox, `sanitizeDownload()` removes: -- **Symlinks** — Prevents sandbox escape via symlink-to-host-path attacks +- **Dangerous symlinks** (absolute targets or targets that escape the repo) — Prevents sandbox escape via symlink-to-host-path attacks; relative in-repo symlinks are kept - **.git/hooks/** — Prevents hook injection that would execute on the host --- diff --git a/internal/cli/run.go b/internal/cli/run.go index a6685edce6..5ff832bd61 100644 --- a/internal/cli/run.go +++ b/internal/cli/run.go @@ -554,8 +554,8 @@ func runAgent(agentName, fullsendDir, outputBase, targetRepo, fullsendBinary str } } - // 9d. Extract target repo back to host. SafeDownload removes symlinks - // and .git/hooks/ after download to prevent sandbox escape. + // 9d. Extract target repo back to host. SafeDownload removes dangerous + // symlinks (absolute or repo-escaping) and .git/hooks/ to prevent sandbox escape. if clearErr := os.RemoveAll(repoSrc); clearErr != nil { return fmt.Errorf("clearing local repo %s before extraction: %w", repoSrc, clearErr) } diff --git a/internal/sandbox/sandbox.go b/internal/sandbox/sandbox.go index 4046a2db54..ba03933ca4 100644 --- a/internal/sandbox/sandbox.go +++ b/internal/sandbox/sandbox.go @@ -25,12 +25,42 @@ const ( ) func sanitizeDownload(localDir string) error { - return filepath.WalkDir(localDir, func(path string, d fs.DirEntry, err error) error { + absLocal, err := filepath.Abs(localDir) + if err != nil { + return err + } + absLocal, err = filepath.EvalSymlinks(absLocal) + if err != nil { + return err + } + + return filepath.WalkDir(absLocal, func(path string, d fs.DirEntry, err error) error { if err != nil { return err } if d.Type()&fs.ModeSymlink != 0 { - return os.Remove(path) + target, readErr := os.Readlink(path) + if readErr != nil { + return os.Remove(path) + } + // Absolute targets always point outside the repo root. + if filepath.IsAbs(target) { + return os.Remove(path) + } + // Use EvalSymlinks, not filepath.Clean: Clean is textual and misses + // chains where an in-repo dir-symlink is used as a component + // (e.g. "sub/link/../../etc/passwd" cleans to inside the repo but + // follows the link to outside). Fall back to remove on error + // (dangling or looping). + rawPath := filepath.Dir(path) + string(filepath.Separator) + target + resolved, evalErr := filepath.EvalSymlinks(rawPath) + if evalErr != nil { + return os.Remove(path) + } + if !strings.HasPrefix(resolved+string(filepath.Separator), absLocal+string(filepath.Separator)) { + return os.Remove(path) + } + return nil } if d.IsDir() && d.Name() == "hooks" && filepath.Base(filepath.Dir(path)) == ".git" { @@ -343,7 +373,7 @@ func DownloadFile(sandboxName, remotePath, localPath string) error { } // SafeDownload copies a directory from a sandbox to the local machine and then -// sanitizes the result by removing symlinks and .git/hooks/. +// sanitizes the result by removing dangerous symlinks (absolute or repo-escaping) and .git/hooks/. func SafeDownload(sandboxName, remoteDir, localDir string) error { if err := Download(sandboxName, remoteDir, localDir); err != nil { return err diff --git a/internal/sandbox/sandbox_test.go b/internal/sandbox/sandbox_test.go index 17e15c716e..63bedaa841 100644 --- a/internal/sandbox/sandbox_test.go +++ b/internal/sandbox/sandbox_test.go @@ -149,25 +149,92 @@ func TestOsRootContainment(t *testing.T) { assert.Error(t, err) } -func TestSanitizeDownload_RemovesSymlinks(t *testing.T) { +func TestSanitizeDownload_RemovesAbsoluteSymlinks(t *testing.T) { dir := t.TempDir() - // Create a regular file. require.NoError(t, os.WriteFile(filepath.Join(dir, "real.txt"), []byte("ok"), 0o644)) - - // Create a symlink (dangling is fine — we just need it to exist). require.NoError(t, os.Symlink("/nonexistent/target", filepath.Join(dir, "danger"))) err := sanitizeDownload(dir) require.NoError(t, err) - // Regular file should survive. _, err = os.Stat(filepath.Join(dir, "real.txt")) assert.NoError(t, err) - // Symlink should be removed. _, err = os.Lstat(filepath.Join(dir, "danger")) - assert.True(t, os.IsNotExist(err), "symlink should have been removed") + assert.True(t, os.IsNotExist(err), "absolute symlink should have been removed") +} + +func TestSanitizeDownload_KeepsRelativeSymlinksInsideRepo(t *testing.T) { + dir := t.TempDir() + + require.NoError(t, os.MkdirAll(filepath.Join(dir, "sub"), 0o755)) + require.NoError(t, os.WriteFile(filepath.Join(dir, "target.txt"), []byte("ok"), 0o644)) + // Relative symlink: sub/link -> ../target.txt (stays inside dir) + require.NoError(t, os.Symlink("../target.txt", filepath.Join(dir, "sub", "link"))) + + err := sanitizeDownload(dir) + require.NoError(t, err) + + _, err = os.Lstat(filepath.Join(dir, "sub", "link")) + assert.NoError(t, err, "relative in-repo symlink should be preserved") +} + +func TestSanitizeDownload_RemovesSymlinkChainEscape(t *testing.T) { + dir := t.TempDir() + + require.NoError(t, os.MkdirAll(filepath.Join(dir, "real"), 0o755)) + require.NoError(t, os.MkdirAll(filepath.Join(dir, "sub"), 0o755)) + // dirlink -> ../real: relative, inside repo — sanitizeDownload keeps it. + require.NoError(t, os.Symlink("../real", filepath.Join(dir, "sub", "dirlink"))) + // escape -> "sub/dirlink/../../etc/passwd": + // filepath.Clean sees: dir/sub/dirlink/../../etc/passwd → dir/etc/passwd (inside, passes textual check) + // EvalSymlinks follows: sub/dirlink → dir/real → ../../etc/passwd → outside dir (escapes) + require.NoError(t, os.Symlink("sub/dirlink/../../etc/passwd", filepath.Join(dir, "escape"))) + + err := sanitizeDownload(dir) + require.NoError(t, err) + + _, err = os.Lstat(filepath.Join(dir, "sub", "dirlink")) + assert.NoError(t, err, "in-repo dirlink should be preserved") + + _, err = os.Lstat(filepath.Join(dir, "escape")) + assert.True(t, os.IsNotExist(err), "symlink-chain escape should be removed") +} + +func TestSanitizeDownload_RemovesRelativeSymlinksEscapingRepo(t *testing.T) { + dir := t.TempDir() + + require.NoError(t, os.MkdirAll(filepath.Join(dir, "sub"), 0o755)) + // Relative symlink that traverses above dir root. + require.NoError(t, os.Symlink("../../etc/passwd", filepath.Join(dir, "sub", "escape"))) + + err := sanitizeDownload(dir) + require.NoError(t, err) + + _, err = os.Lstat(filepath.Join(dir, "sub", "escape")) + assert.True(t, os.IsNotExist(err), "escaping relative symlink should have been removed") +} + +func TestSanitizeDownload_RemovesDirSymlinkIndirection(t *testing.T) { + repo := t.TempDir() + + // Place a secret file outside the repo root. + secret := filepath.Join(filepath.Dir(repo), "secret") + require.NoError(t, os.WriteFile(secret, []byte("leaked"), 0o644)) + t.Cleanup(func() { os.Remove(secret) }) + + // d/x is a directory symlink to "." — relative, inside repo, so kept. + // e targets "d/x/../../secret" which textually cleans to repo/secret (inside), + // but on the filesystem d/x resolves to d/, so ../../secret escapes. + require.NoError(t, os.MkdirAll(filepath.Join(repo, "d"), 0o755)) + require.NoError(t, os.Symlink(".", filepath.Join(repo, "d", "x"))) + require.NoError(t, os.Symlink("d/x/../../secret", filepath.Join(repo, "e"))) + + require.NoError(t, sanitizeDownload(repo)) + + _, err := os.Lstat(filepath.Join(repo, "e")) + assert.True(t, os.IsNotExist(err), "dir-symlink indirection escape should have been removed") } func TestSanitizeDownload_RemovesGitHooks(t *testing.T) {