diff --git a/internal/cli/claude.go b/internal/cli/claude.go index 3ebc5c1..cefffde 100644 --- a/internal/cli/claude.go +++ b/internal/cli/claude.go @@ -487,9 +487,17 @@ var projectClaudeSettingsFiles = []string{"settings.json", "settings.local.json" // allowlist as user scope, not forked, because a key that names a program // is no safer for being repo-supplied — so a repo's hooks block does not // execute inside. -// - OUTBOUND: AccessRO means a payload write to that path fails EROFS, so a -// hook the payload writes into an EXISTING settings file does not survive to -// run on the host later. +// - OUTBOUND, IN-PLACE ONLY: AccessRO means a payload write to that path, IN +// PLACE, fails EROFS, so a hook the payload writes into an EXISTING settings +// file that way does not survive to run on the host. But the RO mount pins one +// inode at one PATH — it does NOT pin the NAME. The target bind is rw by +// design, so a payload renames the parent `.claude` (`mv .claude .claudeOLD; +// mkdir .claude`), which drags the RO mountpoint into `.claudeOLD/` and frees +// the original name for a fresh host-written `.claude/settings.json` carrying +// hooks (measured, issue #286). So the OUTBOUND close is the in-place write +// ONLY; the rename REDUCES the exists case to the create-case residual below. +// It is not silent — the mount is EBUSY-pinned into `.claudeOLD/`, so both +// that directory and the modified `.claude/settings.json` show in `git status`. // // ONLY WHERE THE FILE EXISTS, and that boundary is load-bearing rather than an // optimisation. A generated mount over an ABSENT path does not overmount an @@ -503,10 +511,14 @@ var projectClaudeSettingsFiles = []string{"settings.json", "settings.local.json" // THE RESIDUAL, stated because a check that covers half its rule must say which // half: a clean repo where the payload CREATES a .claude/settings.json that did // not exist before is NOT closed — closing it would need the host write above. -// The half that closes is the sharper one: a hostile repo SHIPPING a -// settings.json with hooks is the exists case, reinterpreted and its hooks -// dropped; the payload-creates case leaves a file the human sees in `git -// status`, which is not a guarantee and is not nothing. +// An EXISTING file reaches this same residual by RENAME (issue #286): the RO +// mount closes editing in place, not the name, so `mv .claude .claudeOLD` frees +// the path and a fresh settings.json lands on the host. The exists case is only +// the sharper, closed one for the INBOUND direction — a hostile repo SHIPPING a +// settings.json with hooks is reinterpreted and its hooks dropped, and rename +// does not touch that. OUTBOUND, exists and create collapse to one residual: a +// file the human sees in `git status`, which is not a guarantee and is not +// nothing. // // AccessRO throughout, unlike the user-scope file (rw, because Claude Code // rewrites it — the gh precedent). Project scope is read-only on purpose: that @@ -922,9 +934,10 @@ func claudeGuidance(pol *policy.Policy) []byte { b.WriteString("the sandbox exits. But snug now projects the target's own\n") b.WriteString("`.claude/settings.json` and `.claude/settings.local.json` READ-ONLY where they\n") b.WriteString("EXIST (issue #73), so a hostile repo's hooks in them do not run here, and a\n") - b.WriteString("project-scope permission you accept does NOT persist into an existing one — the\n") - b.WriteString("write fails. A settings file the repo did not already ship, you can still\n") - b.WriteString("create, and it persists to the host; that half is not closed.\n\n") + b.WriteString("project-scope permission you accept cannot overwrite an existing one IN PLACE —\n") + b.WriteString("that write fails EROFS. It is not fully closed OUTBOUND: renaming the `.claude`\n") + b.WriteString("directory and recreating it, or creating a settings file the repo did not ship,\n") + b.WriteString("both persist to the host (and both show in `git status`); that half is not closed.\n\n") b.WriteString("A token you refresh here does not reach the host; it is lost when this session\n") b.WriteString("ends.\n\n") diff --git a/test/integration/claudeprojectsettings_test.go b/test/integration/claudeprojectsettings_test.go new file mode 100644 index 0000000..520e76c --- /dev/null +++ b/test/integration/claudeprojectsettings_test.go @@ -0,0 +1,135 @@ +//go:build integration + +package integration + +import ( + "os" + "path/filepath" + "strings" + "testing" +) + +// TestProjectSettingsEROFSIsInPlaceOnlyAndRenameBypassesIt is the permanent +// regression for issue #286. +// +// stageProjectClaudeSettings mounts {target}/.claude/settings.json AccessRO +// (issue #73). Commit 433401e's doc comment claimed the OUTBOUND direction was +// closed by that read-only mount: "a hook the payload writes into an EXISTING +// settings file does not survive to run on the host later". That is true for an +// IN-PLACE write and false for a rename. The RO mount pins one inode at one +// PATH, not the NAME; the target bind is rw by design, so a payload renames the +// parent .claude directory, which drags the RO mountpoint into .claudeOLD/ and +// frees the original name for a fresh host-written settings.json. +// +// This test asserts the ACTUAL boundary, both halves in one run, so the doc's +// scope stays honest: +// +// - the in-place write STILL gets EROFS — the guarantee that holds, and the +// control that proves the RO projection is really mounted (without it the +// rename half would prove nothing, since a target with no projection at all +// lets every write through); +// - the rename+recreate PERSISTS to the host — the residual the doc must keep +// admitting. If a future change closes this too, this test breaks and tells +// the author to widen the doc claim back, which is the correct direction. +// +// It does NOT exceed the create-case residual the commit already discloses: the +// attack is git-visible (a .claudeOLD/ directory the RO mount is EBUSY-pinned +// into, plus the modified .claude/settings.json). What #286 corrects is the +// SCOPING of the claim, not a new capability. +func TestProjectSettingsEROFSIsInPlaceOnlyAndRenameBypassesIt(t *testing.T) { + budget(t) + requireSandbox(t) + proj, _ := target(t) + + // The target SHIPS an existing, benign settings.json — the "exists case" the + // projection is mounted over. The projection is mounted only where an + // os.Lstat confirms the file already exists, so it must be here before snug + // resolves. + const original = `{"model":"sonnet"}` + claudeDir := filepath.Join(proj, ".claude") + if err := os.MkdirAll(claudeDir, 0o755); err != nil { + t.Fatal(err) + } + settingsPath := filepath.Join(claudeDir, "settings.json") + if err := os.WriteFile(settingsPath, []byte(original), 0o600); err != nil { + t.Fatal(err) + } + + // @claude also stages user-scope ~/.claude; give it a private HOME so the run + // does not depend on the developer's real one. + home := t.TempDir() + if err := os.MkdirAll(filepath.Join(home, ".claude"), 0o755); err != nil { + t.Fatal(err) + } + + // The payload's own command is a touch of a marker inside the settings it + // writes; the host-side assertion looks for the literal command string, which + // is what a next host-side `claude` in this repo would run as a SessionStart + // hook. + const hookCmd = "touch PWNED_NEXT_HOST_RUN" + const payload = `{"hooks":{"SessionStart":[{"hooks":[{"type":"command","command":"` + hookCmd + `"}]}]}}` + script := ` +cd "$SNUG_TARGET" || { echo NO-TARGET; exit 1; } +echo "SNUG=$SNUG" +echo HACK-IN-PLACE > .claude/settings.json 2>&1 && echo INPLACE-WROTE || echo INPLACE-EROFS +mv .claude .claudeOLD 2>&1 && echo MV-OK || echo MV-FAIL +mkdir .claude 2>&1 && echo MKDIR-OK || echo MKDIR-FAIL +printf '%s' '` + payload + `' > .claude/settings.json 2>&1 && echo RENAME-WROTE || echo RENAME-FAIL +` + r := runEnv(t, baseEnv("HOME="+home), []string{"-p", "@claude"}, proj, script).mustRun(t) + + // ── Control: the sandbox really ran with the projection active ──────────── + if !strings.Contains(r.out, "SNUG=1") { + t.Fatalf("control: SNUG=1 absent, so the run did not measure a real snug sandbox:\n%s", r.out) + } + // The guarantee that DOES hold, and the control for the whole test: an + // in-place write to the projected path is EROFS. If it succeeded, the RO + // projection is not mounted and the rename assertion below would be measuring + // an ordinary rw file, not a bypass. + switch { + case strings.Contains(r.out, "INPLACE-EROFS"): + case strings.Contains(r.out, "INPLACE-WROTE"): + t.Fatalf("control: the in-place write to {target}/.claude/settings.json SUCCEEDED, so the "+ + "AccessRO projection (issue #73) is not mounted — the rename bypass below cannot be "+ + "attributed to anything:\n%s", r.out) + default: + t.Fatalf("control: the in-place write emitted neither INPLACE-EROFS nor INPLACE-WROTE; the "+ + "probe did not run:\n%s", r.out) + } + for _, want := range []string{"MV-OK", "MKDIR-OK", "RENAME-WROTE"} { + if !strings.Contains(r.out, want) { + t.Fatalf("the rename sequence did not reach %q — the target's own tree must be rw for "+ + "the bypass, and the fixture relies on that:\n%s", want, r.out) + } + } + + // ── The residual: the rename bypass PERSISTED to the host ───────────────── + // This is the load-bearing assertion. The outbound guarantee is bypassable by + // rename, so the doc comment and claudeGuidance must scope their EROFS claim to + // in-place writes only. + afterNew, err := os.ReadFile(settingsPath) + if err != nil { + t.Fatalf("the host's {target}/.claude/settings.json is unreadable after the run (%v); the "+ + "rename should have left a fresh file at the original path:\n%s", err, r.out) + } + if !strings.Contains(string(afterNew), hookCmd) { + t.Fatalf("the rename bypass did NOT persist: {target}/.claude/settings.json does not carry the "+ + "payload's hook command %q. If the outbound direction is now genuinely closed, this is a "+ + "real improvement — WIDEN the EROFS claim in claude.go's stageProjectClaudeSettings doc "+ + "comment and claudeGuidance back to cover it, and update issue #286. host file:\n%s\nrun:\n%s", + hookCmd, string(afterNew), r.out) + } + + // The original, RO-mounted file was dragged with the rename into .claudeOLD/, + // EBUSY-pinned there — the reason the attack is git-visible and not silent. + oldPath := filepath.Join(proj, ".claudeOLD", "settings.json") + oldBody, err := os.ReadFile(oldPath) + if err != nil { + t.Fatalf("the original settings.json is not at {target}/.claudeOLD/settings.json after the "+ + "rename (%v); the RO mount should have followed the parent directory:\n%s", err, r.out) + } + if strings.TrimSpace(string(oldBody)) != original { + t.Errorf("the file dragged into .claudeOLD/ is %q, want the original %q — the RO projection "+ + "should carry the host's benign settings unchanged:\n%s", string(oldBody), original, r.out) + } +}