From 5e7d3e12069eae41be988c836c972c438c9342ec Mon Sep 17 00:00:00 2001 From: Olaoluwa Osuntokun Date: Tue, 22 Sep 2026 19:36:04 -0700 Subject: [PATCH] zpay32: reject duplicate payment hash fields 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 e2f2706367aa6e7a39f8ee14204b43087e27600f) --- docs/release-notes/release-notes-0.20.5.md | 9 +++ zpay32/decode.go | 23 ++++--- zpay32/invoice.go | 6 ++ zpay32/invoice_internal_test.go | 42 +++++++++++++ zpay32/invoice_test.go | 70 +++++++++++++++++++++- 5 files changed, 140 insertions(+), 10 deletions(-) diff --git a/docs/release-notes/release-notes-0.20.5.md b/docs/release-notes/release-notes-0.20.5.md index 12d0dd6e52d..f956077e395 100644 --- a/docs/release-notes/release-notes-0.20.5.md +++ b/docs/release-notes/release-notes-0.20.5.md @@ -43,6 +43,14 @@ failures](https://github.com/lightningnetwork/lnd/pull/11161), while preserving the recorded outcome for replayed HTLCs. +* BOLT 11 invoice decoding [now + rejects](https://github.com/lightningnetwork/lnd/pull/11190) invoices that + contain more than one payment hash (`p`) field, including duplicate fields + with unsupported lengths. This is stricter than the current BOLT 11 text, + which tells a reader to use the first `p` field; the change is motivated by + [lightning/bolts#1357](https://github.com/lightning/bolts/pull/1357), and + is an interop consideration for any wallet emitting such invoices. + # New Features ## Functional Enhancements @@ -100,5 +108,6 @@ * Dario Anongba Varela * elsirion * Gijs van Dam +* Olaoluwa Osuntokun * Yong Yu * Ziggie diff --git a/zpay32/decode.go b/zpay32/decode.go index 7f1da7cbfc8..b6fd91585ed 100644 --- a/zpay32/decode.go +++ b/zpay32/decode.go @@ -265,7 +265,11 @@ func parseTimestamp(data []byte) (uint64, error) { // parseTaggedFields takes the base32 encoded tagged fields of the invoice, and // fills the Invoice struct accordingly. func parseTaggedFields(invoice *Invoice, fields []byte, net *chaincfg.Params) error { - index := 0 + var ( + index int + paymentHashSeen bool + ) + for len(fields)-index > 0 { // If there are less than 3 groups to read, there cannot be more // interesting information, as we need the type (1 group) and @@ -294,11 +298,10 @@ func parseTaggedFields(invoice *Invoice, fields []byte, net *chaincfg.Params) er switch typ { case fieldTypeP: - if invoice.PaymentHash != nil { - // We skip the field if we have already seen a - // supported one. - continue + if paymentHashSeen { + return ErrDuplicatePaymentHash } + paymentHashSeen = true invoice.PaymentHash, err = parse32Bytes(base32Data) @@ -446,8 +449,14 @@ func parseFieldDataLength(data []byte) (uint16, error) { func parse32Bytes(data []byte) (*[32]byte, error) { var paymentHash [32]byte - // As BOLT-11 states, a reader must skip over the 32-byte fields if - // it does not have a length of 52, so avoid returning an error. + // A field with an unexpected length is reported as absent rather + // than as an error, leaving it to the caller to decide whether a + // missing field is fatal. Note that BOLT 11 is stricter, and + // requires a reader to fail on a fixed-length field (p, h, s, n) + // with the wrong length. For the payment hash the end result is the + // same: a lone wrong-length field leaves PaymentHash nil and + // validateInvoice rejects the invoice, and a wrong-length field + // paired with a valid one is rejected as a duplicate. if len(data) != hashBase32Len { return nil, nil } diff --git a/zpay32/invoice.go b/zpay32/invoice.go index 9c5d86ce2f1..324fb988a09 100644 --- a/zpay32/invoice.go +++ b/zpay32/invoice.go @@ -103,6 +103,12 @@ var ( // ErrBrokenTaggedField is returned when the last tagged field is // incorrectly formatted and doesn't have enough bytes to be read. ErrBrokenTaggedField = errors.New("last tagged field is broken") + + // ErrDuplicatePaymentHash is returned when an invoice contains more + // than one payment hash field. + ErrDuplicatePaymentHash = errors.New( + "invoice contains multiple payment hashes", + ) ) // MessageSigner is passed to the Encode method to provide a signature diff --git a/zpay32/invoice_internal_test.go b/zpay32/invoice_internal_test.go index 22434a99b5a..c158b7947d1 100644 --- a/zpay32/invoice_internal_test.go +++ b/zpay32/invoice_internal_test.go @@ -1,6 +1,7 @@ package zpay32 import ( + "bytes" "encoding/binary" "math" "reflect" @@ -781,6 +782,32 @@ func TestParseTaggedFields(t *testing.T) { netParams := &chaincfg.SimNetParams + var malformedThenValid bytes.Buffer + require.NoError(t, writeTaggedField( + &malformedThenValid, fieldTypeP, []byte{0}, + )) + require.NoError(t, writeBytes32( + &malformedThenValid, fieldTypeP, [32]byte{}, + )) + + var identicalPaymentHashes bytes.Buffer + require.NoError(t, writeBytes32( + &identicalPaymentHashes, fieldTypeP, testPaymentHash, + )) + require.NoError(t, writeBytes32( + &identicalPaymentHashes, fieldTypeP, testPaymentHash, + )) + + var distinctPaymentHashes bytes.Buffer + require.NoError(t, writeBytes32( + &distinctPaymentHashes, fieldTypeP, testPaymentHash, + )) + var secondPaymentHash [32]byte + copy(secondPaymentHash[:], testDescriptionHash[:]) + require.NoError(t, writeBytes32( + &distinctPaymentHashes, fieldTypeP, secondPaymentHash, + )) + tests := []struct { name string data []byte @@ -807,6 +834,21 @@ func TestParseTaggedFields(t *testing.T) { name: "unknown field valid data", data: []byte{0xff, 0x00, 0x01, 0xab}, }, + { + name: "malformed then valid payment hash", + data: malformedThenValid.Bytes(), + wantErr: ErrDuplicatePaymentHash, + }, + { + name: "identical payment hashes", + data: identicalPaymentHashes.Bytes(), + wantErr: ErrDuplicatePaymentHash, + }, + { + name: "distinct payment hashes", + data: distinctPaymentHashes.Bytes(), + wantErr: ErrDuplicatePaymentHash, + }, { name: "only type specified", data: []byte{0x0d}, diff --git a/zpay32/invoice_test.go b/zpay32/invoice_test.go index bfa1539f3ec..dfff0c10e99 100644 --- a/zpay32/invoice_test.go +++ b/zpay32/invoice_test.go @@ -195,6 +195,7 @@ func TestDecodeEncode(t *testing.T) { decodeOpts []DecodeOption skipEncoding bool beforeEncoding func(*Invoice) + wantErr error }{ { encodedInvoice: "asdsaddnasdnas", // no hrp @@ -324,9 +325,18 @@ func TestDecodeEncode(t *testing.T) { skipEncoding: true, // Skip encoding since we don't have the unknown fields to encode. }, { - // Ignore fields with unknown lengths. - encodedInvoice: "lnbc241pveeq09pp5qqqsyqcyq5rqwzqfqqqsyqcyq5rqwzqfqqqsyqcyq5rqwzqfqypqpp3qqqsyqcyq5rqwzqfqqqsyqcyq5rqwzqfqqqsyqcyq5rqwzqfqhp58yjmdan79s6qqdhdzgynm4zwqd5d7xmw5fk98klysy043l2ahrqshp38yjmdan79s6qqdhdzgynm4zwqd5d7xmw5fk98klysy043l2ahnp4q0n326hr8v9zprg8gsvezcch06gfaqqhde2aj730yg0durunfhv66np3q0n326hr8v9zprg8gsvezcch06gfaqqhde2aj730yg0durunfy8huflvs2zwkymx47cszugvzn5v64ahemzzlmm62rpn9l9rm05h35aceq00tkt296289wepws9jh4499wq2l0vk6xcxffd90dpuqchqqztyayq", - valid: true, + // Ignore fields with unknown lengths. The wrong-length + // duplicates of the h and n fields are skipped, while + // the valid p, h, and n fields are used. + encodedInvoice: "lnbc241pveeq09pp5qqqsyqcyq5rqwzqf" + + "qqqsyqcyq5rqwzqfqqqsyqcyq5rqwzqfqypqhp58yjmdan" + + "79s6qqdhdzgynm4zwqd5d7xmw5fk98klysy043l2ahrqshp38yjmd" + + "an79s6qqdhdzgynm4zwqd5d7xmw5fk98klysy043l2ahnp4q0n326hr8v" + + "9zprg8gsvezcch06gfaqqhde2aj730yg0durunfhv66np3q0n326hr8v" + + "9zprg8gsvezcch06gfaqqhde2aj730yg0durunfp3ngd7vju6eywrly" + + "v9vu7l797m4x5yxvvhqd4rm8guqw5389vna986py0hkxen8kmtmte4d" + + "gv439wksk2rh4smnm5w43a0e43lecjvqptnpz74", + valid: true, decodedInvoice: func() *Invoice { return &Invoice{ Net: &chaincfg.MainNetParams, @@ -340,6 +350,21 @@ func TestDecodeEncode(t *testing.T) { }, skipEncoding: true, // Skip encoding since we don't have the unknown fields to encode. }, + { + // Reject a duplicate payment hash even if it has an + // unknown length. + encodedInvoice: "lnbc241pveeq09pp5qqqsyqcyq5rqwzqf" + + "qqqsyqcyq5rqwzqfqqqsyqcyq5rqwzqfqypqpp3qqqsyqcyq5rq" + + "wzqfqqqsyqcyq5rqwzqfqqqsyqcyq5rqwzqfqhp58yjmdan79s6" + + "qqdhdzgynm4zwqd5d7xmw5fk98klysy043l2ahrqshp38yjmdan79" + + "s6qqdhdzgynm4zwqd5d7xmw5fk98klysy043l2ahnp4q0n326hr8v" + + "9zprg8gsvezcch06gfaqqhde2aj730yg0durunfhv66np3q0n326h" + + "r8v9zprg8gsvezcch06gfaqqhde2aj730yg0durunfy8huflvs2z" + + "wkymx47cszugvzn5v64ahemzzlmm62rpn9l9rm05h35aceq00tkt2" + + "96289wepws9jh4499wq2l0vk6xcxffd90dpuqchqqztyayq", + valid: false, + wantErr: ErrDuplicatePaymentHash, + }, { // Invoice with no amount. encodedInvoice: "lnbc1pvjluezpp5qqqsyqcyq5rqwzqfqqqsyqcyq5rqwzqfqqqsyqcyq5rqwzqfqypqdq5xysxxatsyp3k7enxv4jshwlglv23cytkzvq8ld39drs8sq656yh2zn0aevrwu6uqctaklelhtpjnmgjdzmvwsh0kuxuwqf69fjeap9m5mev2qzpp27xfswhs5vgqmn9xzq", @@ -918,6 +943,9 @@ func TestDecodeEncode(t *testing.T) { ) if !test.valid { require.Error(t, err) + if test.wantErr != nil { + require.ErrorIs(t, err, test.wantErr) + } } else { require.NoError(t, err) require.Equal(t, decodedInvoice, invoice) @@ -948,6 +976,42 @@ func TestDecodeEncode(t *testing.T) { } } +// TestDecodeDuplicatePaymentHashes checks that Decode rejects invoices with +// either distinct or identical duplicate payment hash fields. +func TestDecodeDuplicatePaymentHashes(t *testing.T) { + t.Parallel() + + tests := map[string]string{ + "distinct payment hashes": "lnbc1pvjluezpp5qqqsyqcyq5rqwzqf" + + "qqqsyqcyq5rqwzqfqqqsyqcyq5rqwzqfqypqpp5llllll" + + "lllllllllllllllllllllllllllllllllllllllllllllsdpy" + + "v36hqmrfvdshgefqwpshjmt9de6zq6rpwd5qsp5zyg3zyg3" + + "zyg3zyg3zyg3zyg3zyg3zyg3zyg3zyg3zyg3zyg3zygs9g3" + + "f93cqturay6zk2fyfcmeflphlzew9wfq0n5nf9hqnlwxtht" + + "zqcljcuurljyd2vngkya5hndakf33ghly97qm5nc3umj7j" + + "ep22nfsq3nr0w8", + "identical payment hashes": "lnbc1pvjluezpp5qqqsyqcyq5rqwzqf" + + "qqqsyqcyq5rqwzqfqqqsyqcyq5rqwzqfqypqpp5qqqsyq" + + "cyq5rqwzqfqqqsyqcyq5rqwzqfqqqsyqcyq5rqwzqfqyp" + + "qdpyv36hqmrfvdshgefqwpshjmt9de6zq6rpwd5qsp5zyg" + + "3zyg3zyg3zyg3zyg3zyg3zyg3zyg3zyg3zyg3zyg3zyg3z" + + "ygs29wywgsx0wpv9t045f683nj97nnjk55wt0exe3eassl6" + + "smx60nk9hlaae8vhe0hwv25s6fthcwqkxsw2hpjeptxz7x" + + "ujtexa3l8jrkcqyn037r", + } + + for name, encodedInvoice := range tests { + t.Run(name, func(t *testing.T) { + t.Parallel() + + _, err := Decode( + encodedInvoice, &chaincfg.MainNetParams, + ) + require.ErrorIs(t, err, ErrDuplicatePaymentHash) + }) + } +} + // TestNewInvoice tests that providing the optional arguments to the NewInvoice // method creates an Invoice that encodes to the expected string. func TestNewInvoice(t *testing.T) {