Require minimally-encoded features in BOLT 11 invoices #1245
Conversation
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
24 tasks
There was a problem hiding this comment.
I was just playing with this yesterday lightningdevkit/rust-lightning#3703
I agree that making the invoice valid required a bunch of tricky code that does not make sense to have.
ACK e2e81aa
t-bast
reviewed
Apr 7, 2025
|
Ack. Shall we similarly make the other "SHOULD use minimal encoding" into MUST? |
TheBlueMatt
force-pushed
the
2025-04-require-minimal-11-features
branch
from
7b9451c to
8c712d6
Compare
When decoding BOLT 11 invoices, LDK has always read them into fields, parsing what it can. When parsing features, CLTV deltas, and expiry times, this loses the information on the number of field elements which were used to encode the features in the invoice itself. When we then go to calculate a hash of the invoice for signature validation/key recovery, we go re-serialize the invoice. At this point, any excess field elements used to encode fields will be lost and the invoice's hash will be different from what the original encoder intended. Luckily, this doesn't appear to have ever happened in practice. This was, in fact, only found by @erickcestari, @brunoerg, and @morehouse when doing differential fuzzing. Because this hasn't happened and it breaks a straightforward way to handle BOLT 11 parsing, there's no reason to retain it, so instead here we simply forbid non-minimally-encoded features in BOLT 11 invoices. See lightningdevkit/rust-lightning#3693 for the specific example generated by fuzzing.
@t-bast mentioned on the call that they've run into non-largest multipliers in BOLT 11 invoices in the wild and as such we can't reject them.
TheBlueMatt
force-pushed
the
2025-04-require-minimal-11-features
branch
from
8c712d6 to
4042520
Compare
|
Clarified what the minimal length thing means as requested at the meeting.
Oops, indeed, done. Note that for any of the three LDK would have failed to pay the invoice (invalid signature or wrong pubkey from recovery) and we haven't received any reports of it. Sadly, I did not apply this to the amount because @t-bast indicated at the meeting that they've seen non-minimal ones. |
t-bast
added a commit
to ACINQ/eclair
that referenced
this pull request
Apr 8, 2025
lightning/bolts#1245 highlights that invoice writers must minimally encode Bolt11 features (and other fields). We were already correctly doing that, but tests were lacking. lightning/bolts#1242 added a test vector for invoices without payment secrets, which we're now verifying.
t-bast
added a commit
to ACINQ/lightning-kmp
that referenced
this pull request
Apr 8, 2025
lightning/bolts#1245 highlights that invoice writers must minimally encode Bolt11 features (and other fields). We were already correctly doing that, but tests were lacking. lightning/bolts#1242 added a test vector for invoices without payment secrets, which we're now verifying.
t-bast
approved these changes
Apr 8, 2025
t-bast
added a commit
to ACINQ/eclair
that referenced
this pull request
Apr 8, 2025
lightning/bolts#1245 highlights that invoice writers must minimally encode Bolt11 features (and other fields). We were already correctly doing that, but tests were lacking. lightning/bolts#1242 added a test vector for invoices without payment secrets, which we're now verifying.
t-bast
added a commit
to ACINQ/lightning-kmp
that referenced
this pull request
Apr 8, 2025
lightning/bolts#1245 highlights that invoice writers must minimally encode Bolt11 features (and other fields). We were already correctly doing that, but tests were lacking. lightning/bolts#1242 added a test vector for invoices without payment secrets, which we're now verifying.
23 tasks
|
@TheBlueMatt I think we can merge that PR now, can't we? |
22 tasks
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
When decoding BOLT 11 invoices, LDK has always read them into fields, parsing what it can. When parsing features, this loses the information on the number of field elements which were used to encode the features in the invoice itself.
When we then go to calculate a hash of the invoice for signature validation/key recovery, we go re-serialize the invoice. At this point, any excess field elements used to encode features will be lost and the invoice's hash will be different from what the original encoder intended.
Luckily, this doesn't appear to have ever happened in practice. This was, in fact, only found by @erickcestari, @brunoerg, and @morehouse when doing differential fuzzing.
Because this hasn't happened and it breaks a straightforward way to handle BOLT 11 parsing, there's no reason to retain it, so instead here we simply forbid non-minimally-encoded features in BOLT 11 invoices.
See lightningdevkit/rust-lightning#3693 for the specific example generated by fuzzing.