From befa1b7cf01e5611ecd34fb3ba8717b768942aa4 Mon Sep 17 00:00:00 2001 From: andreihod Date: Mon, 12 Sep 2022 20:58:58 -0300 Subject: [PATCH] routing: Fix possible infinite loop on first hop misleading hint Add ignore condition to additional edges that connect to self. These edges are already known and avoiding these hints protect the payment from malformed channel ids which could lead to infinite loop. Fixes lightningnetwork#6169. Co-authored-by: lsunsi --- routing/integrated_routing_context_test.go | 3 + routing/integrated_routing_test.go | 30 +++++++++ routing/mock_graph_test.go | 8 ++- routing/pathfind.go | 8 +++ routing/pathfind_test.go | 55 ++++++++++++++++ routing/router_test.go | 73 ++++++++++++++++++++++ 6 files changed, 176 insertions(+), 1 deletion(-) diff --git a/routing/integrated_routing_context_test.go b/routing/integrated_routing_context_test.go index d3baa3b41..3c5b63c6f 100644 --- a/routing/integrated_routing_context_test.go +++ b/routing/integrated_routing_context_test.go @@ -10,6 +10,7 @@ import ( "github.com/lightningnetwork/lnd/kvdb" "github.com/lightningnetwork/lnd/lnwire" "github.com/lightningnetwork/lnd/routing/route" + "github.com/lightningnetwork/lnd/zpay32" ) const ( @@ -47,6 +48,7 @@ type integratedRoutingContext struct { mcCfg MissionControlConfig pathFindingCfg PathFindingConfig + routeHints [][]zpay32.HopHint } // newIntegratedRoutingContext instantiates a new integrated routing test @@ -177,6 +179,7 @@ func (c *integratedRoutingContext) testPayment(maxParts uint32, Amount: c.amt, CltvLimit: math.MaxUint32, MaxParts: maxParts, + RouteHints: c.routeHints, } var paymentHash [32]byte diff --git a/routing/integrated_routing_test.go b/routing/integrated_routing_test.go index 0962f45f5..2dd2cf632 100644 --- a/routing/integrated_routing_test.go +++ b/routing/integrated_routing_test.go @@ -4,9 +4,11 @@ import ( "fmt" "testing" + "github.com/btcsuite/btcd/btcec/v2" "github.com/btcsuite/btcd/btcutil" "github.com/davecgh/go-spew/spew" "github.com/lightningnetwork/lnd/lnwire" + "github.com/lightningnetwork/lnd/zpay32" "github.com/stretchr/testify/require" ) @@ -234,6 +236,34 @@ var mppTestCases = []mppSendTestCase{ }, } +// TestBadFirstHopHint tests that a payment with a first hop hint with an +// invalid channel id still works since the node already knows its channel and +// doesn't need hints. +func TestBadFirstHopHint(t *testing.T) { + t.Parallel() + + ctx := newIntegratedRoutingContext(t) + + onePathGraph(ctx.graph) + + sourcePubKey, _ := btcec.ParsePubKey(ctx.source.pubkey[:]) + + hopHint := zpay32.HopHint{ + NodeID: sourcePubKey, + ChannelID: 66, + FeeBaseMSat: 0, + FeeProportionalMillionths: 0, + CLTVExpiryDelta: 100, + } + + ctx.routeHints = [][]zpay32.HopHint{{hopHint}} + + ctx.amt = lnwire.NewMSatFromSatoshis(100) + _, err := ctx.testPayment(1) + + require.NoError(t, err, "payment failed") +} + // TestMppSend tests that a payment can be completed using multiple shards. func TestMppSend(t *testing.T) { for _, testCase := range mppTestCases { diff --git a/routing/mock_graph_test.go b/routing/mock_graph_test.go index 33a9c11af..79d46d012 100644 --- a/routing/mock_graph_test.go +++ b/routing/mock_graph_test.go @@ -5,6 +5,7 @@ import ( "fmt" "testing" + "github.com/btcsuite/btcd/btcec/v2" "github.com/btcsuite/btcd/btcutil" "github.com/lightningnetwork/lnd/channeldb" "github.com/lightningnetwork/lnd/lnwire" @@ -13,7 +14,12 @@ import ( // createPubkey return a new test pubkey. func createPubkey(id byte) route.Vertex { - pubkey := route.Vertex{id} + _, secpPub := btcec.PrivKeyFromBytes([]byte{id}) + + var bytes [33]byte + copy(bytes[:], secpPub.SerializeCompressed()[:33]) + + pubkey := route.Vertex(bytes) return pubkey } diff --git a/routing/pathfind.go b/routing/pathfind.go index a92d0a7e5..0d0a1e920 100644 --- a/routing/pathfind.go +++ b/routing/pathfind.go @@ -538,10 +538,18 @@ func findPath(g *graphParams, r *RestrictParams, cfg *PathFindingConfig, additionalEdgesWithSrc := make(map[route.Vertex][]*edgePolicyWithSource) for vertex, outgoingEdgePolicies := range g.additionalEdges { + // Edges connected to self are always included in the graph, + // therefore can be skipped. This prevents us from trying + // routes to malformed hop hints. + if vertex == self { + continue + } + // Build reverse lookup to find incoming edges. Needed because // search is taken place from target to source. for _, outgoingEdgePolicy := range outgoingEdgePolicies { toVertex := outgoingEdgePolicy.ToNodePubKey() + incomingEdgePolicy := &edgePolicyWithSource{ sourceNode: vertex, edge: outgoingEdgePolicy, diff --git a/routing/pathfind_test.go b/routing/pathfind_test.go index c14317059..a3202d1e9 100644 --- a/routing/pathfind_test.go +++ b/routing/pathfind_test.go @@ -773,6 +773,9 @@ func TestPathFinding(t *testing.T) { }, { name: "path finding with additional edges", fn: runPathFindingWithAdditionalEdges, + }, { + name: "path finding with redundant additional edges", + fn: runPathFindingWithRedundantAdditionalEdges, }, { name: "new route path too long", fn: runNewRoutePathTooLong, @@ -1290,6 +1293,58 @@ func runPathFindingWithAdditionalEdges(t *testing.T, useCache bool) { assertExpectedPath(t, graph.aliasMap, path, "songoku", "doge") } +// runPathFindingWithRedundantAdditionalEdges asserts that we are able to find +// paths to nodes ignoring additional edges that are already known by self node. +func runPathFindingWithRedundantAdditionalEdges(t *testing.T, useCache bool) { + t.Helper() + + var realChannelID uint64 = 3145 + var hintChannelID uint64 = 1618 + + testChannels := []*testChannel{ + symmetricTestChannel("alice", "bob", 100000, &testChannelPolicy{ + Expiry: 144, + FeeRate: 400, + MinHTLC: 1, + MaxHTLC: 100000000, + }, realChannelID), + } + + ctx := newPathFindingTestContext(t, useCache, testChannels, "alice") + + target := ctx.keyFromAlias("bob") + paymentAmt := lnwire.NewMSatFromSatoshis(100) + + // Create the channel edge going from alice to bob and include it in + // our map of additional edges. + aliceToBob := &channeldb.CachedEdgePolicy{ + ToNodePubKey: func() route.Vertex { + return target + }, + ToNodeFeatures: lnwire.EmptyFeatureVector(), + ChannelID: hintChannelID, + FeeBaseMSat: 1, + FeeProportionalMillionths: 1000, + TimeLockDelta: 9, + } + + additionalEdges := map[route.Vertex][]*channeldb.CachedEdgePolicy{ + ctx.source: {aliceToBob}, + } + + path, err := dbFindPath( + ctx.graph, additionalEdges, ctx.bandwidthHints, + &ctx.restrictParams, &ctx.pathFindingConfig, ctx.source, target, + paymentAmt, ctx.timePref, 0, + ) + + require.NoError(t, err, "unable to find path to bob") + require.Len(t, path, 1) + + require.Equal(t, realChannelID, path[0].ChannelID, + "additional edge for known edge wasn't ignored") +} + // TestNewRoute tests whether the construction of hop payloads by newRoute // is executed correctly. func TestNewRoute(t *testing.T) { diff --git a/routing/router_test.go b/routing/router_test.go index 8c1edd253..8df1f1f5a 100644 --- a/routing/router_test.go +++ b/routing/router_test.go @@ -361,6 +361,79 @@ func TestSendPaymentRouteFailureFallback(t *testing.T) { ) } +// TestSendPaymentRouteInfiniteLoopWithBadHopHint tests that when sending +// a payment with a malformed hop hint in the first hop, the hint is ignored +// and the payment succeeds without an infinite loop of retries. +func TestSendPaymentRouteInfiniteLoopWithBadHopHint(t *testing.T) { + t.Parallel() + + const startingBlockHeight = 101 + ctx := createTestCtxFromFile(t, startingBlockHeight, basicGraphFilePath) + + source := ctx.aliases["roasbeef"] + sourceNodeID, err := btcec.ParsePubKey(source[:]) + require.NoError(t, err) + + actualChannelID := ctx.getChannelIDFromAlias(t, "roasbeef", "songoku") + badChannelID := uint64(66666) + + // Craft a LightningPayment struct that'll send a payment from roasbeef + // to songoku for 1000 satoshis. + var payHash lntypes.Hash + paymentAmt := lnwire.NewMSatFromSatoshis(1000) + payment := LightningPayment{ + Target: ctx.aliases["songoku"], + Amount: paymentAmt, + FeeLimit: noFeeLimit, + paymentHash: &payHash, + RouteHints: [][]zpay32.HopHint{{ + zpay32.HopHint{ + NodeID: sourceNodeID, + ChannelID: badChannelID, + FeeBaseMSat: uint32(50), + CLTVExpiryDelta: uint16(200), + }, + }}, + } + + var preImage [32]byte + copy(preImage[:], bytes.Repeat([]byte{9}, 32)) + + // Mock a payment result that always fails with FailUnknownNextPeer when + // the bad channel is the first hop. + badShortChanID := lnwire.NewShortChanIDFromInt(badChannelID) + newFwdError := htlcswitch.NewForwardingError( + &lnwire.FailUnknownNextPeer{}, 0, + ) + + payer, ok := ctx.router.cfg.Payer.(*mockPaymentAttemptDispatcherOld) + require.Equal(t, ok, true, "failed Payer cast") + + payer.setPaymentResult( + func(firstHop lnwire.ShortChannelID) ([32]byte, error) { + // Returns a FailUnknownNextPeer if it's trying + // to pay an invalid channel. + if firstHop == badShortChanID { + return [32]byte{}, newFwdError + } + + return preImage, nil + }) + + // Send off the payment request to the router, should succeed + // ignoring the bad channel id hint. + paymentPreImage, route, paymentErr := ctx.router.SendPayment(&payment) + require.NoError(t, paymentErr, "payment returned an error") + + // The preimage should match up with the one created above. + require.Equal(t, preImage[:], paymentPreImage[:], "incorrect preimage") + + // The route should have songoku as the first hop. + require.Equal(t, actualChannelID, route.Hops[0].ChannelID, + "route should go through the correct channel id", + ) +} + // TestChannelUpdateValidation tests that a failed payment with an associated // channel update will only be applied to the graph when the update contains a // valid signature.