From 6771da0367000ef2bdff140d86d62d6e16696ce3 Mon Sep 17 00:00:00 2001 From: Bastien Teinturier Date: Mon, 16 Sep 2019 10:33:07 +0200 Subject: [PATCH 1/2] Sphinx: accept invalid downstream errors When a downstream node sends us an onion error with an invalid length, we must forward the failure. The recipient won't be able to extract the error but at least it knows the payment failed. --- .../scala/fr/acinq/eclair/crypto/Sphinx.scala | 6 ++++-- .../channel/states/e/NormalStateSpec.scala | 20 ++++++++++++++++++- .../fr/acinq/eclair/crypto/SphinxSpec.scala | 20 ++++++++++++++++--- 3 files changed, 40 insertions(+), 6 deletions(-) diff --git a/eclair-core/src/main/scala/fr/acinq/eclair/crypto/Sphinx.scala b/eclair-core/src/main/scala/fr/acinq/eclair/crypto/Sphinx.scala index 7142924b7a..d8050e6a79 100644 --- a/eclair-core/src/main/scala/fr/acinq/eclair/crypto/Sphinx.scala +++ b/eclair-core/src/main/scala/fr/acinq/eclair/crypto/Sphinx.scala @@ -318,12 +318,14 @@ object Sphinx extends Logging { * @return an encrypted failure packet that can be sent to the destination node. */ def wrap(packet: ByteVector, sharedSecret: ByteVector32): ByteVector = { - require(packet.length == PacketLength, s"invalid error packet length ${packet.length}, must be $PacketLength") + if (packet.length != PacketLength) { + logger.warn(s"invalid error packet length ${packet.length}, must be $PacketLength (malicious or buggy downstream node)") + } val key = generateKey("ammag", sharedSecret) val stream = generateStream(key, PacketLength) logger.debug(s"ammag key: $key") logger.debug(s"error stream: $stream") - packet xor stream + packet.take(PacketLength).padLeft(PacketLength) xor stream } /** diff --git a/eclair-core/src/test/scala/fr/acinq/eclair/channel/states/e/NormalStateSpec.scala b/eclair-core/src/test/scala/fr/acinq/eclair/channel/states/e/NormalStateSpec.scala index 10182a87db..7178b96380 100644 --- a/eclair-core/src/test/scala/fr/acinq/eclair/channel/states/e/NormalStateSpec.scala +++ b/eclair-core/src/test/scala/fr/acinq/eclair/channel/states/e/NormalStateSpec.scala @@ -802,7 +802,6 @@ class NormalStateSpec extends TestkitBaseClass with StateTestsHelperMethods { bob2blockchain.expectMsgType[WatchConfirmed] } - test("recv RevokeAndAck (one htlc sent)") { f => import f._ val sender = TestProbe() @@ -1329,6 +1328,25 @@ class NormalStateSpec extends TestkitBaseClass with StateTestsHelperMethods { alice2blockchain.expectMsgType[WatchConfirmed] } + test("recv UpdateFailHtlc (invalid onion error length)") { f => + import f._ + val sender = TestProbe() + val (_, htlc) = addHtlc(50000000 msat, alice, bob, alice2bob, bob2alice) + crossSign(alice, bob, alice2bob, bob2alice) + // Bob receives a failure with a completely invalid onion error (missing mac) + sender.send(bob, CMD_FAIL_HTLC(htlc.id, Left(ByteVector.fill(260)(42)))) + sender.expectMsg("ok") + val fail = bob2alice.expectMsgType[UpdateFailHtlc] + + // actual test begins + val initialState = alice.stateData.asInstanceOf[DATA_NORMAL] + bob2alice.forward(alice) + awaitCond(alice.stateData == initialState.copy( + commitments = initialState.commitments.copy(remoteChanges = initialState.commitments.remoteChanges.copy(initialState.commitments.remoteChanges.proposed :+ fail)))) + // alice won't forward the fail before it is cross-signed + relayerA.expectNoMsg() + } + test("recv CMD_UPDATE_FEE") { f => import f._ val sender = TestProbe() diff --git a/eclair-core/src/test/scala/fr/acinq/eclair/crypto/SphinxSpec.scala b/eclair-core/src/test/scala/fr/acinq/eclair/crypto/SphinxSpec.scala index 1dbdecffbe..9f08344ecf 100644 --- a/eclair-core/src/test/scala/fr/acinq/eclair/crypto/SphinxSpec.scala +++ b/eclair-core/src/test/scala/fr/acinq/eclair/crypto/SphinxSpec.scala @@ -18,16 +18,16 @@ package fr.acinq.eclair.crypto import fr.acinq.bitcoin.ByteVector32 import fr.acinq.bitcoin.Crypto.{PrivateKey, PublicKey} -import fr.acinq.eclair.{UInt64, wire} import fr.acinq.eclair.wire._ +import fr.acinq.eclair.{UInt64, wire} import org.scalatest.FunSuite import scodec.bits._ import scala.util.Success /** - * Created by fabrice on 10/01/17. - */ + * Created by fabrice on 10/01/17. + */ class SphinxSpec extends FunSuite { import Sphinx._ @@ -299,6 +299,20 @@ class SphinxSpec extends FunSuite { } } + test("intermediate node replies with an invalid onion payload length") { + // The error will not be recoverable by the sender, but we must still forward it. + val sharedSecret = ByteVector32(hex"4242424242424242424242424242424242424242424242424242424242424242") + val errors = Seq( + ByteVector.fill(FailurePacket.PacketLength - MacLength)(13), + ByteVector.fill(FailurePacket.PacketLength + MacLength)(13) + ) + + for (error <- errors) { + val wrapped = FailurePacket.wrap(error, sharedSecret) + assert(wrapped.length === FailurePacket.PacketLength) + } + } + test("intermediate node replies with a failure message (reference test vector)") { for (payloads <- Seq(referenceFixedSizePayloads, referenceVariableSizePayloads, variableSizePayloadsFull)) { // route: origin -> node #0 -> node #1 -> node #2 -> node #3 -> node #4 From adc8f8f3f51ebd0b07f437fb4a16ecade31d85b6 Mon Sep 17 00:00:00 2001 From: Bastien Teinturier Date: Tue, 17 Sep 2019 15:28:04 +0200 Subject: [PATCH 2/2] fixup! Sphinx: accept invalid downstream errors When a downstream node sends us an onion error with an invalid length, we must forward the failure. The recipient won't be able to extract the error but at least it knows the payment failed. --- .../main/scala/fr/acinq/eclair/crypto/Sphinx.scala | 2 ++ .../eclair/channel/states/e/NormalStateSpec.scala | 11 +++-------- 2 files changed, 5 insertions(+), 8 deletions(-) diff --git a/eclair-core/src/main/scala/fr/acinq/eclair/crypto/Sphinx.scala b/eclair-core/src/main/scala/fr/acinq/eclair/crypto/Sphinx.scala index d8050e6a79..ddeaec963b 100644 --- a/eclair-core/src/main/scala/fr/acinq/eclair/crypto/Sphinx.scala +++ b/eclair-core/src/main/scala/fr/acinq/eclair/crypto/Sphinx.scala @@ -325,6 +325,8 @@ object Sphinx extends Logging { val stream = generateStream(key, PacketLength) logger.debug(s"ammag key: $key") logger.debug(s"error stream: $stream") + // If we received a packet with an invalid length, we trim and pad to forward a packet with a normal length upstream. + // This is a poor man's attempt at increasing the likelihood of the sender receiving the error. packet.take(PacketLength).padLeft(PacketLength) xor stream } diff --git a/eclair-core/src/test/scala/fr/acinq/eclair/channel/states/e/NormalStateSpec.scala b/eclair-core/src/test/scala/fr/acinq/eclair/channel/states/e/NormalStateSpec.scala index 7178b96380..b235d19c2a 100644 --- a/eclair-core/src/test/scala/fr/acinq/eclair/channel/states/e/NormalStateSpec.scala +++ b/eclair-core/src/test/scala/fr/acinq/eclair/channel/states/e/NormalStateSpec.scala @@ -1337,14 +1337,9 @@ class NormalStateSpec extends TestkitBaseClass with StateTestsHelperMethods { sender.send(bob, CMD_FAIL_HTLC(htlc.id, Left(ByteVector.fill(260)(42)))) sender.expectMsg("ok") val fail = bob2alice.expectMsgType[UpdateFailHtlc] - - // actual test begins - val initialState = alice.stateData.asInstanceOf[DATA_NORMAL] - bob2alice.forward(alice) - awaitCond(alice.stateData == initialState.copy( - commitments = initialState.commitments.copy(remoteChanges = initialState.commitments.remoteChanges.copy(initialState.commitments.remoteChanges.proposed :+ fail)))) - // alice won't forward the fail before it is cross-signed - relayerA.expectNoMsg() + assert(fail.id === htlc.id) + // We should rectify the packet length before forwarding upstream. + assert(fail.reason.length === Sphinx.FailurePacket.PacketLength) } test("recv CMD_UPDATE_FEE") { f =>