From e2351f8f7b6f18b496c34d4b3d8a7dd9a5a1cea5 Mon Sep 17 00:00:00 2001 From: Padreug Date: Tue, 26 May 2026 10:06:05 +0200 Subject: [PATCH] security(lnbits): Schnorr-verify inbound reply events before decrypting MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit LnbitsClient.handleReply matched replies by request_id alone — no verification that the inbound event was actually signed by the configured LNbits server pubkey. The relay's subscription `authors` filter is relay-honour, not relay-enforced; a malicious or buggy relay could forward an event with the server's pubkey in the body but signed by a different key (or with a tampered body whose id no longer matches). Adds `isAuthenticServerEvent(ev, expectedPubkey)`: - `ev.pubkey === expectedPubkey` (explicit, not relying on filter) - `verifyEvent(ev)` (catches sig/id/content tampering) handleReply calls it before passing the event to decryptContentV2. NIP-44 v2 already binds ciphertext to sender via ECDH, so a relay without the server's nsec can't forge decryptable content — but verifying the outer event keeps `ev.id` trustworthy for any downstream dedup/logging code and matches the symmetric defence on the server side (`nostr_transport/relay_pool.py:~320`) and on the lnbits-bunker-client side (`aiolabs/lnbits` commit 4ebcd959, `NsecBunkerAdminClient._match_response`). Test `packages/lnbits/src/__tests__/client.test.ts` covers: - legitimate server-signed event accepted - event whose pubkey field doesn't match config rejected - forged event (signed by attacker, pubkey overwritten to server) rejected — recomputed id no longer matches stored id - event with tampered content (id mismatch) rejected Gotcha worth noting: `finalizeEvent` stamps `event[verifiedSymbol] = true` to cache the verification result, and `{...ev}` spread copies symbol- keyed properties. So forged/tampered events constructed via spread inherit the cached `true` and `verifyEvent` short-circuits. The test JSON-round-trips through `stripVerifiedCache` to drop the cache. Closes aiolabs/lamassu-next#49. Co-Authored-By: Claude Opus 4.7 (1M context) --- packages/lnbits/src/__tests__/client.test.ts | 129 +++++++++++++++++++ packages/lnbits/src/client.ts | 36 +++++- 2 files changed, 164 insertions(+), 1 deletion(-) create mode 100644 packages/lnbits/src/__tests__/client.test.ts diff --git a/packages/lnbits/src/__tests__/client.test.ts b/packages/lnbits/src/__tests__/client.test.ts new file mode 100644 index 0000000..faf619c --- /dev/null +++ b/packages/lnbits/src/__tests__/client.test.ts @@ -0,0 +1,129 @@ +/** + * Tests for LnbitsClient's defensive authentication of inbound reply events. + * + * Covers the patch that mirrors lnbits commit `4ebcd959` + * (NsecBunkerAdminClient._match_response Schnorr-verifies before decrypting). + * See aiolabs/lamassu-next#49 + the cross-codebase coordination memory. + */ + +import { describe, it, expect } from 'vitest' +import { + generateSecretKey, + getPublicKey, + finalizeEvent, + getEventHash, + type Event as NostrEvent, + type UnsignedEvent, +} from 'nostr-tools' +import { isAuthenticServerEvent } from '../client.js' + +const KIND_LNBITS_RPC = 21000 + +/** + * Forge an event by signing with one key, then overwriting the `pubkey` + * field to claim a different author. The id field stays bound to the + * original (attacker) pubkey, so `verifyEvent` catches the mismatch: + * recomputed id (over the claimed pubkey) != stored id. This models a + * malicious relay that constructs a kind-21000 event claiming to be + * from the server. Mirror of the lnbits-side test + * `test_match_response_rejects_forged_signature`. + * + * Important: `finalizeEvent` stamps `event[verifiedSymbol] = true` on + * its return value, and `verifyEvent` short-circuits on that cached + * flag. Object spread copies symbol-keyed properties, so we round-trip + * through JSON to drop the cache and force re-verification. + */ +function forgeEvent( + template: Omit, + claimedPubkey: string, + signingKey: Uint8Array, +): NostrEvent { + const signed = finalizeEvent(template, signingKey) + const stripped = JSON.parse(JSON.stringify(signed)) as NostrEvent + return { ...stripped, pubkey: claimedPubkey } +} + +/** Drop the cached `verifiedSymbol` flag that `finalizeEvent` stamps. */ +function stripVerifiedCache(ev: NostrEvent): NostrEvent { + return JSON.parse(JSON.stringify(ev)) as NostrEvent +} + +describe('isAuthenticServerEvent', () => { + const serverKey = generateSecretKey() + const serverPubkey = getPublicKey(serverKey) + const recipientKey = generateSecretKey() + const recipientPubkey = getPublicKey(recipientKey) + + it('accepts a genuine event signed by the configured server', () => { + const ev = finalizeEvent( + { + kind: KIND_LNBITS_RPC, + content: '', + tags: [['p', recipientPubkey]], + created_at: Math.floor(Date.now() / 1000), + }, + serverKey, + ) + expect(isAuthenticServerEvent(ev, serverPubkey)).toBe(true) + }) + + it('rejects an event whose pubkey field does not match the configured server', () => { + const otherKey = generateSecretKey() + const ev = finalizeEvent( + { + kind: KIND_LNBITS_RPC, + content: '', + tags: [['p', recipientPubkey]], + created_at: Math.floor(Date.now() / 1000), + }, + otherKey, + ) + // Sanity: this event is internally consistent and would verify + // under its own pubkey. The point is it's NOT from our server. + expect(ev.pubkey).not.toBe(serverPubkey) + expect(isAuthenticServerEvent(ev, serverPubkey)).toBe(false) + }) + + it('rejects a forged event with serverPubkey claim but attacker-signed', () => { + // A malicious relay signs an event with its own key, then overwrites + // the `pubkey` field to claim it came from the server. The id is + // still bound to the attacker's pubkey under the original signature, + // so the recomputed id (over the spoofed pubkey) won't match — + // verifyEvent must reject. + const attackerKey = generateSecretKey() + const forged = forgeEvent( + { + kind: KIND_LNBITS_RPC, + content: '', + tags: [['p', recipientPubkey]], + created_at: Math.floor(Date.now() / 1000), + }, + serverPubkey, + attackerKey, + ) + // Sanity: the body claims serverPubkey… + expect(forged.pubkey).toBe(serverPubkey) + // …but the stored id was computed over the attacker's pubkey, so + // recomputing it over the claimed pubkey gives a different value. + expect(forged.id).not.toBe(getEventHash(forged as UnsignedEvent)) + expect(isAuthenticServerEvent(forged, serverPubkey)).toBe(false) + }) + + it('rejects an event with tampered content (id mismatch)', () => { + // Real signed event, then content is rewritten — id won't match. + const ev = finalizeEvent( + { + kind: KIND_LNBITS_RPC, + content: '', + tags: [['p', recipientPubkey]], + created_at: Math.floor(Date.now() / 1000), + }, + serverKey, + ) + const tampered: NostrEvent = { + ...stripVerifiedCache(ev), + content: '', + } + expect(isAuthenticServerEvent(tampered, serverPubkey)).toBe(false) + }) +}) diff --git a/packages/lnbits/src/client.ts b/packages/lnbits/src/client.ts index 0add0b8..0d4fc51 100644 --- a/packages/lnbits/src/client.ts +++ b/packages/lnbits/src/client.ts @@ -27,7 +27,7 @@ import { encryptContentV2, decryptContentV2, } from '@bitSpire/nostr-client' -import { finalizeEvent } from 'nostr-tools' +import { finalizeEvent, verifyEvent } from 'nostr-tools' import type { LnbitsConfig, @@ -48,6 +48,39 @@ import type { const LNBITS_KIND_RPC = 21000 +/** + * Authenticate an inbound kind-21000 event as genuinely from the configured + * LNbits server before trusting any of its fields. Two checks: + * + * 1. `ev.pubkey === expectedPubkey` — defends against a relay that + * ignores our subscription `authors` filter and forwards events from + * other authors (the filter is relay-honour, not relay-enforced). + * + * 2. `verifyEvent(ev)` — Schnorr-verify the outer signature against the + * claimed pubkey. Defends against a malicious relay that constructs an + * event with the server's pubkey in the `pubkey` field but signs with + * its own key (or any non-server key). Without this check, a + * malformed/tampered event would fall through to `decryptContentV2`, + * which would fail at the MAC layer because NIP-44 v2 binds the + * ciphertext to the sender's key — but defense in depth is worth the + * one extra Schnorr verification, and it keeps `ev.id` trustworthy + * for any logging/dedup code further downstream. + * + * Mirror of the same defence in `aiolabs/lnbits` commit `4ebcd959` + * (`NsecBunkerAdminClient._match_response`). Symmetric trust boundary + * across server inbound, lnbits-bunker-client inbound, and ATM-client + * inbound — see `aiolabs/lamassu-next#49` and the cross-codebase + * coordination memory. + */ +export function isAuthenticServerEvent( + ev: NostrEvent, + expectedPubkey: string, +): boolean { + if (ev.pubkey !== expectedPubkey) return false + if (!verifyEvent(ev)) return false + return true +} + /** Active streaming subscription state held client-side. */ interface ActiveSubscription { subscriptionId: string @@ -446,6 +479,7 @@ export class LnbitsClient { private handleReply(ev: NostrEvent): void { if (!this.identity) return + if (!isAuthenticServerEvent(ev, this.config.serverPubkey)) return let plaintext: string try { plaintext = decryptContentV2(this.identity, this.config.serverPubkey, ev.content)