From 69d5496e7cf7d65806775ec0eca08866ed5ff441 Mon Sep 17 00:00:00 2001 From: Carla Kirk-Cohen Date: Wed, 1 Nov 2023 15:36:44 -0400 Subject: [PATCH] multi: update payload validation to account for blinded routes --- htlcswitch/hop/payload.go | 51 ++++++++++++--- htlcswitch/hop/payload_test.go | 90 +++++++++++++++++++++++++++ routing/route/route.go | 10 +-- routing/route/route_test.go | 110 ++++++++++++++++++++++++++++++--- 4 files changed, 238 insertions(+), 23 deletions(-) diff --git a/htlcswitch/hop/payload.go b/htlcswitch/hop/payload.go index 7c2e607ae..a49a264fe 100644 --- a/htlcswitch/hop/payload.go +++ b/htlcswitch/hop/payload.go @@ -256,33 +256,64 @@ func ValidateParsedPayloadTypes(parsedTypes tlv.TypeMap, _, 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, otherwise 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 d52b056c7..63c9ceeef 100644 --- a/htlcswitch/hop/payload_test.go +++ b/htlcswitch/hop/payload_test.go @@ -254,6 +254,21 @@ var decodePayloadTests = []decodePayloadTest{ FinalHop: false, }, }, + { + 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, @@ -348,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 diff --git a/routing/route/route.go b/routing/route/route.go index 2ed9751ae..934a36ec1 100644 --- a/routing/route/route.go +++ b/routing/route/route.go @@ -42,9 +42,9 @@ var ( // ErrMissingField is returned if a required TLV is missing. ErrMissingField = errors.New("required tlv missing") - // ErrIncorrectField is returned if a tlv field is included when it + // ErrUnexpectedField is returned if a tlv field is included when it // should not be. - ErrIncorrectField = errors.New("incorrect tlv included") + ErrUnexpectedField = errors.New("unexpected tlv included") ) // Vertex is a simple alias for the serialization of a compressed Bitcoin @@ -337,7 +337,7 @@ func optionalBlindedField(isZero, blindedHop, finalHop bool) error { // In an intermediate hop in a blinded route and the field is not zero. case !finalHop && !isZero: - return ErrIncorrectField + return ErrUnexpectedField } return nil @@ -351,7 +351,7 @@ 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 ErrIncorrectField + return ErrUnexpectedField // Otherwise, blinded hops are allowed to have a zero value. case isBlinded: @@ -359,7 +359,7 @@ func validateNextChanID(nextChanIDIsSet, isBlinded, finalHop bool) error { // The final hop in a regular route is expected to have a zero value. case finalHop && nextChanIDIsSet: - return ErrIncorrectField + return ErrUnexpectedField // Intermediate hops in regular routes require non-zero value. case !finalHop && !nextChanIDIsSet: diff --git a/routing/route/route_test.go b/routing/route/route_test.go index e5202d56a..99594833f 100644 --- a/routing/route/route_test.go +++ b/routing/route/route_test.go @@ -160,18 +160,112 @@ func TestAMPHop(t *testing.T) { } } -// 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, 0, false) - 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