Merge pull request #890 from TheBlueMatt/2021-04-fix-chan-shutdown-crash

Fix (and test) panic when our counterparty uses a bogus funding tx

Author: TheBlueMatt

lightning/src/ln/channel.rs

@@ -3574,7 +3574,6 @@ impl<Signer: Sign> Channel<Signer> {
 								#[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(),

lightning/src/ln/channelmanager.rs

@@ -1564,61 +1564,14 @@ impl<Signer: Sign, M: Deref, T: Deref, K: Deref, F: Deref, L: Deref> 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<FundingOutput: Fn(&Channel<Signer>, &Transaction) -> Result<OutPoint, APIError>>
+			(&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 {
@@ -1654,6 +1607,68 @@ impl<Signer: Sign, M: Deref, T: Deref, K: Deref, F: Deref, L: Deref> 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<Signer>) -> Option<msgs::AnnouncementSignatures> {
 		if !chan.should_announce() {
 			log_trace!(self.logger, "Can't send announcement_signatures for private channel {}", log_bytes!(chan.channel_id()));

lightning/src/ln/functional_tests.rs

@@ -8828,3 +8828,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);
+}