Merge pull request #131 from TheBlueMatt/2018-08-bolt-1-compliance

update Error/Init handling to be BOLT 1 compliant

Author: TheBlueMatt

fuzz/Cargo.toml

@@ -60,6 +60,10 @@ path = "fuzz_targets/msg_ping_target.rs"
 name = "msg_pong_target"
 path = "fuzz_targets/msg_pong_target.rs"
 
+[[bin]]
+name = "msg_error_message_target"
+path = "fuzz_targets/msg_error_message_target.rs"
+
 [[bin]]
 name = "msg_accept_channel_target"
 path = "fuzz_targets/msg_targets/msg_accept_channel_target.rs"
@@ -119,7 +123,3 @@ path = "fuzz_targets/msg_targets/msg_update_fail_htlc_target.rs"
 [[bin]]
 name = "msg_channel_reestablish_target"
 path = "fuzz_targets/msg_targets/msg_channel_reestablish_target.rs"
-
-[[bin]]
-name = "msg_error_message_target"
-path = "fuzz_targets/msg_targets/msg_error_message_target.rs"

fuzz/fuzz_targets/msg_targets/msg_error_message_target.rs renamed to fuzz/fuzz_targets/msg_error_message_target.rs

@@ -8,12 +8,14 @@ use lightning::util::reset_rng_state;
 
 use lightning::ln::msgs::{MsgEncodable, MsgDecodable};
 
-mod utils;
-
 #[inline]
 pub fn do_test(data: &[u8]) {
 	reset_rng_state();
-	test_msg!(msgs::ErrorMessage, data);
+	if let Ok(msg) = msgs::ErrorMessage::decode(data){
+		let enc = msg.encode();
+		assert_eq!(&data[0..32], &enc[0..32]);
+		assert_eq!(&data[34..enc.len()], &enc[34..]);
+	}
 }
 
 #[cfg(feature = "afl")]

fuzz/fuzz_targets/msg_targets/gen_target.sh

@@ -1,4 +1,4 @@
-for target in CommitmentSigned FundingCreated FundingLocked FundingSigned OpenChannel RevokeAndACK Shutdown UpdateAddHTLC UpdateFailHTLC UpdateFailMalformedHTLC UpdateFee UpdateFulfillHTLC AcceptChannel ClosingSigned ChannelReestablish ErrorMessage; do
+for target in CommitmentSigned FundingCreated FundingLocked FundingSigned OpenChannel RevokeAndACK Shutdown UpdateAddHTLC UpdateFailHTLC UpdateFailMalformedHTLC UpdateFee UpdateFulfillHTLC AcceptChannel ClosingSigned ChannelReestablish; do
 	tn=$(echo $target | sed 's/\([a-z0-9]\)\([A-Z]\)/\1_\2/g')
 	fn=msg_$(echo $tn | tr '[:upper:]' '[:lower:]')_target.rs
 	cat msg_target_template.txt | sed s/MSG_TARGET/$target/ > $fn

src/ln/channelmanager.rs

@@ -2053,6 +2053,18 @@ impl ChannelMessageHandler for ChannelManager {
 			}
 		}
 	}
+
+	fn handle_error(&self, their_node_id: &PublicKey, msg: &msgs::ErrorMessage) {
+		if msg.channel_id == [0; 32] {
+			for chan in self.list_channels() {
+				if chan.remote_network_id == *their_node_id {
+					self.force_close_channel(&chan.channel_id);
+				}
+			}
+		} else {
+			self.force_close_channel(&msg.channel_id);
+		}
+	}
 }
 
 #[cfg(test)]

src/ln/msgs.rs

@@ -5,7 +5,7 @@ use bitcoin::network::serialize::{deserialize,serialize};
 use bitcoin::blockdata::script::Script;
 
 use std::error::Error;
-use std::fmt;
+use std::{cmp, fmt};
 use std::result::Result;
 
 use util::{byte_utils, internal_traits, events};
