Skip to content

Commit

Permalink
Fix (and test) panic when our counterparty uses a bogus funding tx
Browse files Browse the repository at this point in the history 
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: 60b962a
  • Loading branch information
@TheBlueMatt
TheBlueMatt authored and Antoine Riard committed Apr 27, 2021
1 parent 0d75a63 commit f988fec
Show file tree
Hide file tree
Showing 3 changed files with 117 additions and 53 deletions.
1 change: 0 additions & 1 deletion lightning/src/ln/channel.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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(),
Expand Down
119 changes: 67 additions & 52 deletions lightning/src/ln/channelmanager.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand Down Expand Up @@ -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()));
Expand Down
50 changes: 50 additions & 0 deletions lightning/src/ln/functional_tests.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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);
}

0 comments on commit f988fec

Please sign in to comment.