diff --git a/lightning-transaction-sync/src/common.rs b/lightning-transaction-sync/src/common.rs index 88e52de186d..02df6c28b4d 100644 --- a/lightning-transaction-sync/src/common.rs +++ b/lightning-transaction-sync/src/common.rs @@ -133,6 +133,10 @@ impl FilterQueue { } } +pub(crate) fn is_potentially_unsafe_merkle_leaf(tx: &Transaction) -> bool { + tx.base_size() == 64 +} + #[derive(Debug)] pub(crate) struct ConfirmedTx { pub tx: Transaction, @@ -141,3 +145,35 @@ pub(crate) struct ConfirmedTx { pub block_height: u32, pub pos: usize, } + +#[cfg(test)] +mod tests { + use super::is_potentially_unsafe_merkle_leaf; + + use bitcoin::absolute::LockTime; + use bitcoin::hashes::Hash; + use bitcoin::transaction::Version; + use bitcoin::{Amount, OutPoint, ScriptBuf, Sequence, Transaction, TxIn, TxOut, Txid, Witness}; + + #[test] + fn merkle_leaf_check_uses_non_witness_size() { + let mut witness = Witness::new(); + witness.push(vec![0]); + + let tx = Transaction { + version: Version::TWO, + lock_time: LockTime::ZERO, + input: vec![TxIn { + previous_output: OutPoint { txid: Txid::all_zeros(), vout: 0 }, + script_sig: ScriptBuf::new(), + sequence: Sequence::ZERO, + witness, + }], + output: vec![TxOut { value: Amount::ZERO, script_pubkey: ScriptBuf::from(vec![0; 4]) }], + }; + + assert_eq!(tx.base_size(), 64, "test setup: base size must be 64"); + assert!(tx.total_size() > 64, "test setup: witness must pad total size"); + assert!(is_potentially_unsafe_merkle_leaf(&tx)); + } +} diff --git a/lightning-transaction-sync/src/electrum.rs b/lightning-transaction-sync/src/electrum.rs index cb937248f41..2eb146809a6 100644 --- a/lightning-transaction-sync/src/electrum.rs +++ b/lightning-transaction-sync/src/electrum.rs @@ -5,7 +5,7 @@ // http://opensource.org/licenses/MIT>, at your option. You may not use this file except in // accordance with one or both of these licenses. -use crate::common::{ConfirmedTx, FilterQueue, SyncState}; +use crate::common::{is_potentially_unsafe_merkle_leaf, ConfirmedTx, FilterQueue, SyncState}; use crate::error::{InternalError, TxSyncError}; use electrum_client::utils::validate_merkle_proof; @@ -277,6 +277,11 @@ impl ElectrumSyncClient { for txid in &sync_state.watched_transactions { match self.client.transaction_get(&txid) { Ok(tx) => { + if tx.compute_txid() != *txid { + log_error!(self.logger, "Retrieved transaction for txid {} doesn't match expectations. This should not happen. Please verify server integrity.", txid); + return Err(InternalError::Failed); + } + // Bitcoin Core's Merkle tree implementation has no way to discern between // internal and leaf node entries. As a consequence it is susceptible to an // attacker injecting additional transactions by crafting 64-byte @@ -284,7 +289,7 @@ impl ElectrumSyncClient { // https://web.archive.org/web/20240329003521/https://bitslog.com/2018/06/09/leaf-node-weakness-in-bitcoin-merkle-tree-design/). // To protect against this (highly unlikely) attack vector, we check that the // transaction is at least 65 bytes in length. - if tx.total_size() == 64 { + if is_potentially_unsafe_merkle_leaf(&tx) { log_error!(self.logger, "Skipping transaction {} due to retrieving potentially invalid tx data.", txid); continue; } @@ -369,6 +374,11 @@ impl ElectrumSyncClient { match self.client.transaction_get(&txid) { Ok(tx) => { + if tx.compute_txid() != txid { + log_error!(self.logger, "Retrieved transaction for txid {} doesn't match expectations. This should not happen. Please verify server integrity.", txid); + return Err(InternalError::Failed); + } + let mut is_spend = false; for txin in &tx.input { let watched_outpoint = @@ -517,3 +527,23 @@ impl Filter for ElectrumSyncClient { locked_queue.outputs.insert(output.outpoint.into_bitcoin_outpoint(), output); } } + +#[cfg(test)] +mod tests { + #[test] + fn transaction_get_responses_are_verified_at_call_sites() { + let src = include_str!("electrum.rs"); + let watched_transaction_check = concat!("if tx.compute_", "txid() != *txid"); + let watched_output_spend_check = concat!("if tx.compute_", "txid() != txid"); + + assert!( + src.contains(watched_transaction_check), + "watched transaction_get responses must be verified against the requested txid" + ); + assert!( + src.contains(watched_output_spend_check), + "watched-output spend transaction_get responses must be verified against the \ + requested txid" + ); + } +} diff --git a/lightning-transaction-sync/src/esplora.rs b/lightning-transaction-sync/src/esplora.rs index 52cfb394464..07d2ba26219 100644 --- a/lightning-transaction-sync/src/esplora.rs +++ b/lightning-transaction-sync/src/esplora.rs @@ -5,7 +5,7 @@ // http://opensource.org/licenses/MIT>, at your option. You may not use this file except in // accordance with one or both of these licenses. -use crate::common::{ConfirmedTx, FilterQueue, SyncState}; +use crate::common::{is_potentially_unsafe_merkle_leaf, ConfirmedTx, FilterQueue, SyncState}; use crate::error::{InternalError, TxSyncError}; use lightning::chain::WatchedOutput; @@ -393,7 +393,7 @@ impl EsploraSyncClient { // https://web.archive.org/web/20240329003521/https://bitslog.com/2018/06/09/leaf-node-weakness-in-bitcoin-merkle-tree-design/). // To protect against this (highly unlikely) attack vector, we check that the // transaction is at least 65 bytes in length. - if tx.total_size() == 64 { + if is_potentially_unsafe_merkle_leaf(&tx) { log_error!( self.logger, "Skipping transaction {} due to retrieving potentially invalid tx data.",