Merge pull request #131 from TheBlueMatt/2018-08-bolt-1-compliance
authorMatt Corallo <649246+TheBlueMatt@users.noreply.github.com>
Sun, 26 Aug 2018 19:37:05 +0000 (15:37 -0400)
committerGitHub <noreply@github.com>
Sun, 26 Aug 2018 19:37:05 +0000 (15:37 -0400)
update Error/Init handling to be BOLT 1 compliant

fuzz/Cargo.toml
fuzz/fuzz_targets/msg_error_message_target.rs [new file with mode: 0644]
fuzz/fuzz_targets/msg_targets/gen_target.sh
fuzz/fuzz_targets/msg_targets/msg_error_message_target.rs [deleted file]
src/ln/channelmanager.rs
src/ln/msgs.rs
src/ln/peer_handler.rs
src/util/test_utils.rs

index 4394fd7964d646debbec2b35ec2ff95d65b5ec88..85ba3d11f3b08c6103e45017b8e75ca23e87949e 100644 (file)
@@ -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 (file)
index 0000000..fa021e2
--- /dev/null
@@ -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());
+       }
+}
index 37eac67a720e8304c0db54859ccd49651807a292..4a5dc2fc5565d9c25739c97b01faf512c5f74e72 100755 (executable)
@@ -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 (file)
index a69ded2..0000000
+++ /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());
-       }
-}
index a360c376a16282f450c2b7b8dc62f10547e89f98..3a98cd5a17647fb5a4a036abf5814411018bed22 100644 (file)
@@ -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)]
index c3b997566c81ce1b165e166458f641712cd867bc..44d71bd332938bc3959f3cf768bfebe5bffcbdcb 100644 (file)
@@ -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),
                };
index 4ab039741565521c3813c0d93980f8b834e29d15..f941f47dc10e5c99f13460b5aa82b96fba3a13c4 100644 (file)
@@ -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[..]));
index aff84735ece7e6a15355a1ef7710b80b97d8de9c..dc158230fad812acb467fcc05f08487f34d2c936 100644 (file)
@@ -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 {