[v0.20.x-branch] Backport #11190: zpay32: reject duplicate payment hash fields

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

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
 	}

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

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},

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) {