Skip to content

Fix panic in Bolt11Invoice::recover_payee_pub_key#4717

Merged
TheBlueMatt merged 2 commits into
lightningdevkit:mainfrom
tnull:tnull/2026-06-invoice-recovery-id
Jun 18, 2026
Merged

Fix panic in Bolt11Invoice::recover_payee_pub_key#4717
TheBlueMatt merged 2 commits into
lightningdevkit:mainfrom
tnull:tnull/2026-06-invoice-recovery-id

Conversation

@tnull

@tnull tnull commented Jun 18, 2026

Copy link
Copy Markdown
Contributor

Previously, Bolt11Invoice::recover_payee_pub_key unconditionally expected the inner secp256k1 recovery call. This would allow a signer to mint an invoice whose recoverable signature carries an in-range-but-unusable recovery_id . When any sender-side code later calls recover_payee_pub_key() on the parsed invoice the .expect fires and the process panics.

Here we fix this by first making recover_payee_pub_key fall back to the safe get_payee_pub_key behavior (to be backported) and then make recover_payee_pub_key return an Option in a second commit (not intended for backporting due to API change).

Reported by Project Loupe.

tnull added 2 commits June 18, 2026 18:04
Payment parameters should use the canonical payee key from BOLT11
invoices. When an invoice includes an n field, using that key avoids
attempting signature recovery that may legitimately be unavailable.

Co-Authored-By: HAL 9000

This finding was discovered by Project Loupe
Recovering a BOLT11 payee key can fail even when an invoice includes a
valid n field. Return the recovery result as an option and document
get_payee_pub_key as the canonical accessor.

Co-Authored-By: HAL 9000

This finding was discovered by Project Loupe
@ldk-reviews-bot

Copy link
Copy Markdown

👋 Hi! Please choose at least one reviewer by assigning them on the right bar.
If no reviewers are assigned within 10 minutes, I'll automatically assign one.
Once the first reviewer has submitted a review, a second will be assigned if required.

@ldk-claude-review-bot

Copy link
Copy Markdown
Collaborator

No issues found.

I reviewed all four changed regions:

  • lightning-invoice/src/lib.rsrecover_payee_pub_key now returns Option (no panic), and the .expect moved into get_payee_pub_key's None branch. This is safe: that branch is only reached when no n field is present, and in that case the from_signed constructor's check_signature guarantees signature recovery succeeds (and built invoices are covered by the sign_function contract). The added test correctly exercises the failing-recovery-id case.
  • lightning/src/ln/invoice_utils.rs and lightning/src/routing/router.rs — callers correctly switched from recover_payee_pub_key() to get_payee_pub_key(), which is the API-compatible non-Option accessor. The new router test reproduces the in-range-but-unusable recovery_id scenario.

