From eb42caf8a0608ab004b8d3ae179015702ea8a64d Mon Sep 17 00:00:00 2001 From: Matt Corallo Date: Wed, 21 Apr 2021 00:11:54 +0000 Subject: [PATCH] Fix (and test) panic when our counterparty uses a bogus funding tx During the block API refactor, we started calling Channel::force_shutdown when a channel is closed due to a bogus funding tx. However, we still set the channel's state to Shutdown prior to doing so, leading to an assertion in force_shutdown (that the channel is not already closed). This removes the state-set call and adds a (long-overdue) test for this case. Fixes: 60b962a18ebcf494340ddc001870f8160c625968 --- lightning/src/ln/channel.rs | 1 - lightning/src/ln/channelmanager.rs | 119 +++++++++++++++------------ lightning/src/ln/functional_tests.rs | 50 +++++++++++ 3 files changed, 117 insertions(+), 53 deletions(-) diff --git a/lightning/src/ln/channel.rs b/lightning/src/ln/channel.rs index 5f9874279..1c34e3391 100644 --- a/lightning/src/ln/channel.rs +++ b/lightning/src/ln/channel.rs @@ -3574,7 +3574,6 @@ impl Channel { #[cfg(not(feature = "fuzztarget"))] panic!("Client called ChannelManager::funding_transaction_generated with bogus transaction!"); } - self.channel_state = ChannelState::ShutdownComplete as u32; self.update_time_counter += 1; return Err(msgs::ErrorMessage { channel_id: self.channel_id(), diff --git a/lightning/src/ln/channelmanager.rs b/lightning/src/ln/channelmanager.rs index 39426b3fa..0360f8a69 100644 --- a/lightning/src/ln/channelmanager.rs +++ b/lightning/src/ln/channelmanager.rs @@ -1563,61 +1563,14 @@ impl ChannelMana } } - /// Call this upon creation of a funding transaction for the given channel. - /// - /// Returns an [`APIError::APIMisuseError`] if the funding_transaction spent non-SegWit outputs - /// or if no output was found which matches the parameters in [`Event::FundingGenerationReady`]. - /// - /// Panics if a funding transaction has already been provided for this channel. - /// - /// May panic if the output found in the funding transaction is duplicative with some other - /// channel (note that this should be trivially prevented by using unique funding transaction - /// keys per-channel). - /// - /// Do NOT broadcast the funding transaction yourself. When we have safely received our - /// counterparty's signature the funding transaction will automatically be broadcast via the - /// [`BroadcasterInterface`] provided when this `ChannelManager` was constructed. - /// - /// Note that this includes RBF or similar transaction replacement strategies - lightning does - /// not currently support replacing a funding transaction on an existing channel. Instead, - /// create a new channel with a conflicting funding transaction. - pub fn funding_transaction_generated(&self, temporary_channel_id: &[u8; 32], funding_transaction: Transaction) -> Result<(), APIError> { - let _persistence_guard = PersistenceNotifierGuard::new(&self.total_consistency_lock, &self.persistence_notifier); - - for inp in funding_transaction.input.iter() { - if inp.witness.is_empty() { - return Err(APIError::APIMisuseError { - err: "Funding transaction must be fully signed and spend Segwit outputs".to_owned() - }); - } - } - + /// Handles the generation of a funding transaction, optionally (for tests) with a function + /// which checks the correctness of the funding transaction given the associated channel. + fn funding_transaction_generated_intern, &Transaction) -> Result> + (&self, temporary_channel_id: &[u8; 32], funding_transaction: Transaction, find_funding_output: FundingOutput) -> Result<(), APIError> { let (chan, msg) = { let (res, chan) = match self.channel_state.lock().unwrap().by_id.remove(temporary_channel_id) { Some(mut chan) => { - let mut output_index = None; - let expected_spk = chan.get_funding_redeemscript().to_v0_p2wsh(); - for (idx, outp) in funding_transaction.output.iter().enumerate() { - if outp.script_pubkey == expected_spk && outp.value == chan.get_value_satoshis() { - if output_index.is_some() { - return Err(APIError::APIMisuseError { - err: "Multiple outputs matched the expected script and value".to_owned() - }); - } - if idx > u16::max_value() as usize { - return Err(APIError::APIMisuseError { - err: "Transaction had more than 2^16 outputs, which is not supported".to_owned() - }); - } - output_index = Some(idx as u16); - } - } - if output_index.is_none() { - return Err(APIError::APIMisuseError { - err: "No output matched the script_pubkey and value in the FundingGenerationReady event".to_owned() - }); - } - let funding_txo = OutPoint { txid: funding_transaction.txid(), index: output_index.unwrap() }; + let funding_txo = find_funding_output(&chan, &funding_transaction)?; (chan.get_outbound_funding_created(funding_transaction, funding_txo, &self.logger) .map_err(|e| if let ChannelError::Close(msg) = e { @@ -1653,6 +1606,68 @@ impl ChannelMana Ok(()) } + #[cfg(test)] + pub(crate) fn funding_transaction_generated_unchecked(&self, temporary_channel_id: &[u8; 32], funding_transaction: Transaction, output_index: u16) -> Result<(), APIError> { + self.funding_transaction_generated_intern(temporary_channel_id, funding_transaction, |_, tx| { + Ok(OutPoint { txid: tx.txid(), index: output_index }) + }) + } + + /// Call this upon creation of a funding transaction for the given channel. + /// + /// Returns an [`APIError::APIMisuseError`] if the funding_transaction spent non-SegWit outputs + /// or if no output was found which matches the parameters in [`Event::FundingGenerationReady`]. + /// + /// Panics if a funding transaction has already been provided for this channel. + /// + /// May panic if the output found in the funding transaction is duplicative with some other + /// channel (note that this should be trivially prevented by using unique funding transaction + /// keys per-channel). + /// + /// Do NOT broadcast the funding transaction yourself. When we have safely received our + /// counterparty's signature the funding transaction will automatically be broadcast via the + /// [`BroadcasterInterface`] provided when this `ChannelManager` was constructed. + /// + /// Note that this includes RBF or similar transaction replacement strategies - lightning does + /// not currently support replacing a funding transaction on an existing channel. Instead, + /// create a new channel with a conflicting funding transaction. + pub fn funding_transaction_generated(&self, temporary_channel_id: &[u8; 32], funding_transaction: Transaction) -> Result<(), APIError> { + let _persistence_guard = PersistenceNotifierGuard::new(&self.total_consistency_lock, &self.persistence_notifier); + + for inp in funding_transaction.input.iter() { + if inp.witness.is_empty() { + return Err(APIError::APIMisuseError { + err: "Funding transaction must be fully signed and spend Segwit outputs".to_owned() + }); + } + } + self.funding_transaction_generated_intern(temporary_channel_id, funding_transaction, |chan, tx| { + let mut output_index = None; + let expected_spk = chan.get_funding_redeemscript().to_v0_p2wsh(); + for (idx, outp) in tx.output.iter().enumerate() { + if outp.script_pubkey == expected_spk && outp.value == chan.get_value_satoshis() { + if output_index.is_some() { + return Err(APIError::APIMisuseError { + err: "Multiple outputs matched the expected script and value".to_owned() + }); + } + if idx > u16::max_value() as usize { + return Err(APIError::APIMisuseError { + err: "Transaction had more than 2^16 outputs, which is not supported".to_owned() + }); + } + output_index = Some(idx as u16); + } + } + if output_index.is_none() { + return Err(APIError::APIMisuseError { + err: "No output matched the script_pubkey and value in the FundingGenerationReady event".to_owned() + }); + } + Ok(OutPoint { txid: tx.txid(), index: output_index.unwrap() }) + }) + } + fn get_announcement_sigs(&self, chan: &Channel) -> Option { if !chan.should_announce() { log_trace!(self.logger, "Can't send announcement_signatures for private channel {}", log_bytes!(chan.channel_id())); diff --git a/lightning/src/ln/functional_tests.rs b/lightning/src/ln/functional_tests.rs index 9a0c06e77..f3b8ca155 100644 --- a/lightning/src/ln/functional_tests.rs +++ b/lightning/src/ln/functional_tests.rs @@ -8825,3 +8825,53 @@ fn test_error_chans_closed() { assert_eq!(nodes[0].node.list_usable_channels().len(), 1); assert!(nodes[0].node.list_usable_channels()[0].channel_id == chan_3.2); } + +#[test] +fn test_invalid_funding_tx() { + // Test that we properly handle invalid funding transactions sent to us from a peer. + // + // Previously, all other major lightning implementations had failed to properly sanitize + // funding transactions from their counterparties, leading to a multi-implementation critical + // security vulnerability (though we always sanitized properly, we've previously had + // un-released crashes in the sanitization process). + let chanmon_cfgs = create_chanmon_cfgs(2); + let node_cfgs = create_node_cfgs(2, &chanmon_cfgs); + let node_chanmgrs = create_node_chanmgrs(2, &node_cfgs, &[None, None]); + let nodes = create_network(2, &node_cfgs, &node_chanmgrs); + + nodes[0].node.create_channel(nodes[1].node.get_our_node_id(), 100_000, 10_000, 42, None).unwrap(); + nodes[1].node.handle_open_channel(&nodes[0].node.get_our_node_id(), InitFeatures::known(), &get_event_msg!(nodes[0], MessageSendEvent::SendOpenChannel, nodes[1].node.get_our_node_id())); + nodes[0].node.handle_accept_channel(&nodes[1].node.get_our_node_id(), InitFeatures::known(), &get_event_msg!(nodes[1], MessageSendEvent::SendAcceptChannel, nodes[0].node.get_our_node_id())); + + let (temporary_channel_id, mut tx, _) = create_funding_transaction(&nodes[0], 100_000, 42); + for output in tx.output.iter_mut() { + // Make the confirmed funding transaction have a bogus script_pubkey + output.script_pubkey = bitcoin::Script::new(); + } + + nodes[0].node.funding_transaction_generated_unchecked(&temporary_channel_id, tx.clone(), 0).unwrap(); + nodes[1].node.handle_funding_created(&nodes[0].node.get_our_node_id(), &get_event_msg!(nodes[0], MessageSendEvent::SendFundingCreated, nodes[1].node.get_our_node_id())); + check_added_monitors!(nodes[1], 1); + + nodes[0].node.handle_funding_signed(&nodes[1].node.get_our_node_id(), &get_event_msg!(nodes[1], MessageSendEvent::SendFundingSigned, nodes[0].node.get_our_node_id())); + check_added_monitors!(nodes[0], 1); + + let events_1 = nodes[0].node.get_and_clear_pending_events(); + assert_eq!(events_1.len(), 0); + + assert_eq!(nodes[0].tx_broadcaster.txn_broadcasted.lock().unwrap().len(), 1); + assert_eq!(nodes[0].tx_broadcaster.txn_broadcasted.lock().unwrap()[0], tx); + nodes[0].tx_broadcaster.txn_broadcasted.lock().unwrap().clear(); + + confirm_transaction_at(&nodes[1], &tx, 1); + check_added_monitors!(nodes[1], 1); + let events_2 = nodes[1].node.get_and_clear_pending_msg_events(); + assert_eq!(events_2.len(), 1); + if let MessageSendEvent::HandleError { node_id, action } = &events_2[0] { + assert_eq!(*node_id, nodes[0].node.get_our_node_id()); + if let msgs::ErrorAction::SendErrorMessage { msg } = action { + assert_eq!(msg.data, "funding tx had wrong script/value or output index"); + } else { panic!(); } + } else { panic!(); } + assert_eq!(nodes[1].node.list_channels().len(), 0); +} -- 2.39.5