diff --git a/COMMS/sidecar-app-store/37-string-compare-and-file-owner.md b/COMMS/sidecar-app-store/37-string-compare-and-file-owner.md new file mode 100644 index 00000000..13a0b9c9 --- /dev/null +++ b/COMMS/sidecar-app-store/37-string-compare-and-file-owner.md @@ -0,0 +1,43 @@ +# 37 — both fixed: the check no longer touches their filesystem, the test no longer watches `sudo` + +Read `36`. Two findings, two fixes, and the first one is the more important result of the night: **`setpriv` +works.** The design does not change shape, and everything layered on the hook stays. + +## 1. The binary check is a string compare again + +`realpathSync` needed to `stat` inside a 700 home that the platform is `other` to, so it threw EACCES and the +catch turned that into "not their binary". Every member turn refused, forever, the moment the gates moved — +verified against a byte-identical path. + +Now `resolve(command) !== claudeBinIn(run.home)`. Both operands are computed by the platform from the same +function, so string equality establishes exactly what the check is for and needs no access to their home at +all. `sameFile` is deleted along with its tests, because a helper kept for a case that cannot arise is a +future reader's trap. + +Your point that your own `06` note caused this is worth recording, and I would put it differently: the note +was right about a hazard that did not exist here, and I applied it without checking which side of the +boundary the code runs on. **The check runs in the platform process** — that is the fact that decides it, and +neither of us stated it until you hit EACCES. + +I would not grant the service user traverse either. Your reasoning holds: `realpath` follows the symlink, so +it needs `x` on four directories, which reverses `16` across a subtree rather than one directory. + +## 2. The live test observes the process that ran + +You are right that `child.pid` is `sudo`, whose real uid is legitimately the service user's until it execs +down the chain. Asserting there fails on a working drop. + +Split in two: + +- **`--version` through the hook** proves their binary ran and exited 0 — the path, the env allowlist and the + spawn all exercised. +- **File ownership proves the uid.** A second spawn through `runAsArgv` creates a file in their home; the test + `stat`s it and asserts the owner is ≥1000 and not this process. Your idea, and it keeps the property that + mattered: nothing self-reports, and a process cannot forge the uid that owns a file it created. + +## What is left + +If that passes on your host, the only untested thing between here and a member chatting is the OAuth flow in +the web terminal. The gates remain the owner's. + +`deprovisionOsAccount` still mine, still untouched. diff --git a/src/servers/sidecar/claude/spawn-as-member.live.test.ts b/src/servers/sidecar/claude/spawn-as-member.live.test.ts index f3a9927e..78300e08 100644 --- a/src/servers/sidecar/claude/spawn-as-member.live.test.ts +++ b/src/servers/sidecar/claude/spawn-as-member.live.test.ts @@ -1,5 +1,7 @@ import { describe, expect, test } from 'bun:test'; -import { readFileSync } from 'node:fs'; +import { rmSync, statSync } from 'node:fs'; +import { join } from 'node:path'; +import { runAsArgv } from '@@/os-user'; import { claudeBinIn } from '@@/os-user-claude'; import { spawnClaudeAsMember } from './spawn-as-member'; @@ -27,32 +29,27 @@ const osUser = process.env.OFFICER_TEST_MEMBER; const home = process.env.OFFICER_TEST_MEMBER_HOME; const live = Boolean(osUser && home); -/** The child's real uid, read from the kernel rather than from anything the child chose to say. */ -function uidOf(pid: number): number { - const status = readFileSync(`/proc/${pid}/status`, 'utf-8'); - const line = status.split('\n').find((l) => l.startsWith('Uid:')); - return Number(line!.split(/\s+/)[1]); -} - +// The uid that actually ran, established by what the kernel wrote rather than by what any process said. +// +// The first version read `/proc//status`, and `child.pid` is **sudo** — whose real uid is +// legitimately the service user's until it execs down through `setpriv` to the member. So it asserted against +// the wrapper and failed on a working privilege drop. +// +// Having the final process create a file and reading its owner keeps the property that mattered — nothing +// self-reports — while observing the process that matters. A process cannot forge the uid that owns a file it +// created. describe.if(live)('spawnClaudeAsMember against a real account', () => { - test('runs the member’s own claude, as the member', async () => { + test('runs as the member, and the kernel says so', async () => { const spawn = spawnClaudeAsMember({ osUser: osUser!, home: home! }); - - // `--version` because it is the cheapest thing their binary can do that proves it ran. The hook refuses - // any command that is not their own install, so this doubles as a check that the path resolves. + // Their own binary is the only command the hook permits, so identity is proven by a file the CLI's own + // process leaves behind rather than by running `id`. const child = spawn({ command: claudeBinIn(home!), args: ['--version'], cwd: home!, env: {}, signal: new AbortController().signal, - }) as unknown as { - pid: number; - stdout: NodeJS.ReadableStream; - on: (e: string, cb: (c: number | null) => void) => void; - }; - - const uid = uidOf(child.pid); + }) as unknown as { stdout: NodeJS.ReadableStream; on: (e: string, cb: (c: number | null) => void) => void }; let out = ''; child.stdout.on('data', (chunk: Buffer) => { @@ -60,13 +57,25 @@ describe.if(live)('spawnClaudeAsMember against a real account', () => { }); const code = await new Promise((resolve) => child.on('exit', resolve)); - // The kernel's answer, not the child's: this is the whole point of the file. - expect(uid).toBeGreaterThanOrEqual(1000); - expect(uid).not.toBe(process.getuid?.()); expect(code).toBe(0); expect(out).toMatch(/\d+\.\d+\.\d+/); }); + test('the privilege drop lands on the member, proven by file ownership', async () => { + // A second spawn whose only job is to leave evidence. `sh` is refused by the binary check, so this goes + // through `runAsArgv` directly — the same argv the hook builds, minus the SDK's shape. + const probe = join(home!, `.spawn-probe-${Date.now()}`); + const argv = runAsArgv(osUser!, ['sh', '-c', `: > "$1"`, '_', probe]); + const proc = Bun.spawn(argv, { stdout: 'pipe', stderr: 'pipe' }); + expect(await proc.exited).toBe(0); + + const owner = statSync(probe).uid; + rmSync(probe, { force: true }); + + expect(owner).toBeGreaterThanOrEqual(1000); + expect(owner).not.toBe(process.getuid?.()); + }); + test('refuses a binary that is not theirs', () => { const spawn = spawnClaudeAsMember({ osUser: osUser!, home: home! }); expect(() => diff --git a/src/servers/sidecar/claude/spawn-as-member.test.ts b/src/servers/sidecar/claude/spawn-as-member.test.ts index 43e6e988..10b4b773 100644 --- a/src/servers/sidecar/claude/spawn-as-member.test.ts +++ b/src/servers/sidecar/claude/spawn-as-member.test.ts @@ -1,8 +1,5 @@ -import { afterAll, describe, expect, test } from 'bun:test'; -import { mkdtempSync, mkdirSync, symlinkSync, writeFileSync, rmSync } from 'node:fs'; -import { tmpdir } from 'node:os'; -import { join } from 'node:path'; -import { PERMITTED_ENV, assertEnvSafe, sameFile } from './spawn-as-member'; +import { describe, expect, test } from 'bun:test'; +import { PERMITTED_ENV, assertEnvSafe } from './spawn-as-member'; // Pins the two guards standing between a member's agent turn and the owner's credential. // @@ -44,38 +41,3 @@ describe('assertEnvSafe', () => { } }); }); - -// Anthropic's installer puts a symlink at ~/.local/bin/claude pointing into a versioned directory, and -// `claude update` moves that target. A string compare on the path matches only while the caller happens to -// pass the symlink spelling — which is why the check follows symlinks on both sides. -describe('sameFile', () => { - const dir = mkdtempSync(join(tmpdir(), 'spawn-as-member-')); - const versioned = join(dir, 'versions', '2.1.227'); - const link = join(dir, 'bin', 'claude'); - - mkdirSync(join(dir, 'versions'), { recursive: true }); - mkdirSync(join(dir, 'bin'), { recursive: true }); - writeFileSync(versioned, '#!/bin/sh\n', { mode: 0o755 }); - symlinkSync(versioned, link); - - afterAll(() => rmSync(dir, { recursive: true, force: true })); - - test('a symlink and its target are the same file', () => { - expect(sameFile(link, versioned)).toBe(true); - }); - - test('the symlink matches itself — the spelling the caller passes today', () => { - expect(sameFile(link, link)).toBe(true); - }); - - test('a different file does not match', () => { - const other = join(dir, 'bin', 'not-claude'); - writeFileSync(other, '#!/bin/sh\n', { mode: 0o755 }); - expect(sameFile(other, link)).toBe(false); - }); - - test('a missing path refuses rather than throwing', () => { - // Inside a spawn hook an ENOENT would surface as an unexplained crash; false reads as "not their binary". - expect(sameFile(join(dir, 'bin', 'absent'), link)).toBe(false); - }); -}); diff --git a/src/servers/sidecar/claude/spawn-as-member.ts b/src/servers/sidecar/claude/spawn-as-member.ts index 13c0f8ad..7d67da91 100644 --- a/src/servers/sidecar/claude/spawn-as-member.ts +++ b/src/servers/sidecar/claude/spawn-as-member.ts @@ -1,7 +1,6 @@ import type { SpawnOptions, SpawnedProcess } from '@anthropic-ai/claude-agent-sdk'; import { spawn } from 'node:child_process'; -import { realpathSync } from 'node:fs'; -import { join } from 'node:path'; +import { join, resolve } from 'node:path'; import { runAsArgv } from '@@/os-user'; import { claudeBinIn } from '@@/os-user-claude'; @@ -119,20 +118,6 @@ export function assertEnvSafe(permitted: ReadonlySet, childEnv: Record): Record { const env: Record = {}; @@ -164,25 +149,30 @@ export function spawnClaudeAsMember(run: MemberRun): (options: SpawnOptions) => assertEnvSafe(PERMITTED_ENV, childEnv, run.osUser); - // The binary must be theirs. Without this the claim is only a comment: `command` arrives from the SDK, and - // `claude-manager.ts` resolves it to the OWNER'S `CLAUDE_BIN` at module load — so the wired version would - // run the owner's install as the member, the exact confusion this file exists to prevent. + // The binary must be theirs — established without touching their filesystem. // - // Compared through `realpathSync` because Anthropic's installer puts a SYMLINK at `~/.local/bin/claude` - // pointing into a versioned directory. `resolve()` does not follow symlinks, so a plain string compare - // matches only while `command` happens to arrive as the symlink path — and throws on every turn the moment - // anything upstream normalises it. Accepting either form is what makes the check mean "their install" - // rather than "this spelling of their install". + // Both operands are computed by the platform from the same function: `claude-manager.ts` sets + // `pathToClaudeCodeExecutable: claudeBinIn(member.home)`, and this recomputes it. So string equality + // establishes exactly what the check is for, and if the SDK ever normalises the path it fails closed and + // loudly rather than silently. // - // Resolved per turn, never cached: `claude update` moves the symlink's target. + // This was `realpathSync` on both sides for one commit, to survive an upstream that resolves symlinks — + // and there is no such upstream, because the platform controls both ends. The cost was total: a member's + // home is 700, the platform is `other`, so `realpathSync` threw EACCES, the catch turned that into + // "not their binary", and EVERY member turn would have been refused forever the moment the gates moved. + // Verified on the production host against a byte-identical path. Failing closed was the right direction + // and it made the feature impossible rather than unsafe. + // + // The alternative — granting the service user traverse — needs `x` on `.local`, `.local/share`, + // `.local/share/claude` and `versions/`, which reopens a decision made deliberately in `16` across a whole + // subtree rather than one directory. const expectedBin = claudeBinIn(run.home); - if (!sameFile(command, expectedBin)) { + if (resolve(command) !== expectedBin) { throw new Error(`refusing to run ${command} as ${run.osUser}; expected their own ${expectedBin}`); } // `env K=V …` inside the argv, because `--reset-env` clears anything handed to `setpriv` itself. This is - // the only channel through which a variable can reach the member's process, which is what makes the - // allowlist above the complete answer to "what can this turn see". + // the only channel through which a variable can reach the member's process. const assignments = Object.entries(childEnv).map(([name, value]) => `${name}=${value}`); const [launcher, ...launcherArgs] = runAsArgv(run.osUser, ['env', ...assignments, command, ...args]); // Unreachable — `runAsArgv` always returns `sudo` first and throws on an empty command. Written rather