From: Matt Corallo Date: Thu, 8 Nov 2018 00:06:34 +0000 (+1030) Subject: Fix pre-noise peer disconnect panic on non-Err disconnect X-Git-Tag: v0.0.12~267^2~2 X-Git-Url: http://git.bitcoin.ninja/index.cgi?a=commitdiff_plain;h=b4fc5b65e06e97e94aa5f6bd84fb25f4ce208c6b;p=rust-lightning Fix pre-noise peer disconnect panic on non-Err disconnect 366e79615b7251771465d6c69c2941ac233674da fixed the same crash for Errs that come up during handshake, but was incomplete and should have just dropped the node_id being different based on inbound/outbound. This patch does so and actually fixes the issue. Found by fuzzer. --- diff --git a/src/ln/peer_channel_encryptor.rs b/src/ln/peer_channel_encryptor.rs index 6a2033e4..eaf2f683 100644 --- a/src/ln/peer_channel_encryptor.rs +++ b/src/ln/peer_channel_encryptor.rs @@ -279,7 +279,7 @@ impl PeerChannelEncryptor { self.process_act_one_with_ephemeral_key(act_one, our_node_secret, our_ephemeral_key) } - pub fn process_act_two(&mut self, act_two: &[u8], our_node_secret: &SecretKey) -> Result<[u8; 66], HandleError> { + pub fn process_act_two(&mut self, act_two: &[u8], our_node_secret: &SecretKey) -> Result<([u8; 66], PublicKey), HandleError> { assert_eq!(act_two.len(), 50); let mut final_hkdf = [0; 64]; @@ -334,7 +334,7 @@ impl PeerChannelEncryptor { rck: ck, }; - Ok(res) + Ok((res, self.their_node_id.unwrap().clone())) } pub fn process_act_three(&mut self, act_three: &[u8]) -> Result { @@ -537,7 +537,7 @@ mod tests { let mut outbound_peer = get_outbound_peer_for_initiator_test_vectors(); let act_two = hex::decode("0002466d7fcae563e5cb09a0d1870bb580344804617879a14949cf22285f1bae3f276e2470b93aac583c9ef6eafca3f730ae").unwrap().to_vec(); - assert_eq!(outbound_peer.process_act_two(&act_two[..], &our_node_id).unwrap()[..], hex::decode("00b9e3a702e93e3a9948c2ed6e5fd7590a6e1c3a0344cfc9d5b57357049aa22355361aa02e55a8fc28fef5bd6d71ad0c38228dc68b1c466263b47fdf31e560e139ba").unwrap()[..]); + assert_eq!(outbound_peer.process_act_two(&act_two[..], &our_node_id).unwrap().0[..], hex::decode("00b9e3a702e93e3a9948c2ed6e5fd7590a6e1c3a0344cfc9d5b57357049aa22355361aa02e55a8fc28fef5bd6d71ad0c38228dc68b1c466263b47fdf31e560e139ba").unwrap()[..]); match outbound_peer.noise_state { NoiseState::Finished { sk, sn, sck, rk, rn, rck } => { @@ -694,7 +694,7 @@ mod tests { let our_node_id = SecretKey::from_slice(&secp_ctx, &hex::decode("1111111111111111111111111111111111111111111111111111111111111111").unwrap()[..]).unwrap(); let act_two = hex::decode("0002466d7fcae563e5cb09a0d1870bb580344804617879a14949cf22285f1bae3f276e2470b93aac583c9ef6eafca3f730ae").unwrap().to_vec(); - assert_eq!(outbound_peer.process_act_two(&act_two[..], &our_node_id).unwrap()[..], hex::decode("00b9e3a702e93e3a9948c2ed6e5fd7590a6e1c3a0344cfc9d5b57357049aa22355361aa02e55a8fc28fef5bd6d71ad0c38228dc68b1c466263b47fdf31e560e139ba").unwrap()[..]); + assert_eq!(outbound_peer.process_act_two(&act_two[..], &our_node_id).unwrap().0[..], hex::decode("00b9e3a702e93e3a9948c2ed6e5fd7590a6e1c3a0344cfc9d5b57357049aa22355361aa02e55a8fc28fef5bd6d71ad0c38228dc68b1c466263b47fdf31e560e139ba").unwrap()[..]); match outbound_peer.noise_state { NoiseState::Finished { sk, sn, sck, rk, rn, rck } => { diff --git a/src/ln/peer_handler.rs b/src/ln/peer_handler.rs index 87a2e6d1..1ea82547 100644 --- a/src/ln/peer_handler.rs +++ b/src/ln/peer_handler.rs @@ -234,7 +234,7 @@ impl PeerManager { if peers.peers.insert(descriptor, Peer { channel_encryptor: peer_encryptor, outbound: true, - their_node_id: Some(their_node_id), + their_node_id: None, their_global_features: None, their_local_features: None, @@ -438,9 +438,6 @@ impl PeerManager { macro_rules! try_potential_handleerror { ($thing: expr) => { - try_potential_handleerror!($thing, false); - }; - ($thing: expr, $pre_noise: expr) => { match $thing { Ok(x) => x, Err(e) => { @@ -449,9 +446,6 @@ impl PeerManager { msgs::ErrorAction::DisconnectPeer { msg: _ } => { //TODO: Try to push msg log_trace!(self, "Got Err handling message, disconnecting peer because {}", e.err); - if $pre_noise { - peer.their_node_id = None; // Unset so that we don't generate a peer_disconnected event - } return Err(PeerHandleError{ no_connection_possible: false }); }, msgs::ErrorAction::IgnoreError => { @@ -517,16 +511,17 @@ impl PeerManager { let next_step = peer.channel_encryptor.get_noise_step(); match next_step { NextNoiseStep::ActOne => { - let act_two = try_potential_handleerror!(peer.channel_encryptor.process_act_one_with_key(&peer.pending_read_buffer[..], &self.our_node_secret), true).to_vec(); + let act_two = try_potential_handleerror!(peer.channel_encryptor.process_act_one_with_key(&peer.pending_read_buffer[..], &self.our_node_secret)).to_vec(); peer.pending_outbound_buffer.push_back(act_two); peer.pending_read_buffer = [0; 66].to_vec(); // act three is 66 bytes long }, NextNoiseStep::ActTwo => { - let act_three = try_potential_handleerror!(peer.channel_encryptor.process_act_two(&peer.pending_read_buffer[..], &self.our_node_secret), true).to_vec(); - peer.pending_outbound_buffer.push_back(act_three); + let (act_three, their_node_id) = try_potential_handleerror!(peer.channel_encryptor.process_act_two(&peer.pending_read_buffer[..], &self.our_node_secret)); + peer.pending_outbound_buffer.push_back(act_three.to_vec()); peer.pending_read_buffer = [0; 18].to_vec(); // Message length header is 18 bytes peer.pending_read_is_header = true; + peer.their_node_id = Some(their_node_id); insert_node_id!(); let mut local_features = msgs::LocalFeatures::new(); if self.initial_syncs_sent.load(Ordering::Acquire) < INITIAL_SYNCS_TO_SEND { @@ -539,7 +534,7 @@ impl PeerManager { }, 16); }, NextNoiseStep::ActThree => { - let their_node_id = try_potential_handleerror!(peer.channel_encryptor.process_act_three(&peer.pending_read_buffer[..]), true); + let their_node_id = try_potential_handleerror!(peer.channel_encryptor.process_act_three(&peer.pending_read_buffer[..])); peer.pending_read_buffer = [0; 18].to_vec(); // Message length header is 18 bytes peer.pending_read_is_header = true; peer.their_node_id = Some(their_node_id);