diff --git a/docs/TODO.md b/docs/TODO.md index ce26d19..c6dfb29 100644 --- a/docs/TODO.md +++ b/docs/TODO.md @@ -28,9 +28,18 @@ signed events against the request (signature, identity, field equality); `Nip46Client.close()` wipes the client and conversation keys; passkey labels are distinguishable; postcss ≥8.5.25; Actions pinned to commit SHAs. -Deferred from the audit, non-security interop items: NIP-46 protocol-level -logout on teardown (no stable method name across deployed signers) and the -recommended `switch_relays` request. +Second-pass response (same day): BL-21 — bunker relays capped at 8 and +deduplicated; relay frames over 256K chars dropped before JSON.parse; all +cheap checks (id dedup, capacity) run before Schnorr verification, plus a +per-query verification budget. BL-22 — superseded/abandoned nostrconnect +attempts are cancelled via AbortSignal and their ephemeral keys wiped +(ownership transfers, not dangles, on success); per-candidate conversation +keys wiped in a finally. BL-23 — `security:ci` now runs the custom Semgrep +rules (`security:semgrep`; requires a local semgrep install, same pin as +CI). BL-24 — the courtesy protocol `logout` (current NIP-46 defines it, +explicitly not a security boundary) is now sent on teardown; this corrects +the earlier "no stable method name" note. Still deferred, interop only: +`switch_relays`. Owner action items: diff --git a/package.json b/package.json index 860de60..dd18c65 100644 --- a/package.json +++ b/package.json @@ -17,7 +17,8 @@ "test:watch": "vitest", "typecheck": "npm run typecheck -w @bitlogin/core && npm run typecheck -w @bitlogin/widget", "security:audit": "npm audit --audit-level=high && npm audit signatures", - "security:ci": "npm run build:demo && npm run typecheck && npm test && npm run security:audit" + "security:semgrep": "semgrep scan --config .semgrep.yml --error --timeout 0 packages", + "security:ci": "npm run build:demo && npm run typecheck && npm test && npm run security:audit && npm run security:semgrep" }, "devDependencies": { "typescript": "^5.7.3", diff --git a/packages/core/src/nostr/nip46.test.ts b/packages/core/src/nostr/nip46.test.ts index a75b66f..25e41e8 100644 --- a/packages/core/src/nostr/nip46.test.ts +++ b/packages/core/src/nostr/nip46.test.ts @@ -35,6 +35,13 @@ describe("parseBunkerUri", () => { expect(parsed.secret).toBeUndefined(); }); + it("caps and deduplicates relays from a crafted URI (audit BL-21)", () => { + const relays = Array.from({ length: 5000 }, (_, i) => `relay=wss://r${i % 20}.example`).join("&"); + const parsed = parseBunkerUri(`bunker://${SIGNER_HEX}?${relays}`); + expect(parsed.relayUrls.length).toBe(8); + expect(new Set(parsed.relayUrls).size).toBe(8); + }); + it("rejects wrong schemes, bad pubkeys, and relay-less URIs", () => { expect(() => parseBunkerUri(`nostrconnect://${SIGNER_HEX}?relay=wss://x.example`)).toThrow( /bunker:\/\// @@ -406,4 +413,44 @@ describe("listenForNostrconnect", () => { }) ).rejects.toThrow(/No signer connected/); }); + + it("an AbortSignal cancels the listen promptly (audit BL-22)", async () => { + const controller = new AbortController(); + const pending = listenForNostrconnect({ + clientSecretKey: generatePrivateKey(), + relayUrls: [relay.url], + secret: "s", + timeoutMs: 60_000, + signal: controller.signal + }); + setTimeout(() => controller.abort(), 100); + const start = Date.now(); + await expect(pending).rejects.toThrow(/cancelled/); + expect(Date.now() - start).toBeLessThan(5_000); + }); +}); + +describe("courtesy logout", () => { + it("sends the protocol logout to the signer on teardown (audit BL-24)", async () => { + const relay = await MockRelay.start(); + const bunker = new FakeBunker(relay.url); + await bunker.start(); + const client = new Nip46Client({ + clientSecretKey: generatePrivateKey(), + pointer: { signerPubkey: bunker.signerPubkey, relayUrls: [relay.url] }, + requestTimeoutMs: 5000 + }); + await client.connect(); + await client.logout(); + client.close(); + // FakeBunker records every decrypted request it receives; its processing + // is asynchronous relative to the publish OK, so poll briefly. + const deadline = Date.now() + 2000; + while (!bunker.seen.some((r) => r.method === "logout") && Date.now() < deadline) { + await new Promise((resolve) => setTimeout(resolve, 25)); + } + expect(bunker.seen.some((r) => r.method === "logout")).toBe(true); + bunker.close(); + await relay.close(); + }); }); diff --git a/packages/core/src/nostr/nip46.ts b/packages/core/src/nostr/nip46.ts index d43a7b8..053f281 100644 --- a/packages/core/src/nostr/nip46.ts +++ b/packages/core/src/nostr/nip46.ts @@ -33,6 +33,9 @@ import { wipe } from "../crypto/memory.js"; const HEX64 = /^[0-9a-f]{64}$/u; +/** Ceiling on relays taken from a bunker:// URI (BL-21). */ +const MAX_BUNKER_RELAYS = 8; + export interface BunkerPointer { signerPubkey: string; relayUrls: string[]; @@ -93,10 +96,17 @@ export function parseBunkerUri(uri: string): BunkerPointer { if (!HEX64.test(signerPubkey)) { throw new BunkerUriParseError("The bunker URI does not name a valid signer public key."); } - const relayUrls = url.searchParams - .getAll("relay") - .map((value) => value.trim()) - .filter(Boolean); + // Deduplicate and CAP: the URI is untrusted input, and each relay becomes + // a live WebSocket -- a crafted 5,000-relay URI must not become 5,000 + // sockets (BL-21). Nobody legitimate needs more than a handful. + const relayUrls = [ + ...new Set( + url.searchParams + .getAll("relay") + .map((value) => value.trim()) + .filter(Boolean) + ) + ].slice(0, MAX_BUNKER_RELAYS); if (relayUrls.length === 0) { throw new BunkerUriParseError( "The bunker URI names no relays -- without at least one relay= parameter there is no way to reach the signer." @@ -348,6 +358,38 @@ export class Nip46Client { return this.rpc("nip04_decrypt", [peerPublicKey, payload]); } + /** + * Courtesy logout per current NIP-46 -- explicitly not a security boundary + * (the signer revokes authorization on its own side); best-effort, bounded, + * and never allowed to block or fail teardown. Call before close(). + */ + async logout(timeoutMs = 2000): Promise { + if (this.closed) return; + try { + const id = bytesToHex(randomBytes(8)); + const content = nip44Encrypt( + this.conversationKey, + JSON.stringify({ id, method: "logout", params: [] }) + ); + const event = signNostrEvent( + { + pubkey: this.clientPubkey, + created_at: Math.floor(Date.now() / 1000), + kind: KIND_NOSTR_CONNECT, + tags: [["p", this.pointer.signerPubkey]], + content + }, + this.clientSecretKey + ); + await Promise.race([ + Promise.allSettled(this.conns.map((conn) => conn.publish(event))), + new Promise((resolve) => setTimeout(resolve, timeoutMs)) + ]); + } catch { + // Courtesy only. + } + } + close(): void { this.closed = true; for (const sub of this.subs) sub.close(); @@ -378,6 +420,10 @@ export async function listenForNostrconnect(options: { relayUrls: string[]; secret: string; timeoutMs?: number; + /** Cancels the listen (superseded attempt, user backed out, disconnect): + * connections close and the promise rejects promptly instead of running + * out its full window (BL-22). */ + signal?: AbortSignal; }): Promise<{ signerPubkey: string }> { const clientPubkey = getPublicKeyHex(options.clientSecretKey); const timeoutMs = options.timeoutMs ?? DEFAULT_REQUEST_TIMEOUT_MS; @@ -390,10 +436,12 @@ export async function listenForNostrconnect(options: { return new Promise<{ signerPubkey: string }>((resolve, reject) => { let settled = false; + const onAbort = () => settle(new Error("The signer-connection attempt was cancelled.")); const settle = (outcome: { signerPubkey: string } | Error) => { if (settled) return; settled = true; clearTimeout(timer); + options.signal?.removeEventListener("abort", onAbort); closeAll(); if (outcome instanceof Error) reject(outcome); else resolve(outcome); @@ -407,17 +455,26 @@ export async function listenForNostrconnect(options: { ), timeoutMs ); + if (options.signal?.aborted) { + onAbort(); + return; + } + options.signal?.addEventListener("abort", onAbort, { once: true }); const filter = { kinds: [KIND_NOSTR_CONNECT], "#p": [clientPubkey] }; const onEvent = (event: NostrEvent) => { // The sender is unknown until the secret verifies, so the conversation - // key is derived per candidate event. + // key is derived per candidate event -- and wiped per candidate, not + // left to garbage collection (§11.10, BL-22). let payload: { result?: unknown }; + let conversationKey: Uint8Array | null = null; try { - const conversationKey = getConversationKey(options.clientSecretKey, event.pubkey); + conversationKey = getConversationKey(options.clientSecretKey, event.pubkey); payload = JSON.parse(nip44Decrypt(conversationKey, event.content)) as typeof payload; } catch { return; + } finally { + if (conversationKey) wipe(conversationKey); } if (payload.result === options.secret) settle({ signerPubkey: event.pubkey }); }; diff --git a/packages/core/src/nostr/relay.ts b/packages/core/src/nostr/relay.ts index 83df45d..d41e77f 100644 --- a/packages/core/src/nostr/relay.ts +++ b/packages/core/src/nostr/relay.ts @@ -56,6 +56,19 @@ export interface RelayConnectionOptions { * are single-digit result sets). */ const MAX_BUFFERED_EVENTS = 1000; +/** Frame-size ceiling (UTF-16 code units) applied before JSON.parse. The + * largest legitimate BitLogin events (padded capsules, recovery payloads) + * sit far below this. */ +const MAX_MESSAGE_CHARS = 262_144; + +/** How many signature verifications one buffered query will pay for before + * writing the relay off as hostile. Scales with the requested limit; the + * floor covers replaceable-event churn on tiny queries. */ +function verifyBudgetFor(filter: NostrFilter): number { + const cap = Math.min(filter.limit ?? MAX_BUFFERED_EVENTS, MAX_BUFFERED_EVENTS); + return Math.max(50, cap * 3); +} + export class RelayConnection { readonly url: string; private ws: WebSocketLike | null = null; @@ -66,6 +79,9 @@ export class RelayConnection { events: NostrEvent[]; /** Ids already buffered, for replay dedup (buffered queries only). */ seenIds: Set; + /** Remaining signature verifications this subscription will pay for; + * Infinity for live subscriptions (BL-21). */ + verifyBudget: number; filter: NostrFilter; onEose: () => void; /** Present on live subscriptions (subscribeLive): each verified, @@ -138,6 +154,11 @@ export class RelayConnection { } private handleMessage(raw: string): void { + // Size ceiling BEFORE parsing: an adversarial relay must not buy a large + // JSON.parse (let alone verification) with an oversized frame (BL-21). + // Generously above the largest legitimate BitLogin event; relays + // themselves commonly cap frames well below this. + if (raw.length > MAX_MESSAGE_CHARS) return; let msg: unknown; try { msg = JSON.parse(raw); @@ -151,27 +172,28 @@ export class RelayConnection { const [subId, event] = rest; if (typeof subId !== "string") return; const sub = this.subs.get(subId); + if (!sub) return; // Relays are untrusted and are not entitled to define the result set. // Verify both the event and the full requested NIP-01 filter centrally; // callers may still re-check security-sensitive identity constraints. - if ( - sub && - verifyNostrEvent(event) && - matchesNostrFilter(event, sub.filter) - ) { - if (sub.onEvent) { - sub.onEvent(event); - } else { - // Relays are untrusted: enforce the requested limit and a global - // ceiling LOCALLY, and drop replayed duplicates, so an adversarial - // relay cannot amplify memory/CPU by spraying copies before EOSE. - if (sub.seenIds.has(event.id)) return; - const cap = Math.min(sub.filter.limit ?? MAX_BUFFERED_EVENTS, MAX_BUFFERED_EVENTS); - if (sub.events.length >= cap) return; - sub.seenIds.add(event.id); - sub.events.push(event); - } + if (sub.onEvent) { + if (verifyNostrEvent(event) && matchesNostrFilter(event, sub.filter)) sub.onEvent(event); + return; } + // Buffered query: every CHEAP check runs before Schnorr verification, + // so replayed or over-limit spam cannot buy signature checks (BL-21). + // Replayed copies of an accepted id cost one Set lookup; events past + // capacity cost nothing; and a hostile relay spraying distinct invalid + // events runs out of verification budget rather than our CPU. + const claimedId = (event as { id?: unknown } | null | undefined)?.id; + if (typeof claimedId !== "string" || sub.seenIds.has(claimedId)) return; + const cap = Math.min(sub.filter.limit ?? MAX_BUFFERED_EVENTS, MAX_BUFFERED_EVENTS); + if (sub.events.length >= cap) return; + if (sub.verifyBudget <= 0) return; + sub.verifyBudget--; + if (!verifyNostrEvent(event) || !matchesNostrFilter(event, sub.filter)) return; + sub.seenIds.add(event.id); + sub.events.push(event); return; } if (type === "EOSE") { @@ -254,7 +276,7 @@ export class RelayConnection { ): Promise<{ close: () => void }> { await this.connect(); const subId = bytesToHex(randomBytes(8)); - this.subs.set(subId, { events: [], seenIds: new Set(), filter, onEose: () => {}, onEvent }); + this.subs.set(subId, { events: [], seenIds: new Set(), verifyBudget: Number.POSITIVE_INFINITY, filter, onEose: () => {}, onEvent }); this.send(["REQ", subId, filter]); return { close: () => { @@ -311,7 +333,7 @@ export class RelayConnection { ), timeoutMs, ); - this.subs.set(subId, { events, seenIds: new Set(), filter, onEose: () => settle() }); + this.subs.set(subId, { events, seenIds: new Set(), verifyBudget: verifyBudgetFor(filter), filter, onEose: () => settle() }); this.send(["REQ", subId, filter]); }); } diff --git a/packages/core/src/nostr/relayHardening.test.ts b/packages/core/src/nostr/relayHardening.test.ts index 170f543..6b4031d 100644 --- a/packages/core/src/nostr/relayHardening.test.ts +++ b/packages/core/src/nostr/relayHardening.test.ts @@ -65,6 +65,57 @@ describe("relay hardening", () => { expect(new Set(events.map((e) => e.id)).size).toBe(2); }); + it("BL-21: oversized frames are dropped before parsing", async () => { + const key = generatePrivateKey(); + const real = signedNote(key, "small"); + // A ~600K-character frame (valid JSON, would even match the filter) must + // be discarded before JSON.parse; the legitimate event still arrives. + const giant = { ...real, content: "x".repeat(600_000) }; + relay.unsolicitedQueryEvents = [giant, real]; + const events = await conn.queryOnce({ kinds: [1], authors: [getPublicKeyHex(key)] }, 3000); + expect(events).toHaveLength(1); + expect(events[0]!.content).toBe("small"); + }); + + it("BL-21: replayed copies cost one verification, not one each", async () => { + const key = generatePrivateKey(); + const note = signedNote(key, "replayed"); + relay.unsolicitedQueryEvents = Array.from({ length: 2000 }, () => note); + const start = Date.now(); + const events = await conn.queryOnce( + { kinds: [1], authors: [getPublicKeyHex(key)], limit: 1 }, + 10_000 + ); + const elapsed = Date.now() - start; + expect(events).toHaveLength(1); + // 2,000 Schnorr verifications would take seconds; one verification plus + // 1,999 Set lookups completes far inside this generous bound. + expect(elapsed).toBeLessThan(2_500); + }); + + it("BL-21: distinct invalid events exhaust the verification budget, not the CPU", async () => { + const key = generatePrivateKey(); + const real = signedNote(key, "real"); + // 500 distinct, filter-matching events with forged signatures ahead of + // the real one. The budget (3x limit, floor 50) stops verification long + // before 500; the query still settles cleanly at EOSE. + relay.unsolicitedQueryEvents = [ + ...Array.from({ length: 500 }, (_, i) => ({ + ...signedNote(key, `forged-${i}`, 1_700_000_100 + i), + sig: "ab".repeat(64) + })), + real + ]; + const events = await conn.queryOnce( + { kinds: [1], authors: [getPublicKeyHex(key)], limit: 1 }, + 10_000 + ); + // The budget is exhausted by forgeries before the real event arrives -- + // the query returns empty rather than burning unbounded CPU. This is the + // documented trade: a relay that sprays forgeries is a hostile relay. + expect(events.length).toBeLessThanOrEqual(1); + }); + it("BL-19: distinct events beyond the requested limit are not buffered", async () => { const key = generatePrivateKey(); relay.unsolicitedQueryEvents = Array.from({ length: 10 }, (_, i) => diff --git a/packages/widget/src/worker/cryptoWorker.ts b/packages/widget/src/worker/cryptoWorker.ts index c3f19a0..f41fb1a 100644 --- a/packages/widget/src/worker/cryptoWorker.ts +++ b/packages/widget/src/worker/cryptoWorker.ts @@ -250,7 +250,12 @@ async function findConnection(connectionId: string): Promise<{ // no vault, and nothing persisted. The ephemeral client key never leaves this // worker (§LM5.4); close() ends everything. let nip46: { client: Nip46Client; userPubkey: string } | null = null; -let nostrconnectPending: { clientSecretKey: Uint8Array; relayUrls: string[]; secret: string } | null = null; +let nostrconnectPending: { + clientSecretKey: Uint8Array; + relayUrls: string[]; + secret: string; + abort: AbortController; +} | null = null; function postAuthUrlNotification(url: string): void { (self as unknown as Worker).postMessage({ notify: "nip46-auth-url", url }); @@ -261,8 +266,29 @@ function requireNip46(): { client: Nip46Client; userPubkey: string } { return nip46; } +/** Ends a superseded or abandoned nostrconnect attempt for real (BL-22): + * the active listener is cancelled and the pending ephemeral key wiped -- + * never merely dereferenced. NOT used when the key's ownership transfers + * into an adopted Nip46Client. */ +function discardNostrconnectPending(): void { + if (!nostrconnectPending) return; + nostrconnectPending.abort.abort(); + wipe(nostrconnectPending.clientSecretKey); + nostrconnectPending = null; +} + +/** Retires a previous session with the courtesy protocol logout (current + * NIP-46 defines it as exactly that -- not a security boundary), then + * closes it, which wipes its keys. */ +function retireNip46Client(client: Nip46Client): void { + void client + .logout() + .catch(() => {}) + .finally(() => client.close()); +} + function adoptNip46Client(client: Nip46Client, userPubkey: string): void { - nip46?.client.close(); + if (nip46) retireNip46Client(nip46.client); nip46 = { client, userPubkey }; } @@ -280,6 +306,8 @@ async function handle(action: string, payload: unknown): Promise { await client.connect(); const userPubkey = await client.getUserPublicKey(); adoptNip46Client(client, userPubkey); + // The paste leg won; a QR listener still waiting is superseded. + discardNostrconnectPending(); return { userPubkey }; } catch (err) { client.close(); @@ -294,11 +322,14 @@ async function handle(action: string, payload: unknown): Promise { // harder for a phone camera to lock onto. NIP-46 needs one shared // relay; the second is the redundancy. const relayUrls = vaultRelayUrls.slice(0, 2); + // A fresh attempt supersedes any prior one: cancel its listener and + // wipe its key rather than orphaning them (BL-22). + discardNostrconnectPending(); const clientSecretKey = generatePrivateKey(); // The secret is the proof-of-scan: whoever echoes it back becomes this // session's signer, so it gets real entropy, not a friendly string. const secret = bytesToHex(randomBytes(16)); - nostrconnectPending = { clientSecretKey, relayUrls, secret }; + nostrconnectPending = { clientSecretKey, relayUrls, secret, abort: new AbortController() }; const uri = buildNostrconnectUri({ clientPubkey: getPublicKeyHex(clientSecretKey), relayUrls, @@ -316,9 +347,12 @@ async function handle(action: string, payload: unknown): Promise { clientSecretKey: pending.clientSecretKey, relayUrls: pending.relayUrls, secret: pending.secret, - timeoutMs: 180_000 + timeoutMs: 180_000, + signal: pending.abort.signal }); if (nostrconnectPending !== pending) throw new Error("This signer-connection attempt was superseded."); + // Ownership of the ephemeral key transfers into the client below -- + // dereference only; discardNostrconnectPending would wipe it. nostrconnectPending = null; const client = new Nip46Client({ clientSecretKey: pending.clientSecretKey, @@ -368,9 +402,10 @@ async function handle(action: string, payload: unknown): Promise { } case "nip46Disconnect": { - nip46?.client.close(); + const active = nip46; nip46 = null; - nostrconnectPending = null; + discardNostrconnectPending(); + if (active) retireNip46Client(active.client); return {}; }