diff --git a/.github/workflows/security.yml b/.github/workflows/security.yml index 96b678c..41f7a7b 100644 --- a/.github/workflows/security.yml +++ b/.github/workflows/security.yml @@ -22,7 +22,9 @@ jobs: node-version: 22 cache: npm - run: npm ci --ignore-scripts - - name: Run build, typecheck, tests, dependency audit, and registry signature verification + - name: Install pinned Semgrep (security:ci runs the custom rules) + run: pipx install semgrep==1.171.0 + - name: Run build, typecheck, tests, dependency audit, and static-analysis gate run: npm run security:ci - name: Generate checksums and CycloneDX SBOM shell: bash diff --git a/docs/TODO.md b/docs/TODO.md index c6dfb29..1b2092c 100644 --- a/docs/TODO.md +++ b/docs/TODO.md @@ -41,6 +41,22 @@ 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`. +Third-pass response (same day): BL-25 — live subscriptions now carry a +bounded already-seen id cache (2048, FIFO eviction; a replayed retained +event costs one verification and one delivery, ever) and a fixed-window +verification budget (100 per 10s; distinct-event spray is quarantined for +the window). BL-26 — teardown order inverted: the courtesy logout event is +pre-signed, then close() IMMEDIATELY rejects every in-flight request and +wipes keys, then the pre-signed event is delivered asynchronously and +bounded — nothing can resolve after logout begins (regression-tested). +BL-27 — a nostrconnect listen failing naturally (timeout, unreachable +relays) now discards the pending ephemeral key in a guarded catch instead +of orphaning it. BL-28 — the hosted verify job installs the pinned Semgrep +before running security:ci (it was red at the previous HEAD, skipping SBOM +and checksums). Informational — bunker URIs are bounded: 4096 chars total, +256-char secret, 512-char relay URLs (oversized relays filtered, oversized +URI/secret rejected). + Owner action items: 1. **Live-fire the NIP-46 QR with Amber** on bitlogin.network (flow was diff --git a/packages/core/src/nostr/nip46.test.ts b/packages/core/src/nostr/nip46.test.ts index 25e41e8..108b7b1 100644 --- a/packages/core/src/nostr/nip46.test.ts +++ b/packages/core/src/nostr/nip46.test.ts @@ -36,12 +36,26 @@ describe("parseBunkerUri", () => { }); 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 relays = Array.from({ length: 120 }, (_, 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("bounds the URI, secret, and per-relay lengths (audit round 3)", () => { + expect(() => parseBunkerUri(`bunker://${SIGNER_HEX}?relay=wss://r.example&x=${"a".repeat(5000)}`)).toThrow( + /unreasonably long/ + ); + expect(() => + parseBunkerUri(`bunker://${SIGNER_HEX}?relay=wss://r.example&secret=${"s".repeat(300)}`) + ).toThrow(/secret is unreasonably long/); + // An oversized relay URL is filtered out rather than opening a socket. + const parsed = parseBunkerUri( + `bunker://${SIGNER_HEX}?relay=wss://${"r".repeat(600)}.example&relay=wss://ok.example` + ); + expect(parsed.relayUrls).toEqual(["wss://ok.example"]); + }); + it("rejects wrong schemes, bad pubkeys, and relay-less URIs", () => { expect(() => parseBunkerUri(`nostrconnect://${SIGNER_HEX}?relay=wss://x.example`)).toThrow( /bunker:\/\// @@ -323,6 +337,22 @@ describe("Nip46Client against a fake bunker", () => { ).rejects.toThrow(/different event/); }); + it("close() rejects in-flight requests immediately -- nothing resolves after logout begins (audit BL-26)", async () => { + bunker = new FakeBunker(relay.url); + bunker.handle = () => []; // a signer that never answers + await bunker.start(); + const c = makeClient(undefined, 60_000); + const pending = c.getUserPublicKey(); + pending.catch(() => {}); // observed below; avoid unhandled-rejection noise + await new Promise((resolve) => setTimeout(resolve, 100)); // let the request go out + const farewell = c.buildLogoutEvent(); + const start = Date.now(); + c.close(); + await expect(pending).rejects.toThrow(/connection was closed/); + expect(Date.now() - start).toBeLessThan(1_000); + expect(farewell).not.toBeNull(); + }); + it("times out with a typed error when the signer never answers", async () => { bunker = new FakeBunker(relay.url); bunker.handle = () => []; // listens, never replies @@ -441,8 +471,11 @@ describe("courtesy logout", () => { requestTimeoutMs: 5000 }); await client.connect(); - await client.logout(); + // Local revocation first (BL-26): pre-sign, close immediately, deliver after. + const farewell = client.buildLogoutEvent(); client.close(); + expect(farewell).not.toBeNull(); + await client.publishCourtesy(farewell!); // FakeBunker records every decrypted request it receives; its processing // is asynchronous relative to the publish OK, so poll briefly. const deadline = Date.now() + 2000; diff --git a/packages/core/src/nostr/nip46.ts b/packages/core/src/nostr/nip46.ts index 053f281..c4e7718 100644 --- a/packages/core/src/nostr/nip46.ts +++ b/packages/core/src/nostr/nip46.ts @@ -36,6 +36,13 @@ const HEX64 = /^[0-9a-f]{64}$/u; /** Ceiling on relays taken from a bunker:// URI (BL-21). */ const MAX_BUNKER_RELAYS = 8; +/** Input bounds on the bunker URI as a whole and its parts. Legitimate URIs + * are a few hundred characters; these stop memory amplification from + * crafted multi-megabyte inputs before any further processing. */ +const MAX_BUNKER_URI_CHARS = 4096; +const MAX_BUNKER_SECRET_CHARS = 256; +const MAX_BUNKER_RELAY_URL_CHARS = 512; + export interface BunkerPointer { signerPubkey: string; relayUrls: string[]; @@ -81,6 +88,9 @@ export function sanitizeAuthUrl(candidate: string): string | null { } export function parseBunkerUri(uri: string): BunkerPointer { + if (uri.length > MAX_BUNKER_URI_CHARS) { + throw new BunkerUriParseError("That bunker URI is unreasonably long."); + } let url: URL; try { url = new URL(uri.trim()); @@ -104,7 +114,7 @@ export function parseBunkerUri(uri: string): BunkerPointer { url.searchParams .getAll("relay") .map((value) => value.trim()) - .filter(Boolean) + .filter((value) => value.length > 0 && value.length <= MAX_BUNKER_RELAY_URL_CHARS) ) ].slice(0, MAX_BUNKER_RELAYS); if (relayUrls.length === 0) { @@ -113,6 +123,9 @@ export function parseBunkerUri(uri: string): BunkerPointer { ); } const secret = url.searchParams.get("secret") ?? undefined; + if (secret && secret.length > MAX_BUNKER_SECRET_CHARS) { + throw new BunkerUriParseError("The bunker URI's secret is unreasonably long."); + } return { signerPubkey, relayUrls, secret: secret || undefined }; } @@ -359,19 +372,21 @@ export class Nip46Client { } /** - * 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(). + * Pre-signs the courtesy protocol logout event (current NIP-46 defines + * logout as exactly that -- a courtesy, never a security boundary). Must + * run BEFORE close(), while the keys still exist; the caller then closes + * IMMEDIATELY (local revocation must not wait on the network, BL-26) and + * delivers the pre-signed event afterward via publishCourtesy(). */ - async logout(timeoutMs = 2000): Promise { - if (this.closed) return; + buildLogoutEvent(): NostrEvent | null { + if (this.closed) return null; try { const id = bytesToHex(randomBytes(8)); const content = nip44Encrypt( this.conversationKey, JSON.stringify({ id, method: "logout", params: [] }) ); - const event = signNostrEvent( + return signNostrEvent( { pubkey: this.clientPubkey, created_at: Math.floor(Date.now() / 1000), @@ -381,12 +396,26 @@ export class Nip46Client { }, this.clientSecretKey ); + } catch { + return null; + } + } + + /** + * Best-effort, bounded delivery of a pre-signed event. Safe (and intended) + * to run AFTER close(): it needs no key material, and RelayConnection + * reconnects transiently to publish. Never throws. + */ + async publishCourtesy(event: NostrEvent, timeoutMs = 2000): Promise { + try { await Promise.race([ Promise.allSettled(this.conns.map((conn) => conn.publish(event))), new Promise((resolve) => setTimeout(resolve, timeoutMs)) ]); } catch { // Courtesy only. + } finally { + for (const conn of this.conns) conn.close(); } } diff --git a/packages/core/src/nostr/relay.ts b/packages/core/src/nostr/relay.ts index d41e77f..c0ca73d 100644 --- a/packages/core/src/nostr/relay.ts +++ b/packages/core/src/nostr/relay.ts @@ -69,6 +69,26 @@ function verifyBudgetFor(filter: NostrFilter): number { return Math.max(50, cap * 3); } +/** Live-subscription protections (BL-25): a bounded already-seen id cache + * (replaying one retained valid event must cost one verification and one + * delivery, ever) and a fixed-window verification rate budget (a relay + * spraying DISTINCT events gets quarantined for the rest of the window + * rather than burning CPU). Legitimate NIP-46 traffic -- responses to our + * own requests plus the odd auth_url -- sits orders of magnitude below + * the budget. */ +const LIVE_SEEN_CAP = 2048; +const LIVE_WINDOW_MS = 10_000; +const LIVE_VERIFIES_PER_WINDOW = 100; + +function rememberSeen(seenIds: Set, id: string): void { + if (seenIds.size >= LIVE_SEEN_CAP) { + // Sets iterate in insertion order; evict the oldest. + const oldest = seenIds.values().next().value; + if (oldest !== undefined) seenIds.delete(oldest); + } + seenIds.add(id); +} + export class RelayConnection { readonly url: string; private ws: WebSocketLike | null = null; @@ -82,6 +102,9 @@ export class RelayConnection { /** Remaining signature verifications this subscription will pay for; * Infinity for live subscriptions (BL-21). */ verifyBudget: number; + /** Fixed-window verification rate state for live subscriptions (BL-25). */ + liveWindowStart: number; + liveVerifies: number; filter: NostrFilter; onEose: () => void; /** Present on live subscriptions (subscribeLive): each verified, @@ -176,17 +199,30 @@ export class RelayConnection { // 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.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. + // Every CHEAP check runs before Schnorr verification, so replayed or + // over-limit spam cannot buy signature checks (BL-21, BL-25). const claimedId = (event as { id?: unknown } | null | undefined)?.id; if (typeof claimedId !== "string" || sub.seenIds.has(claimedId)) return; + if (sub.onEvent) { + // Live subscription: bounded dedup plus a rate-limited verification + // window (BL-25). Exceeding the window quarantines further delivery + // from this relay until it rolls over. + const now = Date.now(); + if (now - sub.liveWindowStart >= LIVE_WINDOW_MS) { + sub.liveWindowStart = now; + sub.liveVerifies = 0; + } + if (sub.liveVerifies >= LIVE_VERIFIES_PER_WINDOW) return; + sub.liveVerifies++; + if (!verifyNostrEvent(event) || !matchesNostrFilter(event, sub.filter)) return; + rememberSeen(sub.seenIds, event.id); + sub.onEvent(event); + return; + } + // Buffered query: 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 cap = Math.min(sub.filter.limit ?? MAX_BUFFERED_EVENTS, MAX_BUFFERED_EVENTS); if (sub.events.length >= cap) return; if (sub.verifyBudget <= 0) return; @@ -276,7 +312,16 @@ export class RelayConnection { ): Promise<{ close: () => void }> { await this.connect(); const subId = bytesToHex(randomBytes(8)); - this.subs.set(subId, { events: [], seenIds: new Set(), verifyBudget: Number.POSITIVE_INFINITY, filter, onEose: () => {}, onEvent }); + this.subs.set(subId, { + events: [], + seenIds: new Set(), + verifyBudget: Number.POSITIVE_INFINITY, + liveWindowStart: Date.now(), + liveVerifies: 0, + filter, + onEose: () => {}, + onEvent + }); this.send(["REQ", subId, filter]); return { close: () => { @@ -333,7 +378,15 @@ export class RelayConnection { ), timeoutMs, ); - this.subs.set(subId, { events, seenIds: new Set(), verifyBudget: verifyBudgetFor(filter), filter, onEose: () => settle() }); + this.subs.set(subId, { + events, + seenIds: new Set(), + verifyBudget: verifyBudgetFor(filter), + liveWindowStart: 0, + liveVerifies: 0, + 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 6b4031d..1364e29 100644 --- a/packages/core/src/nostr/relayHardening.test.ts +++ b/packages/core/src/nostr/relayHardening.test.ts @@ -116,6 +116,39 @@ describe("relay hardening", () => { expect(events.length).toBeLessThanOrEqual(1); }); + it("BL-25: live subscriptions dedupe replayed events -- one delivery, ever", async () => { + const key = generatePrivateKey(); + const note = signedNote(key, "live"); + relay.unsolicitedQueryEvents = Array.from({ length: 2000 }, () => note); + const delivered: string[] = []; + const start = Date.now(); + const handle = await conn.subscribeLive({ kinds: [1], authors: [getPublicKeyHex(key)] }, (event) => + delivered.push(event.id) + ); + await new Promise((resolve) => setTimeout(resolve, 600)); + handle.close(); + expect(delivered).toHaveLength(1); + // 2,000 replayed frames cost one verification plus Set lookups. + expect(Date.now() - start).toBeLessThan(3_000); + }); + + it("BL-25: live subscriptions rate-limit verification of distinct events per window", async () => { + const key = generatePrivateKey(); + relay.unsolicitedQueryEvents = Array.from({ length: 300 }, (_, i) => + signedNote(key, `distinct-${i}`, 1_700_000_000 + i) + ); + const delivered: string[] = []; + const handle = await conn.subscribeLive({ kinds: [1], authors: [getPublicKeyHex(key)] }, (event) => + delivered.push(event.id) + ); + await new Promise((resolve) => setTimeout(resolve, 600)); + handle.close(); + // The fixed window admits at most 100 verifications; the rest of the + // spray is quarantined until the window rolls over. + expect(delivered.length).toBeGreaterThan(0); + expect(delivered.length).toBeLessThanOrEqual(100); + }); + 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 f41fb1a..4c1fccd 100644 --- a/packages/widget/src/worker/cryptoWorker.ts +++ b/packages/widget/src/worker/cryptoWorker.ts @@ -277,14 +277,16 @@ function discardNostrconnectPending(): void { 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. */ +/** Retires a session with local revocation FIRST (BL-26): the logout event + * is pre-signed while the key exists, then close() immediately rejects + * every pending request and wipes keys -- nothing can resolve after logout + * begins -- and only then is the courtesy event delivered, asynchronously + * and bounded. Current NIP-46 defines logout as a courtesy, never a + * security boundary, so its delivery is allowed to fail silently. */ function retireNip46Client(client: Nip46Client): void { - void client - .logout() - .catch(() => {}) - .finally(() => client.close()); + const farewell = client.buildLogoutEvent(); + client.close(); + if (farewell) void client.publishCourtesy(farewell); } function adoptNip46Client(client: Nip46Client, userPubkey: string): void { @@ -343,13 +345,23 @@ async function handle(action: string, payload: unknown): Promise { case "nip46NostrconnectAwait": { const pending = nostrconnectPending; if (!pending) throw new Error("No signer-connection attempt is in progress."); - const { signerPubkey } = await listenForNostrconnect({ - clientSecretKey: pending.clientSecretKey, - relayUrls: pending.relayUrls, - secret: pending.secret, - timeoutMs: 180_000, - signal: pending.abort.signal - }); + let signerPubkey: string; + try { + ({ signerPubkey } = await listenForNostrconnect({ + clientSecretKey: pending.clientSecretKey, + relayUrls: pending.relayUrls, + secret: pending.secret, + timeoutMs: 180_000, + signal: pending.abort.signal + })); + } catch (err) { + // Natural failure (timeout, unreachable relays) must not orphan the + // ephemeral key in worker memory (BL-27) -- but only if this attempt + // still owns the pending slot; a superseding attempt already + // discarded it. + if (nostrconnectPending === pending) discardNostrconnectPending(); + throw err; + } 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.