From 33c1efcf3c11e95842ca0f319f92ea41c2982da4 Mon Sep 17 00:00:00 2001 From: sstone Date: Wed, 8 Mar 2023 15:14:34 +0100 Subject: [PATCH] Check that spent amounts and utxos are consistent before we sign a PSBT PSBT utxo fields include the amount that are being spent by the PSBT inputs, but there is a "fee attack" where using amounts that are lower than what is actually spent may make us sign a tx that spends much more in fees than we think. --- .../bitcoind/rpc/BitcoinCoreClient.scala | 4 +- .../keymanager/LocalOnchainKeyManager.scala | 21 +++++- .../bitcoind/BitcoinCoreClientSpec.scala | 6 +- .../LocalOnchainKeyManagerSpec.scala | 71 +++++++------------ 4 files changed, 48 insertions(+), 54 deletions(-) diff --git a/eclair-core/src/main/scala/fr/acinq/eclair/blockchain/bitcoind/rpc/BitcoinCoreClient.scala b/eclair-core/src/main/scala/fr/acinq/eclair/blockchain/bitcoind/rpc/BitcoinCoreClient.scala index b986dbfdfb..7f6662632b 100644 --- a/eclair-core/src/main/scala/fr/acinq/eclair/blockchain/bitcoind/rpc/BitcoinCoreClient.scala +++ b/eclair-core/src/main/scala/fr/acinq/eclair/blockchain/bitcoind/rpc/BitcoinCoreClient.scala @@ -232,7 +232,7 @@ class BitcoinCoreClient(val rpcClient: BitcoinJsonRPCClient, val onchainKeyManag fundTransaction(tx, FundTransactionOptions(feeRate, replaceable, inputWeights = externalInputsWeight.map { case (outpoint, weight) => InputWeight(outpoint, weight) }.toSeq)) } - private def processPsbt(psbt: Psbt, sign: Boolean = true, sighashType: Option[Int] = None)(implicit ec: ExecutionContext): Future[ProcessPsbtResponse] = { + def processPsbt(psbt: Psbt, sign: Boolean = true, sighashType: Option[Int] = None)(implicit ec: ExecutionContext): Future[ProcessPsbtResponse] = { // we use an explicit map here because this RPC calls takes a String for the "sighashtype" parameter with an explicitly limited list of valid values val sighashStrings = Map( SigHash.SIGHASH_DEFAULT -> "DEFAULT", @@ -254,7 +254,7 @@ class BitcoinCoreClient(val rpcClient: BitcoinJsonRPCClient, val onchainKeyManag }) } - private def utxoUpdatePsbt(psbt: Psbt)(implicit ec: ExecutionContext): Future[Psbt] = { + def utxoUpdatePsbt(psbt: Psbt)(implicit ec: ExecutionContext): Future[Psbt] = { val encoded = Base64.getEncoder.encodeToString(Psbt.write(psbt).toByteArray) rpcClient.invoke("utxoupdatepsbt", encoded).map(json => { diff --git a/eclair-core/src/main/scala/fr/acinq/eclair/crypto/keymanager/LocalOnchainKeyManager.scala b/eclair-core/src/main/scala/fr/acinq/eclair/crypto/keymanager/LocalOnchainKeyManager.scala index bd1a156716..d753b2edbd 100644 --- a/eclair-core/src/main/scala/fr/acinq/eclair/crypto/keymanager/LocalOnchainKeyManager.scala +++ b/eclair-core/src/main/scala/fr/acinq/eclair/crypto/keymanager/LocalOnchainKeyManager.scala @@ -3,8 +3,9 @@ package fr.acinq.eclair.crypto.keymanager import fr.acinq.bitcoin.ScriptWitness import fr.acinq.bitcoin.psbt.{Psbt, SignPsbtResult} import fr.acinq.bitcoin.scalacompat.DeterministicWallet._ -import fr.acinq.bitcoin.scalacompat.{Block, ByteVector32, DeterministicWallet, MnemonicCode} +import fr.acinq.bitcoin.scalacompat.{Block, ByteVector32, DeterministicWallet, MnemonicCode, Satoshi, Script} import fr.acinq.bitcoin.utils.EitherKt +import grizzled.slf4j.Logging import scodec.bits.ByteVector import scala.jdk.CollectionConverters.MapHasAsScala @@ -13,7 +14,7 @@ object LocalOnchainKeyManager { def descriptorChecksum(span: String): String = fr.acinq.bitcoin.Descriptor.checksum(span) } -class LocalOnchainKeyManager(entropy: ByteVector, chainHash: ByteVector32, passphrase: String = "") extends OnchainKeyManager { +class LocalOnchainKeyManager(entropy: ByteVector, chainHash: ByteVector32, passphrase: String = "") extends OnchainKeyManager with Logging { import LocalOnchainKeyManager._ @@ -68,13 +69,18 @@ class LocalOnchainKeyManager(entropy: ByteVector, chainHash: ByteVector32, passp } override def signPsbt(psbt: Psbt, ourInputs: Seq[Int], ourOutputs: Seq[Int]): Psbt = { + import fr.acinq.bitcoin.scalacompat.KotlinUtils._ + + val spent = ourInputs.map(i => kmp2scala(psbt.getInput(i).getWitnessUtxo.amount)).sum + val backtous = ourOutputs.map(i => kmp2scala(psbt.getGlobal.getTx.txOut.get(i).amount)).sum + + logger.info(s"signing ${psbt.getGlobal.getTx.txid} fees ${psbt.computeFees()} spent $spent to_us $backtous") ourOutputs.foreach(i => isOurOutput(psbt, i)) ourInputs.foldLeft(psbt) { (p, i) => sigbnPsbtInput(p, i) } } // check that an output belongs to us i.e. we can recompute its public from its bip32 path private def isOurOutput(psbt: Psbt, outputIndex: Int) = { - val output = psbt.getOutputs.get(outputIndex) val txout = psbt.getGlobal.getTx.txOut.get(outputIndex) output.getDerivationPaths.size() match { @@ -83,6 +89,7 @@ class LocalOnchainKeyManager(entropy: ByteVector, chainHash: ByteVector32, passp val priv = fr.acinq.bitcoin.DeterministicWallet.derivePrivateKey(master.priv, keypath.getKeyPath).getPrivateKey val check = priv.publicKey() require(pub == check, s"cannot compute public key for $txout") + require(txout.publicKeyScript.contentEquals(fr.acinq.bitcoin.Script.write(fr.acinq.bitcoin.Script.pay2wpkh(pub))), s"output pubkeyscript does not match ours for $txout") } case _ => throw new IllegalArgumentException(s"cannot verify that $txout sends to us") } @@ -93,6 +100,14 @@ class LocalOnchainKeyManager(entropy: ByteVector, chainHash: ByteVector32, passp val input = psbt.getInput(pos) + // For each wallet input, Bitcoin Core will provide + // - the output that was spent, in the PSBT's witness utxo field + // - the actual transaction that was spent, in the PSBT's non-witness utxo field + // we check that this fields are consistent and match the outpoint that is spent in the PSBT + // this prevents attacks where bitcoin core would lie about the amount being spent and make us pay very high fees + require(input.getNonWitnessUtxo.txid == psbt.getGlobal.getTx.txIn.get(pos).outPoint.txid, "utxo txid mismatch") + require(input.getNonWitnessUtxo.txOut.get(psbt.getGlobal.getTx.txIn.get(pos).outPoint.index.toInt) == input.getWitnessUtxo, "utxo mismatch") + // check that we're signing a p2wpkh input and that the keypath is provided and correct require(Script.isPay2wpkh(input.getWitnessUtxo.publicKeyScript.toByteArray), "spent input is not p2wpkh") require(input.getDerivationPaths.size() == 1, "invalid bip32 path") diff --git a/eclair-core/src/test/scala/fr/acinq/eclair/blockchain/bitcoind/BitcoinCoreClientSpec.scala b/eclair-core/src/test/scala/fr/acinq/eclair/blockchain/bitcoind/BitcoinCoreClientSpec.scala index 8dcdf0cc11..e9569d620e 100644 --- a/eclair-core/src/test/scala/fr/acinq/eclair/blockchain/bitcoind/BitcoinCoreClientSpec.scala +++ b/eclair-core/src/test/scala/fr/acinq/eclair/blockchain/bitcoind/BitcoinCoreClientSpec.scala @@ -1185,7 +1185,7 @@ class BitcoinCoreClientWithExternalSignerSpec extends BitcoinCoreClientSpec { } } - test("use eclair manage onchain keys") { + test("use eclair to manage onchain keys") { val sender = TestProbe() (1 to 10).foreach { _ => @@ -1213,9 +1213,7 @@ class BitcoinCoreClientWithExternalSignerSpec extends BitcoinCoreClientSpec { val error = sender.expectMsgType[Failure] assert(error.cause.getMessage.contains("Private keys are disabled for this wallet")) - wallet1.rpcClient.invoke("estimatesmartfee", 1).map(BitcoinCoreFeeProvider.parseFeeEstimate).pipeTo(sender.ref) - val feeratePerKb = sender.expectMsgType[FeeratePerKB] - wallet1.sendToPubkeyScript(Script.write(addressToPublicKeyScript(address, Block.RegtestGenesisBlock.hash)), 50_000.sat, FeeratePerKw(feeratePerKb)).pipeTo(sender.ref) + wallet1.sendToPubkeyScript(Script.write(addressToPublicKeyScript(address, Block.RegtestGenesisBlock.hash)), 50_000.sat, FeeratePerKw(FeeratePerByte(5.sat))).pipeTo(sender.ref) sender.expectMsgType[ByteVector32] } } diff --git a/eclair-core/src/test/scala/fr/acinq/eclair/crypto/keymanager/LocalOnchainKeyManagerSpec.scala b/eclair-core/src/test/scala/fr/acinq/eclair/crypto/keymanager/LocalOnchainKeyManagerSpec.scala index 29cc9e1dc2..26d6cb4501 100644 --- a/eclair-core/src/test/scala/fr/acinq/eclair/crypto/keymanager/LocalOnchainKeyManagerSpec.scala +++ b/eclair-core/src/test/scala/fr/acinq/eclair/crypto/keymanager/LocalOnchainKeyManagerSpec.scala @@ -48,53 +48,36 @@ class LocalOnchainKeyManagerSpec extends AnyFunSuite { new KeyPathWithMaster(onchainKeyManager.getOnchainMasterMasterFingerprint, new fr.acinq.bitcoin.KeyPath("m/84'/1'/0'/0/2")), ) + val tx = Transaction(version = 2, + txIn = utxos.map(tx => TxIn(OutPoint(tx, 0), Nil, fr.acinq.bitcoin.TxIn.SEQUENCE_FINAL)), + txOut = TxOut(Satoshi(1000_000), Script.pay2wpkh(getPublicKey(0))) :: Nil, lockTime = 0) + val psbt: Psbt = { + val p0 = new Psbt(tx).updateWitnessInput(OutPoint(utxos(0), 0), utxos(0).txOut(0), null, Script.pay2pkh(getPublicKey(0)).map(scala2kmp).asJava, null, java.util.Map.of(getPublicKey(0), bip32paths(0))) + val p1 = EitherKt.flatMap(p0, (psbt: Psbt) => psbt.updateWitnessInput(OutPoint(utxos(1), 0), utxos(1).txOut(0), null, Script.pay2pkh(getPublicKey(1)).map(scala2kmp).asJava, null, java.util.Map.of(getPublicKey(1), bip32paths(1)))) + val p2 = EitherKt.flatMap(p1, (psbt: Psbt) => psbt.updateWitnessInput(OutPoint(utxos(2), 0), utxos(2).txOut(0), null, Script.pay2pkh(getPublicKey(2)).map(scala2kmp).asJava, null, java.util.Map.of(getPublicKey(2), bip32paths(2)))) + val p3 = EitherKt.flatMap(p2, (psbt: Psbt) => psbt.updateNonWitnessInput(utxos(0), 0, null, null, java.util.Map.of())) + val p4 = EitherKt.flatMap(p3, (psbt: Psbt) => psbt.updateNonWitnessInput(utxos(1), 0, null, null, java.util.Map.of())) + val p5 = EitherKt.flatMap(p4, (psbt: Psbt) => psbt.updateNonWitnessInput(utxos(2), 0, null, null, java.util.Map.of())) + val p6 = EitherKt.flatMap(p5, (psbt: Psbt) => psbt.updateWitnessOutput(0, null, null, java.util.Map.of(getPublicKey(0), bip32paths(0)))) + p6.getRight + } + { // sign all inputs and outputs - val tx = Transaction(version = 2, - txIn = utxos.map(tx => TxIn(OutPoint(tx, 0), Nil, fr.acinq.bitcoin.TxIn.SEQUENCE_FINAL)), - txOut = TxOut(Satoshi(1000_000), Script.pay2wpkh(getPublicKey(0))) :: Nil, lockTime = 0) - val psbt = new Psbt(tx) - val updated = { - val p = psbt.updateWitnessInput(OutPoint(utxos(0), 0), utxos(0).txOut(0), null, Script.pay2pkh(getPublicKey(0)).map(scala2kmp).asJava, null, java.util.Map.of(getPublicKey(0), bip32paths(0))) - val p1 = EitherKt.flatMap(p, (psbt: Psbt) => psbt.updateWitnessInput(OutPoint(utxos(1), 0), utxos(1).txOut(0), null, Script.pay2pkh(getPublicKey(1)).map(scala2kmp).asJava, null, java.util.Map.of(getPublicKey(1), bip32paths(1)))) - val p2 = EitherKt.flatMap(p1, (psbt: Psbt) => psbt.updateWitnessInput(OutPoint(utxos(2), 0), utxos(2).txOut(0), null, Script.pay2pkh(getPublicKey(2)).map(scala2kmp).asJava, null, java.util.Map.of(getPublicKey(2), bip32paths(2)))) - val p3 = EitherKt.flatMap(p2, (psbt: Psbt) => psbt.updateWitnessOutput(0, null, null, java.util.Map.of(getPublicKey(0), bip32paths(0)))) - p3.getRight - } - val psbt1 = onchainKeyManager.signPsbt(updated, Seq(0, 1, 2), Seq(0)) + val psbt1 = onchainKeyManager.signPsbt(psbt, Seq(0, 1, 2), Seq(0)) val signedTx = psbt1.extract().getRight Transaction.correctlySpends(signedTx, utxos, ScriptFlags.STANDARD_SCRIPT_VERIFY_FLAGS) } { // sign the first 2 inputs only - val tx = Transaction(version = 2, - txIn = utxos.map(tx => TxIn(OutPoint(tx, 0), Nil, fr.acinq.bitcoin.TxIn.SEQUENCE_FINAL)), - txOut = TxOut(Satoshi(1000_000), Script.pay2wpkh(getPublicKey(0))) :: Nil, lockTime = 0) - val psbt = new Psbt(tx) - val updated = { - val p = psbt.updateWitnessInput(OutPoint(utxos(0), 0), utxos(0).txOut(0), null, Script.pay2pkh(getPublicKey(0)).map(scala2kmp).asJava, null, java.util.Map.of(getPublicKey(0), bip32paths(0))) - val p1 = EitherKt.flatMap(p, (psbt: Psbt) => psbt.updateWitnessInput(OutPoint(utxos(1), 0), utxos(1).txOut(0), null, Script.pay2pkh(getPublicKey(1)).map(scala2kmp).asJava, null, java.util.Map.of(getPublicKey(1), bip32paths(1)))) - val p2 = EitherKt.flatMap(p1, (psbt: Psbt) => psbt.updateWitnessOutput(0, null, null, java.util.Map.of(getPublicKey(0), bip32paths(0)))) - p2.getRight - } - val psbt1 = onchainKeyManager.signPsbt(updated, Seq(0, 1), Seq(0)) + val psbt1 = onchainKeyManager.signPsbt(psbt, Seq(0, 1), Seq(0)) // extracting the final tx fails because no all inputs as signed assert(psbt1.extract().isLeft) assert(psbt1.getInput(2).getScriptWitness == null) } { // provide a wrong derivation path for the first input - val tx = Transaction(version = 2, - txIn = utxos.map(tx => TxIn(OutPoint(tx, 0), Nil, fr.acinq.bitcoin.TxIn.SEQUENCE_FINAL)), - txOut = TxOut(Satoshi(1000_000), Script.pay2wpkh(getPublicKey(0))) :: Nil, lockTime = 0) - val psbt = new Psbt(tx) - val updated = { - val p = psbt.updateWitnessInput(OutPoint(utxos(0), 0), utxos(0).txOut(0), null, Script.pay2pkh(getPublicKey(0)).map(scala2kmp).asJava, null, java.util.Map.of(getPublicKey(0), bip32paths(2))) // wrong bip32 path - val p1 = EitherKt.flatMap(p, (psbt: Psbt) => psbt.updateWitnessInput(OutPoint(utxos(1), 0), utxos(1).txOut(0), null, Script.pay2pkh(getPublicKey(1)).map(scala2kmp).asJava, null, java.util.Map.of(getPublicKey(1), bip32paths(1)))) - val p2 = EitherKt.flatMap(p1, (psbt: Psbt) => psbt.updateWitnessInput(OutPoint(utxos(2), 0), utxos(2).txOut(0), null, Script.pay2pkh(getPublicKey(2)).map(scala2kmp).asJava, null, java.util.Map.of(getPublicKey(2), bip32paths(2)))) - val p3 = EitherKt.flatMap(p2, (psbt: Psbt) => psbt.updateWitnessOutput(0, null, null, java.util.Map.of(getPublicKey(0), bip32paths(0)))) - p3.getRight - } + val updated = psbt.updateWitnessInput(OutPoint(utxos(0), 0), utxos(0).txOut(0), null, Script.pay2pkh(getPublicKey(0)).map(scala2kmp).asJava, null, java.util.Map.of(getPublicKey(0), bip32paths(2))).getRight // wrong bip32 path val error = intercept[IllegalArgumentException] { onchainKeyManager.signPsbt(updated, Seq(0, 1, 2), Seq(0)) } @@ -102,22 +85,20 @@ class LocalOnchainKeyManagerSpec extends AnyFunSuite { } { // provide a wrong derivation path for the first output - val tx = Transaction(version = 2, - txIn = utxos.map(tx => TxIn(OutPoint(tx, 0), Nil, fr.acinq.bitcoin.TxIn.SEQUENCE_FINAL)), - txOut = TxOut(Satoshi(1000_000), Script.pay2wpkh(getPublicKey(0))) :: Nil, lockTime = 0) - val psbt = new Psbt(tx) - val updated = { - val p = psbt.updateWitnessInput(OutPoint(utxos(0), 0), utxos(0).txOut(0), null, Script.pay2pkh(getPublicKey(0)).map(scala2kmp).asJava, null, java.util.Map.of(getPublicKey(0), bip32paths(0))) - val p1 = EitherKt.flatMap(p, (psbt: Psbt) => psbt.updateWitnessInput(OutPoint(utxos(1), 0), utxos(1).txOut(0), null, Script.pay2pkh(getPublicKey(1)).map(scala2kmp).asJava, null, java.util.Map.of(getPublicKey(1), bip32paths(1)))) - val p2 = EitherKt.flatMap(p1, (psbt: Psbt) => psbt.updateWitnessInput(OutPoint(utxos(2), 0), utxos(2).txOut(0), null, Script.pay2pkh(getPublicKey(2)).map(scala2kmp).asJava, null, java.util.Map.of(getPublicKey(2), bip32paths(2)))) - val p3 = EitherKt.flatMap(p2, (psbt: Psbt) => psbt.updateWitnessOutput(0, null, null, java.util.Map.of(getPublicKey(0), bip32paths(1)))) // wrong path - p3.getRight - } + val updated = psbt.updateWitnessOutput(0, null, null, java.util.Map.of(getPublicKey(0), bip32paths(1))).getRight // wrong path val error = intercept[IllegalArgumentException] { onchainKeyManager.signPsbt(updated, Seq(0, 1, 2), Seq(0)) } assert(error.getMessage.contains("cannot compute public key")) } + { + // lie about the amount being spent + val updated = psbt.updateWitnessInput(OutPoint(utxos(0), 0), utxos(0).txOut(0).copy(amount = Satoshi(10)), null, Script.pay2pkh(getPublicKey(0)).map(scala2kmp).asJava, null, java.util.Map.of(getPublicKey(0), bip32paths(0))).getRight + val error = intercept[IllegalArgumentException] { + onchainKeyManager.signPsbt(updated, Seq(0, 1, 2), Seq(0)) + } + assert(error.getMessage.contains("utxo mismatch")) + } } test("compute descriptor checksums") {