diff --git a/docs/release-notes/release-notes-0.18.0.md b/docs/release-notes/release-notes-0.18.0.md index 4d0c7fd7b..b5327c11b 100644 --- a/docs/release-notes/release-notes-0.18.0.md +++ b/docs/release-notes/release-notes-0.18.0.md @@ -100,6 +100,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 diff --git a/htlcswitch/hop/fuzz_test.go b/htlcswitch/hop/fuzz_test.go index 82a92eb22..d7e463543 100644 --- a/htlcswitch/hop/fuzz_test.go +++ b/htlcswitch/hop/fuzz_test.go @@ -92,7 +92,21 @@ 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 @@ -100,25 +114,36 @@ func FuzzPayload(f *testing.F) { 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 - } - require.NoError(t, err) - payload2, err := NewPayloadFromReader(&b) + // 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) + } + + payload2, err := NewPayloadFromReader(&b, finalPayload) require.NoError(t, err) require.Equal(t, payload1, payload2) diff --git a/htlcswitch/hop/iterator.go b/htlcswitch/hop/iterator.go index b5c7d0c83..96a5c5f2b 100644 --- a/htlcswitch/hop/iterator.go +++ b/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", diff --git a/htlcswitch/hop/iterator_test.go b/htlcswitch/hop/iterator_test.go index 524b70df0..cb2a2816f 100644 --- a/htlcswitch/hop/iterator_test.go +++ b/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, }, diff --git a/htlcswitch/hop/payload.go b/htlcswitch/hop/payload.go index 03fdd94f3..a49a264fe 100644 --- a/htlcswitch/hop/payload.go +++ b/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] + + // 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) switch { - - // All hops must include an amount to forward. - case !hasAmt: + // 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. diff --git a/htlcswitch/hop/payload_test.go b/htlcswitch/hop/payload_test.go index 6913f11c6..63c9ceeef 100644 --- a/htlcswitch/hop/payload_test.go +++ b/htlcswitch/hop/payload_test.go @@ -24,6 +24,7 @@ const testUnknownRequiredType = 0x80 type decodePayloadTest struct { name string payload []byte + isFinalHop bool expErr error expCustomRecords map[uint64][]byte shouldHaveMPP bool @@ -36,18 +37,21 @@ type decodePayloadTest struct { var decodePayloadTests = []decodePayloadTest{ { - name: "final hop valid", - payload: []byte{0x02, 0x00, 0x04, 0x00}, + name: "final hop valid", + isFinalHop: true, + payload: []byte{0x02, 0x00, 0x04, 0x00}, }, { - name: "intermediate hop valid", + name: "intermediate hop valid", + isFinalHop: false, payload: []byte{0x02, 0x00, 0x04, 0x00, 0x06, 0x08, 0x01, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, }, }, { - name: "final hop no amount", - payload: []byte{0x04, 0x00}, + name: "final hop no amount", + payload: []byte{0x04, 0x00}, + isFinalHop: true, expErr: hop.ErrInvalidPayload{ Type: record.AmtOnionType, Violation: hop.OmittedViolation, @@ -55,7 +59,8 @@ var decodePayloadTests = []decodePayloadTest{ }, }, { - name: "intermediate hop no amount", + name: "intermediate hop no amount", + isFinalHop: false, payload: []byte{0x04, 0x00, 0x06, 0x08, 0x01, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, }, @@ -66,8 +71,9 @@ var decodePayloadTests = []decodePayloadTest{ }, }, { - name: "final hop no expiry", - payload: []byte{0x02, 0x00}, + name: "final hop no expiry", + isFinalHop: true, + payload: []byte{0x02, 0x00}, expErr: hop.ErrInvalidPayload{ Type: record.LockTimeOnionType, Violation: hop.OmittedViolation, @@ -75,7 +81,8 @@ var decodePayloadTests = []decodePayloadTest{ }, }, { - name: "intermediate hop no expiry", + name: "intermediate hop no expiry", + isFinalHop: false, payload: []byte{0x02, 0x00, 0x06, 0x08, 0x01, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, }, @@ -86,7 +93,8 @@ var decodePayloadTests = []decodePayloadTest{ }, }, { - name: "final hop next sid present", + name: "final hop next sid present", + isFinalHop: true, payload: []byte{0x02, 0x00, 0x04, 0x00, 0x06, 0x08, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, }, @@ -97,7 +105,8 @@ var decodePayloadTests = []decodePayloadTest{ }, }, { - name: "required type after omitted hop id", + name: "required type after omitted hop id", + isFinalHop: true, payload: []byte{ 0x02, 0x00, 0x04, 0x00, testUnknownRequiredType, 0x00, @@ -109,7 +118,8 @@ var decodePayloadTests = []decodePayloadTest{ }, }, { - name: "required type after included hop id", + name: "required type after included hop id", + isFinalHop: false, payload: []byte{ 0x02, 0x00, 0x04, 0x00, 0x06, 0x08, 0x01, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, @@ -122,8 +132,9 @@ var decodePayloadTests = []decodePayloadTest{ }, }, { - name: "required type zero final hop", - payload: []byte{0x00, 0x00, 0x02, 0x00, 0x04, 0x00}, + name: "required type zero final hop", + isFinalHop: true, + payload: []byte{0x00, 0x00, 0x02, 0x00, 0x04, 0x00}, expErr: hop.ErrInvalidPayload{ Type: 0, Violation: hop.RequiredViolation, @@ -131,7 +142,8 @@ var decodePayloadTests = []decodePayloadTest{ }, }, { - name: "required type zero final hop zero sid", + name: "required type zero final hop zero sid", + isFinalHop: true, payload: []byte{0x00, 0x00, 0x02, 0x00, 0x04, 0x00, 0x06, 0x08, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, }, @@ -142,7 +154,8 @@ var decodePayloadTests = []decodePayloadTest{ }, }, { - name: "required type zero intermediate hop", + name: "required type zero intermediate hop", + isFinalHop: false, payload: []byte{0x00, 0x00, 0x02, 0x00, 0x04, 0x00, 0x06, 0x08, 0x01, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, }, @@ -153,8 +166,17 @@ var decodePayloadTests = []decodePayloadTest{ }, }, { - name: "required type in custom range", - payload: []byte{0x02, 0x00, 0x04, 0x00, + name: "required type in custom range", + isFinalHop: false, + payload: []byte{ + // amount + 0x02, 0x00, + // cltv + 0x04, 0x00, + // next hop id + 0x06, 0x08, + 0x01, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, + // custom 0xfe, 0x00, 0x01, 0x00, 0x00, 0x02, 0x10, 0x11, }, expCustomRecords: map[uint64][]byte{ @@ -162,19 +184,22 @@ var decodePayloadTests = []decodePayloadTest{ }, }, { - name: "valid intermediate hop", + name: "valid intermediate hop", + isFinalHop: false, payload: []byte{0x02, 0x00, 0x04, 0x00, 0x06, 0x08, 0x01, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, }, expErr: nil, }, { - name: "valid final hop", - payload: []byte{0x02, 0x00, 0x04, 0x00}, - expErr: nil, + name: "valid final hop", + isFinalHop: true, + payload: []byte{0x02, 0x00, 0x04, 0x00}, + expErr: nil, }, { - name: "intermediate hop with mpp", + name: "intermediate hop with mpp", + isFinalHop: false, payload: []byte{ // amount 0x02, 0x00, @@ -198,7 +223,8 @@ var decodePayloadTests = []decodePayloadTest{ }, }, { - name: "intermediate hop with amp", + name: "intermediate hop with amp", + isFinalHop: false, payload: []byte{ // amount 0x02, 0x00, @@ -229,24 +255,35 @@ var decodePayloadTests = []decodePayloadTest{ }, }, { - name: "intermediate hop with encrypted data", + name: "intermediate hop no next channel", + isFinalHop: false, payload: []byte{ // amount 0x02, 0x00, // cltv 0x04, 0x00, + }, + expErr: hop.ErrInvalidPayload{ + Type: record.NextHopOnionType, + Violation: hop.OmittedViolation, + FinalHop: false, + }, + }, + { + name: "intermediate hop with encrypted data", + isFinalHop: false, + payload: []byte{ // encrypted data 0x0a, 0x03, 0x03, 0x02, 0x01, }, shouldHaveEncData: true, }, { - name: "intermediate hop with blinding point", + name: "intermediate hop with blinding point", + isFinalHop: false, payload: append([]byte{ - // amount - 0x02, 0x00, - // cltv - 0x04, 0x00, + // encrypted data + 0x0a, 0x03, 0x03, 0x02, 0x01, // blinding point (type / length) 0x0c, 0x21, }, @@ -254,9 +291,11 @@ var decodePayloadTests = []decodePayloadTest{ testPubKey.SerializeCompressed()..., ), shouldHaveBlinding: true, + shouldHaveEncData: true, }, { - name: "final hop with mpp", + name: "final hop with mpp", + isFinalHop: true, payload: []byte{ // amount 0x02, 0x00, @@ -274,7 +313,8 @@ var decodePayloadTests = []decodePayloadTest{ shouldHaveMPP: true, }, { - name: "final hop with amp", + name: "final hop with amp", + isFinalHop: true, payload: []byte{ // amount 0x02, 0x00, @@ -298,7 +338,8 @@ var decodePayloadTests = []decodePayloadTest{ shouldHaveAMP: true, }, { - name: "final hop with metadata", + name: "final hop with metadata", + isFinalHop: true, payload: []byte{ // amount 0x02, 0x00, @@ -310,7 +351,8 @@ var decodePayloadTests = []decodePayloadTest{ shouldHaveMetadata: true, }, { - name: "final hop with total amount", + name: "final hop with total amount", + isFinalHop: true, payload: []byte{ // amount 0x02, 0x00, @@ -321,6 +363,81 @@ var decodePayloadTests = []decodePayloadTest{ }, shouldHaveTotalAmt: true, }, + { + name: "final blinded hop with total amount", + isFinalHop: true, + payload: []byte{ + // amount + 0x02, 0x00, + // cltv + 0x04, 0x00, + // encrypted data + 0x0a, 0x03, 0x03, 0x02, 0x01, + }, + shouldHaveEncData: true, + }, + { + name: "final blinded missing amt", + isFinalHop: true, + payload: []byte{ + // cltv + 0x04, 0x00, + // encrypted data + 0x0a, 0x03, 0x03, 0x02, 0x01, + }, + shouldHaveEncData: true, + expErr: hop.ErrInvalidPayload{ + Type: record.AmtOnionType, + Violation: hop.OmittedViolation, + FinalHop: true, + }, + }, + { + name: "final blinded missing cltv", + isFinalHop: true, + payload: []byte{ + // amount + 0x02, 0x00, + // encrypted data + 0x0a, 0x03, 0x03, 0x02, 0x01, + }, + shouldHaveEncData: true, + expErr: hop.ErrInvalidPayload{ + Type: record.LockTimeOnionType, + Violation: hop.OmittedViolation, + FinalHop: true, + }, + }, + { + name: "intermediate blinded has amount", + isFinalHop: false, + payload: []byte{ + // amount + 0x02, 0x00, + // encrypted data + 0x0a, 0x03, 0x03, 0x02, 0x01, + }, + expErr: hop.ErrInvalidPayload{ + Type: record.AmtOnionType, + Violation: hop.IncludedViolation, + FinalHop: false, + }, + }, + { + name: "intermediate blinded has expiry", + isFinalHop: false, + payload: []byte{ + // cltv + 0x04, 0x00, + // encrypted data + 0x0a, 0x03, 0x03, 0x02, 0x01, + }, + expErr: hop.ErrInvalidPayload{ + Type: record.LockTimeOnionType, + Violation: hop.IncludedViolation, + FinalHop: false, + }, + }, } // TestDecodeHopPayloadRecordValidation asserts that parsing the payloads in the @@ -361,7 +478,9 @@ func testDecodeHopPayloadValidation(t *testing.T, test decodePayloadTest) { testChildIndex = uint32(9) ) - p, err := hop.NewPayloadFromReader(bytes.NewReader(test.payload)) + p, err := hop.NewPayloadFromReader( + bytes.NewReader(test.payload), test.isFinalHop, + ) if !reflect.DeepEqual(test.expErr, err) { t.Fatalf("expected error mismatch, want: %v, got: %v", test.expErr, err) diff --git a/routing/route/route.go b/routing/route/route.go index fbf1b5b84..934a36ec1 100644 --- a/routing/route/route.go +++ b/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 } diff --git a/routing/route/route_test.go b/routing/route/route_test.go index 4524b0c6e..99594833f 100644 --- a/routing/route/route_test.go +++ b/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, diff --git a/routing/router_test.go b/routing/router_test.go index d6abfe7a9..227899c83 100644 --- a/routing/router_test.go +++ b/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)