fix: sandbox path jail bypassed by a slashless symlink (escape)#249
fix: sandbox path jail bypassed by a slashless symlink (escape)#249Mubashirrrr wants to merge 2 commits into
Conversation
26-sandbox-runner-denylist: the path jail (_check_path_jail) only resolves and prefix-checks arguments for which _looks_like_path() is true, which requires a path separator (or exactly ./..). A symlink whose name has no slash — e.g. `link.txt` in the project root pointing at /etc/passwd — is therefore never jail-checked, so `cat link.txt` escapes the jail and reads files outside the project root, exactly what the module's advertised "symlink-safe path jail" is meant to prevent. (`sub/link.txt`, having a slash, is correctly denied; the asymmetry is the tell.) Also jail-check arguments that exist on disk under the root (os.path.lexists), so a slashless symlink is resolved and refused. lexists catches the symlink even when its target is missing. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughUpdates ChangesPath Jail Enhancement for Symlink Detection
🎯 3 (Moderate) | ⏱️ ~20 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
phases/19-capstone-projects/26-sandbox-runner-denylist/code/tests/test_main.py (1)
119-131: 💤 Low valueConsider skipping the test on platforms without symlink support.
os.symlink()raisesOSErroron Windows without Developer Mode or admin privileges. If CI ever runs on Windows, this test would fail unexpectedly.♻️ Optional: Add platform guard
def test_slashless_symlink_to_outside_is_denied(self) -> None: # A symlink whose name has no slash points outside the root. The module # advertises a "symlink-safe path jail via realpath prefix check", so the # jail must resolve and refuse it even though _looks_like_path misses it. root, cfg = _make_root() outside_dir = tempfile.mkdtemp(prefix="sandbox-outside-") secret = os.path.join(outside_dir, "secret.txt") with open(secret, "w", encoding="utf-8") as fh: fh.write("TOP-SECRET\n") + try: + os.symlink(secret, os.path.join(root, "link.txt")) + except OSError: + self.skipTest("symlinks not supported on this platform") - os.symlink(secret, os.path.join(root, "link.txt")) reason = _check_path_jail(["cat", "link.txt"], cfg) self.assertIsNotNone(reason) self.assertIn("outside project root", reason)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@phases/19-capstone-projects/26-sandbox-runner-denylist/code/tests/test_main.py` around lines 119 - 131, The test test_slashless_symlink_to_outside_is_denied currently calls os.symlink unguarded; modify the test to skip when the platform/user cannot create symlinks by first checking hasattr(os, "symlink") and then attempting to create and immediately remove a small temporary symlink inside the test (catching OSError); if creation fails, call self.skipTest("symlink not supported or requires elevated privileges") before calling _make_root/_check_path_jail so the test safely skips on Windows/other envs without symlink support.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In
`@phases/19-capstone-projects/26-sandbox-runner-denylist/code/tests/test_main.py`:
- Around line 119-131: The test test_slashless_symlink_to_outside_is_denied
currently calls os.symlink unguarded; modify the test to skip when the
platform/user cannot create symlinks by first checking hasattr(os, "symlink")
and then attempting to create and immediately remove a small temporary symlink
inside the test (catching OSError); if creation fails, call
self.skipTest("symlink not supported or requires elevated privileges") before
calling _make_root/_check_path_jail so the test safely skips on Windows/other
envs without symlink support.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 5dc9e73c-1f29-4cc9-8162-75c6bc2c1042
📒 Files selected for processing (2)
phases/19-capstone-projects/26-sandbox-runner-denylist/code/main.pyphases/19-capstone-projects/26-sandbox-runner-denylist/code/tests/test_main.py
os.symlink raises OSError on Windows without Developer Mode/admin, which would fail this test spuriously in such CI. Skip instead of failing. Addresses CodeRabbit review feedback on the PR. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
Thanks — addressed in |
What
The sandbox in
19-capstone-projects/26-sandbox-runner-denylistcan be escaped with a slashless symlink, reading files outside the project root — the exact thing its "symlink-safe path jail" is documented to prevent.Why
_check_path_jailonly resolves/prefix-checks an argument when_looks_like_path(arg)is true, and that returns true only when the arg contains//\or is exactly./..:A symlink whose name has no separator — e.g.
link.txtin the project root pointing at/etc/passwd— is skipped, socat link.txtruns and exfiltrates host files. The tell:sub/link.txt(same symlink one dir deep, has a slash) is correctly denied. The command runs as a real subprocess withcwd=root, so the jail is the only thing standing between a sandboxed tool and arbitrary host reads.Fix
Also jail-check arguments that exist on disk under the root, so a slashless symlink is resolved and refused (
os.path.lexistscatches the symlink even if its target is missing). Non-path tokens that don't exist under root are still skipped, and real files under root still resolve inside root (no false denials).Test
New
test_slashless_symlink_to_outside_is_deniedfails on the previous code (jail returnedNone/allowed) and passes after the fix.🤖 Generated with Claude Code