Merge pull request #8142 from carlaKC/8137-validateblindedpayload

routing: Add Validation for Decoding Blinded Paths

routing/route/route.go

@@ -38,6 +38,13 @@ var (
 	// ErrAMPMissingMPP is returned when the caller tries to attach an AMP
 	// record but no MPP record is presented for the final hop.
 	ErrAMPMissingMPP = errors.New("cannot send AMP without MPP record")
+
+	// ErrMissingField is returned if a required TLV is missing.
+	ErrMissingField = errors.New("required tlv missing")
+
+	// ErrUnexpectedField is returned if a tlv field is included when it
+	// should not be.
+	ErrUnexpectedField = errors.New("unexpected tlv included")
 )
 
 // Vertex is a simple alias for the serialization of a compressed Bitcoin
@@ -172,11 +179,15 @@ func (h *Hop) Copy() *Hop {
 // PackHopPayload writes to the passed io.Writer, the series of byes that can
 // be placed directly into the per-hop payload (EOB) for this hop. This will
 // include the required routing fields, as well as serializing any of the
-// passed optional TLVRecords.  nextChanID is the unique channel ID that
-// references the _outgoing_ channel ID that follows this hop. This field
-// follows the same semantics as the NextAddress field in the onion: it should
-// be set to zero to indicate the terminal hop.
-func (h *Hop) PackHopPayload(w io.Writer, nextChanID uint64) error {
+// passed optional TLVRecords. nextChanID is the unique channel ID that
+// references the _outgoing_ channel ID that follows this hop. The lastHop bool
+// is used to signal whether this hop is the final hop in a route. Previously,
+// a zero nextChanID would be used for this purpose, but with the addition of
+// blinded routes which allow zero nextChanID values for intermediate hops we
+// add an explicit signal.
+func (h *Hop) PackHopPayload(w io.Writer, nextChanID uint64,
+	finalHop bool) error {
+
 	// If this is a legacy payload, then we'll exit here as this method
 	// shouldn't be called.
 	if h.LegacyPayload == true {
@@ -189,8 +200,28 @@ func (h *Hop) PackHopPayload(w io.Writer, nextChanID uint64) error {
 	var records []tlv.Record
 
 	// Hops that are not part of a blinded path will have an amount and
-	// a CLTV expiry field. Zero values indicate that the hop is inside of
-	// a blinded route, so the TLV should not be included.
+	// a CLTV expiry field. In a blinded route (where encrypted data is
+	// non-nil), these values may be omitted for intermediate nodes.
+	// Validate these fields against the structure of the payload so that
+	// we know they're included (or excluded) correctly.
+	isBlinded := h.EncryptedData != nil
+
+	if err := optionalBlindedField(
+		h.AmtToForward == 0, isBlinded, finalHop,
+	); err != nil {
+		return fmt.Errorf("%w: amount to forward: %v", err,
+			h.AmtToForward)
+	}
+
+	if err := optionalBlindedField(
+		h.OutgoingTimeLock == 0, isBlinded, finalHop,
+	); err != nil {
+		return fmt.Errorf("%w: outgoing timelock: %v", err,
+			h.OutgoingTimeLock)
+	}
+
+	// Once we've validated that these TLVs are set as we expect, we can
+	// go ahead and include them if non-zero.
 	amt := uint64(h.AmtToForward)
 	if amt != 0 {
 		records = append(
@@ -204,10 +235,13 @@ func (h *Hop) PackHopPayload(w io.Writer, nextChanID uint64) error {
 		)
 	}
 
-	// BOLT 04 says the next_hop_id should be omitted for the final hop,
-	// but present for all others.
-	//
-	// TODO(conner): test using hop.Exit once available
+	// Validate channel TLV is present as expected based on location in
+	// route and whether this hop is blinded.
+	err := validateNextChanID(nextChanID != 0, isBlinded, finalHop)
+	if err != nil {
+		return fmt.Errorf("%w: channel id: %v", err, nextChanID)
+	}
+
 	if nextChanID != 0 {
 		records = append(records,
 			record.NewNextHopIDRecord(&nextChanID),
@@ -218,7 +252,7 @@ func (h *Hop) PackHopPayload(w io.Writer, nextChanID uint64) error {
 	// attach it to the final hop. Otherwise the route was constructed
 	// incorrectly.
 	if h.MPP != nil {
-		if nextChanID == 0 {
+		if finalHop {
 			records = append(records, h.MPP.Record())
 		} else {
 			return ErrIntermediateMPPHop
@@ -280,6 +314,62 @@ func (h *Hop) PackHopPayload(w io.Writer, nextChanID uint64) error {
 	return tlvStream.Encode(w)
 }
 
+// optionalBlindedField validates fields that we expect to be non-zero for all
+// hops in a regular route, but may be zero for intermediate nodes in a blinded
+// route. It will validate the following cases:
+// - Not blinded: require non-zero values.
+// - Intermediate blinded node: require zero values.
+// - Final blinded node: require non-zero values.
+func optionalBlindedField(isZero, blindedHop, finalHop bool) error {
+	switch {
+	// We are not in a blinded route and the TLV is not set when it should
+	// be.
+	case !blindedHop && isZero:
+		return ErrMissingField
+
+	// We are not in a blinded route and the TLV is set as expected.
+	case !blindedHop:
+		return nil
+
+	// In a blinded route the final hop is expected to have TLV values set.
+	case finalHop && isZero:
+		return ErrMissingField
+
+	// In an intermediate hop in a blinded route and the field is not zero.
+	case !finalHop && !isZero:
+		return ErrUnexpectedField
+	}
+
+	return nil
+}
+
+// validateNextChanID validates the presence of the nextChanID TLV field in
+// a payload. For regular payments, it is expected to be present for all hops
+// except the final hop. For blinded paths, it is not expected to be included
+// at all (as this value is provided in encrypted data).
+func validateNextChanID(nextChanIDIsSet, isBlinded, finalHop bool) error {
+	switch {
+	// Hops in a blinded route should not have a next channel ID set.
+	case isBlinded && nextChanIDIsSet:
+		return ErrUnexpectedField
+
+	// Otherwise, blinded hops are allowed to have a zero value.
+	case isBlinded:
+		return nil
+
+	// The final hop in a regular route is expected to have a zero value.
+	case finalHop && nextChanIDIsSet:
+		return ErrUnexpectedField
+
+	// Intermediate hops in regular routes require non-zero value.
+	case !finalHop && !nextChanIDIsSet:
+		return ErrMissingField
+
+	default:
+		return nil
+	}
+}
+
 // Size returns the total size this hop's payload would take up in the onion
 // packet.
 func (h *Hop) PayloadSize(nextChanID uint64) uint64 {
@@ -559,7 +649,8 @@ func (r *Route) ToSphinxPath() (*sphinx.PaymentPath, error) {
 
 		// If we aren't on the last hop, then we set the "next address"
 		// field to be the channel that directly follows it.
-		if i != len(r.Hops)-1 {
+		finalHop := i == len(r.Hops)-1
+		if !finalHop {
 			nextHop = r.Hops[i+1].ChannelID
 		}
 
@@ -591,7 +682,7 @@ func (r *Route) ToSphinxPath() (*sphinx.PaymentPath, error) {
 			// channel should be forwarded to so we can construct a
 			// valid payload.
 			var b bytes.Buffer
-			err := hop.PackHopPayload(&b, nextHop)
+			err := hop.PackHopPayload(&b, nextHop, finalHop)
 			if err != nil {
 				return nil, err
 			}

routing/route/route_test.go

@@ -105,7 +105,7 @@ func TestMPPHop(t *testing.T) {
 	// Encoding an MPP record to an intermediate hop should result in a
 	// failure.
 	var b bytes.Buffer
-	err := hop.PackHopPayload(&b, 2)
+	err := hop.PackHopPayload(&b, 2, false)
 	if err != ErrIntermediateMPPHop {
 		t.Fatalf("expected err: %v, got: %v",
 			ErrIntermediateMPPHop, err)
@@ -113,7 +113,7 @@ func TestMPPHop(t *testing.T) {
 
 	// Encoding an MPP record to a final hop should be successful.
 	b.Reset()
-	err = hop.PackHopPayload(&b, 0)
+	err = hop.PackHopPayload(&b, 0, true)
 	if err != nil {
 		t.Fatalf("expected err: %v, got: %v", nil, err)
 	}
@@ -135,7 +135,7 @@ func TestAMPHop(t *testing.T) {
 	// Encoding an AMP record to an intermediate hop w/o an MPP record
 	// should result in a failure.
 	var b bytes.Buffer
-	err := hop.PackHopPayload(&b, 2)
+	err := hop.PackHopPayload(&b, 2, false)
 	if err != ErrAMPMissingMPP {
 		t.Fatalf("expected err: %v, got: %v",
 			ErrAMPMissingMPP, err)
@@ -144,7 +144,7 @@ func TestAMPHop(t *testing.T) {
 	// Encoding an AMP record to a final hop w/o an MPP record should result
 	// in a failure.
 	b.Reset()
-	err = hop.PackHopPayload(&b, 0)
+	err = hop.PackHopPayload(&b, 0, true)
 	if err != ErrAMPMissingMPP {
 		t.Fatalf("expected err: %v, got: %v",
 			ErrAMPMissingMPP, err)
@@ -154,24 +154,118 @@ func TestAMPHop(t *testing.T) {
 	// successful.
 	hop.MPP = record.NewMPP(testAmt, testAddr)
 	b.Reset()
-	err = hop.PackHopPayload(&b, 0)
+	err = hop.PackHopPayload(&b, 0, true)
 	if err != nil {
 		t.Fatalf("expected err: %v, got: %v", nil, err)
 	}
 }
 
-// TestNoForwardingParams tests packing of a hop payload without an amount or
-// expiry height.
-func TestNoForwardingParams(t *testing.T) {
+// TestBlindedHops tests packing of a hop payload for various types of hops in
+// a blinded route.
+func TestBlindedHops(t *testing.T) {
 	t.Parallel()
 
-	hop := Hop{
-		EncryptedData: []byte{1, 2, 3},
+	tests := []struct {
+		name        string
+		hop         Hop
+		nextChannel uint64
+		isFinal     bool
+		err         error
+	}{
+		{
+			name: "introduction point with next channel",
+			hop: Hop{
+				EncryptedData: []byte{1, 2, 3},
+				BlindingPoint: testPubKey,
+			},
+			nextChannel: 1,
+			isFinal:     false,
+			err:         ErrUnexpectedField,
+		},
+		{
+			name: "final node with next channel",
+			hop: Hop{
+				EncryptedData:    []byte{1, 2, 3},
+				AmtToForward:     150,
+				OutgoingTimeLock: 26,
+			},
+			nextChannel: 1,
+			isFinal:     true,
+			err:         ErrUnexpectedField,
+		},
+		{
+			name: "valid introduction point",
+			hop: Hop{
+				EncryptedData: []byte{1, 2, 3},
+				BlindingPoint: testPubKey,
+			},
+			nextChannel: 0,
+			isFinal:     false,
+		},
+		{
+			name: "valid intermediate blinding",
+			hop: Hop{
+				EncryptedData: []byte{1, 2, 3},
+			},
+			nextChannel: 0,
+			isFinal:     false,
+		},
+		{
+			name: "final blinded missing amount",
+			hop: Hop{
+				EncryptedData: []byte{1, 2, 3},
+			},
+			nextChannel: 0,
+			isFinal:     true,
+			err:         ErrMissingField,
+		},
+		{
+			name: "final blinded expiry missing",
+			hop: Hop{
+				EncryptedData: []byte{1, 2, 3},
+				AmtToForward:  100,
+			},
+			nextChannel: 0,
+			isFinal:     true,
+			err:         ErrMissingField,
+		},
+		{
+			name: "valid final blinded",
+			hop: Hop{
+				EncryptedData:    []byte{1, 2, 3},
+				AmtToForward:     100,
+				OutgoingTimeLock: 52,
+			},
+			nextChannel: 0,
+			isFinal:     true,
+		},
+		{
+			// The introduction node can also be the final hop.
+			name: "valid final intro blinded",
+			hop: Hop{
+				EncryptedData:    []byte{1, 2, 3},
+				BlindingPoint:    testPubKey,
+				AmtToForward:     100,
+				OutgoingTimeLock: 52,
+			},
+			nextChannel: 0,
+			isFinal:     true,
+		},
 	}
 
-	var b bytes.Buffer
-	err := hop.PackHopPayload(&b, 2)
-	require.NoError(t, err)
+	for _, testCase := range tests {
+		testCase := testCase
+
+		t.Run(testCase.name, func(t *testing.T) {
+			t.Parallel()
+
+			var b bytes.Buffer
+			err := testCase.hop.PackHopPayload(
+				&b, testCase.nextChannel, testCase.isFinal,
+			)
+			require.ErrorIs(t, err, testCase.err)
+		})
+	}
 }
 
 // TestPayloadSize tests the payload size calculation that is provided by Hop
@@ -182,21 +276,22 @@ func TestPayloadSize(t *testing.T) {
 	hops := []*Hop{
 		{
 			PubKeyBytes:      testPubKeyBytes,
-			AmtToForward:     1000,
+			AmtToForward:     2000,
 			OutgoingTimeLock: 600000,
 			ChannelID:        3432483437438,
 			LegacyPayload:    true,
 		},
 		{
 			PubKeyBytes:      testPubKeyBytes,
-			AmtToForward:     1200,
+			AmtToForward:     1500,
 			OutgoingTimeLock: 700000,
 			ChannelID:        63584534844,
 		},
 		{
 			PubKeyBytes:      testPubKeyBytes,
-			AmtToForward:     1200,
+			AmtToForward:     1000,
 			OutgoingTimeLock: 700000,
+			ChannelID:        51784534844,
 			MPP:              record.NewMPP(500, [32]byte{}),
 			AMP: record.NewAMP(
 				[32]byte{}, [32]byte{}, 8,

routing/router_test.go

@@ -3471,10 +3471,11 @@ func TestSendToRouteSkipTempErrSuccess(t *testing.T) {
 	// Create a simple 1-hop route.
 	hops := []*route.Hop{
 		{
-			ChannelID:    1,
-			PubKeyBytes:  node.PubKeyBytes,
-			AmtToForward: payAmt,
-			MPP:          record.NewMPP(payAmt, [32]byte{}),
+			ChannelID:        1,
+			PubKeyBytes:      node.PubKeyBytes,
+			AmtToForward:     payAmt,
+			OutgoingTimeLock: 120,
+			MPP:              record.NewMPP(payAmt, [32]byte{}),
 		},
 	}
 	rt, err := route.NewRouteFromHops(payAmt, 100, node.PubKeyBytes, hops)
@@ -3607,10 +3608,11 @@ func TestSendToRouteSkipTempErrTempFailure(t *testing.T) {
 	// Create a simple 1-hop route.
 	hops := []*route.Hop{
 		{
-			ChannelID:    1,
-			PubKeyBytes:  node.PubKeyBytes,
-			AmtToForward: payAmt,
-			MPP:          record.NewMPP(payAmt, [32]byte{}),
+			ChannelID:        1,
+			PubKeyBytes:      node.PubKeyBytes,
+			AmtToForward:     payAmt,
+			OutgoingTimeLock: 120,
+			MPP:              record.NewMPP(payAmt, [32]byte{}),
 		},
 	}
 	rt, err := route.NewRouteFromHops(payAmt, 100, node.PubKeyBytes, hops)
@@ -3688,10 +3690,11 @@ func TestSendToRouteSkipTempErrPermanentFailure(t *testing.T) {
 	// Create a simple 1-hop route.
 	hops := []*route.Hop{
 		{
-			ChannelID:    1,
-			PubKeyBytes:  node.PubKeyBytes,
-			AmtToForward: payAmt,
-			MPP:          record.NewMPP(payAmt, [32]byte{}),
+			ChannelID:        1,
+			PubKeyBytes:      node.PubKeyBytes,
+			AmtToForward:     payAmt,
+			OutgoingTimeLock: 120,
+			MPP:              record.NewMPP(payAmt, [32]byte{}),
 		},
 	}
 	rt, err := route.NewRouteFromHops(payAmt, 100, node.PubKeyBytes, hops)
@@ -3773,10 +3776,11 @@ func TestSendToRouteTempFailure(t *testing.T) {
 	// Create a simple 1-hop route.
 	hops := []*route.Hop{
 		{
-			ChannelID:    1,
-			PubKeyBytes:  node.PubKeyBytes,
-			AmtToForward: payAmt,
-			MPP:          record.NewMPP(payAmt, [32]byte{}),
+			ChannelID:        1,
+			PubKeyBytes:      node.PubKeyBytes,
+			AmtToForward:     payAmt,
+			OutgoingTimeLock: 120,
+			MPP:              record.NewMPP(payAmt, [32]byte{}),
 		},
 	}
 	rt, err := route.NewRouteFromHops(payAmt, 100, node.PubKeyBytes, hops)