Skip to content

Commit

Permalink
Check the PK of the source of an error before closing chans from it
Browse files Browse the repository at this point in the history
When we receive an error message from a peer, it can indicate a
channel which we should close. However, we previously did not
check that the counterparty who sends us such a message is the
counterparty with whom we have the channel, allowing any
connected peer to make us force-close any channel we have as long
as they know the channel id.

This commit simply changes the force-close logic to check that the
sender matches the channel's counterparty node_id, though as noted
in lightningdevkit#105, we eventually need to change the indexing anyway to allow
absurdly terrible peers to open channels with us.

Found during review of lightningdevkit#777.
  • Loading branch information
TheBlueMatt authored and valentinewallace committed Feb 18, 2021
1 parent 6dcb7c4 commit e13697a
Showing 1 changed file with 20 additions and 10 deletions.
30 changes: 20 additions & 10 deletions lightning/src/ln/channelmanager.rs
Original file line number Diff line number Diff line change
Expand Up @@ -916,19 +916,22 @@ impl<ChanSigner: ChannelKeys, M: Deref, T: Deref, K: Deref, F: Deref, L: Deref>
}
}

/// Force closes a channel, immediately broadcasting the latest local commitment transaction to
/// the chain and rejecting new HTLCs on the given channel. Fails if channel_id is unknown to the manager.
pub fn force_close_channel(&self, channel_id: &[u8; 32]) -> Result<(), APIError>{
let _consistency_lock = self.total_consistency_lock.read().unwrap();

fn force_close_channel_with_peer(&self, channel_id: &[u8; 32], peer_node_id: Option<&PublicKey>) -> Result<(), APIError> {
let mut chan = {
let mut channel_state_lock = self.channel_state.lock().unwrap();
let channel_state = &mut *channel_state_lock;
if let Some(chan) = channel_state.by_id.remove(channel_id) {
if let Some(short_id) = chan.get_short_channel_id() {
if let hash_map::Entry::Occupied(chan) = channel_state.by_id.entry(channel_id.clone()) {
if let Some(node_id) = peer_node_id {
if chan.get().get_counterparty_node_id() != *node_id {
// Error or Ok here doesn't matter - the result is only exposed publicly
// when peer_node_id is None anyway.
return Ok(());
}
}
if let Some(short_id) = chan.get().get_short_channel_id() {
channel_state.short_to_id.remove(&short_id);
}
chan
chan.remove_entry().1
} else {
return Err(APIError::ChannelUnavailable{err: "No such channel".to_owned()});
}
Expand All @@ -945,6 +948,13 @@ impl<ChanSigner: ChannelKeys, M: Deref, T: Deref, K: Deref, F: Deref, L: Deref>
Ok(())
}

/// Force closes a channel, immediately broadcasting the latest local commitment transaction to
/// the chain and rejecting new HTLCs on the given channel. Fails if channel_id is unknown to the manager.
pub fn force_close_channel(&self, channel_id: &[u8; 32]) -> Result<(), APIError> {
let _consistency_lock = self.total_consistency_lock.read().unwrap();
self.force_close_channel_with_peer(channel_id, None)
}

/// Force close all channels, immediately broadcasting the latest local commitment transaction
/// for each to the chain and rejecting new HTLCs on each.
pub fn force_close_all_channels(&self) {
Expand Down Expand Up @@ -3474,12 +3484,12 @@ impl<ChanSigner: ChannelKeys, M: Deref + Sync + Send, T: Deref + Sync + Send, K:
for chan in self.list_channels() {
if chan.remote_network_id == *counterparty_node_id {
// Untrusted messages from peer, we throw away the error if id points to a non-existent channel
let _ = self.force_close_channel(&msg.channel_id);
let _ = self.force_close_channel_with_peer(&chan.channel_id, Some(counterparty_node_id));
}
}
} else {
// Untrusted messages from peer, we throw away the error if id points to a non-existent channel
let _ = self.force_close_channel(&msg.channel_id);
let _ = self.force_close_channel_with_peer(&msg.channel_id, Some(counterparty_node_id));
}
}
}
Expand Down

0 comments on commit e13697a

Please sign in to comment.