diff --git a/internal/sandbox/manager_test.go b/internal/sandbox/manager_test.go index a2b8550f..43fcde5a 100644 --- a/internal/sandbox/manager_test.go +++ b/internal/sandbox/manager_test.go @@ -378,43 +378,154 @@ func TestCredentialDenyReadPathsIn(t *testing.T) { home := t.TempDir() awsDir := filepath.Join(home, ".aws") gcloudDir := filepath.Join(home, ".config", "gcloud") - if err := mkdirAll(awsDir, gcloudDir); err != nil { + zeroDir := filepath.Join(home, "config", "zero") + if err := mkdirAll(awsDir, gcloudDir, zeroDir); err != nil { t.Fatal(err) } keyFile := filepath.Join(home, "sa-key.json") - if err := os.WriteFile(keyFile, []byte("{}"), 0o600); err != nil { - t.Fatal(err) + oauthOverride := filepath.Join(home, "custom-oauth.json") + mcpOverride := filepath.Join(home, "custom-mcp-oauth.json") + // The migrated legacy MCP token backup and the atomic-write temp siblings + // every store publishes before its rename; none of these are itemized by + // name, so they only stay protected if the whole zeroDir is denied. + zeroFiles := []string{ + filepath.Join(zeroDir, "config.json"), + filepath.Join(zeroDir, "credentials.json"), + filepath.Join(zeroDir, "credentials.enc"), + filepath.Join(zeroDir, "credentials.enc.secret"), + filepath.Join(zeroDir, "oauth-tokens.json"), + filepath.Join(zeroDir, "oauth-tokens.json.secret"), + filepath.Join(zeroDir, "mcp-oauth-tokens.json"), + filepath.Join(zeroDir, "mcp-oauth-tokens.json.secret"), + filepath.Join(zeroDir, "mcp-oauth-tokens.json.migrated"), + filepath.Join(zeroDir, "oauth-tokens.json.tmp-1234-5678"), + filepath.Join(zeroDir, "credentials.enc.9-1.tmp"), + filepath.Join(zeroDir, ".zero-config-1.tmp"), + } + for _, path := range append([]string{keyFile, oauthOverride, oauthOverride + ".secret", mcpOverride, mcpOverride + ".secret"}, zeroFiles...) { + if err := os.WriteFile(path, []byte("{}"), 0o600); err != nil { + t.Fatal(err) + } } - paths := credentialDenyReadPathsIn(home, keyFile, nil) - for _, want := range normalizeProfilePaths([]string{awsDir, gcloudDir, keyFile}) { + options := credentialPathOptions{ + Home: home, + GoogleCredentials: keyFile, + ZeroConfigDir: filepath.Join(home, "config"), + OAuthTokens: oauthOverride, + MCPOAuthTokens: mcpOverride, + } + paths := credentialDenyReadPathsIn(options, nil) + wantPaths := []string{awsDir, gcloudDir, keyFile, oauthOverride, oauthOverride + ".secret", mcpOverride, mcpOverride + ".secret", zeroDir} + for _, want := range normalizeProfilePaths(wantPaths) { if !stringSliceContains(paths, want) { t.Errorf("credential deny paths = %#v, want %q included", paths, want) } } + // zeroFiles is covered by the zeroDir subpath deny above, not by an + // itemized entry — including the never-enumerated migrated backup and + // temp-write siblings. + for _, zeroFile := range zeroFiles { + if stringSliceContains(paths, normalizeProfilePaths([]string{zeroFile})[0]) { + t.Errorf("credential deny paths = %#v, want itemized %q dropped in favor of the zeroDir subpath rule", paths, zeroFile) + } + } - // A path the host does not have is dropped, not emitted blind. - if stringSliceContains(paths, filepath.Join(home, ".azure")) { - t.Errorf("credential deny paths = %#v, must not include the absent ~/.azure", paths) + // A default candidate absent from disk at profile-build time is still + // emitted: a rule installed only for what exists now would miss a store + // created later in a long-lived sandboxed session (e.g. a concurrent + // `zero auth login`, or a token file appearing mid-session). + if !stringSliceContains(paths, filepath.Join(home, ".azure")) { + t.Errorf("credential deny paths = %#v, want the not-yet-existing ~/.azure included", paths) } // An explicit AllowRead entry covering a store is an opt-out. - optedOut := credentialDenyReadPathsIn(home, keyFile, []string{awsDir}) + optedOut := credentialDenyReadPathsIn(options, []string{awsDir, zeroDir}) 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{zeroDir})[0]) { + t.Errorf("credential deny paths = %#v, want AllowRead opt-out to drop %q", optedOut, zeroDir) + } if !stringSliceContains(optedOut, normalizeProfilePaths([]string{keyFile})[0]) { t.Errorf("credential deny paths = %#v, want unrelated entries kept after opt-out", optedOut) } - if got := credentialDenyReadPathsIn(" ", "", nil); len(got) != 0 { + if got := credentialDenyReadPathsIn(credentialPathOptions{}, 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(credentialPathOptions{GoogleCredentials: keyFile}, nil) if !stringSliceContains(homeless, normalizeProfilePaths([]string{keyFile})[0]) { t.Errorf("credential deny paths without home = %#v, want key file included", homeless) } } + +// TestCredentialDenyReadPathsInOverrideMatchesStoreResolution reproduces the +// audit finding that a relative-and-tilde ZERO_OAUTH_TOKENS_PATH / +// ZERO_MCP_OAUTH_TOKENS_PATH override produced a deny rule for a DIFFERENT +// path than the one the token stores actually resolve (oauth.ResolveStorePath +// / mcp.ResolveTokenStorePath never expand "~"; they resolve a relative +// override literally against the working directory), leaving the real file +// unprotected. +func TestCredentialDenyReadPathsInOverrideMatchesStoreResolution(t *testing.T) { + override := "~/relative-tilde-tokens.json" + options := credentialPathOptions{OAuthTokens: override, MCPOAuthTokens: override} + paths := credentialDenyReadPathsIn(options, nil) + + cwd, err := os.Getwd() + if err != nil { + t.Fatal(err) + } + storeResolved := filepath.Clean(filepath.Join(cwd, override)) + if !stringSliceContains(paths, storeResolved) { + t.Errorf("credential deny paths = %#v, want the store's literal resolution %q included", paths, storeResolved) + } + + home, err := os.UserHomeDir() + if err == nil { + tildeExpanded := normalizeProfilePaths([]string{override})[0] + if tildeExpanded != storeResolved && stringSliceContains(paths, tildeExpanded) { + t.Errorf("credential deny paths = %#v, must not deny the tilde-expanded %q instead of what the store resolves to (home %q)", paths, tildeExpanded, home) + } + } +} + +func TestPermissionProfileDeniesZeroCredentialFiles(t *testing.T) { + if runtime.GOOS == "windows" { + t.Skip("Windows credential deny-read is tracked separately") + } + configHome := t.TempDir() + t.Setenv("XDG_CONFIG_HOME", configHome) + zeroDir := filepath.Join(configHome, "zero") + + // Build the profile BEFORE the store directory exists on disk: a + // sandboxed command launched early in a session must still deny reads of + // credentials created later, not just ones present at profile-build time. + profile := PermissionProfileFromPolicy(t.TempDir(), DefaultPolicy(), nil) + want := normalizeProfilePaths([]string{zeroDir})[0] + if !stringSliceContains(profile.FileSystem.DenyRead, want) { + t.Fatalf("DenyRead = %#v, want Zero config directory %q even before it exists", profile.FileSystem.DenyRead, want) + } + + if err := os.MkdirAll(zeroDir, 0o700); err != nil { + t.Fatal(err) + } + secret := filepath.Join(zeroDir, "oauth-tokens.json") + if err := os.WriteFile(secret, []byte("{}"), 0o600); err != nil { + t.Fatal(err) + } + migrated := filepath.Join(zeroDir, "mcp-oauth-tokens.json.migrated") + if err := os.WriteFile(migrated, []byte("{}"), 0o600); err != nil { + t.Fatal(err) + } + + // Re-derive after the files exist: the same directory rule covers both + // the known store filename and the never-itemized migrated backup. + profile = PermissionProfileFromPolicy(t.TempDir(), DefaultPolicy(), nil) + if !stringSliceContains(profile.FileSystem.DenyRead, want) { + t.Fatalf("DenyRead = %#v, want Zero config directory %q", profile.FileSystem.DenyRead, want) + } +} diff --git a/internal/sandbox/profile.go b/internal/sandbox/profile.go index 19473666..08eacbf6 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: +// cloud credential stores, the file GOOGLE_APPLICATION_CREDENTIALS points to, +// and Zero's own config/credential/token directory so sandboxed commands +// cannot read secrets under the read-all workspace posture. Three 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 @@ -158,6 +159,12 @@ func permissionProfileReadRoots(workspaceRoot string, policy Policy, scope *Scop // 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. +// - Candidates are emitted whether or not they currently exist on disk: a +// rule installed only for stores present at profile-build time would miss +// a store created later in a long-lived sandboxed session (e.g. `zero +// auth login` run concurrently), and every backend already treats a deny +// rule over a not-yet-existing path as a harmless no-op that still takes +// effect once the path appears. // // These are profile-level rules only; they are intentionally NOT merged into // Policy.DenyRead, whose emptiness gates escalated (unsandboxed) execution and @@ -166,33 +173,60 @@ func credentialDenyReadPaths(policy Policy) []string { if runtime.GOOS == "windows" { return nil } - // A failed home lookup only drops the home-based candidates; the - // GOOGLE_APPLICATION_CREDENTIALS target must be protected regardless. + // Failed home/config lookups only drop their derived candidates; explicit + // credential-file overrides are still submitted as candidates (and, like + // every candidate, filtered by on-disk existence below). home, _ := os.UserHomeDir() - return credentialDenyReadPathsIn(home, os.Getenv("GOOGLE_APPLICATION_CREDENTIALS"), policy.AllowRead) + configDir, _ := zeroUserConfigDir() + return credentialDenyReadPathsIn(credentialPathOptions{ + Home: home, + GoogleCredentials: os.Getenv("GOOGLE_APPLICATION_CREDENTIALS"), + ZeroConfigDir: configDir, + OAuthTokens: os.Getenv("ZERO_OAUTH_TOKENS_PATH"), + MCPOAuthTokens: os.Getenv("ZERO_MCP_OAUTH_TOKENS_PATH"), + }, policy.AllowRead) +} + +type credentialPathOptions struct { + Home string + GoogleCredentials string + ZeroConfigDir string + OAuthTokens string + MCPOAuthTokens string } // 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(options credentialPathOptions, allowRead []string) []string { var candidates []string - if home = strings.TrimSpace(home); home != "" { + if home := strings.TrimSpace(options.Home); home != "" { candidates = append(candidates, filepath.Join(home, ".aws"), filepath.Join(home, ".config", "gcloud"), filepath.Join(home, ".azure"), ) } - if target := strings.TrimSpace(googleCredentials); target != "" { + if target := strings.TrimSpace(options.GoogleCredentials); target != "" { candidates = append(candidates, target) } + if configDir := strings.TrimSpace(options.ZeroConfigDir); configDir != "" { + // Deny the whole directory rather than an itemized file list. Zero's + // credential/token/config stores each publish through a randomly-named + // sibling before an atomic rename (oauth-tokens.json.tmp--, + // credentials.{enc,json}.*.tmp, *.secret.*.tmp, .zero-config-*.tmp), and + // the legacy MCP token store leaves a mcp-oauth-tokens.json.migrated + // backup behind after importing it — an itemized list can never keep up + // with those names. Nothing else has a legitimate reason to live here. + candidates = append(candidates, filepath.Join(configDir, "zero")) + } + for _, override := range []string{options.OAuthTokens, options.MCPOAuthTokens} { + if tokenPath := resolveCredentialOverridePath(override); tokenPath != "" { + candidates = append(candidates, tokenPath, tokenPath+".secret") + } + } allowRoots := normalizeProfilePaths(allowRead) out := make([]string, 0, len(candidates)) for _, path := range normalizeProfilePaths(candidates) { - // Only stores that actually exist on this host need a deny rule. - if _, err := os.Stat(path); err != nil { - continue - } reincluded := false for _, allow := range allowRoots { if pathWithinRoot(allow, path) { @@ -207,6 +241,50 @@ func credentialDenyReadPathsIn(home string, googleCredentials string, allowRead return out } +// resolveCredentialOverridePath mirrors the token stores' own override +// resolution (oauth.ResolveStorePath, mcp.ResolveTokenStorePath — duplicated +// here rather than imported, the same tradeoff zeroUserConfigDir makes, +// because internal/mcp depends on this package): a relative override is +// resolved literally against the process working directory, NOT tilde- +// expanded the way normalizeProfilePath expands other candidates. Using +// normalizeProfilePath here would derive a deny path that doesn't match +// where the store actually writes — e.g. ZERO_OAUTH_TOKENS_PATH=~/x resolves +// to /~/x on disk (the store never expands "~"), but normalizeProfilePath +// would deny $HOME/x instead, leaving the real file unprotected. +func resolveCredentialOverridePath(override string) string { + override = strings.TrimSpace(override) + if override == "" { + return "" + } + if filepath.IsAbs(override) { + return filepath.Clean(override) + } + abs, err := filepath.Abs(override) + if err != nil { + return "" + } + return filepath.Clean(abs) +} + +// zeroUserConfigDir mirrors config.UserConfigDir without importing config +// (config depends on sandbox). On macOS Zero deliberately honors +// XDG_CONFIG_HOME when set and falls back to ~/.config, instead of the +// os.UserConfigDir default (~/Library/Application Support); everywhere else +// it uses os.UserConfigDir. +func zeroUserConfigDir() (string, error) { + if runtime.GOOS != "darwin" { + return os.UserConfigDir() + } + if xdg := strings.TrimSpace(os.Getenv("XDG_CONFIG_HOME")); xdg != "" { + return xdg, nil + } + home, err := os.UserHomeDir() + if err != nil { + return "", err + } + return filepath.Join(home, ".config"), nil +} + // userGitConfigReadPaths returns the user's global git config FILES so a // sandboxed git can read identity and config (user.name/email, aliases) instead // of failing with "unable to access ~/.gitconfig". It is deliberately the config