Fix (and test) panic when our counterparty uses a bogus funding tx 2021-04-fix-chan-shutdown-crash
authorMatt Corallo <git@bluematt.me>
Wed, 21 Apr 2021 00:11:54 +0000 (00:11 +0000)
committerMatt Corallo <git@bluematt.me>
Fri, 23 Apr 2021 22:52:43 +0000 (22:52 +0000)
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
lightning/src/ln/channelmanager.rs
lightning/src/ln/functional_tests.rs

index 5f987427979cbb9d267a73ea7f3fc0b0dd374378..1c34e3391493fe33a9bddff6c2c680a66f9eac34 100644 (file)
@@ -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(),
index 39426b3fa7b2656b7667c29b94d94c50d6b4cc96..0360f8a69a8649dcb72e03fcf295ed2f2e9daa59 100644 (file)
@@ -1563,61 +1563,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 {
@@ -1653,6 +1606,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()));
index 9a0c06e776cf4db266050b7986d08dc7fab67d2b..f3b8ca15594dcccfa6ec0965a59e94e8c1957f14 100644 (file)
@@ -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);
+}