Merge pull request #3767 from matheusdtech/fix-zpay32

zpay32: Fix broken last tagged field

zpay32/invoice.go

@@ -86,6 +86,14 @@ var (
 
 	// ErrInvoiceTooLarge is returned when an invoice exceeds maxInvoiceLength.
 	ErrInvoiceTooLarge = errors.New("invoice is too large")
+
+	// ErrInvalidFieldLength is returned when a tagged field was specified
+	// with a length larger than the left over bytes of the data field.
+	ErrInvalidFieldLength = errors.New("invalid field length")
+
+	// 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")
 )
 
 // MessageSigner is passed to the Encode method to provide a signature
@@ -600,12 +608,14 @@ func parseTimestamp(data []byte) (uint64, error) {
 // fills the Invoice struct accordingly.
 func parseTaggedFields(invoice *Invoice, fields []byte, net *chaincfg.Params) error {
 	index := 0
-	for {
+	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
 		// length (2 groups).
+		//
+		// This means the last tagged field is broken.
 		if len(fields)-index < 3 {
-			break
+			return ErrBrokenTaggedField
 		}
 
 		typ := fields[index]
@@ -617,7 +627,7 @@ func parseTaggedFields(invoice *Invoice, fields []byte, net *chaincfg.Params) er
 		// If we don't have enough field data left to read this length,
 		// return error.
 		if len(fields) < index+3+int(dataLength) {
-			return fmt.Errorf("invalid field length")
+			return ErrInvalidFieldLength
 		}
 		base32Data := fields[index+3 : index+3+int(dataLength)]
 

zpay32/invoice_internal_test.go

@@ -777,3 +777,75 @@ func TestParseRouteHint(t *testing.T) {
 		}
 	}
 }
+
+// TestParseTaggedFields checks that tagged field data is correctly parsed or
+// errors as expected.
+func TestParseTaggedFields(t *testing.T) {
+	t.Parallel()
+
+	netParams := &chaincfg.SimNetParams
+
+	tests := []struct {
+		name    string
+		data    []byte
+		wantErr error
+	}{
+		{
+			name: "nil data",
+			data: nil,
+		},
+		{
+			name: "empty data",
+			data: []byte{},
+		},
+		{
+			// Type 0xff cannot be encoded in a single 5-bit
+			// element, so it's technically invalid but
+			// parseTaggedFields doesn't error on non-5bpp
+			// compatible codes so we can use a code in tests which
+			// will never become known in the future.
+			name: "valid unknown field",
+			data: []byte{0xff, 0x00, 0x00},
+		},
+		{
+			name: "unknown field valid data",
+			data: []byte{0xff, 0x00, 0x01, 0xab},
+		},
+		{
+			name:    "only type specified",
+			data:    []byte{0x0d},
+			wantErr: ErrBrokenTaggedField,
+		},
+		{
+			name:    "not enough bytes for len",
+			data:    []byte{0x0d, 0x00},
+			wantErr: ErrBrokenTaggedField,
+		},
+		{
+			name:    "no bytes after len",
+			data:    []byte{0x0d, 0x00, 0x01},
+			wantErr: ErrInvalidFieldLength,
+		},
+		{
+			name:    "not enough bytes after len",
+			data:    []byte{0x0d, 0x00, 0x02, 0x01},
+			wantErr: ErrInvalidFieldLength,
+		},
+		{
+			name:    "not enough bytes after len with unknown type",
+			data:    []byte{0xff, 0x00, 0x02, 0x01},
+			wantErr: ErrInvalidFieldLength,
+		},
+	}
+	for _, tc := range tests {
+		tc := tc // pin
+		t.Run(tc.name, func(t *testing.T) {
+			var invoice Invoice
+			gotErr := parseTaggedFields(&invoice, tc.data, netParams)
+			if tc.wantErr != gotErr {
+				t.Fatalf("Unexpected error. want=%v got=%v",
+					tc.wantErr, gotErr)
+			}
+		})
+	}
+}

zpay32/invoice_test.go

@@ -8,6 +8,7 @@ import (
 	"encoding/hex"
 	"fmt"
 	"reflect"
+	"strings"
 	"testing"
 	"time"
 
@@ -843,6 +844,56 @@ func TestMaxInvoiceLength(t *testing.T) {
 	}
 }
 
+// TestInvoiceChecksumMalleability ensures that the malleability of the
+// checksum in bech32 strings cannot cause a signature to become valid and
+// therefore cause a wrong destination to be decoded for invoices where the
+// destination is extracted from the signature.
+func TestInvoiceChecksumMalleability(t *testing.T) {
+	privKeyHex := "a50f3bdf9b6c4b1fdd7c51a8bbf4b5855cf381f413545ed155c0282f4412a1b1"
+	privKeyBytes, _ := hex.DecodeString(privKeyHex)
+	chain := &chaincfg.SimNetParams
+	var payHash [32]byte
+	ts := time.Unix(0, 0)
+
+	privKey, _ := btcec.PrivKeyFromBytes(btcec.S256(), privKeyBytes)
+	msgSigner := MessageSigner{
+		SignCompact: func(hash []byte) ([]byte, error) {
+			return btcec.SignCompact(btcec.S256(), privKey, hash, true)
+		},
+	}
+	opts := []func(*Invoice){Description("test")}
+	invoice, err := NewInvoice(chain, payHash, ts, opts...)
+	if err != nil {
+		t.Fatal(err)
+	}
+
+	encoded, err := invoice.Encode(msgSigner)
+	if err != nil {
+		t.Fatal(err)
+	}
+
+	// Changing a bech32 string which checksum ends in "p" to "(q*)p" can
+	// cause the checksum to return as a valid bech32 string _but_ the
+	// signature field immediately preceding it would be mutaded.  In rare
+	// cases (about 3%) it is still seen as a valid signature and public
+	// key recovery causes a different node than the originally intended
+	// one to be derived.
+	//
+	// We thus modify the checksum here and verify the invoice gets broken
+	// enough that it fails to decode.
+	if !strings.HasSuffix(encoded, "p") {
+		t.Logf("Invoice: %s", encoded)
+		t.Fatalf("Generated invoice checksum does not end in 'p'")
+	}
+	encoded = encoded[:len(encoded)-1] + "qp"
+
+	_, err = Decode(encoded, chain)
+	if err == nil {
+		t.Fatalf("Did not get expected error when decoding invoice")
+	}
+
+}
+
 func compareInvoices(expected, actual *Invoice) error {
 	if !reflect.DeepEqual(expected.Net, actual.Net) {
 		return fmt.Errorf("expected net %v, got %v",