From 85f9c03797344d8bab5b8bc448e9ee3518d17766 Mon Sep 17 00:00:00 2001 From: Matheus Degiovani Date: Tue, 26 Nov 2019 13:13:06 -0300 Subject: [PATCH 1/3] zpay32: Switch to ErrInvalidFieldLength sentinel This switches the applicable error to use an exported sentinel error so that it is more testable. --- zpay32/invoice.go | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/zpay32/invoice.go b/zpay32/invoice.go index d36b31ca..9a9fde33 100644 --- a/zpay32/invoice.go +++ b/zpay32/invoice.go @@ -86,6 +86,10 @@ 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") ) // MessageSigner is passed to the Encode method to provide a signature @@ -617,7 +621,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)] From 409cf5565511ee9600b74f9966ed4e8e1762dab4 Mon Sep 17 00:00:00 2001 From: Matheus Degiovani Date: Mon, 25 Nov 2019 18:52:31 -0300 Subject: [PATCH 2/3] zpay32: Fix broken last tagged field This fixes an issue where the last tagged field of an invoice could get broken due to the malleability of bech32 checksums. The addition of a specific character in the second to last position of the checksum could cause the previous signature field to mutate and thus point to a different public node. --- zpay32/invoice.go | 10 ++++++++-- 1 file changed, 8 insertions(+), 2 deletions(-) diff --git a/zpay32/invoice.go b/zpay32/invoice.go index 9a9fde33..83370737 100644 --- a/zpay32/invoice.go +++ b/zpay32/invoice.go @@ -90,6 +90,10 @@ var ( // 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 @@ -604,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] From cf6ae06b301da6527c441adfdbe34d358db73526 Mon Sep 17 00:00:00 2001 From: Matheus Degiovani Date: Mon, 25 Nov 2019 17:25:12 -0300 Subject: [PATCH 3/3] zpay32: Add tests of checksum malleability This adds tests for checksum malleability issue of bech32 strings as used by LN invoices. --- zpay32/invoice_internal_test.go | 72 +++++++++++++++++++++++++++++++++ zpay32/invoice_test.go | 51 +++++++++++++++++++++++ 2 files changed, 123 insertions(+) diff --git a/zpay32/invoice_internal_test.go b/zpay32/invoice_internal_test.go index 5d0c2cf1..8089c69d 100644 --- a/zpay32/invoice_internal_test.go +++ b/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) + } + }) + } +} diff --git a/zpay32/invoice_test.go b/zpay32/invoice_test.go index 565c3fcf..eec242f7 100644 --- a/zpay32/invoice_test.go +++ b/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",