From 3e21295b884e56ef11197d7476da9847ec7ef984 Mon Sep 17 00:00:00 2001 From: Tobin Harding Date: Tue, 8 Mar 2022 16:43:07 +1100 Subject: [PATCH 1/4] Remove unnecessary whitespace character Typically we do not put a whitespace character before a `:` when using explicit types. --- src/blockdata/transaction.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/blockdata/transaction.rs b/src/blockdata/transaction.rs index 0f7a1624..6fc26205 100644 --- a/src/blockdata/transaction.rs +++ b/src/blockdata/transaction.rs @@ -335,7 +335,7 @@ impl Transaction { script_pubkey: &Script, sighash_type: U, ) -> Result<(), encode::Error> { - let sighash_type : u32 = sighash_type.into(); + let sighash_type: u32 = sighash_type.into(); assert!(input_index < self.input.len()); // Panic on OOB let (sighash, anyone_can_pay) = EcdsaSigHashType::from_u32_consensus(sighash_type).split_anyonecanpay_flag(); From 3831816a732872e06bc62197b166ce4c1c661ed2 Mon Sep 17 00:00:00 2001 From: Tobin Harding Date: Tue, 8 Mar 2022 17:09:19 +1100 Subject: [PATCH 2/4] Move test helper function Move helper function to above the test that uses it. Refactor only, no logic changes. --- src/blockdata/transaction.rs | 38 ++++++++++++++++++------------------ 1 file changed, 19 insertions(+), 19 deletions(-) diff --git a/src/blockdata/transaction.rs b/src/blockdata/transaction.rs index 6fc26205..7c4f1a4e 100644 --- a/src/blockdata/transaction.rs +++ b/src/blockdata/transaction.rs @@ -1082,25 +1082,6 @@ mod tests { serde_round_trip!(tx); } - fn run_test_sighash(tx: &str, script: &str, input_index: usize, hash_type: i32, expected_result: &str) { - let tx: Transaction = deserialize(&Vec::from_hex(tx).unwrap()[..]).unwrap(); - let script = Script::from(Vec::from_hex(script).unwrap()); - let mut raw_expected = Vec::from_hex(expected_result).unwrap(); - raw_expected.reverse(); - let expected_result = SigHash::from_slice(&raw_expected[..]).unwrap(); - - let actual_result = if raw_expected[0] % 2 == 0 { - // tx.signature_hash and cache.legacy_signature_hash are the same, this if helps to test - // both the codepaths without repeating the test code - tx.signature_hash(input_index, &script, hash_type as u32) - } else { - let cache = SigHashCache::new(&tx); - cache.legacy_signature_hash(input_index, &script, hash_type as u32).unwrap() - }; - - assert_eq!(actual_result, expected_result); - } - // Test decoding transaction `4be105f158ea44aec57bf12c5817d073a712ab131df6f37786872cfc70734188` // from testnet, which is the first BIP144-encoded transaction I encountered. #[test] @@ -1155,6 +1136,25 @@ mod tests { assert_eq!(EcdsaSigHashType::from_u32_standard(nonstandard_hashtype), Err(NonStandardSigHashType(0x04))); } + fn run_test_sighash(tx: &str, script: &str, input_index: usize, hash_type: i32, expected_result: &str) { + let tx: Transaction = deserialize(&Vec::from_hex(tx).unwrap()[..]).unwrap(); + let script = Script::from(Vec::from_hex(script).unwrap()); + let mut raw_expected = Vec::from_hex(expected_result).unwrap(); + raw_expected.reverse(); + let expected_result = SigHash::from_slice(&raw_expected[..]).unwrap(); + + let actual_result = if raw_expected[0] % 2 == 0 { + // tx.signature_hash and cache.legacy_signature_hash are the same, this if helps to test + // both the codepaths without repeating the test code + tx.signature_hash(input_index, &script, hash_type as u32) + } else { + let cache = SigHashCache::new(&tx); + cache.legacy_signature_hash(input_index, &script, hash_type as u32).unwrap() + }; + + assert_eq!(actual_result, expected_result); + } + // These test vectors were stolen from libbtc, which is Copyright 2014 Jonas Schnelli MIT // They were transformed by replacing {...} with run_test_sighash(...), then the ones containing // OP_CODESEPARATOR in their pubkeys were removed From 82f29b4267de706c34a5a8c99a2f90860d6aa2ba Mon Sep 17 00:00:00 2001 From: Tobin Harding Date: Tue, 8 Mar 2022 17:20:26 +1100 Subject: [PATCH 3/4] Use 1 signature hash for invalid SIGHASH_SINGLE When signing a transaction will result in the sighash single bug being exploitable we should return the 1 array (equivalent to 1 as a uint256) as the signature hash. Currently we are using the correct array value but are re-hashing it, instead we should directly return it. --- src/blockdata/transaction.rs | 29 ++++++++++++++++++++--------- 1 file changed, 20 insertions(+), 9 deletions(-) diff --git a/src/blockdata/transaction.rs b/src/blockdata/transaction.rs index 7c4f1a4e..9c026097 100644 --- a/src/blockdata/transaction.rs +++ b/src/blockdata/transaction.rs @@ -45,6 +45,14 @@ use VarInt; #[cfg(doc)] use util::sighash::SchnorrSigHashType; +/// Used for signature hash for invalid use of SIGHASH_SINGLE. +const UINT256_ONE: [u8; 32] = [ + 1, 0, 0, 0, 0, 0, 0, 0, + 0, 0, 0, 0, 0, 0, 0, 0, + 0, 0, 0, 0, 0, 0, 0, 0, + 0, 0, 0, 0, 0, 0, 0, 0 +]; + /// A reference to a transaction output. #[derive(Copy, Clone, Debug, Eq, Hash, PartialEq, PartialOrd, Ord)] pub struct OutPoint { @@ -340,15 +348,6 @@ impl Transaction { let (sighash, anyone_can_pay) = EcdsaSigHashType::from_u32_consensus(sighash_type).split_anyonecanpay_flag(); - // Special-case sighash_single bug because this is easy enough. - if sighash == EcdsaSigHashType::Single && input_index >= self.output.len() { - writer.write_all(&[1, 0, 0, 0, 0, 0, 0, 0, - 0, 0, 0, 0, 0, 0, 0, 0, - 0, 0, 0, 0, 0, 0, 0, 0, - 0, 0, 0, 0, 0, 0, 0, 0])?; - return Ok(()); - } - // Build tx to sign let mut tx = Transaction { version: self.version, @@ -404,12 +403,24 @@ impl Transaction { script_pubkey: &Script, sighash_u32: u32 ) -> SigHash { + if self.is_invalid_use_of_sighash_single(sighash_u32, input_index) { + return SigHash::from_slice(&UINT256_ONE).expect("const-size array"); + } + let mut engine = SigHash::engine(); self.encode_signing_data_to(&mut engine, input_index, script_pubkey, sighash_u32) .expect("engines don't error"); SigHash::from_engine(engine) } + /// The SIGHASH_SINGLE bug becomes exploitable when one tries to sign a transaction with + /// SIGHASH_SINGLE and there is not a corresponding output transaction with the same index as + /// the input transaction. + fn is_invalid_use_of_sighash_single(&self, sighash: u32, input_index: usize) -> bool { + let ty = EcdsaSigHashType::from_u32_consensus(sighash); + ty == EcdsaSigHashType::Single && input_index >= self.output.len() + } + /// Gets the "weight" of this transaction, as defined by BIP141. For transactions with an empty /// witness, this is simply the consensus-serialized size times 4. For transactions with a /// witness, this is the non-witness consensus-serialized size multiplied by 3 plus the From d1abfd9c30df9544200940b975508102fac93c22 Mon Sep 17 00:00:00 2001 From: Tobin Harding Date: Tue, 8 Mar 2022 17:19:01 +1100 Subject: [PATCH 4/4] Add unit test for sighash single bug When signing a transaction will result in the sighash single bug being exploitable we should return the 'one array' (equivalent to 1 as a uint256) as the signature hash. Add a unit test to verify we return uint256 1 value when use of SIGHASH_SINGLE is invalid. --- src/blockdata/transaction.rs | 25 ++++++++++++++++++++++++- 1 file changed, 24 insertions(+), 1 deletion(-) diff --git a/src/blockdata/transaction.rs b/src/blockdata/transaction.rs index 9c026097..729e853f 100644 --- a/src/blockdata/transaction.rs +++ b/src/blockdata/transaction.rs @@ -835,7 +835,7 @@ impl ::std::error::Error for SigHashTypeParseError {} #[cfg(test)] mod tests { - use super::{OutPoint, ParseOutPointError, Transaction, TxIn, NonStandardSigHashType}; + use super::*; use core::str::FromStr; use blockdata::constants::WITNESS_SCALE_FACTOR; @@ -1147,6 +1147,29 @@ mod tests { assert_eq!(EcdsaSigHashType::from_u32_standard(nonstandard_hashtype), Err(NonStandardSigHashType(0x04))); } + #[test] + fn sighash_single_bug() { + const SIGHASH_SINGLE: u32 = 3; + // We need a tx with more inputs than outputs. + let mut input = Vec::new(); + input.push(TxIn::default()); + input.push(TxIn::default()); + let mut output = Vec::new(); + output.push(TxOut::default()); + + let tx = Transaction { + version: 1, + lock_time: 0, + input: input, + output: output, // TODO: Use Vec::from([TxOut]) once we bump MSRV. + }; + let script = Script::new(); + let got = tx.signature_hash(1, &script, SIGHASH_SINGLE); + let want = SigHash::from_slice(&UINT256_ONE).unwrap(); + + assert_eq!(got, want) + } + fn run_test_sighash(tx: &str, script: &str, input_index: usize, hash_type: i32, expected_result: &str) { let tx: Transaction = deserialize(&Vec::from_hex(tx).unwrap()[..]).unwrap(); let script = Script::from(Vec::from_hex(script).unwrap());