fix(dor): harden the control socket path and make the host prove itself - #433
fix(dor): harden the control socket path and make the host prove itself#433dormouse-bot wants to merge 3 commits into
Conversation
The control channel grants the whole surface API — `dor send` types into any pane, `dor read` returns its scrollback — and reached it over a PID-derived path in a shared namespace, with the client writing DORMOUSE_CONTROL_TOKEN as the first bytes on the wire to a peer that had proven nothing. - The server picks the path: a per-uid 0700 directory (created and re-checked like peer-link's peerDirIsSafe) with a random name on POSIX, an unguessable pipe name on Windows. - The server proves itself first. The token never goes on the wire; both halves are HMACs over the other side's nonce, compared in constant time. A bad hello is hung up on, not answered. - A lost bind stays fatal to the channel only. Both hosts drop the control variables from the environment they hand to spawned shells and restore them only once the socket is listening, so a squatter cannot get Dormouse to keep feeding it clients and tokens. Closes #431
Deploying mouseterm with
|
| Latest commit: |
eef58ba
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://25750eda.mouseterm.pages.dev |
| Branch Preview URL: | https://fix-issue-431.mouseterm.pages.dev |
dormouse-bot
left a comment
dormouse-bot
left a comment
There was a problem hiding this comment.
Reviewed as the PR's own author, so this is a COMMENT — GitHub won't take a self-approval. The handshake itself traces clean: the server speaks first, the client writes nothing until it has a challenge, the request is written only after a welcome verified over a nonce the client picked, and both settle paths report the same opaque failure. The env gating in both hosts is ordered correctly — the dorControl.ready.then(setEnv) reaction is registered before Promise.race derives its own promise from ready.catch(…), so setEnv always runs a microtask ahead of announceReady / controlSettled = true. Four things worth acting on.
The standalone dev harness still computes and logs a socket path nobody binds. standalone/scripts/dev-agent-browser.mjs is the sibling of the Rust code this PR deleted: it derives controlSocket from process.pid (dormouse-<pid>-browser-dor.sock, or the matching pipe name), passes it as DORMOUSE_CONTROL_SOCKET in the sidecar's spawn env, and prints it on a dor control socket: log line. main.js now deletes that variable at startup and picks its own path, so the value is dead — but the harness still prints it, and that harness is exactly what the debug-standalone-agent-browser skill drives when someone is chasing a dor problem. The real path already reaches the same console through the forwarded sidecar stderr ([dor-control] listening on …). I'll push the deletion — the same three-part removal the PR made in lib.rs (the path function, the .env(…) entry, the append_log line).
Nothing ever sweeps <tmpdir>/dormouse-dor-<uid>/. With PID-derived names a crash-left socket file was eventually reclaimed by the next process to draw that PID; with a fresh 8-byte name per launch, a SIGKILLed host leaves a 0-byte socket that nothing removes — and on macOS $TMPDIR is only swept after three days without access. Low-stakes, but it is the flip side of the same change, and it makes the pre-listen unlinkSync dead in production: with a name nothing has ever created, only the socketPath test seam can reach its non-ENOENT branch. Inline suggestion on the comment above it, which still describes crash recovery it can no longer perform.
ensureControlDir and peerDirIsSafe are now the same predicate in two places. ensureControlDir mirrors peerDirIsSafe() in vscode-ext/src/peer-link.ts down to the comment text ("Ours but loose — a permissive umask…", "keeps the test below exact rather than '0700 or better'"), differing only in sync-vs-async fs and the return type. The reference is one-way: dor-control-server.js names peer-link.ts, but peer-link.ts says nothing about the control channel, so a future correction to the hardening rule reaches one copy and silently not the other. Contrast the duplicated proof construction in control-client.ts — drift there would break the channel, and the new dor/test/control-client.test.mjs catches it by running the real client against the real server module. Nothing tests these two directory checks against each other. Either extract the predicate (pty-host.js already reaches across into standalone/sidecar/) or, cheaper, add the reverse pointer so the next edit finds both.
The third defence is the untested one. The path hardening and the handshake both got real coverage, but the "a lost bind must not keep handing out the token" fix — delete both variables from process.env at startup, restore only on ready, hold spawns until it settles — lives entirely in top-level module code in pty-host.js and main.js, and neither file has a test anywhere in the repo. That is the defence whose failure mode is silent (a token handed to a squatter) rather than loud, and the microtask ordering it leans on is subtle enough to be worth pinning. Not asking for it in this PR, but it is the gap I would name if this regressed later.
Everything else checked out: os is genuinely unused in pty-manager.ts after the removal; getDorRuntimeEnv's omission holds because spawn passes env: dorEnv verbatim and pty-host.js re-adds the pair; a VS Code window launched from inside another Dormouse cannot leak the outer DORMOUSE_CONTROL_SOCKET through the fork env, since the pty-host deletes it; and node scripts/spec-lint.mjs plus node --test standalone/sidecar/dor-control-server.test.js (10/10) pass locally.
The sidecar now picks and binds the control socket path itself, deleting DORMOUSE_CONTROL_SOCKET from its own environment at startup. The harness's PID-derived value was therefore dead, but it was still printed on a `dor control socket:` log line — the wrong path in the one console someone debugging `dor` reads. The real path arrives on the forwarded sidecar stderr as `[dor-control] listening on …`.
…check The socket name is freshly random on every launch, so the pre-`listen` `unlinkSync` can only find nothing in production — a crash leftover sits at a different random name and nothing reclaims it. The comment above it claimed crash recovery it can no longer perform; say instead what the branch is for (the `socketPath` test seam) and where leftovers actually go. `ensureControlDir` and `peerDirIsSafe` are the same hardening predicate in two languages, and the reference was one-way. Add the pointer back from peer-link.ts so a future correction to the rule finds both copies.
|
Actioned the review as author. The harness deletion had already gone in as 65d7073; eef58ba covers the other two. Pre-
Untested third defence — left as-is, as the review said. Worth being explicit about the shape of the gap for whoever picks it up: the token-withholding lives in top-level module code in
|
1 participant
Problem
Three weaknesses in the
dorcontrol channel, reported in #431. The channel grants the full surface API —dor sendtypes arbitrary keystrokes into any pane,dor readreturns its screen and scrollback,dor killdestroys it — so a local principal who gets betweendorand its host owns every terminal.The path was
dormouse-{pid}-dorinos.tmpdir()or the machine-wide Windows pipe namespace, both writable by other principals and both guessable.DorControlClientthen wroteDORMOUSE_CONTROL_TOKENas the first bytes on the wire, before the peer had proven anything — so whoever held the path harvested a bearer credential from the firstdorinvocation that connected. And on Windows a lostlistenwas logged and swallowed whilegetDorRuntimeEnvkept handing that token to every shell it spawned, so winning the pipe race got Dormouse to keep feeding the attacker clients and tokens.Solution
Follows the shape the issue suggested, and the pattern
vscode-ext/src/peer-link.tsalready uses for the peer channel.The server picks the path, and picks it unguessably. POSIX sockets live at
<tmpdir>/dormouse-dor-<uid>/<8 random bytes>.sock; the parent directory is created0700and re-checked on every use (a real directory, not a symlink, owned by this uid, at exactly0700— tightened if it is ours but loose, stood down otherwise), mirroringpeerDirIsSafe(). Windows gets\\.\pipe\dormouse-dor-<8 random bytes>— no directory to harden there, so unpredictability is what is left. 8 bytes rather than 16 because macOS capssun_pathnear 104 and itsos.tmpdir()spends ~50. Neither host computes a path any more, sodor_control_socket_path()is gone fromlib.rsand the socket const is gone frompty-manager.ts.The server proves itself first. The token never goes on the wire in either direction. The server opens with a challenge nonce; the client answers
HMAC-SHA256(token, "dor-control/client <nonce>")plus a nonce of its own; the server answersHMAC-SHA256(token, "dor-control/server <nonce>")before the client sends any request. A peer that fails its half is hung up on with no reply. Both sides compare in constant time, keeping the SHA-256 +timingSafeEqualconstruction the oldtokenMatchesused.A lost bind is fatal to the channel, never to the host.
pty-host.jsand the sidecar delete both control variables from their own environment at startup —pty-coremergesprocess.envinto every shell — and put them back only whenreadyresolves. When the bind is lost, the variables stay gone anddorreports the endpoint as unavailable rather than dialling a stranger. Both hosts hold their spawn path untilreadysettles (2s ceiling) so the first terminal cannot race the bind. The POSIX pre-listenunlinkSyncnow rejectsreadyinstead of throwing, so a socket file it cannot clear kills the channel rather than the sidecar and every PTY in it.docs/specs/dor-cli.mdgains a Control-channel security section covering all three, and the env-contract and host-plumbing sections are updated to match.Testing
standalone/sidecar/dor-control-server.test.js(10 tests, all new or rewritten for the handshake) and a newdor/test/control-client.test.mjsthat runs the realSocketControlClientagainst the real server module both hosts load. The ones that matter:socketPath: nullwith a rejectedready;readywithout throwing.Run locally:
node --test standalone/sidecar/*.test.js(106 pass),pnpm --filter dor test(104 pass),pnpm --filter dormouse test(typecheck + 103 pass),node scripts/spec-lint.mjs. The Rust change is a deletion of a function and its two uses; it could not be compiled here (no GTK dev libs on this runner) and is left to the Standalone job.Not included
The issue also asked to surface a dead control channel in the UI rather than only on stderr. The plumbing is in place — both hosts know the outcome at
ready— but there is no shared place to put a notice across the two hosts (the Baseboard carries the standalone update notice and has no VS Code counterpart), so that is a design call. Recorded as an item indocs/specs/dor-cli.md→## Future.Closes #431 — automated triage