bolt11: reject duplicate payment hashes in invoices - #1357
Open
Roasbeef wants to merge 5 commits into
Open
Conversation
In this commit, we correct the existing tagged-field vector to match the reader requirement for fixed-length fields. The vector previously labeled malformed p, h, s, and n fields as ignored, even though a reader must fail the payment when their lengths are invalid. We now mark the invoice and those fields as invalid, while leaving the unknown fallback version marked as ignored.
In this commit, we require readers to fail a payment when an invoice contains more than one p field. The prior payer rule selected the first field, which allowed two components to interpret the same signed invoice using different payment hashes. A valid invoice now has one unambiguous p field, and the payer must use it.
In this commit, we add two signed invalid invoices with two valid-length p fields. One uses distinct hashes to catch first- or last-field selection, while the other repeats the same hash to ensure implementations count fields instead of deduplicating values. Both vectors use the spec's documented key and have valid signatures and checksums, leaving the duplicate p fields as the intended rejection condition.
In this commit, we move the duplicate payment hash invoice vectors into a machine-readable JSON file. The BOLT 11 document now links to the canonical vectors instead of embedding a second copy.
In this commit, we move every encoded BOLT 11 invoice into the machine-readable test vector file. The prose keeps the detailed breakdowns, while the JSON file becomes the single source for both valid and invalid invoice strings.
29 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
In this PR, we modify the spec to reject an invoice with duplicate payment hashes.
Unfortunately there was a recent hack o fa popular telegram p2p bot that exploited this ambiguity. Some libraries took the first payment hash, while some took the last. If you were running a system that exhibited both variants of the behavior (diff between libraries, or nodes, etc), then your system could be tricked into thinking it never paid out a withdrawl, thereby leading to a vuln that could drain the system.
IMO there's no reason an invoice should have > 1 payment hash, so we should just reject it.
Test vectors have been updated accordingly.