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..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") } @@ -981,6 +989,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..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) } } @@ -653,6 +690,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", }