diff --git a/TODO.md b/TODO.md index 6523d0b9..d0a8deb5 100644 --- a/TODO.md +++ b/TODO.md @@ -143,8 +143,8 @@ everything still open, in the order it should be tackled. Found by reading and probing, not by the suite — all 128 CLI tests passed before and after each was confirmed. Ordered by severity. -- [ ] **The agent can be left holding a parent seed forever, after reporting - failure.** `AgentDaemon.handle`'s `put` writes to `this.held` *before* +- [x] **(fixed 2026-08-17)** **The agent can be left holding a parent seed + forever, after reporting failure.** `AgentDaemon.handle`'s `put` writes to `this.held` *before* awaiting the audit append. A non-numeric `ttl` makes `expiresAt` NaN; the audit write then throws on the non-finite number, so the caller is told the unlock failed — but the seed is resident, `expire()`'s @@ -157,8 +157,8 @@ before and after each was confirmed. Ordered by severity. which is exactly why the daemon must not rely on it — `agent.ts:1-22` states the CLI is untrusted, and the browser extension will be the next thing speaking this protocol. -- [ ] **A `use`-scoped token holder can kill the agent and drop every held - seed.** `stdinSink` writes to `child.stdin` with no `error` listener, so a +- [x] **(fixed 2026-08-17)** **A `use`-scoped token holder can kill the agent + and drop every held seed.** `stdinSink` writes to `child.stdin` with no `error` listener, so a command that exits before draining stdin raises an unhandled EPIPE. Confirmed as an uncaught exception inside the agent process via a `use-sink` request naming `/bin/true`; `main.ts` installs no @@ -201,7 +201,7 @@ before and after each was confirmed. Ordered by severity. alter one can remove the other. The doc comment claims the head "pins the expected length" against exactly this attack. Either require the head once the log exists, or keep it somewhere the log's writer cannot reach. -- [ ] **Unvalidated token constraint shapes.** `token-issue` casts `kinds` and +- [x] **(fixed 2026-08-17)** **Unvalidated token constraint shapes.** `token-issue` casts `kinds` and `scopes` without checking they are arrays. `kinds: "totp"` makes `Array.includes` become `String.includes`, turning an exact kind match into a substring match — inert today only because no SeedPass kind is a diff --git a/js/packages/cli/src/agent.ts b/js/packages/cli/src/agent.ts index b85b0f93..f3d35416 100644 --- a/js/packages/cli/src/agent.ts +++ b/js/packages/cli/src/agent.ts @@ -52,6 +52,22 @@ export const TIMESTAMP_SKEW_SECONDS = 120; export const MAX_LINE_BYTES = 512 * 1024; export const MAX_CONNECTIONS = 64; +/** + * A positive integer taken from a wire message, or null if the value is not + * one. The CLI validates these too, but the daemon cannot rely on that: it + * treats every caller as untrusted (see the module header), and the next + * thing to speak this protocol will be a browser extension. + * + * Coercions that must be rejected here rather than stored: a non-numeric ttl + * makes `expiresAt` NaN, and `NaN <= now` is always false — the seed would + * then never expire. The same NaN in a token's `expires_at` makes a token + * that never expires. + */ +function positiveIntField(value: unknown, fallback: number): number | null { + const n = value === undefined || value === null ? fallback : Number(value); + return Number.isInteger(n) && n >= 1 ? n : null; +} + export type TokenScope = "read" | "use" | "reveal"; export interface TokenRecord { @@ -460,20 +476,35 @@ export class AgentDaemon { // profile must currently be unlocked in this agent. const fingerprint = String(msg["fingerprint"] ?? ""); if (!this.held.has(fingerprint)) return { ok: false, error: "profile not unlocked" }; - const scopes = (msg["scopes"] as TokenScope[] | undefined) ?? ["read"]; - const bad = scopes.filter((s) => !["read", "use", "reveal"].includes(s)); + const scopes = msg["scopes"] ?? ["read"]; + // Shape-check before .filter/.includes run against them: a bare string + // scope would throw a raw TypeError back to the caller, and a string + // `kinds` turns the exact kind match in tokenMaySee into a substring + // match (String.includes, not Array.includes). + if (!Array.isArray(scopes)) return { ok: false, error: "scopes must be an array" }; + const bad = (scopes as unknown[]).filter( + (s) => !["read", "use", "reveal"].includes(s as string), + ); if (bad.length) return { ok: false, error: `unknown scopes: ${bad.join(",")}` }; + const kinds = msg["kinds"]; + if (kinds !== undefined && kinds !== null && !Array.isArray(kinds)) { + return { ok: false, error: "kinds must be an array" }; + } + const ttl = positiveIntField(msg["ttl"], 300); + if (ttl === null) return { ok: false, error: "ttl must be a positive integer" }; + const uses = positiveIntField(msg["uses"], 1); + if (uses === null) return { ok: false, error: "uses must be a positive integer" }; const secret = randomBytes(24).toString("base64url"); const record: TokenRecord = { id: `tok-${randomBytes(6).toString("hex")}`, name: String(msg["name"] ?? "agent"), fingerprint, secret_hash: sha256Hex(utf8(secret)), - scopes, - kinds: (msg["kinds"] as string[] | undefined) ?? null, + scopes: scopes as TokenScope[], + kinds: (kinds as string[] | undefined) ?? null, label_regex: String(msg["label_regex"] ?? ".*"), - expires_at: Math.floor(Date.now() / 1000 + Number(msg["ttl"] ?? 300)), - uses_remaining: Number(msg["uses"] ?? 1), + expires_at: Math.floor(Date.now() / 1000 + ttl), + uses_remaining: uses, revoked: false, ...(Array.isArray(msg["exec_allowlist"]) && (msg["exec_allowlist"] as string[]).length ? { exec_allowlist: msg["exec_allowlist"] as string[] } @@ -605,10 +636,21 @@ export class AgentDaemon { const fingerprint = String(msg["fingerprint"] ?? ""); const mnemonic = String(msg["mnemonic"] ?? ""); if (!fingerprint || !mnemonic) return { ok: false, error: "missing fields" }; - const ttl = Number(msg["ttl"] ?? this.defaultTtl); + const ttl = positiveIntField(msg["ttl"], this.defaultTtl); + if (ttl === null) return { ok: false, error: "ttl must be a positive integer" }; const expiresAt = Math.floor(Date.now() / 1000 + ttl); + // The audit key is derived from the held seed, so the seed must go in + // before the record is written — but an audit failure must then undo + // it. Otherwise a `put` that reports ok:false can leave the seed + // resident and unexpiring, with nothing on record that it was ever + // unlocked. `ok:false` has to mean the vault is genuinely still locked. this.held.set(fingerprint, { mnemonic, expiresAt }); - await this.auditLog(fingerprint, "vault_unlocked", { ttl }); + try { + await this.auditLog(fingerprint, "vault_unlocked", { ttl }); + } catch (e) { + this.forget(fingerprint); + throw e; + } return { ok: true, expires_at: expiresAt }; } case "owner-mnemonic": { diff --git a/js/packages/cli/src/sinks.ts b/js/packages/cli/src/sinks.ts index b8a44be8..61d52838 100644 --- a/js/packages/cli/src/sinks.ts +++ b/js/packages/cli/src/sinks.ts @@ -139,10 +139,21 @@ export async function stdinSink( stdio: ["pipe", "inherit", "inherit"], env: sinkEnv(), }); - child.on("error", reject); - child.stdin.write(secret); - child.stdin.end(); + let spawnFailed = false; + child.on("error", (e) => { + spawnFailed = true; + reject(e); + }); + // A command that exits before reading stdin makes this write fail with + // EPIPE. With no listener that becomes an *unhandled* error event, which + // crashes the whole process — and the session agent runs sinks in-process, + // so a use-scoped token holder could take the agent down (dropping every + // held seed) just by naming a command that exits early. Swallow the stdin + // error: the child's exit code, delivered on `close`, is the real result. + child.stdin.on("error", () => {}); + child.stdin.end(secret); child.on("close", (code) => { + if (spawnFailed) return; resolve({ sink: "stdin", detail: `piped to ${command}`, exitCode: code ?? -1 }); }); }); diff --git a/js/packages/cli/test/agentSecurity.test.ts b/js/packages/cli/test/agentSecurity.test.ts index 723d65b0..da27b139 100644 --- a/js/packages/cli/test/agentSecurity.test.ts +++ b/js/packages/cli/test/agentSecurity.test.ts @@ -385,3 +385,79 @@ describe("locking clears derived state", () => { expect(String(after["error"])).toContain("unknown token"); }); }); + +describe("malformed daemon input cannot corrupt held state", () => { + // Reproduces the 2026-08-17 self-review findings, spoken to the socket + // directly. The CLI would reject these, which is exactly why the daemon + // must too: it treats every caller as untrusted. + const SCRATCH_FP = "BBBBBBBBBBBBBBBB"; + let cap: string; + beforeAll(async () => { + cap = (await readFile(daemon.capabilityPath, "utf8")).trim(); + }); + + it("a put with a non-numeric ttl is refused, and holds nothing", async () => { + const put = await rawRequest({ + op: "put", cap, fingerprint: SCRATCH_FP, mnemonic: MNEMONIC, ttl: "abc", + }); + expect(put["ok"]).toBe(false); + expect(String(put["error"])).toContain("ttl"); + // A reported failure must be a real one: NaN once left the seed resident + // and unexpiring with no audit record. The seed must not be held at all. + const owner = await rawRequest({ op: "owner-mnemonic", cap, fingerprint: SCRATCH_FP }); + expect(owner["ok"]).toBe(false); + expect(JSON.stringify(owner)).not.toContain("abandon"); + }); + + it("token-issue refuses a bare-string scope and a non-numeric ttl without crashing", async () => { + const strScope = await rawRequest({ + op: "token-issue", cap, fingerprint: FINGERPRINT, scopes: "reveal", ttl: 600, uses: 1, + }); + expect(strScope["ok"]).toBe(false); + expect(strScope["token"]).toBeUndefined(); + + const badTtl = await rawRequest({ + op: "token-issue", cap, fingerprint: FINGERPRINT, scopes: ["read"], ttl: "soon", uses: 1, + }); + expect(badTtl["ok"]).toBe(false); + expect(String(badTtl["error"])).toContain("ttl"); + + // The daemon is still answering — a raw TypeError must not have escaped. + expect((await rawRequest({ op: "ping" }))["pong"]).toBe(true); + }); +}); + +describe("a use-scoped token cannot crash the agent", () => { + it("a stdin sink to a command that exits before reading does not kill the daemon", async () => { + // A large secret so the payload cannot fit the pipe buffer: the child + // exits before draining it, which raised an unhandled EPIPE and took the + // whole agent process down. + const bigValue = "y".repeat(200_000); + const added = JSON.parse( + (await run( + { SEEDPASS_MNEMONIC: MNEMONIC }, + "entry", "add", "key-value", "bulk", "k", bigValue, + )).stdout, + ); + const id = String(added.id); + + // A fresh token, not the shared one: earlier tests may have spent its uses. + const useToken = JSON.parse( + (await run( + {}, "agent", "token-issue", "--scope", "read", "use", "--uses", "5", "--ttl", "600", + )).stdout, + ).token; + + const r = await rawRequest({ + op: "use-sink", fingerprint: FINGERPRINT, id, token: useToken, + sink: "stdin", command: ["/bin/true"], + }); + expect(r["ok"]).toBe(true); + + // The load-bearing assertions: the agent is still alive and still holding + // the seed after a delivery that used to crash it. + expect((await rawRequest({ op: "ping" }))["pong"]).toBe(true); + const status = JSON.parse((await run({}, "agent", "status")).stdout); + expect(status.map((p: { fingerprint: string }) => p.fingerprint)).toContain(FINGERPRINT); + }); +});