diff --git a/docs/release-notes/release-notes-0.20.5.md b/docs/release-notes/release-notes-0.20.5.md index 12d0dd6e52..f956077e39 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 7f1da7cbfc..b6fd91585e 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 9c5d86ce2f..324fb988a0 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 22434a99b5..c158b7947d 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 bfa1539f3e..dfff0c10e9 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) {