diff --git a/internal/cli/daemon.go b/internal/cli/daemon.go index 5074312e4..e7e563474 100644 --- a/internal/cli/daemon.go +++ b/internal/cli/daemon.go @@ -63,6 +63,7 @@ Commands: daemon. Requires a bearer token in $ZERO_DAEMON_REMOTE_TOKEN (or $ZERO_DAEMON_REMOTE_TOKEN_FILE). --bundle-dir enables git-bundle uploads, extracted into per-link work trees. + An inline token takes precedence, so a stale token-file pointer is intentionally not protected as the live credential. link --remote --repo --id [--out ] Upload repo's git history to the remote as a bundle and print the extracted remote path. --out saves a session @@ -477,6 +478,12 @@ func runDaemonServeRemote(args []string, stdout io.Writer, stderr io.Writer) int if err != nil { return writeAppError(stderr, err.Error(), exitCrash) } + // Pin the token file to the path this process reads BEFORE any worker + // inherits the variable, so a relative or symlinked value cannot make a + // session's sandbox profile protect a different path than the live bearer file. + if err := remote.CanonicalizeTokenFileEnv(); err != nil { + return writeAppError(stderr, err.Error(), exitCrash) + } token, err := remote.TokenFromEnv() if err != nil { return writeAppError(stderr, err.Error(), exitCrash) diff --git a/internal/cli/daemon_test.go b/internal/cli/daemon_test.go index 5418537cc..ee01cd207 100644 --- a/internal/cli/daemon_test.go +++ b/internal/cli/daemon_test.go @@ -2,8 +2,19 @@ package cli import ( "bytes" + "crypto/ecdsa" + "crypto/elliptic" + "crypto/rand" + "crypto/x509" + "crypto/x509/pkix" + "encoding/pem" + "math/big" + "net" + "os" + "path/filepath" "strings" "testing" + "time" ) // isolateDaemonPaths points DefaultPaths at a temp dir so the test never touches @@ -121,3 +132,76 @@ func TestDaemonSubcommandsRejectExtraArgs(t *testing.T) { } } } + +func TestDaemonServeRemoteCanonicalizesTokenFileBeforeStartingWorkers(t *testing.T) { + isolateDaemonPaths(t) + certFile, keyFile := writeDaemonTestCertificate(t) + startDir := t.TempDir() + t.Chdir(startDir) + if err := os.WriteFile("token", []byte("bridge-token"), 0o600); err != nil { + t.Fatal(err) + } + t.Setenv("ZERO_DAEMON_REMOTE_TOKEN", "") + t.Setenv("ZERO_DAEMON_REMOTE_TOKEN_FILE", "token") + + code, _, _ := runDaemonCLI(t, "serve-remote", "--addr", "127.0.0.1:not-a-port", "--tls-cert", certFile, "--tls-key", keyFile) + if code != exitCrash { + t.Fatalf("serve-remote exit = %d, want bind failure", code) + } + want := filepath.Join(startDir, "token") + want, err := filepath.EvalSymlinks(want) + if err != nil { + t.Fatalf("EvalSymlinks(%q): %v", want, err) + } + if got := os.Getenv("ZERO_DAEMON_REMOTE_TOKEN_FILE"); got != want { + t.Fatalf("ZERO_DAEMON_REMOTE_TOKEN_FILE = %q, want daemon-pinned path %q", got, want) + } +} + +func writeDaemonTestCertificate(t *testing.T) (string, string) { + t.Helper() + key, err := ecdsa.GenerateKey(elliptic.P256(), rand.Reader) + if err != nil { + t.Fatal(err) + } + template := &x509.Certificate{ + SerialNumber: big.NewInt(1), + Subject: pkix.Name{CommonName: "zero-daemon-test"}, + NotBefore: time.Now().Add(-time.Hour), + NotAfter: time.Now().Add(time.Hour), + KeyUsage: x509.KeyUsageDigitalSignature, + ExtKeyUsage: []x509.ExtKeyUsage{x509.ExtKeyUsageServerAuth}, + IPAddresses: []net.IP{net.ParseIP("127.0.0.1")}, + } + der, err := x509.CreateCertificate(rand.Reader, template, template, &key.PublicKey, key) + if err != nil { + t.Fatal(err) + } + dir := t.TempDir() + certFile, keyFile := filepath.Join(dir, "cert.pem"), filepath.Join(dir, "key.pem") + certOut, err := os.Create(certFile) + if err != nil { + t.Fatal(err) + } + if err := pem.Encode(certOut, &pem.Block{Type: "CERTIFICATE", Bytes: der}); err != nil { + t.Fatal(err) + } + if err := certOut.Close(); err != nil { + t.Fatal(err) + } + keyDER, err := x509.MarshalECPrivateKey(key) + if err != nil { + t.Fatal(err) + } + keyOut, err := os.Create(keyFile) + if err != nil { + t.Fatal(err) + } + if err := pem.Encode(keyOut, &pem.Block{Type: "EC PRIVATE KEY", Bytes: keyDER}); err != nil { + t.Fatal(err) + } + if err := keyOut.Close(); err != nil { + t.Fatal(err) + } + return certFile, keyFile +} diff --git a/internal/daemon/remote/auth.go b/internal/daemon/remote/auth.go index 4f1574a9d..b1cedd455 100644 --- a/internal/daemon/remote/auth.go +++ b/internal/daemon/remote/auth.go @@ -23,6 +23,7 @@ import ( "fmt" "io" "os" + "path/filepath" "strings" "github.com/Gitlawb/zero/internal/daemon" @@ -66,13 +67,30 @@ func (a *TokenAuthenticator) Authenticate(token string) error { return ErrUnauthorized } +// TokenFilePathFromEnv returns the configured token-file pointer exactly as the +// operator set it, or "" when the variable is unset or holds only whitespace. +// +// The value is a PATHNAME, and every consumer of it — os.ReadFile here, the +// canonicalization below, the sandbox's protected-credential list — must agree +// on which bytes name the file. A filename may legitimately begin or end with a +// space, so trimming the value would make this boundary read one file while the +// deny rules protect another. A value that is only whitespace still reads as +// unset, which is what a blank variable means. +func TokenFilePathFromEnv() string { + configured := os.Getenv(EnvTokenFile) + if strings.TrimSpace(configured) == "" { + return "" + } + return configured +} + // TokenFromEnv resolves the bridge token from EnvToken, or a file named by // EnvTokenFile. It never logs the token. func TokenFromEnv() (string, error) { if t := strings.TrimSpace(os.Getenv(EnvToken)); t != "" { return t, nil } - if file := strings.TrimSpace(os.Getenv(EnvTokenFile)); file != "" { + if file := TokenFilePathFromEnv(); file != "" { data, err := os.ReadFile(file) if err != nil { return "", fmt.Errorf("remote: read token file: %w", err) @@ -86,6 +104,47 @@ func TokenFromEnv() (string, error) { return "", fmt.Errorf("remote: set %s or %s", EnvToken, EnvTokenFile) } +// CanonicalizeTokenFileEnv rewrites EnvTokenFile in this process's environment +// to the absolute, symlink-resolved path TokenFromEnv actually reads, so every +// child process — and the sandbox profile derived for it — refers to the same +// file this bridge authenticated against. `zero daemon serve-remote` calls it +// before it starts serving. +// +// Two mismatches motivate it. TokenFromEnv passes the value to os.ReadFile, so a +// relative value resolves against the STARTING process's working directory, +// while a worker inherits the same string and resolves it against its own +// session directory — the profile would then protect a path that holds no token +// while the real bearer file stays readable. Resolving symlinks up front also +// keeps a link pathname out of the derived deny rules, which matters because +// bubblewrap cannot mount over a symlink destination. +// +// It is a deliberate no-op when EnvToken supplies the token: TokenFromEnv +// prefers the inline value, so an unused (even dangling) file pointer must not +// change the outcome or fail the start. +func CanonicalizeTokenFileEnv() error { + if strings.TrimSpace(os.Getenv(EnvToken)) != "" { + return nil + } + configured := TokenFilePathFromEnv() + if configured == "" { + return nil + } + absolute, err := filepath.Abs(configured) + if err != nil { + return fmt.Errorf("remote: resolve token file %q: %w", configured, err) + } + resolved, err := filepath.EvalSymlinks(absolute) + if err != nil { + // TokenFromEnv would fail on the same path a moment later; reporting it here + // names the resolution step that failed. + return fmt.Errorf("remote: resolve token file %q: %w", configured, err) + } + if resolved == configured { + return nil + } + return os.Setenv(EnvTokenFile, resolved) +} + // Attestation is an optional post-token hook (e.g. workload attestation). The // default is a no-op; a deployment can supply a stricter implementation. type Attestation interface { diff --git a/internal/daemon/remote/auth_test.go b/internal/daemon/remote/auth_test.go index 448a6d920..0ba660df4 100644 --- a/internal/daemon/remote/auth_test.go +++ b/internal/daemon/remote/auth_test.go @@ -4,6 +4,7 @@ import ( "errors" "os" "path/filepath" + "runtime" "testing" ) @@ -57,6 +58,142 @@ func TestTokenFromEnv(t *testing.T) { } } +// TestTokenFilePathFromEnvPreservesFilenameWhitespace pins the pointer as a +// pathname rather than a trimmed word. Trimming it made this boundary select +// "/bridge-token" while the operator had named "/bridge-token " — +// which fails the daemon start at best, and at worst reads and protects a +// different file than the one holding the bearer token. +func TestTokenFilePathFromEnvPreservesFilenameWhitespace(t *testing.T) { + t.Setenv(EnvToken, "") + for _, configured := range []string{"/srv/zero/bridge-token ", " /srv/zero/bridge-token", "/srv/zero/ token "} { + t.Setenv(EnvTokenFile, configured) + if got := TokenFilePathFromEnv(); got != configured { + t.Fatalf("TokenFilePathFromEnv() = %q, want the configured pathname %q", got, configured) + } + } + for _, blank := range []string{"", " ", "\t\n"} { + t.Setenv(EnvTokenFile, blank) + if got := TokenFilePathFromEnv(); got != "" { + t.Fatalf("TokenFilePathFromEnv() = %q for a blank value, want unset", got) + } + } +} + +// TestCanonicalizeTokenFileEnvKeepsTrailingSpaceFilename covers the same rule at +// the daemon boundary: the file the bridge authenticates against must survive +// canonicalization byte for byte. +func TestCanonicalizeTokenFileEnvKeepsTrailingSpaceFilename(t *testing.T) { + if runtime.GOOS == "windows" { + t.Skip("Windows filenames cannot end in a space") + } + base, err := filepath.EvalSymlinks(t.TempDir()) + if err != nil { + t.Fatalf("EvalSymlinks: %v", err) + } + token := filepath.Join(base, "bridge-token ") + if err := os.WriteFile(token, []byte("from-file\n"), 0o600); err != nil { + t.Fatalf("write token file: %v", err) + } + t.Chdir(base) + t.Setenv(EnvToken, "") + t.Setenv(EnvTokenFile, "bridge-token ") + + if err := CanonicalizeTokenFileEnv(); err != nil { + t.Fatalf("CanonicalizeTokenFileEnv: %v", err) + } + if got := os.Getenv(EnvTokenFile); got != token { + t.Fatalf("%s = %q, want %q", EnvTokenFile, got, token) + } + t.Chdir(t.TempDir()) + if tok, err := TokenFromEnv(); err != nil || tok != "from-file" { + t.Fatalf("TokenFromEnv after canonicalization = %q, %v", tok, err) + } +} + +// TestCanonicalizeTokenFileEnv pins the value every child process (and the +// sandbox profile derived for it) inherits to the file this process reads. +func TestCanonicalizeTokenFileEnv(t *testing.T) { + base, err := filepath.EvalSymlinks(t.TempDir()) + if err != nil { + t.Fatalf("EvalSymlinks: %v", err) + } + token := filepath.Join(base, "tok") + if err := os.WriteFile(token, []byte("from-file\n"), 0o600); err != nil { + t.Fatalf("write token file: %v", err) + } + + t.Run("relative value becomes absolute", func(t *testing.T) { + // A worker resolves the inherited value against its own session directory, + // so a relative value must not survive the daemon boundary. + t.Chdir(base) + t.Setenv(EnvToken, "") + t.Setenv(EnvTokenFile, "tok") + if err := CanonicalizeTokenFileEnv(); err != nil { + t.Fatalf("CanonicalizeTokenFileEnv: %v", err) + } + if got := os.Getenv(EnvTokenFile); got != token { + t.Fatalf("%s = %q, want %q", EnvTokenFile, got, token) + } + // Workers run from session directories, so prove the selected path remains + // pinned after crossing that boundary rather than merely inspecting the env. + t.Chdir(t.TempDir()) + if tok, err := TokenFromEnv(); err != nil || tok != "from-file" { + t.Fatalf("TokenFromEnv from a worker directory after canonicalization = %q, %v", tok, err) + } + }) + + t.Run("symlinked pathname is resolved", func(t *testing.T) { + link := filepath.Join(base, "tok-link") + if err := os.Symlink(token, link); err != nil { + t.Skipf("symlink unsupported: %v", err) + } + t.Setenv(EnvToken, "") + t.Setenv(EnvTokenFile, link) + if err := CanonicalizeTokenFileEnv(); err != nil { + t.Fatalf("CanonicalizeTokenFileEnv: %v", err) + } + if got := os.Getenv(EnvTokenFile); got != token { + t.Fatalf("%s = %q, want the resolved target %q", EnvTokenFile, got, token) + } + }) + + t.Run("an inline token keeps precedence over a dangling pointer", func(t *testing.T) { + // TokenFromEnv prefers EnvToken, so an unused (even dangling) file pointer + // must neither fail the start nor be rewritten. + dangling := filepath.Join(base, "missing", "tok") + t.Setenv(EnvToken, "from-env") + t.Setenv(EnvTokenFile, dangling) + if err := CanonicalizeTokenFileEnv(); err != nil { + t.Fatalf("CanonicalizeTokenFileEnv with an inline token: %v", err) + } + if got := os.Getenv(EnvTokenFile); got != dangling { + t.Fatalf("%s = %q, want it left alone", EnvTokenFile, got) + } + if tok, err := TokenFromEnv(); err != nil || tok != "from-env" { + t.Fatalf("TokenFromEnv = %q, %v, want the inline token", tok, err) + } + }) + + t.Run("a selected but unreadable pointer fails closed", func(t *testing.T) { + t.Setenv(EnvToken, "") + t.Setenv(EnvTokenFile, filepath.Join(base, "missing", "tok")) + if err := CanonicalizeTokenFileEnv(); err == nil { + t.Fatal("a selected token file that cannot be resolved must error") + } + }) + + t.Run("no pointer is a no-op", func(t *testing.T) { + t.Setenv(EnvToken, "") + t.Setenv(EnvTokenFile, "") + if err := CanonicalizeTokenFileEnv(); err != nil { + t.Fatalf("CanonicalizeTokenFileEnv without a pointer: %v", err) + } + if got := os.Getenv(EnvTokenFile); got != "" { + t.Fatalf("%s = %q, want empty", EnvTokenFile, got) + } + }) +} + func TestServerTLSConfigRequiresCertKey(t *testing.T) { if _, err := ServerTLSConfig("", ""); err == nil { t.Fatal("ServerTLSConfig must require a cert and key (TLS mandatory)") diff --git a/internal/sandbox/engine.go b/internal/sandbox/engine.go index ed7440780..f179619e4 100644 --- a/internal/sandbox/engine.go +++ b/internal/sandbox/engine.go @@ -144,19 +144,28 @@ func (engine *Engine) LookupCommandPrefixForSession(toolName string, command []s // don't re-run Abs/EvalSymlinks per visited path. Returns nil for a nil engine // (the matcher's methods treat nil as "exclude nothing"). func (engine *Engine) ReadExclusions() *ReadExclusions { - // A disabled policy enforces nothing, so it must not filter search results - // either (Evaluate already allows every request under ModeDisabled). + // A disabled policy enforces nothing the USER configured, so it must not + // filter search results by DenyRead either (Evaluate likewise allows every + // request under ModeDisabled). The automatic credential exclusion is not + // policy-derived and survives: the bridge token authenticates the channel + // driving these tools, so turning the sandbox off must not hand a remote + // caller its own credential. if engine == nil { return nil } policy := engine.effectivePolicy(engine.policy) if policy.Mode == ModeDisabled { - return nil + protected := protectedCredentialPaths() + if len(protected) == 0 { + return nil + } + return &ReadExclusions{workspaceRoot: engine.workspaceRoot, protectedRoots: protected} } return &ReadExclusions{ - workspaceRoot: engine.workspaceRoot, - denyRoots: resolvePolicyPaths(policy.DenyRead), - allowRoots: resolvePolicyPaths(policy.AllowRead), + workspaceRoot: engine.workspaceRoot, + denyRoots: resolvePolicyPaths(policy.DenyRead), + allowRoots: resolvePolicyPaths(policy.AllowRead), + protectedRoots: protectedCredentialPaths(), } } @@ -164,13 +173,14 @@ func (engine *Engine) ReadExclusions() *ReadExclusions { // engine's policy + scope (see the package-level ReadExclusionGlobs). Empty when // DenyRead is unset or the engine has no scope. func (engine *Engine) ReadExclusionGlobs() []string { - // A disabled policy filters nothing (parity with ReadExclusions / Evaluate). + // A disabled policy filters nothing the user configured, but keeps the + // automatic credential exclusion (parity with ReadExclusions / Evaluate). if engine == nil { return nil } policy := engine.effectivePolicy(engine.policy) if policy.Mode == ModeDisabled { - return nil + return ReadExclusionGlobs(Policy{}, engine.scope) } return ReadExclusionGlobs(policy, engine.scope) } @@ -183,7 +193,14 @@ func (engine *Engine) effectiveNetworkMode(policy Policy) NetworkMode { } // UnsandboxedExecutionAllowed reports whether an escalated shell attempt may -// bypass the native sandbox without dropping active denied-read restrictions. +// bypass the native sandbox without dropping active denied-read restrictions, +// including the automatic remote bridge token exclusion. +// +// ModeDisabled short-circuits to true before the token is consulted, and that is +// the intended reading of the switch rather than an oversight: with the sandbox +// off there is no wrapper for an escalation to bypass, so refusing the +// escalation would deny a capability the operator already granted globally +// without protecting anything. See the ModeDisabled branch in Evaluate. func (engine *Engine) UnsandboxedExecutionAllowed() bool { if engine == nil { return true @@ -192,7 +209,7 @@ func (engine *Engine) UnsandboxedExecutionAllowed() bool { if policy.Mode == ModeDisabled { return true } - return len(normalizeProfilePaths(policy.DenyRead)) == 0 + return len(normalizeProfilePaths(policy.DenyRead)) == 0 && len(protectedCredentialPaths()) == 0 } // toolNetworkExempt reports whether a request is exempt from the engine-level @@ -318,6 +335,24 @@ func (engine *Engine) Evaluate(ctx context.Context, request Request) Decision { risk := classifyWithScope(request, scope) if policy.Mode == ModeDisabled { + // Disabling the sandbox drops every user-configured restriction, but not the + // automatic credential exclusion: the remote bridge token authenticates the + // caller driving these tools, so it stays unreadable and unwritable through + // them. + // + // Shell is deliberately left open here, and it is worth being blunt about + // why. ModeDisabled means no OS wrapper is built at all — the runner sets + // SandboxPreferenceForbid and PermissionProfileFromPolicy returns an + // unrestricted filesystem — so there is no layer left that could confine a + // command. Re-wrapping shell just for this file would quietly undo the + // switch the operator flipped, and would not hold anyway: a shell that can + // run anything can read anything this process can. The exclusion below is + // therefore what it says — a guarantee about Zero's own file tools, not + // about the machine. Running a remote bridge with the sandbox disabled + // means trusting whoever can drive that bridge with the token. + if block := protectedCredentialPathBlock(request, request.WorkspaceRoot); block != nil { + return deny(request, risk, block.Code, block.Path, block.Reason, false) + } return Decision{Action: ActionAllow, Risk: risk, Reason: "sandbox disabled"} } if request.Permission == PermissionDeny { diff --git a/internal/sandbox/manager.go b/internal/sandbox/manager.go index 44fd6f0a0..c2f2f5630 100644 --- a/internal/sandbox/manager.go +++ b/internal/sandbox/manager.go @@ -228,8 +228,15 @@ func (manager SandboxManager) BuildExecutionRequest(request SandboxManagerReques if request.ValidateExecution && preference == SandboxPreferenceRequire && backend.SupportLevel() != BackendSupportNative { return SandboxExecutionRequest{}, nativeSandboxUnavailableError(backend) } - if request.ValidateExecution && preference != SandboxPreferenceForbid && backend.SupportLevel() != BackendSupportNative && policyHasExplicitDeny(policy) { - return SandboxExecutionRequest{}, errors.New("native sandbox unavailable: configured deny_read or deny_write rules cannot be enforced") + protectedCredentials := protectedCredentialPaths() + protectedCredentialNeedsNative := policy.Mode != ModeDisabled && len(protectedCredentials) > 0 + if request.ValidateExecution && preference != SandboxPreferenceForbid && backend.SupportLevel() != BackendSupportNative && + (policyHasExplicitDeny(policy) || protectedCredentialNeedsNative) { + return SandboxExecutionRequest{}, errors.New("native sandbox unavailable: configured deny rules or protected credentials cannot be enforced") + } + if request.ValidateExecution && preference != SandboxPreferenceForbid && policy.Mode != ModeDisabled && manager.goos == "darwin" && + protectedCredentialInWritableMacOSRoot(profile, protectedCredentials) { + return SandboxExecutionRequest{}, errors.New("macOS sandbox cannot protect the remote token file inside a shell-writable root; move the token file outside the workspace and temporary directories") } // Windows: the FULL OS sandbox needs a one-time elevated `zero sandbox setup` // (it applies WFP network filters + workspace ACLs and writes a marker). @@ -290,6 +297,56 @@ func policyHasExplicitDeny(policy Policy) bool { return len(normalizeProfilePaths(policy.DenyRead)) > 0 || len(normalizeProfilePaths(policy.DenyWrite)) > 0 } +func protectedCredentialInWritableMacOSRoot(profile PermissionProfile, protected []string) bool { + if len(protected) == 0 { + return false + } + if profile.FileSystem.Kind == FileSystemUnrestricted { + return true + } + if profile.FileSystem.Kind != FileSystemRestricted { + return false + } + writeRoots := make([]string, 0, len(profile.FileSystem.WriteRoots)+len(sandboxWritableSubpaths)) + for _, root := range profile.FileSystem.WriteRoots { + writeRoots = append(writeRoots, normalizeProfilePath(root.Root)) + } + if profile.FileSystem.AllowTemp { + writeRoots = append(writeRoots, normalizeProfilePaths(sandboxWritableSubpaths)...) + } + for _, credential := range protected { + credential = filepath.Clean(strings.TrimSpace(credential)) + if credential == "." || credential == "" { + continue + } + for _, root := range writeRoots { + if pathWithinMacOSRoot(root, credential) { + return true + } + } + } + return false +} + +func pathWithinMacOSRoot(root, candidate string) bool { + if pathWithinRoot(root, candidate) || pathWithinRoot(strings.ToLower(root), strings.ToLower(candidate)) { + return true + } + rootInfo, err := os.Stat(root) + if err != nil { + return false + } + for current := candidate; ; current = filepath.Dir(current) { + if info, err := os.Stat(current); err == nil && os.SameFile(rootInfo, info) { + return true + } + parent := filepath.Dir(current) + if parent == current { + return false + } + } +} + func (manager SandboxManager) BuildCommandPlan(request SandboxManagerRequest) (CommandPlan, error) { execRequest, err := manager.BuildExecutionRequest(request) if err != nil { diff --git a/internal/sandbox/manager_test.go b/internal/sandbox/manager_test.go index 566bdf7c7..1328e6df0 100644 --- a/internal/sandbox/manager_test.go +++ b/internal/sandbox/manager_test.go @@ -6,6 +6,7 @@ import ( "path/filepath" "reflect" "runtime" + "slices" "strings" "testing" ) @@ -250,6 +251,126 @@ func TestSandboxManagerDegradesUnavailableCommandPlan(t *testing.T) { } } +func TestSandboxManagerRejectsUnavailableBackendForProtectedToken(t *testing.T) { + workspace, _ := protectedTokenFixture(t) + policy := DefaultPolicy() + backend := Backend{Name: BackendUnavailable, Platform: "linux", Fallback: true, Message: "native sandbox unavailable"} + _, err := NewSandboxManager(SandboxManagerOptions{GOOS: "linux", Backend: backend}).BuildCommandPlan(SandboxManagerRequest{ + WorkspaceRoot: workspace, + Command: CommandSpec{Name: "/bin/sh", Args: []string{"-c", "true"}, Dir: workspace}, + Policy: policy, + Profile: PermissionProfileFromPolicy(workspace, policy, nil), + Preference: SandboxPreferenceAuto, + ValidateExecution: true, + }) + if err == nil || !strings.Contains(err.Error(), "protected credentials cannot be enforced") { + t.Fatalf("BuildCommandPlan error = %v, want protected-credential enforcement failure", err) + } +} + +func TestSandboxManagerRejectsMacOSTokenInsideWritableWorkspace(t *testing.T) { + t.Setenv(daemonRemoteTokenEnv, "") + t.Setenv(daemonRemoteTokenFileEnv, "/workspace/bridge-token") + workspace := "/workspace" + policy := DefaultPolicy() + backend := Backend{ + Name: BackendMacOSSeatbelt, + Available: true, + Executable: "/usr/bin/sandbox-exec", + Platform: "darwin", + CommandWrapping: true, + NativeIsolation: true, + } + _, err := NewSandboxManager(SandboxManagerOptions{GOOS: "darwin", Backend: backend}).BuildCommandPlan(SandboxManagerRequest{ + WorkspaceRoot: workspace, + Command: CommandSpec{Name: "/bin/sh", Args: []string{"-c", "true"}, Dir: workspace}, + Policy: policy, + Profile: PermissionProfileFromPolicy(workspace, policy, nil), + Preference: SandboxPreferenceAuto, + ValidateExecution: true, + }) + if err == nil || !strings.Contains(err.Error(), "inside a shell-writable root") { + t.Fatalf("BuildCommandPlan error = %v, want macOS writable-token failure", err) + } +} + +func TestProtectedCredentialInWritableMacOSRootMatchesSeatbeltWrites(t *testing.T) { + restricted := PermissionProfile{FileSystem: FileSystemPolicy{ + Kind: FileSystemRestricted, + WriteRoots: []WritableRoot{{Root: "/Users/Test/Workspace"}}, + }} + if !protectedCredentialInWritableMacOSRoot(restricted, []string{normalizeProfilePath("/users/test/workspace/token")}) { + t.Fatal("case-variant token under a macOS write root should be rejected") + } + if protectedCredentialInWritableMacOSRoot(restricted, []string{normalizeProfilePath("/Users/Test/Credentials/token")}) { + t.Fatal("token outside every macOS write root should remain allowed") + } + restricted.FileSystem.AllowTemp = true + if !protectedCredentialInWritableMacOSRoot(restricted, []string{normalizeProfilePath("/private/tmp/bridge-token")}) { + t.Fatal("token under an allowed temporary root should be rejected") + } + unrestricted := PermissionProfile{FileSystem: FileSystemPolicy{Kind: FileSystemUnrestricted}} + if !protectedCredentialInWritableMacOSRoot(unrestricted, []string{"/credentials/token"}) { + t.Fatal("an unrestricted macOS filesystem makes every token path shell-writable") + } + + writable := t.TempDir() + targetDir := t.TempDir() + target := filepath.Join(targetDir, "token") + if err := os.WriteFile(target, []byte("secret"), 0o600); err != nil { + t.Fatal(err) + } + link := filepath.Join(writable, "token-link") + if err := os.Symlink(target, link); err != nil { + t.Skipf("symlinks unavailable: %v", err) + } + symlinkProfile := PermissionProfile{FileSystem: FileSystemPolicy{ + Kind: FileSystemRestricted, + WriteRoots: []WritableRoot{{Root: writable}}, + }} + if !protectedCredentialInWritableMacOSRoot(symlinkProfile, []string{link, target}) { + t.Fatal("selected symlink inside a write root must be rejected even when its target is outside") + } +} + +func TestSandboxManagerLeavesProtectedTokenShellOpenWhenSandboxDisabled(t *testing.T) { + t.Setenv(daemonRemoteTokenEnv, "") + t.Setenv(daemonRemoteTokenFileEnv, "/workspace/bridge-token") + policy := DefaultPolicy() + policy.Mode = ModeDisabled + backend := Backend{Name: BackendUnavailable, Platform: "darwin", Fallback: true, Message: "native sandbox unavailable"} + request, err := NewSandboxManager(SandboxManagerOptions{GOOS: "darwin", Backend: backend}).BuildExecutionRequest(SandboxManagerRequest{ + WorkspaceRoot: "/workspace", + Command: CommandSpec{Name: "/bin/sh", Args: []string{"-c", "true"}, Dir: "/workspace"}, + Policy: policy, + Profile: PermissionProfileFromPolicy("/workspace", policy, nil), + Preference: SandboxPreferenceAuto, + ValidateExecution: true, + }) + if err != nil { + t.Fatalf("BuildExecutionRequest disabled policy: %v", err) + } + if request.EnforcementLevel != EnforcementDisabled || request.CommandWrapped { + t.Fatalf("disabled request = %#v, want intentionally open shell", request) + } + + policy = DefaultPolicy() + request, err = NewSandboxManager(SandboxManagerOptions{GOOS: "darwin", Backend: backend}).BuildExecutionRequest(SandboxManagerRequest{ + WorkspaceRoot: "/workspace", + Command: CommandSpec{Name: "/bin/sh", Args: []string{"-c", "true"}, Dir: "/workspace"}, + Policy: policy, + Profile: PermissionProfileFromPolicy("/workspace", policy, nil), + Preference: SandboxPreferenceForbid, + ValidateExecution: true, + }) + if err != nil { + t.Fatalf("BuildExecutionRequest forbidden sandbox preference: %v", err) + } + if request.EnforcementLevel != EnforcementDisabled || request.CommandWrapped { + t.Fatalf("forbidden-sandbox request = %#v, want intentionally open shell", request) + } +} + func TestSandboxManagerSelectsPlatformBackend(t *testing.T) { tests := []struct { name string @@ -410,12 +531,17 @@ func TestCredentialDenyReadPathsIn(t *testing.T) { if err := os.WriteFile(keyFile, []byte("{}"), 0o600); err != nil { t.Fatal(err) } + daemonTokenFile := filepath.Join(home, "daemon-token") + if err := os.WriteFile(daemonTokenFile, []byte("secret"), 0o600); err != nil { + t.Fatal(err) + } - paths := credentialDenyReadPathsIn(home, keyFile, nil) + paths := credentialDenyReadPathsIn(home, keyFile, daemonTokenFile, nil) for _, want := range normalizeProfilePaths([]string{ awsDir, gcloudDir, keyFile, + daemonTokenFile, npmrc, ghHosts, netrc, @@ -434,24 +560,104 @@ func TestCredentialDenyReadPathsIn(t *testing.T) { } // An explicit AllowRead entry covering a store is an opt-out. - optedOut := credentialDenyReadPathsIn(home, keyFile, []string{awsDir}) + optedOut := credentialDenyReadPathsIn(home, keyFile, daemonTokenFile, []string{awsDir, daemonTokenFile}) if stringSliceContains(optedOut, normalizeProfilePaths([]string{awsDir})[0]) { t.Errorf("credential deny paths = %#v, want AllowRead opt-out to drop ~/.aws", optedOut) } if !stringSliceContains(optedOut, normalizeProfilePaths([]string{keyFile})[0]) { t.Errorf("credential deny paths = %#v, want unrelated entries kept after opt-out", optedOut) } + // The bridge bearer token is the exception: the in-process tool boundary + // (protectedCredentialPaths) refuses to re-include it through AllowRead, so + // the OS-sandbox profile must not either — otherwise the guarantee would + // depend on whether a wrapped shell command or a built-in tool reads it. + if !stringSliceContains(optedOut, normalizeProfilePaths([]string{daemonTokenFile})[0]) { + t.Errorf("credential deny paths = %#v, want the daemon token file denied despite AllowRead", optedOut) + } - if got := credentialDenyReadPathsIn(" ", "", nil); len(got) != 0 { + if got := credentialDenyReadPathsIn(" ", "", "", nil); len(got) != 0 { t.Errorf("credential deny paths for blank home = %#v, want none", got) } // The GOOGLE_APPLICATION_CREDENTIALS target stays protected even when no // home directory is resolvable. - homeless := credentialDenyReadPathsIn("", keyFile, nil) + homeless := credentialDenyReadPathsIn("", keyFile, daemonTokenFile, nil) if !stringSliceContains(homeless, normalizeProfilePaths([]string{keyFile})[0]) { t.Errorf("credential deny paths without home = %#v, want key file included", homeless) } + if !stringSliceContains(homeless, normalizeProfilePaths([]string{daemonTokenFile})[0]) { + t.Errorf("credential deny paths without home = %#v, want daemon token file included", homeless) + } +} + +func TestPermissionProfileDeniesDaemonTokenFile(t *testing.T) { + if runtime.GOOS == "windows" { + t.Skip("credential deny-read paths are disabled on Windows pending the ACL model") + } + tokenFile := filepath.Join(t.TempDir(), "daemon-token") + if err := os.WriteFile(tokenFile, []byte("secret"), 0o600); err != nil { + t.Fatal(err) + } + t.Setenv(daemonRemoteTokenEnv, "") + t.Setenv(daemonRemoteTokenFileEnv, tokenFile) + + profile := PermissionProfileFromPolicy(t.TempDir(), DefaultPolicy(), nil) + want := normalizeProfilePaths([]string{tokenFile})[0] + if !stringSliceContains(profile.FileSystem.DenyRead, want) { + t.Fatalf("DenyRead = %#v, want daemon token file %q", profile.FileSystem.DenyRead, want) + } + + // TokenFromEnv selects the inline token when both variables are set, so the + // unused file pointer must not become an automatic OS-sandbox deny. + t.Setenv(daemonRemoteTokenEnv, "from-env") + profile = PermissionProfileFromPolicy(t.TempDir(), DefaultPolicy(), nil) + if stringSliceContains(profile.FileSystem.DenyRead, want) { + t.Fatalf("DenyRead = %#v, must not protect unused token file %q when the inline token takes precedence", profile.FileSystem.DenyRead, want) + } +} + +// TestPermissionProfileDeniesAbsentDaemonTokenFile covers the window an external +// secret rotation opens: the token file is gone, but the pathname is still the +// one the next `serve-remote` start will read. Existence-filtering it out of the +// profile would let a sandboxed process recreate it with an attacker-chosen +// bearer, so the mandatory entry survives the file's absence for backend +// enforcement. +func TestPermissionProfileDeniesAbsentDaemonTokenFile(t *testing.T) { + if runtime.GOOS == "windows" { + t.Skip("credential deny-read paths are disabled on Windows pending the ACL model") + } + workspace := t.TempDir() + tokenFile := filepath.Join(workspace, "daemon-token") + if err := os.WriteFile(tokenFile, []byte("secret"), 0o600); err != nil { + t.Fatal(err) + } + t.Setenv(daemonRemoteTokenEnv, "") + t.Setenv(daemonRemoteTokenFileEnv, tokenFile) + + // Rotation removes the file after the bridge has already loaded its token. + if err := os.Remove(tokenFile); err != nil { + t.Fatal(err) + } + + profile := PermissionProfileFromPolicy(workspace, DefaultPolicy(), nil) + want := normalizeProfilePaths([]string{tokenFile})[0] + if !stringSliceContains(profile.FileSystem.DenyRead, want) { + t.Fatalf("DenyRead = %#v, want the absent token path %q still denied", profile.FileSystem.DenyRead, want) + } + + // Seatbelt must also deny writes because the workspace root is otherwise + // writable. The replacement is read by the next bridge start, not this one. + rules := credentialDenyWriteRules(profile.FileSystem, DefaultPolicy()) + if !slices.ContainsFunc(rules, func(rule string) bool { return strings.Contains(rule, want) }) { + t.Fatalf("seatbelt credential write rules = %#v, want a deny for %q", rules, want) + } + + // Bubblewrap has no target to bind for a missing path, so the fallback must + // still produce an unreadable, unwritable mount rather than nothing. + args := appendUnreadableLinuxPathArgs(nil, want) + if !slices.Contains(args, "--tmpfs") || !slices.Contains(args, want) { + t.Fatalf("bubblewrap args for the absent token = %#v, want an unreadable tmpfs at %q", args, want) + } } func TestCredentialDenyReadPathsForEnvironmentHonorsConfigOverrides(t *testing.T) { diff --git a/internal/sandbox/pathlists.go b/internal/sandbox/pathlists.go index f6815a2c7..6ab75b92d 100644 --- a/internal/sandbox/pathlists.go +++ b/internal/sandbox/pathlists.go @@ -3,6 +3,7 @@ package sandbox import ( "os" "path/filepath" + "runtime" "strings" ) @@ -41,6 +42,202 @@ func resolvePolicyPath(entry string) (string, bool) { return resolved, true } +// These name the alternative sources of the remote bridge's bearer token. They +// are duplicated from internal/daemon/remote (which cannot be imported here) +// exactly like the copies scrubSensitiveEnv keeps. +const ( + daemonRemoteTokenEnv = "ZERO_DAEMON_REMOTE_TOKEN" + daemonRemoteTokenFileEnv = "ZERO_DAEMON_REMOTE_TOKEN_FILE" +) + +// selectedDaemonRemoteTokenFile returns the token-file pointer only when the +// daemon would use it. TokenFromEnv gives the inline token precedence, so an +// inherited file pointer is not a credential when both variables are set. +// +// The pointer is used verbatim, mirroring remote.TokenFilePathFromEnv: the +// daemon reads whatever bytes the variable names, so trimming whitespace here +// would leave a token file whose name begins or ends with a space unprotected +// while the daemon still reads it. Only a value that is entirely whitespace +// counts as unset. +func selectedDaemonRemoteTokenFile() string { + if strings.TrimSpace(os.Getenv(daemonRemoteTokenEnv)) != "" { + return "" + } + configured := os.Getenv(daemonRemoteTokenFileEnv) + if strings.TrimSpace(configured) == "" { + return "" + } + return configured +} + +// protectedCredentialPaths returns credential files that Zero's own in-process +// file tools must never read or modify, independent of Policy. +// +// This is deliberately separate from credentialDenyReadPaths, which only shapes +// the OS sandbox profile for wrapped shell commands: read_file resolves scoped +// paths itself, and grep/glob build their exclusions from the policy, so a +// profile-only rule leaves the in-process tool boundary open. It is also +// separate from Policy.DenyRead, whose emptiness gates escalated (unsandboxed) +// execution and must keep reflecting user configuration alone. +// +// Entries here are NOT re-includable through AllowRead/AllowWrite or a +// session/turn permission profile: the bridge bearer token grants control of +// this daemon, so a remote-controlled agent must not be able to read it, nor +// replace it to hijack the next bridge start, even when the file sits inside +// its own session workspace. Unlike the profile list this applies on Windows +// too, where filesystem deny-read has no sandbox representation (#662). +// +// Both the selected pathname and the target it currently resolves to are +// protected: `zero daemon serve-remote` canonicalizes the value it selects (see +// remote.CanonicalizeTokenFileEnv), but a symlinked selected value inherited +// from elsewhere must not leave the link replaceable. +func protectedCredentialPaths() []string { + // os.ReadFile — the daemon's own reader — treats the value literally, so a + // relative path resolves against the working directory and a leading "~" is + // NOT expanded. resolvePolicyPath would expand it and protect the wrong file. + configured := selectedDaemonRemoteTokenFile() + if configured == "" { + return nil + } + absolute, err := filepath.Abs(configured) + if err != nil { + return nil + } + paths := []string{absolute} + if resolved, err := filepath.EvalSymlinks(absolute); err == nil && resolved != absolute { + paths = append(paths, resolved) + } + return dedupeStrings(paths) +} + +// protectedCredentialPathBlock returns the block for the first requested path +// that targets a protected credential file, or nil. It exists for the callers +// that bypass validatePathWithPolicy — currently the ModeDisabled short-circuit, +// where the exclusion still applies because it is not policy-derived. +// +// Only the side effects that name a path are covered. SideEffectShell is not, +// and cannot be: a shell request carries a command line, not a file path, so +// there is nothing here to compare against the token. Shell is confined by the +// OS wrapper instead — which under ModeDisabled does not exist. See the +// ModeDisabled short-circuit in Engine.Evaluate for what that boundary means. +func protectedCredentialPathBlock(request Request, workspaceRoot string) *pathBlock { + switch request.SideEffect { + case SideEffectRead, SideEffectWrite, SideEffectOutOfWorkspace: + default: + return nil + } + protected := protectedCredentialPaths() + if len(protected) == 0 { + return nil + } + verb := "readable" + if request.SideEffect != SideEffectRead { + verb = "writable" + } + for _, path := range requestPaths(request) { + if protectedPathDenied(protected, workspaceRoot, path) { + return &pathBlock{ + Code: BlockDenied, + Path: path, + Reason: path + " holds the remote bridge token and is never " + verb, + } + } + } + return nil +} + +// protectedPathDenied reports whether path targets one of the protected +// credential files. There is no allow-list consultation by design. +func protectedPathDenied(protected []string, workspaceRoot, path string) bool { + if len(protected) == 0 { + return false + } + for _, entry := range protected { + if pathUnderProtectedRoot(path, entry, workspaceRoot) { + return true + } + } + + // Keep the lexical check above: in particular, it protects a configured + // pathname even when the file is absent, preventing its replacement. For an + // existing request, also compare the object reached by the filesystem. This + // closes aliases created after the token path was selected: EvalSymlinks + // catches symbolic links, while SameFile catches hard links (and any other + // platform-specific names for the same file). + // + // This inode-level closure is specific to Zero's in-process tools, which see + // every requested path before opening it. The OS layer is pathname-based and + // stays that way: seatbelt and Bubblewrap rules name paths, so a sandboxed + // shell on macOS can still `ln alias && cat alias` — a hard link is a + // second name for the same inode, and no path-based rule covers a name that + // did not exist when the profile was built. That is the same model a + // user-configured DenyRead has always had, deliberately: an aliasing defense + // at the OS layer would have to resolve every path at open time, which is not + // something either backend's policy language expresses. Shell access to a + // host running a remote bridge is therefore access to the token, and the + // protection here is the in-process boundary plus the pathname deny rules, + // not an inode-tight OS guarantee. + abs := path + if !filepath.IsAbs(abs) { + if workspaceRoot == "" { + return false + } + abs = filepath.Join(workspaceRoot, abs) + } + resolved, err := filepath.EvalSymlinks(abs) + if err != nil { + return false + } + requestInfo, err := os.Stat(resolved) + if err != nil { + return false + } + for _, entry := range protected { + protectedInfo, err := os.Stat(entry) + if err == nil && os.SameFile(requestInfo, protectedInfo) { + return true + } + } + return false +} + +// protectedPathFoldsCase reports whether a case-variant spelling of a path opens +// the SAME file on this platform, so the protected-credential comparison must +// fold case to stay closed. +// +// pathWithinRoot ends in filepath.Rel, which ALREADY folds case on Windows +// (path_windows.go's sameWord uses strings.EqualFold) but never on Unix. macOS +// volumes are case-insensitive by default (APFS), so without folding a request +// for `.../Bridge-Token` misses a protected `.../bridge-token` while the OS +// opens the very same bearer-token file. Windows is listed too so the guarantee +// does not silently depend on a filepath.Rel implementation detail. +// +// Case-sensitive outliers (a case-sensitive APFS volume) only make this +// over-deny a genuinely different file that happens to differ by case alone, +// which is the safe direction for a credential the sandbox must never expose. +func protectedPathFoldsCase() bool { + return runtime.GOOS == "windows" || runtime.GOOS == "darwin" +} + +// pathUnderProtectedRoot is pathUnderPolicyRoot for the automatic credential +// exclusions: identical anchoring and symlink normalization, plus the platform's +// filesystem case semantics. Only the final containment comparison folds — the +// normalization above it keeps operating on the path as spelled, so symlink +// resolution is unaffected. +func pathUnderProtectedRoot(requestedPath, root, workspaceRoot string) bool { + normalized, ok := normalizePathForPolicyRoot(requestedPath, root, workspaceRoot) + if !ok { + return false + } + if pathWithinRoot(root, normalized) { + return true + } + if !protectedPathFoldsCase() { + return false + } + return pathWithinRoot(strings.ToLower(root), strings.ToLower(normalized)) +} + // resolvePolicyPaths resolves and de-duplicates a list of policy path entries, // dropping blanks and non-existent entries. Files and directories are both kept // (a DenyRead/DenyWrite entry may target a single sensitive file). @@ -92,18 +289,29 @@ func resolveWriteRootPaths(entries []string) []string { // symlink prefix cannot evade the match. root must be an already-resolved // absolute path. func pathUnderPolicyRoot(requestedPath, root, workspaceRoot string) bool { - if root == "" { + normalized, ok := normalizePathForPolicyRoot(requestedPath, root, workspaceRoot) + if !ok { return false } + return pathWithinRoot(root, normalized) +} + +// normalizePathForPolicyRoot anchors requestedPath (a relative one against +// workspaceRoot) and symlink-normalizes the portion outside root, yielding the +// path pathUnderPolicyRoot compares. ok is false when there is nothing to +// compare against: a blank root, or a relative path with no workspace root. +func normalizePathForPolicyRoot(requestedPath, root, workspaceRoot string) (string, bool) { + if root == "" { + return "", false + } abs := requestedPath if !filepath.IsAbs(abs) { if workspaceRoot == "" { - return false + return "", false } abs = filepath.Join(workspaceRoot, abs) } - normalized := NormalizePrefixForRoot(abs, root) - return pathWithinRoot(root, normalized) + return NormalizePrefixForRoot(abs, root), true } // readDenied reports whether path is excluded by the DenyRead list with no @@ -151,20 +359,28 @@ type ReadExclusions struct { workspaceRoot string denyRoots []string allowRoots []string + // protectedRoots are the automatic credential exclusions + // (protectedCredentialPaths); AllowRead never re-includes them. + protectedRoots []string } -// Active reports whether any DenyRead root is configured. When false the -// exclusions are a no-op and the search behaves exactly as before. +// Active reports whether anything is excluded: a configured DenyRead root or an +// automatic protected credential path. When false the exclusions are a no-op and +// the search behaves exactly as before. func (rx *ReadExclusions) Active() bool { - return rx != nil && len(rx.denyRoots) > 0 + return rx != nil && (len(rx.denyRoots) > 0 || len(rx.protectedRoots) > 0) } // PathExcluded reports whether reading path is excluded by DenyRead, honoring a -// more-specific AllowRead re-inclusion. It is the per-file predicate for a walk. +// more-specific AllowRead re-inclusion, or by an automatic credential exclusion, +// which no allow entry re-includes. It is the per-file predicate for a walk. func (rx *ReadExclusions) PathExcluded(path string) bool { if !rx.Active() { return false } + if protectedPathDenied(rx.protectedRoots, rx.workspaceRoot, path) { + return true + } return readDeniedResolved(rx.workspaceRoot, rx.denyRoots, rx.allowRoots, path) } @@ -176,6 +392,11 @@ func (rx *ReadExclusions) DirExcluded(path string) bool { if !rx.Active() { return false } + // A protected credential entry is a file, so it only prunes a directory when + // the directory IS that entry; PathExcluded still filters it during the walk. + if protectedPathDenied(rx.protectedRoots, rx.workspaceRoot, path) { + return true + } if !readDeniedResolved(rx.workspaceRoot, rx.denyRoots, rx.allowRoots, path) { return false } @@ -212,6 +433,16 @@ func allowWriteScope(policy Policy) *Scope { // enforceWorkspace; the workspace boundary itself applies only when // enforceWorkspace. It never bypasses the symlink/out-of-workspace guards. func validateWritePath(scope *Scope, policy Policy, enforceWorkspace bool, workspaceRoot, path string) *pathBlock { + // The protected credential files outrank every allow: overwriting or + // truncating the bridge token denies service, and replacing it hands the next + // bridge start an attacker-chosen secret. + if protectedPathDenied(protectedCredentialPaths(), workspaceRoot, path) { + return &pathBlock{ + Code: BlockDenied, + Path: path, + Reason: path + " holds the remote bridge token and is never writable", + } + } // DenyWrite wins regardless of workspace enforcement. for _, deny := range resolvePolicyPaths(policy.DenyWrite) { if pathUnderPolicyRoot(path, deny, workspaceRoot) { @@ -252,7 +483,9 @@ func validatePathWithPolicy(scope *Scope, policy Policy, sideEffect SideEffect, // when there is anything to enforce; otherwise it is a no-op (unchanged from the // pre-path-list behavior, where an empty workspace root skipped validation). if workspaceRoot == "" && !filepath.IsAbs(path) { - if enforceWorkspace || policyHasPathLists(policy) { + // A configured bridge token counts as something to enforce: the relative + // path cannot be anchored, so it cannot be proven to miss the token file. + if enforceWorkspace || policyHasPathLists(policy) || len(protectedCredentialPaths()) > 0 { return &pathBlock{ Code: BlockOutsideWorkspace, Path: path, @@ -263,6 +496,13 @@ func validatePathWithPolicy(scope *Scope, policy Policy, sideEffect SideEffect, } switch sideEffect { case SideEffectRead: + if protectedPathDenied(protectedCredentialPaths(), workspaceRoot, path) { + return &pathBlock{ + Code: BlockDenied, + Path: path, + Reason: path + " holds the remote bridge token and is never readable", + } + } if readDenied(policy, workspaceRoot, path) { return &pathBlock{ Code: BlockDenied, @@ -325,7 +565,9 @@ func workspaceRelGlob(workspaceRoot, target string) (string, bool) { // consumer. Empty when DenyRead is unset (the default), so search behavior is // unchanged. func ReadExclusionGlobs(policy Policy, scope *Scope) []string { - denyRoots := resolvePolicyPaths(policy.DenyRead) + // The automatic credential exclusions ride along so an rg-based consumer never + // walks the bridge token when it happens to live inside the workspace. + denyRoots := dedupeStrings(append(resolvePolicyPaths(policy.DenyRead), protectedCredentialPaths()...)) if len(denyRoots) == 0 || scope == nil { return nil } diff --git a/internal/sandbox/profile.go b/internal/sandbox/profile.go index 47947316c..ec02974d3 100644 --- a/internal/sandbox/profile.go +++ b/internal/sandbox/profile.go @@ -147,15 +147,19 @@ func permissionProfileReadRoots(workspaceRoot string, policy Policy, scope *Scop // credentialDenyReadPaths returns default deny-read entries for well-known // credential stores so sandboxed commands cannot read secrets under the // read-all workspace posture. This includes tool configuration files that can -// carry credentials and are now discoverable through the preserved caller -// environment. Two deliberate limits: +// carry credentials, environment-selected credential files, and the selected +// daemon remote token file. Two deliberate limits: // // - Windows is skipped: a non-empty profile DenyRead switches the Windows // runner onto the capability-SID/ACL deny path and away from the // WRITE_RESTRICTED token, which the unelevated tier depends on. Revisit // once the Windows deny-read model is settled. // - A candidate nested under a user-configured AllowRead entry is dropped, -// so `allowRead: ["~/.aws"]` remains an explicit opt-out. +// so `allowRead: ["~/.aws"]` remains an explicit opt-out. The bridge bearer +// token is the one exception: the in-process tool boundary +// (protectedCredentialPaths) treats it as non-overrideable, and the +// guarantee must not depend on whether a wrapped shell command or a built-in +// tool does the reading. // // These are profile-level rules only; they are intentionally NOT merged into // Policy.DenyRead, whose emptiness gates escalated (unsandboxed) execution and @@ -165,7 +169,7 @@ func credentialDenyReadPaths(policy Policy) []string { return nil } // A failed home lookup only drops the home-based candidates; the - // GOOGLE_APPLICATION_CREDENTIALS target must be protected regardless. + // environment-selected credential targets must be protected regardless. home, _ := os.UserHomeDir() return credentialDenyReadPathsForEnvironment(credentialPathEnvironment{ Home: home, @@ -177,15 +181,17 @@ func credentialDenyReadPaths(policy Policy) []string { Netrc: os.Getenv("NETRC"), DockerConfigDir: os.Getenv("DOCKER_CONFIG"), KubeConfig: os.Getenv("KUBECONFIG"), + DaemonTokenFile: selectedDaemonRemoteTokenFile(), }, policy.AllowRead) } // credentialDenyReadPathsIn is the pure core of credentialDenyReadPaths, // separated so tests can exercise it against a synthetic home directory. -func credentialDenyReadPathsIn(home string, googleCredentials string, allowRead []string) []string { +func credentialDenyReadPathsIn(home string, googleCredentials string, daemonTokenFile string, allowRead []string) []string { return credentialDenyReadPathsForEnvironment(credentialPathEnvironment{ Home: home, GoogleCredentials: googleCredentials, + DaemonTokenFile: daemonTokenFile, }, allowRead) } @@ -199,6 +205,7 @@ type credentialPathEnvironment struct { Netrc string DockerConfigDir string KubeConfig string + DaemonTokenFile string } func credentialDenyReadPathsForEnvironment(env credentialPathEnvironment, allowRead []string) []string { @@ -275,8 +282,15 @@ func credentialDenyReadPathsForEnvironment(env credentialPathEnvironment, allowR candidates = append(candidates, path) } } + // The bridge bearer token grants control of this daemon, so unlike the + // opt-outable credential stores above it stays denied even when AllowRead + // covers it — matching protectedCredentialPaths at the in-process boundary. + var mandatory []string + if target := strings.TrimSpace(env.DaemonTokenFile); target != "" { + mandatory = normalizeProfilePaths([]string{target}) + } allowRoots := normalizeProfilePaths(allowRead) - out := make([]string, 0, len(candidates)) + out := make([]string, 0, len(candidates)+len(mandatory)) for _, path := range normalizeProfilePaths(candidates) { // Only stores that actually exist on this host need a deny rule. if _, err := os.Stat(path); err != nil { @@ -293,7 +307,17 @@ func credentialDenyReadPathsForEnvironment(env credentialPathEnvironment, allowR out = append(out, path) } } - return out + // Unlike the candidates above, the token path is NOT existence-filtered. The + // entry protects a future write target as much as a current secret: after an + // external rotation deletes the file, dropping the rule would let a sandboxed + // process recreate it with an attacker-chosen bearer that the next + // `serve-remote` start would then accept. Each enforcing backend materializes + // a missing target fail-closed — bubblewrap mounts an unreadable tmpfs + // (appendUnreadableLinuxPathArgs), seatbelt names the path both literally and + // as a subpath, and Landlock refuses deny-read profiles outright — which is + // also why protectedCredentialPaths keeps the configured lexical path. + out = append(out, mandatory...) + return dedupeStrings(out) } // userGitConfigReadPaths returns the user's global git config FILES so a diff --git a/internal/sandbox/protected_credentials_test.go b/internal/sandbox/protected_credentials_test.go new file mode 100644 index 000000000..5888747f2 --- /dev/null +++ b/internal/sandbox/protected_credentials_test.go @@ -0,0 +1,393 @@ +package sandbox + +import ( + "context" + "os" + "path/filepath" + "runtime" + "strings" + "testing" +) + +// protectedTokenFixture writes a bridge token inside the workspace and points +// ZERO_DAEMON_REMOTE_TOKEN_FILE at it. +func protectedTokenFixture(t *testing.T) (string, string) { + t.Helper() + ws, err := filepath.EvalSymlinks(t.TempDir()) + if err != nil { + t.Fatalf("EvalSymlinks: %v", err) + } + token := filepath.Join(ws, "bridge-token") + if err := os.WriteFile(token, []byte("secret\n"), 0o600); err != nil { + t.Fatalf("write token: %v", err) + } + t.Setenv(daemonRemoteTokenEnv, "") + t.Setenv(daemonRemoteTokenFileEnv, token) + return ws, token +} + +func TestProtectedCredentialPathsResolveLikeTheDaemonReader(t *testing.T) { + base, err := filepath.EvalSymlinks(t.TempDir()) + if err != nil { + t.Fatalf("EvalSymlinks: %v", err) + } + token := filepath.Join(base, "token") + if err := os.WriteFile(token, []byte("secret\n"), 0o600); err != nil { + t.Fatalf("write token: %v", err) + } + + t.Run("absent variable protects nothing", func(t *testing.T) { + t.Setenv(daemonRemoteTokenEnv, "") + t.Setenv(daemonRemoteTokenFileEnv, "") + if got := protectedCredentialPaths(); len(got) != 0 { + t.Fatalf("protected paths = %#v, want none", got) + } + }) + + t.Run("inline token leaves the unused file pointer unprotected", func(t *testing.T) { + t.Setenv(daemonRemoteTokenEnv, "from-env") + t.Setenv(daemonRemoteTokenFileEnv, token) + if got := protectedCredentialPaths(); len(got) != 0 { + t.Fatalf("protected paths = %#v, want none when the inline token takes precedence", got) + } + }) + + t.Run("relative value resolves against the working directory", func(t *testing.T) { + // os.ReadFile — what the daemon uses — resolves a relative value against the + // working directory, so the protected path must do the same. + t.Chdir(base) + t.Setenv(daemonRemoteTokenEnv, "") + t.Setenv(daemonRemoteTokenFileEnv, "token") + if got := protectedCredentialPaths(); !stringSliceContains(got, token) { + t.Fatalf("protected paths = %#v, want %q", got, token) + } + }) + + t.Run("a literal tilde is not home-expanded", func(t *testing.T) { + // os.ReadFile treats "~" as an ordinary directory name; expanding it here + // would protect a path the daemon never reads. + t.Chdir(base) + t.Setenv(daemonRemoteTokenEnv, "") + t.Setenv(daemonRemoteTokenFileEnv, filepath.Join("~", "token")) + want := filepath.Join(base, "~", "token") + got := protectedCredentialPaths() + if !stringSliceContains(got, want) { + t.Fatalf("protected paths = %#v, want literal %q", got, want) + } + home, err := os.UserHomeDir() + if err == nil && stringSliceContains(got, filepath.Join(home, "token")) { + t.Fatalf("protected paths = %#v, must not home-expand the value", got) + } + }) + + t.Run("filename whitespace is part of the pathname", func(t *testing.T) { + // The daemon reads exactly the bytes the variable names, so trimming the + // pointer here would protect "/token" while the bearer file is + // "/token " — leaving the real credential readable and replaceable. + if runtime.GOOS == "windows" { + t.Skip("Windows filenames cannot end in a space") + } + spaced := filepath.Join(base, "token ") + if err := os.WriteFile(spaced, []byte("secret\n"), 0o600); err != nil { + t.Fatalf("write token: %v", err) + } + t.Setenv(daemonRemoteTokenEnv, "") + t.Setenv(daemonRemoteTokenFileEnv, spaced) + got := protectedCredentialPaths() + if !stringSliceContains(got, spaced) { + t.Fatalf("protected paths = %#v, want %q", got, spaced) + } + if stringSliceContains(got, token) { + t.Fatalf("protected paths = %#v, must not protect the trimmed name %q", got, token) + } + }) + + t.Run("a symlinked pathname protects the link and its target", func(t *testing.T) { + if runtime.GOOS == "windows" { + t.Skip("symlink creation needs elevation on Windows") + } + link := filepath.Join(base, "token-link") + if err := os.Symlink(token, link); err != nil { + t.Skipf("symlink unsupported: %v", err) + } + t.Setenv(daemonRemoteTokenEnv, "") + t.Setenv(daemonRemoteTokenFileEnv, link) + got := protectedCredentialPaths() + for _, want := range []string{link, token} { + if !stringSliceContains(got, want) { + t.Fatalf("protected paths = %#v, want %q", got, want) + } + } + }) +} + +// TestProtectedCredentialsSurviveAllowRead locks in the non-opt-out guarantee: +// the bridge token grants control of the daemon, so neither AllowRead, an +// AllowWrite root, nor a granted permission may re-include it. +func TestProtectedCredentialsSurviveAllowRead(t *testing.T) { + ws, token := protectedTokenFixture(t) + policy := Policy{ + Mode: ModeEnforce, + EnforceWorkspace: true, + AllowRead: []string{ws, token}, + AllowWrite: []string{ws}, + } + scope, err := NewScope(ws, nil) + if err != nil { + t.Fatalf("NewScope: %v", err) + } + + for _, sideEffect := range []SideEffect{SideEffectRead, SideEffectWrite, SideEffectOutOfWorkspace} { + block := validatePathWithPolicy(scope, policy, sideEffect, true, ws, token) + if block == nil || !strings.Contains(block.Reason, "remote bridge token") { + t.Fatalf("%s on the bridge token: block = %#v, want a bridge-token deny", sideEffect, block) + } + } + + // The search-walk matcher enforces the same exclusion without consulting + // AllowRead, and it is active even though DenyRead is empty. + engine := NewEngine(EngineOptions{WorkspaceRoot: ws, Policy: policy, Scope: scope}) + rx := engine.ReadExclusions() + if !rx.Active() { + t.Fatal("read exclusions must be active for the automatic credential deny") + } + if !rx.PathExcluded(token) { + t.Fatalf("read exclusions must exclude the bridge token %q", token) + } + if rx.PathExcluded(filepath.Join(ws, "main.go")) { + t.Fatal("read exclusions must not exclude ordinary workspace files") + } + if globs := ReadExclusionGlobs(policy, scope); !stringSliceContains(globs, "!bridge-token") { + t.Fatalf("read exclusion globs = %#v, want the bridge token excluded", globs) + } +} + +func TestProtectedCredentialDirExcluded(t *testing.T) { + ws, token := protectedTokenFixture(t) + engine := NewEngine(EngineOptions{ + WorkspaceRoot: ws, + Policy: Policy{Mode: ModeEnforce, EnforceWorkspace: true, AllowRead: []string{token}}, + }) + + if !engine.ReadExclusions().DirExcluded(token) { + t.Fatalf("DirExcluded must enforce the protected credential path %q", token) + } +} + +func TestProtectedCredentialPreventsUnsandboxedExecution(t *testing.T) { + ws, _ := protectedTokenFixture(t) + engine := NewEngine(EngineOptions{WorkspaceRoot: ws, Policy: DefaultPolicy()}) + + if engine.UnsandboxedExecutionAllowed() { + t.Fatal("UnsandboxedExecutionAllowed must stay false while a credential path is protected") + } +} + +// TestProtectedCredentialsRejectSessionPermissionProfile covers the other +// re-inclusion route: a session/turn permission profile that asks for the token +// path must not be auto-applicable. +func TestProtectedCredentialsRejectSessionPermissionProfile(t *testing.T) { + ws, token := protectedTokenFixture(t) + scope, err := NewScope(ws, nil) + if err != nil { + t.Fatalf("NewScope: %v", err) + } + engine := NewEngine(EngineOptions{ + WorkspaceRoot: ws, + Policy: Policy{Mode: ModeEnforce, EnforceWorkspace: true}, + Scope: scope, + }) + if engine.CoversRequestPermissions(RequestPermissionProfile{ + FileSystem: &FileSystemPermissions{Read: []string{token}}, + }) { + t.Fatal("a permission request covering the bridge token must not read as already-granted") + } + if !engine.CoversRequestPermissions(RequestPermissionProfile{ + FileSystem: &FileSystemPermissions{Read: []string{filepath.Join(ws, "main.go")}}, + }) { + t.Fatal("an ordinary workspace read request must stay covered by policy") + } +} + +// TestProtectedCredentialsDenyReadAndWriteInSeatbeltProfile covers the macOS +// backend: a token under a writable root was read-denied but still truncatable +// through the broad write allow. A user-configured DenyRead entry keeps the write +// direction (see TestSeatbeltProfileProtectsMetadataAndDenyOrdering). +func TestProtectedCredentialsDenyReadAndWriteInSeatbeltProfile(t *testing.T) { + ws, token := protectedTokenFixture(t) + userDenied := filepath.Join(ws, "generated") + profile := PermissionProfile{ + FileSystem: FileSystemPolicy{ + Kind: FileSystemRestricted, + ReadRoots: []string{string(filepath.Separator)}, + WriteRoots: []WritableRoot{{Root: ws}}, + DenyRead: []string{token, userDenied}, + AllowTemp: true, + }, + Network: NetworkPolicy{Mode: NetworkDeny}, + } + sbpl := seatbeltProfileFromPermissionProfile(profile, Policy{Mode: ModeEnforce, DenyRead: []string{userDenied}}, "") + escaped := sandboxProfileString(normalizeProfilePath(token)) + denyRead := `(deny file-read* (literal "` + escaped + `"))` + denyWrite := `(deny file-write* (literal "` + escaped + `"))` + for _, want := range []string{denyRead, denyWrite} { + if !strings.Contains(sbpl, want) { + t.Fatalf("Seatbelt profile missing %q:\n%s", want, sbpl) + } + } + if strings.Contains(sbpl, `(deny file-write* (literal "`+sandboxProfileString(normalizeProfilePath(userDenied))+`"))`) { + t.Fatalf("a user-configured DenyRead path must stay writable:\n%s", sbpl) + } + // Seatbelt is last-match-wins, so the denial must follow the broad allow. + if allow := strings.Index(sbpl, "(allow file-write*"); allow < 0 || strings.Index(sbpl, denyWrite) < allow { + t.Fatalf("the write denial must follow the broad write allow:\n%s", sbpl) + } +} + +// TestProtectedCredentialsSurviveDisabledPolicy covers the one route that skips +// validatePathWithPolicy entirely: ModeDisabled drops every user-configured +// restriction, but the bridge token authenticates the caller driving these tools. +func TestProtectedCredentialsSurviveDisabledPolicy(t *testing.T) { + ws, token := protectedTokenFixture(t) + engine := NewEngine(EngineOptions{WorkspaceRoot: ws, Policy: Policy{Mode: ModeDisabled}}) + + for _, sideEffect := range []SideEffect{SideEffectRead, SideEffectWrite} { + decision := engine.Evaluate(context.Background(), Request{ + ToolName: "read_file", + WorkspaceRoot: ws, + SideEffect: sideEffect, + Args: map[string]any{"path": token}, + }) + if decision.Action != ActionDeny || !strings.Contains(decision.Reason, "remote bridge token") { + t.Fatalf("%s under a disabled policy: action = %q reason = %q, want a bridge-token deny", sideEffect, decision.Action, decision.Reason) + } + } + + // Everything else stays allowed: a disabled sandbox is still disabled. + decision := engine.Evaluate(context.Background(), Request{ + ToolName: "read_file", + WorkspaceRoot: ws, + SideEffect: SideEffectRead, + Args: map[string]any{"path": filepath.Join(ws, "main.go")}, + }) + if decision.Action != ActionAllow { + t.Fatalf("ordinary read under a disabled policy: action = %q reason = %q, want allow", decision.Action, decision.Reason) + } + + rx := engine.ReadExclusions() + if !rx.Active() || !rx.PathExcluded(token) { + t.Fatalf("read exclusions under a disabled policy must still exclude %q", token) + } + if rx.PathExcluded(filepath.Join(ws, "main.go")) { + t.Fatal("read exclusions under a disabled policy must not exclude ordinary files") + } +} + +// TestDisabledPolicyLeavesShellOutsideTheTokenBoundary pins the boundary jatmn +// asked to see stated for #685: under ModeDisabled the bridge-token exclusion +// covers Zero's in-process file tools and nothing else. No OS wrapper is built +// at all in that mode, so a shell command is confined by nothing and an +// escalation has nothing to bypass. This test exists so that stops being an +// implicit property — changing any of it should mean changing this test on +// purpose, not discovering the behavior later. +func TestDisabledPolicyLeavesShellOutsideTheTokenBoundary(t *testing.T) { + ws, token := protectedTokenFixture(t) + engine := NewEngine(EngineOptions{WorkspaceRoot: ws, Policy: Policy{Mode: ModeDisabled}}) + + shell := engine.Evaluate(context.Background(), Request{ + ToolName: "bash", + WorkspaceRoot: ws, + SideEffect: SideEffectShell, + Args: map[string]any{"command": "cat " + token}, + }) + if shell.Action != ActionAllow { + t.Fatalf("shell under a disabled policy = %q (%s); the token boundary is documented as in-process only", shell.Action, shell.Reason) + } + + // The same command's payload IS blocked when it arrives as a path-carrying + // request, which is the whole of the guarantee. + read := engine.Evaluate(context.Background(), Request{ + ToolName: "read_file", + WorkspaceRoot: ws, + SideEffect: SideEffectRead, + Args: map[string]any{"path": token}, + }) + if read.Action != ActionDeny { + t.Fatalf("in-process read under a disabled policy = %q (%s), want deny", read.Action, read.Reason) + } + + if !engine.UnsandboxedExecutionAllowed() { + t.Fatal("escalation under a disabled policy must stay allowed: there is no wrapper for it to bypass") + } + + // With the sandbox on, the same configured token flips both: the profile is + // built, so escalating out of it would drop a real deny rule. + enforcing := NewEngine(EngineOptions{WorkspaceRoot: ws, Policy: DefaultPolicy()}) + if enforcing.UnsandboxedExecutionAllowed() { + t.Fatal("escalation must be refused while a bridge token is protected by an active profile") + } +} + +// TestProtectedCredentialsMatchCaseVariantOnCaseInsensitiveFilesystems covers the +// bypass a case-variant spelling opened: pathWithinRoot ends in filepath.Rel, +// which folds case on Windows but NOT on darwin, whose default APFS volume is +// case-insensitive — so `.../BRIDGE-TOKEN` missed the protected `.../bridge-token` +// while the OS opened the same bearer-token file. On a case-sensitive filesystem +// the variant is a genuinely different file and must stay unblocked. +func TestProtectedCredentialsMatchCaseVariantOnCaseInsensitiveFilesystems(t *testing.T) { + ws, token := protectedTokenFixture(t) + variant := filepath.Join(filepath.Dir(token), strings.ToUpper(filepath.Base(token))) + if variant == token { + t.Fatalf("fixture token %q has no case variant", token) + } + scope, err := NewScope(ws, nil) + if err != nil { + t.Fatalf("NewScope: %v", err) + } + policy := Policy{Mode: ModeEnforce, EnforceWorkspace: true, AllowRead: []string{ws}, AllowWrite: []string{ws}} + wantDenied := protectedPathFoldsCase() + + for _, sideEffect := range []SideEffect{SideEffectRead, SideEffectWrite, SideEffectOutOfWorkspace} { + block := validatePathWithPolicy(scope, policy, sideEffect, true, ws, variant) + denied := block != nil && strings.Contains(block.Reason, "remote bridge token") + if denied != wantDenied { + t.Fatalf("%s on case variant %q: denied = %t, want %t (block = %#v)", sideEffect, variant, denied, wantDenied, block) + } + } + + engine := NewEngine(EngineOptions{WorkspaceRoot: ws, Policy: policy, Scope: scope}) + if excluded := engine.ReadExclusions().PathExcluded(variant); excluded != wantDenied { + t.Fatalf("read exclusions on case variant %q: excluded = %t, want %t", variant, excluded, wantDenied) + } + // The exact spelling is denied on every platform regardless. + if block := validatePathWithPolicy(scope, policy, SideEffectRead, true, ws, token); block == nil { + t.Fatalf("the configured token path %q must always be denied", token) + } +} + +// TestProtectedCredentialsDoNotBlockUnrelatedRequests keeps the exclusion inert +// for everyone who does not run the remote bridge. +func TestProtectedCredentialsDoNotBlockUnrelatedRequests(t *testing.T) { + t.Setenv(daemonRemoteTokenEnv, "") + t.Setenv(daemonRemoteTokenFileEnv, "") + ws, err := filepath.EvalSymlinks(t.TempDir()) + if err != nil { + t.Fatalf("EvalSymlinks: %v", err) + } + engine := NewEngine(EngineOptions{ + WorkspaceRoot: ws, + Policy: Policy{Mode: ModeEnforce, EnforceWorkspace: true}, + }) + if rx := engine.ReadExclusions(); rx.Active() { + t.Fatal("read exclusions must stay inactive without a configured token file") + } + decision := engine.Evaluate(context.Background(), Request{ + ToolName: "read_file", + WorkspaceRoot: ws, + SideEffect: SideEffectRead, + Args: map[string]any{"path": filepath.Join(ws, "main.go")}, + }) + if decision.Action == ActionDeny { + t.Fatalf("ordinary workspace read was denied: %q", decision.Reason) + } +} diff --git a/internal/sandbox/risk.go b/internal/sandbox/risk.go index a0e784f67..3d198bcec 100644 --- a/internal/sandbox/risk.go +++ b/internal/sandbox/risk.go @@ -261,7 +261,7 @@ func applyPatchRequestPaths(args map[string]any) []string { } cwd := firstArgString(args, "cwd") var paths []string - for _, path := range patchHeaderPaths(patch) { + for _, path := range PatchHeaderPaths(patch) { if path == "" || path == "/dev/null" { continue } @@ -281,7 +281,7 @@ func applyPatchPathBlock(request Request) *pathBlock { if patch == "" { return nil } - for _, path := range patchHeaderPaths(patch) { + for _, path := range PatchHeaderPaths(patch) { if path == "" || path == "/dev/null" { continue } @@ -296,7 +296,10 @@ func applyPatchPathBlock(request Request) *pathBlock { return nil } -func patchHeaderPaths(patch string) []string { +// PatchHeaderPaths returns every source and destination path declared by a +// patch. Keeping this parser shared with apply_patch ensures the sandbox gate +// and the tool's own path validation agree on what git will read or write. +func PatchHeaderPaths(patch string) []string { var paths []string oldRemaining, newRemaining := 0, 0 inHunk := false @@ -317,23 +320,178 @@ func patchHeaderPaths(patch string) []string { inHunk = false switch { case strings.HasPrefix(line, "diff --git "): - fields := strings.Fields(line) - if len(fields) >= 4 { - paths = append(paths, stripPatchPrefix(fields[2]), stripPatchPrefix(fields[3])) + if source, destination, ok := parseDiffGitPaths(line[len("diff --git "):]); ok { + paths = append(paths, stripPatchPrefix(source), stripPatchPrefix(destination)) + } + case strings.HasPrefix(line, "copy from "): + if path, ok := parseExtendedGitPath(line[len("copy from "):]); ok { + paths = append(paths, filepath.ToSlash(path)) + } + case strings.HasPrefix(line, "copy to "): + if path, ok := parseExtendedGitPath(line[len("copy to "):]); ok { + paths = append(paths, filepath.ToSlash(path)) + } + case strings.HasPrefix(line, "rename from "): + if path, ok := parseExtendedGitPath(line[len("rename from "):]); ok { + paths = append(paths, filepath.ToSlash(path)) + } + case strings.HasPrefix(line, "rename to "): + if path, ok := parseExtendedGitPath(line[len("rename to "):]); ok { + paths = append(paths, filepath.ToSlash(path)) + } + case strings.HasPrefix(line, "rename old "): + if path, ok := parseExtendedGitPath(line[len("rename old "):]); ok { + paths = append(paths, filepath.ToSlash(path)) + } + case strings.HasPrefix(line, "rename new "): + if path, ok := parseExtendedGitPath(line[len("rename new "):]); ok { + paths = append(paths, filepath.ToSlash(path)) } case strings.HasPrefix(line, "@@"): oldRemaining, newRemaining = parsePatchHunkCounts(line) inHunk = oldRemaining > 0 || newRemaining > 0 case strings.HasPrefix(line, "--- "), strings.HasPrefix(line, "+++ "): - fields := strings.Fields(line) - if len(fields) >= 2 { - paths = append(paths, stripPatchPrefix(fields[1])) + if path := patchFileHeaderPath(line); path != "" { + paths = append(paths, stripPatchPrefix(path)) } } } return paths } +func parseDiffGitPaths(line string) (string, string, bool) { + line = strings.TrimSpace(line) + if strings.HasPrefix(line, "\"") { + source, rest, ok := consumeGitPath(line) + if !ok { + return "", "", false + } + destination, ok := parseWholeGitPath(rest) + return source, destination, ok + } + + // When only the destination is quoted, its opening quote uniquely separates + // the two operands even if the unquoted source contains ordinary spaces. + for separator := strings.Index(line, " \""); separator >= 0; { + if destination, ok := parseWholeGitPath(line[separator+1:]); ok { + return line[:separator], destination, true + } + next := strings.Index(line[separator+1:], " \"") + if next < 0 { + break + } + separator += next + 1 + } + + // Git commonly leaves ordinary spaces unquoted. For a non-rename diff, the + // synthetic a/ and b/ operands name the same path, which identifies the split + // without treating a filename space as an operand separator. + for separator := 0; separator < len(line); { + next := strings.IndexByte(line[separator:], ' ') + if next < 0 { + break + } + separator += next + source, destination := line[:separator], strings.TrimLeft(line[separator:], " ") + if strings.HasPrefix(source, "a/") && strings.HasPrefix(destination, "b/") && source[2:] == destination[2:] { + return source, destination, true + } + separator++ + } + + // Different unquoted names are disambiguated by the copy/rename headers. Keep + // the simple no-space form for ordinary diffs with distinct path operands. + fields := strings.Fields(line) + if len(fields) == 2 { + return fields[0], fields[1], true + } + return "", "", false +} + +func parseWholeGitPath(input string) (string, bool) { + input = strings.TrimSpace(input) + if input == "" { + return "", false + } + if input[0] != '"' { + return input, true + } + path, rest, ok := consumeGitPath(input) + return path, ok && strings.TrimSpace(rest) == "" +} + +// Extended copy/rename headers consume the whole unquoted remainder as the +// pathname, including leading spaces. For a C-quoted name Git consumes the +// first complete quoted string, so trailing header text is not pathname data. +func parseExtendedGitPath(input string) (string, bool) { + if input == "" { + return "", false + } + if input[0] != '"' { + return input, true + } + path, _, ok := consumeGitPath(input) + return path, ok +} + +// consumeGitPath reads one diff --git path operand. Git uses C-style quoted +// strings for paths containing characters that need escaping; strconv.Unquote +// handles its escaped quotes, backslashes, control characters, and octal bytes. +func consumeGitPath(input string) (string, string, bool) { + input = strings.TrimLeft(input, " \t") + if input == "" { + return "", "", false + } + if input[0] != '"' { + end := strings.IndexAny(input, " \t") + if end < 0 { + return input, "", true + } + return input[:end], input[end:], true + } + + escaped := false + for i := 1; i < len(input); i++ { + switch { + case escaped: + escaped = false + case input[i] == '\\': + escaped = true + case input[i] == '"': + path, err := strconv.Unquote(input[:i+1]) + if err != nil { + return "", "", false + } + return path, input[i+1:], true + } + } + return "", "", false +} + +// patchFileHeaderPath mirrors the apply_patch parser's handling of unified-diff +// file headers: paths may be C-quoted and may contain spaces, while an optional +// timestamp is separated by a tab. +// +// Only the tab is header formatting. Git's own parser takes every remaining byte +// of an unquoted operand as pathname data, so a name with a leading or trailing +// space is written verbatim; trimming here would make the token gate evaluate +// `bridge-token` while git patches the live `bridge-token ` beside it. +func patchFileHeaderPath(line string) string { + rest := line[len("--- "):] // "--- " and "+++ " are both 4 bytes + if tab := strings.IndexByte(rest, '\t'); tab >= 0 { + rest = rest[:tab] + } + if strings.HasPrefix(rest, `"`) { + // A quoted operand ends at its closing quote; git allows nothing but the + // (already removed) timestamp after it, so a trailing remainder means the + // line is not the quoted form and the raw bytes are the pathname. + if path, remainder, ok := consumeGitPath(rest); ok && remainder == "" { + return path + } + } + return rest +} + func parsePatchHunkCounts(line string) (int, int) { _, rest, ok := strings.Cut(line, "@@") if !ok { @@ -366,7 +524,6 @@ func patchHunkCount(spec string) int { } func stripPatchPrefix(path string) string { - path = strings.TrimSpace(path) if strings.HasPrefix(path, "a/") || strings.HasPrefix(path, "b/") { path = path[2:] } diff --git a/internal/sandbox/runner.go b/internal/sandbox/runner.go index fe105754a..f798692f4 100644 --- a/internal/sandbox/runner.go +++ b/internal/sandbox/runner.go @@ -11,6 +11,7 @@ import ( "os/exec" "path/filepath" "runtime" + "slices" "strings" "sync/atomic" @@ -661,6 +662,7 @@ func seatbeltProfileFromPermissionProfile(profile PermissionProfile, policy Poli writeRule, } rules = append(rules, denyReadRules(profile.FileSystem)...) + rules = append(rules, credentialDenyWriteRules(profile.FileSystem, policy)...) rules = append(rules, writeRootCarveoutDenyRules(profile.FileSystem)...) rules = append(rules, denyWriteRulesFromPaths(profile.FileSystem.DenyWrite)...) rules = append(rules, networkRule) @@ -787,6 +789,13 @@ func seatbeltProtectedMetadataRegex(root string, name string) string { return "^" + escapedRoot + "/" + escapedName + "(/.*)?$" } +// denyReadRules emits seatbelt deny rules for the profile's read-denied paths. +// The rules name paths — `(literal …)` for files, `(subpath …)` for directories +// — because that is the whole of seatbelt's vocabulary here. A name created +// afterwards for the same inode, most obviously a hard link made by a sandboxed +// shell, is not covered by any of them. That applies equally to a +// user-configured DenyRead and to the automatic remote-bridge-token deny; the +// in-process tools close inode aliases separately (see protectedPathDenied). func denyReadRules(fs FileSystemPolicy) []string { return denySeatbeltPathRules("file-read*", fs.DenyRead) } @@ -823,6 +832,33 @@ func denyWriteRulesFromPaths(paths []string) []string { return denySeatbeltPathRules("file-write*", paths) } +// credentialDenyWriteRules write-denies the AUTOMATIC credential entries of +// DenyRead — the cloud credential stores and the remote bridge token file. +// Denying reads does not imply denying writes, and the broad +// (allow file-write* ...) above covers every workspace root plus the default +// temp roots, so a credential file under one of those stayed truncatable and +// replaceable: enough to deny service, or to swap the secret the next process +// reads. denySeatbeltPathRules keeps emitting read + unlink denials for the +// whole DenyRead list, so a user-configured read-denied path that their build +// legitimately writes (a cache or generated directory) stays writable — only +// Zero's own credential entries lose the write direction. +func credentialDenyWriteRules(fs FileSystemPolicy, policy Policy) []string { + automatic := normalizeProfilePaths(append(credentialDenyReadPaths(policy), protectedCredentialPaths()...)) + if len(automatic) == 0 { + return nil + } + denied := normalizeProfilePaths(fs.DenyRead) + paths := make([]string, 0, len(automatic)) + for _, path := range automatic { + // Only paths the profile actually read-denies: credentialDenyReadPaths + // already drops AllowRead opt-outs, and this keeps the two lists in step. + if slices.Contains(denied, path) { + paths = append(paths, path) + } + } + return denyWriteRulesFromPaths(dedupeStrings(paths)) +} + func denySeatbeltPathRules(action string, paths []string) []string { resolved := normalizeProfilePaths(paths) if len(resolved) == 0 { @@ -965,10 +1001,10 @@ func seatbeltPlatformRuntimeRules() string { ` (literal "/dev/zero"))`, `(allow file-read-data file-test-existence file-write-data (subpath "/dev/fd"))`, `(allow file-read* file-test-existence file-write-data file-ioctl (literal "/dev/dtracehelper"))`, - `(allow file-read* file-test-existence file-write* (subpath "/tmp"))`, - `(allow file-read* file-write* (subpath "/private/tmp"))`, - `(allow file-read* file-write* (subpath "/var/tmp"))`, - `(allow file-read* file-write* (subpath "/private/var/tmp"))`, + `(allow file-read* file-test-existence (subpath "/tmp"))`, + `(allow file-read* (subpath "/private/tmp"))`, + `(allow file-read* (subpath "/var/tmp"))`, + `(allow file-read* (subpath "/private/var/tmp"))`, `(allow file-read* file-test-existence`, ` (literal "/System/Library/CoreServices")`, ` (literal "/System/Library/CoreServices/.SystemVersionPlatform.plist")`, diff --git a/internal/sandbox/runner_test.go b/internal/sandbox/runner_test.go index 21336832d..35ddd2d99 100644 --- a/internal/sandbox/runner_test.go +++ b/internal/sandbox/runner_test.go @@ -499,6 +499,13 @@ func TestSeatbeltProfileProtectsMetadataAndDenyOrdering(t *testing.T) { t.Fatalf("Seatbelt profile missing %q:\n%s", want, sbpl) } } + // A user-configured read-denied path keeps the write direction: a cache or + // generated directory the build legitimately writes must not become read-only + // just because it is excluded from reads. Only Zero's own automatic credential + // entries are write-denied (see TestProtectedCredentialsDenyReadAndWrite...). + if strings.Contains(sbpl, `(deny file-write* (subpath "`+normalizedSecretRead+`"))`) { + t.Fatalf("a user-configured DenyRead path must stay writable:\n%s", sbpl) + } allowIdx := strings.Index(sbpl, "(allow file-write*") denyReadIdx := strings.Index(sbpl, denySecretReadRule) metadataIdx := strings.Index(sbpl, `(deny file-write* (regex #"^/repo/\.git(/.*)?$"))`) @@ -508,6 +515,21 @@ func TestSeatbeltProfileProtectsMetadataAndDenyOrdering(t *testing.T) { } } +func TestSeatbeltTemporaryWritesFollowPermissionProfile(t *testing.T) { + runtimeRules := seatbeltPlatformRuntimeRules() + if strings.Contains(runtimeRules, `file-write* (subpath "/tmp")`) { + t.Fatal("platform runtime rules must not bypass FileSystemPolicy.AllowTemp") + } + withoutTemp := seatbeltWriteRule(FileSystemPolicy{Kind: FileSystemRestricted}) + if strings.Contains(withoutTemp, `(subpath "/tmp")`) { + t.Fatal("restricted profile with AllowTemp=false unexpectedly permits /tmp writes") + } + withTemp := seatbeltWriteRule(FileSystemPolicy{Kind: FileSystemRestricted, AllowTemp: true}) + if !strings.Contains(withTemp, `(subpath "/tmp")`) { + t.Fatal("restricted profile with AllowTemp=true must permit /tmp writes") + } +} + // TestSeatbeltProfileAllowsGitWritesExceptHooksAndConfig locks in the fix for // git subprocesses (fetch, commit, add, ...) failing under the sandbox: the // default profile must stop write-denying the whole .git tree and only carve @@ -708,6 +730,7 @@ func TestScrubSensitiveEnv(t *testing.T) { "ZERO_OAUTH_MY_SVC_CLIENT_SECRET=oauth-secret", "zero_oauth_second_client_secret=case-insensitive-secret", "ZERO_OAUTH_CLIENT_SECRET=not-a-provider-secret", + "ZERO_DAEMON_REMOTE_TOKEN_FILE=/home/user/daemon-token", "AWS_PROFILE=staging", "SAFE_VAR=hello", } diff --git a/internal/tools/apply_patch.go b/internal/tools/apply_patch.go index 310a3a5ac..a19772f16 100644 --- a/internal/tools/apply_patch.go +++ b/internal/tools/apply_patch.go @@ -8,6 +8,8 @@ import ( "path/filepath" "strconv" "strings" + + "github.com/Gitlawb/zero/internal/sandbox" ) type applyPatchTool struct { @@ -85,7 +87,7 @@ func (tool applyPatchTool) RunWithOptions(ctx context.Context, args map[string]a if options.FileTracker != nil { createdTargets = missingPatchTargets(applyRoot, patch) fullySuppliedTargets = completeCreatedPatchTargets(applyRoot, patch) - for _, path := range patchHeaderPaths(patch) { + for _, path := range sandbox.PatchHeaderPaths(patch) { if path == "" || path == "/dev/null" { continue } @@ -144,7 +146,7 @@ func (tool applyPatchTool) RunWithOptions(ctx context.Context, args map[string]a func missingPatchTargets(root string, patch string) []string { seen := map[string]bool{} var missing []string - for _, path := range patchHeaderPaths(patch) { + for _, path := range sandbox.PatchHeaderPaths(patch) { if path == "" || path == "/dev/null" { continue } @@ -189,18 +191,18 @@ func completeCreatedPatchTargets(root string, patch string) []string { case strings.HasPrefix(line, "diff --git "): fromDevNull = false case strings.HasPrefix(line, "@@"): - oldRemaining, newRemaining = parseHunkCounts(line) + oldRemaining, newRemaining = completePatchHunkCounts(line) inHunk = oldRemaining > 0 || newRemaining > 0 case strings.HasPrefix(line, "--- "): - fromDevNull = patchFileHeaderPath(line) == "/dev/null" + fromDevNull = sharedPatchFileHeaderPath(line) == "/dev/null" case strings.HasPrefix(line, "+++ "): - path := patchFileHeaderPath(line) + path := sharedPatchFileHeaderPath(line) if !fromDevNull || path == "" || path == "/dev/null" { fromDevNull = false continue } fromDevNull = false - absolute, _, err := resolveWorkspaceTargetPath(root, stripPatchPrefix(path)) + absolute, _, err := resolveWorkspaceTargetPath(root, path) if err != nil || seen[absolute] { continue } @@ -214,6 +216,44 @@ func completeCreatedPatchTargets(root string, patch string) []string { return created } +func sharedPatchFileHeaderPath(line string) string { + paths := sandbox.PatchHeaderPaths(line) + if len(paths) == 0 { + return "" + } + return paths[0] +} + +func completePatchHunkCounts(line string) (int, int) { + _, rest, ok := strings.Cut(line, "@@") + if !ok { + return 0, 0 + } + if before, _, ok := strings.Cut(rest, "@@"); ok { + rest = before + } + old, next := 0, 0 + for _, field := range strings.Fields(rest) { + switch { + case strings.HasPrefix(field, "-"): + old = completePatchHunkCount(field[1:]) + case strings.HasPrefix(field, "+"): + next = completePatchHunkCount(field[1:]) + } + } + return old, next +} + +func completePatchHunkCount(spec string) int { + if _, count, ok := strings.Cut(spec, ","); ok { + if n, err := strconv.Atoi(count); err == nil { + return n + } + return 0 + } + return 1 +} + func recordCreatedPatchTargets(tracker *FileTracker, missingBefore []string) { if tracker == nil { return @@ -240,7 +280,7 @@ func recordCreatedPatchTargets(tracker *FileTracker, missingBefore []string) { func changedFilesFromPatch(relativeRoot string, patch string) []string { seen := map[string]bool{} var paths []string - for _, path := range patchHeaderPaths(patch) { + for _, path := range sandbox.PatchHeaderPaths(patch) { if path == "" || path == "/dev/null" { continue } @@ -258,7 +298,7 @@ func changedFilesFromPatch(relativeRoot string, patch string) []string { } func validatePatchPaths(root string, patch string) error { - for _, path := range patchHeaderPaths(patch) { + for _, path := range sandbox.PatchHeaderPaths(patch) { if path == "" || path == "/dev/null" { continue } @@ -273,7 +313,7 @@ func validatePatchPaths(root string, patch string) error { } func recheckPatchWriteTargets(root string, patch string) error { - for _, path := range patchHeaderPaths(patch) { + for _, path := range sandbox.PatchHeaderPaths(patch) { if path == "" || path == "/dev/null" { continue } @@ -283,122 +323,3 @@ func recheckPatchWriteTargets(root string, patch string) error { } return nil } - -// patchHeaderPaths returns the file paths declared in a unified diff's headers -// (`diff --git` and `---`/`+++` lines). It tracks hunk state by counting body -// lines from each `@@ -a,b +c,d @@` header, so a removed/added content line that -// merely begins with "--- "/"+++ " (e.g. the removal of a markdown line "-- x") -// is NOT mistaken for a file header. This mirrors how `git apply` parses hunks, -// so a line this skips is content git won't write to either — no security gap. -func patchHeaderPaths(patch string) []string { - var paths []string - oldRemaining, newRemaining := 0, 0 - inHunk := false - for _, line := range strings.Split(strings.ReplaceAll(patch, "\r\n", "\n"), "\n") { - if inHunk && (oldRemaining > 0 || newRemaining > 0) { - switch { - case strings.HasPrefix(line, "-"): - oldRemaining-- - case strings.HasPrefix(line, "+"): - newRemaining-- - case strings.HasPrefix(line, "\\"): - // "\ No newline at end of file" — not a content line. - default: // context line (" ...") or a blank context line - oldRemaining-- - newRemaining-- - } - continue - } - inHunk = false - switch { - case strings.HasPrefix(line, "diff --git "): - fields := strings.Fields(line) - if len(fields) >= 4 { - paths = append(paths, stripPatchPrefix(fields[2]), stripPatchPrefix(fields[3])) - } - case strings.HasPrefix(line, "@@"): - oldRemaining, newRemaining = parseHunkCounts(line) - inHunk = oldRemaining > 0 || newRemaining > 0 - case strings.HasPrefix(line, "--- "), strings.HasPrefix(line, "+++ "): - if p := patchFileHeaderPath(line); p != "" && p != "/dev/null" { - paths = append(paths, stripPatchPrefix(p)) - } - } - } - return paths -} - -func patchFileHeaderPath(line string) string { - if len(line) < len("--- ") { - return "" - } - rest := line[len("--- "):] // "--- " and "+++ " are both 4 bytes - if tab := strings.IndexByte(rest, '\t'); tab >= 0 { - rest = rest[:tab] - } - return strings.TrimSpace(unquoteGitPath(rest)) -} - -// parseHunkCounts reads the old/new line counts from a "@@ -a,b +c,d @@" header. -// A missing count (e.g. "@@ -a +c @@") means 1 per unified-diff convention. -// -// Only the range section BETWEEN the opening and closing "@@" is parsed. A hunk -// header may carry a free-form section heading after the closing "@@" (e.g. -// "@@ -1,1 +1,1 @@ func foo()"), and that text can itself contain "+"/"-" -// tokens. Scanning the whole line would let a crafted heading like -// "@@ -1,1 +1,1 @@ +1,999999" overwrite the real count, keep the parser stuck in -// hunk mode, and swallow later "--- "/"+++ " file headers so they escape -// validatePatchPaths / recheckPatchWriteTargets — a workspace-confinement bypass. -func parseHunkCounts(line string) (int, int) { - _, rest, ok := strings.Cut(line, "@@") - if !ok { - return 0, 0 - } - rangeSection := rest - if before, _, ok := strings.Cut(rest, "@@"); ok { - rangeSection = before // drop the section heading after the closing "@@" - } - old, next := 0, 0 - for _, field := range strings.Fields(rangeSection) { - switch { - case strings.HasPrefix(field, "-"): - old = hunkCount(field[1:]) - case strings.HasPrefix(field, "+"): - next = hunkCount(field[1:]) - } - } - return old, next -} - -func hunkCount(spec string) int { - if _, count, ok := strings.Cut(spec, ","); ok { - if n, err := strconv.Atoi(count); err == nil { - return n - } - return 0 - } - return 1 -} - -// unquoteGitPath undoes git's C-style quoting of a diff path. Git wraps a path in -// double quotes and backslash-escapes special bytes (spaces, tabs, high bytes as -// octal) when it contains anything unusual; an unquoted path is returned as-is. -func unquoteGitPath(s string) string { - s = strings.TrimSpace(s) - if len(s) >= 2 && s[0] == '"' && s[len(s)-1] == '"' { - if unquoted, err := strconv.Unquote(s); err == nil { - return unquoted - } - } - return s -} - -func stripPatchPrefix(path string) string { - path = strings.TrimSpace(path) - // A unified-diff path carries exactly one of the a/ or b/ prefixes; strip a - // single one so a real directory literally named "a" or "b" is preserved. - if strings.HasPrefix(path, "a/") || strings.HasPrefix(path, "b/") { - path = path[2:] - } - return filepath.ToSlash(path) -} diff --git a/internal/tools/apply_patch_paths_test.go b/internal/tools/apply_patch_paths_test.go index 2d255a358..d5b8bb766 100644 --- a/internal/tools/apply_patch_paths_test.go +++ b/internal/tools/apply_patch_paths_test.go @@ -3,6 +3,8 @@ package tools import ( "slices" "testing" + + "github.com/Gitlawb/zero/internal/sandbox" ) func TestPatchHeaderPathsHandlesQuotedAndSpacedNames(t *testing.T) { @@ -13,7 +15,7 @@ func TestPatchHeaderPathsHandlesQuotedAndSpacedNames(t *testing.T) { patch := "--- \"a/dir/file name.go\"\t2024-01-01 00:00:00\n" + "+++ \"b/dir/file name.go\"\n" + "@@ -1 +1 @@\n-old\n+new\n" - got := patchHeaderPaths(patch) + got := sandbox.PatchHeaderPaths(patch) if !slices.Contains(got, "dir/file name.go") { t.Fatalf("quoted spaced path not extracted: %v", got) } @@ -21,7 +23,7 @@ func TestPatchHeaderPathsHandlesQuotedAndSpacedNames(t *testing.T) { func TestPatchHeaderPathsUnspacedStillWorks(t *testing.T) { patch := "--- a/x.go\n+++ b/x.go\n@@ -1 +1 @@\n-old\n+new\n" - got := patchHeaderPaths(patch) + got := sandbox.PatchHeaderPaths(patch) if !slices.Contains(got, "x.go") { t.Fatalf("plain path not extracted: %v", got) } @@ -30,3 +32,29 @@ func TestPatchHeaderPathsUnspacedStillWorks(t *testing.T) { t.Fatalf("/dev/null should not be a header path: %v", got) } } + +// git only C-quotes names containing characters it must escape, so a filename +// whose last byte is an ordinary space reaches the header raw. git apply reads +// it to the tab or newline and patches that exact file, so trimming the operand +// here would leave the token gate comparing against a neighbouring pathname. +func TestPatchHeaderPathsPreservesSurroundingSpacesInNames(t *testing.T) { + patch := "--- a/bridge-token \t2024-01-01 00:00:00\n" + + "+++ b/bridge-token \n" + + "@@ -1 +1 @@\n-old\n+new\n" + got := sandbox.PatchHeaderPaths(patch) + if !slices.Contains(got, "bridge-token ") { + t.Fatalf("trailing-space path not preserved: %q", got) + } + if slices.Contains(got, "bridge-token") { + t.Fatalf("trailing space trimmed off the header path: %q", got) + } +} + +func TestPatchHeaderPathsHandlesCQuotedDiffGitOperands(t *testing.T) { + patch := "diff --git \"a/bridge\\040token\" \"b/exposed\\tcopy\"\n" + got := sandbox.PatchHeaderPaths(patch) + want := []string{"bridge token", "exposed\tcopy"} + if !slices.Equal(got, want) { + t.Fatalf("C-quoted diff --git operands = %q, want %q", got, want) + } +} diff --git a/internal/tools/bash_auto_allow_test.go b/internal/tools/bash_auto_allow_test.go index ef335229d..3543ca5f2 100644 --- a/internal/tools/bash_auto_allow_test.go +++ b/internal/tools/bash_auto_allow_test.go @@ -130,6 +130,13 @@ func TestBashRequireEscalatedKeepsSandboxWhenDeniedReadsActive(t *testing.T) { } } +func TestBashRequireEscalatedKeepsSandboxWhenDaemonTokenProtected(t *testing.T) { + engine := defaultPolicyDaemonTokenEngine(t) + if commandEngineForSandboxPermissions(engine, SandboxPermissionsRequireEscalated) == nil { + t.Fatal("bash require_escalated must preserve the sandbox when the default remote-daemon policy protects a token file") + } +} + func TestBashStillPromptsWithoutActiveSandbox(t *testing.T) { root := t.TempDir() registry := NewRegistry() diff --git a/internal/tools/daemon_token_exclusion_test.go b/internal/tools/daemon_token_exclusion_test.go new file mode 100644 index 000000000..472defb17 --- /dev/null +++ b/internal/tools/daemon_token_exclusion_test.go @@ -0,0 +1,488 @@ +package tools + +import ( + "context" + "os" + "path/filepath" + "runtime" + "strings" + "testing" + + "github.com/Gitlawb/zero/internal/daemon/remote" + "github.com/Gitlawb/zero/internal/sandbox" +) + +// daemonTokenFixture builds a workspace holding the remote bridge's token file +// alongside an ordinary file, with ZERO_DAEMON_REMOTE_TOKEN_FILE pointing at it — +// the shape a remote daemon session takes when its token lives in the session +// workspace. AllowRead deliberately covers the whole workspace so the tests prove +// the exclusion is not re-includable. +func daemonTokenFixture(t *testing.T) (string, string, *sandbox.Engine) { + t.Helper() + ws, err := filepath.EvalSymlinks(t.TempDir()) + if err != nil { + t.Fatalf("EvalSymlinks: %v", err) + } + if err := os.WriteFile(filepath.Join(ws, "main.go"), []byte("package main // bridge-secret\n"), 0o600); err != nil { + t.Fatalf("write main.go: %v", err) + } + token := filepath.Join(ws, "bridge-token") + if err := os.WriteFile(token, []byte("bridge-secret\n"), 0o600); err != nil { + t.Fatalf("write token: %v", err) + } + t.Setenv(remote.EnvToken, "") + t.Setenv(remote.EnvTokenFile, token) + + scope, err := sandbox.NewScope(ws, nil) + if err != nil { + t.Fatalf("NewScope: %v", err) + } + engine := sandbox.NewEngine(sandbox.EngineOptions{ + WorkspaceRoot: ws, + Policy: sandbox.Policy{ + Mode: sandbox.ModeEnforce, + EnforceWorkspace: true, + AllowRead: []string{ws}, + }, + Scope: scope, + }) + return ws, token, engine +} + +// defaultPolicyDaemonTokenEngine is the remote-daemon shape relevant to shell +// escalation: a selected token file with the default policy and no user +// DenyRead entries. +func defaultPolicyDaemonTokenEngine(t *testing.T) *sandbox.Engine { + t.Helper() + ws, err := filepath.EvalSymlinks(t.TempDir()) + if err != nil { + t.Fatalf("EvalSymlinks: %v", err) + } + token := filepath.Join(ws, "bridge-token") + if err := os.WriteFile(token, []byte("bridge-secret\n"), 0o600); err != nil { + t.Fatalf("write token: %v", err) + } + t.Setenv(remote.EnvToken, "") + t.Setenv(remote.EnvTokenFile, token) + policy := sandbox.DefaultPolicy() + if len(policy.DenyRead) != 0 { + t.Fatalf("default policy DenyRead = %#v, want none", policy.DenyRead) + } + return sandbox.NewEngine(sandbox.EngineOptions{WorkspaceRoot: ws, Policy: policy}) +} + +func TestGrepSkipsDaemonTokenFile(t *testing.T) { + ws, _, engine := daemonTokenFixture(t) + tool, ok := NewScopedGrepTool(ws, nil).(sandboxAwareTool) + if !ok { + t.Fatal("grep tool must be sandbox-aware") + } + args := map[string]any{"pattern": "bridge-secret", "output_mode": "files_with_matches"} + + sandboxed := tool.RunWithSandbox(context.Background(), args, engine) + if sandboxed.Status != StatusOK { + t.Fatalf("grep failed: %s", sandboxed.Output) + } + if !strings.Contains(sandboxed.Output, "main.go") { + t.Fatalf("grep must still match ordinary workspace files, got:\n%s", sandboxed.Output) + } + if strings.Contains(sandboxed.Output, "bridge-token") { + t.Fatalf("grep must NOT surface the remote bridge token file, got:\n%s", sandboxed.Output) + } +} + +func TestGlobSkipsDaemonTokenFile(t *testing.T) { + ws, _, engine := daemonTokenFixture(t) + tool, ok := NewScopedGlobTool(ws, nil).(sandboxAwareTool) + if !ok { + t.Fatal("glob tool must be sandbox-aware") + } + + sandboxed := tool.RunWithSandbox(context.Background(), map[string]any{"pattern": "**/*"}, engine) + if sandboxed.Status != StatusOK { + t.Fatalf("glob failed: %s", sandboxed.Output) + } + if !strings.Contains(sandboxed.Output, "main.go") { + t.Fatalf("glob must still match ordinary workspace files, got:\n%s", sandboxed.Output) + } + if strings.Contains(sandboxed.Output, "bridge-token") { + t.Fatalf("glob must NOT surface the remote bridge token file, got:\n%s", sandboxed.Output) + } +} + +func TestListDirectorySkipsDaemonTokenFile(t *testing.T) { + ws, _, engine := daemonTokenFixture(t) + registry := NewRegistry() + registry.Register(NewScopedListDirectoryTool(ws, nil)) + + result := registry.RunWithOptions(context.Background(), "list_directory", map[string]any{ + "path": ".", + }, RunOptions{Sandbox: engine}) + if result.Status != StatusOK { + t.Fatalf("list_directory failed: %s", result.Output) + } + if !strings.Contains(result.Output, "main.go") { + t.Fatalf("list_directory must still show ordinary workspace files, got:\n%s", result.Output) + } + if strings.Contains(result.Output, "bridge-token") { + t.Fatalf("list_directory must NOT surface the remote bridge token file, got:\n%s", result.Output) + } +} + +// TestEngineDeniesDaemonTokenFileTools covers the request gate the direct file +// tools go through (read_file, write_file, edit_file, apply_patch): the bridge +// token must be neither readable nor writable, even though AllowRead covers the +// whole workspace and the file is inside it. +func TestEngineDeniesDaemonTokenFileTools(t *testing.T) { + ws, token, engine := daemonTokenFixture(t) + for _, tc := range []struct { + name string + toolName string + sideEffect sandbox.SideEffect + }{ + {name: "read_file", toolName: "read_file", sideEffect: sandbox.SideEffectRead}, + {name: "write_file", toolName: "write_file", sideEffect: sandbox.SideEffectWrite}, + {name: "edit_file", toolName: "edit_file", sideEffect: sandbox.SideEffectWrite}, + {name: "apply_patch", toolName: "apply_patch", sideEffect: sandbox.SideEffectWrite}, + } { + t.Run(tc.name, func(t *testing.T) { + decision := engine.Evaluate(context.Background(), sandbox.Request{ + ToolName: tc.toolName, + WorkspaceRoot: ws, + SideEffect: tc.sideEffect, + Args: map[string]any{"path": token}, + // A granted permission must not override the exclusion either. + Permission: sandbox.PermissionAllow, + }) + if decision.Action != sandbox.ActionDeny || !strings.Contains(decision.Reason, "remote bridge token") { + t.Fatalf("%s on the bridge token: action = %q reason = %q, want a bridge-token deny", tc.toolName, decision.Action, decision.Reason) + } + }) + } + + // An ordinary workspace file in the same directory stays usable. + decision := engine.Evaluate(context.Background(), sandbox.Request{ + ToolName: "read_file", + WorkspaceRoot: ws, + SideEffect: sandbox.SideEffectRead, + Args: map[string]any{"path": filepath.Join(ws, "main.go")}, + }) + if decision.Action == sandbox.ActionDeny { + t.Fatalf("ordinary workspace read was denied: %q", decision.Reason) + } +} + +func TestApplyPatchDeniesQuotedDaemonTokenPathWithSpaces(t *testing.T) { + ws, err := filepath.EvalSymlinks(t.TempDir()) + if err != nil { + t.Fatalf("EvalSymlinks: %v", err) + } + token := filepath.Join(ws, "bridge token") + if err := os.WriteFile(token, []byte("bridge-secret\n"), 0o600); err != nil { + t.Fatalf("write token: %v", err) + } + t.Setenv(remote.EnvToken, "") + t.Setenv(remote.EnvTokenFile, token) + engine := sandbox.NewEngine(sandbox.EngineOptions{WorkspaceRoot: ws, Policy: sandbox.DefaultPolicy()}) + + registry := NewRegistry() + registry.Register(NewScopedApplyPatchTool(ws, nil)) + patch := "diff --git \"a/bridge token\" \"b/bridge token\"\n" + + "--- \"a/bridge token\"\n" + + "+++ \"b/bridge token\"\n" + + "@@ -1 +1 @@\n" + + "-bridge-secret\n" + + "+attacker-controlled\n" + result := registry.RunWithOptions(context.Background(), "apply_patch", map[string]any{ + "patch": patch, + }, RunOptions{Sandbox: engine, PermissionGranted: true}) + if result.Status == StatusOK || !strings.Contains(result.Output, "remote bridge token") { + t.Fatalf("apply_patch on quoted protected path: status=%s output=%q, want bridge-token denial", result.Status, result.Output) + } + contents, err := os.ReadFile(token) + if err != nil || string(contents) != "bridge-secret\n" { + t.Fatalf("token changed after denied patch: contents=%q err=%v", contents, err) + } +} + +func TestApplyPatchDeniesHeaderOnlyAndBinaryDaemonTokenPatches(t *testing.T) { + original := []byte("bridge-secret\x00original\n") + for _, tc := range []struct { + name string + tokenName string + patch string + destination string + wantControlSource []byte + wantControlTarget []byte + }{ + { + name: "header-only copy", + tokenName: "bridge token", + patch: "diff --git a/bridge token b/exposed-token\n" + + "similarity index 100%\n" + + "copy from bridge token\n" + + "copy to exposed-token\n", + destination: "exposed-token", + wantControlSource: original, + wantControlTarget: original, + }, + { + name: "header-only rename", + tokenName: "bridge token", + patch: "diff --git a/bridge token b/renamed-token\n" + + "similarity index 100%\n" + + "rename from bridge token\n" + + "rename to renamed-token\n", + destination: "renamed-token", + wantControlTarget: original, + }, + { + name: "header-only rename aliases", + tokenName: "bridge token", + patch: "diff --git a/bridge token b/alias-renamed-token\n" + + "similarity index 100%\n" + + "rename old bridge token\n" + + "rename new alias-renamed-token\n", + destination: "alias-renamed-token", + wantControlTarget: original, + }, + { + name: "header-only copy preserves leading space", + tokenName: " bridge-token", + patch: "diff --git a/ bridge-token b/leading-space-copy\n" + + "similarity index 100%\n" + + "copy from bridge-token\n" + + "copy to leading-space-copy\n", + destination: "leading-space-copy", + wantControlSource: original, + wantControlTarget: original, + }, + { + name: "binary modification", + tokenName: "bridge token", + patch: "diff --git a/bridge token b/bridge token\n" + + "index 6e4018bb778ca15e70706fe1f7a4c22e762f37b6..d3eae57ec6ad245ec7ab173e28141ac2f87cca68 100644\n" + + "GIT binary patch\n" + + "literal 23\n" + + "ecmYc+DM?JuPA$?+Ni0cZ$jwj5Ov_A7;Q|0@R|sMN\n\n" + + "literal 23\n" + + "ecmYc)%1lX5)h$j alias && cat alias` +// — the same pathname model a user-configured DenyRead has always had. See +// protectedPathDenied and denyReadRules in internal/sandbox for why that is the +// boundary rather than an omission. +func TestDaemonTokenAliasesDeniedEndToEnd(t *testing.T) { + for _, aliasKind := range []string{"symlink", "hardlink"} { + t.Run(aliasKind, func(t *testing.T) { + ws, token, engine := daemonTokenFixture(t) + alias := filepath.Join(ws, "token-alias") + var err error + switch aliasKind { + case "symlink": + err = os.Symlink(token, alias) + case "hardlink": + err = os.Link(token, alias) + } + if err != nil { + t.Skipf("%s unsupported: %v", aliasKind, err) + } + + registry := NewRegistry() + registry.Register(NewScopedReadFileTool(ws, nil)) + registry.Register(NewScopedWriteFileTool(ws, nil)) + + read := registry.RunWithOptions(context.Background(), "read_file", map[string]any{"path": alias}, RunOptions{Sandbox: engine}) + if read.Status == StatusOK || strings.Contains(read.Output, "bridge-secret") { + t.Fatalf("read_file followed protected %s: status=%s output=%q", aliasKind, read.Status, read.Output) + } + + write := registry.RunWithOptions(context.Background(), "write_file", map[string]any{"path": alias, "content": "attacker-controlled\n"}, RunOptions{Sandbox: engine}) + if write.Status == StatusOK { + t.Fatalf("write_file followed protected %s: output=%q", aliasKind, write.Output) + } + contents, err := os.ReadFile(token) + if err != nil || string(contents) != "bridge-secret\n" { + t.Fatalf("token changed after denied write through %s: contents=%q err=%v", aliasKind, contents, err) + } + + grep, ok := NewScopedGrepTool(ws, nil).(sandboxAwareTool) + if !ok { + t.Fatal("grep tool must be sandbox-aware") + } + result := grep.RunWithSandbox(context.Background(), map[string]any{ + "pattern": "bridge-secret", + "output_mode": "files_with_matches", + }, engine) + if result.Status != StatusOK { + t.Fatalf("grep failed: %s", result.Output) + } + if strings.Contains(result.Output, "token-alias") || strings.Contains(result.Output, "bridge-token") { + t.Fatalf("grep surfaced protected %s target:\n%s", aliasKind, result.Output) + } + if !strings.Contains(result.Output, "main.go") { + t.Fatalf("grep omitted ordinary file while filtering %s:\n%s", aliasKind, result.Output) + } + }) + } +} diff --git a/internal/tools/exec_command_test.go b/internal/tools/exec_command_test.go index 62671cbb1..312570a9d 100644 --- a/internal/tools/exec_command_test.go +++ b/internal/tools/exec_command_test.go @@ -177,6 +177,13 @@ func TestExecCommandRequireEscalatedBypassesNativeSandboxAfterApproval(t *testin } } +func TestExecCommandRequireEscalatedKeepsSandboxWhenDaemonTokenProtected(t *testing.T) { + engine := defaultPolicyDaemonTokenEngine(t) + if commandEngineForSandboxPermissions(engine, SandboxPermissionsRequireEscalated) == nil { + t.Fatal("exec_command require_escalated must preserve the sandbox when the default remote-daemon policy protects a token file") + } +} + // TestExecCommandRequireEscalatedBypassesMsysGuardAfterApproval mirrors // TestBashToolRequireEscalatedMsysGuard for exec_command: the MSYS sandbox // guard exists only because MSYS/Cygwin coreutils fail under the diff --git a/internal/tools/list_directory.go b/internal/tools/list_directory.go index dd764bd6f..008e69e5a 100644 --- a/internal/tools/list_directory.go +++ b/internal/tools/list_directory.go @@ -46,14 +46,14 @@ func NewScopedListDirectoryTool(workspaceRoot string, scope PathScope) Tool { } func (tool listDirectoryTool) Run(_ context.Context, args map[string]any) Result { - return tool.run(args, true) + return tool.run(args, readExcluder{}, true) } -func (tool listDirectoryTool) RunWithOptions(_ context.Context, args map[string]any, _ RunOptions) Result { - return tool.run(args, false) +func (tool listDirectoryTool) RunWithOptions(_ context.Context, args map[string]any, options RunOptions) Result { + return tool.run(args, sandboxReadExcluder(options.Sandbox), false) } -func (tool listDirectoryTool) run(args map[string]any, directBudget bool) Result { +func (tool listDirectoryTool) run(args map[string]any, exclude readExcluder, directBudget bool) Result { // Optional with a "." default: treat an explicit empty path (a common // weak-model quirk) the same as the key being absent rather than erroring. requestedPath, err := aliasedStringArg(args, []string{"path", "directory", "dir"}, ".", false, true) @@ -80,7 +80,7 @@ func (tool listDirectoryTool) run(args map[string]any, directBudget bool) Result return errorResult("Error listing directory " + requestedPath + ": " + err.Error()) } - entries, err := listDirectoryEntries(absolutePath, 0, maxDepth) + entries, err := listDirectoryEntries(absolutePath, 0, maxDepth, exclude) if err != nil { return errorResult("Error listing directory " + relativePath + ": " + err.Error()) } @@ -95,7 +95,7 @@ func (tool listDirectoryTool) run(args map[string]any, directBudget bool) Result return result } -func listDirectoryEntries(path string, depth int, maxDepth int) ([]string, error) { +func listDirectoryEntries(path string, depth int, maxDepth int, exclude readExcluder) ([]string, error) { dirEntries, err := os.ReadDir(path) if err != nil { return nil, err @@ -112,18 +112,29 @@ func listDirectoryEntries(path string, depth int, maxDepth int) ([]string, error if entry.IsDir() && shouldSkipDirectory(entry.Name()) { continue } + entryPath := filepath.Join(path, entry.Name()) + if entry.IsDir() && exclude.dirExcluded(entryPath) { + continue + } indent := strings.Repeat(" ", depth) if entry.IsDir() { - results = append(results, indent+entry.Name()+"/") + // A denied directory with a nested AllowRead cannot be pruned, but the + // denied directory entry itself must still stay out of the listing. + if !exclude.fileExcluded(entryPath) { + results = append(results, indent+entry.Name()+"/") + } if depth < maxDepth { - children, err := listDirectoryEntries(filepath.Join(path, entry.Name()), depth+1, maxDepth) + children, err := listDirectoryEntries(entryPath, depth+1, maxDepth, exclude) if err == nil { results = append(results, children...) } } continue } + if exclude.fileExcluded(entryPath) { + continue + } results = append(results, fmt.Sprintf("%s%s", indent, entry.Name())) } diff --git a/internal/tools/read_exclusions_test.go b/internal/tools/read_exclusions_test.go index 1feb9880b..0ca174a99 100644 --- a/internal/tools/read_exclusions_test.go +++ b/internal/tools/read_exclusions_test.go @@ -97,3 +97,68 @@ func TestGlobSkipsDenyReadSubtree(t *testing.T) { t.Fatalf("non-sandboxed glob should include the secret file, got:\n%s", plain.Output) } } + +func TestListDirectorySkipsDenyReadSubtree(t *testing.T) { + ws, engine := denyReadFixture(t) + registry := NewRegistry() + registry.Register(NewScopedListDirectoryTool(ws, nil)) + args := map[string]any{"path": ".", "recursive": true, "max_depth": 2} + + sandboxed := registry.RunWithOptions(context.Background(), "list_directory", args, RunOptions{Sandbox: engine}) + if sandboxed.Status != StatusOK { + t.Fatalf("list_directory failed: %s", sandboxed.Output) + } + if !strings.Contains(sandboxed.Output, "main.go") { + t.Fatalf("list_directory must still show the non-denied file, got:\n%s", sandboxed.Output) + } + if strings.Contains(sandboxed.Output, "secret") || strings.Contains(sandboxed.Output, "creds.go") { + t.Fatalf("list_directory must NOT surface a DenyRead subtree, got:\n%s", sandboxed.Output) + } + + plain := NewScopedListDirectoryTool(ws, nil).Run(context.Background(), args) + if !strings.Contains(plain.Output, "secret/") || !strings.Contains(plain.Output, "creds.go") { + t.Fatalf("non-sandboxed list_directory should include the secret subtree, got:\n%s", plain.Output) + } +} + +func TestListDirectoryDescendsToNestedAllowRead(t *testing.T) { + ws, err := filepath.EvalSymlinks(t.TempDir()) + if err != nil { + t.Fatalf("EvalSymlinks: %v", err) + } + secret := filepath.Join(ws, "secret") + allowed := filepath.Join(secret, "allowed") + if err := os.MkdirAll(allowed, 0o755); err != nil { + t.Fatalf("mkdir allowed: %v", err) + } + if err := os.WriteFile(filepath.Join(secret, "hidden.txt"), []byte("hidden\n"), 0o600); err != nil { + t.Fatalf("write hidden: %v", err) + } + if err := os.WriteFile(filepath.Join(allowed, "visible.txt"), []byte("visible\n"), 0o600); err != nil { + t.Fatalf("write visible: %v", err) + } + engine := sandbox.NewEngine(sandbox.EngineOptions{ + WorkspaceRoot: ws, + Policy: sandbox.Policy{ + Mode: sandbox.ModeEnforce, + EnforceWorkspace: true, + DenyRead: []string{secret}, + AllowRead: []string{allowed}, + }, + }) + registry := NewRegistry() + registry.Register(NewScopedListDirectoryTool(ws, nil)) + + result := registry.RunWithOptions(context.Background(), "list_directory", map[string]any{ + "path": ".", "recursive": true, "max_depth": 3, + }, RunOptions{Sandbox: engine}) + if result.Status != StatusOK { + t.Fatalf("list_directory failed: %s", result.Output) + } + if !strings.Contains(result.Output, "allowed/") || !strings.Contains(result.Output, "visible.txt") { + t.Fatalf("list_directory must descend to the nested AllowRead subtree, got:\n%s", result.Output) + } + if strings.Contains(result.Output, "secret/") || strings.Contains(result.Output, "hidden.txt") { + t.Fatalf("list_directory must hide denied entries outside the nested AllowRead subtree, got:\n%s", result.Output) + } +}