From 8678bda57681b0b30c4859240699c37b460d80d1 Mon Sep 17 00:00:00 2001 From: Matt Corallo Date: Thu, 17 Jan 2019 17:36:49 -0500 Subject: [PATCH] Ensure Message always unwraps in fuzztarget Hashes cant be all-0s, so we can normally unwrap, but fuzztarget can generate all-0 hashes, so we have to handle it and swap for something else. --- src/ln/channel.rs | 32 ++++++++++++++++---------------- src/ln/channelmanager.rs | 8 ++++---- src/ln/channelmonitor.rs | 8 ++++---- src/ln/router.rs | 8 ++++---- src/util/fuzz_wrappers.rs | 17 +++++++++++++++++ src/util/mod.rs | 3 +++ 6 files changed, 48 insertions(+), 28 deletions(-) create mode 100644 src/util/fuzz_wrappers.rs diff --git a/src/ln/channel.rs b/src/ln/channel.rs index ba86e6298..42d4caa36 100644 --- a/src/ln/channel.rs +++ b/src/ln/channel.rs @@ -11,7 +11,7 @@ use bitcoin_hashes::sha256::Hash as Sha256; use bitcoin_hashes::hash160::Hash as Hash160; use secp256k1::key::{PublicKey,SecretKey}; -use secp256k1::{Secp256k1,Message,Signature}; +use secp256k1::{Secp256k1,Signature}; use secp256k1; use ln::msgs; @@ -1067,7 +1067,7 @@ impl Channel { let funding_redeemscript = self.get_funding_redeemscript(); - let sighash = Message::from_slice(&bip143::SighashComponents::new(&tx).sighash_all(&tx.input[0], &funding_redeemscript, self.channel_value_satoshis)[..]).unwrap(); + let sighash = hash_to_message!(&bip143::SighashComponents::new(&tx).sighash_all(&tx.input[0], &funding_redeemscript, self.channel_value_satoshis)[..]); let our_sig = self.secp_ctx.sign(&sighash, &self.local_keys.funding_key); tx.input[0].witness.push(Vec::new()); // First is the multisig dummy @@ -1104,7 +1104,7 @@ impl Channel { let htlc_redeemscript = chan_utils::get_htlc_redeemscript(&htlc, &keys); let our_htlc_key = secp_check!(chan_utils::derive_private_key(&self.secp_ctx, &keys.per_commitment_point, &self.local_keys.htlc_base_key), "Derived invalid key, peer is maliciously selecting parameters"); - let sighash = Message::from_slice(&bip143::SighashComponents::new(&tx).sighash_all(&tx.input[0], &htlc_redeemscript, htlc.amount_msat / 1000)[..]).unwrap(); + let sighash = hash_to_message!(&bip143::SighashComponents::new(&tx).sighash_all(&tx.input[0], &htlc_redeemscript, htlc.amount_msat / 1000)[..]); let is_local_tx = PublicKey::from_secret_key(&self.secp_ctx, &our_htlc_key) == keys.a_htlc_key; Ok((htlc_redeemscript, self.secp_ctx.sign(&sighash, &our_htlc_key), is_local_tx)) } @@ -1408,7 +1408,7 @@ impl Channel { let local_keys = self.build_local_transaction_keys(self.cur_local_commitment_transaction_number)?; let mut local_initial_commitment_tx = self.build_commitment_transaction(self.cur_local_commitment_transaction_number, &local_keys, true, false, self.feerate_per_kw).0; - let local_sighash = Message::from_slice(&bip143::SighashComponents::new(&local_initial_commitment_tx).sighash_all(&local_initial_commitment_tx.input[0], &funding_script, self.channel_value_satoshis)[..]).unwrap(); + let local_sighash = hash_to_message!(&bip143::SighashComponents::new(&local_initial_commitment_tx).sighash_all(&local_initial_commitment_tx.input[0], &funding_script, self.channel_value_satoshis)[..]); // They sign the "local" commitment transaction... secp_check!(self.secp_ctx.verify(&local_sighash, &sig, &self.their_funding_pubkey.unwrap()), "Invalid funding_created signature from peer"); @@ -1418,7 +1418,7 @@ impl Channel { let remote_keys = self.build_remote_transaction_keys()?; let remote_initial_commitment_tx = self.build_commitment_transaction(self.cur_remote_commitment_transaction_number, &remote_keys, false, false, self.feerate_per_kw).0; - let remote_sighash = Message::from_slice(&bip143::SighashComponents::new(&remote_initial_commitment_tx).sighash_all(&remote_initial_commitment_tx.input[0], &funding_script, self.channel_value_satoshis)[..]).unwrap(); + let remote_sighash = hash_to_message!(&bip143::SighashComponents::new(&remote_initial_commitment_tx).sighash_all(&remote_initial_commitment_tx.input[0], &funding_script, self.channel_value_satoshis)[..]); // We sign the "remote" commitment transaction, allowing them to broadcast the tx if they wish. Ok((remote_initial_commitment_tx, local_initial_commitment_tx, self.secp_ctx.sign(&remote_sighash, &self.local_keys.funding_key), local_keys)) @@ -1487,7 +1487,7 @@ impl Channel { let local_keys = self.build_local_transaction_keys(self.cur_local_commitment_transaction_number)?; let mut local_initial_commitment_tx = self.build_commitment_transaction(self.cur_local_commitment_transaction_number, &local_keys, true, false, self.feerate_per_kw).0; - let local_sighash = Message::from_slice(&bip143::SighashComponents::new(&local_initial_commitment_tx).sighash_all(&local_initial_commitment_tx.input[0], &funding_script, self.channel_value_satoshis)[..]).unwrap(); + let local_sighash = hash_to_message!(&bip143::SighashComponents::new(&local_initial_commitment_tx).sighash_all(&local_initial_commitment_tx.input[0], &funding_script, self.channel_value_satoshis)[..]); // They sign the "local" commitment transaction, allowing us to broadcast the tx if we wish. secp_check!(self.secp_ctx.verify(&local_sighash, &msg.signature, &self.their_funding_pubkey.unwrap()), "Invalid funding_signed signature from peer"); @@ -1699,7 +1699,7 @@ impl Channel { (commitment_tx.0, commitment_tx.1, htlcs_cloned) }; let local_commitment_txid = local_commitment_tx.0.txid(); - let local_sighash = Message::from_slice(&bip143::SighashComponents::new(&local_commitment_tx.0).sighash_all(&local_commitment_tx.0.input[0], &funding_script, self.channel_value_satoshis)[..]).unwrap(); + let local_sighash = hash_to_message!(&bip143::SighashComponents::new(&local_commitment_tx.0).sighash_all(&local_commitment_tx.0.input[0], &funding_script, self.channel_value_satoshis)[..]); secp_check!(self.secp_ctx.verify(&local_sighash, &msg.signature, &self.their_funding_pubkey.unwrap()), "Invalid commitment tx signature from peer"); //If channel fee was updated by funder confirm funder can afford the new fee rate when applied to the current local commitment transaction @@ -1725,7 +1725,7 @@ impl Channel { if let Some(_) = htlc.transaction_output_index { let mut htlc_tx = self.build_htlc_transaction(&local_commitment_txid, &htlc, true, &local_keys, feerate_per_kw); let htlc_redeemscript = chan_utils::get_htlc_redeemscript(&htlc, &local_keys); - let htlc_sighash = Message::from_slice(&bip143::SighashComponents::new(&htlc_tx).sighash_all(&htlc_tx.input[0], &htlc_redeemscript, htlc.amount_msat / 1000)[..]).unwrap(); + let htlc_sighash = hash_to_message!(&bip143::SighashComponents::new(&htlc_tx).sighash_all(&htlc_tx.input[0], &htlc_redeemscript, htlc.amount_msat / 1000)[..]); secp_check!(self.secp_ctx.verify(&htlc_sighash, &msg.htlc_signatures[idx], &local_keys.b_htlc_key), "Invalid HTLC tx signature from peer"); let htlc_sig = if htlc.offered { let htlc_sig = self.sign_htlc_transaction(&mut htlc_tx, &msg.htlc_signatures[idx], &None, &htlc, &local_keys)?; @@ -2456,7 +2456,7 @@ impl Channel { let (closing_tx, total_fee_satoshis) = self.build_closing_transaction(proposed_total_fee_satoshis, false); let funding_redeemscript = self.get_funding_redeemscript(); - let sighash = Message::from_slice(&bip143::SighashComponents::new(&closing_tx).sighash_all(&closing_tx.input[0], &funding_redeemscript, self.channel_value_satoshis)[..]).unwrap(); + let sighash = hash_to_message!(&bip143::SighashComponents::new(&closing_tx).sighash_all(&closing_tx.input[0], &funding_redeemscript, self.channel_value_satoshis)[..]); self.last_sent_closing_fee = Some((proposed_feerate, total_fee_satoshis)); Some(msgs::ClosingSigned { @@ -2558,7 +2558,7 @@ impl Channel { if used_total_fee != msg.fee_satoshis { return Err(ChannelError::Close("Remote sent us a closing_signed with a fee greater than the value they can claim")); } - let mut sighash = Message::from_slice(&bip143::SighashComponents::new(&closing_tx).sighash_all(&closing_tx.input[0], &funding_redeemscript, self.channel_value_satoshis)[..]).unwrap(); + let mut sighash = hash_to_message!(&bip143::SighashComponents::new(&closing_tx).sighash_all(&closing_tx.input[0], &funding_redeemscript, self.channel_value_satoshis)[..]); match self.secp_ctx.verify(&sighash, &msg.signature, &self.their_funding_pubkey.unwrap()) { Ok(_) => {}, @@ -2566,7 +2566,7 @@ impl Channel { // The remote end may have decided to revoke their output due to inconsistent dust // limits, so check for that case by re-checking the signature here. closing_tx = self.build_closing_transaction(msg.fee_satoshis, true).0; - sighash = Message::from_slice(&bip143::SighashComponents::new(&closing_tx).sighash_all(&closing_tx.input[0], &funding_redeemscript, self.channel_value_satoshis)[..]).unwrap(); + sighash = hash_to_message!(&bip143::SighashComponents::new(&closing_tx).sighash_all(&closing_tx.input[0], &funding_redeemscript, self.channel_value_satoshis)[..]); secp_check!(self.secp_ctx.verify(&sighash, &msg.signature, &self.their_funding_pubkey.unwrap()), "Invalid closing tx signature from peer"); }, }; @@ -2584,7 +2584,7 @@ impl Channel { ($new_feerate: expr) => { let closing_tx_max_weight = Self::get_closing_transaction_weight(&self.get_closing_scriptpubkey(), self.their_shutdown_scriptpubkey.as_ref().unwrap()); let (closing_tx, used_total_fee) = self.build_closing_transaction($new_feerate * closing_tx_max_weight / 1000, false); - sighash = Message::from_slice(&bip143::SighashComponents::new(&closing_tx).sighash_all(&closing_tx.input[0], &funding_redeemscript, self.channel_value_satoshis)[..]).unwrap(); + sighash = hash_to_message!(&bip143::SighashComponents::new(&closing_tx).sighash_all(&closing_tx.input[0], &funding_redeemscript, self.channel_value_satoshis)[..]); let our_sig = self.secp_ctx.sign(&sighash, &self.local_keys.funding_key); self.last_sent_closing_fee = Some(($new_feerate, used_total_fee)); return Ok((Some(msgs::ClosingSigned { @@ -2993,7 +2993,7 @@ impl Channel { let remote_keys = self.build_remote_transaction_keys()?; let remote_initial_commitment_tx = self.build_commitment_transaction(self.cur_remote_commitment_transaction_number, &remote_keys, false, false, self.feerate_per_kw).0; - let remote_sighash = Message::from_slice(&bip143::SighashComponents::new(&remote_initial_commitment_tx).sighash_all(&remote_initial_commitment_tx.input[0], &funding_script, self.channel_value_satoshis)[..]).unwrap(); + let remote_sighash = hash_to_message!(&bip143::SighashComponents::new(&remote_initial_commitment_tx).sighash_all(&remote_initial_commitment_tx.input[0], &funding_script, self.channel_value_satoshis)[..]); // We sign the "remote" commitment transaction, allowing them to broadcast the tx if they wish. Ok((self.secp_ctx.sign(&remote_sighash, &self.local_keys.funding_key), remote_initial_commitment_tx)) @@ -3080,7 +3080,7 @@ impl Channel { excess_data: Vec::new(), }; - let msghash = Message::from_slice(&Sha256dHash::from_data(&msg.encode()[..])[..]).unwrap(); + let msghash = hash_to_message!(&Sha256dHash::from_data(&msg.encode()[..])[..]); let sig = self.secp_ctx.sign(&msghash, &self.local_keys.funding_key); Ok((msg, sig)) @@ -3295,7 +3295,7 @@ impl Channel { let remote_keys = self.build_remote_transaction_keys()?; let remote_commitment_tx = self.build_commitment_transaction(self.cur_remote_commitment_transaction_number, &remote_keys, false, true, feerate_per_kw); let remote_commitment_txid = remote_commitment_tx.0.txid(); - let remote_sighash = Message::from_slice(&bip143::SighashComponents::new(&remote_commitment_tx.0).sighash_all(&remote_commitment_tx.0.input[0], &funding_script, self.channel_value_satoshis)[..]).unwrap(); + let remote_sighash = hash_to_message!(&bip143::SighashComponents::new(&remote_commitment_tx.0).sighash_all(&remote_commitment_tx.0.input[0], &funding_script, self.channel_value_satoshis)[..]); let our_sig = self.secp_ctx.sign(&remote_sighash, &self.local_keys.funding_key); let mut htlc_sigs = Vec::with_capacity(remote_commitment_tx.1); @@ -3303,7 +3303,7 @@ impl Channel { if let Some(_) = htlc.transaction_output_index { let htlc_tx = self.build_htlc_transaction(&remote_commitment_txid, htlc, false, &remote_keys, feerate_per_kw); let htlc_redeemscript = chan_utils::get_htlc_redeemscript(&htlc, &remote_keys); - let htlc_sighash = Message::from_slice(&bip143::SighashComponents::new(&htlc_tx).sighash_all(&htlc_tx.input[0], &htlc_redeemscript, htlc.amount_msat / 1000)[..]).unwrap(); + let htlc_sighash = hash_to_message!(&bip143::SighashComponents::new(&htlc_tx).sighash_all(&htlc_tx.input[0], &htlc_redeemscript, htlc.amount_msat / 1000)[..]); let our_htlc_key = secp_check!(chan_utils::derive_private_key(&self.secp_ctx, &remote_keys.per_commitment_point, &self.local_keys.htlc_base_key), "Derived invalid key, peer is maliciously selecting parameters"); htlc_sigs.push(self.secp_ctx.sign(&htlc_sighash, &our_htlc_key)); } diff --git a/src/ln/channelmanager.rs b/src/ln/channelmanager.rs index f3d51e4bb..c9cd59476 100644 --- a/src/ln/channelmanager.rs +++ b/src/ln/channelmanager.rs @@ -20,7 +20,7 @@ use bitcoin_hashes::sha256::Hash as Sha256; use bitcoin_hashes::cmp::fixed_time_eq; use secp256k1::key::{SecretKey,PublicKey}; -use secp256k1::{Secp256k1,Message}; +use secp256k1::Secp256k1; use secp256k1::ecdh::SharedSecret; use secp256k1; @@ -937,7 +937,7 @@ impl ChannelManager { }; let msg_hash = Sha256dHash::from_data(&unsigned.encode()[..]); - let sig = self.secp_ctx.sign(&Message::from_slice(&msg_hash[..]).unwrap(), &self.our_network_key); + let sig = self.secp_ctx.sign(&hash_to_message!(&msg_hash[..]), &self.our_network_key); Ok(msgs::ChannelUpdate { signature: sig, @@ -1130,7 +1130,7 @@ impl ChannelManager { Ok(res) => res, Err(_) => return None, // Only in case of state precondition violations eg channel is closing }; - let msghash = Message::from_slice(&Sha256dHash::from_data(&announcement.encode()[..])[..]).unwrap(); + let msghash = hash_to_message!(&Sha256dHash::from_data(&announcement.encode()[..])[..]); let our_node_sig = self.secp_ctx.sign(&msghash, &self.our_network_key); Some(msgs::AnnouncementSignatures { @@ -2101,7 +2101,7 @@ impl ChannelManager { try_chan_entry!(self, chan.get_mut().get_channel_announcement(our_node_id.clone(), self.genesis_hash.clone()), channel_state, chan); let were_node_one = announcement.node_id_1 == our_node_id; - let msghash = Message::from_slice(&Sha256dHash::from_data(&announcement.encode()[..])[..]).unwrap(); + let msghash = hash_to_message!(&Sha256dHash::from_data(&announcement.encode()[..])[..]); if self.secp_ctx.verify(&msghash, &msg.node_signature, if were_node_one { &announcement.node_id_2 } else { &announcement.node_id_1 }).is_err() || self.secp_ctx.verify(&msghash, &msg.bitcoin_signature, if were_node_one { &announcement.bitcoin_key_2 } else { &announcement.bitcoin_key_1 }).is_err() { try_chan_entry!(self, Err(ChannelError::Close("Bad announcement_signatures node_signature")), channel_state, chan); diff --git a/src/ln/channelmonitor.rs b/src/ln/channelmonitor.rs index 01634f9d5..8e70ce7ad 100644 --- a/src/ln/channelmonitor.rs +++ b/src/ln/channelmonitor.rs @@ -24,7 +24,7 @@ use bitcoin_hashes::Hash; use bitcoin_hashes::sha256::Hash as Sha256; use bitcoin_hashes::hash160::Hash as Hash160; -use secp256k1::{Secp256k1,Message,Signature}; +use secp256k1::{Secp256k1,Signature}; use secp256k1::key::{SecretKey,PublicKey}; use secp256k1; @@ -1105,7 +1105,7 @@ impl ChannelMonitor { let htlc = &per_commitment_option.unwrap()[$htlc_idx.unwrap()].0; chan_utils::get_htlc_redeemscript_with_explicit_keys(htlc, &a_htlc_key, &b_htlc_key, &revocation_pubkey) }; - let sighash = ignore_error!(Message::from_slice(&$sighash_parts.sighash_all(&$input, &redeemscript, $amount)[..])); + let sighash = hash_to_message!(&$sighash_parts.sighash_all(&$input, &redeemscript, $amount)[..]); let revocation_key = ignore_error!(chan_utils::derive_private_revocation_key(&self.secp_ctx, &per_commitment_key, &revocation_base_key)); (self.secp_ctx.sign(&sighash, &revocation_key), redeemscript) }, @@ -1327,7 +1327,7 @@ impl ChannelMonitor { Storage::Local { ref htlc_base_key, .. } => { let htlc = &per_commitment_option.unwrap()[$input.sequence as usize].0; let redeemscript = chan_utils::get_htlc_redeemscript_with_explicit_keys(htlc, &a_htlc_key, &b_htlc_key, &revocation_pubkey); - let sighash = ignore_error!(Message::from_slice(&$sighash_parts.sighash_all(&$input, &redeemscript, $amount)[..])); + let sighash = hash_to_message!(&$sighash_parts.sighash_all(&$input, &redeemscript, $amount)[..]); let htlc_key = ignore_error!(chan_utils::derive_private_key(&self.secp_ctx, revocation_point, &htlc_base_key)); (self.secp_ctx.sign(&sighash, &htlc_key), redeemscript) }, @@ -1512,7 +1512,7 @@ impl ChannelMonitor { let sig = match self.key_storage { Storage::Local { ref revocation_base_key, .. } => { - let sighash = ignore_error!(Message::from_slice(&sighash_parts.sighash_all(&spend_tx.input[0], &redeemscript, amount)[..])); + let sighash = hash_to_message!(&sighash_parts.sighash_all(&spend_tx.input[0], &redeemscript, amount)[..]); let revocation_key = ignore_error!(chan_utils::derive_private_revocation_key(&self.secp_ctx, &per_commitment_key, &revocation_base_key)); self.secp_ctx.sign(&sighash, &revocation_key) } diff --git a/src/ln/router.rs b/src/ln/router.rs index dd98e188f..e25f9b223 100644 --- a/src/ln/router.rs +++ b/src/ln/router.rs @@ -4,7 +4,7 @@ //! interrogate it to get routes for your own payments. use secp256k1::key::PublicKey; -use secp256k1::{Secp256k1,Message}; +use secp256k1::Secp256k1; use secp256k1; use bitcoin::util::hash::Sha256dHash; @@ -239,7 +239,7 @@ macro_rules! secp_verify_sig { impl RoutingMessageHandler for Router { fn handle_node_announcement(&self, msg: &msgs::NodeAnnouncement) -> Result { - let msg_hash = Message::from_slice(&Sha256dHash::from_data(&msg.contents.encode()[..])[..]).unwrap(); + let msg_hash = hash_to_message!(&Sha256dHash::from_data(&msg.contents.encode()[..])[..]); secp_verify_sig!(self.secp_ctx, &msg_hash, &msg.signature, &msg.contents.node_id); if msg.contents.features.requires_unknown_bits() { @@ -272,7 +272,7 @@ impl RoutingMessageHandler for Router { return Err(HandleError{err: "Channel announcement node had a channel with itself", action: Some(ErrorAction::IgnoreError)}); } - let msg_hash = Message::from_slice(&Sha256dHash::from_data(&msg.contents.encode()[..])[..]).unwrap(); + let msg_hash = hash_to_message!(&Sha256dHash::from_data(&msg.contents.encode()[..])[..]); secp_verify_sig!(self.secp_ctx, &msg_hash, &msg.node_signature_1, &msg.contents.node_id_1); secp_verify_sig!(self.secp_ctx, &msg_hash, &msg.node_signature_2, &msg.contents.node_id_2); secp_verify_sig!(self.secp_ctx, &msg_hash, &msg.bitcoin_signature_1, &msg.contents.bitcoin_key_1); @@ -448,7 +448,7 @@ impl RoutingMessageHandler for Router { }; } } - let msg_hash = Message::from_slice(&Sha256dHash::from_data(&msg.contents.encode()[..])[..]).unwrap(); + let msg_hash = hash_to_message!(&Sha256dHash::from_data(&msg.contents.encode()[..])[..]); if msg.contents.flags & 1 == 1 { dest_node_id = channel.one_to_two.src_node_id.clone(); secp_verify_sig!(self.secp_ctx, &msg_hash, &msg.signature, &channel.two_to_one.src_node_id); diff --git a/src/util/fuzz_wrappers.rs b/src/util/fuzz_wrappers.rs new file mode 100644 index 000000000..508acf08d --- /dev/null +++ b/src/util/fuzz_wrappers.rs @@ -0,0 +1,17 @@ +macro_rules! hash_to_message { + ($slice: expr) => { + { + #[cfg(not(feature = "fuzztarget"))] + { + ::secp256k1::Message::from_slice($slice).unwrap() + } + #[cfg(feature = "fuzztarget")] + { + match ::secp256k1::Message::from_slice($slice) { + Ok(msg) => msg, + Err(_) => ::secp256k1::Message::from_slice(&[1; 32]).unwrap() + } + } + } + } +} diff --git a/src/util/mod.rs b/src/util/mod.rs index c1b6c2300..e4170590d 100644 --- a/src/util/mod.rs +++ b/src/util/mod.rs @@ -27,3 +27,6 @@ pub use self::rng::{reset_rng_state, fill_bytes}; #[cfg(test)] pub(crate) mod test_utils; + +#[macro_use] +pub(crate) mod fuzz_wrappers; -- 2.39.5