From a566d53a6e6692f0acef91327012881f06c2b598 Mon Sep 17 00:00:00 2001 From: Bastien Teinturier <31281497+t-bast@users.noreply.github.com> Date: Fri, 4 Nov 2022 18:57:47 +0100 Subject: [PATCH] Allow nodes to overshoot final htlc amount and expiry (#2468) When nodes receive HTLCs, they verify that the contents of those HTLCs match the intructions that the sender provided in the onion. It is important to ensure that intermediate nodes and final nodes have similar requirements, otherwise a malicious intermediate node could easily probe whether the next node is the final recipient or not. Unfortunately, the requirements for intermediate nodes were more lenient than the requirements for final nodes. Intermediate nodes allowed overpaying and increasing the CLTV expiry, whereas final nodes required a perfect equality between the HTLC values and the onion values. This provided a trivial way of probing: when relaying an HTLC, nodes could relay 1 msat more than what the onion instructed (or increase the outgoing expiry by 1). If the next node was an intermediate node, they would accept this HTLC, but if the next node was the recipient, they would reject it. We update those requirements to fix this probing attack vector. See https://github.com/lightning/bolts/pull/1032 --- .../acinq/eclair/payment/PaymentPacket.scala | 14 +++---- .../eclair/payment/MultiPartHandlerSpec.scala | 41 +++++++++++++++++++ .../eclair/payment/PaymentPacketSpec.scala | 17 +++++++- 3 files changed, 63 insertions(+), 9 deletions(-) diff --git a/eclair-core/src/main/scala/fr/acinq/eclair/payment/PaymentPacket.scala b/eclair-core/src/main/scala/fr/acinq/eclair/payment/PaymentPacket.scala index 2a4cb71588..a6af9b4d2e 100644 --- a/eclair-core/src/main/scala/fr/acinq/eclair/payment/PaymentPacket.scala +++ b/eclair-core/src/main/scala/fr/acinq/eclair/payment/PaymentPacket.scala @@ -170,8 +170,8 @@ object IncomingPaymentPacket { private def validateFinalPayload(add: UpdateAddHtlc, payload: TlvStream[OnionPaymentPayloadTlv]): Either[FailureMessage, FinalPacket] = { FinalPayload.Standard.validate(payload).left.map(_.failureMessage).flatMap { - case payload if add.amountMsat != payload.amount => Left(FinalIncorrectHtlcAmount(add.amountMsat)) - case payload if add.cltvExpiry != payload.expiry => Left(FinalIncorrectCltvExpiry(add.cltvExpiry)) + case payload if add.amountMsat < payload.amount => Left(FinalIncorrectHtlcAmount(add.amountMsat)) + case payload if add.cltvExpiry < payload.expiry => Left(FinalIncorrectCltvExpiry(add.cltvExpiry)) case payload => Right(FinalPacket(add, payload)) } } @@ -189,14 +189,14 @@ object IncomingPaymentPacket { // The outer payload cannot use route blinding, but the inner payload may (but it's not supported yet). FinalPayload.Standard.validate(outerPayload).left.map(_.failureMessage).flatMap { outerPayload => FinalPayload.Standard.validate(innerPayload).left.map(_.failureMessage).flatMap { - case _ if add.amountMsat != outerPayload.amount => Left(FinalIncorrectHtlcAmount(add.amountMsat)) - case _ if add.cltvExpiry != outerPayload.expiry => Left(FinalIncorrectCltvExpiry(add.cltvExpiry)) - case innerPayload if outerPayload.expiry != innerPayload.expiry => Left(FinalIncorrectCltvExpiry(add.cltvExpiry)) // previous trampoline didn't forward the right expiry - case innerPayload if outerPayload.totalAmount != innerPayload.amount => Left(FinalIncorrectHtlcAmount(outerPayload.totalAmount)) // previous trampoline didn't forward the right amount + case _ if add.amountMsat < outerPayload.amount => Left(FinalIncorrectHtlcAmount(add.amountMsat)) + case _ if add.cltvExpiry < outerPayload.expiry => Left(FinalIncorrectCltvExpiry(add.cltvExpiry)) + case innerPayload if outerPayload.expiry < innerPayload.expiry => Left(FinalIncorrectCltvExpiry(add.cltvExpiry)) // previous trampoline didn't forward the right expiry + case innerPayload if outerPayload.totalAmount < innerPayload.amount => Left(FinalIncorrectHtlcAmount(outerPayload.totalAmount)) // previous trampoline didn't forward the right amount case innerPayload => // We merge contents from the outer and inner payloads. // We must use the inner payload's total amount and payment secret because the payment may be split between multiple trampoline payments (#reckless). - Right(FinalPacket(add, FinalPayload.Standard.createMultiPartPayload(outerPayload.amount, innerPayload.totalAmount, outerPayload.expiry, innerPayload.paymentSecret, innerPayload.paymentMetadata))) + Right(FinalPacket(add, FinalPayload.Standard.createMultiPartPayload(outerPayload.amount, innerPayload.totalAmount, innerPayload.expiry, innerPayload.paymentSecret, innerPayload.paymentMetadata))) } } } diff --git a/eclair-core/src/test/scala/fr/acinq/eclair/payment/MultiPartHandlerSpec.scala b/eclair-core/src/test/scala/fr/acinq/eclair/payment/MultiPartHandlerSpec.scala index 135b81ba3b..cb64e22af3 100644 --- a/eclair-core/src/test/scala/fr/acinq/eclair/payment/MultiPartHandlerSpec.scala +++ b/eclair-core/src/test/scala/fr/acinq/eclair/payment/MultiPartHandlerSpec.scala @@ -122,6 +122,22 @@ class MultiPartHandlerSpec extends TestKitBaseClass with FixtureAnyFunSuiteLike sender.expectNoMessage(50 millis) } + { + sender.send(handlerWithoutMpp, ReceiveStandardPayment(Some(50_000 msat), Left("1 coffee with extra fees and expiry"))) + val invoice = sender.expectMsgType[Bolt11Invoice] + + val add = UpdateAddHtlc(ByteVector32.One, 1, 75_000 msat, invoice.paymentHash, defaultExpiry + CltvExpiryDelta(12), TestConstants.emptyOnionPacket, None) + sender.send(handlerWithoutMpp, IncomingPaymentPacket.FinalPacket(add, FinalPayload.Standard.createSinglePartPayload(70_000 msat, defaultExpiry, invoice.paymentSecret, invoice.paymentMetadata))) + assert(register.expectMsgType[Register.Forward[CMD_FULFILL_HTLC]].message.id == add.id) + + val paymentReceived = eventListener.expectMsgType[PaymentReceived] + assert(paymentReceived.copy(parts = paymentReceived.parts.map(_.copy(timestamp = 0 unixms))) == PaymentReceived(add.paymentHash, PartialPayment(add.amountMsat, add.channelId, timestamp = 0 unixms) :: Nil)) + val received = nodeParams.db.payments.getIncomingPayment(invoice.paymentHash) + assert(received.isDefined && received.get.status.isInstanceOf[IncomingPaymentStatus.Received]) + assert(received.get.status.asInstanceOf[IncomingPaymentStatus.Received].copy(receivedAt = 0 unixms) == IncomingPaymentStatus.Received(add.amountMsat, 0 unixms)) + + sender.expectNoMessage(50 millis) + } { sender.send(handlerWithMpp, ReceiveStandardPayment(Some(amountMsat), Left("another coffee with multi-part"))) val invoice = sender.expectMsgType[Bolt11Invoice] @@ -576,6 +592,31 @@ class MultiPartHandlerSpec extends TestKitBaseClass with FixtureAnyFunSuiteLike f.sender.expectMsgType[PendingPayments].paymentHashes.isEmpty } + test("PaymentHandler should handle multi-part over-payment") { f => + val nodeParams = Alice.nodeParams.copy(features = featuresWithMpp) + val handler = TestActorRef[PaymentHandler](PaymentHandler.props(nodeParams, f.register.ref)) + + val preimage = randomBytes32() + f.sender.send(handler, ReceiveStandardPayment(Some(1000 msat), Left("1 coffee with tip please"), paymentPreimage_opt = Some(preimage))) + val invoice = f.sender.expectMsgType[Bolt11Invoice] + + val add1 = UpdateAddHtlc(randomBytes32(), 0, 1100 msat, invoice.paymentHash, f.defaultExpiry, TestConstants.emptyOnionPacket, None) + f.sender.send(handler, IncomingPaymentPacket.FinalPacket(add1, FinalPayload.Standard.createMultiPartPayload(add1.amountMsat, 1500 msat, add1.cltvExpiry, invoice.paymentSecret, invoice.paymentMetadata))) + val add2 = UpdateAddHtlc(randomBytes32(), 1, 500 msat, invoice.paymentHash, f.defaultExpiry, TestConstants.emptyOnionPacket, None) + f.sender.send(handler, IncomingPaymentPacket.FinalPacket(add2, FinalPayload.Standard.createMultiPartPayload(add2.amountMsat, 1500 msat, add2.cltvExpiry, invoice.paymentSecret, invoice.paymentMetadata))) + + f.register.expectMsgAllOf( + Register.Forward(null, add1.channelId, CMD_FULFILL_HTLC(add1.id, preimage, commit = true)), + Register.Forward(null, add2.channelId, CMD_FULFILL_HTLC(add2.id, preimage, commit = true)) + ) + + val paymentReceived = f.eventListener.expectMsgType[PaymentReceived] + assert(paymentReceived.parts.map(_.copy(timestamp = 0 unixms)).toSet == Set(PartialPayment(1100 msat, add1.channelId, 0 unixms), PartialPayment(500 msat, add2.channelId, 0 unixms))) + val received = nodeParams.db.payments.getIncomingPayment(invoice.paymentHash) + assert(received.isDefined && received.get.status.isInstanceOf[IncomingPaymentStatus.Received]) + assert(received.get.status.asInstanceOf[IncomingPaymentStatus.Received].amount == 1600.msat) + } + test("PaymentHandler should handle multi-part payment timeout then success") { f => val nodeParams = Alice.nodeParams.copy(multiPartPaymentExpiry = 250 millis, features = featuresWithMpp) val handler = TestActorRef[PaymentHandler](PaymentHandler.props(nodeParams, f.register.ref)) diff --git a/eclair-core/src/test/scala/fr/acinq/eclair/payment/PaymentPacketSpec.scala b/eclair-core/src/test/scala/fr/acinq/eclair/payment/PaymentPacketSpec.scala index 6360ed8c95..d1f775eb86 100644 --- a/eclair-core/src/test/scala/fr/acinq/eclair/payment/PaymentPacketSpec.scala +++ b/eclair-core/src/test/scala/fr/acinq/eclair/payment/PaymentPacketSpec.scala @@ -148,6 +148,19 @@ class PaymentPacketSpec extends AnyFunSuite with BeforeAndAfterAll { assert(payload_b.asInstanceOf[FinalPayload.Standard].paymentMetadata.contains(paymentMetadata)) } + test("build a command with greater amount and expiry") { + val Success((add, _)) = buildCommand(ActorRef.noSender, Upstream.Local(UUID.randomUUID), paymentHash, hops.take(1), FinalPayload.Standard.createSinglePartPayload(finalAmount, finalExpiry, paymentSecret, None)) + + // let's peel the onion + val add_b = UpdateAddHtlc(randomBytes32(), 0, finalAmount + 100.msat, paymentHash, finalExpiry + CltvExpiryDelta(6), add.onion, None) + val Right(FinalPacket(_, payload_b)) = decrypt(add_b, priv_b.privateKey, Features.empty) + assert(payload_b.isInstanceOf[FinalPayload.Standard]) + assert(payload_b.amount == finalAmount) + assert(payload_b.totalAmount == finalAmount) + assert(payload_b.expiry == finalExpiry) + assert(payload_b.asInstanceOf[FinalPayload.Standard].paymentSecret == paymentSecret) + } + test("build a trampoline payment") { // simple trampoline route to e: // .--. .--. @@ -306,7 +319,7 @@ class PaymentPacketSpec extends AnyFunSuite with BeforeAndAfterAll { assert(failure == FinalIncorrectCltvExpiry(firstExpiry - CltvExpiryDelta(12))) } - test("fail to decrypt at the final trampoline node when amount has been modified by next-to-last trampoline") { + test("fail to decrypt at the final trampoline node when amount has been decreased by next-to-last trampoline") { val Success((amount_ac, expiry_ac, trampolineOnion)) = buildTrampolinePacket(paymentHash, trampolineHops, FinalPayload.Standard.createMultiPartPayload(finalAmount, finalAmount, finalExpiry, paymentSecret, None)) val Success((firstAmount, firstExpiry, onion)) = buildPaymentPacket(paymentHash, trampolineChannelHops, FinalPayload.Standard.createTrampolinePayload(amount_ac, amount_ac, expiry_ac, randomBytes32(), trampolineOnion.packet)) val Right(ChannelRelayPacket(_, _, packet_c)) = decrypt(UpdateAddHtlc(randomBytes32(), 1, firstAmount, paymentHash, firstExpiry, onion.packet, None), priv_b.privateKey, Features.empty) @@ -315,7 +328,7 @@ class PaymentPacketSpec extends AnyFunSuite with BeforeAndAfterAll { val Success((amount_d, expiry_d, onion_d)) = buildPaymentPacket(paymentHash, channelHopFromUpdate(c, d, channelUpdate_cd) :: Nil, FinalPayload.Standard.createTrampolinePayload(amount_cd, amount_cd, expiry_cd, randomBytes32(), packet_d)) val Right(NodeRelayPacket(_, _, _, packet_e)) = decrypt(UpdateAddHtlc(randomBytes32(), 3, amount_d, paymentHash, expiry_d, onion_d.packet, None), priv_d.privateKey, Features.empty) // d forwards an invalid amount to e (the outer total amount doesn't match the inner amount). - val invalidTotalAmount = amount_de + 100.msat + val invalidTotalAmount = amount_de - 1.msat val Success((amount_e, expiry_e, onion_e)) = buildPaymentPacket(paymentHash, channelHopFromUpdate(d, e, channelUpdate_de) :: Nil, FinalPayload.Standard.createTrampolinePayload(amount_de, invalidTotalAmount, expiry_de, randomBytes32(), packet_e)) val Left(failure) = decrypt(UpdateAddHtlc(randomBytes32(), 4, amount_e, paymentHash, expiry_e, onion_e.packet, None), priv_e.privateKey, Features.empty) assert(failure == FinalIncorrectHtlcAmount(invalidTotalAmount))