From f676f5585f46eefbababdc19a5a90d5e0c77f0ca Mon Sep 17 00:00:00 2001 From: Matt Corallo Date: Thu, 22 Jul 2021 15:25:13 +0000 Subject: [PATCH 1/6] Add a new `WarningMessage` message to send and receive warnings --- fuzz/src/msg_targets/gen_target.sh | 1 + fuzz/src/msg_targets/mod.rs | 1 + fuzz/src/msg_targets/msg_warning_message.rs | 27 +++++++++ lightning/src/ln/msgs.rs | 62 +++++++++++++++++++-- 4 files changed, 87 insertions(+), 4 deletions(-) create mode 100644 fuzz/src/msg_targets/msg_warning_message.rs diff --git a/fuzz/src/msg_targets/gen_target.sh b/fuzz/src/msg_targets/gen_target.sh index 2e133484138..0e930cfa1ac 100755 --- a/fuzz/src/msg_targets/gen_target.sh +++ b/fuzz/src/msg_targets/gen_target.sh @@ -43,4 +43,5 @@ GEN_TEST QueryShortChannelIds test_msg "" GEN_TEST ReplyChannelRange test_msg "" GEN_TEST ErrorMessage test_msg_hole ", 32, 2" +GEN_TEST WarningMessage test_msg_hole ", 32, 2" GEN_TEST ChannelUpdate test_msg_hole ", 108, 1" diff --git a/fuzz/src/msg_targets/mod.rs b/fuzz/src/msg_targets/mod.rs index af70c58e30e..8acb690b04b 100644 --- a/fuzz/src/msg_targets/mod.rs +++ b/fuzz/src/msg_targets/mod.rs @@ -28,4 +28,5 @@ pub mod msg_node_announcement; pub mod msg_query_short_channel_ids; pub mod msg_reply_channel_range; pub mod msg_error_message; +pub mod msg_warning_message; pub mod msg_channel_update; diff --git a/fuzz/src/msg_targets/msg_warning_message.rs b/fuzz/src/msg_targets/msg_warning_message.rs new file mode 100644 index 00000000000..f033b6fd6e9 --- /dev/null +++ b/fuzz/src/msg_targets/msg_warning_message.rs @@ -0,0 +1,27 @@ +// This file is Copyright its original authors, visible in version control +// history. +// +// This file is licensed under the Apache License, Version 2.0 or the MIT license +// , at your option. +// You may not use this file except in accordance with one or both of these +// licenses. + +// 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. + +use lightning::ln::msgs; + +use msg_targets::utils::VecWriter; +use utils::test_logger; + +#[inline] +pub fn msg_warning_message_test(data: &[u8], _out: Out) { + test_msg_hole!(msgs::WarningMessage, data, 32, 2); +} + +#[no_mangle] +pub extern "C" fn msg_warning_message_run(data: *const u8, datalen: usize) { + let data = unsafe { std::slice::from_raw_parts(data, datalen) }; + test_msg_hole!(msgs::WarningMessage, data, 32, 2); +} diff --git a/lightning/src/ln/msgs.rs b/lightning/src/ln/msgs.rs index 39c00105f8d..1335a04c47a 100644 --- a/lightning/src/ln/msgs.rs +++ b/lightning/src/ln/msgs.rs @@ -80,12 +80,29 @@ pub struct Init { /// An error message to be sent or received from a peer #[derive(Clone, Debug, PartialEq)] pub struct ErrorMessage { - /// The channel ID involved in the error + /// The channel ID involved in the error. + /// + /// All-0s indicates a general error unrelated to a specific channel, after which all channels + /// with the sending peer should be closed. pub channel_id: [u8; 32], /// A possibly human-readable error description. - /// The string should be sanitized before it is used (e.g. emitted to logs - /// or printed to stdout). Otherwise, a well crafted error message may trigger a security - /// vulnerability in the terminal emulator or the logging subsystem. + /// The string should be sanitized before it is used (e.g. emitted to logs or printed to + /// stdout). Otherwise, a well crafted error message may trigger a security vulnerability in + /// the terminal emulator or the logging subsystem. + pub data: String, +} + +/// A warning message to be sent or received from a peer +#[derive(Clone, Debug, PartialEq)] +pub struct WarningMessage { + /// The channel ID involved in the warning. + /// + /// All-0s indicates a warning unrelated to a specific channel. + pub channel_id: [u8; 32], + /// A possibly human-readable warning description. + /// The string should be sanitized before it is used (e.g. emitted to logs or printed to + /// stdout). Otherwise, a well crafted error message may trigger a security vulnerability in + /// the terminal emulator or the logging subsystem. pub data: String, } @@ -1515,6 +1532,32 @@ impl Readable for ErrorMessage { } } +impl Writeable for WarningMessage { + fn write(&self, w: &mut W) -> Result<(), io::Error> { + self.channel_id.write(w)?; + (self.data.len() as u16).write(w)?; + w.write_all(self.data.as_bytes())?; + Ok(()) + } +} + +impl Readable for WarningMessage { + fn read(r: &mut R) -> Result { + Ok(Self { + channel_id: Readable::read(r)?, + data: { + let mut sz: usize = ::read(r)? as usize; + let data = read_to_end(r)?; + sz = cmp::min(data.len(), sz); + match String::from_utf8(data[..sz as usize].to_vec()) { + Ok(s) => s, + Err(_) => return Err(DecodeError::InvalidValue), + } + } + }) + } +} + impl Writeable for UnsignedNodeAnnouncement { fn write(&self, w: &mut W) -> Result<(), io::Error> { self.features.write(w)?; @@ -2405,6 +2448,17 @@ mod tests { assert_eq!(encoded_value, target_value); } + #[test] + fn encoding_warning() { + let error = msgs::WarningMessage { + channel_id: [2; 32], + data: String::from("rust-lightning"), + }; + let encoded_value = error.encode(); + let target_value = hex::decode("0202020202020202020202020202020202020202020202020202020202020202000e727573742d6c696768746e696e67").unwrap(); + assert_eq!(encoded_value, target_value); + } + #[test] fn encoding_ping() { let ping = msgs::Ping { From 1b3249a1929929170bba9d2553d7a9c8670193e5 Mon Sep 17 00:00:00 2001 From: Matt Corallo Date: Thu, 22 Jul 2021 16:05:48 +0000 Subject: [PATCH 2/6] Handle sending and receiving warning messages --- lightning/src/ln/msgs.rs | 11 ++++++++++- lightning/src/ln/peer_handler.rs | 26 ++++++++++++++++++++++++++ lightning/src/ln/wire.rs | 9 +++++++++ 3 files changed, 45 insertions(+), 1 deletion(-) diff --git a/lightning/src/ln/msgs.rs b/lightning/src/ln/msgs.rs index 1335a04c47a..6c04636a435 100644 --- a/lightning/src/ln/msgs.rs +++ b/lightning/src/ln/msgs.rs @@ -731,7 +731,16 @@ pub enum ErrorAction { /// The peer did something incorrect. Tell them. SendErrorMessage { /// The message to send. - msg: ErrorMessage + msg: ErrorMessage, + }, + /// The peer did something incorrect. Tell them without closing any channels. + SendWarningMessage { + /// The message to send. + msg: WarningMessage, + /// The peer may have done something harmless that we weren't able to meaningfully process, + /// though we should still tell them about it. + /// If this event is logged, log it at the given level. + log_level: logger::Level, }, } diff --git a/lightning/src/ln/peer_handler.rs b/lightning/src/ln/peer_handler.rs index 9157dc87dc2..6607d986aff 100644 --- a/lightning/src/ln/peer_handler.rs +++ b/lightning/src/ln/peer_handler.rs @@ -821,6 +821,11 @@ impl P self.enqueue_message(peer, &msg); continue; }, + msgs::ErrorAction::SendWarningMessage { msg, log_level } => { + log_given_level!(self.logger, log_level, "Error handling message{}; sending warning message with: {}", OptionalFromDebugger(&peer.their_node_id), e.err); + self.enqueue_message(peer, &msg); + continue; + }, } } } @@ -1022,6 +1027,21 @@ impl P return Err(PeerHandleError{ no_connection_possible: true }.into()); } }, + wire::Message::Warning(msg) => { + 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.logger, "Got warning message from {}: {}", log_pubkey!(peer.their_node_id.unwrap()), msg.data); + } else { + log_debug!(self.logger, "Got warning message from {} with non-ASCII error message", log_pubkey!(peer.their_node_id.unwrap())); + } + }, wire::Message::Ping(msg) => { if msg.ponglen < 65532 { @@ -1419,6 +1439,12 @@ impl P msg.data); self.enqueue_message(get_peer_for_forwarding!(node_id), msg); }, + msgs::ErrorAction::SendWarningMessage { ref msg, ref log_level } => { + log_given_level!(self.logger, *log_level, "Handling SendWarningMessage HandleError event in peer_handler for node {} with message {}", + log_pubkey!(node_id), + msg.data); + self.enqueue_message(get_peer_for_forwarding!(node_id), msg); + }, } }, MessageSendEvent::SendChannelRangeQuery { ref node_id, ref msg } => { diff --git a/lightning/src/ln/wire.rs b/lightning/src/ln/wire.rs index d3cc257f5a6..2c77fd46b8d 100644 --- a/lightning/src/ln/wire.rs +++ b/lightning/src/ln/wire.rs @@ -35,6 +35,7 @@ pub trait CustomMessageReader { pub(crate) enum Message where T: core::fmt::Debug + Type { Init(msgs::Init), Error(msgs::ErrorMessage), + Warning(msgs::WarningMessage), Ping(msgs::Ping), Pong(msgs::Pong), OpenChannel(msgs::OpenChannel), @@ -74,6 +75,7 @@ impl Message where T: core::fmt::Debug + Type { match self { &Message::Init(ref msg) => msg.type_id(), &Message::Error(ref msg) => msg.type_id(), + &Message::Warning(ref msg) => msg.type_id(), &Message::Ping(ref msg) => msg.type_id(), &Message::Pong(ref msg) => msg.type_id(), &Message::OpenChannel(ref msg) => msg.type_id(), @@ -133,6 +135,9 @@ where msgs::ErrorMessage::TYPE => { Ok(Message::Error(Readable::read(buffer)?)) }, + msgs::WarningMessage::TYPE => { + Ok(Message::Warning(Readable::read(buffer)?)) + }, msgs::Ping::TYPE => { Ok(Message::Ping(Readable::read(buffer)?)) }, @@ -264,6 +269,10 @@ impl Encode for msgs::ErrorMessage { const TYPE: u16 = 17; } +impl Encode for msgs::WarningMessage { + const TYPE: u16 = 1; +} + impl Encode for msgs::Ping { const TYPE: u16 = 18; } From e137cfb3c48e11ee1ad1e1febdb6511b4dbbbcb4 Mon Sep 17 00:00:00 2001 From: Matt Corallo Date: Thu, 22 Jul 2021 16:06:33 +0000 Subject: [PATCH 3/6] Send warning messages when appropriate in gossip handling pipeline --- lightning/src/ln/channelmanager.rs | 14 +++++---- lightning/src/ln/functional_test_utils.rs | 16 +++++++++++ lightning/src/ln/peer_handler.rs | 3 +- lightning/src/ln/shutdown_tests.rs | 6 ++-- lightning/src/routing/network_graph.rs | 35 +++++++++++++++-------- 5 files changed, 52 insertions(+), 22 deletions(-) diff --git a/lightning/src/ln/channelmanager.rs b/lightning/src/ln/channelmanager.rs index 95f1e86b9c3..e9af5b18681 100644 --- a/lightning/src/ln/channelmanager.rs +++ b/lightning/src/ln/channelmanager.rs @@ -613,8 +613,14 @@ impl MsgHandleErrInternal { Self { err: match err { ChannelError::Warn(msg) => LightningError { - err: msg, - action: msgs::ErrorAction::IgnoreError, + err: msg.clone(), + action: msgs::ErrorAction::SendWarningMessage { + msg: msgs::WarningMessage { + channel_id, + data: msg + }, + log_level: Level::Warn, + }, }, ChannelError::Ignore(msg) => LightningError { err: msg, @@ -1373,9 +1379,7 @@ macro_rules! convert_chan_err { ($self: ident, $err: expr, $short_to_id: expr, $channel: expr, $channel_id: expr) => { match $err { ChannelError::Warn(msg) => { - //TODO: Once warning messages are merged, we should send a `warning` message to our - //peer here. - (false, MsgHandleErrInternal::from_chan_no_close(ChannelError::Ignore(msg), $channel_id.clone())) + (false, MsgHandleErrInternal::from_chan_no_close(ChannelError::Warn(msg), $channel_id.clone())) }, ChannelError::Ignore(msg) => { (false, MsgHandleErrInternal::from_chan_no_close(ChannelError::Ignore(msg), $channel_id.clone())) diff --git a/lightning/src/ln/functional_test_utils.rs b/lightning/src/ln/functional_test_utils.rs index 8ff793ed08b..6012b54644a 100644 --- a/lightning/src/ln/functional_test_utils.rs +++ b/lightning/src/ln/functional_test_utils.rs @@ -780,6 +780,22 @@ macro_rules! get_closing_signed_broadcast { } } +#[cfg(test)] +macro_rules! check_warn_msg { + ($node: expr, $recipient_node_id: expr, $chan_id: expr) => {{ + let msg_events = $node.node.get_and_clear_pending_msg_events(); + assert_eq!(msg_events.len(), 1); + match msg_events[0] { + MessageSendEvent::HandleError { action: ErrorAction::SendWarningMessage { ref msg, log_level: _ }, node_id } => { + assert_eq!(node_id, $recipient_node_id); + assert_eq!(msg.channel_id, $chan_id); + msg.data.clone() + }, + _ => panic!("Unexpected event"), + } + }} +} + /// Check that a channel's closing channel update has been broadcasted, and optionally /// check whether an error message event has occurred. #[macro_export] diff --git a/lightning/src/ln/peer_handler.rs b/lightning/src/ln/peer_handler.rs index 6607d986aff..f6369c62f70 100644 --- a/lightning/src/ln/peer_handler.rs +++ b/lightning/src/ln/peer_handler.rs @@ -918,7 +918,8 @@ impl P msgs::DecodeError::BadLengthDescriptor => return Err(PeerHandleError { no_connection_possible: false }), msgs::DecodeError::Io(_) => return Err(PeerHandleError { no_connection_possible: false }), msgs::DecodeError::UnsupportedCompression => { - log_gossip!(self.logger, "We don't support zlib-compressed message fields, ignoring message"); + log_gossip!(self.logger, "We don't support zlib-compressed message fields, sending a warning and ignoring message"); + self.enqueue_message(peer, &msgs::WarningMessage { channel_id: [0; 32], data: "Unsupported message compression: zlib".to_owned() }); continue; } } diff --git a/lightning/src/ln/shutdown_tests.rs b/lightning/src/ln/shutdown_tests.rs index 791e87377f0..01467259875 100644 --- a/lightning/src/ln/shutdown_tests.rs +++ b/lightning/src/ln/shutdown_tests.rs @@ -762,10 +762,8 @@ fn do_test_closing_signed_reinit_timeout(timeout_step: TimeoutStep) { if timeout_step != TimeoutStep::AfterShutdown { nodes[1].node.handle_closing_signed(&nodes[0].node.get_our_node_id(), &node_0_closing_signed); - // At this point nodes[1] should send back a warning message indicating it disagrees with the - // given channel-closing fee. Currently we do not implement warning messages so instead we - // remain silent here. - assert!(nodes[1].node.get_and_clear_pending_msg_events().is_empty()); + assert!(check_warn_msg!(nodes[1], nodes[0].node.get_our_node_id(), chan_id) + .starts_with("Unable to come to consensus about closing feerate")); // Now deliver a mutated closing_signed indicating a higher acceptable fee range, which // nodes[1] should happily accept and respond to. diff --git a/lightning/src/routing/network_graph.rs b/lightning/src/routing/network_graph.rs index ae597d30364..6f431d940af 100644 --- a/lightning/src/routing/network_graph.rs +++ b/lightning/src/routing/network_graph.rs @@ -294,10 +294,21 @@ where C::Target: chain::Access, L::Target: Logger } macro_rules! secp_verify_sig { - ( $secp_ctx: expr, $msg: expr, $sig: expr, $pubkey: expr ) => { + ( $secp_ctx: expr, $msg: expr, $sig: expr, $pubkey: expr, $msg_type: expr ) => { match $secp_ctx.verify($msg, $sig, $pubkey) { Ok(_) => {}, - Err(_) => return Err(LightningError{err: "Invalid signature from remote node".to_owned(), action: ErrorAction::IgnoreError}), + Err(_) => { + return Err(LightningError { + err: format!("Invalid signature on {} message", $msg_type), + action: ErrorAction::SendWarningMessage { + msg: msgs::WarningMessage { + channel_id: [0; 32], + data: format!("Invalid signature on {} message", $msg_type), + }, + log_level: Level::Trace, + }, + }); + }, } }; } @@ -850,7 +861,7 @@ impl NetworkGraph { /// routing messages from a source using a protocol other than the lightning P2P protocol. pub fn update_node_from_announcement(&self, msg: &msgs::NodeAnnouncement, secp_ctx: &Secp256k1) -> Result<(), LightningError> { let msg_hash = hash_to_message!(&Sha256dHash::hash(&msg.contents.encode()[..])[..]); - secp_verify_sig!(secp_ctx, &msg_hash, &msg.signature, &msg.contents.node_id); + secp_verify_sig!(secp_ctx, &msg_hash, &msg.signature, &msg.contents.node_id, "node_announcement"); self.update_node_from_announcement_intern(&msg.contents, Some(&msg)) } @@ -910,10 +921,10 @@ impl NetworkGraph { C::Target: chain::Access, { let msg_hash = hash_to_message!(&Sha256dHash::hash(&msg.contents.encode()[..])[..]); - secp_verify_sig!(secp_ctx, &msg_hash, &msg.node_signature_1, &msg.contents.node_id_1); - secp_verify_sig!(secp_ctx, &msg_hash, &msg.node_signature_2, &msg.contents.node_id_2); - secp_verify_sig!(secp_ctx, &msg_hash, &msg.bitcoin_signature_1, &msg.contents.bitcoin_key_1); - secp_verify_sig!(secp_ctx, &msg_hash, &msg.bitcoin_signature_2, &msg.contents.bitcoin_key_2); + secp_verify_sig!(secp_ctx, &msg_hash, &msg.node_signature_1, &msg.contents.node_id_1, "channel_announcement"); + secp_verify_sig!(secp_ctx, &msg_hash, &msg.node_signature_2, &msg.contents.node_id_2, "channel_announcement"); + secp_verify_sig!(secp_ctx, &msg_hash, &msg.bitcoin_signature_1, &msg.contents.bitcoin_key_1, "channel_announcement"); + secp_verify_sig!(secp_ctx, &msg_hash, &msg.bitcoin_signature_2, &msg.contents.bitcoin_key_2, "channel_announcement"); self.update_channel_from_unsigned_announcement_intern(&msg.contents, Some(msg), chain_access) } @@ -1239,7 +1250,7 @@ impl NetworkGraph { secp_verify_sig!(ctx, &msg_hash, &sig, &PublicKey::from_slice(channel.node_two.as_slice()).map_err(|_| LightningError{ err: "Couldn't parse source node pubkey".to_owned(), action: ErrorAction::IgnoreAndLog(Level::Debug) - })?); + })?, "channel_update"); } maybe_update_channel_info!(channel.two_to_one, channel.node_two); } else { @@ -1248,7 +1259,7 @@ impl NetworkGraph { secp_verify_sig!(ctx, &msg_hash, &sig, &PublicKey::from_slice(channel.node_one.as_slice()).map_err(|_| LightningError{ err: "Couldn't parse destination node pubkey".to_owned(), action: ErrorAction::IgnoreAndLog(Level::Debug) - })?); + })?, "channel_update"); } maybe_update_channel_info!(channel.one_to_two, channel.node_one); } @@ -1522,7 +1533,7 @@ mod tests { contents: valid_announcement.contents.clone() }) { Ok(_) => panic!(), - Err(e) => assert_eq!(e.err, "Invalid signature from remote node") + Err(e) => assert_eq!(e.err, "Invalid signature on node_announcement message") }; let announcement_with_data = get_signed_node_announcement(|unsigned_announcement| { @@ -1651,7 +1662,7 @@ mod tests { invalid_sig_announcement.contents.excess_data = Vec::new(); match net_graph_msg_handler.handle_channel_announcement(&invalid_sig_announcement) { Ok(_) => panic!(), - Err(e) => assert_eq!(e.err, "Invalid signature from remote node") + Err(e) => assert_eq!(e.err, "Invalid signature on channel_announcement message") }; let channel_to_itself_announcement = get_signed_channel_announcement(|_| {}, node_1_privkey, node_1_privkey, &secp_ctx); @@ -1760,7 +1771,7 @@ mod tests { invalid_sig_channel_update.signature = secp_ctx.sign(&fake_msghash, node_1_privkey); match net_graph_msg_handler.handle_channel_update(&invalid_sig_channel_update) { Ok(_) => panic!(), - Err(e) => assert_eq!(e.err, "Invalid signature from remote node") + Err(e) => assert_eq!(e.err, "Invalid signature on channel_update message") }; } From 26fe0f753dc5116ab6fe5d078c9f30a318559502 Mon Sep 17 00:00:00 2001 From: Matt Corallo Date: Thu, 30 Sep 2021 22:45:07 +0000 Subject: [PATCH 4/6] Convert `shutdown` invalid script checks to warning messages As required by the warning messages PR, we should simply warn our counterparty in this case and let them try again, continuing to try to use the channel until they tell us otherwise. --- lightning/src/ln/channel.rs | 4 +-- lightning/src/ln/shutdown_tests.rs | 40 ++++++++++-------------------- 2 files changed, 15 insertions(+), 29 deletions(-) diff --git a/lightning/src/ln/channel.rs b/lightning/src/ln/channel.rs index ca6c6781fde..d99e64b7e62 100644 --- a/lightning/src/ln/channel.rs +++ b/lightning/src/ln/channel.rs @@ -3723,12 +3723,12 @@ impl Channel { assert_eq!(self.channel_state & ChannelState::ShutdownComplete as u32, 0); if !script::is_bolt2_compliant(&msg.scriptpubkey, their_features) { - return Err(ChannelError::Close(format!("Got a nonstandard scriptpubkey ({}) from remote peer", msg.scriptpubkey.to_bytes().to_hex()))); + return Err(ChannelError::Warn(format!("Got a nonstandard scriptpubkey ({}) from remote peer", msg.scriptpubkey.to_bytes().to_hex()))); } if self.counterparty_shutdown_scriptpubkey.is_some() { if Some(&msg.scriptpubkey) != self.counterparty_shutdown_scriptpubkey.as_ref() { - return Err(ChannelError::Close(format!("Got shutdown request with a scriptpubkey ({}) which did not match their previous scriptpubkey.", msg.scriptpubkey.to_bytes().to_hex()))); + return Err(ChannelError::Warn(format!("Got shutdown request with a scriptpubkey ({}) which did not match their previous scriptpubkey.", msg.scriptpubkey.to_bytes().to_hex()))); } } else { self.counterparty_shutdown_scriptpubkey = Some(msg.scriptpubkey.clone()); diff --git a/lightning/src/ln/shutdown_tests.rs b/lightning/src/ln/shutdown_tests.rs index 01467259875..b712212ab75 100644 --- a/lightning/src/ln/shutdown_tests.rs +++ b/lightning/src/ln/shutdown_tests.rs @@ -415,13 +415,17 @@ fn test_upfront_shutdown_script() { let flags = InitFeatures::known(); let chan = create_announced_chan_between_nodes_with_value(&nodes, 0, 2, 1000000, 1000000, flags.clone(), flags.clone()); nodes[0].node.close_channel(&OutPoint { txid: chan.3.txid(), index: 0 }.to_channel_id()).unwrap(); - let mut node_0_shutdown = get_event_msg!(nodes[0], MessageSendEvent::SendShutdown, nodes[2].node.get_our_node_id()); + let node_0_orig_shutdown = get_event_msg!(nodes[0], MessageSendEvent::SendShutdown, nodes[2].node.get_our_node_id()); + let mut node_0_shutdown = node_0_orig_shutdown.clone(); node_0_shutdown.scriptpubkey = Builder::new().push_opcode(opcodes::all::OP_RETURN).into_script().to_p2sh(); - // Test we enforce upfront_scriptpbukey if by providing a diffrent one at closing that we disconnect peer + // Test we enforce upfront_scriptpbukey if by providing a different one at closing that we warn + // the peer and ignore the message. nodes[2].node.handle_shutdown(&nodes[0].node.get_our_node_id(), &InitFeatures::known(), &node_0_shutdown); - assert!(regex::Regex::new(r"Got shutdown request with a scriptpubkey \([A-Fa-f0-9]+\) which did not match their previous scriptpubkey.").unwrap().is_match(check_closed_broadcast!(nodes[2], true).unwrap().data.as_str())); - check_closed_event!(nodes[2], 1, ClosureReason::ProcessingError { err: "Got shutdown request with a scriptpubkey (a91441c98a140039816273e50db317422c11c2bfcc8887) which did not match their previous scriptpubkey.".to_string() }); - check_added_monitors!(nodes[2], 1); + assert!(regex::Regex::new(r"Got shutdown request with a scriptpubkey \([A-Fa-f0-9]+\) which did not match their previous scriptpubkey.") + .unwrap().is_match(&check_warn_msg!(nodes[2], nodes[0].node.get_our_node_id(), chan.2))); + // This allows nodes[2] to retry the shutdown message, which should get a response: + nodes[2].node.handle_shutdown(&nodes[0].node.get_our_node_id(), &InitFeatures::known(), &node_0_orig_shutdown); + get_event_msg!(nodes[2], MessageSendEvent::SendShutdown, nodes[0].node.get_our_node_id()); // We test that in case of peer committing upfront to a script, if it doesn't change at closing, we sign let chan = create_announced_chan_between_nodes_with_value(&nodes, 0, 2, 1000000, 1000000, flags.clone(), flags.clone()); @@ -668,17 +672,8 @@ fn test_unsupported_anysegwit_shutdown_script() { node_0_shutdown.scriptpubkey = unsupported_shutdown_script.into_inner(); nodes[0].node.handle_shutdown(&nodes[1].node.get_our_node_id(), &node_cfgs[1].features, &node_0_shutdown); - let events = nodes[0].node.get_and_clear_pending_msg_events(); - assert_eq!(events.len(), 2); - match events[1] { - MessageSendEvent::HandleError { action: ErrorAction::SendErrorMessage { ref msg }, node_id } => { - assert_eq!(node_id, nodes[1].node.get_our_node_id()); - assert_eq!(msg.data, "Got a nonstandard scriptpubkey (60020028) from remote peer".to_owned()); - }, - _ => panic!("Unexpected event"), - } - check_added_monitors!(nodes[0], 1); - check_closed_event!(nodes[0], 1, ClosureReason::ProcessingError { err: "Got a nonstandard scriptpubkey (60020028) from remote peer".to_string() }); + assert_eq!(&check_warn_msg!(nodes[0], nodes[1].node.get_our_node_id(), chan.2), + "Got a nonstandard scriptpubkey (60020028) from remote peer"); } #[test] @@ -704,17 +699,8 @@ fn test_invalid_shutdown_script() { .into_script(); nodes[0].node.handle_shutdown(&nodes[1].node.get_our_node_id(), &InitFeatures::known(), &node_0_shutdown); - let events = nodes[0].node.get_and_clear_pending_msg_events(); - assert_eq!(events.len(), 2); - match events[1] { - MessageSendEvent::HandleError { action: ErrorAction::SendErrorMessage { ref msg }, node_id } => { - assert_eq!(node_id, nodes[1].node.get_our_node_id()); - assert_eq!(msg.data, "Got a nonstandard scriptpubkey (00020000) from remote peer".to_owned()) - }, - _ => panic!("Unexpected event"), - } - check_added_monitors!(nodes[0], 1); - check_closed_event!(nodes[0], 1, ClosureReason::ProcessingError { err: "Got a nonstandard scriptpubkey (00020000) from remote peer".to_string() }); + assert_eq!(&check_warn_msg!(nodes[0], nodes[1].node.get_our_node_id(), chan.2), + "Got a nonstandard scriptpubkey (00020000) from remote peer"); } #[derive(PartialEq)] From 2d7b06e6194df42f171bde203500fe10e8b28370 Mon Sep 17 00:00:00 2001 From: Matt Corallo Date: Fri, 7 Jan 2022 20:30:50 +0000 Subject: [PATCH 5/6] Send `warning` instead of `error` when we incounter bogus gossip --- lightning/src/ln/peer_handler.rs | 27 ++++++++++++++++----------- lightning/src/ln/wire.rs | 17 +++++++++++------ 2 files changed, 27 insertions(+), 17 deletions(-) diff --git a/lightning/src/ln/peer_handler.rs b/lightning/src/ln/peer_handler.rs index f6369c62f70..86cd344ce35 100644 --- a/lightning/src/ln/peer_handler.rs +++ b/lightning/src/ln/peer_handler.rs @@ -902,26 +902,31 @@ impl P Ok(x) => x, Err(e) => { match e { - msgs::DecodeError::UnknownVersion => return Err(PeerHandleError { no_connection_possible: false }), - msgs::DecodeError::UnknownRequiredFeature => { + (msgs::DecodeError::UnknownRequiredFeature, _) => { log_gossip!(self.logger, "Got a channel/node announcement with an unknown required feature flag, you may want to update!"); continue; } - msgs::DecodeError::InvalidValue => { + (msgs::DecodeError::UnsupportedCompression, _) => { + log_gossip!(self.logger, "We don't support zlib-compressed message fields, sending a warning and ignoring message"); + self.enqueue_message(peer, &msgs::WarningMessage { channel_id: [0; 32], data: "Unsupported message compression: zlib".to_owned() }); + continue; + } + (_, Some(ty)) if is_gossip_msg(ty) => { + log_gossip!(self.logger, "Got an invalid value while deserializing a gossip message"); + self.enqueue_message(peer, &msgs::WarningMessage { channel_id: [0; 32], data: "Unreadable/bogus gossip message".to_owned() }); + continue; + } + (msgs::DecodeError::UnknownVersion, _) => return Err(PeerHandleError { no_connection_possible: false }), + (msgs::DecodeError::InvalidValue, _) => { log_debug!(self.logger, "Got an invalid value while deserializing message"); return Err(PeerHandleError { no_connection_possible: false }); } - msgs::DecodeError::ShortRead => { + (msgs::DecodeError::ShortRead, _) => { log_debug!(self.logger, "Deserialization failed due to shortness of message"); return Err(PeerHandleError { no_connection_possible: false }); } - msgs::DecodeError::BadLengthDescriptor => return Err(PeerHandleError { no_connection_possible: false }), - msgs::DecodeError::Io(_) => return Err(PeerHandleError { no_connection_possible: false }), - msgs::DecodeError::UnsupportedCompression => { - log_gossip!(self.logger, "We don't support zlib-compressed message fields, sending a warning and ignoring message"); - self.enqueue_message(peer, &msgs::WarningMessage { channel_id: [0; 32], data: "Unsupported message compression: zlib".to_owned() }); - continue; - } + (msgs::DecodeError::BadLengthDescriptor, _) => return Err(PeerHandleError { no_connection_possible: false }), + (msgs::DecodeError::Io(_), _) => return Err(PeerHandleError { no_connection_possible: false }), } } }; diff --git a/lightning/src/ln/wire.rs b/lightning/src/ln/wire.rs index 2c77fd46b8d..d113acbfc7d 100644 --- a/lightning/src/ln/wire.rs +++ b/lightning/src/ln/wire.rs @@ -119,15 +119,20 @@ impl Message where T: core::fmt::Debug + Type { /// # Errors /// /// Returns an error if the message payload code not be decoded as the specified type. -pub(crate) fn read( - buffer: &mut R, - custom_reader: H, -) -> Result, msgs::DecodeError> -where +pub(crate) fn read(buffer: &mut R, custom_reader: H) +-> Result, (msgs::DecodeError, Option)> where + T: core::fmt::Debug + Type + Writeable, + H::Target: CustomMessageReader, +{ + let message_type = ::read(buffer).map_err(|e| (e, None))?; + do_read(buffer, message_type, custom_reader).map_err(|e| (e, Some(message_type))) +} + +fn do_read(buffer: &mut R, message_type: u16, custom_reader: H) +-> Result, msgs::DecodeError> where T: core::fmt::Debug + Type + Writeable, H::Target: CustomMessageReader, { - let message_type = ::read(buffer)?; match message_type { msgs::Init::TYPE => { Ok(Message::Init(Readable::read(buffer)?)) From d786bfaef27b9e49f71ac5b8bf5a4e02cd484f79 Mon Sep 17 00:00:00 2001 From: Matt Corallo Date: Fri, 7 Jan 2022 20:11:31 +0000 Subject: [PATCH 6/6] Rely on Error/Warning message data lengths being correct In https://github.com/lightning/bolts/pull/950, the (somewhat strange) requirement that error messages be handled even if the length field is set larger than the size of the package was removed. Here we change the code to drop the special handling for this, opting to just fail to read the message if the length is incorrect. --- lightning/src/ln/msgs.rs | 20 +++++++++++--------- 1 file changed, 11 insertions(+), 9 deletions(-) diff --git a/lightning/src/ln/msgs.rs b/lightning/src/ln/msgs.rs index 6c04636a435..9007f3f2ea7 100644 --- a/lightning/src/ln/msgs.rs +++ b/lightning/src/ln/msgs.rs @@ -33,7 +33,7 @@ use bitcoin::hash_types::{Txid, BlockHash}; use ln::features::{ChannelFeatures, ChannelTypeFeatures, InitFeatures, NodeFeatures}; use prelude::*; -use core::{cmp, fmt}; +use core::fmt; use core::fmt::Debug; use io::{self, Read}; use io_extras::read_to_end; @@ -1529,10 +1529,11 @@ impl Readable for ErrorMessage { Ok(Self { channel_id: Readable::read(r)?, data: { - let mut sz: usize = ::read(r)? as usize; - let data = read_to_end(r)?; - sz = cmp::min(data.len(), sz); - match String::from_utf8(data[..sz as usize].to_vec()) { + let sz: usize = ::read(r)? as usize; + let mut data = Vec::with_capacity(sz); + data.resize(sz, 0); + r.read_exact(&mut data)?; + match String::from_utf8(data) { Ok(s) => s, Err(_) => return Err(DecodeError::InvalidValue), } @@ -1555,10 +1556,11 @@ impl Readable for WarningMessage { Ok(Self { channel_id: Readable::read(r)?, data: { - let mut sz: usize = ::read(r)? as usize; - let data = read_to_end(r)?; - sz = cmp::min(data.len(), sz); - match String::from_utf8(data[..sz as usize].to_vec()) { + let sz: usize = ::read(r)? as usize; + let mut data = Vec::with_capacity(sz); + data.resize(sz, 0); + r.read_exact(&mut data)?; + match String::from_utf8(data) { Ok(s) => s, Err(_) => return Err(DecodeError::InvalidValue), }