From: Matt Corallo <649246+TheBlueMatt@users.noreply.github.com> Date: Sun, 26 Aug 2018 19:37:05 +0000 (-0400) Subject: Merge pull request #131 from TheBlueMatt/2018-08-bolt-1-compliance X-Git-Tag: v0.0.12~338 X-Git-Url: http://git.bitcoin.ninja/index.cgi?a=commitdiff_plain;h=b83443f0cc0436923450c53082bafc22c18fa669;hp=a33b3a269517f9721db2225a29d27eb03bb16431;p=rust-lightning Merge pull request #131 from TheBlueMatt/2018-08-bolt-1-compliance update Error/Init handling to be BOLT 1 compliant --- diff --git a/fuzz/Cargo.toml b/fuzz/Cargo.toml index 4394fd79..85ba3d11 100644 --- a/fuzz/Cargo.toml +++ b/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" diff --git a/fuzz/fuzz_targets/msg_error_message_target.rs b/fuzz/fuzz_targets/msg_error_message_target.rs new file mode 100644 index 00000000..fa021e23 --- /dev/null +++ b/fuzz/fuzz_targets/msg_error_message_target.rs @@ -0,0 +1,48 @@ +// This file is auto-generated by gen_target.sh based on msg_target_template.txt +// To modify it, modify msg_target_template.txt and run gen_target.sh instead. + +extern crate lightning; + +use lightning::ln::msgs; +use lightning::util::reset_rng_state; + +use lightning::ln::msgs::{MsgEncodable, MsgDecodable}; + +#[inline] +pub fn do_test(data: &[u8]) { + reset_rng_state(); + 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")] +#[macro_use] extern crate afl; +#[cfg(feature = "afl")] +fn main() { + fuzz!(|data| { + do_test(data); + }); +} + +#[cfg(feature = "honggfuzz")] +#[macro_use] extern crate honggfuzz; +#[cfg(feature = "honggfuzz")] +fn main() { + loop { + fuzz!(|data| { + do_test(data); + }); + } +} + +extern crate hex; +#[cfg(test)] +mod tests { + #[test] + fn duplicate_crash() { + super::do_test(&::hex::decode("00").unwrap()); + } +} diff --git a/fuzz/fuzz_targets/msg_targets/gen_target.sh b/fuzz/fuzz_targets/msg_targets/gen_target.sh index 37eac67a..4a5dc2fc 100755 --- a/fuzz/fuzz_targets/msg_targets/gen_target.sh +++ b/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 diff --git a/fuzz/fuzz_targets/msg_targets/msg_error_message_target.rs b/fuzz/fuzz_targets/msg_targets/msg_error_message_target.rs deleted file mode 100644 index a69ded2e..00000000 --- a/fuzz/fuzz_targets/msg_targets/msg_error_message_target.rs +++ /dev/null @@ -1,46 +0,0 @@ -// This file is auto-generated by gen_target.sh based on msg_target_template.txt -// To modify it, modify msg_target_template.txt and run gen_target.sh instead. - -extern crate lightning; - -use lightning::ln::msgs; -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); -} - -#[cfg(feature = "afl")] -#[macro_use] extern crate afl; -#[cfg(feature = "afl")] -fn main() { - fuzz!(|data| { - do_test(data); - }); -} - -#[cfg(feature = "honggfuzz")] -#[macro_use] extern crate honggfuzz; -#[cfg(feature = "honggfuzz")] -fn main() { - loop { - fuzz!(|data| { - do_test(data); - }); - } -} - -extern crate hex; -#[cfg(test)] -mod tests { - #[test] - fn duplicate_crash() { - super::do_test(&::hex::decode("00").unwrap()); - } -} diff --git a/src/ln/channelmanager.rs b/src/ln/channelmanager.rs index a360c376..3a98cd5a 100644 --- a/src/ln/channelmanager.rs +++ b/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)] diff --git a/src/ln/msgs.rs b/src/ln/msgs.rs index c3b99756..44d71bd3 100644 --- a/src/ln/msgs.rs +++ b/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), }; diff --git a/src/ln/peer_handler.rs b/src/ln/peer_handler.rs index 4ab03974..f941f47d 100644 --- a/src/ln/peer_handler.rs +++ b/src/ln/peer_handler.rs @@ -431,7 +431,24 @@ impl PeerManager { } }, 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 PeerManager { }; 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 PeerManager { 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 PeerManager { 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[..])); diff --git a/src/util/test_utils.rs b/src/util/test_utils.rs index aff84735..dc158230 100644 --- a/src/util/test_utils.rs +++ b/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 {