From 85334920b79ae70793e0ff87a302792dac626b22 Mon Sep 17 00:00:00 2001 From: PierrunoYT Date: Tue, 14 Jul 2026 21:42:47 +0200 Subject: [PATCH 1/2] 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 a2b8550f..90ed371b 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 19473666..bc818add 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 4bcf8380..721f73e8 100644 --- a/internal/sandbox/runner.go +++ b/internal/sandbox/runner.go @@ -981,6 +981,7 @@ func scrubSensitiveEnv(env []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 5e7a5b25..a26f5557 100644 --- a/internal/sandbox/runner_test.go +++ b/internal/sandbox/runner_test.go @@ -653,6 +653,7 @@ func TestScrubSensitiveEnv(t *testing.T) { "XAI_API_KEY=xai-12345", "HUGGINGFACE_API_KEY=hf_12345", "GOOGLE_APPLICATION_CREDENTIALS=/home/user/sa-key.json", + "ZERO_DAEMON_REMOTE_TOKEN_FILE=/home/user/daemon-token", "AWS_PROFILE=staging", "SAFE_VAR=hello", } From 2248aca886f2744c31ff28a49125ae3a5350e685 Mon Sep 17 00:00:00 2001 From: PierrunoYT Date: Wed, 15 Jul 2026 04:33:03 +0200 Subject: [PATCH 2/2] fix(sandbox): deny writes to DenyRead credential files on macOS Address code review on PR #685: the Seatbelt profile only translated DenyRead entries into file-read* and file-write-unlink denials. The broad file-write* allowance for workspace/temp write roots still covered a DenyRead file (e.g. the file ZERO_DAEMON_REMOTE_TOKEN_FILE names) if it happened to sit under one of them, so a sandboxed command could discover and overwrite/truncate the daemon bearer-token file even though it couldn't read or delete it. A file a sandboxed command must not read has no legitimate reason to be written either, so seatbeltProfileFromPermissionProfile now also emits a full file-write* deny for every DenyRead path, placed after the broad write allow (deny rules that follow an allow win, matching the existing DenyWrite/metadata-carveout ordering). Adds a regression test with a DenyRead file under a writable /tmp root, and extends the existing deny-ordering test to assert the new file-write* rule. Co-Authored-By: Claude Sonnet 5 --- internal/sandbox/runner.go | 8 +++++++ internal/sandbox/runner_test.go | 41 +++++++++++++++++++++++++++++++-- 2 files changed, 47 insertions(+), 2 deletions(-) diff --git a/internal/sandbox/runner.go b/internal/sandbox/runner.go index 721f73e8..4f13a98c 100644 --- a/internal/sandbox/runner.go +++ b/internal/sandbox/runner.go @@ -608,6 +608,14 @@ func seatbeltProfileFromPermissionProfile(profile PermissionProfile, policy Poli rules = append(rules, denyReadRules(profile.FileSystem)...) rules = append(rules, writeRootCarveoutDenyRules(profile.FileSystem)...) rules = append(rules, denyWriteRulesFromPaths(profile.FileSystem.DenyWrite)...) + // denyReadRules only denies file-read* and, for a regular file, the + // separate file-write-unlink operation — it does NOT deny file-write*, so + // a DenyRead credential file sitting under a writable root/temp tree (e.g. + // ZERO_DAEMON_REMOTE_TOKEN_FILE under /tmp) is still overwritable/ + // truncatable by the broad write-rule allowance above even though it + // can't be read or deleted. A file a sandboxed command must not READ has + // no legitimate reason to be WRITTEN either, so deny both. + rules = append(rules, denyWriteRulesFromPaths(profile.FileSystem.DenyRead)...) rules = append(rules, networkRule) return strings.Join(nonEmptyStrings(rules), "\n") } diff --git a/internal/sandbox/runner_test.go b/internal/sandbox/runner_test.go index a26f5557..2b8ca576 100644 --- a/internal/sandbox/runner_test.go +++ b/internal/sandbox/runner_test.go @@ -439,6 +439,11 @@ func TestSeatbeltProfileProtectsMetadataAndDenyOrdering(t *testing.T) { normalizedSecretWrite := sandboxProfileString(normalizeProfilePath("/repo/secret-write")) denySecretReadRule := `(deny file-read* (subpath "` + normalizedSecretRead + `"))` denySecretReadUnlinkRule := `(deny file-write-unlink (subpath "` + normalizedSecretRead + `"))` + // A DenyRead path must also be write-denied, not just unlink-denied: + // otherwise a sandboxed command can't delete or read it but can still + // overwrite/truncate it if it happens to sit under a writable root (see + // TestSeatbeltProfileDeniesWritesToDenyReadUnderWritableRoot). + denySecretReadWriteRule := `(deny file-write* (subpath "` + normalizedSecretRead + `"))` denySecretWriteRule := `(deny file-write* (subpath "` + normalizedSecretWrite + `"))` for _, want := range []string{ `(deny file-write* (literal "/repo/vendor"))`, @@ -447,6 +452,7 @@ func TestSeatbeltProfileProtectsMetadataAndDenyOrdering(t *testing.T) { `(deny file-write* (regex #"^/repo/\.zero(/.*)?$"))`, denySecretReadRule, denySecretReadUnlinkRule, + denySecretReadWriteRule, denySecretWriteRule, } { if !strings.Contains(sbpl, want) { @@ -455,10 +461,41 @@ func TestSeatbeltProfileProtectsMetadataAndDenyOrdering(t *testing.T) { } allowIdx := strings.Index(sbpl, "(allow file-write*") denyReadIdx := strings.Index(sbpl, denySecretReadRule) + denyReadWriteIdx := strings.Index(sbpl, denySecretReadWriteRule) metadataIdx := strings.Index(sbpl, `(deny file-write* (regex #"^/repo/\.git(/.*)?$"))`) denyWriteIdx := strings.Index(sbpl, denySecretWriteRule) - if allowIdx < 0 || denyReadIdx < allowIdx || metadataIdx < allowIdx || denyWriteIdx < allowIdx { - t.Fatalf("deny rules must follow the broad write allow (allow=%d denyRead=%d metadata=%d denyWrite=%d):\n%s", allowIdx, denyReadIdx, metadataIdx, denyWriteIdx, sbpl) + if allowIdx < 0 || denyReadIdx < allowIdx || denyReadWriteIdx < allowIdx || metadataIdx < allowIdx || denyWriteIdx < allowIdx { + t.Fatalf("deny rules must follow the broad write allow (allow=%d denyRead=%d denyReadWrite=%d metadata=%d denyWrite=%d):\n%s", allowIdx, denyReadIdx, denyReadWriteIdx, metadataIdx, denyWriteIdx, sbpl) + } +} + +// TestSeatbeltProfileDeniesWritesToDenyReadUnderWritableRoot reproduces the +// audit finding that a DenyRead credential file (e.g. the file +// ZERO_DAEMON_REMOTE_TOKEN_FILE names) sitting under a writable root/temp +// tree was still overwritable/truncatable: the old profile only emitted +// file-read* and file-write-unlink denials for DenyRead, and the broad +// file-write* allowance for writable roots covered the file too. +func TestSeatbeltProfileDeniesWritesToDenyReadUnderWritableRoot(t *testing.T) { + profile := PermissionProfile{ + FileSystem: FileSystemPolicy{ + Kind: FileSystemRestricted, + ReadRoots: []string{"/"}, + WriteRoots: []WritableRoot{{Root: "/tmp"}}, + DenyRead: []string{"/tmp/daemon-token"}, + AllowTemp: true, + }, + Network: NetworkPolicy{Mode: NetworkDeny}, + } + sbpl := seatbeltProfileFromPermissionProfile(profile, Policy{Mode: ModeEnforce}, "") + normalizedToken := sandboxProfileString(normalizeProfilePath("/tmp/daemon-token")) + denyWriteRule := `(deny file-write* (subpath "` + normalizedToken + `"))` + if !strings.Contains(sbpl, denyWriteRule) { + t.Fatalf("Seatbelt profile missing %q, so a token file under a writable root stays overwritable:\n%s", denyWriteRule, sbpl) + } + allowIdx := strings.Index(sbpl, "(allow file-write*") + denyIdx := strings.Index(sbpl, denyWriteRule) + if allowIdx < 0 || denyIdx < allowIdx { + t.Fatalf("deny-write for the DenyRead file must follow the broad write allow (allow=%d deny=%d):\n%s", allowIdx, denyIdx, sbpl) } }