From 7be7d5d5247b1051213c39e7eca5682c2b45a379 Mon Sep 17 00:00:00 2001 From: Bastien Teinturier <31281497+t-bast@users.noreply.github.com> Date: Fri, 10 Nov 2023 16:51:24 +0100 Subject: [PATCH] Don't rebroadcast channel updates from `update_fail_htlc` (#2775) When we receive a `channel_update` in `update_fail_htlc`, we should take it into account to exclude channels or correctly retry with updated fees, but we shouldn't apply it to our routing table. If we did, that could be exploited to deanonymize our payments. It shouldn't be necessary anyway, as honest nodes should broadcast those `channel_update`s publicly, so we would receive them through the normal gossip mechanism. Fixes #2767 --- .../eclair/payment/send/PaymentLifecycle.scala | 16 +++++++++++----- .../eclair/payment/PaymentLifecycleSpec.scala | 14 +------------- 2 files changed, 12 insertions(+), 18 deletions(-) diff --git a/eclair-core/src/main/scala/fr/acinq/eclair/payment/send/PaymentLifecycle.scala b/eclair-core/src/main/scala/fr/acinq/eclair/payment/send/PaymentLifecycle.scala index 381ce075e1..809ee8b4ca 100644 --- a/eclair-core/src/main/scala/fr/acinq/eclair/payment/send/PaymentLifecycle.scala +++ b/eclair-core/src/main/scala/fr/acinq/eclair/payment/send/PaymentLifecycle.scala @@ -24,7 +24,7 @@ import fr.acinq.bitcoin.scalacompat.Crypto.PublicKey import fr.acinq.eclair._ import fr.acinq.eclair.channel._ import fr.acinq.eclair.crypto.{Sphinx, TransportHandler} -import fr.acinq.eclair.db.{OutgoingPayment, OutgoingPaymentStatus, PaymentType} +import fr.acinq.eclair.db.{OutgoingPayment, OutgoingPaymentStatus} import fr.acinq.eclair.payment.Invoice.ExtraEdge import fr.acinq.eclair.payment.Monitoring.{Metrics, Tags} import fr.acinq.eclair.payment.OutgoingPaymentPacket.Upstream @@ -272,7 +272,16 @@ class PaymentLifecycle(nodeParams: NodeParams, cfg: SendPaymentConfig, router: A } /** - * Apply the channel update to our routing table. + * Analyze the channel_update we received and update our routing state accordingly. + * + * Note that we don't forward the channel update to the router, because it could leak that we were the payer: + * - a malicious intermediate node would fail the payment with a custom channel_update + * - they would *not* send this channel_update to the rest of the network + * - they would then directly connect to us and ask for our latest channel_update for that channel + * - if we sent them that channel_update, they'd know we were the payer + * + * We don't need to send that update to the router anyway: if the sending node is honest, we should receive it with + * the standard gossip mechanism. * * @return updated routing hints if applicable. */ @@ -318,9 +327,6 @@ class PaymentLifecycle(nodeParams: NodeParams, cfg: SendPaymentConfig, router: A log.error(s"couldn't find node=$nodeId in the route, this should never happen") data.recipient.extraEdges } - // in all cases, we forward the update to the router: if the channel is disabled, the router will remove it from its routing table - // if the channel is not announced (e.g. was from a hint), the router will simply ignore the update - router ! failure.update // we update the recipient's assisted routes: they take precedence over the router's routing table data.recipient match { case recipient: ClearRecipient => recipient.copy(extraEdges = extraEdges1) diff --git a/eclair-core/src/test/scala/fr/acinq/eclair/payment/PaymentLifecycleSpec.scala b/eclair-core/src/test/scala/fr/acinq/eclair/payment/PaymentLifecycleSpec.scala index a598b0e583..0022597a83 100644 --- a/eclair-core/src/test/scala/fr/acinq/eclair/payment/PaymentLifecycleSpec.scala +++ b/eclair-core/src/test/scala/fr/acinq/eclair/payment/PaymentLifecycleSpec.scala @@ -499,8 +499,6 @@ class PaymentLifecycleSpec extends BaseRouterSpec { assert(routerForwarder.expectMsgType[ChannelCouldNotRelay].hop.shortChannelId == update_bc.shortChannelId) routerForwarder.expectMsg(ExcludeChannel(ChannelDesc(update_bc.shortChannelId, b, c), Some(nodeParams.routerConf.channelExcludeDuration))) routerForwarder.forward(routerFixture.router) - // payment lifecycle forwards the embedded channelUpdate to the router - routerForwarder.expectMsg(update_bc) awaitCond(paymentFSM.stateName == WAITING_FOR_ROUTE) routerForwarder.expectMsg(defaultRouteRequest(a, cfg).copy(ignore = Ignore(Set.empty, Set(ChannelDesc(update_bc.shortChannelId, b, c))))) routerForwarder.forward(routerFixture.router) @@ -530,8 +528,6 @@ class PaymentLifecycleSpec extends BaseRouterSpec { // and node replies with a failure containing a new channel update sender.send(paymentFSM, addCompleted(HtlcResult.RemoteFail(UpdateFailHtlc(ByteVector32.Zeroes, 0, Sphinx.FailurePacket.create(sharedSecrets1.head._1, failure))))) - // payment lifecycle forwards the embedded channelUpdate to the router - routerForwarder.expectMsg(channelUpdate_bc_modified) awaitCond(paymentFSM.stateName == WAITING_FOR_ROUTE && nodeParams.db.payments.getOutgoingPayment(id).exists(_.status == OutgoingPaymentStatus.Pending)) // 1 failure but not final, the payment is still PENDING routerForwarder.expectMsg(defaultRouteRequest(a, cfg)) routerForwarder.forward(routerFixture.router) @@ -550,8 +546,6 @@ class PaymentLifecycleSpec extends BaseRouterSpec { // this time the payment lifecycle will ask the router to temporarily exclude this channel from its route calculations routerForwarder.expectMsg(ExcludeChannel(ChannelDesc(update_bc.shortChannelId, b, c), Some(nodeParams.routerConf.channelExcludeDuration))) routerForwarder.forward(routerFixture.router) - // but it will still forward the embedded channelUpdate to the router - routerForwarder.expectMsg(channelUpdate_bc_modified_2) awaitCond(paymentFSM.stateName == WAITING_FOR_ROUTE) routerForwarder.expectMsg(defaultRouteRequest(a, cfg)) routerForwarder.forward(routerFixture.router) @@ -581,7 +575,6 @@ class PaymentLifecycleSpec extends BaseRouterSpec { // we should temporarily exclude that channel assert(routerForwarder.expectMsgType[ChannelCouldNotRelay].hop.shortChannelId == update_bc.shortChannelId) routerForwarder.expectMsg(ExcludeChannel(ChannelDesc(update_bc.shortChannelId, b, c), Some(nodeParams.routerConf.channelExcludeDuration))) - routerForwarder.expectMsg(update_bc) // this was a single attempt payment sender.expectMsgType[PaymentFailed] @@ -614,8 +607,6 @@ class PaymentLifecycleSpec extends BaseRouterSpec { // and node replies with a failure containing a new channel update sender.send(paymentFSM, addCompleted(HtlcResult.RemoteFail(UpdateFailHtlc(ByteVector32.Zeroes, 0, Sphinx.FailurePacket.create(sharedSecrets1.head._1, failure))))) - // payment lifecycle forwards the embedded channelUpdate to the router - routerForwarder.expectMsg(channelUpdate_bc_modified) awaitCond(paymentFSM.stateName == WAITING_FOR_ROUTE && nodeParams.db.payments.getOutgoingPayment(id).exists(_.status == OutgoingPaymentStatus.Pending)) // 1 failure but not final, the payment is still PENDING val extraEdges1 = Seq( recipient.extraEdges(0).update(channelUpdate_bc_modified), @@ -658,7 +649,6 @@ class PaymentLifecycleSpec extends BaseRouterSpec { assert(routerForwarder.expectMsgType[RouteCouldRelay].route.hops.map(_.shortChannelId) == Seq(update_ab, update_bc).map(_.shortChannelId)) routerForwarder.expectMsg(ExcludeChannel(ChannelDesc(update_cd.shortChannelId, c, d), Some(nodeParams.routerConf.channelExcludeDuration))) - routerForwarder.expectMsg(channelUpdate_cd_disabled) } def testPermanentFailure(router: ActorRef, failure: FailureMessage): Unit = { @@ -960,11 +950,9 @@ class PaymentLifecycleSpec extends BaseRouterSpec { // and node replies with a failure containing a new channel update sender.send(paymentFSM, addCompleted(HtlcResult.RemoteFail(UpdateFailHtlc(ByteVector32.Zeroes, 0, Sphinx.FailurePacket.create(sharedSecrets1.head._1, failure))))) - // payment lifecycle forwards the embedded channelUpdate to the router - routerForwarder.expectMsg(channelUpdate_bc_modified) - // The payment fails without retrying sender.expectMsgType[PaymentFailed] + routerForwarder.expectNoMessage(100 millis) // we don't forward the channel_update to the router } }