From 09cf32798ac792b8a80bc29d2072788a2173db82 Mon Sep 17 00:00:00 2001 From: Bastien Teinturier Date: Thu, 22 Aug 2019 14:14:27 +0200 Subject: [PATCH 1/3] Bolt4: remove final_expiry_too_soon error message It allowed probing attacks and the spec deprecated it in favor of IncorrectOrUnknownPaymentDetails. --- .../fr/acinq/eclair/payment/Autoprobe.scala | 2 +- .../eclair/payment/LocalPaymentHandler.scala | 12 +++++------ .../fr/acinq/eclair/wire/FailureMessage.scala | 6 ++++-- .../channel/states/e/OfflineStateSpec.scala | 2 +- .../eclair/integration/IntegrationSpec.scala | 6 +++--- .../eclair/payment/PaymentHandlerSpec.scala | 6 +++--- .../wire/FailureMessageCodecsSpec.scala | 20 +++++++++++++++++-- 7 files changed, 36 insertions(+), 18 deletions(-) diff --git a/eclair-core/src/main/scala/fr/acinq/eclair/payment/Autoprobe.scala b/eclair-core/src/main/scala/fr/acinq/eclair/payment/Autoprobe.scala index c00f627e6d..2494ad3ffc 100644 --- a/eclair-core/src/main/scala/fr/acinq/eclair/payment/Autoprobe.scala +++ b/eclair-core/src/main/scala/fr/acinq/eclair/payment/Autoprobe.scala @@ -62,7 +62,7 @@ class Autoprobe(nodeParams: NodeParams, router: ActorRef, paymentInitiator: Acto case paymentResult: PaymentResult => paymentResult match { - case PaymentFailed(_, _, _ :+ RemoteFailure(_, DecryptedFailurePacket(targetNodeId, IncorrectOrUnknownPaymentDetails(_)))) => + case PaymentFailed(_, _, _ :+ RemoteFailure(_, DecryptedFailurePacket(targetNodeId, IncorrectOrUnknownPaymentDetails(_, _)))) => log.info(s"payment probe successful to node=$targetNodeId") case _ => log.info(s"payment probe failed with paymentResult=$paymentResult") diff --git a/eclair-core/src/main/scala/fr/acinq/eclair/payment/LocalPaymentHandler.scala b/eclair-core/src/main/scala/fr/acinq/eclair/payment/LocalPaymentHandler.scala index 9bc6e425d0..1e43a04392 100644 --- a/eclair-core/src/main/scala/fr/acinq/eclair/payment/LocalPaymentHandler.scala +++ b/eclair-core/src/main/scala/fr/acinq/eclair/payment/LocalPaymentHandler.scala @@ -22,7 +22,7 @@ import fr.acinq.eclair.channel.{CMD_FAIL_HTLC, CMD_FULFILL_HTLC, Channel} import fr.acinq.eclair.db.IncomingPayment import fr.acinq.eclair.payment.PaymentLifecycle.ReceivePayment import fr.acinq.eclair.wire._ -import fr.acinq.eclair.{NodeParams, randomBytes32} +import fr.acinq.eclair.{Globals, NodeParams, randomBytes32} import scala.compat.Platform import scala.concurrent.ExecutionContext @@ -65,15 +65,15 @@ class LocalPaymentHandler(nodeParams: NodeParams) extends Actor with ActorLoggin // see https://github.com/lightningnetwork/lightning-rfc/blob/master/04-onion-routing.md#failure-messages paymentRequest.amount match { case _ if paymentRequest.isExpired => - sender ! CMD_FAIL_HTLC(htlc.id, Right(IncorrectOrUnknownPaymentDetails(htlc.amountMsat)), commit = true) + sender ! CMD_FAIL_HTLC(htlc.id, Right(IncorrectOrUnknownPaymentDetails(htlc.amountMsat, Globals.blockCount.get())), commit = true) case _ if htlc.cltvExpiry < minFinalExpiry => - sender ! CMD_FAIL_HTLC(htlc.id, Right(FinalExpiryTooSoon), commit = true) + sender ! CMD_FAIL_HTLC(htlc.id, Right(IncorrectOrUnknownPaymentDetails(htlc.amountMsat, Globals.blockCount.get())), commit = true) case Some(amount) if htlc.amountMsat < amount => log.warning(s"received payment with amount too small for paymentHash=${htlc.paymentHash} amount=${htlc.amountMsat}") - sender ! CMD_FAIL_HTLC(htlc.id, Right(IncorrectOrUnknownPaymentDetails(htlc.amountMsat)), commit = true) + sender ! CMD_FAIL_HTLC(htlc.id, Right(IncorrectOrUnknownPaymentDetails(htlc.amountMsat, Globals.blockCount.get())), commit = true) case Some(amount) if htlc.amountMsat > amount * 2 => log.warning(s"received payment with amount too large for paymentHash=${htlc.paymentHash} amount=${htlc.amountMsat}") - sender ! CMD_FAIL_HTLC(htlc.id, Right(IncorrectOrUnknownPaymentDetails(htlc.amountMsat)), commit = true) + sender ! CMD_FAIL_HTLC(htlc.id, Right(IncorrectOrUnknownPaymentDetails(htlc.amountMsat, Globals.blockCount.get())), commit = true) case _ => log.info(s"received payment for paymentHash=${htlc.paymentHash} amount=${htlc.amountMsat}") // amount is correct or was not specified in the payment request @@ -82,7 +82,7 @@ class LocalPaymentHandler(nodeParams: NodeParams) extends Actor with ActorLoggin context.system.eventStream.publish(PaymentReceived(htlc.amountMsat, htlc.paymentHash, htlc.channelId)) } case None => - sender ! CMD_FAIL_HTLC(htlc.id, Right(IncorrectOrUnknownPaymentDetails(htlc.amountMsat)), commit = true) + sender ! CMD_FAIL_HTLC(htlc.id, Right(IncorrectOrUnknownPaymentDetails(htlc.amountMsat, Globals.blockCount.get())), commit = true) } } diff --git a/eclair-core/src/main/scala/fr/acinq/eclair/wire/FailureMessage.scala b/eclair-core/src/main/scala/fr/acinq/eclair/wire/FailureMessage.scala index 63a9dd6113..17a19bb8b5 100644 --- a/eclair-core/src/main/scala/fr/acinq/eclair/wire/FailureMessage.scala +++ b/eclair-core/src/main/scala/fr/acinq/eclair/wire/FailureMessage.scala @@ -52,9 +52,11 @@ case class AmountBelowMinimum(amount: MilliSatoshi, update: ChannelUpdate) exten case class FeeInsufficient(amount: MilliSatoshi, update: ChannelUpdate) extends Update { def message = s"payment fee was below the minimum required by the channel" } case class ChannelDisabled(messageFlags: Byte, channelFlags: Byte, update: ChannelUpdate) extends Update { def message = "channel is currently disabled" } case class IncorrectCltvExpiry(expiry: CltvExpiry, update: ChannelUpdate) extends Update { def message = "payment expiry doesn't match the value in the onion" } -case class IncorrectOrUnknownPaymentDetails(amount: MilliSatoshi) extends Perm { def message = "incorrect payment amount or unknown payment hash" } +case class IncorrectOrUnknownPaymentDetails(amount: MilliSatoshi, height: Long) extends Perm { def message = "incorrect payment details or unknown payment hash" } +/** Deprecated: this failure code allows probing attacks: IncorrectOrUnknownPaymentDetails should be used instead. */ case object IncorrectPaymentAmount extends Perm { def message = "payment amount is incorrect" } case class ExpiryTooSoon(update: ChannelUpdate) extends Update { def message = "payment expiry is too close to the current block height for safe handling by the relaying node" } +/** Deprecated: this failure code allows probing attacks: IncorrectOrUnknownPaymentDetails should be used instead. */ case object FinalExpiryTooSoon extends FailureMessage { def message = "payment expiry is too close to the current block height for safe handling by the final node" } case class FinalIncorrectCltvExpiry(expiry: CltvExpiry) extends FailureMessage { def message = "payment expiry doesn't match the value in the onion" } case class FinalIncorrectHtlcAmount(amount: MilliSatoshi) extends FailureMessage { def message = "payment amount is incorrect in the final htlc" } @@ -91,7 +93,7 @@ object FailureMessageCodecs { .typecase(UPDATE | 13, (("expiry" | cltvExpiry) :: ("channelUpdate" | channelUpdateWithLengthCodec)).as[IncorrectCltvExpiry]) .typecase(UPDATE | 14, ("channelUpdate" | channelUpdateWithLengthCodec).as[ExpiryTooSoon]) .typecase(UPDATE | 20, (("messageFlags" | byte) :: ("channelFlags" | byte) :: ("channelUpdate" | channelUpdateWithLengthCodec)).as[ChannelDisabled]) - .typecase(PERM | 15, ("amountMsat" | withDefaultValue(optional(bitsRemaining, millisatoshi), 0 msat)).as[IncorrectOrUnknownPaymentDetails]) + .typecase(PERM | 15, (("amountMsat" | withDefaultValue(optional(bitsRemaining, millisatoshi), 0 msat)) :: ("height" | withDefaultValue(optional(bitsRemaining, uint32), 0L))).as[IncorrectOrUnknownPaymentDetails]) .typecase(PERM | 16, provide(IncorrectPaymentAmount)) .typecase(17, provide(FinalExpiryTooSoon)) .typecase(18, ("expiry" | cltvExpiry).as[FinalIncorrectCltvExpiry]) diff --git a/eclair-core/src/test/scala/fr/acinq/eclair/channel/states/e/OfflineStateSpec.scala b/eclair-core/src/test/scala/fr/acinq/eclair/channel/states/e/OfflineStateSpec.scala index c02f85f58a..00965f35fc 100644 --- a/eclair-core/src/test/scala/fr/acinq/eclair/channel/states/e/OfflineStateSpec.scala +++ b/eclair-core/src/test/scala/fr/acinq/eclair/channel/states/e/OfflineStateSpec.scala @@ -452,7 +452,7 @@ class OfflineStateSpec extends TestkitBaseClass with StateTestsHelperMethods { // We simulate a pending failure on that HTLC. // Even if we get close to expiring upstream we shouldn't close the channel, because we have nothing to lose. - sender.send(commandBuffer, CommandSend(htlc.channelId, htlc.id, CMD_FAIL_HTLC(htlc.id, Right(IncorrectOrUnknownPaymentDetails(0 msat))))) + sender.send(commandBuffer, CommandSend(htlc.channelId, htlc.id, CMD_FAIL_HTLC(htlc.id, Right(IncorrectOrUnknownPaymentDetails(0 msat, 0))))) sender.send(bob, CurrentBlockCount((htlc.cltvExpiry - bob.underlyingActor.nodeParams.fulfillSafetyBeforeTimeoutBlocks).toLong)) bob2blockchain.expectNoMsg(250 millis) diff --git a/eclair-core/src/test/scala/fr/acinq/eclair/integration/IntegrationSpec.scala b/eclair-core/src/test/scala/fr/acinq/eclair/integration/IntegrationSpec.scala index b08b870e69..2a6db6c66e 100644 --- a/eclair-core/src/test/scala/fr/acinq/eclair/integration/IntegrationSpec.scala +++ b/eclair-core/src/test/scala/fr/acinq/eclair/integration/IntegrationSpec.scala @@ -345,7 +345,7 @@ class IntegrationSpec extends TestKit(ActorSystem("test")) with BitcoindService assert(failed.id == paymentId) assert(failed.paymentHash === pr.paymentHash) assert(failed.failures.size === 1) - assert(failed.failures.head.asInstanceOf[RemoteFailure].e === DecryptedFailurePacket(nodes("D").nodeParams.nodeId, IncorrectOrUnknownPaymentDetails(100000000 msat))) + assert(failed.failures.head.asInstanceOf[RemoteFailure].e === DecryptedFailurePacket(nodes("D").nodeParams.nodeId, IncorrectOrUnknownPaymentDetails(100000000 msat, Globals.blockCount.get()))) } test("send an HTLC A->D with a lower amount than requested") { @@ -365,7 +365,7 @@ class IntegrationSpec extends TestKit(ActorSystem("test")) with BitcoindService assert(failed.id == paymentId) assert(failed.paymentHash === pr.paymentHash) assert(failed.failures.size === 1) - assert(failed.failures.head.asInstanceOf[RemoteFailure].e === DecryptedFailurePacket(nodes("D").nodeParams.nodeId, IncorrectOrUnknownPaymentDetails(100000000 msat))) + assert(failed.failures.head.asInstanceOf[RemoteFailure].e === DecryptedFailurePacket(nodes("D").nodeParams.nodeId, IncorrectOrUnknownPaymentDetails(100000000 msat, Globals.blockCount.get()))) } test("send an HTLC A->D with too much overpayment") { @@ -385,7 +385,7 @@ class IntegrationSpec extends TestKit(ActorSystem("test")) with BitcoindService assert(paymentId == failed.id) assert(failed.paymentHash === pr.paymentHash) assert(failed.failures.size === 1) - assert(failed.failures.head.asInstanceOf[RemoteFailure].e === DecryptedFailurePacket(nodes("D").nodeParams.nodeId, IncorrectOrUnknownPaymentDetails(600000000 msat))) + assert(failed.failures.head.asInstanceOf[RemoteFailure].e === DecryptedFailurePacket(nodes("D").nodeParams.nodeId, IncorrectOrUnknownPaymentDetails(600000000 msat, Globals.blockCount.get()))) } test("send an HTLC A->D with a reasonable overpayment") { diff --git a/eclair-core/src/test/scala/fr/acinq/eclair/payment/PaymentHandlerSpec.scala b/eclair-core/src/test/scala/fr/acinq/eclair/payment/PaymentHandlerSpec.scala index 63576b4368..294be04631 100644 --- a/eclair-core/src/test/scala/fr/acinq/eclair/payment/PaymentHandlerSpec.scala +++ b/eclair-core/src/test/scala/fr/acinq/eclair/payment/PaymentHandlerSpec.scala @@ -24,8 +24,8 @@ import fr.acinq.eclair.TestConstants.Alice import fr.acinq.eclair.channel.{CMD_FAIL_HTLC, CMD_FULFILL_HTLC} import fr.acinq.eclair.payment.PaymentLifecycle.ReceivePayment import fr.acinq.eclair.payment.PaymentRequest.ExtraHop -import fr.acinq.eclair.wire.{FinalExpiryTooSoon, UpdateAddHtlc} -import fr.acinq.eclair.{CltvExpiryDelta, LongToBtcAmount, ShortChannelId, TestConstants, randomKey} +import fr.acinq.eclair.wire.{FinalExpiryTooSoon, IncorrectOrUnknownPaymentDetails, UpdateAddHtlc} +import fr.acinq.eclair.{CltvExpiryDelta, Globals, LongToBtcAmount, ShortChannelId, TestConstants, randomKey} import org.scalatest.FunSuiteLike import scodec.bits.ByteVector @@ -83,7 +83,7 @@ class PaymentHandlerSpec extends TestKit(ActorSystem("test")) with FunSuiteLike val add = UpdateAddHtlc(ByteVector32(ByteVector.fill(32)(1)), 0, amountMsat, pr.paymentHash, cltvExpiry = CltvExpiryDelta(3).toCltvExpiry, TestConstants.emptyOnionPacket) sender.send(handler, add) - assert(sender.expectMsgType[CMD_FAIL_HTLC].reason == Right(FinalExpiryTooSoon)) + assert(sender.expectMsgType[CMD_FAIL_HTLC].reason == Right(IncorrectOrUnknownPaymentDetails(amountMsat, Globals.blockCount.get()))) eventListener.expectNoMsg(300 milliseconds) assert(nodeParams.db.payments.getIncomingPayment(pr.paymentHash).isEmpty) } diff --git a/eclair-core/src/test/scala/fr/acinq/eclair/wire/FailureMessageCodecsSpec.scala b/eclair-core/src/test/scala/fr/acinq/eclair/wire/FailureMessageCodecsSpec.scala index 615a7f1adf..f70400869c 100644 --- a/eclair-core/src/test/scala/fr/acinq/eclair/wire/FailureMessageCodecsSpec.scala +++ b/eclair-core/src/test/scala/fr/acinq/eclair/wire/FailureMessageCodecsSpec.scala @@ -47,7 +47,7 @@ class FailureMessageCodecsSpec extends FunSuite { InvalidOnionVersion(randomBytes32) :: InvalidOnionHmac(randomBytes32) :: InvalidOnionKey(randomBytes32) :: InvalidOnionPayload(randomBytes32) :: TemporaryChannelFailure(channelUpdate) :: PermanentChannelFailure :: RequiredChannelFeatureMissing :: UnknownNextPeer :: AmountBelowMinimum(123456 msat, channelUpdate) :: FeeInsufficient(546463 msat, channelUpdate) :: IncorrectCltvExpiry(CltvExpiry(1211), channelUpdate) :: ExpiryTooSoon(channelUpdate) :: - IncorrectOrUnknownPaymentDetails(123456 msat) :: IncorrectPaymentAmount :: FinalExpiryTooSoon :: FinalIncorrectCltvExpiry(CltvExpiry(1234)) :: ChannelDisabled(0, 1, channelUpdate) :: ExpiryTooFar :: Nil + IncorrectOrUnknownPaymentDetails(123456 msat, 1105) :: IncorrectPaymentAmount :: FinalExpiryTooSoon :: FinalIncorrectCltvExpiry(CltvExpiry(1234)) :: ChannelDisabled(0, 1, channelUpdate) :: ExpiryTooFar :: Nil msgs.foreach { msg => { @@ -75,7 +75,7 @@ class FailureMessageCodecsSpec extends FunSuite { val codec = failureOnionCodec(Hmac256(ByteVector32.Zeroes)) val testCases = Map( InvalidOnionKey(ByteVector32(hex"2a2a2a2a2a2a2a2a2a2a2a2a2a2a2a2a2a2a2a2a2a2a2a2a2a2a2a2a2a2a2a2a")) -> hex"41a824e2d630111669fa3e52b600a518f369691909b4e89205dc624ee17ed2c1 0022 c006 2a2a2a2a2a2a2a2a2a2a2a2a2a2a2a2a2a2a2a2a2a2a2a2a2a2a2a2a2a2a2a2a 00de 000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000", - IncorrectOrUnknownPaymentDetails(42 msat) -> hex"ba6e122b2941619e2106e8437bf525356ffc8439ac3b2245f68546e298a08cc6 000a 400f 000000000000002a 00f6 000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000" + IncorrectOrUnknownPaymentDetails(42 msat, 1105) -> hex"5eb766da1b2f45b4182e064dacd8da9eca2c9a33f0dce363ff308e9bdb3ee4e3 000e 400f 000000000000002a 00000451 00f2 0000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000" ) for ((expected, bin) <- testCases) { @@ -87,6 +87,22 @@ class FailureMessageCodecsSpec extends FunSuite { } } + test("decode backwards-compatible IncorrectOrUnknownPaymentDetails") { + val codec = failureOnionCodec(Hmac256(ByteVector32.Zeroes)) + val testCases = Map( + // Without any data. + IncorrectOrUnknownPaymentDetails(MilliSatoshi(0), 0) -> hex"0d83b55dd5a6086e4033c3659125ed1ff436964ce0e67ed5a03bddb16a9a1041 0002 400f 00fe 0000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000", + // With an amount but no height. + IncorrectOrUnknownPaymentDetails(MilliSatoshi(42), 0) -> hex"ba6e122b2941619e2106e8437bf525356ffc8439ac3b2245f68546e298a08cc6 000a 400f 000000000000002a 00f6 000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000", + // With amount and height. + IncorrectOrUnknownPaymentDetails(MilliSatoshi(42), 1105) -> hex"5eb766da1b2f45b4182e064dacd8da9eca2c9a33f0dce363ff308e9bdb3ee4e3 000e 400f 000000000000002a 00000451 00f2 0000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000" + ) + + for ((expected, bin) <- testCases) { + assert(codec.decode(bin.bits).require.value === expected) + } + } + test("decode invalid failure onion packet") { val codec = failureOnionCodec(Hmac256(ByteVector32.Zeroes)) val testCases = Seq( From a802c7b00e32dd87a518728c5291f8a358e5cb57 Mon Sep 17 00:00:00 2001 From: Bastien Teinturier Date: Fri, 30 Aug 2019 16:43:16 +0200 Subject: [PATCH 2/3] Allow unknown failure messages. Extract failure code to test PERM and NODE bits. --- .../fr/acinq/eclair/payment/Relayer.scala | 2 +- .../fr/acinq/eclair/wire/FailureMessage.scala | 95 ++++++++++++------- .../eclair/payment/PaymentHandlerSpec.scala | 2 +- .../eclair/payment/PaymentLifecycleSpec.scala | 14 ++- .../wire/FailureMessageCodecsSpec.scala | 27 +++++- 5 files changed, 96 insertions(+), 44 deletions(-) diff --git a/eclair-core/src/main/scala/fr/acinq/eclair/payment/Relayer.scala b/eclair-core/src/main/scala/fr/acinq/eclair/payment/Relayer.scala index 8bd3959e94..673da84e34 100644 --- a/eclair-core/src/main/scala/fr/acinq/eclair/payment/Relayer.scala +++ b/eclair-core/src/main/scala/fr/acinq/eclair/payment/Relayer.scala @@ -120,7 +120,7 @@ class Relayer(nodeParams: NodeParams, register: ActorRef, paymentHandler: ActorR } case Left(badOnion) => log.warning(s"couldn't parse onion: reason=${badOnion.message}") - val cmdFail = CMD_FAIL_MALFORMED_HTLC(add.id, badOnion.onionHash, FailureMessageCodecs.failureCode(badOnion), commit = true) + val cmdFail = CMD_FAIL_MALFORMED_HTLC(add.id, badOnion.onionHash, badOnion.code, commit = true) log.info(s"rejecting htlc #${add.id} paymentHash=${add.paymentHash} from channelId=${add.channelId} reason=malformed onionHash=${cmdFail.onionHash} failureCode=${cmdFail.failureCode}") commandBuffer ! CommandBuffer.CommandSend(add.channelId, add.id, cmdFail) } diff --git a/eclair-core/src/main/scala/fr/acinq/eclair/wire/FailureMessage.scala b/eclair-core/src/main/scala/fr/acinq/eclair/wire/FailureMessage.scala index 17a19bb8b5..4b34a1fbe0 100644 --- a/eclair-core/src/main/scala/fr/acinq/eclair/wire/FailureMessage.scala +++ b/eclair-core/src/main/scala/fr/acinq/eclair/wire/FailureMessage.scala @@ -18,7 +18,8 @@ package fr.acinq.eclair.wire import fr.acinq.bitcoin.ByteVector32 import fr.acinq.eclair.crypto.Mac32 -import fr.acinq.eclair.wire.CommonCodecs.{cltvExpiry, millisatoshi, sha256} +import fr.acinq.eclair.wire.CommonCodecs.{cltvExpiry, discriminatorWithDefault, millisatoshi, sha256} +import fr.acinq.eclair.wire.FailureMessageCodecs.failureMessageCodec import fr.acinq.eclair.wire.LightningMessageCodecs.{channelUpdateCodec, lightningMessageCodec} import fr.acinq.eclair.{CltvExpiry, LongToBtcAmount, MilliSatoshi} import scodec.codecs._ @@ -30,7 +31,12 @@ import scodec.{Attempt, Codec} */ // @formatter:off -sealed trait FailureMessage { def message: String } +sealed trait FailureMessage { + def message: String + // We actually encode the failure message, which is a bit clunky and not particularly efficient. + // It would be nice to be able to get that value from the discriminated codec directly. + lazy val code: Int = failureMessageCodec.encode(this).flatMap(uint16.decode).require.value +} sealed trait BadOnion extends FailureMessage { def onionHash: ByteVector32 } sealed trait Perm extends FailureMessage sealed trait Node extends FailureMessage @@ -53,14 +59,24 @@ case class FeeInsufficient(amount: MilliSatoshi, update: ChannelUpdate) extends case class ChannelDisabled(messageFlags: Byte, channelFlags: Byte, update: ChannelUpdate) extends Update { def message = "channel is currently disabled" } case class IncorrectCltvExpiry(expiry: CltvExpiry, update: ChannelUpdate) extends Update { def message = "payment expiry doesn't match the value in the onion" } case class IncorrectOrUnknownPaymentDetails(amount: MilliSatoshi, height: Long) extends Perm { def message = "incorrect payment details or unknown payment hash" } -/** Deprecated: this failure code allows probing attacks: IncorrectOrUnknownPaymentDetails should be used instead. */ -case object IncorrectPaymentAmount extends Perm { def message = "payment amount is incorrect" } case class ExpiryTooSoon(update: ChannelUpdate) extends Update { def message = "payment expiry is too close to the current block height for safe handling by the relaying node" } -/** Deprecated: this failure code allows probing attacks: IncorrectOrUnknownPaymentDetails should be used instead. */ -case object FinalExpiryTooSoon extends FailureMessage { def message = "payment expiry is too close to the current block height for safe handling by the final node" } case class FinalIncorrectCltvExpiry(expiry: CltvExpiry) extends FailureMessage { def message = "payment expiry doesn't match the value in the onion" } case class FinalIncorrectHtlcAmount(amount: MilliSatoshi) extends FailureMessage { def message = "payment amount is incorrect in the final htlc" } case object ExpiryTooFar extends FailureMessage { def message = "payment expiry is too far in the future" } + +/** + * We allow remote nodes to send us unknown failure codes (e.g. deprecated failure codes). + * By reading the PERM and NODE bits we can still extract useful information for payment retry even without knowing how + * to decode the failure payload (but we can't extract a channel update or onion hash). + */ +sealed trait UnknownFailureMessage extends FailureMessage { + def message = "unknown failure message" + override def toString = s"$message ($code)" + override def equals(obj: Any): Boolean = obj match { + case f: UnknownFailureMessage => f.code == code + case _ => false + } +} // @formatter:on object FailureMessageCodecs { @@ -75,36 +91,43 @@ object FailureMessageCodecs { // this codec supports both versions for decoding, and will encode with the message type val channelUpdateWithLengthCodec = variableSizeBytes(uint16, choice(channelUpdateCodecWithType, channelUpdateCodec)) - val failureMessageCodec = discriminated[FailureMessage].by(uint16) - .typecase(PERM | 1, provide(InvalidRealm)) - .typecase(NODE | 2, provide(TemporaryNodeFailure)) - .typecase(PERM | 2, provide(PermanentNodeFailure)) - .typecase(PERM | NODE | 3, provide(RequiredNodeFeatureMissing)) - .typecase(BADONION | PERM, sha256.as[InvalidOnionPayload]) - .typecase(BADONION | PERM | 4, sha256.as[InvalidOnionVersion]) - .typecase(BADONION | PERM | 5, sha256.as[InvalidOnionHmac]) - .typecase(BADONION | PERM | 6, sha256.as[InvalidOnionKey]) - .typecase(UPDATE | 7, ("channelUpdate" | channelUpdateWithLengthCodec).as[TemporaryChannelFailure]) - .typecase(PERM | 8, provide(PermanentChannelFailure)) - .typecase(PERM | 9, provide(RequiredChannelFeatureMissing)) - .typecase(PERM | 10, provide(UnknownNextPeer)) - .typecase(UPDATE | 11, (("amountMsat" | millisatoshi) :: ("channelUpdate" | channelUpdateWithLengthCodec)).as[AmountBelowMinimum]) - .typecase(UPDATE | 12, (("amountMsat" | millisatoshi) :: ("channelUpdate" | channelUpdateWithLengthCodec)).as[FeeInsufficient]) - .typecase(UPDATE | 13, (("expiry" | cltvExpiry) :: ("channelUpdate" | channelUpdateWithLengthCodec)).as[IncorrectCltvExpiry]) - .typecase(UPDATE | 14, ("channelUpdate" | channelUpdateWithLengthCodec).as[ExpiryTooSoon]) - .typecase(UPDATE | 20, (("messageFlags" | byte) :: ("channelFlags" | byte) :: ("channelUpdate" | channelUpdateWithLengthCodec)).as[ChannelDisabled]) - .typecase(PERM | 15, (("amountMsat" | withDefaultValue(optional(bitsRemaining, millisatoshi), 0 msat)) :: ("height" | withDefaultValue(optional(bitsRemaining, uint32), 0L))).as[IncorrectOrUnknownPaymentDetails]) - .typecase(PERM | 16, provide(IncorrectPaymentAmount)) - .typecase(17, provide(FinalExpiryTooSoon)) - .typecase(18, ("expiry" | cltvExpiry).as[FinalIncorrectCltvExpiry]) - .typecase(19, ("amountMsat" | millisatoshi).as[FinalIncorrectHtlcAmount]) - .typecase(21, provide(ExpiryTooFar)) - - /** - * Return the failure code for a given failure message. This method actually encodes the failure message, which is a - * bit clunky and not particularly efficient. It shouldn't be used on the application's hot path. - */ - def failureCode(failure: FailureMessage): Int = failureMessageCodec.encode(failure).flatMap(uint16.decode).require.value + val failureMessageCodec = discriminatorWithDefault( + discriminated[FailureMessage].by(uint16) + .typecase(PERM | 1, provide(InvalidRealm)) + .typecase(NODE | 2, provide(TemporaryNodeFailure)) + .typecase(PERM | NODE | 2, provide(PermanentNodeFailure)) + .typecase(PERM | NODE | 3, provide(RequiredNodeFeatureMissing)) + .typecase(BADONION | PERM, sha256.as[InvalidOnionPayload]) + .typecase(BADONION | PERM | 4, sha256.as[InvalidOnionVersion]) + .typecase(BADONION | PERM | 5, sha256.as[InvalidOnionHmac]) + .typecase(BADONION | PERM | 6, sha256.as[InvalidOnionKey]) + .typecase(UPDATE | 7, ("channelUpdate" | channelUpdateWithLengthCodec).as[TemporaryChannelFailure]) + .typecase(PERM | 8, provide(PermanentChannelFailure)) + .typecase(PERM | 9, provide(RequiredChannelFeatureMissing)) + .typecase(PERM | 10, provide(UnknownNextPeer)) + .typecase(UPDATE | 11, (("amountMsat" | millisatoshi) :: ("channelUpdate" | channelUpdateWithLengthCodec)).as[AmountBelowMinimum]) + .typecase(UPDATE | 12, (("amountMsat" | millisatoshi) :: ("channelUpdate" | channelUpdateWithLengthCodec)).as[FeeInsufficient]) + .typecase(UPDATE | 13, (("expiry" | cltvExpiry) :: ("channelUpdate" | channelUpdateWithLengthCodec)).as[IncorrectCltvExpiry]) + .typecase(UPDATE | 14, ("channelUpdate" | channelUpdateWithLengthCodec).as[ExpiryTooSoon]) + .typecase(UPDATE | 20, (("messageFlags" | byte) :: ("channelFlags" | byte) :: ("channelUpdate" | channelUpdateWithLengthCodec)).as[ChannelDisabled]) + .typecase(PERM | 15, (("amountMsat" | withDefaultValue(optional(bitsRemaining, millisatoshi), 0 msat)) :: ("height" | withDefaultValue(optional(bitsRemaining, uint32), 0L))).as[IncorrectOrUnknownPaymentDetails]) + // PERM | 16 (incorrect_payment_amount) has been deprecated because it allowed probing attacks: IncorrectOrUnknownPaymentDetails should be used instead. + // PERM | 17 (final_expiry_too_soon) has been deprecated because it allowed probing attacks: IncorrectOrUnknownPaymentDetails should be used instead. + .typecase(18, ("expiry" | cltvExpiry).as[FinalIncorrectCltvExpiry]) + .typecase(19, ("amountMsat" | millisatoshi).as[FinalIncorrectHtlcAmount]) + .typecase(21, provide(ExpiryTooFar)), + uint16.xmap(code => { + val failureMessage = code match { + // @formatter:off + case fc if (fc & PERM) != 0 && (fc & NODE) != 0 => new UnknownFailureMessage with Perm with Node { override lazy val code = fc } + case fc if (fc & NODE) != 0 => new UnknownFailureMessage with Node { override lazy val code = fc } + case fc if (fc & PERM) != 0 => new UnknownFailureMessage with Perm { override lazy val code = fc } + case fc => new UnknownFailureMessage { override lazy val code = fc } + // @formatter:on + } + failureMessage.asInstanceOf[FailureMessage] + }, (_: FailureMessage).code) + ) /** * An onion-encrypted failure from an intermediate node: diff --git a/eclair-core/src/test/scala/fr/acinq/eclair/payment/PaymentHandlerSpec.scala b/eclair-core/src/test/scala/fr/acinq/eclair/payment/PaymentHandlerSpec.scala index 294be04631..e44b8d8a05 100644 --- a/eclair-core/src/test/scala/fr/acinq/eclair/payment/PaymentHandlerSpec.scala +++ b/eclair-core/src/test/scala/fr/acinq/eclair/payment/PaymentHandlerSpec.scala @@ -24,7 +24,7 @@ import fr.acinq.eclair.TestConstants.Alice import fr.acinq.eclair.channel.{CMD_FAIL_HTLC, CMD_FULFILL_HTLC} import fr.acinq.eclair.payment.PaymentLifecycle.ReceivePayment import fr.acinq.eclair.payment.PaymentRequest.ExtraHop -import fr.acinq.eclair.wire.{FinalExpiryTooSoon, IncorrectOrUnknownPaymentDetails, UpdateAddHtlc} +import fr.acinq.eclair.wire.{IncorrectOrUnknownPaymentDetails, UpdateAddHtlc} import fr.acinq.eclair.{CltvExpiryDelta, Globals, LongToBtcAmount, ShortChannelId, TestConstants, randomKey} import org.scalatest.FunSuiteLike import scodec.bits.ByteVector 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 feb6b560ea..b36f29d777 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 @@ -327,7 +327,7 @@ class PaymentLifecycleSpec extends BaseRouterSpec { awaitCond(paymentDb.getOutgoingPayment(id).exists(_.status == OutgoingPaymentStatus.FAILED)) } - test("payment failed (PermanentChannelFailure)") { fixture => + def testPermanentFailure(fixture: FixtureParam, failure: FailureMessage): Unit = { import fixture._ val nodeParams = TestConstants.Alice.nodeParams.copy(keyManager = testKeyManager) val paymentDb = nodeParams.db.payments @@ -351,8 +351,6 @@ class PaymentLifecycleSpec extends BaseRouterSpec { awaitCond(paymentFSM.stateName == WAITING_FOR_PAYMENT_COMPLETE) val WaitingForComplete(_, _, cmd1, Nil, sharedSecrets1, _, _, hops) = paymentFSM.stateData - val failure = PermanentChannelFailure - relayer.expectMsg(ForwardShortId(channelId_ab, cmd1)) sender.send(paymentFSM, UpdateFailHtlc(ByteVector32.Zeroes, 0, Sphinx.FailurePacket.create(sharedSecrets1.head._1, failure))) @@ -366,6 +364,16 @@ class PaymentLifecycleSpec extends BaseRouterSpec { awaitCond(paymentDb.getOutgoingPayment(id).exists(_.status == OutgoingPaymentStatus.FAILED)) } + test("payment failed (PermanentChannelFailure)") { fixture => + testPermanentFailure(fixture, PermanentChannelFailure) + } + + test("payment failed (deprecated permanent failure)") { fixture => + import scodec.bits.HexStringSyntax + // PERM | 17 (final_expiry_too_soon) has been deprecated but older nodes might still use it. + testPermanentFailure(fixture, FailureMessageCodecs.failureMessageCodec.decode(hex"4011".bits).require.value) + } + test("payment succeeded") { fixture => import fixture._ val defaultPaymentHash = randomBytes32 diff --git a/eclair-core/src/test/scala/fr/acinq/eclair/wire/FailureMessageCodecsSpec.scala b/eclair-core/src/test/scala/fr/acinq/eclair/wire/FailureMessageCodecsSpec.scala index f70400869c..30ea734322 100644 --- a/eclair-core/src/test/scala/fr/acinq/eclair/wire/FailureMessageCodecsSpec.scala +++ b/eclair-core/src/test/scala/fr/acinq/eclair/wire/FailureMessageCodecsSpec.scala @@ -41,13 +41,13 @@ class FailureMessageCodecsSpec extends FunSuite { feeProportionalMillionths = 76, htlcMaximumMsat = None) - test("encode/decode all channel messages") { + test("encode/decode all failure messages") { val msgs: List[FailureMessage] = InvalidRealm :: TemporaryNodeFailure :: PermanentNodeFailure :: RequiredNodeFeatureMissing :: InvalidOnionVersion(randomBytes32) :: InvalidOnionHmac(randomBytes32) :: InvalidOnionKey(randomBytes32) :: InvalidOnionPayload(randomBytes32) :: TemporaryChannelFailure(channelUpdate) :: PermanentChannelFailure :: RequiredChannelFeatureMissing :: UnknownNextPeer :: AmountBelowMinimum(123456 msat, channelUpdate) :: FeeInsufficient(546463 msat, channelUpdate) :: IncorrectCltvExpiry(CltvExpiry(1211), channelUpdate) :: ExpiryTooSoon(channelUpdate) :: - IncorrectOrUnknownPaymentDetails(123456 msat, 1105) :: IncorrectPaymentAmount :: FinalExpiryTooSoon :: FinalIncorrectCltvExpiry(CltvExpiry(1234)) :: ChannelDisabled(0, 1, channelUpdate) :: ExpiryTooFar :: Nil + IncorrectOrUnknownPaymentDetails(123456 msat, 1105) :: FinalIncorrectCltvExpiry(CltvExpiry(1234)) :: ChannelDisabled(0, 1, channelUpdate) :: ExpiryTooFar :: Nil msgs.foreach { msg => { @@ -58,6 +58,27 @@ class FailureMessageCodecsSpec extends FunSuite { } } + test("decode unknown failure messages") { + val testCases = Seq( + // Deprecated incorrect_payment_amount. + (false, true, hex"4010"), + // Deprecated final_expiry_too_soon. + (false, true, hex"4011"), + // Unknown failure messages. + (false, false, hex"00ff 42"), + (true, false, hex"20ff 42"), + (true, true, hex"60ff 42") + ) + + for ((node, perm, bin) <- testCases) { + val decoded = failureMessageCodec.decode(bin.bits).require.value + assert(decoded.isInstanceOf[FailureMessage]) + assert(decoded.isInstanceOf[UnknownFailureMessage]) + assert(decoded.isInstanceOf[Node] === node) + assert(decoded.isInstanceOf[Perm] === perm) + } + } + test("bad onion failure code") { val msgs = Map( (BADONION | PERM | 4) -> InvalidOnionVersion(randomBytes32), @@ -67,7 +88,7 @@ class FailureMessageCodecsSpec extends FunSuite { ) for ((code, message) <- msgs) { - assert(failureCode(message) === code) + assert(message.code === code) } } From eda932ccee300e0502e3a17007a99377fd5b16aa Mon Sep 17 00:00:00 2001 From: Bastien Teinturier Date: Fri, 30 Aug 2019 18:07:47 +0200 Subject: [PATCH 3/3] fixup! Allow unknown failure messages. Extract failure code to test PERM and NODE bits. --- .../src/main/scala/fr/acinq/eclair/wire/FailureMessage.scala | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/eclair-core/src/main/scala/fr/acinq/eclair/wire/FailureMessage.scala b/eclair-core/src/main/scala/fr/acinq/eclair/wire/FailureMessage.scala index 4b34a1fbe0..b3afca1f24 100644 --- a/eclair-core/src/main/scala/fr/acinq/eclair/wire/FailureMessage.scala +++ b/eclair-core/src/main/scala/fr/acinq/eclair/wire/FailureMessage.scala @@ -71,7 +71,7 @@ case object ExpiryTooFar extends FailureMessage { def message = "payment expiry */ sealed trait UnknownFailureMessage extends FailureMessage { def message = "unknown failure message" - override def toString = s"$message ($code)" + override def toString = s"$message (${code.toHexString})" override def equals(obj: Any): Boolean = obj match { case f: UnknownFailureMessage => f.code == code case _ => false