aiolabs/bitspire
9
0
Fork 0
Code Issues 52 Pull requests 2 Projects Releases Packages Wiki Activity Actions

security: verify reply event signature in LnbitsClient.sendRpc #49

New issue
Closed
opened 2026-06-13 22:03:01 +00:00 by padreug · 1 comment
padreug commented 2026-06-13 22:03:01 +00:00
Owner
Copy link

Migrated from aiolabs/lamassu-next#49 — opened by @padreug on 2026-05-26.\n\n## Problem

packages/lnbits/src/client.ts:sendRpc matches replies by request_id string alone — there is no signature verification on the inbound reply event before its decrypted content is used to resolve the pending Promise.

A compromised relay can forge a kind-21000 event spoofing the LNbits server pubkey (the relay can publish any event with any author claim — the relay isn't bound by signature on rebroadcast unless the consumer checks). With a stolen / observed request_id, the relay can return a doctored response — e.g. "payment.status = success" when the actual payment never settled, or a tampered payment_request value redirecting customer payments.

The current code at packages/lnbits/src/client.ts:447-481 (approx) decrypts and uses the reply event's content directly:

const pending = this.pending.get(parsed.request_id)
if (!pending) return
// ... decrypt, parse, resolve(parsed.data)

There is no verifyEvent() call against the configured serverPubkey.

Why this matters

  • The relay is in our confidentiality boundary (it sees ciphertext that's MAC-protected by NIP-44 v2) but the reply path is NOT in our integrity boundary because we never verify the reply event's outer Schnorr signature against the configured server pubkey.
  • A compromised relay can forge "payment settled" → ATM dispenses cash.
  • This is the highest-leverage 2h patch in the codebase from a security standpoint.

Fix

Before resolving the pending promise, verify the inbound event:

import { verifyEvent } from 'nostr-tools/pure'

// in the relay subscription handler
if (ev.pubkey !== this.config.serverPubkey) return  // wrong author
if (!verifyEvent(ev)) return                         // forged signature
// only NOW decrypt and resolve

Two-step check: author pubkey matches config AND Schnorr sig is valid. Both checks are cheap; verifyEvent is what we already use to validate any inbound kind-21000.

Acceptance

  • LnbitsClient.sendRpc rejects reply events whose pubkey ≠ configured serverPubkey.
  • LnbitsClient.sendRpc rejects reply events whose Schnorr signature does not verify.
  • Negative test: publish a doctored kind-21000 event spoofing the server pubkey (but signed by a different key) to the relay — pending promise must NOT resolve.
  • Healthy production path round-trip latency change: < 1ms.

References

  • Identified during the 2026-05-26 cross-codebase review pass (security audit agent finding #3, severity HIGH).
  • Related: aiolabs/lamassu-next#24 (Nostr-native LNURL completion — also needs sig verify on inbound notification events).
  • Estimated effort: 2h.
> _Migrated from [aiolabs/lamassu-next#49](https://git.atitlan.io/aiolabs/lamassu-next/issues/49) — opened by @padreug on 2026-05-26._\n\n## Problem `packages/lnbits/src/client.ts:sendRpc` matches replies by `request_id` string alone — there is **no signature verification on the inbound reply event** before its decrypted content is used to resolve the pending Promise. A compromised relay can forge a kind-21000 event spoofing the LNbits server pubkey (the relay can publish any event with any author claim — the relay isn't bound by signature on rebroadcast unless the consumer checks). With a stolen / observed `request_id`, the relay can return a doctored response — e.g. "payment.status = success" when the actual payment never settled, or a tampered `payment_request` value redirecting customer payments. The current code at `packages/lnbits/src/client.ts:447-481` (approx) decrypts and uses the reply event's content directly: ```ts const pending = this.pending.get(parsed.request_id) if (!pending) return // ... decrypt, parse, resolve(parsed.data) ``` There is no `verifyEvent()` call against the configured `serverPubkey`. ## Why this matters - The relay is in our *confidentiality* boundary (it sees ciphertext that's MAC-protected by NIP-44 v2) but the reply path is NOT in our *integrity* boundary because we never verify the reply event's outer Schnorr signature against the configured server pubkey. - A compromised relay can forge "payment settled" → ATM dispenses cash. - This is the highest-leverage 2h patch in the codebase from a security standpoint. ## Fix Before resolving the pending promise, verify the inbound event: ```ts import { verifyEvent } from 'nostr-tools/pure' // in the relay subscription handler if (ev.pubkey !== this.config.serverPubkey) return // wrong author if (!verifyEvent(ev)) return // forged signature // only NOW decrypt and resolve ``` Two-step check: author pubkey matches config AND Schnorr sig is valid. Both checks are cheap; `verifyEvent` is what we already use to validate any inbound kind-21000. ## Acceptance - [ ] `LnbitsClient.sendRpc` rejects reply events whose `pubkey` ≠ configured `serverPubkey`. - [ ] `LnbitsClient.sendRpc` rejects reply events whose Schnorr signature does not verify. - [ ] Negative test: publish a doctored kind-21000 event spoofing the server pubkey (but signed by a different key) to the relay — pending promise must NOT resolve. - [ ] Healthy production path round-trip latency change: < 1ms. ## References - Identified during the 2026-05-26 cross-codebase review pass (security audit agent finding #3, severity HIGH). - Related: `aiolabs/lamassu-next#24` (Nostr-native LNURL completion — also needs sig verify on inbound notification events). - Estimated effort: 2h.
padreug commented 2026-06-13 22:05:07 +00:00
Author
Owner
Copy link

@padreug commented on 2026-05-26 (lamassu-next#49):

Shipped on dev at commit 0dcbe44.

Implementation:

  • New isAuthenticServerEvent(ev, expectedPubkey) exported from packages/lnbits/src/client.ts:51-78. Two checks: ev.pubkey === expectedPubkey (defends against relay ignoring authors filter) + verifyEvent(ev) (catches forged sig / tampered body).
  • handleReply() calls it before decryptContentV2.
  • Symmetric with aiolabs/lnbits commit 4ebcd959 (NsecBunkerAdminClient._match_response) and the server-side check at nostr_transport/relay_pool.py:~320.

Tests: packages/lnbits/src/__tests__/client.test.ts — 4/4 passing. Covers:

  • legitimate event accepted
  • pubkey-mismatch rejected
  • forged event (attacker-signed, pubkey overwritten to server) rejected
  • tampered content (id mismatch) rejected

Threat-model nuance worth noting in retrospect: the original issue body claimed a compromised relay could forge "payment settled" → ATM dispenses. That's not actually possible against the current code because NIP-44 v2 already binds the ciphertext to the sender's pubkey via ECDH(server_priv, recipient_pub) — a relay without the server's nsec can't produce content that decrypts cleanly. So decryptContentV2 alone provides sender authentication in the current code path.

The patch is still valuable as:

  1. Defense in depth — explicit verification independent of the encryption layer's properties.
  2. Audit clarity — "verify before trust" is the canonical Nostr pattern; not doing it required reading the NIP-44 v2 internals to know why it was safe.
  3. Future-proofing — if a future refactor ever decouples content authentication from server pubkey (e.g. derives sender from ev.pubkey), the guard prevents regression.
  4. Symmetry across the stack — server inbound, lnbits-bunker-client inbound, and ATM-client inbound now all verify the same way.

Gotcha discovered: finalizeEvent stamps event[verifiedSymbol] = true to cache verifyEvent's result. Object spread copies symbol-keyed properties. So a tampered event constructed via {...ev, content: 'tampered'} inherits the cached true and verifyEvent short-circuits. The test JSON-round-trips through stripVerifiedCache() to drop the cache before forging. Noting this here so future Nostr-related tests in the codebase don't trip on the same thing.

Closing.

> _@padreug commented on 2026-05-26 ([lamassu-next#49](https://git.atitlan.io/aiolabs/lamassu-next/issues/49#issuecomment-1122)):_ Shipped on `dev` at commit `0dcbe44`. **Implementation:** - New `isAuthenticServerEvent(ev, expectedPubkey)` exported from `packages/lnbits/src/client.ts:51-78`. Two checks: `ev.pubkey === expectedPubkey` (defends against relay ignoring `authors` filter) + `verifyEvent(ev)` (catches forged sig / tampered body). - `handleReply()` calls it before `decryptContentV2`. - Symmetric with `aiolabs/lnbits` commit `4ebcd959` (`NsecBunkerAdminClient._match_response`) and the server-side check at `nostr_transport/relay_pool.py:~320`. **Tests:** `packages/lnbits/src/__tests__/client.test.ts` — 4/4 passing. Covers: - legitimate event accepted - pubkey-mismatch rejected - forged event (attacker-signed, pubkey overwritten to server) rejected - tampered content (id mismatch) rejected **Threat-model nuance worth noting in retrospect:** the original issue body claimed a compromised relay could forge "payment settled" → ATM dispenses. That's *not actually possible against the current code* because NIP-44 v2 already binds the ciphertext to the sender's pubkey via ECDH(server_priv, recipient_pub) — a relay without the server's nsec can't produce content that decrypts cleanly. So `decryptContentV2` alone provides sender authentication in the current code path. The patch is still valuable as: 1. **Defense in depth** — explicit verification independent of the encryption layer's properties. 2. **Audit clarity** — "verify before trust" is the canonical Nostr pattern; not doing it required reading the NIP-44 v2 internals to know why it was safe. 3. **Future-proofing** — if a future refactor ever decouples content authentication from server pubkey (e.g. derives sender from `ev.pubkey`), the guard prevents regression. 4. **Symmetry across the stack** — server inbound, lnbits-bunker-client inbound, and ATM-client inbound now all verify the same way. **Gotcha discovered:** `finalizeEvent` stamps `event[verifiedSymbol] = true` to cache `verifyEvent`'s result. Object spread copies symbol-keyed properties. So a tampered event constructed via `{...ev, content: 'tampered'}` inherits the cached `true` and `verifyEvent` short-circuits. The test JSON-round-trips through `stripVerifiedCache()` to drop the cache before forging. Noting this here so future Nostr-related tests in the codebase don't trip on the same thing. Closing.
Sign in to join this conversation.
No Branch/Tag specified
Branches Tags
main
dev
feat/rpi4-target
feat/rpi5-apex-hal
fix/sintra-timezone
wip/rpi4-sketch
No results found.
Labels
Clear labels
No items
No labels
Milestone
Clear milestone
No items
No milestone
Projects
Clear projects
No items
No project
Assignees
Clear assignees
No assignees
1 participant
Notifications
Due date
The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set.

Reference
aiolabs/bitspire#49
No description provided.