From f6e019151ad859614f120fa4d27a69b2e3d29a40 Mon Sep 17 00:00:00 2001 From: PierrunoYT Date: Tue, 14 Jul 2026 21:42:47 +0200 Subject: [PATCH 01/10] fix(sandbox): protect daemon token file --- internal/sandbox/manager_test.go | 37 +++++++++++++++++++++++++++----- internal/sandbox/profile.go | 21 ++++++++++++------ internal/sandbox/runner.go | 1 + internal/sandbox/runner_test.go | 1 + 4 files changed, 49 insertions(+), 11 deletions(-) diff --git a/internal/sandbox/manager_test.go b/internal/sandbox/manager_test.go index a2b8550f2..90ed371b8 100644 --- a/internal/sandbox/manager_test.go +++ b/internal/sandbox/manager_test.go @@ -385,9 +385,13 @@ func TestCredentialDenyReadPathsIn(t *testing.T) { if err := os.WriteFile(keyFile, []byte("{}"), 0o600); err != nil { t.Fatal(err) } + daemonTokenFile := filepath.Join(home, "daemon-token") + if err := os.WriteFile(daemonTokenFile, []byte("secret"), 0o600); err != nil { + t.Fatal(err) + } - paths := credentialDenyReadPathsIn(home, keyFile, nil) - for _, want := range normalizeProfilePaths([]string{awsDir, gcloudDir, keyFile}) { + paths := credentialDenyReadPathsIn(home, keyFile, daemonTokenFile, nil) + for _, want := range normalizeProfilePaths([]string{awsDir, gcloudDir, keyFile, daemonTokenFile}) { if !stringSliceContains(paths, want) { t.Errorf("credential deny paths = %#v, want %q included", paths, want) } @@ -399,22 +403,45 @@ func TestCredentialDenyReadPathsIn(t *testing.T) { } // An explicit AllowRead entry covering a store is an opt-out. - optedOut := credentialDenyReadPathsIn(home, keyFile, []string{awsDir}) + optedOut := credentialDenyReadPathsIn(home, keyFile, daemonTokenFile, []string{awsDir, daemonTokenFile}) if stringSliceContains(optedOut, normalizeProfilePaths([]string{awsDir})[0]) { t.Errorf("credential deny paths = %#v, want AllowRead opt-out to drop ~/.aws", optedOut) } if !stringSliceContains(optedOut, normalizeProfilePaths([]string{keyFile})[0]) { t.Errorf("credential deny paths = %#v, want unrelated entries kept after opt-out", optedOut) } + if stringSliceContains(optedOut, normalizeProfilePaths([]string{daemonTokenFile})[0]) { + t.Errorf("credential deny paths = %#v, want AllowRead opt-out to drop daemon token file", optedOut) + } - if got := credentialDenyReadPathsIn(" ", "", nil); len(got) != 0 { + if got := credentialDenyReadPathsIn(" ", "", "", nil); len(got) != 0 { t.Errorf("credential deny paths for blank home = %#v, want none", got) } // The GOOGLE_APPLICATION_CREDENTIALS target stays protected even when no // home directory is resolvable. - homeless := credentialDenyReadPathsIn("", keyFile, nil) + homeless := credentialDenyReadPathsIn("", keyFile, daemonTokenFile, nil) if !stringSliceContains(homeless, normalizeProfilePaths([]string{keyFile})[0]) { t.Errorf("credential deny paths without home = %#v, want key file included", homeless) } + if !stringSliceContains(homeless, normalizeProfilePaths([]string{daemonTokenFile})[0]) { + t.Errorf("credential deny paths without home = %#v, want daemon token file included", homeless) + } +} + +func TestPermissionProfileDeniesDaemonTokenFile(t *testing.T) { + if runtime.GOOS == "windows" { + t.Skip("credential deny-read paths are disabled on Windows pending the ACL model") + } + tokenFile := filepath.Join(t.TempDir(), "daemon-token") + if err := os.WriteFile(tokenFile, []byte("secret"), 0o600); err != nil { + t.Fatal(err) + } + t.Setenv("ZERO_DAEMON_REMOTE_TOKEN_FILE", tokenFile) + + profile := PermissionProfileFromPolicy(t.TempDir(), DefaultPolicy(), nil) + want := normalizeProfilePaths([]string{tokenFile})[0] + if !stringSliceContains(profile.FileSystem.DenyRead, want) { + t.Fatalf("DenyRead = %#v, want daemon token file %q", profile.FileSystem.DenyRead, want) + } } diff --git a/internal/sandbox/profile.go b/internal/sandbox/profile.go index 194736667..bc818add7 100644 --- a/internal/sandbox/profile.go +++ b/internal/sandbox/profile.go @@ -148,9 +148,10 @@ func permissionProfileReadRoots(workspaceRoot string, policy Policy, scope *Scop } // credentialDenyReadPaths returns default deny-read entries for well-known -// credential stores (~/.aws, ~/.config/gcloud, ~/.azure, and the file -// GOOGLE_APPLICATION_CREDENTIALS points to) so sandboxed commands cannot read -// cloud secrets under the read-all workspace posture. Two deliberate limits: +// credential stores (~/.aws, ~/.config/gcloud, ~/.azure, and the files named by +// GOOGLE_APPLICATION_CREDENTIALS and ZERO_DAEMON_REMOTE_TOKEN_FILE) so +// sandboxed commands cannot read secrets under the read-all workspace posture. +// Two deliberate limits: // // - Windows is skipped: a non-empty profile DenyRead switches the Windows // runner onto the capability-SID/ACL deny path and away from the @@ -167,14 +168,19 @@ func credentialDenyReadPaths(policy Policy) []string { return nil } // A failed home lookup only drops the home-based candidates; the - // GOOGLE_APPLICATION_CREDENTIALS target must be protected regardless. + // environment-selected credential targets must be protected regardless. home, _ := os.UserHomeDir() - return credentialDenyReadPathsIn(home, os.Getenv("GOOGLE_APPLICATION_CREDENTIALS"), policy.AllowRead) + return credentialDenyReadPathsIn( + home, + os.Getenv("GOOGLE_APPLICATION_CREDENTIALS"), + os.Getenv("ZERO_DAEMON_REMOTE_TOKEN_FILE"), + policy.AllowRead, + ) } // 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 { var candidates []string if home = strings.TrimSpace(home); home != "" { candidates = append(candidates, @@ -186,6 +192,9 @@ func credentialDenyReadPathsIn(home string, googleCredentials string, allowRead if target := strings.TrimSpace(googleCredentials); target != "" { candidates = append(candidates, target) } + if target := strings.TrimSpace(daemonTokenFile); target != "" { + candidates = append(candidates, target) + } allowRoots := normalizeProfilePaths(allowRead) out := make([]string, 0, len(candidates)) for _, path := range normalizeProfilePaths(candidates) { diff --git a/internal/sandbox/runner.go b/internal/sandbox/runner.go index 506acb393..5cc256fae 100644 --- a/internal/sandbox/runner.go +++ b/internal/sandbox/runner.go @@ -1006,6 +1006,7 @@ func scrubSensitiveEnv(env []string, additionalKeys ...string) []string { "GH_TOKEN", "ZERO_WEBSEARCH_API_KEY", "ZERO_DAEMON_REMOTE_TOKEN", + "ZERO_DAEMON_REMOTE_TOKEN_FILE", } for _, descriptor := range providercatalog.All() { for _, key := range descriptor.AuthEnvVars { diff --git a/internal/sandbox/runner_test.go b/internal/sandbox/runner_test.go index 67b66866c..882a007f7 100644 --- a/internal/sandbox/runner_test.go +++ b/internal/sandbox/runner_test.go @@ -698,6 +698,7 @@ func TestScrubSensitiveEnv(t *testing.T) { "ZERO_OAUTH_MY_SVC_CLIENT_SECRET=oauth-secret", "zero_oauth_second_client_secret=case-insensitive-secret", "ZERO_OAUTH_CLIENT_SECRET=not-a-provider-secret", + "ZERO_DAEMON_REMOTE_TOKEN_FILE=/home/user/daemon-token", "AWS_PROFILE=staging", "SAFE_VAR=hello", } From caccae4f67411d03ba952711d80f73ff5564506b Mon Sep 17 00:00:00 2001 From: PierrunoYT Date: Sat, 25 Jul 2026 12:51:05 +0200 Subject: [PATCH 02/10] fix(sandbox): protect the daemon token from Zero's own tools MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Addresses the four P1 findings on #685. In-process tool boundary (findings 1 and 2). The token file only entered PermissionProfile.FileSystem.DenyRead, which shapes the OS sandbox profile for wrapped shell commands. read_file resolves scoped paths itself and grep/glob build exclusions from the policy, so a remote-controlled agent whose token file sits in its session workspace could read it with read_file/grep/glob, or truncate and replace it with a write tool. A new automatic protected-credential list is enforced by validatePathWithPolicy (read, write, and out-of-workspace) and by ReadExclusions/ReadExclusionGlobs. It is deliberately not Policy.DenyRead — whose emptiness gates escalated execution — and it is not re-includable through AllowRead, AllowWrite, a granted permission, or a session/turn permission profile. It applies on Windows too, where filesystem deny-read has no sandbox representation. Seatbelt write direction (finding 2, macOS). A read-denied path kept its file-write-unlink deny only, while the broad file-write* allow covered every workspace and temp root, leaving a credential file there truncatable and replaceable. Read denies now also deny file-write*, which subsumes unlink. Daemon boundary (findings 3 and 4). serve-remote canonicalizes ZERO_DAEMON_REMOTE_TOKEN_FILE to the absolute, symlink-resolved path it actually reads before any worker inherits it, so a relative value no longer resolves against each session's directory (protecting a path that holds no token) and no link pathname reaches the derived deny rules — bubblewrap cannot mount over a symlink destination. It stays a no-op when ZERO_DAEMON_REMOTE_TOKEN supplies the token, so an unused or dangling file pointer neither changes the outcome nor fails the start. The in-process list protects both the configured pathname and its resolved target, so a stale symlinked pointer inherited from elsewhere leaves the link unreplaceable through Zero's tools. --- internal/cli/daemon.go | 6 + internal/daemon/remote/auth.go | 42 ++++ internal/daemon/remote/auth_test.go | 81 +++++++ internal/sandbox/engine.go | 7 +- internal/sandbox/pathlists.go | 102 ++++++++- .../sandbox/protected_credentials_test.go | 210 ++++++++++++++++++ internal/sandbox/runner.go | 8 +- internal/sandbox/runner_test.go | 8 +- internal/tools/daemon_token_exclusion_test.go | 129 +++++++++++ 9 files changed, 581 insertions(+), 12 deletions(-) create mode 100644 internal/sandbox/protected_credentials_test.go create mode 100644 internal/tools/daemon_token_exclusion_test.go diff --git a/internal/cli/daemon.go b/internal/cli/daemon.go index 5074312e4..dafede623 100644 --- a/internal/cli/daemon.go +++ b/internal/cli/daemon.go @@ -477,6 +477,12 @@ func runDaemonServeRemote(args []string, stdout io.Writer, stderr io.Writer) int if err != nil { return writeAppError(stderr, err.Error(), exitCrash) } + // Pin the token file to the path this process reads BEFORE any worker + // inherits the variable, so a relative or symlinked value cannot make a + // session's sandbox profile protect a different path than the live bearer file. + if err := remote.CanonicalizeTokenFileEnv(); err != nil { + return writeAppError(stderr, err.Error(), exitCrash) + } token, err := remote.TokenFromEnv() if err != nil { return writeAppError(stderr, err.Error(), exitCrash) diff --git a/internal/daemon/remote/auth.go b/internal/daemon/remote/auth.go index 4f1574a9d..abc44de52 100644 --- a/internal/daemon/remote/auth.go +++ b/internal/daemon/remote/auth.go @@ -23,6 +23,7 @@ import ( "fmt" "io" "os" + "path/filepath" "strings" "github.com/Gitlawb/zero/internal/daemon" @@ -86,6 +87,47 @@ func TokenFromEnv() (string, error) { return "", fmt.Errorf("remote: set %s or %s", EnvToken, EnvTokenFile) } +// CanonicalizeTokenFileEnv rewrites EnvTokenFile in this process's environment +// to the absolute, symlink-resolved path TokenFromEnv actually reads, so every +// child process — and the sandbox profile derived for it — refers to the same +// file this bridge authenticated against. `zero daemon serve-remote` calls it +// before it starts serving. +// +// Two mismatches motivate it. TokenFromEnv passes the value to os.ReadFile, so a +// relative value resolves against the STARTING process's working directory, +// while a worker inherits the same string and resolves it against its own +// session directory — the profile would then protect a path that holds no token +// while the real bearer file stays readable. Resolving symlinks up front also +// keeps a link pathname out of the derived deny rules, which matters because +// bubblewrap cannot mount over a symlink destination. +// +// It is a deliberate no-op when EnvToken supplies the token: TokenFromEnv +// prefers the inline value, so an unused (even dangling) file pointer must not +// change the outcome or fail the start. +func CanonicalizeTokenFileEnv() error { + if strings.TrimSpace(os.Getenv(EnvToken)) != "" { + return nil + } + configured := strings.TrimSpace(os.Getenv(EnvTokenFile)) + if configured == "" { + return nil + } + absolute, err := filepath.Abs(configured) + if err != nil { + return fmt.Errorf("remote: resolve token file %q: %w", configured, err) + } + resolved, err := filepath.EvalSymlinks(absolute) + if err != nil { + // TokenFromEnv would fail on the same path a moment later; reporting it here + // names the resolution step that failed. + return fmt.Errorf("remote: resolve token file %q: %w", configured, err) + } + if resolved == configured { + return nil + } + return os.Setenv(EnvTokenFile, resolved) +} + // Attestation is an optional post-token hook (e.g. workload attestation). The // default is a no-op; a deployment can supply a stricter implementation. type Attestation interface { diff --git a/internal/daemon/remote/auth_test.go b/internal/daemon/remote/auth_test.go index 448a6d920..4fef66b82 100644 --- a/internal/daemon/remote/auth_test.go +++ b/internal/daemon/remote/auth_test.go @@ -57,6 +57,87 @@ func TestTokenFromEnv(t *testing.T) { } } +// TestCanonicalizeTokenFileEnv pins the value every child process (and the +// sandbox profile derived for it) inherits to the file this process reads. +func TestCanonicalizeTokenFileEnv(t *testing.T) { + base, err := filepath.EvalSymlinks(t.TempDir()) + if err != nil { + t.Fatalf("EvalSymlinks: %v", err) + } + token := filepath.Join(base, "tok") + if err := os.WriteFile(token, []byte("from-file\n"), 0o600); err != nil { + t.Fatalf("write token file: %v", err) + } + + t.Run("relative value becomes absolute", func(t *testing.T) { + // A worker resolves the inherited value against its own session directory, + // so a relative value must not survive the daemon boundary. + t.Chdir(base) + t.Setenv(EnvToken, "") + t.Setenv(EnvTokenFile, "tok") + if err := CanonicalizeTokenFileEnv(); err != nil { + t.Fatalf("CanonicalizeTokenFileEnv: %v", err) + } + if got := os.Getenv(EnvTokenFile); got != token { + t.Fatalf("%s = %q, want %q", EnvTokenFile, got, token) + } + if tok, err := TokenFromEnv(); err != nil || tok != "from-file" { + t.Fatalf("TokenFromEnv after canonicalization = %q, %v", tok, err) + } + }) + + t.Run("symlinked pathname is resolved", func(t *testing.T) { + link := filepath.Join(base, "tok-link") + if err := os.Symlink(token, link); err != nil { + t.Skipf("symlink unsupported: %v", err) + } + t.Setenv(EnvToken, "") + t.Setenv(EnvTokenFile, link) + if err := CanonicalizeTokenFileEnv(); err != nil { + t.Fatalf("CanonicalizeTokenFileEnv: %v", err) + } + if got := os.Getenv(EnvTokenFile); got != token { + t.Fatalf("%s = %q, want the resolved target %q", EnvTokenFile, got, token) + } + }) + + t.Run("an inline token keeps precedence over a dangling pointer", func(t *testing.T) { + // TokenFromEnv prefers EnvToken, so an unused (even dangling) file pointer + // must neither fail the start nor be rewritten. + dangling := filepath.Join(base, "missing", "tok") + t.Setenv(EnvToken, "from-env") + t.Setenv(EnvTokenFile, dangling) + if err := CanonicalizeTokenFileEnv(); err != nil { + t.Fatalf("CanonicalizeTokenFileEnv with an inline token: %v", err) + } + if got := os.Getenv(EnvTokenFile); got != dangling { + t.Fatalf("%s = %q, want it left alone", EnvTokenFile, got) + } + if tok, err := TokenFromEnv(); err != nil || tok != "from-env" { + t.Fatalf("TokenFromEnv = %q, %v, want the inline token", tok, err) + } + }) + + t.Run("a selected but unreadable pointer fails closed", func(t *testing.T) { + t.Setenv(EnvToken, "") + t.Setenv(EnvTokenFile, filepath.Join(base, "missing", "tok")) + if err := CanonicalizeTokenFileEnv(); err == nil { + t.Fatal("a selected token file that cannot be resolved must error") + } + }) + + t.Run("no pointer is a no-op", func(t *testing.T) { + t.Setenv(EnvToken, "") + t.Setenv(EnvTokenFile, "") + if err := CanonicalizeTokenFileEnv(); err != nil { + t.Fatalf("CanonicalizeTokenFileEnv without a pointer: %v", err) + } + if got := os.Getenv(EnvTokenFile); got != "" { + t.Fatalf("%s = %q, want empty", EnvTokenFile, got) + } + }) +} + func TestServerTLSConfigRequiresCertKey(t *testing.T) { if _, err := ServerTLSConfig("", ""); err == nil { t.Fatal("ServerTLSConfig must require a cert and key (TLS mandatory)") diff --git a/internal/sandbox/engine.go b/internal/sandbox/engine.go index db66d54a9..92ef514fa 100644 --- a/internal/sandbox/engine.go +++ b/internal/sandbox/engine.go @@ -154,9 +154,10 @@ func (engine *Engine) ReadExclusions() *ReadExclusions { return nil } return &ReadExclusions{ - workspaceRoot: engine.workspaceRoot, - denyRoots: resolvePolicyPaths(policy.DenyRead), - allowRoots: resolvePolicyPaths(policy.AllowRead), + workspaceRoot: engine.workspaceRoot, + denyRoots: resolvePolicyPaths(policy.DenyRead), + allowRoots: resolvePolicyPaths(policy.AllowRead), + protectedRoots: protectedCredentialPaths(), } } diff --git a/internal/sandbox/pathlists.go b/internal/sandbox/pathlists.go index f6815a2c7..5700aa793 100644 --- a/internal/sandbox/pathlists.go +++ b/internal/sandbox/pathlists.go @@ -41,6 +41,62 @@ func resolvePolicyPath(entry string) (string, bool) { return resolved, true } +// daemonRemoteTokenFileEnv names the file holding the remote bridge's bearer +// token. It is duplicated from internal/daemon/remote (which cannot be imported +// here) exactly like the copy scrubSensitiveEnv keeps. +const daemonRemoteTokenFileEnv = "ZERO_DAEMON_REMOTE_TOKEN_FILE" + +// protectedCredentialPaths returns credential files that Zero's own in-process +// file tools must never read or modify, independent of Policy. +// +// This is deliberately separate from credentialDenyReadPaths, which only shapes +// the OS sandbox profile for wrapped shell commands: read_file resolves scoped +// paths itself, and grep/glob build their exclusions from the policy, so a +// profile-only rule leaves the in-process tool boundary open. It is also +// separate from Policy.DenyRead, whose emptiness gates escalated (unsandboxed) +// execution and must keep reflecting user configuration alone. +// +// Entries here are NOT re-includable through AllowRead/AllowWrite or a +// session/turn permission profile: the bridge bearer token grants control of +// this daemon, so a remote-controlled agent must not be able to read it, nor +// replace it to hijack the next bridge start, even when the file sits inside +// its own session workspace. Unlike the profile list this applies on Windows +// too, where filesystem deny-read has no sandbox representation (#662). +// +// Both the configured pathname and the target it currently resolves to are +// protected: `zero daemon serve-remote` canonicalizes the value it selects (see +// remote.CanonicalizeTokenFileEnv), but a stale symlinked value inherited from +// elsewhere must not leave the link replaceable. +func protectedCredentialPaths() []string { + // os.ReadFile — the daemon's own reader — treats the value literally, so a + // relative path resolves against the working directory and a leading "~" is + // NOT expanded. resolvePolicyPath would expand it and protect the wrong file. + configured := strings.TrimSpace(os.Getenv(daemonRemoteTokenFileEnv)) + if configured == "" { + return nil + } + absolute, err := filepath.Abs(configured) + if err != nil { + return nil + } + paths := []string{absolute} + if resolved, err := filepath.EvalSymlinks(absolute); err == nil && resolved != absolute { + paths = append(paths, resolved) + } + return dedupeStrings(paths) +} + +// protectedPathDenied reports whether path targets one of the protected +// credential files. There is no allow-list consultation by design. +func protectedPathDenied(protected []string, workspaceRoot, path string) bool { + for _, entry := range protected { + if pathUnderPolicyRoot(path, entry, workspaceRoot) { + return true + } + } + return false +} + // resolvePolicyPaths resolves and de-duplicates a list of policy path entries, // dropping blanks and non-existent entries. Files and directories are both kept // (a DenyRead/DenyWrite entry may target a single sensitive file). @@ -151,20 +207,28 @@ type ReadExclusions struct { workspaceRoot string denyRoots []string allowRoots []string + // protectedRoots are the automatic credential exclusions + // (protectedCredentialPaths); AllowRead never re-includes them. + protectedRoots []string } -// Active reports whether any DenyRead root is configured. When false the -// exclusions are a no-op and the search behaves exactly as before. +// Active reports whether anything is excluded: a configured DenyRead root or an +// automatic protected credential path. When false the exclusions are a no-op and +// the search behaves exactly as before. func (rx *ReadExclusions) Active() bool { - return rx != nil && len(rx.denyRoots) > 0 + return rx != nil && (len(rx.denyRoots) > 0 || len(rx.protectedRoots) > 0) } // PathExcluded reports whether reading path is excluded by DenyRead, honoring a -// more-specific AllowRead re-inclusion. It is the per-file predicate for a walk. +// more-specific AllowRead re-inclusion, or by an automatic credential exclusion, +// which no allow entry re-includes. It is the per-file predicate for a walk. func (rx *ReadExclusions) PathExcluded(path string) bool { if !rx.Active() { return false } + if protectedPathDenied(rx.protectedRoots, rx.workspaceRoot, path) { + return true + } return readDeniedResolved(rx.workspaceRoot, rx.denyRoots, rx.allowRoots, path) } @@ -176,6 +240,11 @@ func (rx *ReadExclusions) DirExcluded(path string) bool { if !rx.Active() { return false } + // A protected credential entry is a file, so it only prunes a directory when + // the directory IS that entry; PathExcluded still filters it during the walk. + if protectedPathDenied(rx.protectedRoots, rx.workspaceRoot, path) { + return true + } if !readDeniedResolved(rx.workspaceRoot, rx.denyRoots, rx.allowRoots, path) { return false } @@ -212,6 +281,16 @@ func allowWriteScope(policy Policy) *Scope { // enforceWorkspace; the workspace boundary itself applies only when // enforceWorkspace. It never bypasses the symlink/out-of-workspace guards. func validateWritePath(scope *Scope, policy Policy, enforceWorkspace bool, workspaceRoot, path string) *pathBlock { + // The protected credential files outrank every allow: overwriting or + // truncating the bridge token denies service, and replacing it hands the next + // bridge start an attacker-chosen secret. + if protectedPathDenied(protectedCredentialPaths(), workspaceRoot, path) { + return &pathBlock{ + Code: BlockDenied, + Path: path, + Reason: path + " holds the remote bridge token and is never writable", + } + } // DenyWrite wins regardless of workspace enforcement. for _, deny := range resolvePolicyPaths(policy.DenyWrite) { if pathUnderPolicyRoot(path, deny, workspaceRoot) { @@ -252,7 +331,9 @@ func validatePathWithPolicy(scope *Scope, policy Policy, sideEffect SideEffect, // when there is anything to enforce; otherwise it is a no-op (unchanged from the // pre-path-list behavior, where an empty workspace root skipped validation). if workspaceRoot == "" && !filepath.IsAbs(path) { - if enforceWorkspace || policyHasPathLists(policy) { + // A configured bridge token counts as something to enforce: the relative + // path cannot be anchored, so it cannot be proven to miss the token file. + if enforceWorkspace || policyHasPathLists(policy) || len(protectedCredentialPaths()) > 0 { return &pathBlock{ Code: BlockOutsideWorkspace, Path: path, @@ -263,6 +344,13 @@ func validatePathWithPolicy(scope *Scope, policy Policy, sideEffect SideEffect, } switch sideEffect { case SideEffectRead: + if protectedPathDenied(protectedCredentialPaths(), workspaceRoot, path) { + return &pathBlock{ + Code: BlockDenied, + Path: path, + Reason: path + " holds the remote bridge token and is never readable", + } + } if readDenied(policy, workspaceRoot, path) { return &pathBlock{ Code: BlockDenied, @@ -325,7 +413,9 @@ func workspaceRelGlob(workspaceRoot, target string) (string, bool) { // consumer. Empty when DenyRead is unset (the default), so search behavior is // unchanged. func ReadExclusionGlobs(policy Policy, scope *Scope) []string { - denyRoots := resolvePolicyPaths(policy.DenyRead) + // The automatic credential exclusions ride along so an rg-based consumer never + // walks the bridge token when it happens to live inside the workspace. + denyRoots := dedupeStrings(append(resolvePolicyPaths(policy.DenyRead), protectedCredentialPaths()...)) if len(denyRoots) == 0 || scope == nil { return nil } diff --git a/internal/sandbox/protected_credentials_test.go b/internal/sandbox/protected_credentials_test.go new file mode 100644 index 000000000..34720d1a2 --- /dev/null +++ b/internal/sandbox/protected_credentials_test.go @@ -0,0 +1,210 @@ +package sandbox + +import ( + "context" + "os" + "path/filepath" + "runtime" + "strings" + "testing" +) + +// protectedTokenFixture writes a bridge token inside the workspace and points +// ZERO_DAEMON_REMOTE_TOKEN_FILE at it. +func protectedTokenFixture(t *testing.T) (string, string) { + t.Helper() + ws, err := filepath.EvalSymlinks(t.TempDir()) + if err != nil { + t.Fatalf("EvalSymlinks: %v", err) + } + token := filepath.Join(ws, "bridge-token") + if err := os.WriteFile(token, []byte("secret\n"), 0o600); err != nil { + t.Fatalf("write token: %v", err) + } + t.Setenv(daemonRemoteTokenFileEnv, token) + return ws, token +} + +func TestProtectedCredentialPathsResolveLikeTheDaemonReader(t *testing.T) { + base, err := filepath.EvalSymlinks(t.TempDir()) + if err != nil { + t.Fatalf("EvalSymlinks: %v", err) + } + token := filepath.Join(base, "token") + if err := os.WriteFile(token, []byte("secret\n"), 0o600); err != nil { + t.Fatalf("write token: %v", err) + } + + t.Run("absent variable protects nothing", func(t *testing.T) { + t.Setenv(daemonRemoteTokenFileEnv, "") + if got := protectedCredentialPaths(); len(got) != 0 { + t.Fatalf("protected paths = %#v, want none", got) + } + }) + + t.Run("relative value resolves against the working directory", func(t *testing.T) { + // os.ReadFile — what the daemon uses — resolves a relative value against the + // working directory, so the protected path must do the same. + t.Chdir(base) + t.Setenv(daemonRemoteTokenFileEnv, "token") + if got := protectedCredentialPaths(); !stringSliceContains(got, token) { + t.Fatalf("protected paths = %#v, want %q", got, token) + } + }) + + t.Run("a literal tilde is not home-expanded", func(t *testing.T) { + // os.ReadFile treats "~" as an ordinary directory name; expanding it here + // would protect a path the daemon never reads. + t.Chdir(base) + t.Setenv(daemonRemoteTokenFileEnv, filepath.Join("~", "token")) + want := filepath.Join(base, "~", "token") + got := protectedCredentialPaths() + if !stringSliceContains(got, want) { + t.Fatalf("protected paths = %#v, want literal %q", got, want) + } + home, err := os.UserHomeDir() + if err == nil && stringSliceContains(got, filepath.Join(home, "token")) { + t.Fatalf("protected paths = %#v, must not home-expand the value", got) + } + }) + + t.Run("a symlinked pathname protects the link and its target", func(t *testing.T) { + if runtime.GOOS == "windows" { + t.Skip("symlink creation needs elevation on Windows") + } + link := filepath.Join(base, "token-link") + if err := os.Symlink(token, link); err != nil { + t.Skipf("symlink unsupported: %v", err) + } + t.Setenv(daemonRemoteTokenFileEnv, link) + got := protectedCredentialPaths() + for _, want := range []string{link, token} { + if !stringSliceContains(got, want) { + t.Fatalf("protected paths = %#v, want %q", got, want) + } + } + }) +} + +// TestProtectedCredentialsSurviveAllowRead locks in the non-opt-out guarantee: +// the bridge token grants control of the daemon, so neither AllowRead, an +// AllowWrite root, nor a granted permission may re-include it. +func TestProtectedCredentialsSurviveAllowRead(t *testing.T) { + ws, token := protectedTokenFixture(t) + policy := Policy{ + Mode: ModeEnforce, + EnforceWorkspace: true, + AllowRead: []string{ws, token}, + AllowWrite: []string{ws}, + } + scope, err := NewScope(ws, nil) + if err != nil { + t.Fatalf("NewScope: %v", err) + } + + for _, sideEffect := range []SideEffect{SideEffectRead, SideEffectWrite, SideEffectOutOfWorkspace} { + block := validatePathWithPolicy(scope, policy, sideEffect, true, ws, token) + if block == nil || !strings.Contains(block.Reason, "remote bridge token") { + t.Fatalf("%s on the bridge token: block = %#v, want a bridge-token deny", sideEffect, block) + } + } + + // The search-walk matcher enforces the same exclusion without consulting + // AllowRead, and it is active even though DenyRead is empty. + engine := NewEngine(EngineOptions{WorkspaceRoot: ws, Policy: policy, Scope: scope}) + rx := engine.ReadExclusions() + if !rx.Active() { + t.Fatal("read exclusions must be active for the automatic credential deny") + } + if !rx.PathExcluded(token) { + t.Fatalf("read exclusions must exclude the bridge token %q", token) + } + if rx.PathExcluded(filepath.Join(ws, "main.go")) { + t.Fatal("read exclusions must not exclude ordinary workspace files") + } + if globs := ReadExclusionGlobs(policy, scope); !stringSliceContains(globs, "!bridge-token") { + t.Fatalf("read exclusion globs = %#v, want the bridge token excluded", globs) + } +} + +// TestProtectedCredentialsRejectSessionPermissionProfile covers the other +// re-inclusion route: a session/turn permission profile that asks for the token +// path must not be auto-applicable. +func TestProtectedCredentialsRejectSessionPermissionProfile(t *testing.T) { + ws, token := protectedTokenFixture(t) + scope, err := NewScope(ws, nil) + if err != nil { + t.Fatalf("NewScope: %v", err) + } + engine := NewEngine(EngineOptions{ + WorkspaceRoot: ws, + Policy: Policy{Mode: ModeEnforce, EnforceWorkspace: true}, + Scope: scope, + }) + if engine.CoversRequestPermissions(RequestPermissionProfile{ + FileSystem: &FileSystemPermissions{Read: []string{token}}, + }) { + t.Fatal("a permission request covering the bridge token must not read as already-granted") + } + if !engine.CoversRequestPermissions(RequestPermissionProfile{ + FileSystem: &FileSystemPermissions{Read: []string{filepath.Join(ws, "main.go")}}, + }) { + t.Fatal("an ordinary workspace read request must stay covered by policy") + } +} + +// TestProtectedCredentialsDenyReadAndWriteInSeatbeltProfile covers the macOS +// backend: a token under a writable root was read-denied but still truncatable +// through the broad write allow. +func TestProtectedCredentialsDenyReadAndWriteInSeatbeltProfile(t *testing.T) { + token := "/private/tmp/zero-bridge-token" + profile := PermissionProfile{ + FileSystem: FileSystemPolicy{ + Kind: FileSystemRestricted, + ReadRoots: []string{string(filepath.Separator)}, + WriteRoots: []WritableRoot{{Root: "/repo"}}, + DenyRead: []string{token}, + AllowTemp: true, + }, + Network: NetworkPolicy{Mode: NetworkDeny}, + } + sbpl := seatbeltProfileFromPermissionProfile(profile, Policy{Mode: ModeEnforce}, "") + escaped := sandboxProfileString(normalizeProfilePath(token)) + denyRead := `(deny file-read* (literal "` + escaped + `"))` + denyWrite := `(deny file-write* (literal "` + escaped + `"))` + for _, want := range []string{denyRead, denyWrite} { + if !strings.Contains(sbpl, want) { + t.Fatalf("Seatbelt profile missing %q:\n%s", want, sbpl) + } + } + // Seatbelt is last-match-wins, so both denials must follow the broad allow. + if allow := strings.Index(sbpl, "(allow file-write*"); allow < 0 || strings.Index(sbpl, denyWrite) < allow { + t.Fatalf("the write denial must follow the broad write allow:\n%s", sbpl) + } +} + +// TestProtectedCredentialsDoNotBlockUnrelatedRequests keeps the exclusion inert +// for everyone who does not run the remote bridge. +func TestProtectedCredentialsDoNotBlockUnrelatedRequests(t *testing.T) { + t.Setenv(daemonRemoteTokenFileEnv, "") + ws, err := filepath.EvalSymlinks(t.TempDir()) + if err != nil { + t.Fatalf("EvalSymlinks: %v", err) + } + engine := NewEngine(EngineOptions{ + WorkspaceRoot: ws, + Policy: Policy{Mode: ModeEnforce, EnforceWorkspace: true}, + }) + if rx := engine.ReadExclusions(); rx.Active() { + t.Fatal("read exclusions must stay inactive without a configured token file") + } + decision := engine.Evaluate(context.Background(), Request{ + ToolName: "read_file", + WorkspaceRoot: ws, + SideEffect: SideEffectRead, + Args: map[string]any{"path": filepath.Join(ws, "main.go")}, + }) + if decision.Action == ActionDeny { + t.Fatalf("ordinary workspace read was denied: %q", decision.Reason) + } +} diff --git a/internal/sandbox/runner.go b/internal/sandbox/runner.go index fa99b3aba..d7a4702ea 100644 --- a/internal/sandbox/runner.go +++ b/internal/sandbox/runner.go @@ -839,7 +839,13 @@ func denySeatbeltPathRules(action string, paths []string) []string { for _, filter := range filters { out = append(out, "(deny "+action+" "+filter+")") if action == "file-read*" { - out = append(out, "(deny file-write-unlink "+filter+")") + // Denying reads does not imply denying writes, and the broad + // (allow file-write* ...) emitted above covers every workspace root + // plus the default temp roots. A credential file under one of those + // stayed truncatable and replaceable — enough to deny service, or to + // swap the secret a later process reads. file-write* subsumes the + // unlink denial this previously emitted on its own. + out = append(out, "(deny file-write* "+filter+")") } } } diff --git a/internal/sandbox/runner_test.go b/internal/sandbox/runner_test.go index a29302989..f28c85279 100644 --- a/internal/sandbox/runner_test.go +++ b/internal/sandbox/runner_test.go @@ -484,7 +484,11 @@ func TestSeatbeltProfileProtectsMetadataAndDenyOrdering(t *testing.T) { normalizedSecretRead := sandboxProfileString(normalizeProfilePath("/repo/secret-read")) normalizedSecretWrite := sandboxProfileString(normalizeProfilePath("/repo/secret-write")) denySecretReadRule := `(deny file-read* (subpath "` + normalizedSecretRead + `"))` - denySecretReadUnlinkRule := `(deny file-write-unlink (subpath "` + normalizedSecretRead + `"))` + // A read-denied path must also be write-denied: the broad write allow above + // covers every workspace and temp root, so a credential file under one of them + // would otherwise stay truncatable and replaceable. file-write* subsumes the + // file-write-unlink denial this used to assert on its own. + denySecretReadWriteRule := `(deny file-write* (subpath "` + normalizedSecretRead + `"))` denySecretWriteRule := `(deny file-write* (subpath "` + normalizedSecretWrite + `"))` for _, want := range []string{ `(deny file-write* (literal "/repo/vendor"))`, @@ -492,7 +496,7 @@ func TestSeatbeltProfileProtectsMetadataAndDenyOrdering(t *testing.T) { `(deny file-write* (regex #"^/repo/\.git(/.*)?$"))`, `(deny file-write* (regex #"^/repo/\.zero(/.*)?$"))`, denySecretReadRule, - denySecretReadUnlinkRule, + denySecretReadWriteRule, denySecretWriteRule, } { if !strings.Contains(sbpl, want) { diff --git a/internal/tools/daemon_token_exclusion_test.go b/internal/tools/daemon_token_exclusion_test.go new file mode 100644 index 000000000..0ae96cd8d --- /dev/null +++ b/internal/tools/daemon_token_exclusion_test.go @@ -0,0 +1,129 @@ +package tools + +import ( + "context" + "os" + "path/filepath" + "strings" + "testing" + + "github.com/Gitlawb/zero/internal/daemon/remote" + "github.com/Gitlawb/zero/internal/sandbox" +) + +// daemonTokenFixture builds a workspace holding the remote bridge's token file +// alongside an ordinary file, with ZERO_DAEMON_REMOTE_TOKEN_FILE pointing at it — +// the shape a remote daemon session takes when its token lives in the session +// workspace. AllowRead deliberately covers the whole workspace so the tests prove +// the exclusion is not re-includable. +func daemonTokenFixture(t *testing.T) (string, string, *sandbox.Engine) { + t.Helper() + ws, err := filepath.EvalSymlinks(t.TempDir()) + if err != nil { + t.Fatalf("EvalSymlinks: %v", err) + } + if err := os.WriteFile(filepath.Join(ws, "main.go"), []byte("package main // bridge-secret\n"), 0o600); err != nil { + t.Fatalf("write main.go: %v", err) + } + token := filepath.Join(ws, "bridge-token") + if err := os.WriteFile(token, []byte("bridge-secret\n"), 0o600); err != nil { + t.Fatalf("write token: %v", err) + } + t.Setenv(remote.EnvTokenFile, token) + + scope, err := sandbox.NewScope(ws, nil) + if err != nil { + t.Fatalf("NewScope: %v", err) + } + engine := sandbox.NewEngine(sandbox.EngineOptions{ + WorkspaceRoot: ws, + Policy: sandbox.Policy{ + Mode: sandbox.ModeEnforce, + EnforceWorkspace: true, + AllowRead: []string{ws}, + }, + Scope: scope, + }) + return ws, token, engine +} + +func TestGrepSkipsDaemonTokenFile(t *testing.T) { + ws, _, engine := daemonTokenFixture(t) + tool, ok := NewScopedGrepTool(ws, nil).(sandboxAwareTool) + if !ok { + t.Fatal("grep tool must be sandbox-aware") + } + args := map[string]any{"pattern": "bridge-secret", "output_mode": "files_with_matches"} + + sandboxed := tool.RunWithSandbox(context.Background(), args, engine) + if sandboxed.Status != StatusOK { + t.Fatalf("grep failed: %s", sandboxed.Output) + } + if !strings.Contains(sandboxed.Output, "main.go") { + t.Fatalf("grep must still match ordinary workspace files, got:\n%s", sandboxed.Output) + } + if strings.Contains(sandboxed.Output, "bridge-token") { + t.Fatalf("grep must NOT surface the remote bridge token file, got:\n%s", sandboxed.Output) + } +} + +func TestGlobSkipsDaemonTokenFile(t *testing.T) { + ws, _, engine := daemonTokenFixture(t) + tool, ok := NewScopedGlobTool(ws, nil).(sandboxAwareTool) + if !ok { + t.Fatal("glob tool must be sandbox-aware") + } + + sandboxed := tool.RunWithSandbox(context.Background(), map[string]any{"pattern": "**/*"}, engine) + if sandboxed.Status != StatusOK { + t.Fatalf("glob failed: %s", sandboxed.Output) + } + if !strings.Contains(sandboxed.Output, "main.go") { + t.Fatalf("glob must still match ordinary workspace files, got:\n%s", sandboxed.Output) + } + if strings.Contains(sandboxed.Output, "bridge-token") { + t.Fatalf("glob must NOT surface the remote bridge token file, got:\n%s", sandboxed.Output) + } +} + +// TestEngineDeniesDaemonTokenFileTools covers the request gate the direct file +// tools go through (read_file, write_file, edit_file, apply_patch): the bridge +// token must be neither readable nor writable, even though AllowRead covers the +// whole workspace and the file is inside it. +func TestEngineDeniesDaemonTokenFileTools(t *testing.T) { + ws, token, engine := daemonTokenFixture(t) + for _, tc := range []struct { + name string + toolName string + sideEffect sandbox.SideEffect + }{ + {name: "read_file", toolName: "read_file", sideEffect: sandbox.SideEffectRead}, + {name: "write_file", toolName: "write_file", sideEffect: sandbox.SideEffectWrite}, + {name: "edit_file", toolName: "edit_file", sideEffect: sandbox.SideEffectWrite}, + } { + t.Run(tc.name, func(t *testing.T) { + decision := engine.Evaluate(context.Background(), sandbox.Request{ + ToolName: tc.toolName, + WorkspaceRoot: ws, + SideEffect: tc.sideEffect, + Args: map[string]any{"path": token}, + // A granted permission must not override the exclusion either. + Permission: sandbox.PermissionAllow, + }) + if decision.Action != sandbox.ActionDeny || !strings.Contains(decision.Reason, "remote bridge token") { + t.Fatalf("%s on the bridge token: action = %q reason = %q, want a bridge-token deny", tc.toolName, decision.Action, decision.Reason) + } + }) + } + + // An ordinary workspace file in the same directory stays usable. + decision := engine.Evaluate(context.Background(), sandbox.Request{ + ToolName: "read_file", + WorkspaceRoot: ws, + SideEffect: sandbox.SideEffectRead, + Args: map[string]any{"path": filepath.Join(ws, "main.go")}, + }) + if decision.Action == sandbox.ActionDeny { + t.Fatalf("ordinary workspace read was denied: %q", decision.Reason) + } +} From e109db32c2ac87835d99c8521fdcf1028b282c3b Mon Sep 17 00:00:00 2001 From: PierrunoYT Date: Sat, 25 Jul 2026 13:13:25 +0200 Subject: [PATCH 03/10] fix(sandbox): scope the credential write-deny and keep it under a disabled policy Addresses the CodeRabbit review on the previous commit. - The Seatbelt write-deny no longer applies to every user-configured DenyRead entry, only to the automatic credential entries (cloud stores plus the bridge token). A read-denied cache or generated directory that the build legitimately writes stays writable on macOS; user entries keep the read + unlink denial they already had. - ModeDisabled no longer drops the automatic credential exclusion. Disabling the sandbox drops user-configured restrictions, but the bridge token authenticates the caller driving these tools, so Evaluate, ReadExclusions, and the glob export keep protecting it. Everything else stays allowed. - The direct-tool table now includes the apply_patch case its comment promised. --- internal/sandbox/engine.go | 26 +++++++-- internal/sandbox/pathlists.go | 30 ++++++++++ .../sandbox/protected_credentials_test.go | 56 +++++++++++++++++-- internal/sandbox/runner.go | 37 +++++++++--- internal/sandbox/runner_test.go | 15 +++-- internal/tools/daemon_token_exclusion_test.go | 1 + 6 files changed, 141 insertions(+), 24 deletions(-) diff --git a/internal/sandbox/engine.go b/internal/sandbox/engine.go index 92ef514fa..883c27ce0 100644 --- a/internal/sandbox/engine.go +++ b/internal/sandbox/engine.go @@ -144,14 +144,22 @@ func (engine *Engine) LookupCommandPrefixForSession(toolName string, command []s // don't re-run Abs/EvalSymlinks per visited path. Returns nil for a nil engine // (the matcher's methods treat nil as "exclude nothing"). func (engine *Engine) ReadExclusions() *ReadExclusions { - // A disabled policy enforces nothing, so it must not filter search results - // either (Evaluate already allows every request under ModeDisabled). + // A disabled policy enforces nothing the USER configured, so it must not + // filter search results by DenyRead either (Evaluate likewise allows every + // request under ModeDisabled). The automatic credential exclusion is not + // policy-derived and survives: the bridge token authenticates the channel + // driving these tools, so turning the sandbox off must not hand a remote + // caller its own credential. if engine == nil { return nil } policy := engine.effectivePolicy(engine.policy) if policy.Mode == ModeDisabled { - return nil + protected := protectedCredentialPaths() + if len(protected) == 0 { + return nil + } + return &ReadExclusions{workspaceRoot: engine.workspaceRoot, protectedRoots: protected} } return &ReadExclusions{ workspaceRoot: engine.workspaceRoot, @@ -165,13 +173,14 @@ func (engine *Engine) ReadExclusions() *ReadExclusions { // engine's policy + scope (see the package-level ReadExclusionGlobs). Empty when // DenyRead is unset or the engine has no scope. func (engine *Engine) ReadExclusionGlobs() []string { - // A disabled policy filters nothing (parity with ReadExclusions / Evaluate). + // A disabled policy filters nothing the user configured, but keeps the + // automatic credential exclusion (parity with ReadExclusions / Evaluate). if engine == nil { return nil } policy := engine.effectivePolicy(engine.policy) if policy.Mode == ModeDisabled { - return nil + return ReadExclusionGlobs(Policy{}, engine.scope) } return ReadExclusionGlobs(policy, engine.scope) } @@ -319,6 +328,13 @@ func (engine *Engine) Evaluate(ctx context.Context, request Request) Decision { risk := classifyWithScope(request, scope) if policy.Mode == ModeDisabled { + // Disabling the sandbox drops every user-configured restriction, but not the + // automatic credential exclusion: the remote bridge token authenticates the + // caller driving these tools, so it stays unreadable and unwritable through + // them (a shell command is a separate, OS-level boundary). + if block := protectedCredentialPathBlock(request, request.WorkspaceRoot); block != nil { + return deny(request, risk, block.Code, block.Path, block.Reason, false) + } return Decision{Action: ActionAllow, Risk: risk, Reason: "sandbox disabled"} } if request.Permission == PermissionDeny { diff --git a/internal/sandbox/pathlists.go b/internal/sandbox/pathlists.go index 5700aa793..558219ab2 100644 --- a/internal/sandbox/pathlists.go +++ b/internal/sandbox/pathlists.go @@ -86,6 +86,36 @@ func protectedCredentialPaths() []string { return dedupeStrings(paths) } +// protectedCredentialPathBlock returns the block for the first requested path +// that targets a protected credential file, or nil. It exists for the callers +// that bypass validatePathWithPolicy — currently the ModeDisabled short-circuit, +// where the exclusion still applies because it is not policy-derived. +func protectedCredentialPathBlock(request Request, workspaceRoot string) *pathBlock { + switch request.SideEffect { + case SideEffectRead, SideEffectWrite, SideEffectOutOfWorkspace: + default: + return nil + } + protected := protectedCredentialPaths() + if len(protected) == 0 { + return nil + } + verb := "readable" + if request.SideEffect != SideEffectRead { + verb = "writable" + } + for _, path := range requestPaths(request) { + if protectedPathDenied(protected, workspaceRoot, path) { + return &pathBlock{ + Code: BlockDenied, + Path: path, + Reason: path + " holds the remote bridge token and is never " + verb, + } + } + } + return nil +} + // protectedPathDenied reports whether path targets one of the protected // credential files. There is no allow-list consultation by design. func protectedPathDenied(protected []string, workspaceRoot, path string) bool { diff --git a/internal/sandbox/protected_credentials_test.go b/internal/sandbox/protected_credentials_test.go index 34720d1a2..bc5c82123 100644 --- a/internal/sandbox/protected_credentials_test.go +++ b/internal/sandbox/protected_credentials_test.go @@ -155,20 +155,22 @@ func TestProtectedCredentialsRejectSessionPermissionProfile(t *testing.T) { // TestProtectedCredentialsDenyReadAndWriteInSeatbeltProfile covers the macOS // backend: a token under a writable root was read-denied but still truncatable -// through the broad write allow. +// through the broad write allow. A user-configured DenyRead entry keeps the write +// direction (see TestSeatbeltProfileProtectsMetadataAndDenyOrdering). func TestProtectedCredentialsDenyReadAndWriteInSeatbeltProfile(t *testing.T) { - token := "/private/tmp/zero-bridge-token" + ws, token := protectedTokenFixture(t) + userDenied := filepath.Join(ws, "generated") profile := PermissionProfile{ FileSystem: FileSystemPolicy{ Kind: FileSystemRestricted, ReadRoots: []string{string(filepath.Separator)}, - WriteRoots: []WritableRoot{{Root: "/repo"}}, - DenyRead: []string{token}, + WriteRoots: []WritableRoot{{Root: ws}}, + DenyRead: []string{token, userDenied}, AllowTemp: true, }, Network: NetworkPolicy{Mode: NetworkDeny}, } - sbpl := seatbeltProfileFromPermissionProfile(profile, Policy{Mode: ModeEnforce}, "") + sbpl := seatbeltProfileFromPermissionProfile(profile, Policy{Mode: ModeEnforce, DenyRead: []string{userDenied}}, "") escaped := sandboxProfileString(normalizeProfilePath(token)) denyRead := `(deny file-read* (literal "` + escaped + `"))` denyWrite := `(deny file-write* (literal "` + escaped + `"))` @@ -177,12 +179,54 @@ func TestProtectedCredentialsDenyReadAndWriteInSeatbeltProfile(t *testing.T) { t.Fatalf("Seatbelt profile missing %q:\n%s", want, sbpl) } } - // Seatbelt is last-match-wins, so both denials must follow the broad allow. + if strings.Contains(sbpl, `(deny file-write* (literal "`+sandboxProfileString(normalizeProfilePath(userDenied))+`"))`) { + t.Fatalf("a user-configured DenyRead path must stay writable:\n%s", sbpl) + } + // Seatbelt is last-match-wins, so the denial must follow the broad allow. if allow := strings.Index(sbpl, "(allow file-write*"); allow < 0 || strings.Index(sbpl, denyWrite) < allow { t.Fatalf("the write denial must follow the broad write allow:\n%s", sbpl) } } +// TestProtectedCredentialsSurviveDisabledPolicy covers the one route that skips +// validatePathWithPolicy entirely: ModeDisabled drops every user-configured +// restriction, but the bridge token authenticates the caller driving these tools. +func TestProtectedCredentialsSurviveDisabledPolicy(t *testing.T) { + ws, token := protectedTokenFixture(t) + engine := NewEngine(EngineOptions{WorkspaceRoot: ws, Policy: Policy{Mode: ModeDisabled}}) + + for _, sideEffect := range []SideEffect{SideEffectRead, SideEffectWrite} { + decision := engine.Evaluate(context.Background(), Request{ + ToolName: "read_file", + WorkspaceRoot: ws, + SideEffect: sideEffect, + Args: map[string]any{"path": token}, + }) + if decision.Action != ActionDeny || !strings.Contains(decision.Reason, "remote bridge token") { + t.Fatalf("%s under a disabled policy: action = %q reason = %q, want a bridge-token deny", sideEffect, decision.Action, decision.Reason) + } + } + + // Everything else stays allowed: a disabled sandbox is still disabled. + decision := engine.Evaluate(context.Background(), Request{ + ToolName: "read_file", + WorkspaceRoot: ws, + SideEffect: SideEffectRead, + Args: map[string]any{"path": filepath.Join(ws, "main.go")}, + }) + if decision.Action != ActionAllow { + t.Fatalf("ordinary read under a disabled policy: action = %q reason = %q, want allow", decision.Action, decision.Reason) + } + + rx := engine.ReadExclusions() + if !rx.Active() || !rx.PathExcluded(token) { + t.Fatalf("read exclusions under a disabled policy must still exclude %q", token) + } + if rx.PathExcluded(filepath.Join(ws, "main.go")) { + t.Fatal("read exclusions under a disabled policy must not exclude ordinary files") + } +} + // TestProtectedCredentialsDoNotBlockUnrelatedRequests keeps the exclusion inert // for everyone who does not run the remote bridge. func TestProtectedCredentialsDoNotBlockUnrelatedRequests(t *testing.T) { diff --git a/internal/sandbox/runner.go b/internal/sandbox/runner.go index d7a4702ea..339fd3a18 100644 --- a/internal/sandbox/runner.go +++ b/internal/sandbox/runner.go @@ -11,6 +11,7 @@ import ( "os/exec" "path/filepath" "runtime" + "slices" "strings" "sync/atomic" @@ -661,6 +662,7 @@ func seatbeltProfileFromPermissionProfile(profile PermissionProfile, policy Poli writeRule, } rules = append(rules, denyReadRules(profile.FileSystem)...) + rules = append(rules, credentialDenyWriteRules(profile.FileSystem, policy)...) rules = append(rules, writeRootCarveoutDenyRules(profile.FileSystem)...) rules = append(rules, denyWriteRulesFromPaths(profile.FileSystem.DenyWrite)...) rules = append(rules, networkRule) @@ -823,6 +825,33 @@ func denyWriteRulesFromPaths(paths []string) []string { return denySeatbeltPathRules("file-write*", paths) } +// credentialDenyWriteRules write-denies the AUTOMATIC credential entries of +// DenyRead — the cloud credential stores and the remote bridge token file. +// Denying reads does not imply denying writes, and the broad +// (allow file-write* ...) above covers every workspace root plus the default +// temp roots, so a credential file under one of those stayed truncatable and +// replaceable: enough to deny service, or to swap the secret the next process +// reads. denySeatbeltPathRules keeps emitting read + unlink denials for the +// whole DenyRead list, so a user-configured read-denied path that their build +// legitimately writes (a cache or generated directory) stays writable — only +// Zero's own credential entries lose the write direction. +func credentialDenyWriteRules(fs FileSystemPolicy, policy Policy) []string { + automatic := normalizeProfilePaths(append(credentialDenyReadPaths(policy), protectedCredentialPaths()...)) + if len(automatic) == 0 { + return nil + } + denied := normalizeProfilePaths(fs.DenyRead) + paths := make([]string, 0, len(automatic)) + for _, path := range automatic { + // Only paths the profile actually read-denies: credentialDenyReadPaths + // already drops AllowRead opt-outs, and this keeps the two lists in step. + if slices.Contains(denied, path) { + paths = append(paths, path) + } + } + return denyWriteRulesFromPaths(dedupeStrings(paths)) +} + func denySeatbeltPathRules(action string, paths []string) []string { resolved := normalizeProfilePaths(paths) if len(resolved) == 0 { @@ -839,13 +868,7 @@ func denySeatbeltPathRules(action string, paths []string) []string { for _, filter := range filters { out = append(out, "(deny "+action+" "+filter+")") if action == "file-read*" { - // Denying reads does not imply denying writes, and the broad - // (allow file-write* ...) emitted above covers every workspace root - // plus the default temp roots. A credential file under one of those - // stayed truncatable and replaceable — enough to deny service, or to - // swap the secret a later process reads. file-write* subsumes the - // unlink denial this previously emitted on its own. - out = append(out, "(deny file-write* "+filter+")") + out = append(out, "(deny file-write-unlink "+filter+")") } } } diff --git a/internal/sandbox/runner_test.go b/internal/sandbox/runner_test.go index f28c85279..eb55e56e8 100644 --- a/internal/sandbox/runner_test.go +++ b/internal/sandbox/runner_test.go @@ -484,11 +484,7 @@ func TestSeatbeltProfileProtectsMetadataAndDenyOrdering(t *testing.T) { normalizedSecretRead := sandboxProfileString(normalizeProfilePath("/repo/secret-read")) normalizedSecretWrite := sandboxProfileString(normalizeProfilePath("/repo/secret-write")) denySecretReadRule := `(deny file-read* (subpath "` + normalizedSecretRead + `"))` - // A read-denied path must also be write-denied: the broad write allow above - // covers every workspace and temp root, so a credential file under one of them - // would otherwise stay truncatable and replaceable. file-write* subsumes the - // file-write-unlink denial this used to assert on its own. - denySecretReadWriteRule := `(deny file-write* (subpath "` + normalizedSecretRead + `"))` + denySecretReadUnlinkRule := `(deny file-write-unlink (subpath "` + normalizedSecretRead + `"))` denySecretWriteRule := `(deny file-write* (subpath "` + normalizedSecretWrite + `"))` for _, want := range []string{ `(deny file-write* (literal "/repo/vendor"))`, @@ -496,13 +492,20 @@ func TestSeatbeltProfileProtectsMetadataAndDenyOrdering(t *testing.T) { `(deny file-write* (regex #"^/repo/\.git(/.*)?$"))`, `(deny file-write* (regex #"^/repo/\.zero(/.*)?$"))`, denySecretReadRule, - denySecretReadWriteRule, + denySecretReadUnlinkRule, denySecretWriteRule, } { if !strings.Contains(sbpl, want) { t.Fatalf("Seatbelt profile missing %q:\n%s", want, sbpl) } } + // A user-configured read-denied path keeps the write direction: a cache or + // generated directory the build legitimately writes must not become read-only + // just because it is excluded from reads. Only Zero's own automatic credential + // entries are write-denied (see TestProtectedCredentialsDenyReadAndWrite...). + if strings.Contains(sbpl, `(deny file-write* (subpath "`+normalizedSecretRead+`"))`) { + t.Fatalf("a user-configured DenyRead path must stay writable:\n%s", sbpl) + } allowIdx := strings.Index(sbpl, "(allow file-write*") denyReadIdx := strings.Index(sbpl, denySecretReadRule) metadataIdx := strings.Index(sbpl, `(deny file-write* (regex #"^/repo/\.git(/.*)?$"))`) diff --git a/internal/tools/daemon_token_exclusion_test.go b/internal/tools/daemon_token_exclusion_test.go index 0ae96cd8d..e98a1676f 100644 --- a/internal/tools/daemon_token_exclusion_test.go +++ b/internal/tools/daemon_token_exclusion_test.go @@ -100,6 +100,7 @@ func TestEngineDeniesDaemonTokenFileTools(t *testing.T) { {name: "read_file", toolName: "read_file", sideEffect: sandbox.SideEffectRead}, {name: "write_file", toolName: "write_file", sideEffect: sandbox.SideEffectWrite}, {name: "edit_file", toolName: "edit_file", sideEffect: sandbox.SideEffectWrite}, + {name: "apply_patch", toolName: "apply_patch", sideEffect: sandbox.SideEffectWrite}, } { t.Run(tc.name, func(t *testing.T) { decision := engine.Evaluate(context.Background(), sandbox.Request{ From b7823318700fb7f814c57a7e328e9ed816bf7f83 Mon Sep 17 00:00:00 2001 From: PierrunoYT Date: Sun, 26 Jul 2026 17:18:07 +0200 Subject: [PATCH 04/10] fix(sandbox): match the bridge token across case and allow-read The protected-credential comparison ends in pathWithinRoot -> filepath.Rel, which folds case on Windows but not on darwin, whose default APFS volume is case-insensitive. A request for .../BRIDGE-TOKEN therefore missed a protected .../bridge-token while the OS opened the same bearer-token file, letting a remote session read or replace the token through the direct file tools. Fold case in the protected-path check on both case-insensitive platforms. The OS-sandbox profile also dropped the token candidate when a user AllowRead entry covered it, so a wrapped shell command could read a token the in-process boundary treats as non-overrideable. Keep the AllowRead opt-out for the ordinary credential stores and make the bridge token the documented exception, so both layers give the same guarantee. --- internal/sandbox/manager_test.go | 8 ++- internal/sandbox/pathlists.go | 59 +++++++++++++++++-- internal/sandbox/profile.go | 24 ++++++-- .../sandbox/protected_credentials_test.go | 37 ++++++++++++ 4 files changed, 117 insertions(+), 11 deletions(-) diff --git a/internal/sandbox/manager_test.go b/internal/sandbox/manager_test.go index 90ed371b8..b04fa13fc 100644 --- a/internal/sandbox/manager_test.go +++ b/internal/sandbox/manager_test.go @@ -410,8 +410,12 @@ func TestCredentialDenyReadPathsIn(t *testing.T) { if !stringSliceContains(optedOut, normalizeProfilePaths([]string{keyFile})[0]) { t.Errorf("credential deny paths = %#v, want unrelated entries kept after opt-out", optedOut) } - if stringSliceContains(optedOut, normalizeProfilePaths([]string{daemonTokenFile})[0]) { - t.Errorf("credential deny paths = %#v, want AllowRead opt-out to drop daemon token file", optedOut) + // The bridge bearer token is the exception: the in-process tool boundary + // (protectedCredentialPaths) refuses to re-include it through AllowRead, so + // the OS-sandbox profile must not either — otherwise the guarantee would + // depend on whether a wrapped shell command or a built-in tool reads it. + if !stringSliceContains(optedOut, normalizeProfilePaths([]string{daemonTokenFile})[0]) { + t.Errorf("credential deny paths = %#v, want the daemon token file denied despite AllowRead", optedOut) } if got := credentialDenyReadPathsIn(" ", "", "", nil); len(got) != 0 { diff --git a/internal/sandbox/pathlists.go b/internal/sandbox/pathlists.go index 558219ab2..2dbef2af6 100644 --- a/internal/sandbox/pathlists.go +++ b/internal/sandbox/pathlists.go @@ -3,6 +3,7 @@ package sandbox import ( "os" "path/filepath" + "runtime" "strings" ) @@ -120,13 +121,50 @@ func protectedCredentialPathBlock(request Request, workspaceRoot string) *pathBl // credential files. There is no allow-list consultation by design. func protectedPathDenied(protected []string, workspaceRoot, path string) bool { for _, entry := range protected { - if pathUnderPolicyRoot(path, entry, workspaceRoot) { + if pathUnderProtectedRoot(path, entry, workspaceRoot) { return true } } return false } +// protectedPathFoldsCase reports whether a case-variant spelling of a path opens +// the SAME file on this platform, so the protected-credential comparison must +// fold case to stay closed. +// +// pathWithinRoot ends in filepath.Rel, which ALREADY folds case on Windows +// (path_windows.go's sameWord uses strings.EqualFold) but never on Unix. macOS +// volumes are case-insensitive by default (APFS), so without folding a request +// for `.../Bridge-Token` misses a protected `.../bridge-token` while the OS +// opens the very same bearer-token file. Windows is listed too so the guarantee +// does not silently depend on a filepath.Rel implementation detail. +// +// Case-sensitive outliers (a case-sensitive APFS volume) only make this +// over-deny a genuinely different file that happens to differ by case alone, +// which is the safe direction for a credential the sandbox must never expose. +func protectedPathFoldsCase() bool { + return runtime.GOOS == "windows" || runtime.GOOS == "darwin" +} + +// pathUnderProtectedRoot is pathUnderPolicyRoot for the automatic credential +// exclusions: identical anchoring and symlink normalization, plus the platform's +// filesystem case semantics. Only the final containment comparison folds — the +// normalization above it keeps operating on the path as spelled, so symlink +// resolution is unaffected. +func pathUnderProtectedRoot(requestedPath, root, workspaceRoot string) bool { + normalized, ok := normalizePathForPolicyRoot(requestedPath, root, workspaceRoot) + if !ok { + return false + } + if pathWithinRoot(root, normalized) { + return true + } + if !protectedPathFoldsCase() { + return false + } + return pathWithinRoot(strings.ToLower(root), strings.ToLower(normalized)) +} + // resolvePolicyPaths resolves and de-duplicates a list of policy path entries, // dropping blanks and non-existent entries. Files and directories are both kept // (a DenyRead/DenyWrite entry may target a single sensitive file). @@ -178,18 +216,29 @@ func resolveWriteRootPaths(entries []string) []string { // symlink prefix cannot evade the match. root must be an already-resolved // absolute path. func pathUnderPolicyRoot(requestedPath, root, workspaceRoot string) bool { - if root == "" { + normalized, ok := normalizePathForPolicyRoot(requestedPath, root, workspaceRoot) + if !ok { return false } + return pathWithinRoot(root, normalized) +} + +// normalizePathForPolicyRoot anchors requestedPath (a relative one against +// workspaceRoot) and symlink-normalizes the portion outside root, yielding the +// path pathUnderPolicyRoot compares. ok is false when there is nothing to +// compare against: a blank root, or a relative path with no workspace root. +func normalizePathForPolicyRoot(requestedPath, root, workspaceRoot string) (string, bool) { + if root == "" { + return "", false + } abs := requestedPath if !filepath.IsAbs(abs) { if workspaceRoot == "" { - return false + return "", false } abs = filepath.Join(workspaceRoot, abs) } - normalized := NormalizePrefixForRoot(abs, root) - return pathWithinRoot(root, normalized) + return NormalizePrefixForRoot(abs, root), true } // readDenied reports whether path is excluded by the DenyRead list with no diff --git a/internal/sandbox/profile.go b/internal/sandbox/profile.go index 93bfe7ac5..ed214c0d1 100644 --- a/internal/sandbox/profile.go +++ b/internal/sandbox/profile.go @@ -155,7 +155,11 @@ func permissionProfileReadRoots(workspaceRoot string, policy Policy, scope *Scop // WRITE_RESTRICTED token, which the unelevated tier depends on. Revisit // once the Windows deny-read model is settled. // - A candidate nested under a user-configured AllowRead entry is dropped, -// so `allowRead: ["~/.aws"]` remains an explicit opt-out. +// so `allowRead: ["~/.aws"]` remains an explicit opt-out. The bridge bearer +// token is the one exception: the in-process tool boundary +// (protectedCredentialPaths) treats it as non-overrideable, and the +// guarantee must not depend on whether a wrapped shell command or a built-in +// tool does the reading. // // These are profile-level rules only; they are intentionally NOT merged into // Policy.DenyRead, whose emptiness gates escalated (unsandboxed) execution and @@ -189,11 +193,15 @@ func credentialDenyReadPathsIn(home string, googleCredentials string, daemonToke if target := strings.TrimSpace(googleCredentials); target != "" { candidates = append(candidates, target) } + // The bridge bearer token grants control of this daemon, so unlike the + // opt-outable credential stores above it stays denied even when AllowRead + // covers it — matching protectedCredentialPaths at the in-process boundary. + var mandatory []string if target := strings.TrimSpace(daemonTokenFile); target != "" { - candidates = append(candidates, target) + mandatory = normalizeProfilePaths([]string{target}) } allowRoots := normalizeProfilePaths(allowRead) - out := make([]string, 0, len(candidates)) + out := make([]string, 0, len(candidates)+len(mandatory)) for _, path := range normalizeProfilePaths(candidates) { // Only stores that actually exist on this host need a deny rule. if _, err := os.Stat(path); err != nil { @@ -210,7 +218,15 @@ func credentialDenyReadPathsIn(home string, googleCredentials string, daemonToke out = append(out, path) } } - return out + for _, path := range mandatory { + // Existence filtering still applies: a deny rule for a path that is not + // there protects nothing, and Bubblewrap cannot bind a missing target. + if _, err := os.Stat(path); err != nil { + continue + } + out = append(out, path) + } + return dedupeStrings(out) } // userGitConfigReadPaths returns the user's global git config FILES so a diff --git a/internal/sandbox/protected_credentials_test.go b/internal/sandbox/protected_credentials_test.go index bc5c82123..f022a3bee 100644 --- a/internal/sandbox/protected_credentials_test.go +++ b/internal/sandbox/protected_credentials_test.go @@ -227,6 +227,43 @@ func TestProtectedCredentialsSurviveDisabledPolicy(t *testing.T) { } } +// TestProtectedCredentialsMatchCaseVariantOnCaseInsensitiveFilesystems covers the +// bypass a case-variant spelling opened: pathWithinRoot ends in filepath.Rel, +// which folds case on Windows but NOT on darwin, whose default APFS volume is +// case-insensitive — so `.../BRIDGE-TOKEN` missed the protected `.../bridge-token` +// while the OS opened the same bearer-token file. On a case-sensitive filesystem +// the variant is a genuinely different file and must stay unblocked. +func TestProtectedCredentialsMatchCaseVariantOnCaseInsensitiveFilesystems(t *testing.T) { + ws, token := protectedTokenFixture(t) + variant := filepath.Join(filepath.Dir(token), strings.ToUpper(filepath.Base(token))) + if variant == token { + t.Fatalf("fixture token %q has no case variant", token) + } + scope, err := NewScope(ws, nil) + if err != nil { + t.Fatalf("NewScope: %v", err) + } + policy := Policy{Mode: ModeEnforce, EnforceWorkspace: true, AllowRead: []string{ws}, AllowWrite: []string{ws}} + wantDenied := protectedPathFoldsCase() + + for _, sideEffect := range []SideEffect{SideEffectRead, SideEffectWrite, SideEffectOutOfWorkspace} { + block := validatePathWithPolicy(scope, policy, sideEffect, true, ws, variant) + denied := block != nil && strings.Contains(block.Reason, "remote bridge token") + if denied != wantDenied { + t.Fatalf("%s on case variant %q: denied = %t, want %t (block = %#v)", sideEffect, variant, denied, wantDenied, block) + } + } + + engine := NewEngine(EngineOptions{WorkspaceRoot: ws, Policy: policy, Scope: scope}) + if excluded := engine.ReadExclusions().PathExcluded(variant); excluded != wantDenied { + t.Fatalf("read exclusions on case variant %q: excluded = %t, want %t", variant, excluded, wantDenied) + } + // The exact spelling is denied on every platform regardless. + if block := validatePathWithPolicy(scope, policy, SideEffectRead, true, ws, token); block == nil { + t.Fatalf("the configured token path %q must always be denied", token) + } +} + // TestProtectedCredentialsDoNotBlockUnrelatedRequests keeps the exclusion inert // for everyone who does not run the remote bridge. func TestProtectedCredentialsDoNotBlockUnrelatedRequests(t *testing.T) { From f3a5a4a6316939502faeca0c3b89689a478713df Mon Sep 17 00:00:00 2001 From: Amp Date: Sun, 26 Jul 2026 20:42:38 +0000 Subject: [PATCH 05/10] fix(sandbox): honor inline daemon token precedence Amp-Thread-ID: https://ampcode.com/threads/T-019fa018-e3c8-705b-9627-3a7564f8d66a Co-authored-by: Pierre Bruno --- internal/sandbox/manager_test.go | 11 ++++++- internal/sandbox/pathlists.go | 29 ++++++++++++++----- internal/sandbox/profile.go | 2 +- .../sandbox/protected_credentials_test.go | 14 +++++++++ internal/tools/daemon_token_exclusion_test.go | 1 + 5 files changed, 47 insertions(+), 10 deletions(-) diff --git a/internal/sandbox/manager_test.go b/internal/sandbox/manager_test.go index b04fa13fc..a9e0160a3 100644 --- a/internal/sandbox/manager_test.go +++ b/internal/sandbox/manager_test.go @@ -441,11 +441,20 @@ func TestPermissionProfileDeniesDaemonTokenFile(t *testing.T) { if err := os.WriteFile(tokenFile, []byte("secret"), 0o600); err != nil { t.Fatal(err) } - t.Setenv("ZERO_DAEMON_REMOTE_TOKEN_FILE", tokenFile) + t.Setenv(daemonRemoteTokenEnv, "") + t.Setenv(daemonRemoteTokenFileEnv, tokenFile) profile := PermissionProfileFromPolicy(t.TempDir(), DefaultPolicy(), nil) want := normalizeProfilePaths([]string{tokenFile})[0] if !stringSliceContains(profile.FileSystem.DenyRead, want) { t.Fatalf("DenyRead = %#v, want daemon token file %q", profile.FileSystem.DenyRead, want) } + + // TokenFromEnv selects the inline token when both variables are set, so the + // unused file pointer must not become an automatic OS-sandbox deny. + t.Setenv(daemonRemoteTokenEnv, "from-env") + profile = PermissionProfileFromPolicy(t.TempDir(), DefaultPolicy(), nil) + if stringSliceContains(profile.FileSystem.DenyRead, want) { + t.Fatalf("DenyRead = %#v, must not protect unused token file %q when the inline token takes precedence", profile.FileSystem.DenyRead, want) + } } diff --git a/internal/sandbox/pathlists.go b/internal/sandbox/pathlists.go index 2dbef2af6..79a82a617 100644 --- a/internal/sandbox/pathlists.go +++ b/internal/sandbox/pathlists.go @@ -42,10 +42,23 @@ func resolvePolicyPath(entry string) (string, bool) { return resolved, true } -// daemonRemoteTokenFileEnv names the file holding the remote bridge's bearer -// token. It is duplicated from internal/daemon/remote (which cannot be imported -// here) exactly like the copy scrubSensitiveEnv keeps. -const daemonRemoteTokenFileEnv = "ZERO_DAEMON_REMOTE_TOKEN_FILE" +// These name the alternative sources of the remote bridge's bearer token. They +// are duplicated from internal/daemon/remote (which cannot be imported here) +// exactly like the copies scrubSensitiveEnv keeps. +const ( + daemonRemoteTokenEnv = "ZERO_DAEMON_REMOTE_TOKEN" + daemonRemoteTokenFileEnv = "ZERO_DAEMON_REMOTE_TOKEN_FILE" +) + +// selectedDaemonRemoteTokenFile returns the token-file pointer only when the +// daemon would use it. TokenFromEnv gives the inline token precedence, so an +// inherited file pointer is not a credential when both variables are set. +func selectedDaemonRemoteTokenFile() string { + if strings.TrimSpace(os.Getenv(daemonRemoteTokenEnv)) != "" { + return "" + } + return strings.TrimSpace(os.Getenv(daemonRemoteTokenFileEnv)) +} // protectedCredentialPaths returns credential files that Zero's own in-process // file tools must never read or modify, independent of Policy. @@ -64,15 +77,15 @@ const daemonRemoteTokenFileEnv = "ZERO_DAEMON_REMOTE_TOKEN_FILE" // its own session workspace. Unlike the profile list this applies on Windows // too, where filesystem deny-read has no sandbox representation (#662). // -// Both the configured pathname and the target it currently resolves to are +// Both the selected pathname and the target it currently resolves to are // protected: `zero daemon serve-remote` canonicalizes the value it selects (see -// remote.CanonicalizeTokenFileEnv), but a stale symlinked value inherited from -// elsewhere must not leave the link replaceable. +// remote.CanonicalizeTokenFileEnv), but a symlinked selected value inherited +// from elsewhere must not leave the link replaceable. func protectedCredentialPaths() []string { // os.ReadFile — the daemon's own reader — treats the value literally, so a // relative path resolves against the working directory and a leading "~" is // NOT expanded. resolvePolicyPath would expand it and protect the wrong file. - configured := strings.TrimSpace(os.Getenv(daemonRemoteTokenFileEnv)) + configured := selectedDaemonRemoteTokenFile() if configured == "" { return nil } diff --git a/internal/sandbox/profile.go b/internal/sandbox/profile.go index ed214c0d1..4927fd07c 100644 --- a/internal/sandbox/profile.go +++ b/internal/sandbox/profile.go @@ -174,7 +174,7 @@ func credentialDenyReadPaths(policy Policy) []string { return credentialDenyReadPathsIn( home, os.Getenv("GOOGLE_APPLICATION_CREDENTIALS"), - os.Getenv("ZERO_DAEMON_REMOTE_TOKEN_FILE"), + selectedDaemonRemoteTokenFile(), policy.AllowRead, ) } diff --git a/internal/sandbox/protected_credentials_test.go b/internal/sandbox/protected_credentials_test.go index f022a3bee..1270525e7 100644 --- a/internal/sandbox/protected_credentials_test.go +++ b/internal/sandbox/protected_credentials_test.go @@ -21,6 +21,7 @@ func protectedTokenFixture(t *testing.T) (string, string) { if err := os.WriteFile(token, []byte("secret\n"), 0o600); err != nil { t.Fatalf("write token: %v", err) } + t.Setenv(daemonRemoteTokenEnv, "") t.Setenv(daemonRemoteTokenFileEnv, token) return ws, token } @@ -36,16 +37,26 @@ func TestProtectedCredentialPathsResolveLikeTheDaemonReader(t *testing.T) { } t.Run("absent variable protects nothing", func(t *testing.T) { + t.Setenv(daemonRemoteTokenEnv, "") t.Setenv(daemonRemoteTokenFileEnv, "") if got := protectedCredentialPaths(); len(got) != 0 { t.Fatalf("protected paths = %#v, want none", got) } }) + t.Run("inline token leaves the unused file pointer unprotected", func(t *testing.T) { + t.Setenv(daemonRemoteTokenEnv, "from-env") + t.Setenv(daemonRemoteTokenFileEnv, token) + if got := protectedCredentialPaths(); len(got) != 0 { + t.Fatalf("protected paths = %#v, want none when the inline token takes precedence", got) + } + }) + t.Run("relative value resolves against the working directory", func(t *testing.T) { // os.ReadFile — what the daemon uses — resolves a relative value against the // working directory, so the protected path must do the same. t.Chdir(base) + t.Setenv(daemonRemoteTokenEnv, "") t.Setenv(daemonRemoteTokenFileEnv, "token") if got := protectedCredentialPaths(); !stringSliceContains(got, token) { t.Fatalf("protected paths = %#v, want %q", got, token) @@ -56,6 +67,7 @@ func TestProtectedCredentialPathsResolveLikeTheDaemonReader(t *testing.T) { // os.ReadFile treats "~" as an ordinary directory name; expanding it here // would protect a path the daemon never reads. t.Chdir(base) + t.Setenv(daemonRemoteTokenEnv, "") t.Setenv(daemonRemoteTokenFileEnv, filepath.Join("~", "token")) want := filepath.Join(base, "~", "token") got := protectedCredentialPaths() @@ -76,6 +88,7 @@ func TestProtectedCredentialPathsResolveLikeTheDaemonReader(t *testing.T) { if err := os.Symlink(token, link); err != nil { t.Skipf("symlink unsupported: %v", err) } + t.Setenv(daemonRemoteTokenEnv, "") t.Setenv(daemonRemoteTokenFileEnv, link) got := protectedCredentialPaths() for _, want := range []string{link, token} { @@ -267,6 +280,7 @@ func TestProtectedCredentialsMatchCaseVariantOnCaseInsensitiveFilesystems(t *tes // TestProtectedCredentialsDoNotBlockUnrelatedRequests keeps the exclusion inert // for everyone who does not run the remote bridge. func TestProtectedCredentialsDoNotBlockUnrelatedRequests(t *testing.T) { + t.Setenv(daemonRemoteTokenEnv, "") t.Setenv(daemonRemoteTokenFileEnv, "") ws, err := filepath.EvalSymlinks(t.TempDir()) if err != nil { diff --git a/internal/tools/daemon_token_exclusion_test.go b/internal/tools/daemon_token_exclusion_test.go index e98a1676f..19e71dcbe 100644 --- a/internal/tools/daemon_token_exclusion_test.go +++ b/internal/tools/daemon_token_exclusion_test.go @@ -29,6 +29,7 @@ func daemonTokenFixture(t *testing.T) (string, string, *sandbox.Engine) { if err := os.WriteFile(token, []byte("bridge-secret\n"), 0o600); err != nil { t.Fatalf("write token: %v", err) } + t.Setenv(remote.EnvToken, "") t.Setenv(remote.EnvTokenFile, token) scope, err := sandbox.NewScope(ws, nil) From d3bc4232452cd1682abfe2379c28be79c91e5ea8 Mon Sep 17 00:00:00 2001 From: Amp Date: Mon, 27 Jul 2026 07:38:11 +0000 Subject: [PATCH 06/10] fix(sandbox): close daemon token tool bypasses Amp-Thread-ID: https://ampcode.com/threads/T-019fa018-e3c8-705b-9627-3a7564f8d66a Co-authored-by: Pierre Bruno --- internal/sandbox/engine.go | 5 +- internal/tools/bash_auto_allow_test.go | 7 ++ internal/tools/daemon_token_exclusion_test.go | 41 ++++++++++++ internal/tools/exec_command_test.go | 7 ++ internal/tools/list_directory.go | 27 +++++--- internal/tools/read_exclusions_test.go | 65 +++++++++++++++++++ 6 files changed, 142 insertions(+), 10 deletions(-) diff --git a/internal/sandbox/engine.go b/internal/sandbox/engine.go index ec882ae64..50a4a057b 100644 --- a/internal/sandbox/engine.go +++ b/internal/sandbox/engine.go @@ -193,7 +193,8 @@ func (engine *Engine) effectiveNetworkMode(policy Policy) NetworkMode { } // UnsandboxedExecutionAllowed reports whether an escalated shell attempt may -// bypass the native sandbox without dropping active denied-read restrictions. +// bypass the native sandbox without dropping active denied-read restrictions, +// including the automatic remote bridge token exclusion. func (engine *Engine) UnsandboxedExecutionAllowed() bool { if engine == nil { return true @@ -202,7 +203,7 @@ func (engine *Engine) UnsandboxedExecutionAllowed() bool { if policy.Mode == ModeDisabled { return true } - return len(normalizeProfilePaths(policy.DenyRead)) == 0 + return len(normalizeProfilePaths(policy.DenyRead)) == 0 && len(protectedCredentialPaths()) == 0 } // toolNetworkExempt reports whether a request is exempt from the engine-level diff --git a/internal/tools/bash_auto_allow_test.go b/internal/tools/bash_auto_allow_test.go index ef335229d..3543ca5f2 100644 --- a/internal/tools/bash_auto_allow_test.go +++ b/internal/tools/bash_auto_allow_test.go @@ -130,6 +130,13 @@ func TestBashRequireEscalatedKeepsSandboxWhenDeniedReadsActive(t *testing.T) { } } +func TestBashRequireEscalatedKeepsSandboxWhenDaemonTokenProtected(t *testing.T) { + engine := defaultPolicyDaemonTokenEngine(t) + if commandEngineForSandboxPermissions(engine, SandboxPermissionsRequireEscalated) == nil { + t.Fatal("bash require_escalated must preserve the sandbox when the default remote-daemon policy protects a token file") + } +} + func TestBashStillPromptsWithoutActiveSandbox(t *testing.T) { root := t.TempDir() registry := NewRegistry() diff --git a/internal/tools/daemon_token_exclusion_test.go b/internal/tools/daemon_token_exclusion_test.go index 19e71dcbe..d72d235a5 100644 --- a/internal/tools/daemon_token_exclusion_test.go +++ b/internal/tools/daemon_token_exclusion_test.go @@ -48,6 +48,28 @@ func daemonTokenFixture(t *testing.T) (string, string, *sandbox.Engine) { return ws, token, engine } +// defaultPolicyDaemonTokenEngine is the remote-daemon shape relevant to shell +// escalation: a selected token file with the default policy and no user +// DenyRead entries. +func defaultPolicyDaemonTokenEngine(t *testing.T) *sandbox.Engine { + t.Helper() + ws, err := filepath.EvalSymlinks(t.TempDir()) + if err != nil { + t.Fatalf("EvalSymlinks: %v", err) + } + token := filepath.Join(ws, "bridge-token") + if err := os.WriteFile(token, []byte("bridge-secret\n"), 0o600); err != nil { + t.Fatalf("write token: %v", err) + } + t.Setenv(remote.EnvToken, "") + t.Setenv(remote.EnvTokenFile, token) + policy := sandbox.DefaultPolicy() + if len(policy.DenyRead) != 0 { + t.Fatalf("default policy DenyRead = %#v, want none", policy.DenyRead) + } + return sandbox.NewEngine(sandbox.EngineOptions{WorkspaceRoot: ws, Policy: policy}) +} + func TestGrepSkipsDaemonTokenFile(t *testing.T) { ws, _, engine := daemonTokenFixture(t) tool, ok := NewScopedGrepTool(ws, nil).(sandboxAwareTool) @@ -87,6 +109,25 @@ func TestGlobSkipsDaemonTokenFile(t *testing.T) { } } +func TestListDirectorySkipsDaemonTokenFile(t *testing.T) { + ws, _, engine := daemonTokenFixture(t) + registry := NewRegistry() + registry.Register(NewScopedListDirectoryTool(ws, nil)) + + result := registry.RunWithOptions(context.Background(), "list_directory", map[string]any{ + "path": ".", + }, RunOptions{Sandbox: engine}) + if result.Status != StatusOK { + t.Fatalf("list_directory failed: %s", result.Output) + } + if !strings.Contains(result.Output, "main.go") { + t.Fatalf("list_directory must still show ordinary workspace files, got:\n%s", result.Output) + } + if strings.Contains(result.Output, "bridge-token") { + t.Fatalf("list_directory must NOT surface the remote bridge token file, got:\n%s", result.Output) + } +} + // TestEngineDeniesDaemonTokenFileTools covers the request gate the direct file // tools go through (read_file, write_file, edit_file, apply_patch): the bridge // token must be neither readable nor writable, even though AllowRead covers the diff --git a/internal/tools/exec_command_test.go b/internal/tools/exec_command_test.go index 8b4f11c8f..e0ba65510 100644 --- a/internal/tools/exec_command_test.go +++ b/internal/tools/exec_command_test.go @@ -173,6 +173,13 @@ func TestExecCommandRequireEscalatedBypassesNativeSandboxAfterApproval(t *testin } } +func TestExecCommandRequireEscalatedKeepsSandboxWhenDaemonTokenProtected(t *testing.T) { + engine := defaultPolicyDaemonTokenEngine(t) + if commandEngineForSandboxPermissions(engine, SandboxPermissionsRequireEscalated) == nil { + t.Fatal("exec_command require_escalated must preserve the sandbox when the default remote-daemon policy protects a token file") + } +} + // TestExecCommandRequireEscalatedBypassesMsysGuardAfterApproval mirrors // TestBashToolRequireEscalatedMsysGuard for exec_command: the MSYS sandbox // guard exists only because MSYS/Cygwin coreutils fail under the diff --git a/internal/tools/list_directory.go b/internal/tools/list_directory.go index dd764bd6f..008e69e5a 100644 --- a/internal/tools/list_directory.go +++ b/internal/tools/list_directory.go @@ -46,14 +46,14 @@ func NewScopedListDirectoryTool(workspaceRoot string, scope PathScope) Tool { } func (tool listDirectoryTool) Run(_ context.Context, args map[string]any) Result { - return tool.run(args, true) + return tool.run(args, readExcluder{}, true) } -func (tool listDirectoryTool) RunWithOptions(_ context.Context, args map[string]any, _ RunOptions) Result { - return tool.run(args, false) +func (tool listDirectoryTool) RunWithOptions(_ context.Context, args map[string]any, options RunOptions) Result { + return tool.run(args, sandboxReadExcluder(options.Sandbox), false) } -func (tool listDirectoryTool) run(args map[string]any, directBudget bool) Result { +func (tool listDirectoryTool) run(args map[string]any, exclude readExcluder, directBudget bool) Result { // Optional with a "." default: treat an explicit empty path (a common // weak-model quirk) the same as the key being absent rather than erroring. requestedPath, err := aliasedStringArg(args, []string{"path", "directory", "dir"}, ".", false, true) @@ -80,7 +80,7 @@ func (tool listDirectoryTool) run(args map[string]any, directBudget bool) Result return errorResult("Error listing directory " + requestedPath + ": " + err.Error()) } - entries, err := listDirectoryEntries(absolutePath, 0, maxDepth) + entries, err := listDirectoryEntries(absolutePath, 0, maxDepth, exclude) if err != nil { return errorResult("Error listing directory " + relativePath + ": " + err.Error()) } @@ -95,7 +95,7 @@ func (tool listDirectoryTool) run(args map[string]any, directBudget bool) Result return result } -func listDirectoryEntries(path string, depth int, maxDepth int) ([]string, error) { +func listDirectoryEntries(path string, depth int, maxDepth int, exclude readExcluder) ([]string, error) { dirEntries, err := os.ReadDir(path) if err != nil { return nil, err @@ -112,18 +112,29 @@ func listDirectoryEntries(path string, depth int, maxDepth int) ([]string, error if entry.IsDir() && shouldSkipDirectory(entry.Name()) { continue } + entryPath := filepath.Join(path, entry.Name()) + if entry.IsDir() && exclude.dirExcluded(entryPath) { + continue + } indent := strings.Repeat(" ", depth) if entry.IsDir() { - results = append(results, indent+entry.Name()+"/") + // A denied directory with a nested AllowRead cannot be pruned, but the + // denied directory entry itself must still stay out of the listing. + if !exclude.fileExcluded(entryPath) { + results = append(results, indent+entry.Name()+"/") + } if depth < maxDepth { - children, err := listDirectoryEntries(filepath.Join(path, entry.Name()), depth+1, maxDepth) + children, err := listDirectoryEntries(entryPath, depth+1, maxDepth, exclude) if err == nil { results = append(results, children...) } } continue } + if exclude.fileExcluded(entryPath) { + continue + } results = append(results, fmt.Sprintf("%s%s", indent, entry.Name())) } diff --git a/internal/tools/read_exclusions_test.go b/internal/tools/read_exclusions_test.go index 1feb9880b..0ca174a99 100644 --- a/internal/tools/read_exclusions_test.go +++ b/internal/tools/read_exclusions_test.go @@ -97,3 +97,68 @@ func TestGlobSkipsDenyReadSubtree(t *testing.T) { t.Fatalf("non-sandboxed glob should include the secret file, got:\n%s", plain.Output) } } + +func TestListDirectorySkipsDenyReadSubtree(t *testing.T) { + ws, engine := denyReadFixture(t) + registry := NewRegistry() + registry.Register(NewScopedListDirectoryTool(ws, nil)) + args := map[string]any{"path": ".", "recursive": true, "max_depth": 2} + + sandboxed := registry.RunWithOptions(context.Background(), "list_directory", args, RunOptions{Sandbox: engine}) + if sandboxed.Status != StatusOK { + t.Fatalf("list_directory failed: %s", sandboxed.Output) + } + if !strings.Contains(sandboxed.Output, "main.go") { + t.Fatalf("list_directory must still show the non-denied file, got:\n%s", sandboxed.Output) + } + if strings.Contains(sandboxed.Output, "secret") || strings.Contains(sandboxed.Output, "creds.go") { + t.Fatalf("list_directory must NOT surface a DenyRead subtree, got:\n%s", sandboxed.Output) + } + + plain := NewScopedListDirectoryTool(ws, nil).Run(context.Background(), args) + if !strings.Contains(plain.Output, "secret/") || !strings.Contains(plain.Output, "creds.go") { + t.Fatalf("non-sandboxed list_directory should include the secret subtree, got:\n%s", plain.Output) + } +} + +func TestListDirectoryDescendsToNestedAllowRead(t *testing.T) { + ws, err := filepath.EvalSymlinks(t.TempDir()) + if err != nil { + t.Fatalf("EvalSymlinks: %v", err) + } + secret := filepath.Join(ws, "secret") + allowed := filepath.Join(secret, "allowed") + if err := os.MkdirAll(allowed, 0o755); err != nil { + t.Fatalf("mkdir allowed: %v", err) + } + if err := os.WriteFile(filepath.Join(secret, "hidden.txt"), []byte("hidden\n"), 0o600); err != nil { + t.Fatalf("write hidden: %v", err) + } + if err := os.WriteFile(filepath.Join(allowed, "visible.txt"), []byte("visible\n"), 0o600); err != nil { + t.Fatalf("write visible: %v", err) + } + engine := sandbox.NewEngine(sandbox.EngineOptions{ + WorkspaceRoot: ws, + Policy: sandbox.Policy{ + Mode: sandbox.ModeEnforce, + EnforceWorkspace: true, + DenyRead: []string{secret}, + AllowRead: []string{allowed}, + }, + }) + registry := NewRegistry() + registry.Register(NewScopedListDirectoryTool(ws, nil)) + + result := registry.RunWithOptions(context.Background(), "list_directory", map[string]any{ + "path": ".", "recursive": true, "max_depth": 3, + }, RunOptions{Sandbox: engine}) + if result.Status != StatusOK { + t.Fatalf("list_directory failed: %s", result.Output) + } + if !strings.Contains(result.Output, "allowed/") || !strings.Contains(result.Output, "visible.txt") { + t.Fatalf("list_directory must descend to the nested AllowRead subtree, got:\n%s", result.Output) + } + if strings.Contains(result.Output, "secret/") || strings.Contains(result.Output, "hidden.txt") { + t.Fatalf("list_directory must hide denied entries outside the nested AllowRead subtree, got:\n%s", result.Output) + } +} From fc777ca8c1326ac6300a72e8db458a95b2511475 Mon Sep 17 00:00:00 2001 From: Amp Date: Tue, 28 Jul 2026 08:33:04 +0000 Subject: [PATCH 07/10] fix(sandbox): deny daemon token aliases Amp-Thread-ID: https://ampcode.com/threads/T-019fa7a0-1223-701d-9529-48ba5d7cf8c8 Co-authored-by: Pierre Bruno --- internal/sandbox/pathlists.go | 28 ++++++++++ internal/tools/daemon_token_exclusion_test.go | 55 +++++++++++++++++++ 2 files changed, 83 insertions(+) diff --git a/internal/sandbox/pathlists.go b/internal/sandbox/pathlists.go index 79a82a617..b49b26dab 100644 --- a/internal/sandbox/pathlists.go +++ b/internal/sandbox/pathlists.go @@ -138,6 +138,34 @@ func protectedPathDenied(protected []string, workspaceRoot, path string) bool { return true } } + + // Keep the lexical check above: in particular, it protects a configured + // pathname even when the file is absent, preventing its replacement. For an + // existing request, also compare the object reached by the filesystem. This + // closes aliases created after the token path was selected: EvalSymlinks + // catches symbolic links, while SameFile catches hard links (and any other + // platform-specific names for the same file). + abs := path + if !filepath.IsAbs(abs) { + if workspaceRoot == "" { + return false + } + abs = filepath.Join(workspaceRoot, abs) + } + resolved, err := filepath.EvalSymlinks(abs) + if err != nil { + return false + } + requestInfo, err := os.Stat(resolved) + if err != nil { + return false + } + for _, entry := range protected { + protectedInfo, err := os.Stat(entry) + if err == nil && os.SameFile(requestInfo, protectedInfo) { + return true + } + } return false } diff --git a/internal/tools/daemon_token_exclusion_test.go b/internal/tools/daemon_token_exclusion_test.go index d72d235a5..f2bb8e8f0 100644 --- a/internal/tools/daemon_token_exclusion_test.go +++ b/internal/tools/daemon_token_exclusion_test.go @@ -170,3 +170,58 @@ func TestEngineDeniesDaemonTokenFileTools(t *testing.T) { t.Fatalf("ordinary workspace read was denied: %q", decision.Reason) } } + +func TestDaemonTokenAliasesDeniedEndToEnd(t *testing.T) { + for _, aliasKind := range []string{"symlink", "hardlink"} { + t.Run(aliasKind, func(t *testing.T) { + ws, token, engine := daemonTokenFixture(t) + alias := filepath.Join(ws, "token-alias") + var err error + switch aliasKind { + case "symlink": + err = os.Symlink(token, alias) + case "hardlink": + err = os.Link(token, alias) + } + if err != nil { + t.Skipf("%s unsupported: %v", aliasKind, err) + } + + registry := NewRegistry() + registry.Register(NewScopedReadFileTool(ws, nil)) + registry.Register(NewScopedWriteFileTool(ws, nil)) + + read := registry.RunWithOptions(context.Background(), "read_file", map[string]any{"path": alias}, RunOptions{Sandbox: engine}) + if read.Status == StatusOK || strings.Contains(read.Output, "bridge-secret") { + t.Fatalf("read_file followed protected %s: status=%s output=%q", aliasKind, read.Status, read.Output) + } + + write := registry.RunWithOptions(context.Background(), "write_file", map[string]any{"path": alias, "content": "attacker-controlled\n"}, RunOptions{Sandbox: engine}) + if write.Status == StatusOK { + t.Fatalf("write_file followed protected %s: output=%q", aliasKind, write.Output) + } + contents, err := os.ReadFile(token) + if err != nil || string(contents) != "bridge-secret\n" { + t.Fatalf("token changed after denied write through %s: contents=%q err=%v", aliasKind, contents, err) + } + + grep, ok := NewScopedGrepTool(ws, nil).(sandboxAwareTool) + if !ok { + t.Fatal("grep tool must be sandbox-aware") + } + result := grep.RunWithSandbox(context.Background(), map[string]any{ + "pattern": "bridge-secret", + "output_mode": "files_with_matches", + }, engine) + if result.Status != StatusOK { + t.Fatalf("grep failed: %s", result.Output) + } + if strings.Contains(result.Output, "token-alias") || strings.Contains(result.Output, "bridge-token") { + t.Fatalf("grep surfaced protected %s target:\n%s", aliasKind, result.Output) + } + if !strings.Contains(result.Output, "main.go") { + t.Fatalf("grep omitted ordinary file while filtering %s:\n%s", aliasKind, result.Output) + } + }) + } +} From 643eb6e32a00e4d943b6320d55325a655ca07515 Mon Sep 17 00:00:00 2001 From: PierrunoYT Date: Tue, 28 Jul 2026 22:34:03 +0200 Subject: [PATCH 08/10] docs(sandbox): state the daemon-token boundary at the OS layer MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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) --- internal/sandbox/engine.go | 19 +++++++- internal/sandbox/pathlists.go | 19 ++++++++ .../sandbox/protected_credentials_test.go | 45 +++++++++++++++++++ internal/sandbox/runner.go | 7 +++ internal/tools/daemon_token_exclusion_test.go | 10 +++++ 5 files changed, 99 insertions(+), 1 deletion(-) diff --git a/internal/sandbox/engine.go b/internal/sandbox/engine.go index 50a4a057b..f179619e4 100644 --- a/internal/sandbox/engine.go +++ b/internal/sandbox/engine.go @@ -195,6 +195,12 @@ func (engine *Engine) effectiveNetworkMode(policy Policy) NetworkMode { // UnsandboxedExecutionAllowed reports whether an escalated shell attempt may // bypass the native sandbox without dropping active denied-read restrictions, // including the automatic remote bridge token exclusion. +// +// ModeDisabled short-circuits to true before the token is consulted, and that is +// the intended reading of the switch rather than an oversight: with the sandbox +// off there is no wrapper for an escalation to bypass, so refusing the +// escalation would deny a capability the operator already granted globally +// without protecting anything. See the ModeDisabled branch in Evaluate. func (engine *Engine) UnsandboxedExecutionAllowed() bool { if engine == nil { return true @@ -332,7 +338,18 @@ func (engine *Engine) Evaluate(ctx context.Context, request Request) Decision { // Disabling the sandbox drops every user-configured restriction, but not the // automatic credential exclusion: the remote bridge token authenticates the // caller driving these tools, so it stays unreadable and unwritable through - // them (a shell command is a separate, OS-level boundary). + // them. + // + // Shell is deliberately left open here, and it is worth being blunt about + // why. ModeDisabled means no OS wrapper is built at all — the runner sets + // SandboxPreferenceForbid and PermissionProfileFromPolicy returns an + // unrestricted filesystem — so there is no layer left that could confine a + // command. Re-wrapping shell just for this file would quietly undo the + // switch the operator flipped, and would not hold anyway: a shell that can + // run anything can read anything this process can. The exclusion below is + // therefore what it says — a guarantee about Zero's own file tools, not + // about the machine. Running a remote bridge with the sandbox disabled + // means trusting whoever can drive that bridge with the token. if block := protectedCredentialPathBlock(request, request.WorkspaceRoot); block != nil { return deny(request, risk, block.Code, block.Path, block.Reason, false) } diff --git a/internal/sandbox/pathlists.go b/internal/sandbox/pathlists.go index b49b26dab..811254c37 100644 --- a/internal/sandbox/pathlists.go +++ b/internal/sandbox/pathlists.go @@ -104,6 +104,12 @@ func protectedCredentialPaths() []string { // that targets a protected credential file, or nil. It exists for the callers // that bypass validatePathWithPolicy — currently the ModeDisabled short-circuit, // where the exclusion still applies because it is not policy-derived. +// +// Only the side effects that name a path are covered. SideEffectShell is not, +// and cannot be: a shell request carries a command line, not a file path, so +// there is nothing here to compare against the token. Shell is confined by the +// OS wrapper instead — which under ModeDisabled does not exist. See the +// ModeDisabled short-circuit in Engine.Evaluate for what that boundary means. func protectedCredentialPathBlock(request Request, workspaceRoot string) *pathBlock { switch request.SideEffect { case SideEffectRead, SideEffectWrite, SideEffectOutOfWorkspace: @@ -145,6 +151,19 @@ func protectedPathDenied(protected []string, workspaceRoot, path string) bool { // closes aliases created after the token path was selected: EvalSymlinks // catches symbolic links, while SameFile catches hard links (and any other // platform-specific names for the same file). + // + // This inode-level closure is specific to Zero's in-process tools, which see + // every requested path before opening it. The OS layer is pathname-based and + // stays that way: seatbelt and Bubblewrap rules name paths, so a sandboxed + // shell on macOS can still `ln alias && cat alias` — a hard link is a + // second name for the same inode, and no path-based rule covers a name that + // did not exist when the profile was built. That is the same model a + // user-configured DenyRead has always had, deliberately: an aliasing defense + // at the OS layer would have to resolve every path at open time, which is not + // something either backend's policy language expresses. Shell access to a + // host running a remote bridge is therefore access to the token, and the + // protection here is the in-process boundary plus the pathname deny rules, + // not an inode-tight OS guarantee. abs := path if !filepath.IsAbs(abs) { if workspaceRoot == "" { diff --git a/internal/sandbox/protected_credentials_test.go b/internal/sandbox/protected_credentials_test.go index 1270525e7..cc2f46cfb 100644 --- a/internal/sandbox/protected_credentials_test.go +++ b/internal/sandbox/protected_credentials_test.go @@ -240,6 +240,51 @@ func TestProtectedCredentialsSurviveDisabledPolicy(t *testing.T) { } } +// TestDisabledPolicyLeavesShellOutsideTheTokenBoundary pins the boundary jatmn +// asked to see stated for #685: under ModeDisabled the bridge-token exclusion +// covers Zero's in-process file tools and nothing else. No OS wrapper is built +// at all in that mode, so a shell command is confined by nothing and an +// escalation has nothing to bypass. This test exists so that stops being an +// implicit property — changing any of it should mean changing this test on +// purpose, not discovering the behavior later. +func TestDisabledPolicyLeavesShellOutsideTheTokenBoundary(t *testing.T) { + ws, token := protectedTokenFixture(t) + engine := NewEngine(EngineOptions{WorkspaceRoot: ws, Policy: Policy{Mode: ModeDisabled}}) + + shell := engine.Evaluate(context.Background(), Request{ + ToolName: "bash", + WorkspaceRoot: ws, + SideEffect: SideEffectShell, + Args: map[string]any{"command": "cat " + token}, + }) + if shell.Action != ActionAllow { + t.Fatalf("shell under a disabled policy = %q (%s); the token boundary is documented as in-process only", shell.Action, shell.Reason) + } + + // The same command's payload IS blocked when it arrives as a path-carrying + // request, which is the whole of the guarantee. + read := engine.Evaluate(context.Background(), Request{ + ToolName: "read_file", + WorkspaceRoot: ws, + SideEffect: SideEffectRead, + Args: map[string]any{"path": token}, + }) + if read.Action != ActionDeny { + t.Fatalf("in-process read under a disabled policy = %q (%s), want deny", read.Action, read.Reason) + } + + if !engine.UnsandboxedExecutionAllowed() { + t.Fatal("escalation under a disabled policy must stay allowed: there is no wrapper for it to bypass") + } + + // With the sandbox on, the same configured token flips both: the profile is + // built, so escalating out of it would drop a real deny rule. + enforcing := NewEngine(EngineOptions{WorkspaceRoot: ws, Policy: DefaultPolicy()}) + if enforcing.UnsandboxedExecutionAllowed() { + t.Fatal("escalation must be refused while a bridge token is protected by an active profile") + } +} + // TestProtectedCredentialsMatchCaseVariantOnCaseInsensitiveFilesystems covers the // bypass a case-variant spelling opened: pathWithinRoot ends in filepath.Rel, // which folds case on Windows but NOT on darwin, whose default APFS volume is diff --git a/internal/sandbox/runner.go b/internal/sandbox/runner.go index a30030f01..128655eb7 100644 --- a/internal/sandbox/runner.go +++ b/internal/sandbox/runner.go @@ -789,6 +789,13 @@ func seatbeltProtectedMetadataRegex(root string, name string) string { return "^" + escapedRoot + "/" + escapedName + "(/.*)?$" } +// denyReadRules emits seatbelt deny rules for the profile's read-denied paths. +// The rules name paths — `(literal …)` for files, `(subpath …)` for directories +// — because that is the whole of seatbelt's vocabulary here. A name created +// afterwards for the same inode, most obviously a hard link made by a sandboxed +// shell, is not covered by any of them. That applies equally to a +// user-configured DenyRead and to the automatic remote-bridge-token deny; the +// in-process tools close inode aliases separately (see protectedPathDenied). func denyReadRules(fs FileSystemPolicy) []string { return denySeatbeltPathRules("file-read*", fs.DenyRead) } diff --git a/internal/tools/daemon_token_exclusion_test.go b/internal/tools/daemon_token_exclusion_test.go index f2bb8e8f0..c28db439a 100644 --- a/internal/tools/daemon_token_exclusion_test.go +++ b/internal/tools/daemon_token_exclusion_test.go @@ -171,6 +171,16 @@ func TestEngineDeniesDaemonTokenFileTools(t *testing.T) { } } +// TestDaemonTokenAliasesDeniedEndToEnd covers the in-process tools, which are +// the layer that can close inode aliases: they see every requested path before +// opening it, so a symlink or hard link to the token resolves back to the +// protected inode and is refused. +// +// The OS layer deliberately does not match this. Seatbelt and Bubblewrap rules +// name paths, so a sandboxed shell on macOS can `ln alias && cat alias` +// — the same pathname model a user-configured DenyRead has always had. See +// protectedPathDenied and denyReadRules in internal/sandbox for why that is the +// boundary rather than an omission. func TestDaemonTokenAliasesDeniedEndToEnd(t *testing.T) { for _, aliasKind := range []string{"symlink", "hardlink"} { t.Run(aliasKind, func(t *testing.T) { From 6e561b63fc88ef02a8dbb1fcca5902846d4912af Mon Sep 17 00:00:00 2001 From: Amp Date: Wed, 29 Jul 2026 23:58:55 +0000 Subject: [PATCH 09/10] test(sandbox): pin daemon token protections Amp-Thread-ID: https://ampcode.com/threads/T-019fafa2-7d9d-75bc-8801-6d0efc8db1df Co-authored-by: Pierre Bruno --- internal/cli/daemon.go | 1 + internal/cli/daemon_test.go | 80 +++++++++++++++++++ internal/daemon/remote/auth_test.go | 5 +- internal/sandbox/pathlists.go | 3 + .../sandbox/protected_credentials_test.go | 21 +++++ 5 files changed, 109 insertions(+), 1 deletion(-) diff --git a/internal/cli/daemon.go b/internal/cli/daemon.go index dafede623..e7e563474 100644 --- a/internal/cli/daemon.go +++ b/internal/cli/daemon.go @@ -63,6 +63,7 @@ Commands: daemon. Requires a bearer token in $ZERO_DAEMON_REMOTE_TOKEN (or $ZERO_DAEMON_REMOTE_TOKEN_FILE). --bundle-dir enables git-bundle uploads, extracted into per-link work trees. + An inline token takes precedence, so a stale token-file pointer is intentionally not protected as the live credential. link --remote --repo --id [--out ] Upload repo's git history to the remote as a bundle and print the extracted remote path. --out saves a session diff --git a/internal/cli/daemon_test.go b/internal/cli/daemon_test.go index 5418537cc..f37145d1a 100644 --- a/internal/cli/daemon_test.go +++ b/internal/cli/daemon_test.go @@ -2,8 +2,19 @@ package cli import ( "bytes" + "crypto/ecdsa" + "crypto/elliptic" + "crypto/rand" + "crypto/x509" + "crypto/x509/pkix" + "encoding/pem" + "math/big" + "net" + "os" + "path/filepath" "strings" "testing" + "time" ) // isolateDaemonPaths points DefaultPaths at a temp dir so the test never touches @@ -121,3 +132,72 @@ func TestDaemonSubcommandsRejectExtraArgs(t *testing.T) { } } } + +func TestDaemonServeRemoteCanonicalizesTokenFileBeforeStartingWorkers(t *testing.T) { + isolateDaemonPaths(t) + certFile, keyFile := writeDaemonTestCertificate(t) + startDir := t.TempDir() + t.Chdir(startDir) + if err := os.WriteFile("token", []byte("bridge-token"), 0o600); err != nil { + t.Fatal(err) + } + t.Setenv("ZERO_DAEMON_REMOTE_TOKEN", "") + t.Setenv("ZERO_DAEMON_REMOTE_TOKEN_FILE", "token") + + code, _, _ := runDaemonCLI(t, "serve-remote", "--addr", "127.0.0.1:not-a-port", "--tls-cert", certFile, "--tls-key", keyFile) + if code != exitCrash { + t.Fatalf("serve-remote exit = %d, want bind failure", code) + } + want := filepath.Join(startDir, "token") + if got := os.Getenv("ZERO_DAEMON_REMOTE_TOKEN_FILE"); got != want { + t.Fatalf("ZERO_DAEMON_REMOTE_TOKEN_FILE = %q, want daemon-pinned path %q", got, want) + } +} + +func writeDaemonTestCertificate(t *testing.T) (string, string) { + t.Helper() + key, err := ecdsa.GenerateKey(elliptic.P256(), rand.Reader) + if err != nil { + t.Fatal(err) + } + template := &x509.Certificate{ + SerialNumber: big.NewInt(1), + Subject: pkix.Name{CommonName: "zero-daemon-test"}, + NotBefore: time.Now().Add(-time.Hour), + NotAfter: time.Now().Add(time.Hour), + KeyUsage: x509.KeyUsageDigitalSignature, + ExtKeyUsage: []x509.ExtKeyUsage{x509.ExtKeyUsageServerAuth}, + IPAddresses: []net.IP{net.ParseIP("127.0.0.1")}, + } + der, err := x509.CreateCertificate(rand.Reader, template, template, &key.PublicKey, key) + if err != nil { + t.Fatal(err) + } + dir := t.TempDir() + certFile, keyFile := filepath.Join(dir, "cert.pem"), filepath.Join(dir, "key.pem") + certOut, err := os.Create(certFile) + if err != nil { + t.Fatal(err) + } + if err := pem.Encode(certOut, &pem.Block{Type: "CERTIFICATE", Bytes: der}); err != nil { + t.Fatal(err) + } + if err := certOut.Close(); err != nil { + t.Fatal(err) + } + keyDER, err := x509.MarshalECPrivateKey(key) + if err != nil { + t.Fatal(err) + } + keyOut, err := os.Create(keyFile) + if err != nil { + t.Fatal(err) + } + if err := pem.Encode(keyOut, &pem.Block{Type: "EC PRIVATE KEY", Bytes: keyDER}); err != nil { + t.Fatal(err) + } + if err := keyOut.Close(); err != nil { + t.Fatal(err) + } + return certFile, keyFile +} diff --git a/internal/daemon/remote/auth_test.go b/internal/daemon/remote/auth_test.go index 4fef66b82..25c21acee 100644 --- a/internal/daemon/remote/auth_test.go +++ b/internal/daemon/remote/auth_test.go @@ -81,8 +81,11 @@ func TestCanonicalizeTokenFileEnv(t *testing.T) { if got := os.Getenv(EnvTokenFile); got != token { t.Fatalf("%s = %q, want %q", EnvTokenFile, got, token) } + // Workers run from session directories, so prove the selected path remains + // pinned after crossing that boundary rather than merely inspecting the env. + t.Chdir(t.TempDir()) if tok, err := TokenFromEnv(); err != nil || tok != "from-file" { - t.Fatalf("TokenFromEnv after canonicalization = %q, %v", tok, err) + t.Fatalf("TokenFromEnv from a worker directory after canonicalization = %q, %v", tok, err) } }) diff --git a/internal/sandbox/pathlists.go b/internal/sandbox/pathlists.go index 811254c37..9d6d2e748 100644 --- a/internal/sandbox/pathlists.go +++ b/internal/sandbox/pathlists.go @@ -139,6 +139,9 @@ func protectedCredentialPathBlock(request Request, workspaceRoot string) *pathBl // protectedPathDenied reports whether path targets one of the protected // credential files. There is no allow-list consultation by design. func protectedPathDenied(protected []string, workspaceRoot, path string) bool { + if len(protected) == 0 { + return false + } for _, entry := range protected { if pathUnderProtectedRoot(path, entry, workspaceRoot) { return true diff --git a/internal/sandbox/protected_credentials_test.go b/internal/sandbox/protected_credentials_test.go index cc2f46cfb..25d5e7cc9 100644 --- a/internal/sandbox/protected_credentials_test.go +++ b/internal/sandbox/protected_credentials_test.go @@ -140,6 +140,27 @@ func TestProtectedCredentialsSurviveAllowRead(t *testing.T) { } } +func TestProtectedCredentialDirExcluded(t *testing.T) { + ws, token := protectedTokenFixture(t) + engine := NewEngine(EngineOptions{ + WorkspaceRoot: ws, + Policy: Policy{Mode: ModeEnforce, EnforceWorkspace: true, AllowRead: []string{token}}, + }) + + if !engine.ReadExclusions().DirExcluded(token) { + t.Fatalf("DirExcluded must enforce the protected credential path %q", token) + } +} + +func TestProtectedCredentialPreventsUnsandboxedExecution(t *testing.T) { + ws, _ := protectedTokenFixture(t) + engine := NewEngine(EngineOptions{WorkspaceRoot: ws, Policy: DefaultPolicy()}) + + if engine.UnsandboxedExecutionAllowed() { + t.Fatal("UnsandboxedExecutionAllowed must stay false while a credential path is protected") + } +} + // TestProtectedCredentialsRejectSessionPermissionProfile covers the other // re-inclusion route: a session/turn permission profile that asks for the token // path must not be auto-applicable. From d402545a69d123d7883c06cd190bc595317066ca Mon Sep 17 00:00:00 2001 From: PierrunoYT Date: Thu, 30 Jul 2026 09:25:15 +0200 Subject: [PATCH 10/10] test(cli): compare canonical daemon token path Amp-Thread-ID: https://ampcode.com/threads/T-019fb1dc-2159-77ee-b020-f471d016544e Co-authored-by: Amp --- internal/cli/daemon_test.go | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/internal/cli/daemon_test.go b/internal/cli/daemon_test.go index f37145d1a..ee01cd207 100644 --- a/internal/cli/daemon_test.go +++ b/internal/cli/daemon_test.go @@ -149,6 +149,10 @@ func TestDaemonServeRemoteCanonicalizesTokenFileBeforeStartingWorkers(t *testing t.Fatalf("serve-remote exit = %d, want bind failure", code) } want := filepath.Join(startDir, "token") + want, err := filepath.EvalSymlinks(want) + if err != nil { + t.Fatalf("EvalSymlinks(%q): %v", want, err) + } if got := os.Getenv("ZERO_DAEMON_REMOTE_TOKEN_FILE"); got != want { t.Fatalf("ZERO_DAEMON_REMOTE_TOKEN_FILE = %q, want daemon-pinned path %q", got, want) }