From 6bef94ea0d03cd95a3e9af52b9d2c3e44a7839cc Mon Sep 17 00:00:00 2001 From: Hermes Agent Date: Thu, 16 Jul 2026 15:43:16 -0500 Subject: [PATCH] fix(mosaic): close dup-proxy race + verify listener identity (re-review #2, #790) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Addresses the three items from PR #793 re-review #2, all TDD red-first on this branch: F1 (BLOCKER, dup-proxy race) — ensureProxyRunning no longer falls back to nohup after `systemctl start` is ACCEPTED. An accepted-but-not-yet-healthy systemd unit may bind late or be restarted; spawning nohup then would create a second proxy contending for :18765. Once systemd accepts the job we poll to the startup deadline and, on a miss, report a managed-service startup failure. nohup is now reachable only when systemd never accepted the job. New test: "does NOT fall back to nohup after systemd accepts but never binds". F2 (BLOCKER, CWE-345) — a 2xx on /healthz is liveness, not identity. Added verifyListenerIdentity(): an OS-level check (via `ss` + /proc) that the process owning :18765 is the current uid running the expected proxy executable, failing CLOSED (`unknown`) when identity can't be established — needs no upstream shared-secret/unix-socket support. ensureProxyRunning now gates EVERY trust point on it: an already-present responder that fails identity returns `untrusted` without starting anything; a started proxy is trusted only once it is both live AND identity-verified. probeLiveness doc-comment updated to record the residual CWE-345 risk and point at the identity check (multi-user warning deferred). F3 (SHOULD-FIX) — parseAuthStatus no longer treats a signal-terminated check (status null) with an auth-looking line as valid; a clean exit 0 is required. Regression test: { status: null, stdout: 'Authenticated' } → unknown. Gates green: typecheck, lint, format:check; full mosaic suite 1110 passing; claudex-proxy.ts coverage 92.3% stmts/lines, 88.13% branch (>= 85%). Co-Authored-By: Claude Opus 4.8 --- .../mosaic/src/commands/claudex-proxy.spec.ts | 173 +++++++++++++++-- packages/mosaic/src/commands/claudex-proxy.ts | 176 +++++++++++++++--- 2 files changed, 309 insertions(+), 40 deletions(-) diff --git a/packages/mosaic/src/commands/claudex-proxy.spec.ts b/packages/mosaic/src/commands/claudex-proxy.spec.ts index ddc9c4a..5b43650 100644 --- a/packages/mosaic/src/commands/claudex-proxy.spec.ts +++ b/packages/mosaic/src/commands/claudex-proxy.spec.ts @@ -21,11 +21,13 @@ import { systemdUnitPath, installSystemdUnit, startNohupProxy, + verifyListenerIdentity, runProxyPreflight, ensureProxyRunning, type AuthStatus, type ProxyRunResult, type SpawnedChild, + type ListenerIdentity, } from './claudex-proxy.js'; /** @@ -94,6 +96,14 @@ describe('parseAuthStatus', () => { expect(s.state).toBe('unknown'); }); + it('does NOT trust a signal-terminated check (status null) even with an auth-looking line', () => { + // status: null means the process was killed by a signal — an INCOMPLETE + // check. An auth-looking line that happened to be flushed must not be read + // as valid, or preflight passes on a check that never finished. + const s = parseAuthStatus({ status: null, stdout: 'Authenticated', stderr: '' }); + expect(s.state).toBe('unknown'); + }); + it('extracts a best-effort expiry in days when present', () => { const s = parseAuthStatus({ status: 0, @@ -521,15 +531,93 @@ describe('startNohupProxy (finding #1 — async spawn error must not crash)', () }); }); +describe('verifyListenerIdentity (finding #2 — OS-level listener identity, CWE-345)', () => { + const me: ListenerIdentity = { + pid: 4242, + uid: 1000, + exePath: '/home/me/.local/bin/claude-code-proxy', + }; + + it('accepts a listener owned by the current uid whose exe is the expected proxy path', () => { + const verdict = verifyListenerIdentity({ + identify: () => me, + currentUid: () => 1000, + expectedExe: () => '/home/me/.local/bin/claude-code-proxy', + }); + expect(verdict).toBe('ok'); + }); + + it('accepts by basename when the expected path is unknown but the exe is claude-code-proxy', () => { + const verdict = verifyListenerIdentity({ + identify: () => me, + currentUid: () => 1000, + expectedExe: () => null, + }); + expect(verdict).toBe('ok'); + }); + + it('rejects a listener owned by a DIFFERENT uid (foreign-user) — fail closed', () => { + const verdict = verifyListenerIdentity({ + identify: () => ({ ...me, uid: 0 }), + currentUid: () => 1000, + expectedExe: () => '/home/me/.local/bin/claude-code-proxy', + }); + expect(verdict).toBe('foreign-user'); + }); + + it('rejects a same-user listener whose exe is NOT the proxy (wrong-exe)', () => { + const verdict = verifyListenerIdentity({ + identify: () => ({ ...me, exePath: '/usr/bin/nc' }), + currentUid: () => 1000, + expectedExe: () => '/home/me/.local/bin/claude-code-proxy', + }); + expect(verdict).toBe('wrong-exe'); + }); + + it('returns unknown (fail closed) when the listener cannot be identified', () => { + const verdict = verifyListenerIdentity({ + identify: () => null, + currentUid: () => 1000, + expectedExe: () => '/home/me/.local/bin/claude-code-proxy', + }); + expect(verdict).toBe('unknown'); + }); + + it('returns unknown when the current uid is unavailable (non-posix)', () => { + const verdict = verifyListenerIdentity({ + identify: () => me, + currentUid: () => -1, + expectedExe: () => '/home/me/.local/bin/claude-code-proxy', + }); + expect(verdict).toBe('unknown'); + }); + + it('returns unknown when the listener exe path cannot be read', () => { + const verdict = verifyListenerIdentity({ + identify: () => ({ ...me, exePath: null }), + currentUid: () => 1000, + expectedExe: () => '/home/me/.local/bin/claude-code-proxy', + }); + expect(verdict).toBe('unknown'); + }); + + it('runs with real defaults without throwing (identity may be unresolved → verdict)', () => { + const verdict = verifyListenerIdentity(); + expect(['ok', 'foreign-user', 'wrong-exe', 'unknown']).toContain(verdict); + }); +}); + describe('ensureProxyRunning', () => { const ok: ProxyRunResult = { status: 0, stdout: '', stderr: '' }; const nohupOk = async (): Promise => ok; + const trusted = () => 'ok' as const; - it('is a no-op when the proxy is already live', async () => { + it('is a no-op when the proxy is already live AND identity-verified', async () => { const startSystemd = vi.fn(() => ok); const startNohup = vi.fn(nohupOk); const r = await ensureProxyRunning({ probe: async () => true, + verifyListener: trusted, startSystemd, startNohup, waitMs: async () => {}, @@ -540,10 +628,30 @@ describe('ensureProxyRunning', () => { expect(startNohup).not.toHaveBeenCalled(); }); - it('starts via systemd when available and then becomes live', async () => { + it('fails closed as untrusted when a responder holds :18765 but identity is NOT ours', async () => { + // A foreign process answers /healthz but the listener is not our proxy + // (foreign uid / wrong exe / unidentifiable). We must NOT trust it and must + // NOT start a second proxy (the port is already taken) — fail closed. + const startSystemd = vi.fn(() => ok); + const startNohup = vi.fn(nohupOk); + const r = await ensureProxyRunning({ + probe: async () => true, + verifyListener: () => 'foreign-user', + startSystemd, + startNohup, + waitMs: async () => {}, + }); + expect(r.method).toBe('untrusted'); + expect(r.live).toBe(false); + expect(startSystemd).not.toHaveBeenCalled(); + expect(startNohup).not.toHaveBeenCalled(); + }); + + it('starts via systemd when available and then becomes trusted-live', async () => { let calls = 0; const r = await ensureProxyRunning({ probe: async () => calls++ > 0, // dead first, live after start + verifyListener: trusted, startSystemd: () => ok, startNohup: async () => { throw new Error('should not fall back'); @@ -554,7 +662,7 @@ describe('ensureProxyRunning', () => { expect(r.live).toBe(true); }); - it('waits past a slow systemd bind before falling back (finding #2 — no duplicate proxy)', async () => { + it('waits past a slow systemd bind before giving up (finding #2 — no duplicate proxy)', async () => { // systemd `start` returns 0 (job accepted) but the socket only binds on the // 4th probe — still well within the startup deadline. nohup must NOT run, // or two proxies would contend for :18765. @@ -562,6 +670,7 @@ describe('ensureProxyRunning', () => { const startNohup = vi.fn(nohupOk); const r = await ensureProxyRunning({ probe: async () => probes++ >= 3, + verifyListener: trusted, startSystemd: () => ok, startNohup, waitMs: async () => {}, @@ -573,28 +682,54 @@ describe('ensureProxyRunning', () => { expect(startNohup).not.toHaveBeenCalled(); }); - it('falls back to nohup when systemd is accepted but never binds in the deadline', async () => { + it('does NOT fall back to nohup after systemd accepts but never binds (finding #1 — dup-proxy race)', async () => { + // systemctl start exit 0 means the job was ACCEPTED, not bound. If it binds + // just after our deadline (or systemd restarts it), a nohup fallback would + // create a SECOND proxy contending for :18765. Once systemd has accepted the + // job we never spawn nohup — we report a managed-service startup failure. const startNohup = vi.fn(nohupOk); const r = await ensureProxyRunning({ - // Dead until nohup has actually run; systemd's whole poll window stays dead. - probe: async () => startNohup.mock.calls.length > 0, + probe: async () => false, // never becomes live within the deadline + verifyListener: trusted, startSystemd: () => ok, startNohup, waitMs: async () => {}, settleMs: 10, startupDeadlineMs: 30, }); - expect(startNohup).toHaveBeenCalledTimes(1); - expect(r.method).toBe('nohup'); - expect(r.live).toBe(true); + expect(startNohup).not.toHaveBeenCalled(); + expect(r.method).toBe('failed'); + expect(r.live).toBe(false); }); - it('falls back to nohup when systemd start fails outright', async () => { + it('does NOT trust a systemd-started responder whose identity cannot be verified', async () => { + // Dead at first (so we reach the systemd start), then the socket binds — but + // identity never verifies (e.g. a squatter beat systemd to the port). A live + // responder that fails identity must never be reported as a successful start. + let calls = 0; + const startNohup = vi.fn(nohupOk); + const r = await ensureProxyRunning({ + probe: async () => calls++ > 0, + verifyListener: () => 'wrong-exe', + startSystemd: () => ok, + startNohup, + waitMs: async () => {}, + settleMs: 10, + startupDeadlineMs: 30, + }); + expect(startNohup).not.toHaveBeenCalled(); + expect(r.method).toBe('failed'); + expect(r.live).toBe(false); + }); + + it('falls back to nohup only when systemd start FAILS outright (not accepted)', async () => { let calls = 0; const r = await ensureProxyRunning({ // A failed systemd start skips its post-start poll, so probes are: - // #0 initial (dead), #1 after nohup (live). + // #0 initial (dead), #1 after nohup (live). nohup fallback is reachable + // ONLY because systemd never accepted the job (status 1). probe: async () => calls++ > 0, + verifyListener: trusted, startSystemd: () => ({ status: 1, stdout: '', stderr: 'no systemd' }), startNohup: nohupOk, waitMs: async () => {}, @@ -605,9 +740,25 @@ describe('ensureProxyRunning', () => { expect(r.live).toBe(true); }); + it('does NOT trust a nohup-started responder whose identity cannot be verified', async () => { + let calls = 0; + const r = await ensureProxyRunning({ + probe: async () => calls++ > 0, + verifyListener: () => 'unknown', + startSystemd: () => ({ status: 1, stdout: '', stderr: 'no systemd' }), + startNohup: nohupOk, + waitMs: async () => {}, + settleMs: 10, + startupDeadlineMs: 30, + }); + expect(r.method).toBe('failed'); + expect(r.live).toBe(false); + }); + it('reports failed when nothing brings the proxy up', async () => { const r = await ensureProxyRunning({ probe: async () => false, + verifyListener: trusted, startSystemd: () => ({ status: 1, stdout: '', stderr: '' }), startNohup: async () => ({ status: 1, stdout: '', stderr: '' }), waitMs: async () => {}, diff --git a/packages/mosaic/src/commands/claudex-proxy.ts b/packages/mosaic/src/commands/claudex-proxy.ts index b162c65..a749899 100644 --- a/packages/mosaic/src/commands/claudex-proxy.ts +++ b/packages/mosaic/src/commands/claudex-proxy.ts @@ -17,9 +17,9 @@ */ import { execFileSync, spawn, spawnSync } from 'node:child_process'; -import { mkdirSync, writeFileSync } from 'node:fs'; +import { mkdirSync, readFileSync, readlinkSync, writeFileSync } from 'node:fs'; import { homedir } from 'node:os'; -import { dirname, join } from 'node:path'; +import { basename, dirname, join } from 'node:path'; // ─── Endpoint / command constants (spec table) ────────────────────────────── @@ -126,7 +126,10 @@ export function parseAuthStatus(result: ProxyRunResult): AuthStatus { state = 'expired'; } else if (unauth) { state = 'unauthenticated'; - } else if (authed && (result.status === 0 || result.status === null)) { + } else if (authed && result.status === 0) { + // A `null` status means the check was killed by a signal — an INCOMPLETE + // run. We require a clean exit 0 for `valid`; a partially-flushed auth line + // from a signal-terminated check must never be trusted (finding #3). state = 'valid'; } else if (result.status === 0) { state = 'valid'; @@ -181,16 +184,21 @@ export function runDeviceReauth(spawnImpl: InheritSpawn = defaultInheritSpawn): * Probe the proxy for liveness by hitting its dedicated `GET /healthz` endpoint * and requiring a 2xx response. * - * This deliberately does NOT trust an arbitrary HTTP response on the port. The - * proxy binds loopback with no client authentication, so on a shared host any - * local process could occupy :18765; trusting "any response = alive" (as an - * earlier revision did) would let such a squatter be mistaken for the proxy and - * intercept Claude traffic (CWE-345). Requiring a 2xx on the proxy's own - * `/healthz` contract is the strongest identity signal available without an - * upstream shared-secret/unix-socket handshake (which `claude-code-proxy` does - * not provide). It also resolves spec gotcha #1: the root path returns non-2xx, - * but `/healthz` returns 2xx when healthy, so a live proxy is never mistaken for - * dead and no duplicate is spawned. + * This is a LIVENESS check only — it answers "is a healthy proxy responding?", + * not "is that responder actually ours?". Requiring a 2xx on the proxy's own + * `/healthz` contract (rather than "any HTTP response = alive") resolves spec + * gotcha #1: the root path returns non-2xx, but `/healthz` returns 2xx when + * healthy, so a live proxy is never mistaken for dead and no duplicate proxy is + * spawned. + * + * Residual risk (CWE-345): the proxy binds loopback with NO client + * authentication, so on a shared host a local process could occupy :18765 and + * serve a 2xx here. A 2xx therefore does NOT by itself establish that the + * listener is our proxy. Identity is verified SEPARATELY and at every trust + * point by {@link verifyListenerIdentity} (OS-level uid + executable check), + * which fails closed when identity can't be established. See + * {@link ensureProxyRunning}. (Broader multi-user hardening — a persistent + * warning when a foreign listener is seen — is tracked for a later phase.) */ export async function probeLiveness( url: string = CLAUDEX_HEALTH_URL, @@ -222,6 +230,92 @@ export async function probeLiveness( } } +// ─── Listener identity (OS-level, CWE-345 mitigation) ───────────────────────── + +/** + * The result of verifying who actually owns the :18765 listener. + * - `ok` — same-user process running the expected proxy binary. + * - `foreign-user` — a process owned by a DIFFERENT uid holds the port. + * - `wrong-exe` — same-user, but the executable is not the proxy. + * - `unknown` — identity could not be established (fail closed). + */ +export type ListenerVerdict = 'ok' | 'foreign-user' | 'wrong-exe' | 'unknown'; + +/** OS-level identity of the process bound to the proxy port. */ +export interface ListenerIdentity { + pid: number; + uid: number; + /** Absolute path of the process executable, or null if unreadable. */ + exePath: string | null; +} + +export interface VerifyListenerDeps { + /** Resolve the process bound to the proxy port (null → unidentifiable). */ + identify?: () => ListenerIdentity | null; + /** The current process uid (-1 when unavailable, e.g. non-posix). */ + currentUid?: () => number; + /** The expected proxy executable path (null when it can't be resolved). */ + expectedExe?: () => string | null; +} + +/** + * Identify the process listening on the proxy port via `ss` + `/proc`. Every + * failure path returns null so the caller fails closed. Reads no credential + * material — only pid/uid/exe path of the listener. + */ +function defaultIdentifyListener(port: number = CLAUDEX_PROXY_PORT): ListenerIdentity | null { + try { + const out = execFileSync('ss', ['-H', '-ltnp', `sport = :${port}`], { encoding: 'utf8' }); + const pidMatch = /pid=(\d+)/.exec(out); + if (!pidMatch) return null; + const pid = Number(pidMatch[1]); + if (!Number.isInteger(pid) || pid <= 0) return null; + + const status = readFileSync(`/proc/${pid}/status`, 'utf8'); + const uidLine = /^Uid:\s*(\d+)/m.exec(status); + if (!uidLine) return null; + const uid = Number(uidLine[1]); + + let exePath: string | null = null; + try { + exePath = readlinkSync(`/proc/${pid}/exe`); + } catch { + exePath = null; + } + return { pid, uid, exePath }; + } catch { + return null; + } +} + +/** + * Verify that the process owning :18765 is genuinely OUR proxy before trusting + * it. The proxy binds loopback with NO client authentication, so on a shared + * host any local process could squat the port and a liveness 2xx alone does not + * prove identity (CWE-345). We check the OS-level identity of the listener — it + * must be owned by the current uid and be the expected proxy executable — and + * FAIL CLOSED (`unknown`) whenever identity cannot be established. This needs no + * upstream shared-secret/unix-socket support from `claude-code-proxy`. + */ +export function verifyListenerIdentity(deps: VerifyListenerDeps = {}): ListenerVerdict { + const identify = deps.identify ?? (() => defaultIdentifyListener()); + const currentUid = + deps.currentUid ?? (() => (typeof process.getuid === 'function' ? process.getuid() : -1)); + const expectedExe = deps.expectedExe ?? (() => checkProxyBinary().path); + + const id = identify(); + if (!id) return 'unknown'; // can't see the listener → don't trust it + const uid = currentUid(); + if (uid < 0) return 'unknown'; // can't establish our own identity → fail closed + if (id.uid !== uid) return 'foreign-user'; // someone else's process holds the port + if (!id.exePath) return 'unknown'; // can't confirm the executable → fail closed + + const expected = expectedExe(); + if (expected && id.exePath === expected) return 'ok'; + if (basename(id.exePath) === CLAUDEX_PROXY_BINARY) return 'ok'; + return 'wrong-exe'; +} + // ─── systemd user unit ─────────────────────────────────────────────────────── export function systemdUnitPath(home: string = homedir()): string { @@ -383,7 +477,7 @@ export async function runProxyPreflight(deps: PreflightDeps = {}): Promise Promise; + /** OS-level identity check for the process holding the proxy port. */ + verifyListener?: () => ListenerVerdict; startSystemd?: () => ProxyRunResult; startNohup?: () => Promise; waitMs?: (ms: number) => Promise; @@ -476,12 +572,15 @@ function defaultStartSystemd(): ProxyRunResult { } /** - * Poll liveness up to a bounded startup deadline. A start command returning 0 - * only means the job was ACCEPTED, not that the socket is bound — so we keep - * probing at `intervalMs` until either a probe succeeds or the deadline elapses. + * Poll for a TRUSTED-live proxy up to a bounded startup deadline. A start command + * returning 0 only means the job was ACCEPTED, not that the socket is bound — so + * we keep probing at `intervalMs` until either the deadline elapses or the port + * both responds AND passes the OS-level identity check. Liveness alone is not + * enough: a responder that fails identity (a squatter) must never be trusted. */ -async function waitForLive( +async function waitForTrusted( probe: () => Promise, + verifyListener: () => ListenerVerdict, waitMs: (ms: number) => Promise, intervalMs: number, deadlineMs: number, @@ -490,7 +589,7 @@ async function waitForLive( while (elapsed < deadlineMs) { await waitMs(intervalMs); elapsed += intervalMs; - if (await probe()) { + if ((await probe()) && verifyListener() === 'ok') { return true; } } @@ -501,16 +600,29 @@ async function waitForLive( * Ensure a proxy is listening. No-op when already live. Otherwise prefer the * systemd user unit, then fall back to a detached background process. * - * After a start command is accepted we poll liveness to a bounded startup - * deadline BEFORE considering the next method: `systemctl start` exit 0 means - * the job was accepted, not that the socket bound within one probe interval. If - * we fell back on the first miss we could launch a second proxy that then - * contends with the (slower) systemd one for :18765 — the very duplicate-proxy - * outcome this function exists to prevent. Liveness itself is the proxy-specific - * `/healthz` check (see {@link probeLiveness}). + * Every trust point is gated on OS-level listener identity, not just liveness: + * the proxy has no client authentication, so on a shared host a local process + * could squat :18765 and a 2xx `/healthz` alone would not prove it is our proxy + * (CWE-345, finding #2). We only trust a responder whose owning process is the + * current uid running the expected proxy binary; otherwise we fail closed. + * + * If a responder is already present but its identity does NOT verify, we return + * `untrusted` WITHOUT starting anything — the port is taken, so spawning would + * only create contention, and we must never route Claude traffic through an + * unverified listener. + * + * After a start command is accepted we poll to a bounded startup deadline before + * giving up: `systemctl start` exit 0 means the job was accepted, not that the + * socket bound within one probe interval. Critically, once systemd ACCEPTS the + * job we do NOT fall back to nohup even if it never becomes trusted-live in the + * deadline (finding #1): the accepted unit may bind late or be restarted by + * systemd, and a second proxy would then contend for :18765 — the very + * duplicate-proxy outcome this function exists to prevent. nohup is reachable + * only when systemd never accepted the job at all. */ export async function ensureProxyRunning(deps: EnsureProxyDeps = {}): Promise { const probe = deps.probe ?? (() => probeLiveness()); + const verifyListener = deps.verifyListener ?? (() => verifyListenerIdentity()); const startSystemd = deps.startSystemd ?? defaultStartSystemd; const startNohup = deps.startNohup ?? (() => startNohupProxy()); const waitMs = deps.waitMs ?? defaultWait; @@ -518,19 +630,25 @@ export async function ensureProxyRunning(deps: EnsureProxyDeps = {}): Promise