From 1d46ee9519431b41ed96c3d2463c3ed26d89ce54 Mon Sep 17 00:00:00 2001 From: cairn-intern Date: Fri, 28 Aug 2026 02:02:28 +0000 Subject: [PATCH 01/14] fix(sandbox): deny SSH private keys and the GPG keyring #816 closed the git credential half of #815. Linux still allowed a sandboxed command to read ~/.ssh/id_* and ~/.gnupg. Deny that key material (not the whole of ~/.ssh) and IdentityFile paths from ssh config so git host resolution still works. Fixes Gitlawb/zero#815 --- internal/sandbox/git_credential_deny_test.go | 10 +- internal/sandbox/profile.go | 18 +- internal/sandbox/ssh_gpg_deny_test.go | 170 +++++++++++ internal/sandbox/ssh_key_deny.go | 296 +++++++++++++++++++ 4 files changed, 481 insertions(+), 13 deletions(-) create mode 100644 internal/sandbox/ssh_gpg_deny_test.go create mode 100644 internal/sandbox/ssh_key_deny.go diff --git a/internal/sandbox/git_credential_deny_test.go b/internal/sandbox/git_credential_deny_test.go index 7296a295d..6bdb9b3b4 100644 --- a/internal/sandbox/git_credential_deny_test.go +++ b/internal/sandbox/git_credential_deny_test.go @@ -16,12 +16,10 @@ import ( // git's credential store holds host passwords and personal access tokens in // cleartext, in one of two locations depending on whether the user is on the // XDG layout. Neither was denied, so a sandboxed command could read them -// (#815). -// -// Scoped to the credential files on purpose. Denying ~/.ssh as well would stop -// a sandboxed git push over SSH from working, which is a functional trade that -// issue tracks separately; these two cost nothing, because git reads them for -// authentication rather than identity. +// (#815). #816 closed this half: the stores are denied as files, not the +// surrounding git config directory. SSH private keys and the GPG keyring are +// the remaining #815 scope and are covered in ssh_gpg_deny_test.go (key +// material, not the whole of ~/.ssh). func TestCredentialDenyReadPathsCoversGitCredentialStores(t *testing.T) { home := t.TempDir() configHome := filepath.Join(home, ".config") diff --git a/internal/sandbox/profile.go b/internal/sandbox/profile.go index 349e2b1c6..55d79785b 100644 --- a/internal/sandbox/profile.go +++ b/internal/sandbox/profile.go @@ -507,18 +507,22 @@ func credentialDenyReadPathsIn(options credentialPathOptions, allowRead []string homeDirs := []string{ filepath.Join(home, ".aws"), filepath.Join(home, ".azure"), + // GPG secret keyring (secring.gpg, private-keys-v1.d). Directory- + // shaped like ~/.aws so a mount-based backend masks the whole + // store, including files created later in the session (#815). + filepath.Join(home, ".gnupg"), } candidates = append(candidates, homeDirs...) dirs = append(dirs, homeDirs...) // git's credential store backend, which holds host passwords and - // personal access tokens in cleartext. Denied rather than the whole - // of ~/.ssh, because these cost nothing functionally: git reads them - // through a credential helper for authentication, not for identity, - // so a sandboxed git still works and simply cannot authenticate as - // the user. SSH key material is a harder trade and is tracked - // separately (#815). A file, so it joins candidates only — dirs - // drives directory-shaped handling (bwrap binds, carveouts). + // personal access tokens in cleartext (#816). A file, so it joins + // candidates only — dirs drives directory-shaped handling (bwrap + // binds, carveouts). SSH private keys are denied separately as key + // material (id_*, *.pem, IdentityFile paths) rather than the whole + // of ~/.ssh, so config and known_hosts stay readable for git host + // resolution (#815). candidates = append(candidates, filepath.Join(home, ".git-credentials")) + candidates = append(candidates, sshPrivateKeyDenyCandidates(home)...) } candidates = append(candidates, options.GoogleCredentials...) candidates = append(candidates, options.NPMUserConfigs...) diff --git a/internal/sandbox/ssh_gpg_deny_test.go b/internal/sandbox/ssh_gpg_deny_test.go new file mode 100644 index 000000000..d95b13f3d --- /dev/null +++ b/internal/sandbox/ssh_gpg_deny_test.go @@ -0,0 +1,170 @@ +package sandbox + +import ( + "os" + "path/filepath" + "testing" +) + +func denyCovered(denied []string, target string) bool { + norm := normalizeProfilePath(target) + for _, entry := range denied { + if entry == norm || pathWithinRoot(entry, norm) { + return true + } + } + return false +} + +func mustWriteFile(t *testing.T, path, content string) { + t.Helper() + if err := os.MkdirAll(filepath.Dir(path), 0o700); err != nil { + t.Fatal(err) + } + if err := os.WriteFile(path, []byte(content), 0o600); err != nil { + t.Fatal(err) + } +} + +func sshGPGDenied(t *testing.T, home string, allowRead []string) []string { + t.Helper() + return credentialDenyReadPathsIn(credentialPathOptions{ + Homes: []string{home}, + ConfigDirs: []string{filepath.Join(home, ".config")}, + }, allowRead).Paths +} + +// Option 2 of #815: deny SSH private key material and the GPG keyring, not +// the whole of ~/.ssh. git credential files from #816 must stay denied. +func TestCredentialDenyReadPathsDeniesSSHKeyMaterialNotDirectory(t *testing.T) { + home := t.TempDir() + sshDir := filepath.Join(home, ".ssh") + idEd := filepath.Join(sshDir, "id_ed25519") + idPub := filepath.Join(sshDir, "id_ed25519.pub") + config := filepath.Join(sshDir, "config") + knownHosts := filepath.Join(sshDir, "known_hosts") + fooPEM := filepath.Join(sshDir, "foo.pem") + rsaPEM := filepath.Join(sshDir, "id_rsa.pem") + secring := filepath.Join(home, ".gnupg", "secring.gpg") + privateKey := filepath.Join(home, ".gnupg", "private-keys-v1.d", "keygrip.key") + gitCredentials := filepath.Join(home, ".git-credentials") + xdgCredentials := filepath.Join(home, ".config", "git", "credentials") + + mustWriteFile(t, idEd, "-----BEGIN OPENSSH PRIVATE KEY-----\nx\n-----END OPENSSH PRIVATE KEY-----\n") + mustWriteFile(t, idPub, "ssh-ed25519 AAAA public\n") + mustWriteFile(t, config, "Host *\n") + mustWriteFile(t, knownHosts, "github.com ssh-ed25519 AAAA\n") + mustWriteFile(t, fooPEM, "-----BEGIN PRIVATE KEY-----\nx\n-----END PRIVATE KEY-----\n") + mustWriteFile(t, rsaPEM, "-----BEGIN RSA PRIVATE KEY-----\nx\n-----END RSA PRIVATE KEY-----\n") + mustWriteFile(t, secring, "fake-secring") + mustWriteFile(t, privateKey, "fake-keygrip") + mustWriteFile(t, gitCredentials, "https://user:token@github.com") + mustWriteFile(t, xdgCredentials, "https://user:token@github.com") + + denied := sshGPGDenied(t, home, nil) + + if !denyCovered(denied, idEd) { + t.Fatalf("~/.ssh/id_ed25519 is readable; deny list = %v", denied) + } + if denyCovered(denied, idPub) { + t.Fatalf("~/.ssh/id_ed25519.pub was denied; public keys must stay readable") + } + if denyCovered(denied, config) { + t.Fatalf("~/.ssh/config was denied; git host resolution would break") + } + if denyCovered(denied, knownHosts) { + t.Fatalf("~/.ssh/known_hosts was denied; git host resolution would break") + } + if denyCovered(denied, sshDir) { + t.Fatalf("~/.ssh was denied wholesale; option 2 keeps the directory readable") + } + if !denyCovered(denied, fooPEM) { + t.Fatalf("~/.ssh/foo.pem is readable; deny list = %v", denied) + } + if !denyCovered(denied, rsaPEM) { + t.Fatalf("~/.ssh/id_rsa.pem is readable; deny list = %v", denied) + } + if !denyCovered(denied, secring) { + t.Fatalf("~/.gnupg/secring.gpg is readable; deny list = %v", denied) + } + if !denyCovered(denied, privateKey) { + t.Fatalf("~/.gnupg/private-keys-v1.d file is readable; deny list = %v", denied) + } + if !denyCovered(denied, gitCredentials) { + t.Fatalf("~/.git-credentials is readable after #815 SSH work; deny list = %v", denied) + } + if !denyCovered(denied, xdgCredentials) { + t.Fatalf("~/.config/git/credentials is readable after #815 SSH work; deny list = %v", denied) + } +} + +func TestCredentialDenyReadPathsDeniesSSHConfigIdentityFileOutsideSSH(t *testing.T) { + home := t.TempDir() + sshDir := filepath.Join(home, ".ssh") + workKey := filepath.Join(home, "keys", "work_ed25519") + mustWriteFile(t, workKey, "-----BEGIN OPENSSH PRIVATE KEY-----\nx\n-----END OPENSSH PRIVATE KEY-----\n") + mustWriteFile(t, workKey+".pub", "ssh-ed25519 AAAA work\n") + mustWriteFile(t, filepath.Join(sshDir, "config"), `Host work + IdentityFile ~/keys/work_ed25519 + CertificateFile ~/keys/work_ed25519.pub + UserKnownHostsFile ~/.ssh/known_hosts +`) + mustWriteFile(t, filepath.Join(sshDir, "known_hosts"), "example.com ssh-ed25519 AAAA\n") + + denied := sshGPGDenied(t, home, nil) + if !denyCovered(denied, workKey) { + t.Fatalf("IdentityFile ~/keys/work_ed25519 is readable; deny list = %v", denied) + } + if denyCovered(denied, filepath.Join(sshDir, "known_hosts")) { + t.Fatalf("known_hosts was denied because a path-valued directive pointed at it") + } + if denyCovered(denied, workKey+".pub") { + t.Fatalf("CertificateFile *.pub was denied; option 2 keeps public keys readable") + } + if denyCovered(denied, filepath.Join(sshDir, "config")) { + t.Fatalf("~/.ssh/config was denied") + } +} + +func TestCredentialDenyReadPathsFollowsSSHConfigIncludeAndStopsCycles(t *testing.T) { + home := t.TempDir() + sshDir := filepath.Join(home, ".ssh") + includedKey := filepath.Join(home, "keys", "included_ed25519") + cycleKey := filepath.Join(home, "keys", "cycle_ed25519") + mustWriteFile(t, includedKey, "-----BEGIN OPENSSH PRIVATE KEY-----\nx\n-----END OPENSSH PRIVATE KEY-----\n") + mustWriteFile(t, cycleKey, "-----BEGIN OPENSSH PRIVATE KEY-----\nx\n-----END OPENSSH PRIVATE KEY-----\n") + mustWriteFile(t, filepath.Join(sshDir, "config"), "Include extra_config\nInclude cycle_a\nInclude missing_include\n") + mustWriteFile(t, filepath.Join(sshDir, "extra_config"), "IdentityFile ~/keys/included_ed25519\n") + mustWriteFile(t, filepath.Join(sshDir, "cycle_a"), "Include cycle_b\n") + mustWriteFile(t, filepath.Join(sshDir, "cycle_b"), "Include cycle_a\nIdentityFile ~/keys/cycle_ed25519\n") + + denied := sshGPGDenied(t, home, nil) + if !denyCovered(denied, includedKey) { + t.Fatalf("Include IdentityFile is readable; deny list = %v", denied) + } + if !denyCovered(denied, cycleKey) { + t.Fatalf("cyclic Include IdentityFile is readable; deny list = %v", denied) + } +} + +func TestSSHKeyDenyYieldsToExplicitAllowRead(t *testing.T) { + home := t.TempDir() + idEd := filepath.Join(home, ".ssh", "id_ed25519") + mustWriteFile(t, idEd, "-----BEGIN OPENSSH PRIVATE KEY-----\nx\n-----END OPENSSH PRIVATE KEY-----\n") + target := normalizeProfilePath(idEd) + listed := func(entries []string) bool { + for _, entry := range entries { + if entry == target { + return true + } + } + return false + } + + if !listed(sshGPGDenied(t, home, nil)) { + t.Fatalf("~/.ssh/id_ed25519 is not denied without an allowRead; nothing for the grant to override") + } + if listed(sshGPGDenied(t, home, []string{home})) { + t.Fatalf("explicit allowRead %q did not re-include the SSH private key", home) + } +} diff --git a/internal/sandbox/ssh_key_deny.go b/internal/sandbox/ssh_key_deny.go new file mode 100644 index 000000000..f6bce2a49 --- /dev/null +++ b/internal/sandbox/ssh_key_deny.go @@ -0,0 +1,296 @@ +package sandbox + +import ( + "os" + "path/filepath" + "strings" +) + +// sshConfigMaxIncludeDepth bounds Include recursion. Unreadable or cyclic +// includes are skipped rather than failing the profile build. +const sshConfigMaxIncludeDepth = 16 + +const sshConfigMaxBytes = 1 << 20 + +const sshIncludeMatchCap = 64 + +// sshWellKnownPrivateKeyNames are the OpenSSH default private-key basenames. +// They are emitted even when ~/.ssh is absent so pathname-policy backends can +// reserve them; mount-based Linux still masks only paths that exist. +var sshWellKnownPrivateKeyNames = []string{ + "id_rsa", + "id_dsa", + "id_ecdsa", + "id_ed25519", + "id_ecdsa_sk", + "id_ed25519_sk", +} + +// sshPathValuedDirectives are ssh_config keywords whose values name files or +// sockets. IdentityFile is the important one for relocated keys; the rest are +// collected so a CertificateFile or RevokedHostKeys path outside ~/.ssh is not +// left readable. UserKnownHostsFile / GlobalKnownHostsFile values that resolve +// to known_hosts are dropped later so option 2 keeps host resolution working. +var sshPathValuedDirectives = map[string]bool{ + "certificatefile": true, + "controlpath": true, + "globalknownhostsfile": true, + "identityagent": true, + "identityfile": true, + "revokedhostkeys": true, + "userknownhostsfile": true, +} + +// sshPrivateKeyDenyCandidates returns deny-read candidates for SSH private key +// material under home. ~/.ssh itself is not denied: config, known_hosts, and +// *.pub stay readable so git host resolution still works. Keys named outside +// ~/.ssh are discovered by parsing ~/.ssh/config (and Include) for IdentityFile +// and the other path-valued directives. +func sshPrivateKeyDenyCandidates(home string) []string { + home = strings.TrimSpace(home) + if home == "" { + return nil + } + sshDir := filepath.Join(home, ".ssh") + var candidates []string + for _, name := range sshWellKnownPrivateKeyNames { + candidates = append(candidates, filepath.Join(sshDir, name)) + } + entries, err := os.ReadDir(sshDir) + if err == nil { + for _, entry := range entries { + if entry.IsDir() { + continue + } + name := entry.Name() + path := filepath.Join(sshDir, name) + if isSSHPrivateKeyFileName(name) || sshFileLooksLikePrivateKey(path) { + candidates = append(candidates, path) + } + } + } + candidates = append(candidates, sshConfigReferencedPaths(home, sshDir)...) + return candidates +} + +func isSSHPrivateKeyFileName(name string) bool { + if sshPublicOrConfigName(name) { + return false + } + if strings.HasPrefix(name, "id_") { + return true + } + return strings.HasSuffix(strings.ToLower(name), ".pem") +} + +func sshPublicOrConfigName(name string) bool { + switch name { + case "config", "known_hosts", "known_hosts.old", "authorized_keys", "authorized_keys2": + return true + } + return strings.HasSuffix(name, ".pub") +} + +func sshFileLooksLikePrivateKey(path string) bool { + if sshPublicOrConfigName(filepath.Base(path)) { + return false + } + f, err := os.Open(path) + if err != nil { + return false + } + defer f.Close() + buf := make([]byte, 128) + n, err := f.Read(buf) + if n == 0 && err != nil { + return false + } + s := strings.TrimSpace(string(buf[:n])) + if !strings.HasPrefix(s, "-----BEGIN ") { + return false + } + return strings.Contains(s, "PRIVATE KEY") +} + +func sshConfigReferencedPaths(home, sshDir string) []string { + return collectSSHConfigPaths(filepath.Join(sshDir, "config"), home, sshDir, make(map[string]bool), 0) +} + +func collectSSHConfigPaths(path, home, sshDir string, seen map[string]bool, depth int) []string { + if depth > sshConfigMaxIncludeDepth { + return nil + } + identity := sshConfigIdentity(path) + if identity == "" || seen[identity] { + return nil + } + seen[identity] = true + + data, err := os.ReadFile(path) + if err != nil { + return nil + } + if len(data) > sshConfigMaxBytes { + data = data[:sshConfigMaxBytes] + } + + var out []string + for _, line := range strings.Split(strings.ReplaceAll(string(data), "\r\n", "\n"), "\n") { + key, values := parseSSHDirective(line) + if key == "" || len(values) == 0 { + continue + } + if key == "include" { + for _, pattern := range values { + for _, include := range sshIncludePaths(pattern, home, sshDir) { + out = append(out, collectSSHConfigPaths(include, home, sshDir, seen, depth+1)...) + } + } + continue + } + if !sshPathValuedDirectives[key] { + continue + } + for _, raw := range values { + expanded := expandSSHConfigPath(raw, home, sshDir) + if !sshShouldDenyReferencedPath(expanded, home, sshDir) { + continue + } + out = append(out, expanded) + } + } + return out +} + +func sshConfigIdentity(path string) string { + if n := normalizeProfilePath(path); n != "" { + return n + } + cleaned := filepath.Clean(path) + if cleaned == "." || cleaned == "" { + return "" + } + return cleaned +} + +func sshIncludePaths(pattern, home, sshDir string) []string { + expanded := expandSSHConfigPath(pattern, home, sshDir) + if expanded == "" { + return nil + } + matches, err := filepath.Glob(expanded) + if err != nil || len(matches) == 0 { + return nil + } + if len(matches) > sshIncludeMatchCap { + matches = matches[:sshIncludeMatchCap] + } + return matches +} + +func parseSSHDirective(line string) (string, []string) { + line = strings.TrimSpace(line) + if line == "" || strings.HasPrefix(line, "#") { + return "", nil + } + tokens := splitSSHTokens(line) + if len(tokens) == 0 { + return "", nil + } + first := tokens[0] + rest := tokens[1:] + if i := strings.IndexByte(first, '='); i > 0 { + rest = append([]string{first[i+1:]}, rest...) + first = first[:i] + if rest[0] == "" { + rest = rest[1:] + } + } + key := strings.ToLower(first) + if key == "" || len(rest) == 0 { + return "", nil + } + return key, rest +} + +func splitSSHTokens(s string) []string { + var out []string + var cur strings.Builder + inQuote := byte(0) + flush := func() { + if cur.Len() == 0 { + return + } + out = append(out, cur.String()) + cur.Reset() + } + for i := 0; i < len(s); i++ { + c := s[i] + if inQuote != 0 { + if c == inQuote { + inQuote = 0 + continue + } + if c == '\\' && inQuote == '"' && i+1 < len(s) { + cur.WriteByte(s[i+1]) + i++ + continue + } + cur.WriteByte(c) + continue + } + switch c { + case '\'', '"': + inQuote = c + case ' ', '\t': + flush() + case '#': + flush() + return out + default: + cur.WriteByte(c) + } + } + flush() + return out +} + +func expandSSHConfigPath(value, home, sshDir string) string { + value = strings.TrimSpace(value) + if value == "" || strings.EqualFold(value, "none") || strings.EqualFold(value, "SSH_AUTH_SOCK") { + return "" + } + if strings.Contains(value, "%") { + return "" + } + switch { + case value == "~": + return filepath.Clean(home) + case strings.HasPrefix(value, "~/"): + return filepath.Join(home, value[2:]) + case strings.HasPrefix(value, "~"): + return "" + case filepath.IsAbs(value): + return filepath.Clean(value) + default: + return filepath.Join(sshDir, value) + } +} + +func sshShouldDenyReferencedPath(path, home, sshDir string) bool { + path = strings.TrimSpace(path) + if path == "" { + return false + } + cleaned := filepath.Clean(path) + if cleaned == string(filepath.Separator) { + return false + } + if home != "" && cleaned == filepath.Clean(home) { + return false + } + if sshDir != "" && cleaned == filepath.Clean(sshDir) { + return false + } + return !sshPublicOrConfigName(filepath.Base(cleaned)) +} From e1b5161401788c0dcf3e5e021ccb8e26a85480f8 Mon Sep 17 00:00:00 2001 From: cairn-intern Date: Fri, 28 Aug 2026 02:29:00 +0000 Subject: [PATCH 02/14] fix(sandbox): expand SSH %d and keep lexical credential denies OpenSSH IdentityFile supports %d as the local home; expand that (and %%) before rejecting leftover percent tokens. Keep the lexical candidate path on the deny list alongside any EvalSymlinks target for ~/.gnupg, ~/.git-credentials, and SSH private keys so a same-user symlink retarget cannot drop the deny. Tests cover %d outside ~/.ssh, a Windows-style token fake, and lexical symlink candidates. Do not deny wholesale ~/.ssh. --- internal/sandbox/profile.go | 58 +++++++++- internal/sandbox/ssh_gpg_deny_test.go | 159 ++++++++++++++++++++++++-- internal/sandbox/ssh_key_deny.go | 35 +++++- 3 files changed, 239 insertions(+), 13 deletions(-) diff --git a/internal/sandbox/profile.go b/internal/sandbox/profile.go index 55d79785b..431180f4f 100644 --- a/internal/sandbox/profile.go +++ b/internal/sandbox/profile.go @@ -500,17 +500,20 @@ func credentialDenyReadPathsIn(options credentialPathOptions, allowRead []string var carveouts []string var ensureDirs []string var dirs []string + var lexicalCandidates []string + var lexicalDirs []string for _, home := range options.Homes { if strings.TrimSpace(home) == "" { continue } + gnupg := filepath.Join(home, ".gnupg") homeDirs := []string{ filepath.Join(home, ".aws"), filepath.Join(home, ".azure"), // GPG secret keyring (secring.gpg, private-keys-v1.d). Directory- // shaped like ~/.aws so a mount-based backend masks the whole // store, including files created later in the session (#815). - filepath.Join(home, ".gnupg"), + gnupg, } candidates = append(candidates, homeDirs...) dirs = append(dirs, homeDirs...) @@ -521,8 +524,18 @@ func credentialDenyReadPathsIn(options credentialPathOptions, allowRead []string // material (id_*, *.pem, IdentityFile paths) rather than the whole // of ~/.ssh, so config and known_hosts stay readable for git host // resolution (#815). - candidates = append(candidates, filepath.Join(home, ".git-credentials")) - candidates = append(candidates, sshPrivateKeyDenyCandidates(home)...) + gitCredentials := filepath.Join(home, ".git-credentials") + sshKeys := sshPrivateKeyDenyCandidates(home) + candidates = append(candidates, gitCredentials) + candidates = append(candidates, sshKeys...) + // Keep the lexical candidate as well as any EvalSymlinks target so a + // same-user atomic symlink retarget after profile construction still + // hits a deny on ~/.gnupg, ~/.git-credentials, and SSH private keys. + // Use-time handle-relative / openat enforcement is a pre-existing + // backend gap, not introduced here. + lexicalCandidates = append(lexicalCandidates, gnupg, gitCredentials) + lexicalCandidates = append(lexicalCandidates, sshKeys...) + lexicalDirs = append(lexicalDirs, gnupg) } candidates = append(candidates, options.GoogleCredentials...) candidates = append(candidates, options.NPMUserConfigs...) @@ -600,21 +613,56 @@ func credentialDenyReadPathsIn(options credentialPathOptions, allowRead []string candidates = append(candidates, tokenPath, tokenPath+".migrated") } allowRoots := normalizeProfilePaths(allowRead) - out := make([]string, 0, len(candidates)) + out := make([]string, 0, len(candidates)+len(lexicalCandidates)) for _, path := range normalizeProfilePaths(candidates) { if credentialPathReincluded(allowRoots, path) { continue } out = append(out, path) } + out = appendLexicalCredentialDenyPaths(out, allowRoots, lexicalCandidates) + dirList := normalizeProfilePaths(dirs) + dirList = appendLexicalCredentialDenyPaths(dirList, nil, lexicalDirs) return credentialDenyPaths{ Paths: out, Carveouts: credentialCarveoutPaths(out, carveouts), EnsureDirs: credentialRetainedDirs(out, normalizeProfilePaths(ensureDirs)), - Dirs: credentialRetainedDirs(out, normalizeProfilePaths(dirs)), + Dirs: credentialRetainedDirs(out, dirList), } } +// appendLexicalCredentialDenyPaths adds the pre-EvalSymlinks spelling of each +// candidate. normalizeProfilePath replaces a symlink with its target, so +// omitting the lexical path would let a later atomic retarget of the same +// pathname escape the deny list. +func appendLexicalCredentialDenyPaths(out, allowRoots, candidates []string) []string { + if len(candidates) == 0 { + return out + } + seen := make(map[string]struct{}, len(out)) + for _, path := range out { + seen[path] = struct{}{} + } + for _, path := range candidates { + lexical := normalizeProfilePathLexically(path) + if lexical == "" { + continue + } + if _, ok := seen[lexical]; ok { + continue + } + if credentialPathReincluded(allowRoots, lexical) { + continue + } + if resolved := normalizeProfilePath(path); resolved != "" && credentialPathReincluded(allowRoots, resolved) { + continue + } + seen[lexical] = struct{}{} + out = append(out, lexical) + } + return out +} + // credentialTokenStorePaths returns the deny entries for one token-store path: // the store, its lock siblings, its encryption-key sibling, and the directory // it publishes new contents through. The names are fixed so an override outside diff --git a/internal/sandbox/ssh_gpg_deny_test.go b/internal/sandbox/ssh_gpg_deny_test.go index d95b13f3d..560221019 100644 --- a/internal/sandbox/ssh_gpg_deny_test.go +++ b/internal/sandbox/ssh_gpg_deny_test.go @@ -3,6 +3,7 @@ package sandbox import ( "os" "path/filepath" + "runtime" "testing" ) @@ -16,6 +17,15 @@ func denyCovered(denied []string, target string) bool { return false } +func denyListedExact(denied []string, target string) bool { + for _, entry := range denied { + if entry == target { + return true + } + } + return false +} + func mustWriteFile(t *testing.T, path, content string) { t.Helper() if err := os.MkdirAll(filepath.Dir(path), 0o700); err != nil { @@ -26,6 +36,16 @@ func mustWriteFile(t *testing.T, path, content string) { } } +func mustSymlink(t *testing.T, target, link string) { + t.Helper() + if err := os.MkdirAll(filepath.Dir(link), 0o700); err != nil { + t.Fatal(err) + } + if err := os.Symlink(target, link); err != nil { + t.Fatal(err) + } +} + func sshGPGDenied(t *testing.T, home string, allowRead []string) []string { t.Helper() return credentialDenyReadPathsIn(credentialPathOptions{ @@ -34,6 +54,15 @@ func sshGPGDenied(t *testing.T, home string, allowRead []string) []string { }, allowRead).Paths } +func sshGPGNormalizationHome() (home, sshDir string) { + if runtime.GOOS == "windows" { + home = `C:\Users\zero-sandbox` + } else { + home = "/home/zero-sandbox" + } + return home, filepath.Join(home, ".ssh") +} + // Option 2 of #815: deny SSH private key material and the GPG keyring, not // the whole of ~/.ssh. git credential files from #816 must stay denied. func TestCredentialDenyReadPathsDeniesSSHKeyMaterialNotDirectory(t *testing.T) { @@ -50,12 +79,14 @@ func TestCredentialDenyReadPathsDeniesSSHKeyMaterialNotDirectory(t *testing.T) { gitCredentials := filepath.Join(home, ".git-credentials") xdgCredentials := filepath.Join(home, ".config", "git", "credentials") - mustWriteFile(t, idEd, "-----BEGIN OPENSSH PRIVATE KEY-----\nx\n-----END OPENSSH PRIVATE KEY-----\n") + // Path-based denials (id_*, *.pem, ~/.gnupg, credential stores). Empty or + // obviously-fake bodies so scanners do not treat fixtures as live keys. + mustWriteFile(t, idEd, "") mustWriteFile(t, idPub, "ssh-ed25519 AAAA public\n") mustWriteFile(t, config, "Host *\n") mustWriteFile(t, knownHosts, "github.com ssh-ed25519 AAAA\n") - mustWriteFile(t, fooPEM, "-----BEGIN PRIVATE KEY-----\nx\n-----END PRIVATE KEY-----\n") - mustWriteFile(t, rsaPEM, "-----BEGIN RSA PRIVATE KEY-----\nx\n-----END RSA PRIVATE KEY-----\n") + mustWriteFile(t, fooPEM, "") + mustWriteFile(t, rsaPEM, "") mustWriteFile(t, secring, "fake-secring") mustWriteFile(t, privateKey, "fake-keygrip") mustWriteFile(t, gitCredentials, "https://user:token@github.com") @@ -102,7 +133,7 @@ func TestCredentialDenyReadPathsDeniesSSHConfigIdentityFileOutsideSSH(t *testing home := t.TempDir() sshDir := filepath.Join(home, ".ssh") workKey := filepath.Join(home, "keys", "work_ed25519") - mustWriteFile(t, workKey, "-----BEGIN OPENSSH PRIVATE KEY-----\nx\n-----END OPENSSH PRIVATE KEY-----\n") + mustWriteFile(t, workKey, "") mustWriteFile(t, workKey+".pub", "ssh-ed25519 AAAA work\n") mustWriteFile(t, filepath.Join(sshDir, "config"), `Host work IdentityFile ~/keys/work_ed25519 @@ -126,13 +157,28 @@ func TestCredentialDenyReadPathsDeniesSSHConfigIdentityFileOutsideSSH(t *testing } } +func TestCredentialDenyReadPathsDeniesSSHConfigIdentityFilePercentD(t *testing.T) { + home := t.TempDir() + workKey := filepath.Join(home, "keys", "work_ed25519") + mustWriteFile(t, workKey, "") + mustWriteFile(t, filepath.Join(home, ".ssh", "config"), "IdentityFile %d/keys/work_ed25519\n") + + denied := sshGPGDenied(t, home, nil) + if !denyCovered(denied, workKey) { + t.Fatalf("IdentityFile %%d/keys/work_ed25519 is readable; deny list = %v", denied) + } + if denyCovered(denied, filepath.Join(home, ".ssh")) { + t.Fatalf("~/.ssh was denied wholesale") + } +} + func TestCredentialDenyReadPathsFollowsSSHConfigIncludeAndStopsCycles(t *testing.T) { home := t.TempDir() sshDir := filepath.Join(home, ".ssh") includedKey := filepath.Join(home, "keys", "included_ed25519") cycleKey := filepath.Join(home, "keys", "cycle_ed25519") - mustWriteFile(t, includedKey, "-----BEGIN OPENSSH PRIVATE KEY-----\nx\n-----END OPENSSH PRIVATE KEY-----\n") - mustWriteFile(t, cycleKey, "-----BEGIN OPENSSH PRIVATE KEY-----\nx\n-----END OPENSSH PRIVATE KEY-----\n") + mustWriteFile(t, includedKey, "") + mustWriteFile(t, cycleKey, "") mustWriteFile(t, filepath.Join(sshDir, "config"), "Include extra_config\nInclude cycle_a\nInclude missing_include\n") mustWriteFile(t, filepath.Join(sshDir, "extra_config"), "IdentityFile ~/keys/included_ed25519\n") mustWriteFile(t, filepath.Join(sshDir, "cycle_a"), "Include cycle_b\n") @@ -150,7 +196,7 @@ func TestCredentialDenyReadPathsFollowsSSHConfigIncludeAndStopsCycles(t *testing func TestSSHKeyDenyYieldsToExplicitAllowRead(t *testing.T) { home := t.TempDir() idEd := filepath.Join(home, ".ssh", "id_ed25519") - mustWriteFile(t, idEd, "-----BEGIN OPENSSH PRIVATE KEY-----\nx\n-----END OPENSSH PRIVATE KEY-----\n") + mustWriteFile(t, idEd, "") target := normalizeProfilePath(idEd) listed := func(entries []string) bool { for _, entry := range entries { @@ -168,3 +214,102 @@ func TestSSHKeyDenyYieldsToExplicitAllowRead(t *testing.T) { t.Fatalf("explicit allowRead %q did not re-include the SSH private key", home) } } + +// Path-sensitive SSH/GPG handling needs a non-Linux case (or a hermetic fake +// of the same normalization). Token expansion is GOOS-independent, so a +// Windows home spelling exercises %d without touching the host filesystem. +func TestExpandSSHConfigPathTokensWindowsStyleHome(t *testing.T) { + home := `C:\Users\zero-sandbox` + got, ok := expandSSHConfigPathTokens(`%d\keys\work_ed25519`, home) + if !ok { + t.Fatal("supported %d token was rejected") + } + want := `C:\Users\zero-sandbox\keys\work_ed25519` + if got != want { + t.Fatalf("Windows-style %%d expansion = %q, want %q", got, want) + } + got, ok = expandSSHConfigPathTokens("%d/keys/work_ed25519", home) + if !ok { + t.Fatal("supported %d token with slash was rejected") + } + want = `C:\Users\zero-sandbox/keys/work_ed25519` + if got != want { + t.Fatalf("Windows-style %%d with slash = %q, want %q", got, want) + } + if _, ok := expandSSHConfigPathTokens("%h/keys/work_ed25519", home); ok { + t.Fatal("unsupported %h token must be rejected") + } + got, ok = expandSSHConfigPathTokens("id%%ed25519", home) + if !ok || got != "id%ed25519" { + t.Fatalf("%% -> %% expansion = %q ok=%v, want %q", got, ok, "id%ed25519") + } +} + +func TestExpandSSHConfigPathPercentDUsesSuppliedHome(t *testing.T) { + home, sshDir := sshGPGNormalizationHome() + got := expandSSHConfigPath("%d/keys/work_ed25519", home, sshDir) + want := filepath.Join(home, "keys", "work_ed25519") + if got != want { + t.Fatalf("expandSSHConfigPath(%%d) = %q, want %q", got, want) + } + if expandSSHConfigPath("%h/keys/work_ed25519", home, sshDir) != "" { + t.Fatalf("unsupported %%h token must be dropped") + } + if expandSSHConfigPath("%d/%h/keys", home, sshDir) != "" { + t.Fatalf("remaining unsupported token after %%d must be dropped") + } + got = expandSSHConfigPath("id%%ed25519", home, sshDir) + want = filepath.Join(sshDir, "id%ed25519") + if got != want { + t.Fatalf("literal %% expansion = %q, want %q", got, want) + } + if sshShouldDenyReferencedPath(sshDir, home, sshDir) { + t.Fatalf("~/.ssh was denied wholesale under the fake home") + } + idEd := filepath.Join(sshDir, "id_ed25519") + if !sshShouldDenyReferencedPath(idEd, home, sshDir) { + t.Fatalf("well-known SSH key under fake home was not a deny candidate") + } + gnupg := filepath.Join(home, ".gnupg") + gitCredentials := filepath.Join(home, ".git-credentials") + if filepath.Base(gnupg) != ".gnupg" || filepath.Base(gitCredentials) != ".git-credentials" { + t.Fatalf("GPG/git credential join lost the host separator; gnupg=%q git=%q", gnupg, gitCredentials) + } +} + +func TestCredentialDenyReadPathsKeepsLexicalSymlinkCandidates(t *testing.T) { + home := t.TempDir() + realDir := t.TempDir() + + gnupgLink := filepath.Join(home, ".gnupg") + gnupgTarget := filepath.Join(realDir, "gnupg-store") + if err := os.MkdirAll(gnupgTarget, 0o700); err != nil { + t.Fatal(err) + } + mustSymlink(t, gnupgTarget, gnupgLink) + + gitLink := filepath.Join(home, ".git-credentials") + gitTarget := filepath.Join(realDir, "git-credentials") + mustWriteFile(t, gitTarget, "x") + mustSymlink(t, gitTarget, gitLink) + + sshLink := filepath.Join(home, ".ssh", "id_ed25519") + sshTarget := filepath.Join(realDir, "id_ed25519") + mustWriteFile(t, sshTarget, "") + mustSymlink(t, sshTarget, sshLink) + + denied := sshGPGDenied(t, home, nil) + for _, candidate := range []string{gnupgLink, gitLink, sshLink} { + lexical := normalizeProfilePathLexically(candidate) + if !denyListedExact(denied, lexical) { + t.Fatalf("lexical candidate %q missing from deny list %v", lexical, denied) + } + resolved := normalizeProfilePath(candidate) + if resolved != "" && resolved != lexical && !denyListedExact(denied, resolved) { + t.Fatalf("resolved target %q missing from deny list %v", resolved, denied) + } + } + if denyCovered(denied, filepath.Join(home, ".ssh")) { + t.Fatalf("~/.ssh was denied wholesale") + } +} diff --git a/internal/sandbox/ssh_key_deny.go b/internal/sandbox/ssh_key_deny.go index f6bce2a49..2722a6a17 100644 --- a/internal/sandbox/ssh_key_deny.go +++ b/internal/sandbox/ssh_key_deny.go @@ -260,9 +260,11 @@ func expandSSHConfigPath(value, home, sshDir string) string { if value == "" || strings.EqualFold(value, "none") || strings.EqualFold(value, "SSH_AUTH_SOCK") { return "" } - if strings.Contains(value, "%") { + expanded, ok := expandSSHConfigPathTokens(value, home) + if !ok { return "" } + value = expanded switch { case value == "~": return filepath.Clean(home) @@ -277,6 +279,37 @@ func expandSSHConfigPath(value, home, sshDir string) string { } } +// expandSSHConfigPathTokens resolves OpenSSH path tokens we can expand without +// a live connection: %d is the supplied local home, %% is a literal %. Any +// remaining percent token (%h, a trailing %, ...) is unsupported and the path +// is dropped so we never deny (or follow) an unresolved pattern. +func expandSSHConfigPathTokens(value, home string) (string, bool) { + if !strings.Contains(value, "%") { + return value, true + } + var b strings.Builder + b.Grow(len(value) + len(home)) + for i := 0; i < len(value); i++ { + if value[i] != '%' { + b.WriteByte(value[i]) + continue + } + if i+1 >= len(value) { + return "", false + } + switch value[i+1] { + case '%': + b.WriteByte('%') + case 'd': + b.WriteString(home) + default: + return "", false + } + i++ + } + return b.String(), true +} + func sshShouldDenyReferencedPath(path, home, sshDir string) bool { path = strings.TrimSpace(path) if path == "" { From eea3fdecbb120fc63bc2abf4d6420a614dc4022b Mon Sep 17 00:00:00 2001 From: cairn-intern Date: Fri, 28 Aug 2026 05:36:19 +0000 Subject: [PATCH 03/14] fix(sandbox): keep lexical credential denies through bwrap and Seatbelt Carry symlink lexical identity into the final bwrap dest and Seatbelt rules so a later retarget of ~/.git-credentials, ~/.gnupg, or an SSH key cannot drop the mask. Overlap and user-deny coverage compare canonical paths so lexical /var candidates do not survive a /private/var root or turn a command HOME into a missing CommandDenyReadDirs refusal. Walk ~/.ssh recursively for nested key material (depth-capped, no dir symlink follow). Lstat and LimitReader so FIFOs, devices, and oversized configs cannot hang profile construction. Escape t.Fatal %d for vet. Do not deny wholesale ~/.ssh. --- internal/sandbox/linux_helper.go | 20 +++- internal/sandbox/profile.go | 80 ++++++++++++- internal/sandbox/runner.go | 2 +- internal/sandbox/ssh_gpg_deny_test.go | 132 ++++++++++++++++++++- internal/sandbox/ssh_gpg_deny_unix_test.go | 53 +++++++++ internal/sandbox/ssh_key_deny.go | 107 +++++++++++++---- 6 files changed, 362 insertions(+), 32 deletions(-) create mode 100644 internal/sandbox/ssh_gpg_deny_unix_test.go diff --git a/internal/sandbox/linux_helper.go b/internal/sandbox/linux_helper.go index f3ea5c457..ea8e52711 100644 --- a/internal/sandbox/linux_helper.go +++ b/internal/sandbox/linux_helper.go @@ -311,14 +311,16 @@ func buildLinuxBwrapFilesystemPlan(profile PermissionProfile) linuxBwrapFilesyst // never cause host filesystem mutations before sandbox launch. ensureLinuxDenyReadDirs(fs.EnsureDenyReadDirs) for _, path := range fs.DenyReadIfExists { - if !pathExists(path) { + if !pathExists(path) && !pathExistsNoFollow(path) { // A baseline credential path is emitted for every run, so an absent // entry is the common case on a fresh machine — a third-party store // such as ~/.aws that Zero must not create. The read-all profile starts // from a read-only host-root bind where bubblewrap cannot create a // missing mount destination, and masking the nearest existing parent // could hide HOME, /tmp, or the workspace. Path-based backends - // (seatbelt) still deny these paths before they exist. + // (seatbelt) still deny these paths before they exist. A dangling + // symlink still exists as a pathname and must be masked so a later + // retarget cannot reopen it. continue } args = appendUnreadableLinuxPathArgs(args, path, fs.DenyReadCarveouts) @@ -398,11 +400,13 @@ func appendReadOnlyLinuxPathArgs(args []string, path string) []string { } func appendUnreadableLinuxPathArgs(args []string, path string, carveouts []string) []string { - path = normalizeProfilePath(path) + path = unreadableEnforcementPath(path) if path == "" { return args } - if info, err := os.Stat(path); err == nil && !info.IsDir() { + // Lstat so a credential symlink is masked at its lexical pathname rather + // than following to a dest that a later retarget would miss. + if info, err := os.Lstat(path); err == nil && !info.IsDir() { return append(args, "--ro-bind", "/dev/null", path) } nested := nestedCarveoutPaths(path, carveouts) @@ -471,6 +475,14 @@ func pathExists(path string) bool { return err == nil } +func pathExistsNoFollow(path string) bool { + if strings.TrimSpace(path) == "" { + return false + } + _, err := os.Lstat(path) + return err == nil +} + func findLinuxSandboxHelperCommand() (LinuxSandboxHelperCommand, error) { if exe, err := os.Executable(); err == nil { candidate := filepath.Join(filepath.Dir(exe), LinuxSandboxHelperName) diff --git a/internal/sandbox/profile.go b/internal/sandbox/profile.go index 431180f4f..2b171a698 100644 --- a/internal/sandbox/profile.go +++ b/internal/sandbox/profile.go @@ -712,7 +712,7 @@ func pathsOutsideRoots(paths []string, roots []string) []string { } out := make([]string, 0, len(paths)) for _, path := range paths { - if credentialPathReincluded(roots, path) { + if credentialPathCoveredByCanonicalRoots(roots, path) { continue } out = append(out, path) @@ -731,7 +731,7 @@ func pathsOutsideOverlappingRoots(paths []string, roots []string) []string { for _, path := range paths { overlaps := false for _, root := range roots { - if pathWithinRoot(root, path) || pathWithinRoot(path, root) { + if pathWithinRootCanonical(root, path) || pathWithinRootCanonical(path, root) { overlaps = true break } @@ -752,6 +752,31 @@ func credentialPathReincluded(allowRoots []string, path string) bool { return false } +// pathWithinRootCanonical compares after EvalSymlinks so a lexical /var/... +// candidate is recognized as lying under a canonical /private/var/... root. +// Overlap and allow checks use this identity; backends emit lexical symlink +// dests separately via unreadableEnforcementPath. +func pathWithinRootCanonical(root, candidate string) bool { + nr := normalizeProfilePath(root) + if nr == "" { + nr = root + } + nc := normalizeProfilePath(candidate) + if nc == "" { + nc = candidate + } + return pathWithinRoot(nr, nc) +} + +func credentialPathCoveredByCanonicalRoots(roots []string, path string) bool { + for _, root := range roots { + if pathWithinRootCanonical(root, path) { + return true + } + } + return false +} + // credentialCarveoutPaths keeps only the carveouts that sit inside a path that // is actually denied, so an AllowRead opt-out that removed the deny does not // leave a stray allow-back rule behind. @@ -1058,6 +1083,57 @@ func normalizeCredentialFinalPath(path string) string { return filepath.Join(parent, filepath.Base(filepath.Clean(path))) } +// unreadableEnforcementPath is the dest a bwrap bind or Seatbelt rule should +// use for path. Symlinks keep their lexical spelling so a later atomic retarget +// still hits the same pathname; other paths keep EvalSymlinks so aliases such +// as macOS /var -> /private/var continue to match existing roots. Overlap and +// allow checks use canonical identity via pathWithinRootCanonical, not this. +func unreadableEnforcementPath(path string) string { + lexical := normalizeProfilePathLexically(path) + if lexical == "" { + return "" + } + if info, err := os.Lstat(lexical); err == nil && info.Mode().Type() == os.ModeSymlink { + return lexical + } + if resolved := normalizeProfilePath(path); resolved != "" { + return resolved + } + return lexical +} + +// unreadableEnforcementPaths preserves lexical symlink identity alongside any +// resolved target so Seatbelt emits both spellings. Non-symlink paths stay +// canonical, matching normalizeProfilePaths. +func unreadableEnforcementPaths(paths []string) []string { + if len(paths) == 0 { + return nil + } + seen := map[string]struct{}{} + out := make([]string, 0, len(paths)*2) + add := func(p string) { + if p == "" { + return + } + if _, ok := seen[p]; ok { + return + } + seen[p] = struct{}{} + out = append(out, p) + } + for _, path := range paths { + lexical := normalizeProfilePathLexically(path) + if lexical == "" { + continue + } + if info, err := os.Lstat(lexical); err == nil && info.Mode().Type() == os.ModeSymlink { + add(lexical) + } + add(normalizeProfilePath(path)) + } + return out +} + // normalizeProfilePathLexically expands and absolutizes a profile path without // resolving symlinks. Credential carveouts use it so their fixed lexical name // can never become an allow rule for a symlink target. diff --git a/internal/sandbox/runner.go b/internal/sandbox/runner.go index 8528e7e82..a225a4387 100644 --- a/internal/sandbox/runner.go +++ b/internal/sandbox/runner.go @@ -900,7 +900,7 @@ func denyWriteRulesFromPaths(paths []string) []string { } func denySeatbeltPathRules(action string, paths []string) []string { - return denySeatbeltNormalizedPathRules(action, normalizeProfilePaths(paths)) + return denySeatbeltNormalizedPathRules(action, unreadableEnforcementPaths(paths)) } func denySeatbeltNormalizedPathRules(action string, paths []string) []string { diff --git a/internal/sandbox/ssh_gpg_deny_test.go b/internal/sandbox/ssh_gpg_deny_test.go index 560221019..00fcea824 100644 --- a/internal/sandbox/ssh_gpg_deny_test.go +++ b/internal/sandbox/ssh_gpg_deny_test.go @@ -4,7 +4,9 @@ import ( "os" "path/filepath" "runtime" + "strings" "testing" + "time" ) func denyCovered(denied []string, target string) bool { @@ -222,7 +224,7 @@ func TestExpandSSHConfigPathTokensWindowsStyleHome(t *testing.T) { home := `C:\Users\zero-sandbox` got, ok := expandSSHConfigPathTokens(`%d\keys\work_ed25519`, home) if !ok { - t.Fatal("supported %d token was rejected") + t.Fatalf("supported %%d token was rejected") } want := `C:\Users\zero-sandbox\keys\work_ed25519` if got != want { @@ -230,14 +232,14 @@ func TestExpandSSHConfigPathTokensWindowsStyleHome(t *testing.T) { } got, ok = expandSSHConfigPathTokens("%d/keys/work_ed25519", home) if !ok { - t.Fatal("supported %d token with slash was rejected") + t.Fatalf("supported %%d token with slash was rejected") } want = `C:\Users\zero-sandbox/keys/work_ed25519` if got != want { t.Fatalf("Windows-style %%d with slash = %q, want %q", got, want) } if _, ok := expandSSHConfigPathTokens("%h/keys/work_ed25519", home); ok { - t.Fatal("unsupported %h token must be rejected") + t.Fatalf("unsupported %%h token must be rejected") } got, ok = expandSSHConfigPathTokens("id%%ed25519", home) if !ok || got != "id%ed25519" { @@ -313,3 +315,127 @@ func TestCredentialDenyReadPathsKeepsLexicalSymlinkCandidates(t *testing.T) { t.Fatalf("~/.ssh was denied wholesale") } } + +func TestCredentialDenyReadPathsDeniesNestedSSHPrivateKeys(t *testing.T) { + home := t.TempDir() + sshDir := filepath.Join(home, ".ssh") + nestedKey := filepath.Join(sshDir, "keys", "work") + nestedID := filepath.Join(sshDir, "work", "id_rsa") + nestedPub := filepath.Join(sshDir, "keys", "work.pub") + nestedConfig := filepath.Join(sshDir, "keys", "config") + nestedKnown := filepath.Join(sshDir, "keys", "known_hosts") + mustWriteFile(t, nestedKey, "-----BEGIN OPENSSH PRIVATE KEY-----\nfixture\n") + mustWriteFile(t, nestedID, "") + mustWriteFile(t, nestedPub, "ssh-ed25519 AAAA nested\n") + mustWriteFile(t, nestedConfig, "Host *\n") + mustWriteFile(t, nestedKnown, "example.com ssh-ed25519 AAAA\n") + + denied := sshGPGDenied(t, home, nil) + if !denyCovered(denied, nestedKey) { + t.Fatalf("~/.ssh/keys/work is readable; deny list = %v", denied) + } + if !denyCovered(denied, nestedID) { + t.Fatalf("~/.ssh/work/id_rsa is readable; deny list = %v", denied) + } + if denyCovered(denied, nestedPub) { + t.Fatalf("nested *.pub was denied; option 2 keeps public keys readable") + } + if denyCovered(denied, nestedConfig) { + t.Fatalf("nested config was denied") + } + if denyCovered(denied, nestedKnown) { + t.Fatalf("nested known_hosts was denied") + } + if denyCovered(denied, sshDir) { + t.Fatalf("~/.ssh was denied wholesale") + } +} + +func TestLinuxBwrapAndSeatbeltKeepLexicalCredentialSymlinkPaths(t *testing.T) { + if runtime.GOOS == "windows" { + t.Skip("symlink creation is not reliably available on Windows CI") + } + home := t.TempDir() + realDir := t.TempDir() + + gnupgLink := filepath.Join(home, ".gnupg") + gnupgTarget := filepath.Join(realDir, "gnupg-store") + if err := os.MkdirAll(gnupgTarget, 0o700); err != nil { + t.Fatal(err) + } + mustSymlink(t, gnupgTarget, gnupgLink) + + gitLink := filepath.Join(home, ".git-credentials") + gitTarget := filepath.Join(realDir, "git-credentials") + mustWriteFile(t, gitTarget, "x") + mustSymlink(t, gitTarget, gitLink) + + sshLink := filepath.Join(home, ".ssh", "id_ed25519") + sshTarget := filepath.Join(realDir, "id_ed25519") + mustWriteFile(t, sshTarget, "") + mustSymlink(t, sshTarget, sshLink) + + denied := sshGPGDenied(t, home, nil) + profile := PermissionProfile{ + FileSystem: FileSystemPolicy{ + Kind: FileSystemRestricted, + ReadRoots: []string{string(filepath.Separator)}, + DenyReadIfExists: denied, + }, + } + args := linuxBwrapFilesystemArgs(profile) + sbpl := strings.Join(denyReadRules(profile.FileSystem), "\n") + for _, candidate := range []string{gnupgLink, gitLink, sshLink} { + lexical := normalizeProfilePathLexically(candidate) + assertArgsContainSequence(t, args, "--ro-bind", "/dev/null", lexical) + if !strings.Contains(sbpl, sandboxProfileString(lexical)) { + t.Fatalf("Seatbelt rules missing lexical pathname %q:\n%s", lexical, sbpl) + } + } + + newGit := filepath.Join(realDir, "other-credentials") + mustWriteFile(t, newGit, "retargeted") + if err := os.Remove(gitLink); err != nil { + t.Fatal(err) + } + mustSymlink(t, newGit, gitLink) + + lexicalGit := normalizeProfilePathLexically(gitLink) + if !argsContainSequence(args, "--ro-bind", "/dev/null", lexicalGit) { + t.Fatalf("pre-retarget bwrap args lost lexical dest %q: %#v", lexicalGit, args) + } + reemitted := linuxBwrapFilesystemArgs(profile) + assertArgsContainSequence(t, reemitted, "--ro-bind", "/dev/null", lexicalGit) + + deniedAfter := sshGPGDenied(t, home, nil) + if !denyListedExact(deniedAfter, lexicalGit) { + t.Fatalf("lexical git-credentials path missing after retarget: %v", deniedAfter) + } + newResolved := normalizeProfilePath(gitLink) + if newResolved != "" && newResolved != lexicalGit && !denyListedExact(deniedAfter, newResolved) { + t.Fatalf("retargeted git-credentials target %q missing from deny list %v", newResolved, deniedAfter) + } + if denyCovered(deniedAfter, filepath.Join(home, ".ssh")) { + t.Fatalf("~/.ssh was denied wholesale") + } +} + +func TestSSHConfigDiscoveryBoundsOversizedConfig(t *testing.T) { + home := t.TempDir() + workKey := filepath.Join(home, "keys", "work_ed25519") + mustWriteFile(t, workKey, "") + padding := strings.Repeat("#", sshConfigMaxBytes+64*1024) + mustWriteFile(t, filepath.Join(home, ".ssh", "config"), "IdentityFile ~/keys/work_ed25519\n"+padding) + + start := time.Now() + denied := sshGPGDenied(t, home, nil) + if elapsed := time.Since(start); elapsed > 2*time.Second { + t.Fatalf("oversized config discovery took %s", elapsed) + } + if !denyCovered(denied, workKey) { + t.Fatalf("IdentityFile in the first 1 MiB of an oversized config is readable; deny list = %v", denied) + } + if denyCovered(denied, filepath.Join(home, ".ssh")) { + t.Fatalf("~/.ssh was denied wholesale") + } +} diff --git a/internal/sandbox/ssh_gpg_deny_unix_test.go b/internal/sandbox/ssh_gpg_deny_unix_test.go new file mode 100644 index 000000000..00f5c1378 --- /dev/null +++ b/internal/sandbox/ssh_gpg_deny_unix_test.go @@ -0,0 +1,53 @@ +//go:build unix + +package sandbox + +import ( + "os" + "path/filepath" + "syscall" + "testing" + "time" +) + +func TestSSHKeyDiscoverySkipsFIFOAndDeviceWithoutBlocking(t *testing.T) { + home := t.TempDir() + sshDir := filepath.Join(home, ".ssh") + if err := os.MkdirAll(sshDir, 0o700); err != nil { + t.Fatal(err) + } + fifoKey := filepath.Join(sshDir, "custom-key") + if err := syscall.Mkfifo(fifoKey, 0o600); err != nil { + t.Fatalf("Mkfifo custom-key: %v", err) + } + fifoConfig := filepath.Join(sshDir, "config") + if err := syscall.Mkfifo(fifoConfig, 0o600); err != nil { + t.Fatalf("Mkfifo config: %v", err) + } + device := filepath.Join(sshDir, "custom-device") + deviceCreated := syscall.Mknod(device, syscall.S_IFCHR|0o600, 0) == nil + + done := make(chan []string, 1) + go func() { + done <- credentialDenyReadPathsIn(credentialPathOptions{ + Homes: []string{home}, + ConfigDirs: []string{filepath.Join(home, ".config")}, + }, nil).Paths + }() + var denied []string + select { + case denied = <-done: + case <-time.After(300 * time.Millisecond): + t.Fatal("SSH/GPG discovery blocked on a FIFO or device") + } + + if denyCovered(denied, fifoKey) { + t.Fatalf("FIFO ~/.ssh/custom-key was denied; special files are not key material: %v", denied) + } + if deviceCreated && denyCovered(denied, device) { + t.Fatalf("device ~/.ssh/custom-device was denied: %v", denied) + } + if denyCovered(denied, sshDir) { + t.Fatalf("~/.ssh was denied wholesale") + } +} diff --git a/internal/sandbox/ssh_key_deny.go b/internal/sandbox/ssh_key_deny.go index 2722a6a17..1e61e46fd 100644 --- a/internal/sandbox/ssh_key_deny.go +++ b/internal/sandbox/ssh_key_deny.go @@ -1,6 +1,7 @@ package sandbox import ( + "io" "os" "path/filepath" "strings" @@ -14,6 +15,15 @@ const sshConfigMaxBytes = 1 << 20 const sshIncludeMatchCap = 64 +// sshPrivateKeyWalkMaxDepth bounds recursive discovery under ~/.ssh. Nested +// directories such as ~/.ssh/keys are walked; directory symlinks are not +// followed, so a cycle cannot hang profile construction. +const sshPrivateKeyWalkMaxDepth = 8 + +const sshPrivateKeyWalkMaxEntries = 256 + +const sshPrivateKeySniffBytes = 128 + // sshWellKnownPrivateKeyNames are the OpenSSH default private-key basenames. // They are emitted even when ~/.ssh is absent so pathname-policy backends can // reserve them; mount-based Linux still masks only paths that exist. @@ -56,21 +66,60 @@ func sshPrivateKeyDenyCandidates(home string) []string { for _, name := range sshWellKnownPrivateKeyNames { candidates = append(candidates, filepath.Join(sshDir, name)) } - entries, err := os.ReadDir(sshDir) - if err == nil { + candidates = append(candidates, walkSSHPrivateKeyFiles(sshDir)...) + candidates = append(candidates, sshConfigReferencedPaths(home, sshDir)...) + return candidates +} + +func walkSSHPrivateKeyFiles(sshDir string) []string { + var out []string + n := 0 + var walk func(dir string, depth int) + walk = func(dir string, depth int) { + if depth > sshPrivateKeyWalkMaxDepth || n >= sshPrivateKeyWalkMaxEntries { + return + } + entries, err := os.ReadDir(dir) + if err != nil { + return + } for _, entry := range entries { - if entry.IsDir() { - continue + if n >= sshPrivateKeyWalkMaxEntries { + return } name := entry.Name() - path := filepath.Join(sshDir, name) + if name == "." || name == ".." { + continue + } + path := filepath.Join(dir, name) + n++ + info, err := os.Lstat(path) + if err != nil { + continue + } + mode := info.Mode() + if mode.Type() == os.ModeSymlink { + // Name-based only: do not follow, so a FIFO or cycle behind + // the link cannot block profile construction. + if isSSHPrivateKeyFileName(name) { + out = append(out, path) + } + continue + } + if info.IsDir() { + walk(path, depth+1) + continue + } + if !mode.IsRegular() { + continue + } if isSSHPrivateKeyFileName(name) || sshFileLooksLikePrivateKey(path) { - candidates = append(candidates, path) + out = append(out, path) } } } - candidates = append(candidates, sshConfigReferencedPaths(home, sshDir)...) - return candidates + walk(sshDir, 0) + return out } func isSSHPrivateKeyFileName(name string) bool { @@ -95,23 +144,40 @@ func sshFileLooksLikePrivateKey(path string) bool { if sshPublicOrConfigName(filepath.Base(path)) { return false } - f, err := os.Open(path) - if err != nil { - return false - } - defer f.Close() - buf := make([]byte, 128) - n, err := f.Read(buf) - if n == 0 && err != nil { + data, ok := readRegularFileBounded(path, sshPrivateKeySniffBytes) + if !ok { return false } - s := strings.TrimSpace(string(buf[:n])) + s := strings.TrimSpace(string(data)) if !strings.HasPrefix(s, "-----BEGIN ") { return false } return strings.Contains(s, "PRIVATE KEY") } +// readRegularFileBounded Lstats first and refuses FIFOs, devices, sockets, +// and symlinks so profile construction cannot block on a special file. The +// subsequent read is capped with LimitReader. +func readRegularFileBounded(path string, maxBytes int) ([]byte, bool) { + if maxBytes <= 0 { + return nil, false + } + info, err := os.Lstat(path) + if err != nil || !info.Mode().IsRegular() { + return nil, false + } + f, err := os.Open(path) + if err != nil { + return nil, false + } + defer f.Close() + data, err := io.ReadAll(io.LimitReader(f, int64(maxBytes))) + if err != nil { + return nil, false + } + return data, true +} + func sshConfigReferencedPaths(home, sshDir string) []string { return collectSSHConfigPaths(filepath.Join(sshDir, "config"), home, sshDir, make(map[string]bool), 0) } @@ -126,13 +192,10 @@ func collectSSHConfigPaths(path, home, sshDir string, seen map[string]bool, dept } seen[identity] = true - data, err := os.ReadFile(path) - if err != nil { + data, ok := readRegularFileBounded(path, sshConfigMaxBytes) + if !ok { return nil } - if len(data) > sshConfigMaxBytes { - data = data[:sshConfigMaxBytes] - } var out []string for _, line := range strings.Split(strings.ReplaceAll(string(data), "\r\n", "\n"), "\n") { From 5774d3384c58889d0be3c84ae33e71f9c3f1f0ef Mon Sep 17 00:00:00 2001 From: cairn-intern Date: Fri, 28 Aug 2026 06:41:53 +0000 Subject: [PATCH 04/14] fix(sandbox): follow SSH config symlinks and keep lexical dir dests OpenSSH reads ~/.ssh/config and Include targets through regular-file symlinks. Follow those to a regular file, then bound-read the resolved path so a FIFO behind the link cannot hang profile construction. Preserve lexical enforcement and Seatbelt paths whenever the lexical spelling differs from EvalSymlinks, including a symlinked ~/.ssh with a regular key inside, so retargeting the directory cannot expose the key. Do not deny wholesale ~/.ssh. --- internal/sandbox/profile.go | 32 ++++---- internal/sandbox/ssh_gpg_deny_test.go | 91 ++++++++++++++++++++++ internal/sandbox/ssh_gpg_deny_unix_test.go | 43 ++++++++++ internal/sandbox/ssh_key_deny.go | 28 +++++-- 4 files changed, 176 insertions(+), 18 deletions(-) diff --git a/internal/sandbox/profile.go b/internal/sandbox/profile.go index 2b171a698..a195ade2c 100644 --- a/internal/sandbox/profile.go +++ b/internal/sandbox/profile.go @@ -1084,27 +1084,32 @@ func normalizeCredentialFinalPath(path string) string { } // unreadableEnforcementPath is the dest a bwrap bind or Seatbelt rule should -// use for path. Symlinks keep their lexical spelling so a later atomic retarget -// still hits the same pathname; other paths keep EvalSymlinks so aliases such -// as macOS /var -> /private/var continue to match existing roots. Overlap and -// allow checks use canonical identity via pathWithinRootCanonical, not this. +// use for path. When the lexical spelling differs from the EvalSymlinks +// target — a leaf symlink or an intermediate directory symlink such as +// ~/.ssh — keep the lexical pathname so a later atomic retarget still hits +// the same dest. Other paths keep EvalSymlinks so aliases such as macOS +// /var -> /private/var continue to match existing roots. Overlap and allow +// checks use canonical identity via pathWithinRootCanonical, not this. func unreadableEnforcementPath(path string) string { lexical := normalizeProfilePathLexically(path) if lexical == "" { return "" } - if info, err := os.Lstat(lexical); err == nil && info.Mode().Type() == os.ModeSymlink { + resolved := normalizeProfilePath(path) + if resolved == "" || resolved == lexical { + if resolved != "" { + return resolved + } return lexical } - if resolved := normalizeProfilePath(path); resolved != "" { - return resolved - } return lexical } -// unreadableEnforcementPaths preserves lexical symlink identity alongside any -// resolved target so Seatbelt emits both spellings. Non-symlink paths stay -// canonical, matching normalizeProfilePaths. +// unreadableEnforcementPaths preserves lexical identity whenever it differs +// from the EvalSymlinks target, including intermediate directory symlinks +// (for example ~/.ssh -> elsewhere with a regular key file inside). A later +// retarget of that directory would otherwise expose the key through the +// original pathname. Non-symlink paths stay canonical. func unreadableEnforcementPaths(paths []string) []string { if len(paths) == 0 { return nil @@ -1126,10 +1131,11 @@ func unreadableEnforcementPaths(paths []string) []string { if lexical == "" { continue } - if info, err := os.Lstat(lexical); err == nil && info.Mode().Type() == os.ModeSymlink { + canonical := normalizeProfilePath(path) + if lexical != canonical { add(lexical) } - add(normalizeProfilePath(path)) + add(canonical) } return out } diff --git a/internal/sandbox/ssh_gpg_deny_test.go b/internal/sandbox/ssh_gpg_deny_test.go index 00fcea824..ec0da8a50 100644 --- a/internal/sandbox/ssh_gpg_deny_test.go +++ b/internal/sandbox/ssh_gpg_deny_test.go @@ -439,3 +439,94 @@ func TestSSHConfigDiscoveryBoundsOversizedConfig(t *testing.T) { t.Fatalf("~/.ssh was denied wholesale") } } + +func TestCredentialDenyReadPathsFollowsSymlinkedSSHConfig(t *testing.T) { + if runtime.GOOS == "windows" { + t.Skip("symlink creation is not reliably available on Windows CI") + } + home := t.TempDir() + workKey := filepath.Join(home, "keys", "work_ed25519") + mustWriteFile(t, workKey, "") + realConfig := filepath.Join(t.TempDir(), "root-config") + mustWriteFile(t, realConfig, "IdentityFile ~/keys/work_ed25519\n") + mustSymlink(t, realConfig, filepath.Join(home, ".ssh", "config")) + + denied := sshGPGDenied(t, home, nil) + if !denyCovered(denied, workKey) { + t.Fatalf("IdentityFile via symlinked ~/.ssh/config is readable; deny list = %v", denied) + } + if denyCovered(denied, filepath.Join(home, ".ssh")) { + t.Fatalf("~/.ssh was denied wholesale") + } + if denyCovered(denied, filepath.Join(home, ".ssh", "config")) { + t.Fatalf("~/.ssh/config was denied") + } +} + +func TestCredentialDenyReadPathsFollowsSymlinkedSSHConfigInclude(t *testing.T) { + if runtime.GOOS == "windows" { + t.Skip("symlink creation is not reliably available on Windows CI") + } + home := t.TempDir() + sshDir := filepath.Join(home, ".ssh") + includedKey := filepath.Join(home, "keys", "included_ed25519") + mustWriteFile(t, includedKey, "") + realInclude := filepath.Join(t.TempDir(), "extra_config") + mustWriteFile(t, realInclude, "IdentityFile ~/keys/included_ed25519\n") + mustWriteFile(t, filepath.Join(sshDir, "config"), "Include extra_config\n") + mustSymlink(t, realInclude, filepath.Join(sshDir, "extra_config")) + + denied := sshGPGDenied(t, home, nil) + if !denyCovered(denied, includedKey) { + t.Fatalf("IdentityFile via symlinked Include target is readable; deny list = %v", denied) + } + if denyCovered(denied, sshDir) { + t.Fatalf("~/.ssh was denied wholesale") + } +} + +func TestUnreadableEnforcementPreservesLexicalWhenSSHDirIsSymlink(t *testing.T) { + if runtime.GOOS == "windows" { + t.Skip("symlink creation is not reliably available on Windows CI") + } + home := t.TempDir() + realSSH := filepath.Join(t.TempDir(), "ssh-store") + idEd := filepath.Join(realSSH, "id_ed25519") + mustWriteFile(t, idEd, "") + mustSymlink(t, realSSH, filepath.Join(home, ".ssh")) + + lexicalKey := filepath.Join(home, ".ssh", "id_ed25519") + lexical := normalizeProfilePathLexically(lexicalKey) + if info, err := os.Lstat(lexical); err != nil { + t.Fatal(err) + } else if info.Mode().Type() == os.ModeSymlink { + t.Fatalf("expected regular leaf under a symlinked ~/.ssh, got symlink") + } + + denied := sshGPGDenied(t, home, nil) + if !denyListedExact(denied, lexical) { + t.Fatalf("lexical candidate %q missing from deny list %v", lexical, denied) + } + if denyCovered(denied, filepath.Join(home, ".ssh")) { + t.Fatalf("~/.ssh was denied wholesale") + } + + enforced := unreadableEnforcementPaths(denied) + if !denyListedExact(enforced, lexical) { + t.Fatalf("lexical path %q missing from enforcement list %v", lexical, enforced) + } + + profile := PermissionProfile{ + FileSystem: FileSystemPolicy{ + Kind: FileSystemRestricted, + ReadRoots: []string{string(filepath.Separator)}, + DenyReadIfExists: denied, + }, + } + args := linuxBwrapFilesystemArgs(profile) + sbpl := strings.Join(denyReadRules(profile.FileSystem), "\n") + assertArgsContainSequence(t, args, "--ro-bind", "/dev/null", lexical) + if !strings.Contains(sbpl, sandboxProfileString(lexical)) { + t.Fatalf("Seatbelt rules missing lexical pathname %q:\n%s", lexical, sbpl) + } +} diff --git a/internal/sandbox/ssh_gpg_deny_unix_test.go b/internal/sandbox/ssh_gpg_deny_unix_test.go index 00f5c1378..197402231 100644 --- a/internal/sandbox/ssh_gpg_deny_unix_test.go +++ b/internal/sandbox/ssh_gpg_deny_unix_test.go @@ -51,3 +51,46 @@ func TestSSHKeyDiscoverySkipsFIFOAndDeviceWithoutBlocking(t *testing.T) { t.Fatalf("~/.ssh was denied wholesale") } } + +func TestSSHConfigDiscoverySkipsSymlinkToFIFOWithoutBlocking(t *testing.T) { + home := t.TempDir() + sshDir := filepath.Join(home, ".ssh") + if err := os.MkdirAll(sshDir, 0o700); err != nil { + t.Fatal(err) + } + fifo := filepath.Join(t.TempDir(), "fifo-config") + if err := syscall.Mkfifo(fifo, 0o600); err != nil { + t.Fatalf("Mkfifo fifo-config: %v", err) + } + if err := os.Symlink(fifo, filepath.Join(sshDir, "config")); err != nil { + t.Fatal(err) + } + workKey := filepath.Join(home, "keys", "work_ed25519") + if err := os.MkdirAll(filepath.Dir(workKey), 0o700); err != nil { + t.Fatal(err) + } + if err := os.WriteFile(workKey, nil, 0o600); err != nil { + t.Fatal(err) + } + + done := make(chan []string, 1) + go func() { + done <- credentialDenyReadPathsIn(credentialPathOptions{ + Homes: []string{home}, + ConfigDirs: []string{filepath.Join(home, ".config")}, + }, nil).Paths + }() + var denied []string + select { + case denied = <-done: + case <-time.After(300 * time.Millisecond): + t.Fatal("SSH config discovery blocked on a FIFO behind a config symlink") + } + + if denyCovered(denied, workKey) { + t.Fatalf("IdentityFile was discovered through a FIFO config symlink: %v", denied) + } + if denyCovered(denied, sshDir) { + t.Fatalf("~/.ssh was denied wholesale") + } +} diff --git a/internal/sandbox/ssh_key_deny.go b/internal/sandbox/ssh_key_deny.go index 1e61e46fd..0c0cd973f 100644 --- a/internal/sandbox/ssh_key_deny.go +++ b/internal/sandbox/ssh_key_deny.go @@ -155,18 +155,36 @@ func sshFileLooksLikePrivateKey(path string) bool { return strings.Contains(s, "PRIVATE KEY") } -// readRegularFileBounded Lstats first and refuses FIFOs, devices, sockets, -// and symlinks so profile construction cannot block on a special file. The -// subsequent read is capped with LimitReader. +// readRegularFileBounded Lstats first and refuses FIFOs, devices, and +// sockets so profile construction cannot block on a special file. Regular-file +// symlinks are followed: OpenSSH reads ~/.ssh/config and Include targets +// through them, so a relocated IdentityFile would otherwise stay readable. +// The resolved path is Lstat'd again and opened (bounded LimitReader) so a +// FIFO or device behind the link is never opened. func readRegularFileBounded(path string, maxBytes int) ([]byte, bool) { if maxBytes <= 0 { return nil, false } info, err := os.Lstat(path) - if err != nil || !info.Mode().IsRegular() { + if err != nil { + return nil, false + } + readPath := path + if info.Mode().Type() == os.ModeSymlink { + resolved, err := filepath.EvalSymlinks(path) + if err != nil { + return nil, false + } + info, err = os.Lstat(resolved) + if err != nil { + return nil, false + } + readPath = resolved + } + if !info.Mode().IsRegular() { return nil, false } - f, err := os.Open(path) + f, err := os.Open(readPath) if err != nil { return nil, false } From 6536486b6c104a81607afe4925ba55145140aaeb Mon Sep 17 00:00:00 2001 From: cairn-intern Date: Fri, 28 Aug 2026 17:46:40 +0000 Subject: [PATCH 05/14] fix(sandbox): dual-add lexical dests only when a symlink is involved Windows EvalSymlinks rewrites regular files to 8.3 short names, so treating any lexical vs canonical spelling difference as a symlink dual-added both RUNNER~1 and runneradmin and broke existing bwrap dest sequences. Keep the lexical extra only when Lstat of the path or an ancestor is a symlink. Exempt the known-hosts family and /dev/null from ssh_config denials, skip the new symlink test on Windows, cap the SSH walk per directory instead of unwinding the tree, sniff PuTTY PPK keys, and pin the resolved-target deny half without requiring OS symlinks. --- internal/sandbox/profile.go | 91 ++++++++++++--- internal/sandbox/ssh_gpg_deny_test.go | 161 ++++++++++++++++++++++++++ internal/sandbox/ssh_key_deny.go | 51 +++++++- 3 files changed, 279 insertions(+), 24 deletions(-) diff --git a/internal/sandbox/profile.go b/internal/sandbox/profile.go index a195ade2c..b4d24bc42 100644 --- a/internal/sandbox/profile.go +++ b/internal/sandbox/profile.go @@ -632,9 +632,13 @@ func credentialDenyReadPathsIn(options credentialPathOptions, allowRead []string } // appendLexicalCredentialDenyPaths adds the pre-EvalSymlinks spelling of each -// candidate. normalizeProfilePath replaces a symlink with its target, so -// omitting the lexical path would let a later atomic retarget of the same -// pathname escape the deny list. +// candidate when a symlink is in the resolution chain. normalizeProfilePath +// replaces a symlink with its target, so omitting the lexical path would let +// a later atomic retarget of the same pathname escape the deny list. +// String inequality alone is not enough: Windows EvalSymlinks rewrites +// regular files to 8.3 short names (RUNNER~1 vs runneradmin) even when no +// symlink is involved, and dual-adding those spellings breaks exact bwrap +// dest sequences. func appendLexicalCredentialDenyPaths(out, allowRoots, candidates []string) []string { if len(candidates) == 0 { return out @@ -654,7 +658,11 @@ func appendLexicalCredentialDenyPaths(out, allowRoots, candidates []string) []st if credentialPathReincluded(allowRoots, lexical) { continue } - if resolved := normalizeProfilePath(path); resolved != "" && credentialPathReincluded(allowRoots, resolved) { + resolved := normalizeProfilePath(path) + if resolved != "" && credentialPathReincluded(allowRoots, resolved) { + continue + } + if resolved != "" && resolved != lexical && !pathResolutionInvolvesSymlink(path) { continue } seen[lexical] = struct{}{} @@ -1046,6 +1054,9 @@ func normalizeProfilePath(entry string) string { if absolute == "" { return "" } + if canonical, _, ok := lookupTestCredentialPathAlias(absolute); ok { + return canonical + } if resolved, err := filepath.EvalSymlinks(absolute); err == nil { return resolved } @@ -1084,32 +1095,34 @@ func normalizeCredentialFinalPath(path string) string { } // unreadableEnforcementPath is the dest a bwrap bind or Seatbelt rule should -// use for path. When the lexical spelling differs from the EvalSymlinks -// target — a leaf symlink or an intermediate directory symlink such as -// ~/.ssh — keep the lexical pathname so a later atomic retarget still hits -// the same dest. Other paths keep EvalSymlinks so aliases such as macOS -// /var -> /private/var continue to match existing roots. Overlap and allow -// checks use canonical identity via pathWithinRootCanonical, not this. +// use for path. Dual-emitting the pre-EvalSymlinks spelling is only useful +// when a symlink is in the resolution chain (a leaf symlink, an intermediate +// directory symlink such as ~/.ssh, or macOS /var -> /private/var). In that +// case keep the lexical pathname so a later atomic retarget still hits the +// same dest. Other paths keep EvalSymlinks so Windows 8.3 rewrites of regular +// files are not treated as a second dest. Overlap and allow checks use +// canonical identity via pathWithinRootCanonical, not this. func unreadableEnforcementPath(path string) string { lexical := normalizeProfilePathLexically(path) if lexical == "" { return "" } resolved := normalizeProfilePath(path) - if resolved == "" || resolved == lexical { - if resolved != "" { - return resolved - } + if resolved == "" { return lexical } + if resolved == lexical || !pathResolutionInvolvesSymlink(path) { + return resolved + } return lexical } -// unreadableEnforcementPaths preserves lexical identity whenever it differs -// from the EvalSymlinks target, including intermediate directory symlinks +// unreadableEnforcementPaths preserves lexical identity only when a symlink +// is in the resolution chain, including intermediate directory symlinks // (for example ~/.ssh -> elsewhere with a regular key file inside). A later // retarget of that directory would otherwise expose the key through the -// original pathname. Non-symlink paths stay canonical. +// original pathname. Non-symlink paths stay canonical, even when EvalSymlinks +// rewrites the spelling (Windows 8.3 short names). func unreadableEnforcementPaths(paths []string) []string { if len(paths) == 0 { return nil @@ -1132,7 +1145,11 @@ func unreadableEnforcementPaths(paths []string) []string { continue } canonical := normalizeProfilePath(path) - if lexical != canonical { + if canonical == "" { + add(lexical) + continue + } + if lexical != canonical && pathResolutionInvolvesSymlink(path) { add(lexical) } add(canonical) @@ -1140,6 +1157,44 @@ func unreadableEnforcementPaths(paths []string) []string { return out } +// testCredentialPathAlias remaps a lexically normalized path for tests so +// both the EvalSymlinks (canonical) deny entry and the lexical extra can be +// pinned without creating OS symlinks. Production leaves it nil. +var testCredentialPathAlias func(lexical string) (canonical string, involvesSymlink bool, ok bool) + +func lookupTestCredentialPathAlias(lexical string) (canonical string, involvesSymlink bool, ok bool) { + if testCredentialPathAlias == nil || lexical == "" { + return "", false, false + } + return testCredentialPathAlias(lexical) +} + +// pathResolutionInvolvesSymlink reports whether Lstat of path or an ancestor +// is a symlink. Dual-adding lexical + EvalSymlinks target is only valid in +// that case: macOS /var -> /private/var and a real ~/.ssh directory symlink +// need both spellings, but Windows EvalSymlinks 8.3 short names of regular +// files must not dual-add. +func pathResolutionInvolvesSymlink(path string) bool { + current := normalizeProfilePathLexically(path) + if current == "" { + return false + } + if _, involves, ok := lookupTestCredentialPathAlias(current); ok { + return involves + } + for { + info, err := os.Lstat(current) + if err == nil && info.Mode().Type() == os.ModeSymlink { + return true + } + parent := filepath.Dir(current) + if parent == current { + return false + } + current = parent + } +} + // normalizeProfilePathLexically expands and absolutizes a profile path without // resolving symlinks. Credential carveouts use it so their fixed lexical name // can never become an allow rule for a symlink target. diff --git a/internal/sandbox/ssh_gpg_deny_test.go b/internal/sandbox/ssh_gpg_deny_test.go index ec0da8a50..3ec06e159 100644 --- a/internal/sandbox/ssh_gpg_deny_test.go +++ b/internal/sandbox/ssh_gpg_deny_test.go @@ -1,6 +1,7 @@ package sandbox import ( + "fmt" "os" "path/filepath" "runtime" @@ -280,6 +281,9 @@ func TestExpandSSHConfigPathPercentDUsesSuppliedHome(t *testing.T) { } func TestCredentialDenyReadPathsKeepsLexicalSymlinkCandidates(t *testing.T) { + if runtime.GOOS == "windows" { + t.Skip("symlink creation is not reliably available on Windows CI") + } home := t.TempDir() realDir := t.TempDir() @@ -530,3 +534,160 @@ func TestUnreadableEnforcementPreservesLexicalWhenSSHDirIsSymlink(t *testing.T) t.Fatalf("Seatbelt rules missing lexical pathname %q:\n%s", lexical, sbpl) } } + +func TestSSHShouldDenyReferencedPathExemptsKnownHostsFamilyAndDevNull(t *testing.T) { + home, sshDir := sshGPGNormalizationHome() + keepReadable := []string{ + filepath.Join(sshDir, "known_hosts"), + filepath.Join(sshDir, "known_hosts2"), + filepath.Join(sshDir, "known_hosts.old"), + filepath.Join(sshDir, "ssh_known_hosts"), + filepath.Join(sshDir, "ssh_known_hosts2"), + "/dev/null", + os.DevNull, + } + for _, path := range keepReadable { + if sshShouldDenyReferencedPath(path, home, sshDir) { + t.Fatalf("%q must stay readable (known-hosts family or /dev/null)", path) + } + } + idEd := filepath.Join(sshDir, "id_ed25519") + if !sshShouldDenyReferencedPath(idEd, home, sshDir) { + t.Fatalf("well-known SSH key under fake home was not a deny candidate") + } + if !sshShouldDenyReferencedPath(filepath.Join(sshDir, "custom-key"), home, sshDir) { + t.Fatalf("non-exempt referenced path was not a deny candidate") + } +} + +func TestCredentialDenyReadPathsKeepsKnownHostsFamilyFromConfig(t *testing.T) { + home := t.TempDir() + sshDir := filepath.Join(home, ".ssh") + known2 := filepath.Join(sshDir, "known_hosts2") + sshKnown := filepath.Join(home, "ssh_known_hosts") + mustWriteFile(t, known2, "example.com ssh-ed25519 AAAA\n") + mustWriteFile(t, sshKnown, "example.com ssh-ed25519 AAAA\n") + mustWriteFile(t, filepath.Join(sshDir, "config"), "UserKnownHostsFile ~/.ssh/known_hosts2 /dev/null\nGlobalKnownHostsFile "+sshKnown+"\n") + + denied := sshGPGDenied(t, home, nil) + if denyCovered(denied, known2) { + t.Fatalf("known_hosts2 was denied because UserKnownHostsFile pointed at it: %v", denied) + } + if denyCovered(denied, sshKnown) { + t.Fatalf("ssh_known_hosts was denied because GlobalKnownHostsFile pointed at it: %v", denied) + } + if denyCovered(denied, filepath.Join(sshDir, "config")) { + t.Fatalf("~/.ssh/config was denied") + } + if denyListedExact(denied, filepath.Clean("/dev/null")) || denyListedExact(denied, "/dev/null") { + t.Fatalf("/dev/null was denied from UserKnownHostsFile: %v", denied) + } + if denyCovered(denied, sshDir) { + t.Fatalf("~/.ssh was denied wholesale") + } +} + +func TestWalkSSHPrivateKeyFilesFindsKeyAfterCrowdedSiblingDir(t *testing.T) { + home := t.TempDir() + sshDir := filepath.Join(home, ".ssh") + junkDir := filepath.Join(sshDir, "aaa_known_hosts.d") + if err := os.MkdirAll(junkDir, 0o700); err != nil { + t.Fatal(err) + } + for i := 0; i < sshPrivateKeyWalkMaxEntries+32; i++ { + mustWriteFile(t, filepath.Join(junkDir, fmt.Sprintf("host-%04d", i)), "ssh-ed25519 AAAA\n") + } + nestedKey := filepath.Join(sshDir, "keys", "work_ed25519") + mustWriteFile(t, nestedKey, "-----BEGIN OPENSSH PRIVATE KEY-----\nfixture\n") + + denied := sshGPGDenied(t, home, nil) + if !denyCovered(denied, nestedKey) { + t.Fatalf("private key in a sibling of a crowded directory was not found; deny list = %v", denied) + } + if denyCovered(denied, sshDir) { + t.Fatalf("~/.ssh was denied wholesale") + } +} + +func TestCredentialDenyReadPathsDeniesPuttyPPK(t *testing.T) { + home := t.TempDir() + ppk := filepath.Join(home, ".ssh", "putty-key.ppk") + custom := filepath.Join(home, ".ssh", "custom-putty") + mustWriteFile(t, ppk, "") + mustWriteFile(t, custom, "PuTTY-User-Key-File-2: ssh-rsa\nEncryption: none\n") + + denied := sshGPGDenied(t, home, nil) + if !denyCovered(denied, ppk) { + t.Fatalf(".ppk is readable; deny list = %v", denied) + } + if !denyCovered(denied, custom) { + t.Fatalf("PuTTY-User-Key-File sniff missed custom-putty; deny list = %v", denied) + } + if denyCovered(denied, filepath.Join(home, ".ssh")) { + t.Fatalf("~/.ssh was denied wholesale") + } +} + +func TestCredentialDenyReadPathsPinsResolvedTargetWithoutOSSymlink(t *testing.T) { + home := t.TempDir() + lexicalKey := normalizeProfilePathLexically(filepath.Join(home, ".ssh", "id_ed25519")) + resolvedKey := filepath.Join(t.TempDir(), "resolved-id_ed25519-target") + testCredentialPathAlias = func(lexical string) (string, bool, bool) { + if lexical == lexicalKey { + return resolvedKey, true, true + } + return "", false, false + } + t.Cleanup(func() { testCredentialPathAlias = nil }) + + denied := sshGPGDenied(t, home, nil) + if !denyListedExact(denied, resolvedKey) { + t.Fatalf("resolved-target half missing; removing the sshKeys candidates append would cause this: %v", denied) + } + if !denyListedExact(denied, lexicalKey) { + t.Fatalf("lexical symlink extra missing: %v", denied) + } + + enforced := unreadableEnforcementPaths([]string{lexicalKey}) + if !denyListedExact(enforced, resolvedKey) { + t.Fatalf("enforcement list missing resolved target %q: %v", resolvedKey, enforced) + } + if !denyListedExact(enforced, lexicalKey) { + t.Fatalf("enforcement list missing lexical extra %q: %v", lexicalKey, enforced) + } +} + +func TestUnreadableEnforcementPathsSkipsNonSymlinkSpellingRewrite(t *testing.T) { + home := t.TempDir() + lexicalKey := normalizeProfilePathLexically(filepath.Join(home, ".ssh", "id_ed25519")) + shortName := filepath.Join(t.TempDir(), "RUNNER~1", "id_ed25519") + testCredentialPathAlias = func(lexical string) (string, bool, bool) { + if lexical == lexicalKey { + return shortName, false, true + } + return "", false, false + } + t.Cleanup(func() { testCredentialPathAlias = nil }) + + if pathResolutionInvolvesSymlink(lexicalKey) { + t.Fatalf("8.3-style rewrite must not count as a symlink") + } + enforced := unreadableEnforcementPaths([]string{lexicalKey}) + if denyListedExact(enforced, lexicalKey) { + t.Fatalf("non-symlink spelling rewrite dual-added lexical dest %q: %v", lexicalKey, enforced) + } + if !denyListedExact(enforced, shortName) { + t.Fatalf("canonical 8.3-style dest missing: %v", enforced) + } + if got := unreadableEnforcementPath(lexicalKey); got != shortName { + t.Fatalf("bwrap dest = %q, want canonical %q", got, shortName) + } + + denied := sshGPGDenied(t, home, nil) + if denyListedExact(denied, lexicalKey) && lexicalKey != shortName { + t.Fatalf("lexical 8.3 extra was dual-added: %v", denied) + } + if !denyListedExact(denied, shortName) { + t.Fatalf("canonical ssh key missing after 8.3-style rewrite: %v", denied) + } +} diff --git a/internal/sandbox/ssh_key_deny.go b/internal/sandbox/ssh_key_deny.go index 0c0cd973f..7d15cff50 100644 --- a/internal/sandbox/ssh_key_deny.go +++ b/internal/sandbox/ssh_key_deny.go @@ -20,6 +20,11 @@ const sshIncludeMatchCap = 64 // followed, so a cycle cannot hang profile construction. const sshPrivateKeyWalkMaxDepth = 8 +// sshPrivateKeyWalkMaxEntries is a per-directory cap on entries considered +// under ~/.ssh. Extra entries in one directory (a large known_hosts.d, for +// example) are skipped; walking continues in sibling and parent directories +// so a private key elsewhere is still discovered. It is not a process-wide +// abort that unwinds the whole tree. const sshPrivateKeyWalkMaxEntries = 256 const sshPrivateKeySniffBytes = 128 @@ -73,19 +78,20 @@ func sshPrivateKeyDenyCandidates(home string) []string { func walkSSHPrivateKeyFiles(sshDir string) []string { var out []string - n := 0 var walk func(dir string, depth int) walk = func(dir string, depth int) { - if depth > sshPrivateKeyWalkMaxDepth || n >= sshPrivateKeyWalkMaxEntries { + if depth > sshPrivateKeyWalkMaxDepth { return } entries, err := os.ReadDir(dir) if err != nil { return } + n := 0 for _, entry := range entries { if n >= sshPrivateKeyWalkMaxEntries { - return + // Skip the rest of this directory only; sibling dirs still walk. + break } name := entry.Name() if name == "." || name == ".." { @@ -129,15 +135,31 @@ func isSSHPrivateKeyFileName(name string) bool { if strings.HasPrefix(name, "id_") { return true } - return strings.HasSuffix(strings.ToLower(name), ".pem") + lower := strings.ToLower(name) + return strings.HasSuffix(lower, ".pem") || strings.HasSuffix(lower, ".ppk") } func sshPublicOrConfigName(name string) bool { switch name { - case "config", "known_hosts", "known_hosts.old", "authorized_keys", "authorized_keys2": + case "config", "authorized_keys", "authorized_keys2": return true } - return strings.HasSuffix(name, ".pub") + if strings.HasSuffix(name, ".pub") { + return true + } + return sshKnownHostsFamilyName(name) +} + +// sshKnownHostsFamilyName reports OpenSSH known-hosts filenames that must stay +// readable so git host resolution still works. The family is the known_hosts / +// ssh_known_hosts spellings (including *2 and *.old), not five exact literals. +func sshKnownHostsFamilyName(name string) bool { + switch name { + case "known_hosts", "known_hosts2", "known_hosts.old", + "ssh_known_hosts", "ssh_known_hosts2": + return true + } + return strings.HasPrefix(name, "known_hosts.") || strings.HasPrefix(name, "ssh_known_hosts.") } func sshFileLooksLikePrivateKey(path string) bool { @@ -149,6 +171,9 @@ func sshFileLooksLikePrivateKey(path string) bool { return false } s := strings.TrimSpace(string(data)) + if strings.HasPrefix(s, "PuTTY-User-Key-File") { + return true + } if !strings.HasPrefix(s, "-----BEGIN ") { return false } @@ -406,5 +431,19 @@ func sshShouldDenyReferencedPath(path, home, sshDir string) bool { if sshDir != "" && cleaned == filepath.Clean(sshDir) { return false } + if sshIsDevNullPath(cleaned) { + return false + } return !sshPublicOrConfigName(filepath.Base(cleaned)) } + +// sshIsDevNullPath reports UserKnownHostsFile /dev/null (and the host equivalent +// os.DevNull). The basename of that path is "null", which is not a known-hosts +// name; denying it would install a Seatbelt deny file-read* on /dev/null. +func sshIsDevNullPath(path string) bool { + cleaned := filepath.Clean(path) + if cleaned == os.DevNull || strings.EqualFold(cleaned, os.DevNull) { + return true + } + return filepath.ToSlash(cleaned) == "/dev/null" +} From cfb89654f4e87338246c521f1ef8d98f5f78a246 Mon Sep 17 00:00:00 2001 From: cairn-intern Date: Fri, 28 Aug 2026 18:08:37 +0000 Subject: [PATCH 06/14] fix(sandbox): bound SSH walks and honor nested GPG allowRead Cap per-directory SSH discovery with File.ReadDir so a large sibling cannot unboundedly allocate. Restrict known-hosts exemptions to supported OpenSSH filenames so known_hosts.private with a key payload is denied. Omit a credential directory deny when a nested allowRead file would be masked by bwrap/Seatbelt. Inspect leaf key symlinks. Build private-key test headers from fragments at runtime. --- internal/sandbox/profile.go | 42 +++++++++ internal/sandbox/ssh_gpg_deny_test.go | 127 +++++++++++++++++++++++++- internal/sandbox/ssh_key_deny.go | 26 ++++-- 3 files changed, 184 insertions(+), 11 deletions(-) diff --git a/internal/sandbox/profile.go b/internal/sandbox/profile.go index b4d24bc42..12e8434f6 100644 --- a/internal/sandbox/profile.go +++ b/internal/sandbox/profile.go @@ -618,6 +618,14 @@ func credentialDenyReadPathsIn(options credentialPathOptions, allowRead []string if credentialPathReincluded(allowRoots, path) { continue } + for _, nested := range credentialNestedAllowReads(allowRoots, path) { + if normalizeCredentialCarveoutPath(nested) != "" { + carveouts = append(carveouts, nested) + } + } + if credentialDirDenyHidesNestedAllow(allowRoots, path) { + continue + } out = append(out, path) } out = appendLexicalCredentialDenyPaths(out, allowRoots, lexicalCandidates) @@ -658,6 +666,9 @@ func appendLexicalCredentialDenyPaths(out, allowRoots, candidates []string) []st if credentialPathReincluded(allowRoots, lexical) { continue } + if credentialDirDenyHidesNestedAllow(allowRoots, lexical) { + continue + } resolved := normalizeProfilePath(path) if resolved != "" && credentialPathReincluded(allowRoots, resolved) { continue @@ -760,6 +771,37 @@ func credentialPathReincluded(allowRoots []string, path string) bool { return false } +// credentialNestedAllowReads returns allowRead paths that sit strictly inside +// path — a nested grant under a credential directory. +func credentialNestedAllowReads(allowRoots []string, path string) []string { + if path == "" || len(allowRoots) == 0 { + return nil + } + var out []string + for _, allow := range allowRoots { + if allow != path && pathWithinRoot(path, allow) { + out = append(out, allow) + } + } + return out +} + +// credentialDirDenyHidesNestedAllow reports that some allowRead sits under +// path and cannot be expressed as a directory DenyReadCarveout. Existing +// carveouts only re-bind directories (Zero's plugins/specialists/commands). +// A nested file grant such as $HOME/.gnupg/private-keys-v1.d/keygrip.key +// would stay unreadable if path were still emitted as a directory deny: +// bubblewrap masks the dir and Seatbelt denies the subtree after the read +// rule. In that case the parent dir deny is omitted. +func credentialDirDenyHidesNestedAllow(allowRoots []string, path string) bool { + for _, allow := range credentialNestedAllowReads(allowRoots, path) { + if normalizeCredentialCarveoutPath(allow) == "" { + return true + } + } + return false +} + // pathWithinRootCanonical compares after EvalSymlinks so a lexical /var/... // candidate is recognized as lying under a canonical /private/var/... root. // Overlap and allow checks use this identity; backends emit lexical symlink diff --git a/internal/sandbox/ssh_gpg_deny_test.go b/internal/sandbox/ssh_gpg_deny_test.go index 3ec06e159..58ca207a7 100644 --- a/internal/sandbox/ssh_gpg_deny_test.go +++ b/internal/sandbox/ssh_gpg_deny_test.go @@ -49,6 +49,14 @@ func mustSymlink(t *testing.T, target, link string) { } } +func sshPrivateKeyFixture() string { + return strings.Join([]string{"-----BEGIN OPENSSH", " PRIVATE KEY-----\nfixture\n"}, "") +} + +func puttyPrivateKeyFixture() string { + return strings.Join([]string{"PuTTY-User-Key", "-File-2: ssh-rsa\nEncryption: none\n"}, "") +} + func sshGPGDenied(t *testing.T, home string, allowRead []string) []string { t.Helper() return credentialDenyReadPathsIn(credentialPathOptions{ @@ -328,7 +336,7 @@ func TestCredentialDenyReadPathsDeniesNestedSSHPrivateKeys(t *testing.T) { nestedPub := filepath.Join(sshDir, "keys", "work.pub") nestedConfig := filepath.Join(sshDir, "keys", "config") nestedKnown := filepath.Join(sshDir, "keys", "known_hosts") - mustWriteFile(t, nestedKey, "-----BEGIN OPENSSH PRIVATE KEY-----\nfixture\n") + mustWriteFile(t, nestedKey, sshPrivateKeyFixture()) mustWriteFile(t, nestedID, "") mustWriteFile(t, nestedPub, "ssh-ed25519 AAAA nested\n") mustWriteFile(t, nestedConfig, "Host *\n") @@ -558,6 +566,9 @@ func TestSSHShouldDenyReferencedPathExemptsKnownHostsFamilyAndDevNull(t *testing if !sshShouldDenyReferencedPath(filepath.Join(sshDir, "custom-key"), home, sshDir) { t.Fatalf("non-exempt referenced path was not a deny candidate") } + if !sshShouldDenyReferencedPath(filepath.Join(sshDir, "known_hosts.private"), home, sshDir) { + t.Fatalf("known_hosts.private must not be treated as a known-hosts family name") + } } func TestCredentialDenyReadPathsKeepsKnownHostsFamilyFromConfig(t *testing.T) { @@ -598,7 +609,7 @@ func TestWalkSSHPrivateKeyFilesFindsKeyAfterCrowdedSiblingDir(t *testing.T) { mustWriteFile(t, filepath.Join(junkDir, fmt.Sprintf("host-%04d", i)), "ssh-ed25519 AAAA\n") } nestedKey := filepath.Join(sshDir, "keys", "work_ed25519") - mustWriteFile(t, nestedKey, "-----BEGIN OPENSSH PRIVATE KEY-----\nfixture\n") + mustWriteFile(t, nestedKey, sshPrivateKeyFixture()) denied := sshGPGDenied(t, home, nil) if !denyCovered(denied, nestedKey) { @@ -614,7 +625,7 @@ func TestCredentialDenyReadPathsDeniesPuttyPPK(t *testing.T) { ppk := filepath.Join(home, ".ssh", "putty-key.ppk") custom := filepath.Join(home, ".ssh", "custom-putty") mustWriteFile(t, ppk, "") - mustWriteFile(t, custom, "PuTTY-User-Key-File-2: ssh-rsa\nEncryption: none\n") + mustWriteFile(t, custom, puttyPrivateKeyFixture()) denied := sshGPGDenied(t, home, nil) if !denyCovered(denied, ppk) { @@ -691,3 +702,113 @@ func TestUnreadableEnforcementPathsSkipsNonSymlinkSpellingRewrite(t *testing.T) t.Fatalf("canonical ssh key missing after 8.3-style rewrite: %v", denied) } } + +func TestCredentialDenyReadPathsDeniesKnownHostsPrivateNamedKey(t *testing.T) { + home := t.TempDir() + key := filepath.Join(home, ".ssh", "known_hosts.private") + mustWriteFile(t, key, sshPrivateKeyFixture()) + + denied := sshGPGDenied(t, home, nil) + if !denyCovered(denied, key) { + t.Fatalf("known_hosts.private with a private-key payload is readable; deny list = %v", denied) + } + if denyCovered(denied, filepath.Join(home, ".ssh")) { + t.Fatalf("~/.ssh was denied wholesale") + } + if denyCovered(denied, filepath.Join(home, ".ssh", "known_hosts")) { + t.Fatalf("supported known_hosts was denied") + } +} + +func TestWalkSSHPrivateKeyFilesDeniesCustomNamedSymlinkToPrivateKey(t *testing.T) { + if runtime.GOOS == "windows" { + t.Skip("symlink creation is not reliably available on Windows CI") + } + home := t.TempDir() + target := filepath.Join(t.TempDir(), "real-key") + mustWriteFile(t, target, sshPrivateKeyFixture()) + link := filepath.Join(home, ".ssh", "work") + mustSymlink(t, target, link) + + denied := sshGPGDenied(t, home, nil) + if !denyCovered(denied, link) { + t.Fatalf("custom-named symlink to a private key is readable; deny list = %v", denied) + } + if denyCovered(denied, filepath.Join(home, ".ssh")) { + t.Fatalf("~/.ssh was denied wholesale") + } +} + +func TestCredentialDenyReadPathsNestedGPGAllowReadOmitsParentDir(t *testing.T) { + home := t.TempDir() + key := filepath.Join(home, ".gnupg", "private-keys-v1.d", "keygrip.key") + mustWriteFile(t, key, "fake-keygrip") + mustWriteFile(t, filepath.Join(home, ".gnupg", "secring.gpg"), "fake-secring") + mustWriteFile(t, filepath.Join(home, ".git-credentials"), "https://user:token@github.com") + + allow := []string{key} + denied := sshGPGDenied(t, home, allow) + gnupg := normalizeProfilePath(filepath.Join(home, ".gnupg")) + if denyListedExact(denied, gnupg) { + t.Fatalf("nested allowRead left parent ~/.gnupg in DenyReadIfExists: %v", denied) + } + if denyCovered(denied, key) { + t.Fatalf("nested allowRead key is still denied: %v", denied) + } + if !denyCovered(denied, filepath.Join(home, ".git-credentials")) { + t.Fatalf("git-credentials must stay denied when only a nested GPG key is allowed: %v", denied) + } + if denyCovered(denied, filepath.Join(home, ".ssh")) { + t.Fatalf("~/.ssh was denied wholesale") + } +} + +func TestLinuxBwrapAndSeatbeltHonorNestedGPGAllowRead(t *testing.T) { + home := t.TempDir() + key := filepath.Join(home, ".gnupg", "private-keys-v1.d", "keygrip.key") + mustWriteFile(t, key, "fake-keygrip") + mustWriteFile(t, filepath.Join(home, ".gnupg", "secring.gpg"), "fake-secring") + + allow := []string{key} + creds := credentialDenyReadPathsIn(credentialPathOptions{ + Homes: []string{home}, + ConfigDirs: []string{filepath.Join(home, ".config")}, + }, allow) + gnupg := normalizeProfilePath(filepath.Join(home, ".gnupg")) + if denyListedExact(creds.Paths, gnupg) { + t.Fatalf("nested allowRead left parent ~/.gnupg in DenyReadIfExists: %v", creds.Paths) + } + if denyCovered(creds.Paths, key) { + t.Fatalf("nested allowRead key is still denied: %v", creds.Paths) + } + + profile := PermissionProfile{ + FileSystem: FileSystemPolicy{ + Kind: FileSystemRestricted, + ReadRoots: []string{string(filepath.Separator), normalizeProfilePath(key)}, + DenyReadIfExists: creds.Paths, + DenyReadCarveouts: creds.Carveouts, + }, + } + args := linuxBwrapFilesystemArgs(profile) + if argsContainSequence(args, "--perms", "000", "--tmpfs", gnupg) || + argsContainSequence(args, "--perms", "111", "--tmpfs", gnupg) || + argsContainSequence(args, "--ro-bind", "/dev/null", gnupg) { + t.Fatalf("bwrap masked ~/.gnupg despite nested allowRead: %#v", args) + } + + sbpl := strings.Join(denyReadRules(profile.FileSystem), "\n") + if strings.Contains(sbpl, sandboxProfileString(gnupg)) { + t.Fatalf("Seatbelt deny rules still cover ~/.gnupg after nested allowRead:\n%s", sbpl) + } + keyLit := sandboxProfileString(normalizeProfilePath(key)) + if strings.Contains(sbpl, `(deny file-read* (literal "`+keyLit+`"))`) || + strings.Contains(sbpl, `(deny file-read* (subpath "`+keyLit+`"))`) { + t.Fatalf("Seatbelt still denies the nested allowRead key:\n%s", sbpl) + } + full := seatbeltProfileFromPermissionProfile(profile, Policy{}, "") + denyIdx := strings.LastIndex(full, `(deny file-read* (subpath "`+sandboxProfileString(gnupg)+`"))`) + if denyIdx >= 0 { + t.Fatalf("full Seatbelt profile still denies ~/.gnupg subtree:\n%s", full) + } +} diff --git a/internal/sandbox/ssh_key_deny.go b/internal/sandbox/ssh_key_deny.go index 7d15cff50..3438974fd 100644 --- a/internal/sandbox/ssh_key_deny.go +++ b/internal/sandbox/ssh_key_deny.go @@ -83,10 +83,17 @@ func walkSSHPrivateKeyFiles(sshDir string) []string { if depth > sshPrivateKeyWalkMaxDepth { return } - entries, err := os.ReadDir(dir) + d, err := os.Open(dir) if err != nil { return } + // Bound allocation to the per-directory cap. os.ReadDir would load the + // whole directory first. Overflow of one dir must not abort siblings. + entries, err := d.ReadDir(sshPrivateKeyWalkMaxEntries) + _ = d.Close() + if err != nil && err != io.EOF { + return + } n := 0 for _, entry := range entries { if n >= sshPrivateKeyWalkMaxEntries { @@ -105,9 +112,10 @@ func walkSSHPrivateKeyFiles(sshDir string) []string { } mode := info.Mode() if mode.Type() == os.ModeSymlink { - // Name-based only: do not follow, so a FIFO or cycle behind - // the link cannot block profile construction. - if isSSHPrivateKeyFileName(name) { + // Inspect leaf symlinks (bounded, specials rejected) so a + // custom-named link to a PEM/OpenSSH key is still denied. + // Directory symlinks are not traversed. + if isSSHPrivateKeyFileName(name) || sshFileLooksLikePrivateKey(path) { out = append(out, path) } continue @@ -150,16 +158,18 @@ func sshPublicOrConfigName(name string) bool { return sshKnownHostsFamilyName(name) } -// sshKnownHostsFamilyName reports OpenSSH known-hosts filenames that must stay -// readable so git host resolution still works. The family is the known_hosts / -// ssh_known_hosts spellings (including *2 and *.old), not five exact literals. +// sshKnownHostsFamilyName reports the supported OpenSSH known-hosts filenames +// that must stay readable so git host resolution still works. Arbitrary +// known_hosts.* / ssh_known_hosts.* names are not included: a private key +// named known_hosts.private must still be detected. /dev/null is exempted in +// sshShouldDenyReferencedPath, not here (its basename is "null"). func sshKnownHostsFamilyName(name string) bool { switch name { case "known_hosts", "known_hosts2", "known_hosts.old", "ssh_known_hosts", "ssh_known_hosts2": return true } - return strings.HasPrefix(name, "known_hosts.") || strings.HasPrefix(name, "ssh_known_hosts.") + return false } func sshFileLooksLikePrivateKey(path string) bool { From 8550f7b72fff0c3c645d00dba8db70b7d67cce9b Mon Sep 17 00:00:00 2001 From: cairn-intern Date: Fri, 28 Aug 2026 18:40:22 +0000 Subject: [PATCH 07/14] fix(sandbox): sniff .pub keys, expand ${HOME}, mask symlink dests Address CodeRabbit follow-ups on #990: content-sniff private keys named *.pub, expand ${HOME}/$HOME from the supplied home, compare lexical credential dir denies against canonical nested allowRead, and stop using symlink paths as bwrap --ro-bind destinations. --- internal/sandbox/linux_helper.go | 189 ++++++++++++++++++-- internal/sandbox/profile.go | 11 +- internal/sandbox/ssh_gpg_deny_test.go | 238 +++++++++++++++++++++++++- internal/sandbox/ssh_key_deny.go | 81 ++++++++- 4 files changed, 498 insertions(+), 21 deletions(-) diff --git a/internal/sandbox/linux_helper.go b/internal/sandbox/linux_helper.go index ea8e52711..e830a5b2b 100644 --- a/internal/sandbox/linux_helper.go +++ b/internal/sandbox/linux_helper.go @@ -303,9 +303,8 @@ func buildLinuxBwrapFilesystemPlan(profile PermissionProfile) linuxBwrapFilesyst for _, path := range fs.DenyWrite { args = appendReadOnlyLinuxPathArgs(args, path) } - for _, path := range fs.DenyRead { - args = appendUnreadableLinuxPathArgs(args, path, fs.DenyReadCarveouts) - } + var unreadable []string + unreadable = append(unreadable, fs.DenyRead...) // The profile includes only trusted, process-environment-derived directories // here. Command-controlled credential roots remain deny-if-present and must // never cause host filesystem mutations before sandbox launch. @@ -323,8 +322,9 @@ func buildLinuxBwrapFilesystemPlan(profile PermissionProfile) linuxBwrapFilesyst // retarget cannot reopen it. continue } - args = appendUnreadableLinuxPathArgs(args, path, fs.DenyReadCarveouts) + unreadable = append(unreadable, path) } + args = appendUnreadableLinuxPaths(args, unreadable, fs.DenyReadCarveouts, fs.WriteRoots) return linuxBwrapFilesystemPlan{ Args: args, ProtectedCreateTargets: dedupeStrings(protectedCreateTargets), @@ -400,15 +400,112 @@ func appendReadOnlyLinuxPathArgs(args []string, path string) []string { } func appendUnreadableLinuxPathArgs(args []string, path string, carveouts []string) []string { - path = unreadableEnforcementPath(path) - if path == "" { + return appendUnreadableLinuxPaths(args, []string{path}, carveouts, nil) +} + +// appendUnreadableLinuxPaths emits bwrap args that hide the given deny paths. +// Directories keep the existing tmpfs mask. Regular files stay `--ro-bind +// /dev/null path`. Symlink dests cannot use that bind: mount(2) LOOKUP_FOLLOW +// would mask the current target (so a later retarget reopens a new credential) +// or ENOENT a dangling link. Instead mask the resolved regular-file target and, +// when the parent is a credential directory, tmpfs-overlay the parent omitting +// denied basenames so the lexical dentry disappears without following. +func appendUnreadableLinuxPaths(args []string, paths []string, carveouts []string, writeRoots []WritableRoot) []string { + classified := classifyUnreadableLinuxPaths(paths) + for _, dir := range classified.dirs { + args = appendUnreadableLinuxDirArgs(args, dir, carveouts) + } + omits := linuxDeniedBasenamesByParent(classified.files, classified.links) + seenParents := make(map[string]struct{}) + for _, link := range classified.links { + args = appendUnreadableLinuxResolvedSymlinkArgs(args, link, carveouts) + parent := filepath.Clean(filepath.Dir(link)) + if _, dup := seenParents[parent]; dup { + continue + } + if !linuxCredentialParentSafeToTmpfs(parent, writeRoots) { + continue + } + seenParents[parent] = struct{}{} + args = appendLinuxParentTmpfsOmitting(args, parent, omits[parent]) + } + for _, file := range classified.files { + args = append(args, "--ro-bind", "/dev/null", file) + } + return args +} + +type linuxUnreadableClassified struct { + files []string + dirs []string + links []string +} + +func classifyUnreadableLinuxPaths(paths []string) linuxUnreadableClassified { + var out linuxUnreadableClassified + seen := make(map[string]struct{}, len(paths)) + for _, path := range paths { + path = unreadableEnforcementPath(path) + if path == "" { + continue + } + if _, ok := seen[path]; ok { + continue + } + seen[path] = struct{}{} + info, err := os.Lstat(path) + if err != nil { + continue + } + switch { + case info.Mode().Type() == os.ModeSymlink: + out.links = append(out.links, path) + case info.IsDir(): + out.dirs = append(out.dirs, path) + default: + out.files = append(out.files, path) + } + } + return out +} + +func linuxDeniedBasenamesByParent(files, links []string) map[string]map[string]struct{} { + out := make(map[string]map[string]struct{}) + add := func(path string) { + parent := filepath.Clean(filepath.Dir(path)) + base := filepath.Base(path) + m, ok := out[parent] + if !ok { + m = make(map[string]struct{}) + out[parent] = m + } + m[base] = struct{}{} + } + for _, path := range files { + add(path) + } + for _, path := range links { + add(path) + } + return out +} + +func appendUnreadableLinuxResolvedSymlinkArgs(args []string, path string, carveouts []string) []string { + resolved, err := filepath.EvalSymlinks(path) + if err != nil || resolved == "" { + return args + } + info, err := os.Lstat(resolved) + if err != nil { return args } - // Lstat so a credential symlink is masked at its lexical pathname rather - // than following to a dest that a later retarget would miss. - if info, err := os.Lstat(path); err == nil && !info.IsDir() { - return append(args, "--ro-bind", "/dev/null", path) + if info.IsDir() { + return appendUnreadableLinuxDirArgs(args, resolved, carveouts) } + return append(args, "--ro-bind", "/dev/null", resolved) +} + +func appendUnreadableLinuxDirArgs(args []string, path string, carveouts []string) []string { nested := nestedCarveoutPaths(path, carveouts) if len(nested) == 0 { return append(args, "--perms", "000", "--tmpfs", path, "--remount-ro", path) @@ -427,6 +524,78 @@ func appendUnreadableLinuxPathArgs(args []string, path string, carveouts []strin return append(args, "--remount-ro", path) } +// linuxCredentialParentSafeToTmpfs reports that parent may be reconstructed +// inside the sandbox to hide a lexical symlink dentry. HOME, `/`, `/tmp`, +// `/etc`, `/var`, and write roots must never be tmpfs-overlaid: reconstructing +// HOME is forbidden and would hide the workspace. Only credential directories +// such as ~/.ssh and ~/.gnupg (including nested dirs under them) qualify. +func linuxCredentialParentSafeToTmpfs(parent string, writeRoots []WritableRoot) bool { + parent = filepath.Clean(parent) + if parent == "" || parent == "." || parent == string(filepath.Separator) { + return false + } + switch parent { + case "/tmp", "/etc", "/var", "/usr", "/home", "/root", "/opt", "/dev", "/proc", "/sys", "/run", "/mnt", "/media": + return false + } + switch strings.ToLower(filepath.Base(parent)) { + case "tmp", "etc", "var", "usr", "home", "root", "opt", "dev", "proc", "sys", "run": + return false + } + for _, wr := range writeRoots { + root := filepath.Clean(strings.TrimSpace(wr.Root)) + if root != "" && parent == root { + return false + } + } + if !linuxCredentialDirPath(parent) { + return false + } + info, err := os.Lstat(parent) + if err != nil || !info.IsDir() { + return false + } + return true +} + +func linuxCredentialDirPath(path string) bool { + base := filepath.Base(filepath.Clean(path)) + switch base { + case ".ssh", ".gnupg", ".aws", ".azure": + return true + } + slash := filepath.ToSlash(filepath.Clean(path)) + for _, marker := range []string{"/.ssh/", "/.gnupg/", "/.aws/", "/.azure/"} { + if strings.Contains(slash, marker) { + return true + } + } + return false +} + +func appendLinuxParentTmpfsOmitting(args []string, parent string, omit map[string]struct{}) []string { + parent = filepath.Clean(parent) + entries, err := os.ReadDir(parent) + if err != nil { + return args + } + // 555 keeps option-2 public names (config, known_hosts, *.pub) listable + // after the overlay; denied basenames are simply not rebound. + args = append(args, "--perms", "555", "--tmpfs", parent) + for _, entry := range entries { + name := entry.Name() + if name == "." || name == ".." { + continue + } + if _, skip := omit[name]; skip { + continue + } + sibling := filepath.Join(parent, name) + args = append(args, "--ro-bind", sibling, sibling) + } + return append(args, "--remount-ro", parent) +} + // nestedCarveoutPaths returns the carveouts that sit strictly inside root, // shallowest first so a parent bind is created before a nested one. func nestedCarveoutPaths(root string, carveouts []string) []string { diff --git a/internal/sandbox/profile.go b/internal/sandbox/profile.go index 12e8434f6..09cabbff9 100644 --- a/internal/sandbox/profile.go +++ b/internal/sandbox/profile.go @@ -772,14 +772,21 @@ func credentialPathReincluded(allowRoots []string, path string) bool { } // credentialNestedAllowReads returns allowRead paths that sit strictly inside -// path — a nested grant under a credential directory. +// path — a nested grant under a credential directory. Containment is canonical +// so a lexical ~/.gnupg symlink is recognized as the parent of a nested +// allowRead that lives under the symlink target. pathWithinRoot on the lexical +// spelling would miss that pair, keep the lexical dir deny, and let Seatbelt +// and bwrap expand it onto the canonical store. func credentialNestedAllowReads(allowRoots []string, path string) []string { if path == "" || len(allowRoots) == 0 { return nil } var out []string for _, allow := range allowRoots { - if allow != path && pathWithinRoot(path, allow) { + if allow == path { + continue + } + if pathWithinRootCanonical(path, allow) && !pathWithinRootCanonical(allow, path) { out = append(out, allow) } } diff --git a/internal/sandbox/ssh_gpg_deny_test.go b/internal/sandbox/ssh_gpg_deny_test.go index 58ca207a7..426d1495f 100644 --- a/internal/sandbox/ssh_gpg_deny_test.go +++ b/internal/sandbox/ssh_gpg_deny_test.go @@ -399,11 +399,21 @@ func TestLinuxBwrapAndSeatbeltKeepLexicalCredentialSymlinkPaths(t *testing.T) { sbpl := strings.Join(denyReadRules(profile.FileSystem), "\n") for _, candidate := range []string{gnupgLink, gitLink, sshLink} { lexical := normalizeProfilePathLexically(candidate) - assertArgsContainSequence(t, args, "--ro-bind", "/dev/null", lexical) + assertBwrapDoesNotFollowBindSymlinkDest(t, args, lexical) if !strings.Contains(sbpl, sandboxProfileString(lexical)) { t.Fatalf("Seatbelt rules missing lexical pathname %q:\n%s", lexical, sbpl) } } + assertArgsContainSequence(t, args, "--ro-bind", "/dev/null", normalizeProfilePath(gitTarget)) + assertArgsContainSequence(t, args, "--ro-bind", "/dev/null", normalizeProfilePath(sshTarget)) + sshDir := normalizeProfilePathLexically(filepath.Join(home, ".ssh")) + if !argsContainSequence(args, "--tmpfs", sshDir) { + t.Fatalf("expected tmpfs overlay of ~/.ssh to hide lexical key symlink: %#v", args) + } + gnupgTargetNorm := normalizeProfilePath(gnupgTarget) + if !argsContainSequence(args, "--tmpfs", gnupgTargetNorm) { + t.Fatalf("expected tmpfs mask of resolved ~/.gnupg target: %#v", args) + } newGit := filepath.Join(realDir, "other-credentials") mustWriteFile(t, newGit, "retargeted") @@ -413,11 +423,10 @@ func TestLinuxBwrapAndSeatbeltKeepLexicalCredentialSymlinkPaths(t *testing.T) { mustSymlink(t, newGit, gitLink) lexicalGit := normalizeProfilePathLexically(gitLink) - if !argsContainSequence(args, "--ro-bind", "/dev/null", lexicalGit) { - t.Fatalf("pre-retarget bwrap args lost lexical dest %q: %#v", lexicalGit, args) - } + assertBwrapDoesNotFollowBindSymlinkDest(t, args, lexicalGit) reemitted := linuxBwrapFilesystemArgs(profile) - assertArgsContainSequence(t, reemitted, "--ro-bind", "/dev/null", lexicalGit) + assertBwrapDoesNotFollowBindSymlinkDest(t, reemitted, lexicalGit) + assertArgsContainSequence(t, reemitted, "--ro-bind", "/dev/null", normalizeProfilePath(newGit)) deniedAfter := sshGPGDenied(t, home, nil) if !denyListedExact(deniedAfter, lexicalGit) { @@ -731,8 +740,9 @@ func TestWalkSSHPrivateKeyFilesDeniesCustomNamedSymlinkToPrivateKey(t *testing.T mustSymlink(t, target, link) denied := sshGPGDenied(t, home, nil) - if !denyCovered(denied, link) { - t.Fatalf("custom-named symlink to a private key is readable; deny list = %v", denied) + lexical := normalizeProfilePathLexically(link) + if !denyListedExact(denied, lexical) && !denyListedExact(denied, link) { + t.Fatalf("custom-named symlink lost its lexical deny entry; deny list = %v", denied) } if denyCovered(denied, filepath.Join(home, ".ssh")) { t.Fatalf("~/.ssh was denied wholesale") @@ -812,3 +822,217 @@ func TestLinuxBwrapAndSeatbeltHonorNestedGPGAllowRead(t *testing.T) { t.Fatalf("full Seatbelt profile still denies ~/.gnupg subtree:\n%s", full) } } + +func assertBwrapDoesNotFollowBindSymlinkDest(t *testing.T, args []string, lexical string) { + t.Helper() + if argsContainSequence(args, "--ro-bind", "/dev/null", lexical) { + t.Fatalf("bwrap --ro-bind /dev/null used symlink dest %q (follows / ENOENTs): %#v", lexical, args) + } +} + +func TestExpandSSHConfigPathHomeEnvFromSuppliedHome(t *testing.T) { + home, sshDir := sshGPGNormalizationHome() + got := expandSSHConfigPath("${HOME}/keys/work_ed25519", home, sshDir) + want := filepath.Join(home, "keys", "work_ed25519") + if got != want { + t.Fatalf("expandSSHConfigPath(${HOME}) = %q, want %q", got, want) + } + got = expandSSHConfigPath("$HOME/keys/work_ed25519", home, sshDir) + if got != want { + t.Fatalf("expandSSHConfigPath($HOME) = %q, want %q", got, want) + } + if expandSSHConfigPath("${NOTHOME}/keys/x", home, sshDir) != "" { + t.Fatalf("unknown ${NOTHOME} must be dropped, not joined under ~/.ssh") + } + if expandSSHConfigPath("$NOTHOME/keys/x", home, sshDir) != "" { + t.Fatalf("unknown $NOTHOME must be dropped, not joined under ~/.ssh") + } + nonsense := filepath.Join(sshDir, "${NOTHOME}", "keys", "x") + if expandSSHConfigPath("${NOTHOME}/keys/x", home, sshDir) == nonsense { + t.Fatalf("unknown ${NOTHOME} was joined under ~/.ssh as %q", nonsense) + } +} + +func TestCredentialDenyReadPathsDeniesSSHConfigIdentityFileHomeEnv(t *testing.T) { + home := t.TempDir() + workKey := filepath.Join(home, "keys", "work_ed25519") + mustWriteFile(t, workKey, "") + mustWriteFile(t, filepath.Join(home, ".ssh", "config"), "IdentityFile ${HOME}/keys/work_ed25519\n") + + denied := sshGPGDenied(t, home, nil) + if !denyCovered(denied, workKey) { + t.Fatalf("IdentityFile ${HOME}/keys/work_ed25519 is readable; deny list = %v", denied) + } + if denyCovered(denied, filepath.Join(home, ".ssh")) { + t.Fatalf("~/.ssh was denied wholesale") + } + + mustWriteFile(t, filepath.Join(home, ".ssh", "config"), "IdentityFile $HOME/keys/work_ed25519\nIdentityFile ${NOTHOME}/keys/x\n") + denied = sshGPGDenied(t, home, nil) + if !denyCovered(denied, workKey) { + t.Fatalf("IdentityFile $HOME/keys/work_ed25519 is readable; deny list = %v", denied) + } + nonsense := filepath.Join(home, ".ssh", "${NOTHOME}", "keys", "x") + if denyListedExact(denied, nonsense) || denyCovered(denied, nonsense) { + t.Fatalf("unknown ${NOTHOME} was joined under ~/.ssh: %v", denied) + } +} + +func TestWalkSSHPrivateKeyFilesDeniesPrivateKeyPayloadNamedPub(t *testing.T) { + home := t.TempDir() + sshDir := filepath.Join(home, ".ssh") + fakePub := filepath.Join(sshDir, "work.pub") + realPub := filepath.Join(sshDir, "id_ed25519.pub") + mustWriteFile(t, fakePub, sshPrivateKeyFixture()) + mustWriteFile(t, realPub, "ssh-ed25519 AAAA public\n") + + denied := sshGPGDenied(t, home, nil) + if !denyCovered(denied, fakePub) { + t.Fatalf("private-key payload at ~/.ssh/work.pub is readable; deny list = %v", denied) + } + if denyCovered(denied, realPub) { + t.Fatalf("real ssh-ed25519 .pub was denied; public keys must stay readable") + } + if denyCovered(denied, sshDir) { + t.Fatalf("~/.ssh was denied wholesale") + } +} + +func TestLinuxBwrapAndSeatbeltHonorNestedGPGAllowReadThroughDirSymlink(t *testing.T) { + if runtime.GOOS == "windows" { + t.Skip("symlink creation is not reliably available on Windows CI") + } + home := t.TempDir() + gnupgTarget := filepath.Join(t.TempDir(), "gnupg-store") + key := filepath.Join(gnupgTarget, "private-keys-v1.d", "keygrip.key") + mustWriteFile(t, key, "fake-keygrip") + mustWriteFile(t, filepath.Join(gnupgTarget, "secring.gpg"), "fake-secring") + mustSymlink(t, gnupgTarget, filepath.Join(home, ".gnupg")) + + allow := []string{key} + creds := credentialDenyReadPathsIn(credentialPathOptions{ + Homes: []string{home}, + ConfigDirs: []string{filepath.Join(home, ".config")}, + }, allow) + lexicalGnupg := normalizeProfilePathLexically(filepath.Join(home, ".gnupg")) + canonicalGnupg := normalizeProfilePath(filepath.Join(home, ".gnupg")) + if denyListedExact(creds.Paths, lexicalGnupg) { + t.Fatalf("lexical ~/.gnupg dir deny retained despite nested canonical allowRead: %v", creds.Paths) + } + if canonicalGnupg != "" && denyListedExact(creds.Paths, canonicalGnupg) { + t.Fatalf("canonical ~/.gnupg dir deny retained despite nested allowRead: %v", creds.Paths) + } + if denyCovered(creds.Paths, key) { + t.Fatalf("nested allowRead key is still denied: %v", creds.Paths) + } + + profile := PermissionProfile{ + FileSystem: FileSystemPolicy{ + Kind: FileSystemRestricted, + ReadRoots: []string{string(filepath.Separator), normalizeProfilePath(key)}, + DenyReadIfExists: creds.Paths, + DenyReadCarveouts: creds.Carveouts, + }, + } + args := linuxBwrapFilesystemArgs(profile) + if argsContainSequence(args, "--perms", "000", "--tmpfs", lexicalGnupg) || + argsContainSequence(args, "--perms", "111", "--tmpfs", lexicalGnupg) || + argsContainSequence(args, "--perms", "555", "--tmpfs", lexicalGnupg) || + argsContainSequence(args, "--ro-bind", "/dev/null", lexicalGnupg) || + (canonicalGnupg != "" && (argsContainSequence(args, "--perms", "000", "--tmpfs", canonicalGnupg) || + argsContainSequence(args, "--perms", "111", "--tmpfs", canonicalGnupg) || + argsContainSequence(args, "--ro-bind", "/dev/null", canonicalGnupg))) { + t.Fatalf("bwrap masked ~/.gnupg despite nested allowRead under dir symlink: %#v", args) + } + + sbpl := strings.Join(denyReadRules(profile.FileSystem), "\n") + if strings.Contains(sbpl, sandboxProfileString(lexicalGnupg)) { + t.Fatalf("Seatbelt deny rules still cover lexical ~/.gnupg after nested allowRead:\n%s", sbpl) + } + if canonicalGnupg != "" && strings.Contains(sbpl, sandboxProfileString(canonicalGnupg)) { + t.Fatalf("Seatbelt deny rules still cover canonical ~/.gnupg after nested allowRead:\n%s", sbpl) + } + keyLit := sandboxProfileString(normalizeProfilePath(key)) + if strings.Contains(sbpl, `(deny file-read* (literal "`+keyLit+`"))`) || + strings.Contains(sbpl, `(deny file-read* (subpath "`+keyLit+`"))`) { + t.Fatalf("Seatbelt still denies the nested allowRead key:\n%s", sbpl) + } + full := seatbeltProfileFromPermissionProfile(profile, Policy{}, "") + if canonicalGnupg != "" { + denyIdx := strings.LastIndex(full, `(deny file-read* (subpath "`+sandboxProfileString(canonicalGnupg)+`"))`) + if denyIdx >= 0 { + t.Fatalf("full Seatbelt profile still denies canonical ~/.gnupg subtree:\n%s", full) + } + } +} + +func TestLinuxBwrapMasksLiveAndDanglingCredentialSymlinks(t *testing.T) { + if runtime.GOOS == "windows" { + t.Skip("symlink creation is not reliably available on Windows CI") + } + home := t.TempDir() + realDir := t.TempDir() + sshDir := filepath.Join(home, ".ssh") + config := filepath.Join(sshDir, "config") + knownHosts := filepath.Join(sshDir, "known_hosts") + pub := filepath.Join(sshDir, "id_ed25519.pub") + mustWriteFile(t, config, "Host *\n") + mustWriteFile(t, knownHosts, "github.com ssh-ed25519 AAAA\n") + mustWriteFile(t, pub, "ssh-ed25519 AAAA public\n") + + liveTarget := filepath.Join(realDir, "id_ed25519") + mustWriteFile(t, liveTarget, sshPrivateKeyFixture()) + liveLink := filepath.Join(sshDir, "id_ed25519") + mustSymlink(t, liveTarget, liveLink) + + danglingTarget := filepath.Join(realDir, "missing-id_rsa") + danglingLink := filepath.Join(sshDir, "id_rsa") + mustSymlink(t, danglingTarget, danglingLink) + + gitTarget := filepath.Join(realDir, "git-credentials") + mustWriteFile(t, gitTarget, "x") + gitLink := filepath.Join(home, ".git-credentials") + mustSymlink(t, gitTarget, gitLink) + + denied := sshGPGDenied(t, home, nil) + profile := PermissionProfile{ + FileSystem: FileSystemPolicy{ + Kind: FileSystemRestricted, + ReadRoots: []string{string(filepath.Separator)}, + DenyReadIfExists: denied, + }, + } + args := linuxBwrapFilesystemArgs(profile) + + lexicalLive := normalizeProfilePathLexically(liveLink) + lexicalDangling := normalizeProfilePathLexically(danglingLink) + lexicalGit := normalizeProfilePathLexically(gitLink) + assertBwrapDoesNotFollowBindSymlinkDest(t, args, lexicalLive) + assertBwrapDoesNotFollowBindSymlinkDest(t, args, lexicalDangling) + assertBwrapDoesNotFollowBindSymlinkDest(t, args, lexicalGit) + + assertArgsContainSequence(t, args, "--ro-bind", "/dev/null", normalizeProfilePath(liveTarget)) + assertArgsContainSequence(t, args, "--ro-bind", "/dev/null", normalizeProfilePath(gitTarget)) + if argsContainSequence(args, "--ro-bind", "/dev/null", danglingTarget) || + argsContainSequence(args, "--ro-bind", "/dev/null", lexicalDangling) { + t.Fatalf("dangling symlink must not be a hard --ro-bind dest: %#v", args) + } + + sshDirLex := normalizeProfilePathLexically(sshDir) + if !argsContainSequence(args, "--tmpfs", sshDirLex) { + t.Fatalf("expected tmpfs overlay of ~/.ssh for live/dangling key symlinks: %#v", args) + } + assertArgsContainSequence(t, args, "--ro-bind", config, config) + assertArgsContainSequence(t, args, "--ro-bind", knownHosts, knownHosts) + assertArgsContainSequence(t, args, "--ro-bind", pub, pub) + if argsContainSequence(args, "--ro-bind", liveLink, liveLink) || + argsContainSequence(args, "--ro-bind", danglingLink, danglingLink) { + t.Fatalf("denied symlink basenames were rebound into ~/.ssh overlay: %#v", args) + } + if argsContainSequence(args, "--tmpfs", home) || argsContainSequence(args, "--tmpfs", filepath.Clean(home)) { + t.Fatalf("HOME must never be tmpfs-overlaid: %#v", args) + } + if denyCovered(denied, sshDir) { + t.Fatalf("~/.ssh was denied wholesale") + } +} diff --git a/internal/sandbox/ssh_key_deny.go b/internal/sandbox/ssh_key_deny.go index 3438974fd..2e3e0a886 100644 --- a/internal/sandbox/ssh_key_deny.go +++ b/internal/sandbox/ssh_key_deny.go @@ -173,7 +173,11 @@ func sshKnownHostsFamilyName(name string) bool { } func sshFileLooksLikePrivateKey(path string) bool { - if sshPublicOrConfigName(filepath.Base(path)) { + // Basename-based denial still treats *.pub as public, but a PEM/OpenSSH/PuTTY + // private key named work.pub must not stay readable. Sniff .pub payloads. + // Keep config / authorized_keys / known-hosts family exemptions: those names + // are never content-denied here (CertificateFile and host-key files). + if sshConfigOrKnownHostsName(filepath.Base(path)) { return false } data, ok := readRegularFileBounded(path, sshPrivateKeySniffBytes) @@ -190,6 +194,17 @@ func sshFileLooksLikePrivateKey(path string) bool { return strings.Contains(s, "PRIVATE KEY") } +// sshConfigOrKnownHostsName is the subset of sshPublicOrConfigName that must +// not be content-sniffed. *.pub is intentionally excluded so a private-key +// payload at that name is still denied. +func sshConfigOrKnownHostsName(name string) bool { + switch name { + case "config", "authorized_keys", "authorized_keys2": + return true + } + return sshKnownHostsFamilyName(name) +} + // readRegularFileBounded Lstats first and refuses FIFOs, devices, and // sockets so profile construction cannot block on a special file. Regular-file // symlinks are followed: OpenSSH reads ~/.ssh/config and Include targets @@ -376,7 +391,15 @@ func expandSSHConfigPath(value, home, sshDir string) string { if value == "" || strings.EqualFold(value, "none") || strings.EqualFold(value, "SSH_AUTH_SOCK") { return "" } - expanded, ok := expandSSHConfigPathTokens(value, home) + // OpenSSH expands environment variables in IdentityFile. Only ${HOME}/$HOME + // from the supplied home argument (never process env). Unknown $VAR is + // treated like an unsupported percent token: drop the path so we never deny + // or follow an unresolved pattern. + expandedEnv, ok := expandSSHConfigPathEnv(value, home) + if !ok { + return "" + } + expanded, ok := expandSSHConfigPathTokens(expandedEnv, home) if !ok { return "" } @@ -395,6 +418,60 @@ func expandSSHConfigPath(value, home, sshDir string) string { } } +// expandSSHConfigPathEnv resolves ${HOME} and $HOME from the supplied home +// argument. Any other ${VAR}/$VAR, a dangling $, or a malformed ${...} drops +// the path. No live process environment map is consulted. +func expandSSHConfigPathEnv(value, home string) (string, bool) { + if !strings.Contains(value, "$") { + return value, true + } + var b strings.Builder + b.Grow(len(value) + len(home)) + for i := 0; i < len(value); i++ { + if value[i] != '$' { + b.WriteByte(value[i]) + continue + } + if i+1 >= len(value) { + return "", false + } + if value[i+1] == '{' { + end := strings.IndexByte(value[i+2:], '}') + if end < 0 { + return "", false + } + name := value[i+2 : i+2+end] + if name != "HOME" { + return "", false + } + b.WriteString(home) + i += 2 + end + continue + } + if !sshEnvVarStart(value[i+1]) { + return "", false + } + j := i + 1 + for j < len(value) && sshEnvVarChar(value[j]) { + j++ + } + if value[i+1:j] != "HOME" { + return "", false + } + b.WriteString(home) + i = j - 1 + } + return b.String(), true +} + +func sshEnvVarStart(c byte) bool { + return (c >= 'A' && c <= 'Z') || (c >= 'a' && c <= 'z') || c == '_' +} + +func sshEnvVarChar(c byte) bool { + return sshEnvVarStart(c) || (c >= '0' && c <= '9') +} + // expandSSHConfigPathTokens resolves OpenSSH path tokens we can expand without // a live connection: %d is the supplied local home, %% is a literal %. Any // remaining percent token (%h, a trailing %, ...) is unsupported and the path From 9111fb319612eba4656d1f180793a571ea8a506a Mon Sep 17 00:00:00 2001 From: cairn-intern Date: Fri, 28 Aug 2026 21:59:26 +0000 Subject: [PATCH 08/14] fix(sandbox): skip overlaid file binds and sniff named keys Address CodeRabbit follow-ups on #990: do not --ro-bind /dev/null onto files whose parent was already tmpfs-overlaid, skip dangling sibling bind sources, and sniff IdentityFile paths even when the basename looks public. --- internal/sandbox/linux_helper.go | 13 ++ internal/sandbox/ssh_gpg_deny_test.go | 154 +++++++++++++++++++++ internal/sandbox/ssh_gpg_deny_unix_test.go | 4 +- internal/sandbox/ssh_key_deny.go | 29 ++-- 4 files changed, 182 insertions(+), 18 deletions(-) diff --git a/internal/sandbox/linux_helper.go b/internal/sandbox/linux_helper.go index e830a5b2b..79544267f 100644 --- a/internal/sandbox/linux_helper.go +++ b/internal/sandbox/linux_helper.go @@ -430,6 +430,14 @@ func appendUnreadableLinuxPaths(args []string, paths []string, carveouts []strin args = appendLinuxParentTmpfsOmitting(args, parent, omits[parent]) } for _, file := range classified.files { + parent := filepath.Clean(filepath.Dir(file)) + if _, overlaid := seenParents[parent]; overlaid { + // Parent was already tmpfs-overlaid (symlink sibling in the same + // credential dir). Re-binding /dev/null onto the regular file would + // target a dest that no longer exists after the overlay and can + // abort bubblewrap at startup. + continue + } args = append(args, "--ro-bind", "/dev/null", file) } return args @@ -591,6 +599,11 @@ func appendLinuxParentTmpfsOmitting(args []string, parent string, omit map[strin continue } sibling := filepath.Join(parent, name) + if !pathExists(sibling) { + // os.ReadDir returns dangling symlinks; bwrap --ro-bind sources + // must resolve, so skip them rather than aborting sandbox startup. + continue + } args = append(args, "--ro-bind", sibling, sibling) } return append(args, "--remount-ro", parent) diff --git a/internal/sandbox/ssh_gpg_deny_test.go b/internal/sandbox/ssh_gpg_deny_test.go index 426d1495f..949b149fb 100644 --- a/internal/sandbox/ssh_gpg_deny_test.go +++ b/internal/sandbox/ssh_gpg_deny_test.go @@ -898,6 +898,46 @@ func TestWalkSSHPrivateKeyFilesDeniesPrivateKeyPayloadNamedPub(t *testing.T) { } } +func TestCredentialDenyReadPathsDeniesSSHConfigIdentityFilePubWithPrivateKeyPayload(t *testing.T) { + home := t.TempDir() + fakePub := filepath.Join(home, "keys", "work.pub") + realPub := filepath.Join(home, "keys", "id_ed25519.pub") + mustWriteFile(t, fakePub, sshPrivateKeyFixture()) + mustWriteFile(t, realPub, "ssh-ed25519 AAAA public\n") + mustWriteFile(t, filepath.Join(home, ".ssh", "config"), "IdentityFile ~/keys/work.pub\nIdentityFile ~/keys/id_ed25519.pub\n") + + denied := sshGPGDenied(t, home, nil) + if !denyCovered(denied, fakePub) { + t.Fatalf("IdentityFile ~/keys/work.pub with private-key payload is readable; deny list = %v", denied) + } + if denyCovered(denied, realPub) { + t.Fatalf("real ssh-ed25519 .pub at IdentityFile path was denied; public keys must stay readable") + } + if denyCovered(denied, filepath.Join(home, ".ssh")) { + t.Fatalf("~/.ssh was denied wholesale") + } +} + +func TestCredentialDenyReadPathsDeniesSSHConfigIdentityFileNamedKnownHosts(t *testing.T) { + home := t.TempDir() + fakeKnown := filepath.Join(home, "keys", "known_hosts") + realKnown := filepath.Join(home, "other", "known_hosts") + mustWriteFile(t, fakeKnown, sshPrivateKeyFixture()) + mustWriteFile(t, realKnown, "example.com ssh-ed25519 AAAA\n") + mustWriteFile(t, filepath.Join(home, ".ssh", "config"), "IdentityFile ~/keys/known_hosts\nUserKnownHostsFile ~/other/known_hosts\n") + + denied := sshGPGDenied(t, home, nil) + if !denyCovered(denied, fakeKnown) { + t.Fatalf("IdentityFile of a private-key file named known_hosts is readable; deny list = %v", denied) + } + if denyCovered(denied, realKnown) { + t.Fatalf("real known_hosts file was denied") + } + if denyCovered(denied, filepath.Join(home, ".ssh")) { + t.Fatalf("~/.ssh was denied wholesale") + } +} + func TestLinuxBwrapAndSeatbeltHonorNestedGPGAllowReadThroughDirSymlink(t *testing.T) { if runtime.GOOS == "windows" { t.Skip("symlink creation is not reliably available on Windows CI") @@ -966,6 +1006,67 @@ func TestLinuxBwrapAndSeatbeltHonorNestedGPGAllowReadThroughDirSymlink(t *testin } } +func TestLinuxBwrapAndSeatbeltHonorNestedGPGDirAllowReadThroughDirSymlink(t *testing.T) { + if runtime.GOOS == "windows" { + t.Skip("symlink creation is not reliably available on Windows CI") + } + home := t.TempDir() + gnupgTarget := filepath.Join(t.TempDir(), "gnupg-store") + keyDir := filepath.Join(gnupgTarget, "private-keys-v1.d") + key := filepath.Join(keyDir, "keygrip.key") + mustWriteFile(t, key, "fake-keygrip") + mustWriteFile(t, filepath.Join(gnupgTarget, "secring.gpg"), "fake-secring") + mustSymlink(t, gnupgTarget, filepath.Join(home, ".gnupg")) + + allow := []string{filepath.Join(home, ".gnupg", "private-keys-v1.d")} + creds := credentialDenyReadPathsIn(credentialPathOptions{ + Homes: []string{home}, + ConfigDirs: []string{filepath.Join(home, ".config")}, + }, allow) + canonicalGnupg := normalizeProfilePath(filepath.Join(home, ".gnupg")) + canonicalKeyDir := normalizeCredentialCarveoutPath(filepath.Join(home, ".gnupg", "private-keys-v1.d")) + if canonicalKeyDir == "" { + t.Fatalf("nested directory grant did not produce a credential carveout") + } + if canonicalGnupg == "" || !denyListedExact(creds.Paths, canonicalGnupg) { + t.Fatalf("canonical ~/.gnupg must stay denied so the directory carveout can re-bind: %v", creds.Paths) + } + if !denyListedExact(creds.Carveouts, canonicalKeyDir) { + t.Fatalf("canonical private-keys-v1.d carveout missing: %v", creds.Carveouts) + } + if denyListedExact(creds.Paths, canonicalKeyDir) || denyListedExact(creds.Paths, keyDir) { + t.Fatalf("nested directory grant itself was emitted as a deny path: %v", creds.Paths) + } + + profile := PermissionProfile{ + FileSystem: FileSystemPolicy{ + Kind: FileSystemRestricted, + ReadRoots: []string{string(filepath.Separator), canonicalKeyDir}, + DenyReadIfExists: creds.Paths, + DenyReadCarveouts: creds.Carveouts, + }, + } + args := linuxBwrapFilesystemArgs(profile) + assertArgsContainSequence(t, args, "--perms", "111", "--tmpfs", canonicalGnupg) + assertArgsContainSequence(t, args, "--ro-bind", canonicalKeyDir, canonicalKeyDir) + assertArgsContainSequence(t, args, "--remount-ro", canonicalGnupg) + bindIdx := argsSequenceIndex(args, "--ro-bind", canonicalKeyDir, canonicalKeyDir) + remountIdx := argsSequenceIndex(args, "--remount-ro", canonicalGnupg) + if bindIdx < 0 || remountIdx < 0 || bindIdx > remountIdx { + t.Fatalf("canonical carveout bind (%d) must precede tmpfs remount-ro (%d): %#v", bindIdx, remountIdx, args) + } + + sbpl := strings.Join(denyReadCarveoutRules(profile.FileSystem), "\n") + keyDirLit := sandboxProfileString(canonicalKeyDir) + if !strings.Contains(sbpl, `(allow file-read* file-test-existence (subpath "`+keyDirLit+`"))`) { + t.Fatalf("Seatbelt carveout rules missing canonical private-keys-v1.d:\n%s", sbpl) + } + full := seatbeltProfileFromPermissionProfile(profile, Policy{}, "") + if !strings.Contains(full, `(allow file-read* file-test-existence (subpath "`+keyDirLit+`"))`) { + t.Fatalf("full Seatbelt profile missing canonical directory carveout:\n%s", full) + } +} + func TestLinuxBwrapMasksLiveAndDanglingCredentialSymlinks(t *testing.T) { if runtime.GOOS == "windows" { t.Skip("symlink creation is not reliably available on Windows CI") @@ -1036,3 +1137,56 @@ func TestLinuxBwrapMasksLiveAndDanglingCredentialSymlinks(t *testing.T) { t.Fatalf("~/.ssh was denied wholesale") } } + +func TestLinuxBwrapSkipsFileBindsUnderOverlaidCredentialParent(t *testing.T) { + if runtime.GOOS == "windows" { + t.Skip("symlink creation is not reliably available on Windows CI") + } + home := t.TempDir() + sshDir := filepath.Join(home, ".ssh") + realDir := t.TempDir() + workTarget := filepath.Join(realDir, "work") + mustWriteFile(t, workTarget, sshPrivateKeyFixture()) + workLink := filepath.Join(sshDir, "work") + mustSymlink(t, workTarget, workLink) + idEd := filepath.Join(sshDir, "id_ed25519") + mustWriteFile(t, idEd, sshPrivateKeyFixture()) + config := filepath.Join(sshDir, "config") + mustWriteFile(t, config, "Host *\n") + danglingSibling := filepath.Join(sshDir, "config.local") + mustSymlink(t, filepath.Join(realDir, "missing-config.local"), danglingSibling) + + denied := sshGPGDenied(t, home, nil) + if !denyCovered(denied, workLink) { + t.Fatalf("denied symlink ~/.ssh/work missing from deny list: %v", denied) + } + if !denyCovered(denied, idEd) { + t.Fatalf("denied regular ~/.ssh/id_ed25519 missing from deny list: %v", denied) + } + profile := PermissionProfile{ + FileSystem: FileSystemPolicy{ + Kind: FileSystemRestricted, + ReadRoots: []string{string(filepath.Separator)}, + DenyReadIfExists: denied, + }, + } + args := linuxBwrapFilesystemArgs(profile) + sshDirLex := normalizeProfilePathLexically(sshDir) + if !argsContainSequence(args, "--tmpfs", sshDirLex) { + t.Fatalf("expected tmpfs overlay of ~/.ssh once a denied symlink is present: %#v", args) + } + if argsContainSequence(args, "--ro-bind", "/dev/null", idEd) || + argsContainSequence(args, "--ro-bind", "/dev/null", normalizeProfilePath(idEd)) { + t.Fatalf("--ro-bind /dev/null onto regular file whose parent was tmpfs-overlaid: %#v", args) + } + if argsContainSequence(args, "--ro-bind", idEd, idEd) { + t.Fatalf("denied regular key was rebound into ~/.ssh overlay: %#v", args) + } + if argsContainSequence(args, "--ro-bind", danglingSibling, danglingSibling) { + t.Fatalf("dangling sibling used as --ro-bind source: %#v", args) + } + assertArgsContainSequence(t, args, "--ro-bind", config, config) + if denyCovered(denied, sshDir) { + t.Fatalf("~/.ssh was denied wholesale") + } +} diff --git a/internal/sandbox/ssh_gpg_deny_unix_test.go b/internal/sandbox/ssh_gpg_deny_unix_test.go index 197402231..2e4acc2fa 100644 --- a/internal/sandbox/ssh_gpg_deny_unix_test.go +++ b/internal/sandbox/ssh_gpg_deny_unix_test.go @@ -37,7 +37,7 @@ func TestSSHKeyDiscoverySkipsFIFOAndDeviceWithoutBlocking(t *testing.T) { var denied []string select { case denied = <-done: - case <-time.After(300 * time.Millisecond): + case <-time.After(5 * time.Second): t.Fatal("SSH/GPG discovery blocked on a FIFO or device") } @@ -83,7 +83,7 @@ func TestSSHConfigDiscoverySkipsSymlinkToFIFOWithoutBlocking(t *testing.T) { var denied []string select { case denied = <-done: - case <-time.After(300 * time.Millisecond): + case <-time.After(5 * time.Second): t.Fatal("SSH config discovery blocked on a FIFO behind a config symlink") } diff --git a/internal/sandbox/ssh_key_deny.go b/internal/sandbox/ssh_key_deny.go index 2e3e0a886..1e8a62f1c 100644 --- a/internal/sandbox/ssh_key_deny.go +++ b/internal/sandbox/ssh_key_deny.go @@ -173,11 +173,12 @@ func sshKnownHostsFamilyName(name string) bool { } func sshFileLooksLikePrivateKey(path string) bool { - // Basename-based denial still treats *.pub as public, but a PEM/OpenSSH/PuTTY - // private key named work.pub must not stay readable. Sniff .pub payloads. - // Keep config / authorized_keys / known-hosts family exemptions: those names - // are never content-denied here (CertificateFile and host-key files). - if sshConfigOrKnownHostsName(filepath.Base(path)) { + // Basename-based denial still treats *.pub and known-hosts names as public, + // but a PEM/OpenSSH/PuTTY private key at those names must not stay readable. + // Sniff those payloads. Keep config / authorized_keys exemptions: + // CertificateFile and authorized_keys are never content-denied here. + switch filepath.Base(path) { + case "config", "authorized_keys", "authorized_keys2": return false } data, ok := readRegularFileBounded(path, sshPrivateKeySniffBytes) @@ -194,17 +195,6 @@ func sshFileLooksLikePrivateKey(path string) bool { return strings.Contains(s, "PRIVATE KEY") } -// sshConfigOrKnownHostsName is the subset of sshPublicOrConfigName that must -// not be content-sniffed. *.pub is intentionally excluded so a private-key -// payload at that name is still denied. -func sshConfigOrKnownHostsName(name string) bool { - switch name { - case "config", "authorized_keys", "authorized_keys2": - return true - } - return sshKnownHostsFamilyName(name) -} - // readRegularFileBounded Lstats first and refuses FIFOs, devices, and // sockets so profile construction cannot block on a special file. Regular-file // symlinks are followed: OpenSSH reads ~/.ssh/config and Include targets @@ -521,6 +511,13 @@ func sshShouldDenyReferencedPath(path, home, sshDir string) bool { if sshIsDevNullPath(cleaned) { return false } + // Sniff before the public-name exemption so IdentityFile ~/keys/work.pub + // (or a relocated key named known_hosts) with a private-key payload is + // denied. Genuine public keys, genuine known-hosts, config, and + // authorized_keys do not match and stay readable. + if sshFileLooksLikePrivateKey(cleaned) { + return true + } return !sshPublicOrConfigName(filepath.Base(cleaned)) } From abf7671d5818017e47e92d330421bc4bf54e627a Mon Sep 17 00:00:00 2001 From: cairn-intern Date: Fri, 28 Aug 2026 22:13:38 +0000 Subject: [PATCH 09/14] fix(sandbox): bind when overlay fails; sniff named IdentityFiles Record tmpfs-overlaid parents only after the overlay is applied so a ReadDir failure still /dev/null-binds denied files. Sniff IdentityFile targets named config or authorized_keys for private-key payloads. --- internal/sandbox/linux_helper.go | 16 +++-- internal/sandbox/ssh_gpg_deny_test.go | 86 +++++++++++++++++++++++++++ internal/sandbox/ssh_key_deny.go | 19 +++--- 3 files changed, 105 insertions(+), 16 deletions(-) diff --git a/internal/sandbox/linux_helper.go b/internal/sandbox/linux_helper.go index 79544267f..dcfa0df6f 100644 --- a/internal/sandbox/linux_helper.go +++ b/internal/sandbox/linux_helper.go @@ -426,8 +426,14 @@ func appendUnreadableLinuxPaths(args []string, paths []string, carveouts []strin if !linuxCredentialParentSafeToTmpfs(parent, writeRoots) { continue } - seenParents[parent] = struct{}{} - args = appendLinuxParentTmpfsOmitting(args, parent, omits[parent]) + var applied bool + args, applied = appendLinuxParentTmpfsOmitting(args, parent, omits[parent]) + if applied { + // Record the parent only after the overlay is actually added. A + // ReadDir failure leaves the directory intact, so denied regular + // files under it still need --ro-bind /dev/null. + seenParents[parent] = struct{}{} + } } for _, file := range classified.files { parent := filepath.Clean(filepath.Dir(file)) @@ -581,11 +587,11 @@ func linuxCredentialDirPath(path string) bool { return false } -func appendLinuxParentTmpfsOmitting(args []string, parent string, omit map[string]struct{}) []string { +func appendLinuxParentTmpfsOmitting(args []string, parent string, omit map[string]struct{}) ([]string, bool) { parent = filepath.Clean(parent) entries, err := os.ReadDir(parent) if err != nil { - return args + return args, false } // 555 keeps option-2 public names (config, known_hosts, *.pub) listable // after the overlay; denied basenames are simply not rebound. @@ -606,7 +612,7 @@ func appendLinuxParentTmpfsOmitting(args []string, parent string, omit map[strin } args = append(args, "--ro-bind", sibling, sibling) } - return append(args, "--remount-ro", parent) + return append(args, "--remount-ro", parent), true } // nestedCarveoutPaths returns the carveouts that sit strictly inside root, diff --git a/internal/sandbox/ssh_gpg_deny_test.go b/internal/sandbox/ssh_gpg_deny_test.go index 949b149fb..82a504c7f 100644 --- a/internal/sandbox/ssh_gpg_deny_test.go +++ b/internal/sandbox/ssh_gpg_deny_test.go @@ -1190,3 +1190,89 @@ func TestLinuxBwrapSkipsFileBindsUnderOverlaidCredentialParent(t *testing.T) { t.Fatalf("~/.ssh was denied wholesale") } } + +func TestLinuxBwrapBindsDeniedFileWhenParentOverlayFails(t *testing.T) { + if runtime.GOOS == "windows" { + t.Skip("symlink creation is not reliably available on Windows CI") + } + home := t.TempDir() + sshDir := filepath.Join(home, ".ssh") + realDir := t.TempDir() + workTarget := filepath.Join(realDir, "work") + mustWriteFile(t, workTarget, sshPrivateKeyFixture()) + workLink := filepath.Join(sshDir, "work") + mustSymlink(t, workTarget, workLink) + idEd := filepath.Join(sshDir, "id_ed25519") + mustWriteFile(t, idEd, sshPrivateKeyFixture()) + config := filepath.Join(sshDir, "config") + mustWriteFile(t, config, "Host *\n") + + denied := sshGPGDenied(t, home, nil) + if !denyCovered(denied, workLink) { + t.Fatalf("denied symlink ~/.ssh/work missing from deny list: %v", denied) + } + if !denyCovered(denied, idEd) { + t.Fatalf("denied regular ~/.ssh/id_ed25519 missing from deny list: %v", denied) + } + + // Execute-only: Lstat of children still classifies the symlink+file, but + // ReadDir fails so the tmpfs overlay is not applied. Build the deny list + // first while the directory is readable. + if err := os.Chmod(sshDir, 0o111); err != nil { + t.Fatal(err) + } + t.Cleanup(func() { _ = os.Chmod(sshDir, 0o700) }) + if _, err := os.ReadDir(sshDir); err == nil { + t.Skip("parent ReadDir succeeded after chmod 0111 (likely running as root)") + } + + profile := PermissionProfile{ + FileSystem: FileSystemPolicy{ + Kind: FileSystemRestricted, + ReadRoots: []string{string(filepath.Separator)}, + DenyReadIfExists: denied, + }, + } + args := linuxBwrapFilesystemArgs(profile) + sshDirLex := normalizeProfilePathLexically(sshDir) + if argsContainSequence(args, "--tmpfs", sshDirLex) { + t.Fatalf("overlay must not apply when parent ReadDir fails: %#v", args) + } + assertArgsContainSequence(t, args, "--ro-bind", "/dev/null", idEd) + if denyCovered(denied, sshDir) { + t.Fatalf("~/.ssh was denied wholesale") + } +} + +func TestCredentialDenyReadPathsDeniesSSHConfigIdentityFileNamedConfigOrAuthorizedKeys(t *testing.T) { + home := t.TempDir() + sshDir := filepath.Join(home, ".ssh") + fakeConfig := filepath.Join(home, "keys", "config") + fakeAuthorized := filepath.Join(home, "keys", "authorized_keys") + realConfig := filepath.Join(sshDir, "config") + realAuthorized := filepath.Join(sshDir, "authorized_keys") + mustWriteFile(t, fakeConfig, sshPrivateKeyFixture()) + mustWriteFile(t, fakeAuthorized, sshPrivateKeyFixture()) + mustWriteFile(t, realAuthorized, "ssh-ed25519 AAAA user@host\n") + mustWriteFile(t, realConfig, "IdentityFile ~/keys/config\nIdentityFile ~/keys/authorized_keys\nUserKnownHostsFile /dev/null\n") + + denied := sshGPGDenied(t, home, nil) + if !denyCovered(denied, fakeConfig) { + t.Fatalf("IdentityFile ~/keys/config with private-key payload is readable; deny list = %v", denied) + } + if !denyCovered(denied, fakeAuthorized) { + t.Fatalf("IdentityFile ~/keys/authorized_keys with private-key payload is readable; deny list = %v", denied) + } + if denyCovered(denied, realConfig) { + t.Fatalf("real ~/.ssh/config was denied") + } + if denyCovered(denied, realAuthorized) { + t.Fatalf("real ~/.ssh/authorized_keys was denied") + } + if denyListedExact(denied, filepath.Clean("/dev/null")) || denyListedExact(denied, "/dev/null") { + t.Fatalf("/dev/null was denied from UserKnownHostsFile: %v", denied) + } + if denyCovered(denied, sshDir) { + t.Fatalf("~/.ssh was denied wholesale") + } +} diff --git a/internal/sandbox/ssh_key_deny.go b/internal/sandbox/ssh_key_deny.go index 1e8a62f1c..f66a5372c 100644 --- a/internal/sandbox/ssh_key_deny.go +++ b/internal/sandbox/ssh_key_deny.go @@ -173,14 +173,11 @@ func sshKnownHostsFamilyName(name string) bool { } func sshFileLooksLikePrivateKey(path string) bool { - // Basename-based denial still treats *.pub and known-hosts names as public, - // but a PEM/OpenSSH/PuTTY private key at those names must not stay readable. - // Sniff those payloads. Keep config / authorized_keys exemptions: - // CertificateFile and authorized_keys are never content-denied here. - switch filepath.Base(path) { - case "config", "authorized_keys", "authorized_keys2": - return false - } + // Always sniff. IdentityFile ~/keys/config (or authorized_keys / *.pub / + // known_hosts) can hold a PEM/OpenSSH/PuTTY private-key payload and must + // not stay readable. Real config, authorized_keys, public keys, and + // known-hosts files do not match these headers, so name-only exemptions + // in sshShouldDenyReferencedPath still keep genuine support files readable. data, ok := readRegularFileBounded(path, sshPrivateKeySniffBytes) if !ok { return false @@ -512,9 +509,9 @@ func sshShouldDenyReferencedPath(path, home, sshDir string) bool { return false } // Sniff before the public-name exemption so IdentityFile ~/keys/work.pub - // (or a relocated key named known_hosts) with a private-key payload is - // denied. Genuine public keys, genuine known-hosts, config, and - // authorized_keys do not match and stay readable. + // (or a relocated key named config / authorized_keys / known_hosts) with a + // private-key payload is denied. Genuine public keys, genuine known-hosts, + // config, and authorized_keys do not match and stay readable. if sshFileLooksLikePrivateKey(cleaned) { return true } From 64b0e7091d7eed725d5e03818304db87960c5377 Mon Sep 17 00:00:00 2001 From: cairn-intern Date: Tue, 1 Sep 2026 03:51:32 -0400 Subject: [PATCH 10/14] fix(sandbox): deny GNUPGHOME and unify bwrap dest spellings GnuPG's effective home is GNUPGHOME when set, but credential discovery only denied ~/.gnupg. Thread inherited and command-supplied GNUPGHOME through the existing override flow so the alternate directory and its secret-key subtree are denied, while allowRead still re-includes them. bwrap overlay and file-bind dests could mix lexical /var with canonical /private/var on macOS. Classify regular dests canonically unless a non-platform symlink is in the path, and record every parent spelling when a credential directory is tmpfs-overlaid. Extend the manager credential-deny golden with .gnupg and the well-known SSH key names. --- internal/cli/sandbox_test.go | 7 ++ internal/sandbox/linux_helper.go | 137 ++++++++++++++++++++++---- internal/sandbox/profile.go | 15 +++ internal/sandbox/ssh_gpg_deny_test.go | 48 +++++++++ 4 files changed, 190 insertions(+), 17 deletions(-) diff --git a/internal/cli/sandbox_test.go b/internal/cli/sandbox_test.go index 53ffd7b4d..f6e892786 100644 --- a/internal/cli/sandbox_test.go +++ b/internal/cli/sandbox_test.go @@ -569,6 +569,13 @@ func normalizeSandboxPolicyGoldenTempRoots(t *testing.T, gotBytes []byte, worksp wantDenyRead = []string{ filepath.Join(credentialHome, ".aws"), filepath.Join(credentialHome, ".azure"), + filepath.Join(credentialHome, ".gnupg"), + filepath.Join(credentialHome, ".ssh", "id_rsa"), + filepath.Join(credentialHome, ".ssh", "id_dsa"), + filepath.Join(credentialHome, ".ssh", "id_ecdsa"), + filepath.Join(credentialHome, ".ssh", "id_ed25519"), + filepath.Join(credentialHome, ".ssh", "id_ecdsa_sk"), + filepath.Join(credentialHome, ".ssh", "id_ed25519_sk"), // git's cleartext credential stores, in both the home and XDG // layouts (#816). Listed here so the exported policy JSON is what // catches a regression: this baseline is the contract a user reads diff --git a/internal/sandbox/linux_helper.go b/internal/sandbox/linux_helper.go index dcfa0df6f..5198d8088 100644 --- a/internal/sandbox/linux_helper.go +++ b/internal/sandbox/linux_helper.go @@ -420,24 +420,27 @@ func appendUnreadableLinuxPaths(args []string, paths []string, carveouts []strin for _, link := range classified.links { args = appendUnreadableLinuxResolvedSymlinkArgs(args, link, carveouts) parent := filepath.Clean(filepath.Dir(link)) - if _, dup := seenParents[parent]; dup { + overlayParent := linuxCanonicalDest(parent) + if linuxParentOverlaid(seenParents, overlayParent) { continue } - if !linuxCredentialParentSafeToTmpfs(parent, writeRoots) { + if !linuxCredentialParentSafeToTmpfs(overlayParent, writeRoots) && !linuxCredentialParentSafeToTmpfs(parent, writeRoots) { continue } var applied bool - args, applied = appendLinuxParentTmpfsOmitting(args, parent, omits[parent]) + args, applied = appendLinuxParentTmpfsOmitting(args, overlayParent, omits[overlayParent]) if applied { - // Record the parent only after the overlay is actually added. A - // ReadDir failure leaves the directory intact, so denied regular - // files under it still need --ro-bind /dev/null. - seenParents[parent] = struct{}{} + // Record every spelling of the parent only after the overlay is + // actually added. macOS /var vs /private/var (and similar aliases) + // must skip file binds using either form, otherwise --ro-bind + // /dev/null and --tmpfs name different dests for the same directory. + recordLinuxParentSpellings(seenParents, parent) + recordLinuxParentSpellings(seenParents, overlayParent) } } for _, file := range classified.files { parent := filepath.Clean(filepath.Dir(file)) - if _, overlaid := seenParents[parent]; overlaid { + if linuxParentOverlaid(seenParents, parent) { // Parent was already tmpfs-overlaid (symlink sibling in the same // credential dir). Re-binding /dev/null onto the regular file would // target a dest that no longer exists after the overlay and can @@ -458,35 +461,135 @@ type linuxUnreadableClassified struct { func classifyUnreadableLinuxPaths(paths []string) linuxUnreadableClassified { var out linuxUnreadableClassified seen := make(map[string]struct{}, len(paths)) - for _, path := range paths { - path = unreadableEnforcementPath(path) + add := func(bucket *[]string, path string) { if path == "" { - continue + return } if _, ok := seen[path]; ok { - continue + return } seen[path] = struct{}{} - info, err := os.Lstat(path) + *bucket = append(*bucket, path) + } + for _, path := range paths { + lexical := normalizeProfilePathLexically(path) + canonical := normalizeProfilePath(path) + inspect := lexical + if inspect == "" { + inspect = canonical + } + if inspect == "" { + continue + } + info, err := os.Lstat(inspect) + if err != nil && canonical != "" && canonical != inspect { + info, err = os.Lstat(canonical) + inspect = canonical + } if err != nil { continue } switch { case info.Mode().Type() == os.ModeSymlink: - out.links = append(out.links, path) + // Keep the lexical dentry so a later retarget still hits the dest. + add(&out.links, inspect) case info.IsDir(): - out.dirs = append(out.dirs, path) + dest := inspect + if canonical != "" && !linuxNonPlatformSymlinkInPath(inspect) { + dest = canonical + } + add(&out.dirs, dest) default: - out.files = append(out.files, path) + dest := inspect + if canonical != "" && !linuxNonPlatformSymlinkInPath(inspect) { + dest = canonical + } + add(&out.files, dest) } } return out } +func linuxCanonicalDest(path string) string { + path = filepath.Clean(path) + if canonical := normalizeProfilePath(path); canonical != "" { + return canonical + } + return path +} + +// linuxNonPlatformSymlinkInPath reports a symlink in path's resolution other +// than host aliases such as macOS /var -> /private/var. Those aliases should +// use the canonical bwrap dest so overlay and file binds name the same place. +// A credential directory symlink (for example ~/.ssh -> a store) must keep the +// lexical dest so a later retarget is still denied. +func linuxNonPlatformSymlinkInPath(path string) bool { + current := normalizeProfilePathLexically(path) + if current == "" { + current = filepath.Clean(path) + } + for { + info, err := os.Lstat(current) + if err == nil && info.Mode().Type() == os.ModeSymlink && !linuxPlatformPrefixSymlink(current) { + return true + } + parent := filepath.Dir(current) + if parent == current { + return false + } + current = parent + } +} + +func linuxPlatformPrefixSymlink(path string) bool { + switch filepath.Clean(path) { + case "/var", "/etc", "/tmp", "/private/var", "/private/etc", "/private/tmp": + return true + default: + return false + } +} + +func linuxParentSpellings(parent string) []string { + parent = filepath.Clean(parent) + seen := make(map[string]struct{}) + var out []string + add := func(path string) { + path = filepath.Clean(strings.TrimSpace(path)) + if path == "" { + return + } + if _, ok := seen[path]; ok { + return + } + seen[path] = struct{}{} + out = append(out, path) + } + add(parent) + add(normalizeProfilePathLexically(parent)) + add(normalizeProfilePath(parent)) + return out +} + +func recordLinuxParentSpellings(seen map[string]struct{}, parent string) { + for _, spelling := range linuxParentSpellings(parent) { + seen[spelling] = struct{}{} + } +} + +func linuxParentOverlaid(seen map[string]struct{}, parent string) bool { + for _, spelling := range linuxParentSpellings(parent) { + if _, ok := seen[spelling]; ok { + return true + } + } + return false +} + func linuxDeniedBasenamesByParent(files, links []string) map[string]map[string]struct{} { out := make(map[string]map[string]struct{}) add := func(path string) { - parent := filepath.Clean(filepath.Dir(path)) + parent := linuxCanonicalDest(filepath.Dir(path)) base := filepath.Base(path) m, ok := out[parent] if !ok { diff --git a/internal/sandbox/profile.go b/internal/sandbox/profile.go index 09cabbff9..7f2be1bdf 100644 --- a/internal/sandbox/profile.go +++ b/internal/sandbox/profile.go @@ -439,6 +439,7 @@ func credentialPathOptionsFromEnvironment(baseDirs []string, env []string) crede } return credentialPathOptions{ Homes: homes, + GPGHomes: resolveCredentialOverridePaths(credentialEnvValue(env, "GNUPGHOME"), baseDirs), ConfigDirs: dedupeStrings(configDirs), CloudSDKConfigDirs: dedupeStrings(cloudSDKConfigDirs), GoogleCredentials: resolveCredentialOverridePaths(credentialEnvValue(env, "GOOGLE_APPLICATION_CREDENTIALS"), baseDirs), @@ -466,6 +467,7 @@ func credentialEnvValue(env []string, key string) string { type credentialPathOptions struct { Homes []string + GPGHomes []string ConfigDirs []string CloudSDKConfigDirs []string GoogleCredentials []string @@ -537,6 +539,19 @@ func credentialDenyReadPathsIn(options credentialPathOptions, allowRead []string lexicalCandidates = append(lexicalCandidates, sshKeys...) lexicalDirs = append(lexicalDirs, gnupg) } + for _, gnupg := range options.GPGHomes { + gnupg = strings.TrimSpace(gnupg) + if gnupg == "" { + continue + } + // GnuPG's effective home is GNUPGHOME when set, not only ~/.gnupg. + // Treat it as the same directory-shaped secret store so inherited and + // command-supplied values reach DenyReadIfExists. + candidates = append(candidates, gnupg) + dirs = append(dirs, gnupg) + lexicalCandidates = append(lexicalCandidates, gnupg) + lexicalDirs = append(lexicalDirs, gnupg) + } candidates = append(candidates, options.GoogleCredentials...) candidates = append(candidates, options.NPMUserConfigs...) candidates = append(candidates, options.Netrcs...) diff --git a/internal/sandbox/ssh_gpg_deny_test.go b/internal/sandbox/ssh_gpg_deny_test.go index 82a504c7f..d882a53b6 100644 --- a/internal/sandbox/ssh_gpg_deny_test.go +++ b/internal/sandbox/ssh_gpg_deny_test.go @@ -1276,3 +1276,51 @@ func TestCredentialDenyReadPathsDeniesSSHConfigIdentityFileNamedConfigOrAuthoriz t.Fatalf("~/.ssh was denied wholesale") } } + +func TestCredentialDenyReadPathsDeniesGNUPGHOME(t *testing.T) { + if runtime.GOOS == "windows" { + t.Skip("credential deny-read is not applied on Windows") + } + home := t.TempDir() + alt := t.TempDir() + secring := filepath.Join(alt, "secring.gpg") + key := filepath.Join(alt, "private-keys-v1.d", "keygrip.key") + mustWriteFile(t, secring, "fake-secring") + mustWriteFile(t, key, "fake-keygrip") + env := []string{"HOME=" + home, "GNUPGHOME=" + alt} + + t.Run("inherited environment", func(t *testing.T) { + options := credentialPathOptionsFromEnvironment([]string{home}, env) + denied := credentialDenyReadPathsIn(options, nil).Paths + if !denyCovered(denied, alt) { + t.Fatalf("inherited GNUPGHOME is readable; deny list = %v", denied) + } + if !denyCovered(denied, secring) { + t.Fatalf("GNUPGHOME secring is readable; deny list = %v", denied) + } + if !denyCovered(denied, key) { + t.Fatalf("GNUPGHOME private-keys-v1.d is readable; deny list = %v", denied) + } + }) + + t.Run("command-supplied environment", func(t *testing.T) { + creds := credentialDenyReadPaths(Policy{}, "", env, nil) + if !denyCovered(creds.Paths, alt) { + t.Fatalf("command-supplied GNUPGHOME is readable; deny list = %v", creds.Paths) + } + if !denyCovered(creds.Paths, key) { + t.Fatalf("command-supplied GNUPGHOME subtree is readable; deny list = %v", creds.Paths) + } + }) + + t.Run("allowRead reincludes", func(t *testing.T) { + options := credentialPathOptionsFromEnvironment([]string{home}, env) + denied := credentialDenyReadPathsIn(options, []string{alt}).Paths + if denyCovered(denied, alt) { + t.Fatalf("allowRead GNUPGHOME is still denied: %v", denied) + } + if denyCovered(denied, key) { + t.Fatalf("allowRead GNUPGHOME subtree is still denied: %v", denied) + } + }) +} From 8c2e861bdf61339eb900c6ee3bc62ca581f43ab0 Mon Sep 17 00:00:00 2001 From: cairn-intern Date: Tue, 1 Sep 2026 16:16:33 -0400 Subject: [PATCH 11/14] fix(sandbox): remove unused appendUnreadableLinuxPathArgs helper --- internal/sandbox/linux_helper.go | 4 ---- 1 file changed, 4 deletions(-) diff --git a/internal/sandbox/linux_helper.go b/internal/sandbox/linux_helper.go index 5198d8088..6656ab13e 100644 --- a/internal/sandbox/linux_helper.go +++ b/internal/sandbox/linux_helper.go @@ -399,10 +399,6 @@ func appendReadOnlyLinuxPathArgs(args []string, path string) []string { return append(args, "--perms", "555", "--tmpfs", path, "--remount-ro", path) } -func appendUnreadableLinuxPathArgs(args []string, path string, carveouts []string) []string { - return appendUnreadableLinuxPaths(args, []string{path}, carveouts, nil) -} - // appendUnreadableLinuxPaths emits bwrap args that hide the given deny paths. // Directories keep the existing tmpfs mask. Regular files stay `--ro-bind // /dev/null path`. Symlink dests cannot use that bind: mount(2) LOOKUP_FOLLOW From 8620515953cf082e71eed9ac0512db2a7e4b37ba Mon Sep 17 00:00:00 2001 From: cairn-intern Date: Tue, 1 Sep 2026 17:49:21 -0400 Subject: [PATCH 12/14] fix(sandbox): normalize dangling symlink assertion and narrow tmpfs parent check --- internal/sandbox/linux_helper.go | 6 +----- internal/sandbox/ssh_gpg_deny_test.go | 1 + 2 files changed, 2 insertions(+), 5 deletions(-) diff --git a/internal/sandbox/linux_helper.go b/internal/sandbox/linux_helper.go index 6656ab13e..a088f6c16 100644 --- a/internal/sandbox/linux_helper.go +++ b/internal/sandbox/linux_helper.go @@ -651,8 +651,7 @@ func linuxCredentialParentSafeToTmpfs(parent string, writeRoots []WritableRoot) case "/tmp", "/etc", "/var", "/usr", "/home", "/root", "/opt", "/dev", "/proc", "/sys", "/run", "/mnt", "/media": return false } - switch strings.ToLower(filepath.Base(parent)) { - case "tmp", "etc", "var", "usr", "home", "root", "opt", "dev", "proc", "sys", "run": + if !linuxCredentialDirPath(parent) { return false } for _, wr := range writeRoots { @@ -661,9 +660,6 @@ func linuxCredentialParentSafeToTmpfs(parent string, writeRoots []WritableRoot) return false } } - if !linuxCredentialDirPath(parent) { - return false - } info, err := os.Lstat(parent) if err != nil || !info.IsDir() { return false diff --git a/internal/sandbox/ssh_gpg_deny_test.go b/internal/sandbox/ssh_gpg_deny_test.go index d882a53b6..a6c4fc8c1 100644 --- a/internal/sandbox/ssh_gpg_deny_test.go +++ b/internal/sandbox/ssh_gpg_deny_test.go @@ -1115,6 +1115,7 @@ func TestLinuxBwrapMasksLiveAndDanglingCredentialSymlinks(t *testing.T) { assertArgsContainSequence(t, args, "--ro-bind", "/dev/null", normalizeProfilePath(liveTarget)) assertArgsContainSequence(t, args, "--ro-bind", "/dev/null", normalizeProfilePath(gitTarget)) if argsContainSequence(args, "--ro-bind", "/dev/null", danglingTarget) || + argsContainSequence(args, "--ro-bind", "/dev/null", normalizeProfilePath(danglingTarget)) || argsContainSequence(args, "--ro-bind", "/dev/null", lexicalDangling) { t.Fatalf("dangling symlink must not be a hard --ro-bind dest: %#v", args) } From 25974e975573043bb9e4a243a66d39c54f0b38c1 Mon Sep 17 00:00:00 2001 From: cairn-intern Date: Tue, 1 Sep 2026 19:16:17 -0400 Subject: [PATCH 13/14] fix(sandbox): traverse directory symlinks, support OpenSSH escape/env syntax, and preserve granular carveouts --- internal/cli/sandbox_test.go | 84 ++++++---- internal/sandbox/linux_helper.go | 4 +- internal/sandbox/profile.go | 6 +- internal/sandbox/ssh_gpg_deny_test.go | 227 +++++++++++++++++++------- internal/sandbox/ssh_key_deny.go | 70 +++++--- 5 files changed, 272 insertions(+), 119 deletions(-) diff --git a/internal/cli/sandbox_test.go b/internal/cli/sandbox_test.go index f6e892786..5891324f6 100644 --- a/internal/cli/sandbox_test.go +++ b/internal/cli/sandbox_test.go @@ -561,34 +561,38 @@ func normalizeSandboxPolicyGoldenTempRoots(t *testing.T, gotBytes []byte, worksp profile, _ := plan["permissionProfile"].(map[string]any) fileSystem, _ := profile["fileSystem"].(map[string]any) wantDenyRead := []string(nil) - credentialHome := emptyHome if runtime.GOOS != "windows" { - if resolved, err := filepath.EvalSymlinks(emptyHome); err == nil { - credentialHome = resolved + homes := []string{emptyHome} + if resolved, err := filepath.EvalSymlinks(emptyHome); err == nil && resolved != emptyHome { + homes = append(homes, resolved) } - wantDenyRead = []string{ - filepath.Join(credentialHome, ".aws"), - filepath.Join(credentialHome, ".azure"), - filepath.Join(credentialHome, ".gnupg"), - filepath.Join(credentialHome, ".ssh", "id_rsa"), - filepath.Join(credentialHome, ".ssh", "id_dsa"), - filepath.Join(credentialHome, ".ssh", "id_ecdsa"), - filepath.Join(credentialHome, ".ssh", "id_ed25519"), - filepath.Join(credentialHome, ".ssh", "id_ecdsa_sk"), - filepath.Join(credentialHome, ".ssh", "id_ed25519_sk"), - // git's cleartext credential stores, in both the home and XDG - // layouts (#816). Listed here so the exported policy JSON is what - // catches a regression: this baseline is the contract a user reads - // with `zero sandbox policy --json`. - filepath.Join(credentialHome, ".git-credentials"), - filepath.Join(credentialHome, ".config", "git", "credentials"), - filepath.Join(credentialHome, ".npmrc"), - filepath.Join(credentialHome, ".netrc"), - filepath.Join(credentialHome, ".kube", "config"), - filepath.Join(credentialHome, ".docker", "config.json"), - filepath.Join(credentialHome, ".config", "gh", "hosts.yml"), - filepath.Join(credentialHome, ".config", "gcloud"), - filepath.Join(credentialHome, ".config", "zero"), + for _, credentialHome := range homes { + for _, rel := range []string{ + ".aws", + ".azure", + ".gnupg", + filepath.Join(".ssh", "id_rsa"), + filepath.Join(".ssh", "id_dsa"), + filepath.Join(".ssh", "id_ecdsa"), + filepath.Join(".ssh", "id_ed25519"), + filepath.Join(".ssh", "id_ecdsa_sk"), + filepath.Join(".ssh", "id_ed25519_sk"), + // git's cleartext credential stores, in both the home and XDG + // layouts (#816). Listed here so the exported policy JSON is what + // catches a regression: this baseline is the contract a user reads + // with `zero sandbox policy --json`. + ".git-credentials", + filepath.Join(".config", "git", "credentials"), + ".npmrc", + ".netrc", + filepath.Join(".kube", "config"), + filepath.Join(".docker", "config.json"), + filepath.Join(".config", "gh", "hosts.yml"), + filepath.Join(".config", "gcloud"), + filepath.Join(".config", "zero"), + } { + wantDenyRead = append(wantDenyRead, filepath.Join(credentialHome, rel)) + } } } gotDenyRead := jsonStringSlice(fileSystem["denyReadIfExists"]) @@ -600,18 +604,30 @@ func normalizeSandboxPolicyGoldenTempRoots(t *testing.T, gotBytes []byte, worksp wantCarveouts := []string(nil) wantEnsureDirs := []string(nil) if runtime.GOOS != "windows" { - zeroDir := filepath.Join(credentialHome, ".config", "zero") - wantCarveouts = []string{ - filepath.Join(zeroDir, "plugins"), - filepath.Join(zeroDir, "specialists"), - filepath.Join(zeroDir, "commands"), + homes := []string{emptyHome} + if resolved, err := filepath.EvalSymlinks(emptyHome); err == nil && resolved != emptyHome { + homes = append(homes, resolved) + } + for _, credentialHome := range homes { + zeroDir := filepath.Join(credentialHome, ".config", "zero") + wantCarveouts = append(wantCarveouts, + filepath.Join(zeroDir, "plugins"), + filepath.Join(zeroDir, "specialists"), + filepath.Join(zeroDir, "commands"), + ) + wantEnsureDirs = append(wantEnsureDirs, zeroDir) } - wantEnsureDirs = []string{zeroDir} } - if gotCarveouts := jsonStringSlice(fileSystem["denyReadCarveouts"]); !reflect.DeepEqual(gotCarveouts, wantCarveouts) { + gotCarveouts := jsonStringSlice(fileSystem["denyReadCarveouts"]) + sort.Strings(gotCarveouts) + sort.Strings(wantCarveouts) + if !reflect.DeepEqual(gotCarveouts, wantCarveouts) { t.Fatalf("manager credential carveouts = %#v, want %#v", gotCarveouts, wantCarveouts) } - if gotEnsureDirs := jsonStringSlice(fileSystem["ensureDenyReadDirs"]); !reflect.DeepEqual(gotEnsureDirs, wantEnsureDirs) { + gotEnsureDirs := jsonStringSlice(fileSystem["ensureDenyReadDirs"]) + sort.Strings(gotEnsureDirs) + sort.Strings(wantEnsureDirs) + if !reflect.DeepEqual(gotEnsureDirs, wantEnsureDirs) { t.Fatalf("manager credential ensure dirs = %#v, want %#v", gotEnsureDirs, wantEnsureDirs) } delete(fileSystem, "denyReadIfExists") diff --git a/internal/sandbox/linux_helper.go b/internal/sandbox/linux_helper.go index a088f6c16..3ba639e94 100644 --- a/internal/sandbox/linux_helper.go +++ b/internal/sandbox/linux_helper.go @@ -630,7 +630,7 @@ func appendUnreadableLinuxDirArgs(args []string, path string, carveouts []string // --remount-ro, which is what freezes the tmpfs. args = append(args, "--perms", "111", "--tmpfs", path) for _, carveout := range nested { - if info, err := os.Lstat(carveout); err == nil && info.IsDir() { + if info, err := os.Lstat(carveout); err == nil && info.Mode()&os.ModeSymlink == 0 { args = append(args, "--ro-bind", carveout, carveout) } } @@ -656,7 +656,7 @@ func linuxCredentialParentSafeToTmpfs(parent string, writeRoots []WritableRoot) } for _, wr := range writeRoots { root := filepath.Clean(strings.TrimSpace(wr.Root)) - if root != "" && parent == root { + if root != "" && (parent == root || pathWithinRoot(parent, root) || pathWithinRoot(root, parent)) { return false } } diff --git a/internal/sandbox/profile.go b/internal/sandbox/profile.go index 7f2be1bdf..da662758e 100644 --- a/internal/sandbox/profile.go +++ b/internal/sandbox/profile.go @@ -883,10 +883,10 @@ func normalizeCredentialCarveoutPath(entry string) string { return "" } // A missing fixed subtree may be installed later by trusted host code, but - // an existing entry must be a real directory. In particular, never turn a - // plugins symlink into an allow rule for its credential-file target. + // an existing entry must be a real directory or regular file. Never turn a + // symlink into an allow rule for its credential target. if info, err := os.Lstat(carveout); err == nil { - if !info.IsDir() { + if info.Mode()&os.ModeSymlink != 0 { return "" } } else if !os.IsNotExist(err) { diff --git a/internal/sandbox/ssh_gpg_deny_test.go b/internal/sandbox/ssh_gpg_deny_test.go index a6c4fc8c1..360910884 100644 --- a/internal/sandbox/ssh_gpg_deny_test.go +++ b/internal/sandbox/ssh_gpg_deny_test.go @@ -749,7 +749,7 @@ func TestWalkSSHPrivateKeyFilesDeniesCustomNamedSymlinkToPrivateKey(t *testing.T } } -func TestCredentialDenyReadPathsNestedGPGAllowReadOmitsParentDir(t *testing.T) { +func TestCredentialDenyReadPathsNestedGPGAllowReadKeepsParentDirAndCarvesOut(t *testing.T) { home := t.TempDir() key := filepath.Join(home, ".gnupg", "private-keys-v1.d", "keygrip.key") mustWriteFile(t, key, "fake-keygrip") @@ -759,11 +759,8 @@ func TestCredentialDenyReadPathsNestedGPGAllowReadOmitsParentDir(t *testing.T) { allow := []string{key} denied := sshGPGDenied(t, home, allow) gnupg := normalizeProfilePath(filepath.Join(home, ".gnupg")) - if denyListedExact(denied, gnupg) { - t.Fatalf("nested allowRead left parent ~/.gnupg in DenyReadIfExists: %v", denied) - } - if denyCovered(denied, key) { - t.Fatalf("nested allowRead key is still denied: %v", denied) + if !denyListedExact(denied, gnupg) { + t.Fatalf("nested allowRead must keep parent ~/.gnupg in DenyReadIfExists: %v", denied) } if !denyCovered(denied, filepath.Join(home, ".git-credentials")) { t.Fatalf("git-credentials must stay denied when only a nested GPG key is allowed: %v", denied) @@ -776,8 +773,9 @@ func TestCredentialDenyReadPathsNestedGPGAllowReadOmitsParentDir(t *testing.T) { func TestLinuxBwrapAndSeatbeltHonorNestedGPGAllowRead(t *testing.T) { home := t.TempDir() key := filepath.Join(home, ".gnupg", "private-keys-v1.d", "keygrip.key") + secring := filepath.Join(home, ".gnupg", "secring.gpg") mustWriteFile(t, key, "fake-keygrip") - mustWriteFile(t, filepath.Join(home, ".gnupg", "secring.gpg"), "fake-secring") + mustWriteFile(t, secring, "fake-secring") allow := []string{key} creds := credentialDenyReadPathsIn(credentialPathOptions{ @@ -785,11 +783,14 @@ func TestLinuxBwrapAndSeatbeltHonorNestedGPGAllowRead(t *testing.T) { ConfigDirs: []string{filepath.Join(home, ".config")}, }, allow) gnupg := normalizeProfilePath(filepath.Join(home, ".gnupg")) - if denyListedExact(creds.Paths, gnupg) { - t.Fatalf("nested allowRead left parent ~/.gnupg in DenyReadIfExists: %v", creds.Paths) + if !denyListedExact(creds.Paths, gnupg) { + t.Fatalf("nested allowRead must keep parent ~/.gnupg in DenyReadIfExists: %v", creds.Paths) + } + if !denyListedExact(creds.Carveouts, normalizeProfilePath(key)) { + t.Fatalf("nested allowRead key must be in DenyReadCarveouts: %v", creds.Carveouts) } - if denyCovered(creds.Paths, key) { - t.Fatalf("nested allowRead key is still denied: %v", creds.Paths) + if denyCovered(creds.Carveouts, secring) { + t.Fatalf("secring.gpg was unexpectedly carved out: %v", creds.Carveouts) } profile := PermissionProfile{ @@ -801,25 +802,28 @@ func TestLinuxBwrapAndSeatbeltHonorNestedGPGAllowRead(t *testing.T) { }, } args := linuxBwrapFilesystemArgs(profile) - if argsContainSequence(args, "--perms", "000", "--tmpfs", gnupg) || - argsContainSequence(args, "--perms", "111", "--tmpfs", gnupg) || - argsContainSequence(args, "--ro-bind", "/dev/null", gnupg) { - t.Fatalf("bwrap masked ~/.gnupg despite nested allowRead: %#v", args) + if !argsContainSequence(args, "--perms", "111", "--tmpfs", gnupg) { + t.Fatalf("bwrap should tmpfs-mask ~/.gnupg to protect sibling secrets: %#v", args) } - - sbpl := strings.Join(denyReadRules(profile.FileSystem), "\n") - if strings.Contains(sbpl, sandboxProfileString(gnupg)) { - t.Fatalf("Seatbelt deny rules still cover ~/.gnupg after nested allowRead:\n%s", sbpl) + if !argsContainSequence(args, "--ro-bind", key, key) { + t.Fatalf("bwrap should --ro-bind the carved-out key: %#v", args) } - keyLit := sandboxProfileString(normalizeProfilePath(key)) - if strings.Contains(sbpl, `(deny file-read* (literal "`+keyLit+`"))`) || - strings.Contains(sbpl, `(deny file-read* (subpath "`+keyLit+`"))`) { - t.Fatalf("Seatbelt still denies the nested allowRead key:\n%s", sbpl) + if argsContainSequence(args, "--ro-bind", secring, secring) { + t.Fatalf("bwrap unexpectedly rebound secring: %#v", args) } + full := seatbeltProfileFromPermissionProfile(profile, Policy{}, "") denyIdx := strings.LastIndex(full, `(deny file-read* (subpath "`+sandboxProfileString(gnupg)+`"))`) - if denyIdx >= 0 { - t.Fatalf("full Seatbelt profile still denies ~/.gnupg subtree:\n%s", full) + if denyIdx < 0 { + t.Fatalf("full Seatbelt profile must deny ~/.gnupg subtree to protect sibling secrets:\n%s", full) + } + keyLit := sandboxProfileString(normalizeProfilePath(key)) + allowIdx := strings.LastIndex(full, `(allow file-read* file-test-existence (literal "`+keyLit+`"))`) + if allowIdx < 0 || allowIdx < denyIdx { + t.Fatalf("Seatbelt profile must allow the carved-out key AFTER the parent deny rule:\n%s", full) + } + if strings.Contains(full, `(allow file-read* file-test-existence (literal "`+sandboxProfileString(secring)+`"))`) { + t.Fatalf("Seatbelt profile must not allow sibling secring.gpg:\n%s", full) } } @@ -945,8 +949,9 @@ func TestLinuxBwrapAndSeatbeltHonorNestedGPGAllowReadThroughDirSymlink(t *testin home := t.TempDir() gnupgTarget := filepath.Join(t.TempDir(), "gnupg-store") key := filepath.Join(gnupgTarget, "private-keys-v1.d", "keygrip.key") + secring := filepath.Join(gnupgTarget, "secring.gpg") mustWriteFile(t, key, "fake-keygrip") - mustWriteFile(t, filepath.Join(gnupgTarget, "secring.gpg"), "fake-secring") + mustWriteFile(t, secring, "fake-secring") mustSymlink(t, gnupgTarget, filepath.Join(home, ".gnupg")) allow := []string{key} @@ -954,16 +959,15 @@ func TestLinuxBwrapAndSeatbeltHonorNestedGPGAllowReadThroughDirSymlink(t *testin Homes: []string{home}, ConfigDirs: []string{filepath.Join(home, ".config")}, }, allow) - lexicalGnupg := normalizeProfilePathLexically(filepath.Join(home, ".gnupg")) canonicalGnupg := normalizeProfilePath(filepath.Join(home, ".gnupg")) - if denyListedExact(creds.Paths, lexicalGnupg) { - t.Fatalf("lexical ~/.gnupg dir deny retained despite nested canonical allowRead: %v", creds.Paths) + if canonicalGnupg == "" || !denyListedExact(creds.Paths, canonicalGnupg) { + t.Fatalf("canonical ~/.gnupg dir deny must be retained: %v", creds.Paths) } - if canonicalGnupg != "" && denyListedExact(creds.Paths, canonicalGnupg) { - t.Fatalf("canonical ~/.gnupg dir deny retained despite nested allowRead: %v", creds.Paths) + if !denyListedExact(creds.Carveouts, normalizeProfilePath(key)) { + t.Fatalf("nested allowRead key must be in DenyReadCarveouts: %v", creds.Carveouts) } - if denyCovered(creds.Paths, key) { - t.Fatalf("nested allowRead key is still denied: %v", creds.Paths) + if denyCovered(creds.Carveouts, secring) { + t.Fatalf("secring.gpg must not be carved out: %v", creds.Carveouts) } profile := PermissionProfile{ @@ -975,34 +979,28 @@ func TestLinuxBwrapAndSeatbeltHonorNestedGPGAllowReadThroughDirSymlink(t *testin }, } args := linuxBwrapFilesystemArgs(profile) - if argsContainSequence(args, "--perms", "000", "--tmpfs", lexicalGnupg) || - argsContainSequence(args, "--perms", "111", "--tmpfs", lexicalGnupg) || - argsContainSequence(args, "--perms", "555", "--tmpfs", lexicalGnupg) || - argsContainSequence(args, "--ro-bind", "/dev/null", lexicalGnupg) || - (canonicalGnupg != "" && (argsContainSequence(args, "--perms", "000", "--tmpfs", canonicalGnupg) || - argsContainSequence(args, "--perms", "111", "--tmpfs", canonicalGnupg) || - argsContainSequence(args, "--ro-bind", "/dev/null", canonicalGnupg))) { - t.Fatalf("bwrap masked ~/.gnupg despite nested allowRead under dir symlink: %#v", args) + if !argsContainSequence(args, "--perms", "111", "--tmpfs", canonicalGnupg) { + t.Fatalf("bwrap should tmpfs-mask canonical gnupg to protect sibling secrets: %#v", args) } - - sbpl := strings.Join(denyReadRules(profile.FileSystem), "\n") - if strings.Contains(sbpl, sandboxProfileString(lexicalGnupg)) { - t.Fatalf("Seatbelt deny rules still cover lexical ~/.gnupg after nested allowRead:\n%s", sbpl) + if !argsContainSequence(args, "--ro-bind", key, key) { + t.Fatalf("bwrap should --ro-bind carved-out key: %#v", args) + } + if argsContainSequence(args, "--ro-bind", secring, secring) { + t.Fatalf("bwrap unexpectedly rebound secring: %#v", args) } - if canonicalGnupg != "" && strings.Contains(sbpl, sandboxProfileString(canonicalGnupg)) { - t.Fatalf("Seatbelt deny rules still cover canonical ~/.gnupg after nested allowRead:\n%s", sbpl) + + full := seatbeltProfileFromPermissionProfile(profile, Policy{}, "") + denyIdx := strings.LastIndex(full, `(deny file-read* (subpath "`+sandboxProfileString(canonicalGnupg)+`"))`) + if denyIdx < 0 { + t.Fatalf("full Seatbelt profile must deny canonical ~/.gnupg subtree:\n%s", full) } keyLit := sandboxProfileString(normalizeProfilePath(key)) - if strings.Contains(sbpl, `(deny file-read* (literal "`+keyLit+`"))`) || - strings.Contains(sbpl, `(deny file-read* (subpath "`+keyLit+`"))`) { - t.Fatalf("Seatbelt still denies the nested allowRead key:\n%s", sbpl) + allowIdx := strings.LastIndex(full, `(allow file-read* file-test-existence (literal "`+keyLit+`"))`) + if allowIdx < 0 || allowIdx < denyIdx { + t.Fatalf("Seatbelt profile must allow the carved-out key AFTER the parent deny rule:\n%s", full) } - full := seatbeltProfileFromPermissionProfile(profile, Policy{}, "") - if canonicalGnupg != "" { - denyIdx := strings.LastIndex(full, `(deny file-read* (subpath "`+sandboxProfileString(canonicalGnupg)+`"))`) - if denyIdx >= 0 { - t.Fatalf("full Seatbelt profile still denies canonical ~/.gnupg subtree:\n%s", full) - } + if strings.Contains(full, `(allow file-read* file-test-existence (literal "`+sandboxProfileString(secring)+`"))`) { + t.Fatalf("Seatbelt profile must not allow sibling secring.gpg:\n%s", full) } } @@ -1325,3 +1323,120 @@ func TestCredentialDenyReadPathsDeniesGNUPGHOME(t *testing.T) { } }) } + +func TestCredentialDenyReadPathsTraversesNestedDirectorySymlink(t *testing.T) { + if runtime.GOOS == "windows" { + t.Skip("credential deny-read is not applied on Windows") + } + home := t.TempDir() + sshDir := filepath.Join(home, ".ssh") + keyStore := t.TempDir() + workKey := filepath.Join(keyStore, "work") + mustWriteFile(t, workKey, sshPrivateKeyFixture()) + + // Symlink ~/.ssh/keys -> keyStore + mustSymlink(t, keyStore, filepath.Join(sshDir, "keys")) + mustWriteFile(t, filepath.Join(sshDir, "config"), "Host *\n") + + denied := sshGPGDenied(t, home, nil) + lexicalTarget := filepath.Join(sshDir, "keys", "work") + if !denyCovered(denied, lexicalTarget) && !denyCovered(denied, workKey) { + t.Fatalf("key reachable through directory symlink was not denied; deny list = %v", denied) + } + if denyCovered(denied, filepath.Join(sshDir, "config")) { + t.Fatalf("~/.ssh/config was unexpectedly denied: %v", denied) + } + if denyCovered(denied, sshDir) { + t.Fatalf("~/.ssh was denied wholesale: %v", denied) + } +} + +func TestOpenSSHPathParsingEscapesAndEnv(t *testing.T) { + home, sshDir := sshGPGNormalizationHome() + t.Setenv("SSH_KEY_DIR", filepath.Join(home, "secret-keys")) + + t.Run("unquoted escaped spaces", func(t *testing.T) { + tokens := splitSSHTokens(`IdentityFile ~/My\ Keys/work`) + if len(tokens) != 2 || tokens[0] != "IdentityFile" || tokens[1] != "~/My Keys/work" { + t.Fatalf("splitSSHTokens unexpected tokens: %#v", tokens) + } + }) + + t.Run("environment variable expansion", func(t *testing.T) { + got := expandSSHConfigPath("${SSH_KEY_DIR}/work", home, sshDir) + want := filepath.Join(home, "secret-keys", "work") + if got != want { + t.Fatalf("expandSSHConfigPath(${SSH_KEY_DIR}) = %q, want %q", got, want) + } + }) + + t.Run("unresolvable variable dropped", func(t *testing.T) { + got := expandSSHConfigPath("${DEFINITELY_UNSET_VAR_XYZ}/work", home, sshDir) + if got != "" { + t.Fatalf("expected unset variable to be dropped, got %q", got) + } + }) +} + +func TestAllowReadSingleFileInsideGNUPGPreservesSiblingDenies(t *testing.T) { + if runtime.GOOS == "windows" { + t.Skip("credential deny-read is not applied on Windows") + } + home := t.TempDir() + gnupgDir := filepath.Join(home, ".gnupg") + publicFile := filepath.Join(gnupgDir, "public.txt") + secringFile := filepath.Join(gnupgDir, "secring.gpg") + keyFile := filepath.Join(gnupgDir, "private-keys-v1.d", "keygrip.key") + + mustWriteFile(t, publicFile, "public info") + mustWriteFile(t, secringFile, "secret keyring") + mustWriteFile(t, keyFile, "secret keygrip") + + options := credentialPathOptions{ + Homes: []string{home}, + ConfigDirs: []string{filepath.Join(home, ".config")}, + } + creds := credentialDenyReadPathsIn(options, []string{publicFile}) + + // 1. .gnupg must remain denied as a directory root + if !denyCovered(creds.Paths, gnupgDir) { + t.Fatalf("expected .gnupg directory to remain in deny list, got %v", creds.Paths) + } + + // 2. publicFile must be present in Carveouts + if !denyListedExact(creds.Carveouts, publicFile) && !denyCovered(creds.Carveouts, publicFile) { + t.Fatalf("expected publicFile in Carveouts, got %v", creds.Carveouts) + } + + // 3. Sibling secrets must NOT be in Carveouts + if denyCovered(creds.Carveouts, secringFile) || denyCovered(creds.Carveouts, keyFile) { + t.Fatalf("sibling secrets unexpectedly carved out: %v", creds.Carveouts) + } + + // 4. In Seatbelt profile: verify public.txt has allow rule, while secring stays denied + fs := FileSystemPolicy{ + DenyReadIfExists: creds.Paths, + DenyReadCarveouts: creds.Carveouts, + } + sbRules := strings.Join(denyReadCarveoutRules(fs), "\n") + if !strings.Contains(sbRules, publicFile) { + t.Fatalf("seatbelt rules missing allow for public file: %s", sbRules) + } + if strings.Contains(sbRules, secringFile) { + t.Fatalf("seatbelt rules allow sibling secret: %s", sbRules) + } +} + +func TestLinuxHelperCredentialParentTmpfsRejectsNestedWriteRoots(t *testing.T) { + home := t.TempDir() + sshDir := filepath.Join(home, ".ssh") + if err := os.MkdirAll(sshDir, 0o700); err != nil { + t.Fatal(err) + } + writeRoots := []WritableRoot{ + {Root: filepath.Join(sshDir, "project")}, + } + if linuxCredentialParentSafeToTmpfs(sshDir, writeRoots) { + t.Fatal("expected linuxCredentialParentSafeToTmpfs to reject parent containing nested write root") + } +} diff --git a/internal/sandbox/ssh_key_deny.go b/internal/sandbox/ssh_key_deny.go index f66a5372c..8850a3c21 100644 --- a/internal/sandbox/ssh_key_deny.go +++ b/internal/sandbox/ssh_key_deny.go @@ -78,11 +78,21 @@ func sshPrivateKeyDenyCandidates(home string) []string { func walkSSHPrivateKeyFiles(sshDir string) []string { var out []string + visitedDirs := make(map[string]bool) var walk func(dir string, depth int) walk = func(dir string, depth int) { if depth > sshPrivateKeyWalkMaxDepth { return } + realDir := dir + if resolved, err := filepath.EvalSymlinks(dir); err == nil { + realDir = resolved + } + if visitedDirs[realDir] { + return + } + visitedDirs[realDir] = true + d, err := os.Open(dir) if err != nil { return @@ -112,9 +122,13 @@ func walkSSHPrivateKeyFiles(sshDir string) []string { } mode := info.Mode() if mode.Type() == os.ModeSymlink { + targetStat, err := os.Stat(path) + if err == nil && targetStat.IsDir() { + walk(path, depth+1) + continue + } // Inspect leaf symlinks (bounded, specials rejected) so a // custom-named link to a PEM/OpenSSH key is still denied. - // Directory symlinks are not traversed. if isSSHPrivateKeyFileName(name) || sshFileLooksLikePrivateKey(path) { out = append(out, path) } @@ -357,6 +371,11 @@ func splitSSHTokens(s string) []string { cur.WriteByte(c) continue } + if c == '\\' && i+1 < len(s) { + cur.WriteByte(s[i+1]) + i++ + continue + } switch c { case '\'', '"': inQuote = c @@ -378,10 +397,10 @@ func expandSSHConfigPath(value, home, sshDir string) string { if value == "" || strings.EqualFold(value, "none") || strings.EqualFold(value, "SSH_AUTH_SOCK") { return "" } - // OpenSSH expands environment variables in IdentityFile. Only ${HOME}/$HOME - // from the supplied home argument (never process env). Unknown $VAR is - // treated like an unsupported percent token: drop the path so we never deny - // or follow an unresolved pattern. + // OpenSSH expands environment variables in IdentityFile. ${HOME}/$HOME + // resolves to the supplied home argument. Other variables resolve from the + // process environment. Unset or invalid $VAR is treated like an unsupported + // token: drop the path so we never deny or follow an unresolved pattern. expandedEnv, ok := expandSSHConfigPathEnv(value, home) if !ok { return "" @@ -405,9 +424,9 @@ func expandSSHConfigPath(value, home, sshDir string) string { } } -// expandSSHConfigPathEnv resolves ${HOME} and $HOME from the supplied home -// argument. Any other ${VAR}/$VAR, a dangling $, or a malformed ${...} drops -// the path. No live process environment map is consulted. +// expandSSHConfigPathEnv resolves ${VAR} and $VAR. ${HOME} and $HOME resolve +// to the supplied home argument. Other variables resolve from the environment. +// An undefined variable, dangling $, or malformed ${...} drops the path. func expandSSHConfigPathEnv(value, home string) (string, bool) { if !strings.Contains(value, "$") { return value, true @@ -422,31 +441,34 @@ func expandSSHConfigPathEnv(value, home string) (string, bool) { if i+1 >= len(value) { return "", false } + var name string if value[i+1] == '{' { end := strings.IndexByte(value[i+2:], '}') if end < 0 { return "", false } - name := value[i+2 : i+2+end] - if name != "HOME" { + name = value[i+2 : i+2+end] + i += 2 + end + } else { + if !sshEnvVarStart(value[i+1]) { return "", false } - b.WriteString(home) - i += 2 + end - continue - } - if !sshEnvVarStart(value[i+1]) { - return "", false - } - j := i + 1 - for j < len(value) && sshEnvVarChar(value[j]) { - j++ + j := i + 1 + for j < len(value) && sshEnvVarChar(value[j]) { + j++ + } + name = value[i+1 : j] + i = j - 1 } - if value[i+1:j] != "HOME" { - return "", false + if name == "HOME" { + b.WriteString(home) + } else { + val := os.Getenv(name) + if val == "" { + return "", false + } + b.WriteString(val) } - b.WriteString(home) - i = j - 1 } return b.String(), true } From eb1966302e72949e11abbcca4487c3f932424e21 Mon Sep 17 00:00:00 2001 From: cairn-intern Date: Wed, 2 Sep 2026 04:57:31 -0400 Subject: [PATCH 14/14] test(sandbox): skip symlink tests gracefully when symlinks are unpermitted --- internal/sandbox/ssh_gpg_deny_test.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/internal/sandbox/ssh_gpg_deny_test.go b/internal/sandbox/ssh_gpg_deny_test.go index 360910884..2f95076fa 100644 --- a/internal/sandbox/ssh_gpg_deny_test.go +++ b/internal/sandbox/ssh_gpg_deny_test.go @@ -45,7 +45,7 @@ func mustSymlink(t *testing.T, target, link string) { t.Fatal(err) } if err := os.Symlink(target, link); err != nil { - t.Fatal(err) + t.Skipf("symlinks not supported or permitted in this environment: %v", err) } }