Merge pull request #9993 from MPins/issue-9904-9915

Validate UTF-8 description and empty route hints when parsing BOLT-11 invoices

Author: ellemouton

docs/release-notes/release-notes-0.20.0.md

@@ -29,6 +29,10 @@
 - Fixed [shutdown deadlock](https://github.com/lightningnetwork/lnd/pull/10042)
   when we fail starting up LND before we startup the chanbackup sub-server.
 
+- Fixed BOLT-11 invoice parsing behavior: [now errors](
+  https://github.com/lightningnetwork/lnd/pull/9993) are returned when receiving
+  empty route hints or a non-UTF-8-encoded description.
+
 - [Fixed](https://github.com/lightningnetwork/lnd/pull/10027) an issue where
   known TLV fields were incorrectly encoded into the `ExtraData` field of
   messages in the dynamic commitment set.

zpay32/decode.go

@@ -7,6 +7,7 @@ import (
 	"fmt"
 	"strings"
 	"time"
+	"unicode/utf8"
 
 	"github.com/btcsuite/btcd/btcec/v2"
 	"github.com/btcsuite/btcd/btcec/v2/ecdsa"
@@ -18,6 +19,21 @@ import (
 	"github.com/lightningnetwork/lnd/lnwire"
 )
 
+var (
+	// ErrInvalidUTF8Description is returned if the invoice description is
+	// not valid UTF-8.
+	ErrInvalidUTF8Description = errors.New("description is not valid UTF-8")
+
+	// ErrLengthNotMultipleOfHopHintLength is returned if the length of the
+	// route hint data is not a multiple of the hop hint length.
+	ErrLengthNotMultipleOfHopHint = errors.New("length is not a multiple " +
+		"of hop hint length")
+
+	// ErrEmptyRouteHint is returned if the route hint field contains no hop
+	// data.
+	ErrEmptyRouteHint = errors.New("route hint field contains no hop data")
+)
+
 // DecodeOption is a type that can be used to supply functional options to the
 // Decode function.
 type DecodeOption func(*decodeOptions)
@@ -446,6 +462,10 @@ func parseDescription(data []byte) (*string, error) {
 		return nil, err
 	}
 
+	if !utf8.Valid(base256Data) {
+		return nil, ErrInvalidUTF8Description
+	}
+
 	description := string(base256Data)
 
 	return &description, nil
@@ -565,8 +585,12 @@ func parseRouteHint(data []byte) ([]HopHint, error) {
 
 	// Check that base256Data is a multiple of hopHintLen.
 	if len(base256Data)%hopHintLen != 0 {
-		return nil, fmt.Errorf("expected length multiple of %d bytes, "+
-			"got %d", hopHintLen, len(base256Data))
+		return nil, ErrLengthNotMultipleOfHopHint
+	}
+
+	// Check for empty route hint
+	if len(base256Data) == 0 {
+		return nil, ErrEmptyRouteHint
 	}
 
 	var routeHint []HopHint

zpay32/invoice_internal_test.go

@@ -369,14 +369,25 @@ func TestParse32Bytes(t *testing.T) {
 func TestParseDescription(t *testing.T) {
 	t.Parallel()
 
+	testNonUTF8StrData, _ := bech32.ConvertBits(
+		[]byte(testNonUTF8Str), 8, 5, true,
+	)
+
 	testCupOfCoffeeData, _ := bech32.ConvertBits([]byte(testCupOfCoffee), 8, 5, true)
 	testPleaseConsiderData, _ := bech32.ConvertBits([]byte(testPleaseConsider), 8, 5, true)
 
 	tests := []struct {
-		data   []byte
-		valid  bool
-		result *string
+		data        []byte
+		valid       bool
+		result      *string
+		expectedErr error
 	}{
+		{
+			data:        testNonUTF8StrData,
+			valid:       false,
+			expectedErr: ErrInvalidUTF8Description,
+			result:      nil,
+		},
 		{
 			data:   []byte{},
 			valid:  true,
@@ -400,6 +411,7 @@ func TestParseDescription(t *testing.T) {
 			t.Errorf("description decoding test %d failed: %v", i, err)
 			return
 		}
+		require.ErrorIs(t, err, test.expectedErr)
 		if test.valid && !reflect.DeepEqual(description, test.result) {
 			t.Fatalf("test %d failed decoding description: "+
 				"expected \"%s\", got \"%s\"",
@@ -685,27 +697,34 @@ func TestParseRouteHint(t *testing.T) {
 	testDoubleHopData, _ = bech32.ConvertBits(testDoubleHopData, 8, 5, true)
 
 	tests := []struct {
-		data   []byte
-		valid  bool
-		result []HopHint
+		data        []byte
+		valid       bool
+		result      []HopHint
+		expectedErr error
 	}{
 		{
-			data:  []byte{0x0, 0x0, 0x0, 0x0},
-			valid: false, // data too short, not multiple of 51 bytes
+			data: []byte{0x0, 0x0, 0x0, 0x0},
+			// data too short, not multiple of 51 bytes
+			valid:       false,
+			expectedErr: ErrLengthNotMultipleOfHopHint,
 		},
 		{
-			data:   []byte{},
-			valid:  true,
-			result: []HopHint{},
+			data:        []byte{},
+			valid:       false,
+			result:      []HopHint{},
+			expectedErr: ErrEmptyRouteHint,
 		},
 		{
 			data:   testSingleHopData,
 			valid:  true,
 			result: testSingleHop,
 		},
 		{
-			data:  append(testSingleHopData, 0x0),
-			valid: false, // data too long, not multiple of 51 bytes
+			data: append(testSingleHopData,
+				[]byte{0x0, 0x0}...),
+			// data too long, not multiple of 51 bytes
+			valid:       false,
+			expectedErr: ErrLengthNotMultipleOfHopHint,
 		},
 		{
 			data:   testDoubleHopData,
@@ -720,6 +739,7 @@ func TestParseRouteHint(t *testing.T) {
 			t.Errorf("routing info decoding test %d failed: %v", i, err)
 			return
 		}
+		require.ErrorIs(t, err, test.expectedErr)
 		if test.valid {
 			if err := compareRouteHints(test.result, routeHint); err != nil {
 				t.Fatalf("test %d failed decoding routing info: %v", i, err)

zpay32/invoice_test.go

@@ -50,6 +50,7 @@ var (
 		0x11, 0x11, 0x11, 0x11, 0x11, 0x11, 0x11, 0x11,
 	}
 
+	testNonUTF8Str      = "1 cup coffee\xff\xfe\xfd"
 	testEmptyString     = ""
 	testCupOfCoffee     = "1 cup coffee"
 	testCoffeeBeans     = "coffee beans"