@@ -439,12 +439,14 @@ pub trait ChannelMessageHandler : events::EventsProvider + Send + Sync {
 	// Channel-to-announce:
 	fn handle_announcement_signatures(&self, their_node_id: &PublicKey, msg: &AnnouncementSignatures) -> Result<(), HandleError>;
 
-	// Informational:
+	// Error conditions:
 	/// Indicates a connection to the peer failed/an existing connection was lost. If no connection
 	/// is believed to be possible in the future (eg they're sending us messages we don't
 	/// understand or indicate they require unknown feature bits), no_connection_possible is set
 	/// and any outstanding channels should be failed.
 	fn peer_disconnected(&self, their_node_id: &PublicKey, no_connection_possible: bool);
+
+	fn handle_error(&self, their_node_id: &PublicKey, msg: &ErrorMessage);
 }
 
 pub trait RoutingMessageHandler : Send + Sync {
@@ -1624,11 +1626,9 @@ impl MsgDecodable for ErrorMessage {
 		if v.len() < 34 {
 			return Err(DecodeError::ShortRead);
 		}
-		let len = byte_utils::slice_to_be16(&v[32..34]);
-		if v.len() < 34 + len as usize {
-			return Err(DecodeError::ShortRead);
-		}
-		let data = match String::from_utf8(v[34..34 + len as usize].to_vec()) {
+		// Unlike most messages, BOLT 1 requires we truncate our read if the value is out of range
+		let len = cmp::min(byte_utils::slice_to_be16(&v[32..34]) as usize, v.len() - 34);
+		let data = match String::from_utf8(v[34..34 + len].to_vec()) {
 			Ok(s) => s,
 			Err(_) => return Err(DecodeError::BadText),
 		};

src/ln/peer_handler.rs

@@ -431,7 +431,24 @@ impl<Descriptor: SocketDescriptor> PeerManager<Descriptor> {
 												}
 											},
 											17 => {
-												// Error msg
+												let msg = try_potential_decodeerror!(msgs::ErrorMessage::decode(&msg_data[2..]));
+												let mut data_is_printable = true;
+												for b in msg.data.bytes() {
+													if b < 32 || b > 126 {
+														data_is_printable = false;
+														break;
+													}
+												}
+
+												if data_is_printable {
+													log_debug!(self, "Got Err message from {}: {}", log_pubkey!(peer.their_node_id.unwrap()), msg.data);
+												} else {
+													log_debug!(self, "Got Err message from {} with non-ASCII error message", log_pubkey!(peer.their_node_id.unwrap()));
+												}
+												self.message_handler.chan_handler.handle_error(&peer.their_node_id.unwrap(), &msg);
+												if msg.channel_id == [0; 32] {
+													return Err(PeerHandleError{ no_connection_possible: true });
+												}
 											},
 
 											18 => {
@@ -621,6 +638,10 @@ impl<Descriptor: SocketDescriptor> PeerManager<Descriptor> {
 							};
 							match peers.peers.get_mut(&descriptor) {
 								Some(peer) => {
+									if peer.their_global_features.is_none() {
+										$handle_no_such_peer;
+										continue;
+									}
 									(descriptor, peer)
 								},
 								None => panic!("Inconsistent peers set state!"),
@@ -717,7 +738,7 @@ impl<Descriptor: SocketDescriptor> PeerManager<Descriptor> {
 							let encoded_update_msg = encode_msg!(update_msg, 258);
 
 							for (ref descriptor, ref mut peer) in peers.peers.iter_mut() {
-								if !peer.channel_encryptor.is_ready_for_encryption() {
+								if !peer.channel_encryptor.is_ready_for_encryption() || peer.their_global_features.is_none() {
 									continue
 								}
 								match peer.their_node_id {
@@ -741,7 +762,7 @@ impl<Descriptor: SocketDescriptor> PeerManager<Descriptor> {
 							let encoded_msg = encode_msg!(msg, 258);
 
 							for (ref descriptor, ref mut peer) in peers.peers.iter_mut() {
-								if !peer.channel_encryptor.is_ready_for_encryption() {
+								if !peer.channel_encryptor.is_ready_for_encryption() || peer.their_global_features.is_none() {
 									continue
 								}
 								peer.pending_outbound_buffer.push_back(peer.channel_encryptor.encrypt_message(&encoded_msg[..]));

src/util/test_utils.rs

@@ -115,6 +115,7 @@ impl msgs::ChannelMessageHandler for TestChannelMessageHandler {
 		Err(HandleError { err: "", action: None })
 	}
 	fn peer_disconnected(&self, _their_node_id: &PublicKey, _no_connection_possible: bool) {}
+	fn handle_error(&self, _their_node_id: &PublicKey, _msg: &msgs::ErrorMessage) {}
 }
 
 impl events::EventsProvider for TestChannelMessageHandler {