fix(sandbox): protect daemon token file - #685
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughThe daemon token file is canonicalized before remote serving, added to sandbox credential protections, excluded from grep/glob and direct file operations, and removed from spawned command environments. Tests cover path resolution, policy enforcement, seatbelt rules, deny-read profiles, and edge cases. ChangesDaemon token protection
Estimated code review effort: 4 (Complex) | ~45 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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.
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.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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.
Inline comments:
In `@internal/sandbox/profile.go`:
- Around line 320-328: Keep normalizeProfilePath purely lexical by removing its
filepath.EvalSymlinks resolution and returning the result of
normalizeProfilePathLexical unchanged. Resolve symlinks only within
normalizeProfilePathVariants while retaining both the configured lexical path
and resolved target for deny-policy expansion, and add a regression test
covering a writable denied symlink.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 974aa02c-d6a1-45e8-ae0b-c2df72771e98
📒 Files selected for processing (4)
internal/sandbox/manager_test.gointernal/sandbox/profile.gointernal/sandbox/runner.gointernal/sandbox/runner_test.go
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] Do not pass a lexical symlink to Bubblewrap's deny mount
internal/sandbox/profile.go:200
For an existingZERO_DAEMON_REMOTE_TOKEN_FILEsymlink, the new variant list includes the symlink pathname as well as its target. The Linux backend then emits--ro-bind /dev/null <symlink>for that pathname; Bubblewrap rejects a symlink mount destination before the command starts (Can't create file at .../daemon-token: No such file or directory). Thus configuring the supported token-file option through a symlink makes every Linux sandboxed command fail to launch. Materialize/protect that pathname with a Bubblewrap-safe mechanism (or avoid adding it to the Linux deny-mount list) and add a Linux regression test. -
[P1] Resolve the token-file path in the daemon's context, not each worker's
internal/sandbox/profile.go:195
TokenFromEnvaccepts relative token paths, andserve-remotereads one before it starts workers. The daemon then preservesZERO_DAEMON_REMOTE_TOKEN_FILEfor workers whosecmd.Diris the per-sessionspec.Cwd;normalizeProfilePathLexicalconsequently turnstokeninto a path beneath that session instead of the daemon startup directory that contains the actual bearer-token file. The real file is left outsideDenyReadunder the read-all posture, so a sandboxed command that can infer its location can read it. Normalize the value at the daemon boundary (or pass an already-absolute protected path) and cover a remote worker whose session CWD differs from the daemon CWD.
|
Following up on my earlier approve, which I am pulling back from for now. jatmn's latest P1 is a real one: the symlink-protection commit adds the ZERO_DAEMON_REMOTE_TOKEN_FILE symlink pathname itself, not just its resolved target, to the Linux deny-mount list, and Bubblewrap rejects a symlink as a mount destination, so every sandboxed command on Linux fails to launch when that option points at a symlink. I am on Windows and cannot reproduce the bwrap behavior here, but jatmn tested it on Linux with the exact "Can't create file ... daemon-token" error and the mechanism is sound. The target protection and the macOS write-deny are still the right hardening. This just needs the Linux side to protect that pathname without ro-binding the symlink itself (materialize it, or keep the symlink pathname off the Linux deny-mount list). Not re-approving until that is closed. |
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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.
Inline comments:
In `@internal/sandbox/linux_helper.go`:
- Around line 319-324: Add the same lexical-symlink guard used in the DenyRead
path to appendReadOnlyLinuxPathArgs, checking the mount path with os.Lstat and
returning the existing args unchanged when it is a symlink. Keep the current
handling for non-symlink paths unchanged.
In `@internal/sandbox/profile.go`:
- Line 325: The FileSystemPolicy initializers in PermissionProfileFromPolicy and
seatbeltCompatibilityPermissionProfile must preserve both lexical and resolved
paths for user deny policies. Replace single-path normalization for
policy.DenyRead and policy.DenyWrite with normalizeProfilePathVariants, while
leaving normalizeProfilePath unchanged for other uses.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: cbb0b9b3-3559-4c77-bb5a-2c1692650e7a
📒 Files selected for processing (7)
internal/cli/daemon.gointernal/cli/daemon_test.gointernal/sandbox/linux_helper.gointernal/sandbox/linux_helper_test.gointernal/sandbox/manager_test.gointernal/sandbox/profile.gointernal/sandbox/runner_test.go
🚧 Files skipped from review as they are similar to previous changes (2)
- internal/sandbox/runner_test.go
- internal/sandbox/manager_test.go
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] Preserve the resolved target for user-configured
DenyReadsymlinks
internal/sandbox/profile.go:104
normalizeProfilePathis now lexical-only, while this initializer still usesnormalizeProfilePathsfor policy entries. On Linux,appendUnreadableLinuxPathArgsthen skips that symlink mount destination and no resolved target is present (unlike the credential-path branch). Thus a policy such asdenyRead: [link], wherelinkpoints to a secret, produces no deny mount under the read-all profile and the sandboxed command can read the target. Keep both variants for deny paths (and update the macOS compatibility initializer) so the Bubblewrap-safe target is actually denied. -
[P1] Do not use lexical paths for ordinary sandbox roots
internal/sandbox/profile.go:324
This changed the shared normalizer used forworkspaceRoot,AllowWrite, andDenyWrite, not just the new credential deny variant. A workspace opened through a symlink now reaches Linux Bubblewrap as--bind <link> <link>; Bubblewrap rejects a symlink mount destination, so every sandboxed command fails before it starts. I reproduced the failure with a symlinked workspace. Restore resolved normalization for ordinary roots and keep lexical-plus-resolved handling scoped to deny-path expansion. -
[P1] Do not leave a writable token-file symlink unprotected on Linux
internal/sandbox/linux_helper.go:319
Skipping the lexical symlink avoids Bubblewrap's invalid mount destination, but only its original target is masked. IfZERO_DAEMON_REMOTE_TOKEN_FILEis a symlink under a writable root such as/tmp, a sandboxed command can replace it with a link to another host-readable file and read through the replacement; it can also corrupt the daemon's token path. The test currently asserts the unsafe omission. Protect or materialize the lexical pathname with a Bubblewrap-safe mechanism rather than simply dropping its deny rule. -
[P1] Handle symlinked parent directories before emitting a deny mount
internal/sandbox/linux_helper.go:319
TheLstatcheck catches only a final-component symlink. For a supported token path such as/tmp/linkdir/token, wherelinkdiris a symlink,Lstat(token)reports a regular file and the helper emits a deny mount through the symlinked parent. Bubblewrap rejects that destination and every Linux sandbox launch fails. Detect path traversal through a symlink (or omit the lexical variant after retaining the resolved target) and add a regression case for this layout.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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.
Inline comments:
In `@internal/sandbox/linux_helper.go`:
- Around line 323-349: The Linux path argument helpers currently abort on
lexical symlinks instead of skipping them when their resolved target is also
protected. Update the profile-processing flow around appendReadOnlyLinuxPathArgs
and appendUnreadableLinuxPathArgs to recognize lexical symlink entries whose
resolved targets exist in the same deny set, skip those entries, and continue
enforcing the target; retain the existing error behavior when no enforceable
target is present. Update the related test to assert successful sandbox startup
and target enforcement.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: baa5d0ce-25e0-42a5-8752-15ae141e7d1d
📒 Files selected for processing (7)
internal/cli/daemon.gointernal/cli/daemon_test.gointernal/sandbox/linux_helper.gointernal/sandbox/linux_helper_test.gointernal/sandbox/manager_test.gointernal/sandbox/profile.gointernal/sandbox/runner.go
🚧 Files skipped from review as they are similar to previous changes (2)
- internal/cli/daemon.go
- internal/sandbox/runner.go
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] Keep the remote token excluded from in-process file tools
internal/sandbox/profile.go:104
The new daemon-token path is added only toPermissionProfile.FileSystem.DenyRead, which protects wrapped shell commands. Built-in tools do not consume that profile:read_filereads scoped files directly, and grep/glob exclusions are built fromPolicy.DenyRead. If the token file is inside a remote session workspace (for example, a daemon started with a relative token-file path from that workspace), a remote-controlled agent can useread_fileto exfiltrate the bridge bearer token. Apply the automatic credential exclusion to the in-process read/search tool boundary as well, and cover this with an end-to-end tool test. -
[P1] Preserve inline-token precedence when a token-file variable is stale
internal/cli/daemon.go:480
TokenFromEnvintentionally returns a nonemptyZERO_DAEMON_REMOTE_TOKENbefore consultingZERO_DAEMON_REMOTE_TOKEN_FILE, but this new preflight resolves the file first. Consequently, a valid inline token plus an inherited missing or dangling token-file variable now makesdaemon serve-remoteexit instead of starting. Only canonicalize the file when it is the selected source (or otherwise leave an ignored file pointer from changing the result), and add the both-variables regression case. -
[P1] Do not make symlink-backed credential paths disable every Linux sandbox command
internal/sandbox/linux_helper.go:344
The profile now deliberately retains both lexical and resolved forms of every credential/deny path, but the Linux argument builder aborts whenever either form has a symlink component. This makes common configurations such asGOOGLE_APPLICATION_CREDENTIALS=/var/run/...(where/var/runis commonly a symlink to/run) fail plan construction for every sandboxed command; the pre-PR profile kept only the resolved target. Preserve the denial of the resolved target while using a Bubblewrap-safe treatment for the lexical path instead of turning a valid credential configuration into a global sandbox-startup failure.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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.
Inline comments:
In `@internal/sandbox/engine.go`:
- Around line 57-75: Update withAutomaticDenyRead to recompute automaticDenyRead
from the current effective policy before merging it with policy.DenyRead, rather
than reusing the constructor-time list. Ensure credential paths allowed through
session or turn permission profiles are removed from the automatic deny set
while preserving deduplication.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 059619b5-dbbc-4812-a361-6fad61cca69c
📒 Files selected for processing (6)
internal/cli/daemon.gointernal/cli/daemon_test.gointernal/sandbox/engine.gointernal/sandbox/linux_helper.gointernal/sandbox/linux_helper_test.gointernal/tools/read_exclusions_test.go
🚧 Files skipped from review as they are similar to previous changes (2)
- internal/cli/daemon.go
- internal/sandbox/linux_helper.go
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
- [P2] Ignore an unused token-file pointer when the inline token is selected
internal/sandbox/pathlists.go:75
TokenFromEnvgivesZERO_DAEMON_REMOTE_TOKENprecedence, andCanonicalizeTokenFileEnvdeliberately leavesZERO_DAEMON_REMOTE_TOKEN_FILEalone in that case. However,protectedCredentialPaths(and the profile builder) still treats that unused file pointer as the live credential. A valid daemon configuration with both variables set therefore makes the file named by a stale pointer unreadable, unsearchable, and unwritable to every session even though it was never used for authentication. Apply file-token protection only when the inline token is blank (or remove the unused pointer before workers inherit it), and cover the dual-variable case.
Amp-Thread-ID: https://ampcode.com/threads/T-019fa018-e3c8-705b-9627-3a7564f8d66a Co-authored-by: Pierre Bruno <pierrebruno@hotmail.ch>
jatmn
left a comment
jatmn
left a comment
There was a problem hiding this comment.
Findings
-
[P2]
require_escalatedshell bypasses all token protection when userDenyReadis empty
internal/sandbox/engine.go:197-205
internal/tools/bash.go:241-245
internal/sandbox/profile.go:164-166
The bridge token is deliberately kept out ofPolicy.DenyReadso escalated execution stays available, butUnsandboxedExecutionAlloweduses that same emptiness check. With the default remote-daemon policy (no userDenyRead), an approvedsandbox_permissions: "require_escalated"shell runs withcommandEngine == nil— completely outside bubblewrap/seatbelt — even though the token is in the profileDenyReadviacredentialDenyReadPaths. A remote-controlled session that learns the token path (for example vialist_directory) can therefore read the bearer secret through an escalation the user approved for unrelated host-state work.TestBashRequireEscalatedKeepsSandboxWhenDeniedReadsActiveonly covers userpolicy.DenyRead, not automatic credential denies. Please makeUnsandboxedExecutionAllowed(and the exec_command twin) treat configured protected credential paths the same as userDenyReadfor escalation gating, or blockrequire_escalatedoutright for remote-bridge sessions. -
[P3]
list_directorystill surfaces the protected token filename
internal/tools/list_directory.go:83-95
internal/tools/read_exclusions.go:22-30
This PR wires the new automatic credential exclusion intoEngine.ReadExclusions()and the grep/glob walkers (sandboxReadExcluder), butlist_directorynever consults that matcher — it listsos.ReadDirentries directly after the registry gate passes on the requested directory path. A remote session whose token file lives in the workspace can therefore learn the basename (for examplebridge-token) even thoughread_file, grep, and glob are blocked. Content does not leak, and the same omission exists for userDenyRead, but this is the one discovery tool left out of the new protected-credential path. Please filter listed entries throughReadExclusions().PathExcluded(same pattern as grep/glob) or add a focused regression alongsidedaemon_token_exclusion_test.go.
…protect-daemon-token-file
Amp-Thread-ID: https://ampcode.com/threads/T-019fa018-e3c8-705b-9627-3a7564f8d66a Co-authored-by: Pierre Bruno <pierrebruno@hotmail.ch>
|
Final follow-up on current head
The PR is mergeable. The only remaining inline thread is the future #681 sequencing note; #681 is still open. PierrunoYT requests a fresh human review to supersede the previous |
…n-token-file # Conflicts: # internal/sandbox/manager_test.go # internal/sandbox/profile.go
|
Pushed The expected #681 conflicts were resolved by retaining its expanded credential inventory and environment overrides while carrying Re-verified the latest security requests against the merged tree:
Validation on Go 1.26.5/linux-amd64:
The sole unresolved inline thread was the anticipated sibling-PR sequencing note; this merge addresses that integration. Fresh review requested for the merged head. |
jatmn
left a comment
jatmn
left a comment
There was a problem hiding this comment.
I rechecked head f833e42b against the earlier review threads. The blocking items from prior rounds look addressed on the current tree: in-process credential exclusion, macOS credential write-deny scoping, ModeDisabled tool protection, daemon-boundary canonicalization, case folding on darwin/windows, mandatory token deny under AllowRead, require_escalated gating, and list_directory read exclusions. CI is green and the branch is mergeable.
One follow-up remains.
Findings
- [P2] Workspace symlink/hardlink aliases can bypass the protected-token pathname check
internal/sandbox/pathlists.go:167-178
protectedPathDeniedcompares only the requested spelling viapathUnderProtectedRoot→normalizePathForPolicyRoot, which deliberately leaves in-workspace symlink components unresolved (scope.go:286-288).validateWorkspacePaththen permits symlinks whose target stays inside the workspace (paths.go:67-74). In the common remote-daemon fixture (token file inside the session workspace), an agent can runbashto createln -s bridge-token leak(or a hard link), then callread_file,write_file,edit_file, orapply_patchonleak;Evaluateallows the request becauseleakis not spelled like the protected path, and the tool layer follows the alias to the bearer file (workspace.go:57-60).grephas the same shape:walkGrepFilescallsexclude.fileExcluded(path)on the unresolved walk path (grep.go:302-304) butscanGrepFileopens the symlink-resolved target (grep.go:365-383). This is a real bypass ofprotectedCredentialPaths, but it is not a regression from this PR: user-configuredDenyReaduses the same lexicalpathUnderPolicyRootmodel, so any workspace-local deny-by-pathname rule is aliasable the same way. Please either resolve the request path (or compare inode identity) insideprotectedPathDeniedbefore returning nil — with an end-to-end regression coveringread_file,write_file, andgrep— or document that credential protection intentionally matches existingDenyReadpathname semantics and does not block in-workspace aliases.
Amp-Thread-ID: https://ampcode.com/threads/T-019fa7a0-1223-701d-9529-48ba5d7cf8c8 Co-authored-by: Pierre Bruno <pierrebruno@hotmail.ch>
|
PierrunoYT addressed the remaining protected-token alias bypass in
Validation: focused sandbox/tools tests pass with |
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.
The in-process daemon-token boundary on fc777ca looks sound after rebasing: alias closure via SameFile, ModeDisabled tool protection, escalation gating, and list_directory exclusions in the agent path all check out. What blocks merge today is stale-branch drift against current main, not a broken core patch. The items below separate merge prerequisites from PR-scoped follow-ups and pre-existing platform limits.
Findings
-
[P1] Rebase onto current
mainand re-run CI on the resolved tree
internal/sandbox/profile.go,internal/sandbox/runner.go,internal/sandbox/runner_test.go,internal/sandbox/protected_credentials_test.go,internal/tools/daemon_token_exclusion_test.go,internal/tools/exec_command_test.go
origin/mainhas moved ahead of this branch (3e7692d#818 env scrub,33c94ec#816 git credential denies,1c38e8c#811 provider-command timeout).git merge-treereportschanged in bothon the files above, and captured GitHub metadata showsmergeStateStatus: DIRTY/mergeable: CONFLICTING. Recent author comments say the PR is mergeable; that does not match the current GitHub state. None of the daemon-token changes should land until this is rebased and conflicts are resolved. -
[P1] Restore #816 git credential-store denies during the rebase (stale-branch regression, not intentional PR work)
internal/sandbox/profile.go,internal/sandbox/profile_test.go
Relative to the PR base (8acd42a3, which already includes #816), headfc777caremovesfilepath.Join(home, ".git-credentials")andfilepath.Join(configHome, "git", "credentials")fromcredentialDenyReadPathsForEnvironmentand dropsprofile_test.go. That is branch drift, not part of the 15-file daemon-token diff, but merging without fixing it would reopen cleartext git credential reads for sandboxed commands. Keep the #816 candidates and tests while integrating the daemon-tokenmandatorylist. -
[P1] Restore #811 provider-command timeout hardening during the rebase (stale-branch regression)
internal/config/command.go
The same base-to-head comparison rolls backsyncBuffer,awaitWithGrace, deadline-before-start wiring, and richer timeout errors inLoadProviderCommand/runProviderCommand. This file is outside the PR scope and should come back verbatim frommain; it is not a deliberate change in the daemon-token patch. -
[P2] macOS sandboxed shell can still read the token through a workspace hard-link alias
internal/sandbox/pathlists.go:133-169,internal/sandbox/runner.go(denyReadRules/credentialDenyWriteRules)
fc777cacloses inode aliases for built-in tools viaos.SameFile, but the OS profile still denies only configured pathname literals. On macOS, a sandboxedbash/exec_commandcanln bridge-token aliasandcat aliasbecause seatbelt rules are path-based, not inode-based. This is the same shell-side model as user-configuredDenyRead, but it leaves a real bypass for the credential this PR is hardening.TestDaemonTokenAliasesDeniedEndToEndcovers built-in tools only. Please add a shell regression on darwin or document that OS-layer protection intentionally matches existingDenyReadpathname semantics. -
[P2]
ModeDisabledleaves shell outside bridge-token protection
internal/sandbox/pathlists.go:107-111,internal/sandbox/engine.go:331-339,internal/sandbox/profile.go:76-80,internal/sandbox/runner.go:179-180,internal/tools/bash.go:241-245
protectedCredentialPathBlockhandles onlySideEffectRead,SideEffectWrite, andSideEffectOutOfWorkspace, sobash/exec_command(SideEffectShell) is allowed underModeDisabledwhilePermissionProfileFromPolicyreturnsFileSystemUnrestrictedand the runner setsSandboxPreferenceForbid. In-process file tools stay blocked; shell does not.UnsandboxedExecutionAllowed()also returnstrueforModeDisabledbefore consultingprotectedCredentialPaths(), sorequire_escalatedcan drop the native sandbox entirely. The code comments treat shell as a separate boundary; if remote-bridge sessions can run with sandbox disabled, please either keep shell sandboxed when a token file is configured or state explicitly thatModeDisabledis shell-open by design.
Accepted residuals (not merge blockers for this PR)
-
Windows shell OS deny-read (#662).
credentialDenyReadPathsstill returnsnilon Windows, so wrapped shell commands have no profileDenyReadfor the token. This PR adds new in-process protection on Windows; it does not regress OS-level shell behavior. Track under #662 unless Windows shell coverage is in scope here. -
Literal
~/…token paths.protectedCredentialPathsmatchesos.ReadFile(no~expansion) whilecredentialDenyReadPathsusesnormalizeProfilePath(does expand~). Obscure misconfiguration only; normalserve-remotepaths are absolute afterCanonicalizeTokenFileEnv. -
MCP
list_directory.internal/mcp/server.gocallsRunWithOptionswithoutSandbox, so MCP clients do not get read exclusions. Pre-existing wiring; this PR fixed the agent-loop path only. -
Linux shell alias/write concerns. Linux
DenyReaduses--ro-bind /dev/nullon the token path, which masks the host inode for that pathname; the macOS seatbelt write-allow problem and separate Linux write-deny rule do not apply the same way. No Linux shell hard-link read bypass was verified on the current tree.
…n-token-file # Conflicts: # internal/sandbox/runner.go
Two properties of the token protection were only implicit, and both are the kind that get mistaken for bugs or, worse, for guarantees. Inode aliasing is closed by the in-process tools alone. They see a path before opening it, so a symlink or hard link resolves back to the protected inode. Seatbelt and Bubblewrap rules name paths, so a sandboxed shell on macOS can still hard-link the token to a fresh name and read that — the same pathname model a user-configured DenyRead has always had, and not something either backend's policy language can express otherwise. ModeDisabled is shell-open by design. In that mode no wrapper is built at all, so there is no layer left to confine a command and nothing for an escalation to bypass; re-wrapping shell for this one file would undo the switch the operator flipped without holding. The exclusion is a guarantee about Zero's own file tools, not about the machine. Both are now documented where the decision lives, and the ModeDisabled boundary is pinned by a test so changing it has to be deliberate. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
Addressed all five findings. Branch is [P1] Integrated current Only one file conflicted: The resolved tree now differs from [P1] #816 git credential denies are back and integrated, not merely restored. [P1] #811 provider-command timeout is back verbatim. [P2] macOS hard-link alias — documented, per your second option. I took the documentation route rather than a darwin shell regression, for a reason I want to be straight about: I can't run or verify a seatbelt test from this host, and a test I can't execute is worse than an honest comment. The comment on [P2] I left the four accepted residuals alone as you scoped them. Validation on the merged tree
|
Vasanthdev2004
left a comment
Vasanthdev2004
left a comment
There was a problem hiding this comment.
Reviewed this properly. Not blocking: @jatmn's approval should stand and nothing below is a correctness or security defect. Notes worth your time though, and one answer you specifically need.
It does not collide with #818. I merged #818 yesterday, which adds ZERO_DAEMON_REMOTE_TOKEN_FILE to scrubSensitiveEnv so a sandboxed command never inherits the path. That is the pointer; this is the file. They complement rather than duplicate, and this still applies cleanly. I checked because if your approach superseded my one-liner I would rather revert mine than have two mechanisms.
The one I would actually fix
protectedPathDenied (internal/sandbox/pathlists.go:141) has no len(protected) == 0 early-out, so it runs filepath.EvalSymlinks (:174) and os.Stat (:178) before looping over nothing. It is the first predicate in PathExcluded (:368) and DirExcluded (:384), and Active() is true on denyRoots alone, so a user with a DenyRead entry and no token configured pays both syscalls on every entry visited by grep, glob and list_directory.
Being straight about magnitude, because my first measurement was wrong: alternating same-process A/B puts it at roughly +7.5% median for the no-token case, not the 27% I first got. The guard is free and semantics-preserving, since both return true sites are inside the loop over protected.
The bridge-daemon case is the larger one and the guard does not help it: with protected entries present the cost is roughly +107%, dominated by the per-entry NormalizePrefixForRoot component walk at :142-146, and protectedCredentialPaths can return two entries. If that matters, hoist the normalization rather than adding the empty guard. I would not cache os.Stat results at construction, since that weakens the inode-alias closure at :148-166: a FileInfo captured early goes stale if the token file is replaced mid-session.
Coverage gaps
Three behaviour-bearing paths delete clean with the suite still green:
- the
|| len(protectedCredentialPaths()) > 0disjunct atpathlists.go:475 - the protected-path branch in
DirExcludedatpathlists.go:384 remote.CanonicalizeTokenFileEnv()atinternal/cli/daemon.go:483, which is the only thing pinning the variable before workers inherit it, andauth.go:128is its sole writer in the tree
Given the PR states the exclusion as absolute, those three are the ones I would want pinned.
The question for you
selectedDaemonRemoteTokenFile() (pathlists.go:56-61) returns "" whenever ZERO_DAEMON_REMOTE_TOKEN is non-empty, so the file's protection is conditional on the daemon selecting the file form, and nothing tells the operator when it is not selected. Is that intended? If so it is worth a line in the docs, because "the token file is protected" reads as unconditional right now.
Also worth knowing
On macOS the seatbelt profile names the token by literal path only, so the protection does not follow an alias to the same inode the way the in-process check does. Lower severity than it sounds, but the two layers disagree.
Four larger claims I chased did not survive checking, including that this disables escalated execution on Windows and that it adds no OS-level protection there. Mentioning it so you know those were considered rather than missed.
Amp-Thread-ID: https://ampcode.com/threads/T-019fafa2-7d9d-75bc-8801-6d0efc8db1df Co-authored-by: Pierre Bruno <pierrebruno@hotmail.ch>
|
PierrunoYT completed the remaining daemon-token follow-ups in
Validation passed: @jatmn @Vasanthdev2004 — PierrunoYT requests a fresh review of the current head. The account does not have permission to use GitHub’s reviewer-request button. |
Summary
ZERO_DAEMON_REMOTE_TOKEN_FILEfrom inherited sandbox command environmentsAllowReadopt-out behaviorRoot 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 existing normalized target is protected through the same credential deny-read mechanism used for
GOOGLE_APPLICATION_CREDENTIALSon 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 test ./internal/sandbox -count=1go vet ./...git diff --checkThe repository-pinned
golangci-lint@v2.12.2andgovulncheck@v1.3.0currently build with Go 1.25.12 and cannot load this repository's Go 1.26.5 packages.Summary by CodeRabbit