routing: Add Validation for Decoding Blinded Paths

docs/release-notes/release-notes-0.18.0.md

@@ -90,6 +90,10 @@
   primitive type of the tlv package.
 
 ## Misc
+* [Added](https://github.com/lightningnetwork/lnd/pull/8142) full validation 
+  for blinded path payloads to allow fuzzing before LND fully supports 
+  blinded payment relay.
+
 ### Logging
 * [Add the htlc amount](https://github.com/lightningnetwork/lnd/pull/8156) to
   contract court logs in case of timed-out htlcs in order to easily spot dust

htlcswitch/hop/fuzz_test.go

@@ -92,33 +92,58 @@ func hopFromPayload(p *Payload) (*route.Hop, uint64) {
 	}, p.FwdInfo.NextHop.ToUint64()
 }
 
-func FuzzPayload(f *testing.F) {
+// FuzzPayloadFinal fuzzes final hop payloads, providing the additional context
+// that the hop should be final (which is usually obtained by the structure
+// of the sphinx packet).
+func FuzzPayloadFinal(f *testing.F) {
+	fuzzPayload(f, true)
+}
+
+// FuzzPayloadIntermediate fuzzes intermediate hop payloads, providing the
+// additional context that a hop should be intermediate (which is usually
+// obtained by the structure of the sphinx packet).
+func FuzzPayloadIntermediate(f *testing.F) {
+	fuzzPayload(f, false)
+}
+
+func fuzzPayload(f *testing.F, finalPayload bool) {
 	f.Fuzz(func(t *testing.T, data []byte) {
 		if len(data) > sphinx.MaxPayloadSize {
 			return
 		}
 
 		r := bytes.NewReader(data)
 
-		payload1, err := NewPayloadFromReader(r)
+		payload1, err := NewPayloadFromReader(r, finalPayload)
 		if err != nil {
 			return
 		}
 
 		var b bytes.Buffer
 		hop, nextChanID := hopFromPayload(payload1)
-		err = hop.PackHopPayload(&b, nextChanID)
-		if errors.Is(err, route.ErrAMPMissingMPP) {
-			// PackHopPayload refuses to encode an AMP record
-			// without an MPP record. However, NewPayloadFromReader
-			// does allow decoding an AMP record without an MPP
-			// record, since validation is done at a later stage. Do
-			// not report a bug for this case.
+		err = hop.PackHopPayload(&b, nextChanID, finalPayload)
+		switch {
+		// PackHopPayload refuses to encode an AMP record
+		// without an MPP record. However, NewPayloadFromReader
+		// does allow decoding an AMP record without an MPP
+		// record, since validation is done at a later stage. Do
+		// not report a bug for this case.
+		case errors.Is(err, route.ErrAMPMissingMPP):
 			return
+
+		// PackHopPayload will not encode regular payloads or final
+		// hops in blinded routes that do not have an amount or expiry
+		// TLV set. However, NewPayloadFromReader will allow creation
+		// of payloads where these TLVs are present, but they have
+		// zero values because validation is done at a later stage.
+		case errors.Is(err, route.ErrMissingField):
+			return
+
+		default:
+			require.NoError(t, err)
 		}
-		require.NoError(t, err)
 
-		payload2, err := NewPayloadFromReader(&b)
+		payload2, err := NewPayloadFromReader(&b, finalPayload)
 		require.NoError(t, err)
 
 		require.Equal(t, payload1, payload2)

htlcswitch/hop/iterator.go

@@ -90,9 +90,10 @@ func (r *sphinxHopIterator) HopPayload() (*Payload, error) {
 	// Otherwise, if this is the TLV payload, then we'll make a new stream
 	// to decode only what we need to make routing decisions.
 	case sphinx.PayloadTLV:
-		return NewPayloadFromReader(bytes.NewReader(
-			r.processedPacket.Payload.Payload,
-		))
+		return NewPayloadFromReader(
+			bytes.NewReader(r.processedPacket.Payload.Payload),
+			r.processedPacket.Action == sphinx.ExitNode,
+		)
 
 	default:
 		return nil, fmt.Errorf("unknown sphinx payload type: %v",

htlcswitch/hop/iterator_test.go

@@ -65,14 +65,16 @@ func TestSphinxHopIteratorForwardingInstructions(t *testing.T) {
 			},
 			expectedFwdInfo: expectedFwdInfo,
 		},
-		// A TLV payload, we can leave off the action as we'll always
-		// read the cid encoded.
+		// A TLV payload, which includes the sphinx action as
+		// cid may be zero for blinded routes (thus we require the
+		// action to signal whether we are at the final hop).
 		{
 			sphinxPacket: &sphinx.ProcessedPacket{
 				Payload: sphinx.HopPayload{
 					Type:    sphinx.PayloadTLV,
 					Payload: b.Bytes(),
 				},
+				Action: sphinx.MoreHops,
 			},
 			expectedFwdInfo: expectedFwdInfo,
 		},

htlcswitch/hop/payload.go

@@ -127,8 +127,10 @@ func NewLegacyPayload(f *sphinx.HopData) *Payload {
 }
 
 // NewPayloadFromReader builds a new Hop from the passed io.Reader. The reader
-// should correspond to the bytes encapsulated in a TLV onion payload.
-func NewPayloadFromReader(r io.Reader) (*Payload, error) {
+// should correspond to the bytes encapsulated in a TLV onion payload. The
+// final hop bool signals that this payload was the final packet parsed by
+// sphinx.
+func NewPayloadFromReader(r io.Reader, finalHop bool) (*Payload, error) {
 	var (
 		cid           uint64
 		amt           uint64
@@ -165,8 +167,7 @@ func NewPayloadFromReader(r io.Reader) (*Payload, error) {
 
 	// Validate whether the sender properly included or omitted tlv records
 	// in accordance with BOLT 04.
-	nextHop := lnwire.NewShortChanIDFromInt(cid)
-	err = ValidateParsedPayloadTypes(parsedTypes, nextHop)
+	err = ValidateParsedPayloadTypes(parsedTypes, finalHop)
 	if err != nil {
 		return nil, err
 	}
@@ -177,7 +178,7 @@ func NewPayloadFromReader(r io.Reader) (*Payload, error) {
 		return nil, ErrInvalidPayload{
 			Type:      *violatingType,
 			Violation: RequiredViolation,
-			FinalHop:  nextHop == Exit,
+			FinalHop:  finalHop,
 		}
 	}
 
@@ -210,7 +211,7 @@ func NewPayloadFromReader(r io.Reader) (*Payload, error) {
 
 	return &Payload{
 		FwdInfo: ForwardingInfo{
-			NextHop:         nextHop,
+			NextHop:         lnwire.NewShortChanIDFromInt(cid),
 			AmountToForward: lnwire.MilliSatoshi(amt),
 			OutgoingCTLV:    cltv,
 		},
@@ -248,42 +249,71 @@ func NewCustomRecords(parsedTypes tlv.TypeMap) record.CustomSet {
 // boolean should be true if the payload was parsed for an exit hop. The
 // requirements for this method are described in BOLT 04.
 func ValidateParsedPayloadTypes(parsedTypes tlv.TypeMap,
-	nextHop lnwire.ShortChannelID) error {
-
-	isFinalHop := nextHop == Exit
+	isFinalHop bool) error {
 
 	_, hasAmt := parsedTypes[record.AmtOnionType]
 	_, hasLockTime := parsedTypes[record.LockTimeOnionType]
 	_, hasNextHop := parsedTypes[record.NextHopOnionType]
 	_, hasMPP := parsedTypes[record.MPPOnionType]
 	_, hasAMP := parsedTypes[record.AMPOnionType]
+	_, hasEncryptedData := parsedTypes[record.EncryptedDataOnionType]
 
-	switch {
+	// All cleartext hops (including final hop) and the final hop in a
+	// blinded path require the forwading amount and expiry TLVs to be set.
+	needFwdInfo := isFinalHop || !hasEncryptedData
+
+	// No blinded hops should have a next hop specified, and only the final
+	// hop in a cleartext route should exclude it.
+	needNextHop := !(hasEncryptedData || isFinalHop)
 
-	// All hops must include an amount to forward.
-	case !hasAmt:
+	switch {
+	// Hops that need forwarding info must include an amount to forward.
+	case needFwdInfo && !hasAmt:
 		return ErrInvalidPayload{
 			Type:      record.AmtOnionType,
 			Violation: OmittedViolation,
 			FinalHop:  isFinalHop,
 		}
 
-	// All hops must include a cltv expiry.
-	case !hasLockTime:
+	// Hops that need forwarding info must include a cltv expiry.
+	case needFwdInfo && !hasLockTime:
 		return ErrInvalidPayload{
 			Type:      record.LockTimeOnionType,
 			Violation: OmittedViolation,
 			FinalHop:  isFinalHop,
 		}
 
-	// The exit hop should omit the next hop id. If nextHop != Exit, the
-	// sender must have included a record, so we don't need to test for its
-	// inclusion at intermediate hops directly.
-	case isFinalHop && hasNextHop:
+	// Hops that don't need forwarding info shouldn't have an amount TLV.
+	case !needFwdInfo && hasAmt:
+		return ErrInvalidPayload{
+			Type:      record.AmtOnionType,
+			Violation: IncludedViolation,
+			FinalHop:  isFinalHop,
+		}
+
+	// Hops that don't need forwarding info shouldn't have a cltv TLV.
+	case !needFwdInfo && hasLockTime:
+		return ErrInvalidPayload{
+			Type:      record.LockTimeOnionType,
+			Violation: IncludedViolation,
+			FinalHop:  isFinalHop,
+		}
+
+	// The exit hop and all blinded hops should omit the next hop id.
+	case !needNextHop && hasNextHop:
 		return ErrInvalidPayload{
 			Type:      record.NextHopOnionType,
 			Violation: IncludedViolation,
-			FinalHop:  true,
+			FinalHop:  isFinalHop,
+		}
+
+	// Require that the next hop is set for intermediate hops in regular
+	// routes.
+	case needNextHop && !hasNextHop:
+		return ErrInvalidPayload{
+			Type:      record.NextHopOnionType,
+			Violation: OmittedViolation,
+			FinalHop:  isFinalHop,
 		}
 
 	// Intermediate nodes should never receive MPP fields.