From 61164afb2bdd19e487ca3ec1c6435baf65e0c3af Mon Sep 17 00:00:00 2001 From: Matt Corallo Date: Fri, 15 Jan 2021 21:34:17 -0500 Subject: [PATCH 1/3] 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 e353700be16..621013724bc 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 Date: Sun, 7 Feb 2021 18:49:48 -0500 Subject: [PATCH 2/3] Add test for error message hangline resulting in force-close --- lightning/src/ln/functional_tests.rs | 63 ++++++++++++++++++++++++++++ 1 file changed, 63 insertions(+) diff --git a/lightning/src/ln/functional_tests.rs b/lightning/src/ln/functional_tests.rs index 10c16c75b0f..77b791e0136 100644 --- a/lightning/src/ln/functional_tests.rs +++ b/lightning/src/ln/functional_tests.rs @@ -8836,3 +8836,66 @@ fn test_duplicate_chan_id() { update_nodes_with_chan_announce(&nodes, 0, 1, &announcement, &as_update, &bs_update); send_payment(&nodes[0], &[&nodes[1]], 8000000, 8_000_000); } + +#[test] +fn test_error_chans_closed() { + // Test that we properly handle error messages, closing appropriate channels. + // + // Prior to #787 we'd allow a peer to make us force-close a channel we had with a different + // peer. The "real" fix for that is to index channels with peers_ids, however in the mean time + // we can test various edge cases around it to ensure we don't regress. + let chanmon_cfgs = create_chanmon_cfgs(3); + let node_cfgs = create_node_cfgs(3, &chanmon_cfgs); + let node_chanmgrs = create_node_chanmgrs(3, &node_cfgs, &[None, None, None]); + let nodes = create_network(3, &node_cfgs, &node_chanmgrs); + + // Create some initial channels + let chan_1 = create_announced_chan_between_nodes_with_value(&nodes, 0, 1, 100000, 10001, InitFeatures::known(), InitFeatures::known()); + let chan_2 = create_announced_chan_between_nodes_with_value(&nodes, 0, 1, 100000, 10001, InitFeatures::known(), InitFeatures::known()); + let chan_3 = create_announced_chan_between_nodes_with_value(&nodes, 0, 2, 100000, 10001, InitFeatures::known(), InitFeatures::known()); + + assert_eq!(nodes[0].node.list_usable_channels().len(), 3); + assert_eq!(nodes[1].node.list_usable_channels().len(), 2); + assert_eq!(nodes[2].node.list_usable_channels().len(), 1); + + // Closing a channel from a different peer has no effect + nodes[0].node.handle_error(&nodes[1].node.get_our_node_id(), &msgs::ErrorMessage { channel_id: chan_3.2, data: "ERR".to_owned() }); + assert_eq!(nodes[0].node.list_usable_channels().len(), 3); + + // Closing one channel doesn't impact others + nodes[0].node.handle_error(&nodes[1].node.get_our_node_id(), &msgs::ErrorMessage { channel_id: chan_2.2, data: "ERR".to_owned() }); + check_added_monitors!(nodes[0], 1); + check_closed_broadcast!(nodes[0], false); + assert_eq!(nodes[0].node.list_usable_channels().len(), 2); + assert!(nodes[0].node.list_usable_channels()[0].channel_id == chan_1.2 || nodes[0].node.list_usable_channels()[1].channel_id == chan_1.2); + assert!(nodes[0].node.list_usable_channels()[0].channel_id == chan_3.2 || nodes[0].node.list_usable_channels()[1].channel_id == chan_3.2); + + // A null channel ID should close all channels + let _chan_4 = create_announced_chan_between_nodes_with_value(&nodes, 0, 1, 100000, 10001, InitFeatures::known(), InitFeatures::known()); + nodes[0].node.handle_error(&nodes[1].node.get_our_node_id(), &msgs::ErrorMessage { channel_id: [0; 32], data: "ERR".to_owned() }); + check_added_monitors!(nodes[0], 2); + let events = nodes[0].node.get_and_clear_pending_msg_events(); + assert_eq!(events.len(), 2); + match events[0] { + MessageSendEvent::BroadcastChannelUpdate { ref msg } => { + assert_eq!(msg.contents.flags & 2, 2); + }, + _ => panic!("Unexpected event"), + } + match events[1] { + MessageSendEvent::BroadcastChannelUpdate { ref msg } => { + assert_eq!(msg.contents.flags & 2, 2); + }, + _ => panic!("Unexpected event"), + } + // Note that at this point users of a standard PeerHandler will end up calling + // peer_disconnected with no_connection_possible set to false, duplicating the + // close-all-channels logic. That's OK, we don't want to end up not force-closing channels for + // users with their own peer handling logic. We duplicate the call here, however. + assert_eq!(nodes[0].node.list_usable_channels().len(), 1); + assert!(nodes[0].node.list_usable_channels()[0].channel_id == chan_3.2); + + nodes[0].node.peer_disconnected(&nodes[1].node.get_our_node_id(), true); + assert_eq!(nodes[0].node.list_usable_channels().len(), 1); + assert!(nodes[0].node.list_usable_channels()[0].channel_id == chan_3.2); +} From bd8382a4d34e20e82f3b8584b1d0d6910b7b0ad8 Mon Sep 17 00:00:00 2001 From: Matt Corallo Date: Sun, 31 Jan 2021 20:53:28 -0500 Subject: [PATCH 3/3] Fix documentation on PeerHandleError --- lightning/src/ln/peer_handler.rs | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/lightning/src/ln/peer_handler.rs b/lightning/src/ln/peer_handler.rs index e083c0119ce..742b1ff3a1f 100644 --- a/lightning/src/ln/peer_handler.rs +++ b/lightning/src/ln/peer_handler.rs @@ -88,10 +88,8 @@ pub trait SocketDescriptor : cmp::Eq + hash::Hash + Clone { } /// Error for PeerManager errors. If you get one of these, you must disconnect the socket and -/// generate no further read_event/write_buffer_space_avail calls for the descriptor, only -/// triggering a single socket_disconnected call (unless it was provided in response to a -/// new_*_connection event, in which case no such socket_disconnected() must be called and the -/// socket silently disconencted). +/// generate no further read_event/write_buffer_space_avail/socket_disconnected calls for the +/// descriptor. pub struct PeerHandleError { /// Used to indicate that we probably can't make any future connections to this peer, implying /// we should go ahead and force-close any channels we have with it.