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.
TheBlueMatt
left a comment
TheBlueMatt
left a comment
There was a problem hiding this comment.
Ugh. Yea, should do, obviously.
|
|
||
| # Examples | ||
|
|
||
| Machine-readable invoice test vectors for all examples below are provided in |
There was a problem hiding this comment.
Seems weird to move the invoice data to a json file but leave the descriptions here, makes it much harder to parse.
|
Concept ACK on the duplicate rule. One suggestion on the test vectors. Marking the "fields which must be ignored" invoice as invalid is correct, but it was also the only example exercising two forward-compatibility paths. I walked the tagged fields of all 28 vectors in the proposed |
| > ### Same, but with unknown fields and invalid fixed-length fields. | ||
|
|
||
| > ### Same, but including fields which must be ignored. | ||
| > lnbc25m1pvjluezpp5qqqsyqcyq5rqwzqfqqqsyqcyq5rqwzqfqqqsyqcyq5rqwzqfqypqdq5vdhkven9v5sxyetpdeessp5zyg3zyg3zyg3zyg3zyg3zyg3zyg3zyg3zyg3zyg3zyg3zyg3zygs9q5sqqqqqqqqqqqqqqqqsgq2qrqqqfppnqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqppnqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqpp4qqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqhpnqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqhp4qqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqspnqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqsp4qqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqnp5qqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqnpkqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqz599y53s3ujmcfjp5xrdap68qxymkqphwsexhmhr8wdz5usdzkzrse33chw6dlp3jhuhge9ley7j2ayx36kawe7kmgg8sv5ugdyusdcqzn8z9x |
There was a problem hiding this comment.
Marking this invoice as invalid is correct, but it was also the only example exercising two forward-compatibility paths:
- the unknown field type 2
- the f field with unknown version 19
There was a problem hiding this comment.
Concept ACK
Fwiw, I was confused about the sender vs. receiver requirements for parsing fields with a fixed length (#1305). I mentioned in the LDK Discord that these two statements seemed to be in conflict if the invoice contained both a valid and an invalid p field:
[a payer] SHOULD use the first
pfield as the payment hash.
[a reader] MUST fail the payment if any field with fixed data_length (p, h, s, n) does not have the correct length (52, 52, 52, 53).
It was confusing because it wasn't clear whether the fixed-length check applies after we pick the first p, or whether it applies to any p, which implies there might be multiple.
I mentioned that I would try to make this less confusing. Maybe I would have found this vulnerability if I had done that 😅
I think that with "MUST use the first p field as the payment hash" and "MUST fail the payment if more than one p field is present" from this PR, it is now clearer how to handle p.
|
Concept ACK. Two things, one substantive and one about the vector format. The same rule arguably belongs on
|
In this commit, we address the reviewer feedback on the duplicate payment hash rejection. We move the release notes entry from BOLT Spec Updates to Bug Fixes in the v0.22.0 notes, since rejecting a second payment hash field is deliberately ahead of the current BOLT 11 text; the entry now carries an interop note and links lightning/bolts#1357 as the motivation. We also add the same entry to the v0.20.5 and v0.21.4 notes, as the fix will be backported to those releases. We replace the stale comment above parse32Bytes, which quoted the old BOLT 11 rule that a reader must skip over 32-byte fields with an unsupported length. The current reader requirements say the opposite: a reader must fail the payment if a fixed-length field (p, h, s, n) does not have the correct length. The new comment states that reporting the field as absent is a known divergence, and explains why the end result for the payment hash is the same either way. In the tests, we keep a valid variant of the flipped vector that drops the duplicate payment hash but retains the wrong-length h and n fields, so the coverage that such fields are skipped survives. The duplicate vector stays in the decode table, now pinning ErrDuplicatePaymentHash through the new wantErr field rather than asserting any decode error. We also replace the rapid property test with two table rows in TestParseTaggedFields covering identical and distinct payment hashes, since the drawn bytes could not influence the outcome.
In this commit, we address the reviewer feedback on the duplicate payment hash rejection. We move the release notes entry from BOLT Spec Updates to Bug Fixes in the v0.22.0 notes, since rejecting a second payment hash field is deliberately ahead of the current BOLT 11 text; the entry now carries an interop note and links lightning/bolts#1357 as the motivation. We also add the same entry to the v0.20.5 and v0.21.4 notes, as the fix will be backported to those releases. We replace the stale comment above parse32Bytes, which quoted the old BOLT 11 rule that a reader must skip over 32-byte fields with an unsupported length. The current reader requirements say the opposite: a reader must fail the payment if a fixed-length field (p, h, s, n) does not have the correct length. The new comment states that reporting the field as absent is a known divergence, and explains why the end result for the payment hash is the same either way. In the tests, we keep a valid variant of the flipped vector that drops the duplicate payment hash but retains the wrong-length h and n fields, so the coverage that such fields are skipped survives. The duplicate vector stays in the decode table, now pinning ErrDuplicatePaymentHash through the new wantErr field rather than asserting any decode error. We also replace the rapid property test with two table rows in TestParseTaggedFields covering identical and distinct payment hashes, since the drawn bytes could not influence the outcome.
In this commit, we reject BOLT 11 invoices that contain more than one payment hash (`p`) field. `zpay32.Decode` previously kept the first supported payment hash and ignored later fields, so the interpreted payment hash depended on field order. The decoder now tracks whether a `p` field has appeared separately from whether its contents parsed successfully, and any later `p` field returns `ErrDuplicatePaymentHash`. This covers identical hashes, distinct hashes, and a malformed first field followed by a valid one. The change is deliberately stricter than the current BOLT 11 text, which tells a reader to use the first `p` field; it is motivated by lightning/bolts#1357, and will be backported to the v0.20.5 and v0.21.4 releases. We also replace the stale comment above parse32Bytes, which quoted the old BOLT 11 rule that a reader must skip over 32-byte fields with an unsupported length. The current reader requirements say the opposite: a reader must fail the payment if a fixed-length field (p, h, s, n) does not have the correct length. The new comment states that reporting the field as absent is a known divergence, and explains why the end result for the payment hash is the same either way. In the tests, we cover identical and distinct duplicate hashes, a malformed first field followed by a valid one, and the signed vectors. A valid variant of the flipped vector keeps the wrong-length h and n fields, so the coverage that such fields are skipped survives. The duplicate vector in the decode table pins ErrDuplicatePaymentHash through a new wantErr field rather than asserting any decode error.
In this commit, we reject BOLT 11 invoices that contain more than one payment hash (`p`) field. `zpay32.Decode` previously kept the first supported payment hash and ignored later fields, so the interpreted payment hash depended on field order. The decoder now tracks whether a `p` field has appeared separately from whether its contents parsed successfully, and any later `p` field returns `ErrDuplicatePaymentHash`. This covers identical hashes, distinct hashes, and a malformed first field followed by a valid one. The change is deliberately stricter than the current BOLT 11 text, which tells a reader to use the first `p` field; it is motivated by lightning/bolts#1357, and will be backported to the v0.20.5 and v0.21.4 releases. We also replace the stale comment above parse32Bytes, which quoted the old BOLT 11 rule that a reader must skip over 32-byte fields with an unsupported length. The current reader requirements say the opposite: a reader must fail the payment if a fixed-length field (p, h, s, n) does not have the correct length. The new comment states that reporting the field as absent is a known divergence, and explains why the end result for the payment hash is the same either way. In the tests, we cover identical and distinct duplicate hashes, a malformed first field followed by a valid one, and the signed vectors. A valid variant of the flipped vector keeps the wrong-length h and n fields, so the coverage that such fields are skipped survives. The duplicate vector in the decode table pins ErrDuplicatePaymentHash through a new wantErr field rather than asserting any decode error. (cherry picked from commit e2f2706)
In this commit, we reject BOLT 11 invoices that contain more than one payment hash (`p`) field. `zpay32.Decode` previously kept the first supported payment hash and ignored later fields, so the interpreted payment hash depended on field order. The decoder now tracks whether a `p` field has appeared separately from whether its contents parsed successfully, and any later `p` field returns `ErrDuplicatePaymentHash`. This covers identical hashes, distinct hashes, and a malformed first field followed by a valid one. The change is deliberately stricter than the current BOLT 11 text, which tells a reader to use the first `p` field; it is motivated by lightning/bolts#1357, and will be backported to the v0.20.5 and v0.21.4 releases. We also replace the stale comment above parse32Bytes, which quoted the old BOLT 11 rule that a reader must skip over 32-byte fields with an unsupported length. The current reader requirements say the opposite: a reader must fail the payment if a fixed-length field (p, h, s, n) does not have the correct length. The new comment states that reporting the field as absent is a known divergence, and explains why the end result for the payment hash is the same either way. In the tests, we cover identical and distinct duplicate hashes, a malformed first field followed by a valid one, and the signed vectors. A valid variant of the flipped vector keeps the wrong-length h and n fields, so the coverage that such fields are skipped survives. The duplicate vector in the decode table pins ErrDuplicatePaymentHash through a new wantErr field rather than asserting any decode error. (cherry picked from commit e2f2706)
| A reader: | ||
| - MUST skip over `f` fields that use an unknown `version`. | ||
| - MUST fail the payment if any field with fixed `data_length` (`p`, `h`, `s`, `n`) does not have the correct length (52, 52, 52, 53). | ||
| - MUST fail the payment if more than one `p` field is present. |
There was a problem hiding this comment.
should we rephrase this to something like "MUST fail unless exactly one p field is present" such that it's clear we should fail if p < 1?
7 participants
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.