From f404425b4d3bb50bc778b125dcdc71119c04eb26 Mon Sep 17 00:00:00 2001 From: Richard Ulrich Date: Tue, 12 Dec 2023 08:48:15 +0100 Subject: [PATCH] Properly handling signature validation errors. Patch provided by https://github.com/Ademan --- .github/workflows/cont_integration.yml | 6 +- Cargo.toml | 4 +- src/reserves.rs | 97 +++++++++++++++++--------- tests/tampering.rs | 2 +- 4 files changed, 73 insertions(+), 36 deletions(-) diff --git a/.github/workflows/cont_integration.yml b/.github/workflows/cont_integration.yml index 49ecf84..e91e498 100644 --- a/.github/workflows/cont_integration.yml +++ b/.github/workflows/cont_integration.yml @@ -38,8 +38,12 @@ jobs: - name: Pin dependencies for MSRV if: matrix.rust == '1.57.0' run: | - cargo update -p log:0.4.19 --precise 0.4.18 + cargo update -p log:0.4.20 --precise 0.4.18 cargo update -p tempfile --precise 3.6.0 + cargo update -p webpki:0.22.4 --precise 0.22.0 + cargo update -p rustls:0.20.9 --precise 0.20.8 + cargo update -p sct:0.7.1 --precise 0.7.0 + cargo update -p tokio:1.35.0 --precise 1.29.1 - name: Build run: cargo build - name: Clippy diff --git a/Cargo.toml b/Cargo.toml index 906163a..565506a 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -1,6 +1,6 @@ [package] name = "bdk-reserves" -version = "0.28.0" +version = "0.28.1" authors = ["Richard Ulrich "] edition = "2018" description = "Proof of reserves for bitcoin dev kit" @@ -20,4 +20,4 @@ bdk-testutils = "^0.4" bdk = { version = "0.28", default-features = true } electrsd = { version = "0.21", features = ["bitcoind_22_0", "electrs_0_9_1"] } # base64ct versions at 1.6.0 and higher have MSRV 1.60.0 -base64ct = { version = "<1.6.0", features = ["alloc"] } \ No newline at end of file +base64ct = { version = "<1.6.0", features = ["alloc"] } diff --git a/src/reserves.rs b/src/reserves.rs index 092d97b..c6f4a22 100644 --- a/src/reserves.rs +++ b/src/reserves.rs @@ -238,6 +238,35 @@ pub fn verify_proof( return Err(ProofError::UnsupportedSighashType(i)); } + // calculate the spendable amount of the proof + let sum = tx + .input + .iter() + .map(|tx_in| { + if let Some(op) = outpoints.iter().find(|op| op.0 == tx_in.previous_output) { + op.1.value + } else { + 0 + } + }) + .sum(); + + // inflow and outflow being equal means no miner fee + if tx.output[0].value != sum { + return Err(ProofError::InAndOutValueNotEqual); + } + + // verify the unspendable output + let pkh = PubkeyHash::from_hash(hash160::Hash::hash(&[0])); + let out_script_unspendable = Address { + payload: Payload::PubkeyHash(pkh), + network, + } + .script_pubkey(); + if tx.output[0].script_pubkey != out_script_unspendable { + return Err(ProofError::InvalidOutput); + } + let serialized_tx = serialize(&tx); // Verify the challenge input if let Some(utxo) = &psbt.inputs[0].witness_utxo { @@ -271,12 +300,13 @@ pub fn verify_proof( .map(|(i, res)| match res { Ok(txout) => ( i, - Ok(bitcoinconsensus::verify( + bitcoinconsensus::verify( txout.script_pubkey.to_bytes().as_slice(), txout.value, &serialized_tx, i, - )), + ) + .map_err(|e| ProofError::SignatureValidation(i, format!("{:?}", e))), ), Err(err) => (i, Err(err)), }) @@ -288,35 +318,6 @@ pub fn verify_proof( )); } - // calculate the spendable amount of the proof - let sum = tx - .input - .iter() - .map(|tx_in| { - if let Some(op) = outpoints.iter().find(|op| op.0 == tx_in.previous_output) { - op.1.value - } else { - 0 - } - }) - .sum(); - - // verify the unspendable output - let pkh = PubkeyHash::from_hash(hash160::Hash::hash(&[0])); - let out_script_unspendable = Address { - payload: Payload::PubkeyHash(pkh), - network, - } - .script_pubkey(); - if tx.output[0].script_pubkey != out_script_unspendable { - return Err(ProofError::InvalidOutput); - } - - // inflow and outflow being equal means no miner fee - if tx.output[0].value != sum { - return Err(ProofError::InAndOutValueNotEqual); - } - Ok(sum) } @@ -336,8 +337,11 @@ mod test { use super::*; use base64ct::{Base64, Encoding}; use bdk::bitcoin::consensus::encode::deserialize; - use bdk::bitcoin::{Address, Network}; + use bdk::bitcoin::hashes::sha256; + use bdk::bitcoin::secp256k1::{ecdsa::SerializedSignature, Message, Secp256k1, SecretKey}; + use bdk::bitcoin::{Address, EcdsaSighashType, Network, Witness}; use bdk::wallet::get_funded_wallet; + use std::str::FromStr; #[test] fn test_proof() { @@ -485,6 +489,35 @@ mod test { wallet.verify_proof(&psbt, message, None).unwrap(); } + #[test] + #[should_panic(expected = "SignatureValidation")] + fn invalid_signature() { + let descriptor = "wpkh(cVpPVruEDdmutPzisEsYvtST1usBR3ntr8pXSyt6D2YYqXRyPcFW)"; + let (wallet, _, _) = get_funded_wallet(descriptor); + + let message = "This belongs to me."; + let mut psbt = get_signed_proof(); + psbt.inputs[1].final_script_sig = None; + + let secp = Secp256k1::new(); + // privkey from milk sad ... + let privkey = + SecretKey::from_str("4dcaff8ed1975fe2cebbd7c03384902c2189a2e6de11f1bb1c9dc784e8e4d11e") + .expect("valid privkey"); + + let invalid_message = + Message::from_hashed_data::("Invalid signing data".as_bytes()); + let signature = secp.sign_ecdsa(&invalid_message, &privkey); + + let mut invalid_witness = Witness::new(); + + let signature = SerializedSignature::from_signature(&signature); + invalid_witness.push_bitcoin_signature(&signature, EcdsaSighashType::All); + psbt.inputs[1].final_script_witness = Some(invalid_witness); + + wallet.verify_proof(&psbt, message, None).unwrap(); + } + #[test] #[should_panic(expected = "UnsupportedSighashType(1)")] fn wrong_sighash_type() { diff --git a/tests/tampering.rs b/tests/tampering.rs index 43e3c87..0c4a53b 100644 --- a/tests/tampering.rs +++ b/tests/tampering.rs @@ -33,7 +33,7 @@ fn tampered_proof_message() { let res_alice = wallet.verify_proof(&psbt_alice, message_alice, None); let res_bob = wallet.verify_proof(&psbt_alice, message_bob, None); assert!(res_alice.is_err()); - assert!(res_bob.is_ok()); + assert!(res_bob.is_err()); res_alice.unwrap(); res_bob.unwrap(); }