fix(sandbox): protect daemon token file#685
Conversation
|
Warning Review limit reached
Next review available in: 34 minutes Your organization has reached its usage spending cap. Adjust your spending cap in the billing tab. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
WalkthroughThe sandbox now protects the file referenced by ChangesDaemon token protection
Estimated code review effort: 2 (Simple) | ~10 minutes Possibly related issues
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Pull request overview
This PR closes a sandbox escape where ZERO_DAEMON_REMOTE_TOKEN_FILE could be inherited by sandboxed commands (allowing them to locate and read the daemon bearer token file under the read-all posture). It scrubs the pointer env var across platforms and extends the existing “credential deny-read” profile logic to also deny reads of the referenced token file where deny-read enforcement is supported.
Changes:
- Scrub
ZERO_DAEMON_REMOTE_TOKEN_FILEfrom sandbox command environments (in addition to the inline token env var). - Extend
credentialDenyReadPathsto include the path named byZERO_DAEMON_REMOTE_TOKEN_FILE(alongsideGOOGLE_APPLICATION_CREDENTIALS) and plumb this through the pure helper. - Add/extend regression tests covering env scrubbing and permission-profile deny-read construction (skipping the deny-read assertion on Windows per existing platform limitations).
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| internal/sandbox/runner.go | Adds ZERO_DAEMON_REMOTE_TOKEN_FILE to the sandbox env scrub list. |
| internal/sandbox/runner_test.go | Extends env scrubbing regression test to ensure the pointer env var is removed. |
| internal/sandbox/profile.go | Adds ZERO_DAEMON_REMOTE_TOKEN_FILE to default credential deny-read path construction and updates helper signature/docs. |
| internal/sandbox/manager_test.go | Updates credential deny-read tests for the new parameter and adds a profile-level regression test for daemon token file denial (non-Windows). |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
jatmn
left a comment
jatmn
left a comment
There was a problem hiding this comment.
I found issues that need to be addressed before this is ready.
Findings
- [P1] Deny writes to the daemon token file on macOS as well
internal/sandbox/profile.go:176
The new target entersDenyRead, but the Seatbelt backend translates that only intofile-read*and unlink denials. Its broadfile-write*allowance still covers every workspace root and the default temporary roots. Therefore, whenZERO_DAEMON_REMOTE_TOKEN_FILEnames a file under/tmpor another writable root, a sandboxed command can discover the filename from its parent directory and overwrite or truncate the bearer-token file. This makes the remote bridge unavailable and can replace its credential on a restart/reload. Add a write denial for credentialDenyReadfiles in the Seatbelt profile (and a macOS regression case for a token under a writable temporary root).
Address code review on PR Gitlawb#685: the Seatbelt profile only translated DenyRead entries into file-read* and file-write-unlink denials. The broad file-write* allowance for workspace/temp write roots still covered a DenyRead file (e.g. the file ZERO_DAEMON_REMOTE_TOKEN_FILE names) if it happened to sit under one of them, so a sandboxed command could discover and overwrite/truncate the daemon bearer-token file even though it couldn't read or delete it. A file a sandboxed command must not read has no legitimate reason to be written either, so seatbeltProfileFromPermissionProfile now also emits a full file-write* deny for every DenyRead path, placed after the broad write allow (deny rules that follow an allow win, matching the existing DenyWrite/metadata-carveout ordering). Adds a regression test with a DenyRead file under a writable /tmp root, and extends the existing deny-ordering test to assert the new file-write* rule. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
anandh8x
left a comment
anandh8x
left a comment
There was a problem hiding this comment.
Re-reviewing against commit 2248aca8 (head). The macOS Seatbelt fix (patch 2/2) is the right primitive: a DenyRead file that's also under a writable root was overwritable/truncatable because the prior profile only emitted file-read* and file-write-unlink, not file-write*. Denying the full write direction for every DenyRead path is correct, the ordering (deny after the broad allow) is correct, and TestSeatbeltProfileDeniesWritesToDenyReadUnderWritableRoot covers both the rule presence and the ordering. The TestSeatbeltProfileProtectsMetadataAndDenyOrdering extension covers the general case.
LGTM.
Cross-PR note: #685 depends on the credentialDenyReadPathsIn signature change from #681 (daemon token file as a parameter) and the scrubSensitiveEnv plumbed sensitiveEnvKeys from #682. Recommend rebasing #685 onto #681 + #682 in that order.
gnanam1990
left a comment
gnanam1990
left a comment
There was a problem hiding this comment.
Local review: built and ran go test ./internal/sandbox on darwin/arm64; all pass. The deny-write-for-DenyRead fix is a genuine security improvement (closes the truncate/overwrite bypass under a writable root). One integration note.
| // credentialDenyReadPathsIn is the pure core of credentialDenyReadPaths, | ||
| // separated so tests can exercise it against a synthetic home directory. | ||
| func credentialDenyReadPathsIn(home string, googleCredentials string, allowRead []string) []string { | ||
| func credentialDenyReadPathsIn(home string, googleCredentials string, daemonTokenFile string, allowRead []string) []string { |
There was a problem hiding this comment.
[P2] Sibling-PR merge conflict on credentialDenyReadPathsIn signature
This PR keeps the flat (home, googleCredentials, daemonTokenFile, allowRead) signature, while #681 rewrites the same function to take a credentialPathOptions struct. Both PRs also edit manager_test.go and runner_test.go against the same test. Each merges into main cleanly on its own, but only one lands as-is; the other must rebase. Also note #682 changes scrubSensitiveEnv in runner.go (this PR adds ZERO_DAEMON_REMOTE_TOKEN_FILE to the same function's literal list) -> sequence the three sandbox credential PRs.
jatmn
left a comment
jatmn
left a comment
There was a problem hiding this comment.
I found issues that need to be addressed before this is ready.
Findings
- [P1] Protect the configured symlink pathname as well as its target
internal/sandbox/profile.go:200
normalizeProfilePathsresolvesZERO_DAEMON_REMOTE_TOKEN_FILEthrough symlinks before it is added toDenyRead. If the configured pathname is a symlink under a writable root such as/tmp, the new deny rules protect only its current referent; a sandboxed command can unlink the writable symlink and recreate a regular file at the configured pathname. On the next remote-daemon start,TokenFromEnvreads that replacement pathname and accepts the attacker-chosen bearer token (or fails, causing a denial of service). Preserve and deny the lexical configured path in addition to its resolved target, and add a symlink-replacement regression test.
There was a problem hiding this comment.
Approving clean security hardening. Scrubbing ZERO_DAEMON_REMOTE_TOKEN_FILE from child envs and adding its target to the credential deny-read set closes a real hole (a sandboxed command could otherwise resolve the pointer and read the daemon bearer-token file under the read-all posture), and extending the macOS seatbelt profile to file-write*-deny every DenyRead path is the right fix: denyReadRules only blocked read and unlink, leaving a credential file under a writable root overwritable/truncatable. I checked the Linux bubblewrap path and it already bind-mounts DenyRead targets read-only, so this just brings macOS to parity. One thing to be aware of: the write-deny now covers all DenyRead paths (~/.aws, ~/.azure, etc.), so no sandboxed command can update cloud creds consistent with the existing unlink-deny and fine under the current threat model, just calling it out.
Summary
ZERO_DAEMON_REMOTE_TOKEN_FILEfrom inherited sandbox command environmentsAllowReadopt-outsRoot cause
The sandbox scrubbed the inline
ZERO_DAEMON_REMOTE_TOKENvalue but left its file-pointer alternative in the child environment. Under the read-all workspace posture, a sandboxed command could read that pointer and then the bearer-token file outside the workspace.The pointer is now scrubbed on every platform. Its target is protected by the existing credential deny-read mechanism on platforms where that mechanism is enabled. Windows filesystem deny-read remains subject to the existing ACL-model limitation tracked by #662.
Fixes #677
Validation
go fmt ./...go vet ./...go test ./internal/sandbox -count=1go test ./... -count=1with TestLoadProviderCommandTimeout waits for child completion on Windows #683 and Real gopls manual test can fail the default suite on an unusable installation #684's known tests excluded: every package except unrelated Windows process-lifecycle tests ininternal/toolspassedgo run golang.org/x/vuln/cmd/govulncheck@v1.3.0 ./...: no vulnerabilities foundKnown baseline/environment limitations
make lintuses a Unix shell assignment infmt-checkthat does not run under this Windows shell.internal/toolsprocess-termination and temporary-directory cleanup tests fail on Windows because child processes retain file handles.TestLoadProviderCommandTimeoutand the real-goplsintegration test are tracked separately by TestLoadProviderCommandTimeout waits for child completion on Windows #683 and Real gopls manual test can fail the default suite on an unusable installation #684.Summary by CodeRabbit
Security
Tests