From 61164afb2bdd19e487ca3ec1c6435baf65e0c3af Mon Sep 17 00:00:00 2001 From: Matt Corallo Date: Fri, 15 Jan 2021 21:34:17 -0500 Subject: [PATCH] Check the PK of the source of an error before closing chans from it 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 #105, we eventually need to change the indexing anyway to allow absurdly terrible peers to open channels with us. Found during review of #777. --- lightning/src/ln/channelmanager.rs | 30 ++++++++++++++++++++---------- 1 file changed, 20 insertions(+), 10 deletions(-) diff --git a/lightning/src/ln/channelmanager.rs b/lightning/src/ln/channelmanager.rs index e353700be..621013724 100644 --- a/lightning/src/ln/channelmanager.rs +++ b/lightning/src/ln/channelmanager.rs @@ -916,19 +916,22 @@ impl } } - /// 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()}); } @@ -945,6 +948,13 @@ impl 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) { @@ -3474,12 +3484,12 @@ impl