The public API change (recover_payee_pub_keyOption) is intentional per the PR description, and all in-repo callers were updated (it's now only referenced within lib.rs).

@TheBlueMatt TheBlueMatt left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thanks

@TheBlueMatt TheBlueMatt merged commit ab69b33 into lightningdevkit:main Jun 18, 2026
1 check passed
This was referenced Jun 18, 2026
@TheBlueMatt

Copy link
Copy Markdown
Collaborator

Backported to 0.2 in #4722.

@TheBlueMatt

Copy link
Copy Markdown
Collaborator

Backported to 0.1 in #4720.

TheBlueMatt added a commit that referenced this pull request Jun 19, 2026
v0.1.10 - Jun 18, 2026 - "Loupe de Loupe"

API Updates
===========

 * `DefaultMessageRouter` will now always generate blinded message paths that
   provide no privacy (where our node is the introduction node) for nodes with
   public channels. This works around an issue which will appear for any nodes
   with LND peers that enable onion messaging - such peers will refuse to
   forward BOLT 12 messages from unknown third parties, which most BOLT 12
   payers rely on today (#4647).
 * Explicit `amount_msats` of 0 is rejected in BOLT 12 `Offer`s; `OfferBuilder`
   now maps 0-amounts to an amount of `None` (#4324).

Bug Fixes
=========

 * Async `ChannelMonitorUpdate` persistence operations which complete, but are
   not marked as complete in a persisted `ChannelManager` prior to restart,
   followed immediately by a block connection and then another restart could
   result in some channel operations hanging leading for force-closures (#4377).
 * If an MPP payment is claimed but `ChannelMonitorUpdate`s for some parts are
   still being completed asynchronously, further channel updates (e.g.
   forwarding another payment) are pending and the node restarts, the channel
   could have become stuck (#4520).
 * The presence of unconfirmed transactions actually no longer causes
   `ElectrumSyncClient` to spuriously fail to sync (#4590).
 * `FilesystemStore::list_all_keys` will no longer fail if there are stale
   intermediate files lying around from a previous unclean shutdown (#4618).
 * When forwarding an HTLC while in a blinded path with proportional fees over
   200%, LDK will no longer spuriously allow a forward that pays us 1 msat too
   little in fees (#4697).
 * Fixed a rare case where a channel could get stuck on reconnect when using
   both async `ChannelMonitorUpdate` persistence and async signing (#4684).
 * `Event::PaymentSent::fee_paid_msat` is no longer `None` in cases where
   `ChannelManager::abandon_payment` was called before the payment ultimately
   completes anyway (#4651).
 * Syncing a `ChainMonitor` using the `Confirm` trait will no longer write some
   full `ChannelMonitor`s to disk several times per block (#4544).
 * `OMDomainResolver` now correctly accounts for failed queries when rate
   limiting, ensuring we continue to respond to queries after failures (#4591).
 * Calling `ChannelManager::send_payment_with_route` without a `route_params`
   and with an invalid `Route` will no longer panic (#4707).
 * `lightning-custom-message`'s handling of `peer_connected` events now ensures
   that sub-handlers will see a `peer_disconnected` event if a different
   sub-handler refused the connection by `Err`ing `peer_connected` (#4595).
 * Incomplete MPP keysend payments will no longer see their HTLCs held until
   expiry (#4558).
 * `InvoiceRequestBuilder` will no longer accept a `quantity` of `0` for a
   BOLT 12 `Offer`, allowing any quantity up to a bound (#4667).
 * `lightning-custom-message` handlers that return `Ok(None)` when asked to
   deserialize a message in their defined range no longer cause panics (#4709).
 * Several spurious debug assertions were fixed (#4537, #4618).

Security
========

0.1.10 fixes a sanitization issue and several denial-of-service vulnerabilities.
 * `Bolt11Invoice::recover_payee_pub_key` no longer panics if called on an
   invoice which set an explicit public key, rather than relying on public key
   recovery. This method is called from `payment_parameters_from_invoice` and
   `payment_parameters_from_variable_amount_invoice` (#4717).
 * Maliciously-crafted unpayable invoices which have overflowing feerates will
   no longer cause an `unwrap` failure panic (#4716).
 * `possiblyrandom` did not properly generate random data except when it was
   explicitly configured to. By default this means LDK is vulnerable to various
   HashDoS attacks (#4719).
 * `OMNameResolver` will no longer panic when looking up payment instructions
   which include unicode characters at the start of a TXT record (#4718).
 * `PrintableString` did not properly sanitize unicode format characters,
   allowing an attacker to corrupt the rendering of logs or UI (#4593, #4605).
 * RGS data is now limited in how large of a graph it is able to cause a client
   to store in memory. Note that RGS data is still considered a DoS vector in
   general and you should only use semi-trusted RGS data (#4713).
 * Counterparty-provided strings in failure messages are no longer logged in
   full, reducing the ability of such a counterparty to spam our logs (#4714).
 * Reading a corrupted `ChannelManager` or `ProbabilisticScorer` can no longer
   cause us to allocate large amounts of memory (#4712).

Thanks to Project Loupe for reporting most of the issues fixed in this release.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants