From: Matt Corallo <649246+TheBlueMatt@users.noreply.github.com> Date: Tue, 24 Nov 2020 16:36:14 +0000 (-0800) Subject: Merge pull request #748 from TheBlueMatt/2020-11-router-fuzzer X-Git-Tag: v0.0.12~2 X-Git-Url: http://git.bitcoin.ninja/index.cgi?a=commitdiff_plain;h=fc7df54f8dc3baf710371e2ad2beb862946d5b1c;hp=3c4a0c1fb30452d77177edc09b2f321e6502ac08;p=rust-lightning Merge pull request #748 from TheBlueMatt/2020-11-router-fuzzer Make router_target a bit easier for fuzzers to explore and fix two found bugs --- diff --git a/fuzz/src/router.rs b/fuzz/src/router.rs index 5f2114b6..ef7669f1 100644 --- a/fuzz/src/router.rs +++ b/fuzz/src/router.rs @@ -11,20 +11,22 @@ use bitcoin::blockdata::script::Builder; use bitcoin::blockdata::transaction::TxOut; use bitcoin::hash_types::BlockHash; +use bitcoin::secp256k1; + use lightning::chain; use lightning::ln::channelmanager::ChannelDetails; use lightning::ln::features::InitFeatures; use lightning::ln::msgs; -use lightning::ln::msgs::RoutingMessageHandler; use lightning::routing::router::{get_route, RouteHint}; use lightning::util::logger::Logger; use lightning::util::ser::Readable; -use lightning::routing::network_graph::{NetGraphMsgHandler, RoutingFees}; +use lightning::routing::network_graph::{NetworkGraph, RoutingFees}; use bitcoin::secp256k1::key::PublicKey; use utils::test_logger; +use std::collections::HashSet; use std::sync::Arc; use std::sync::atomic::{AtomicUsize, Ordering}; @@ -150,16 +152,12 @@ pub fn do_test(data: &[u8], out: Out) { } let logger: Arc = Arc::new(test_logger::TestLogger::new("".to_owned(), out)); - let chain_source = if get_slice!(1)[0] % 2 == 0 { - None - } else { - Some(Arc::new(FuzzChainSource { - input: Arc::clone(&input), - })) - }; let our_pubkey = get_pubkey!(); - let net_graph_msg_handler = NetGraphMsgHandler::new(chain_source, Arc::clone(&logger)); + let mut net_graph = NetworkGraph::new(); + + let mut node_pks = HashSet::new(); + let mut scid = 42; loop { match get_slice!(1)[0] { @@ -169,39 +167,44 @@ pub fn do_test(data: &[u8], out: Out) { if addr_len > (37+1)*4 { return; } - let _ = net_graph_msg_handler.handle_node_announcement(&decode_msg_with_len16!(msgs::NodeAnnouncement, 64, 288)); + let msg = decode_msg_with_len16!(msgs::NodeAnnouncement, 64, 288); + node_pks.insert(msg.contents.node_id); + let _ = net_graph.update_node_from_announcement::(&msg, None); }, 1 => { - let _ = net_graph_msg_handler.handle_channel_announcement(&decode_msg_with_len16!(msgs::ChannelAnnouncement, 64*4, 32+8+33*4)); + let msg = decode_msg_with_len16!(msgs::ChannelAnnouncement, 64*4, 32+8+33*4); + node_pks.insert(msg.contents.node_id_1); + node_pks.insert(msg.contents.node_id_2); + let _ = net_graph.update_channel_from_announcement::(&msg, None, None); }, 2 => { - let _ = net_graph_msg_handler.handle_channel_update(&decode_msg!(msgs::ChannelUpdate, 136)); + let msg = decode_msg_with_len16!(msgs::ChannelAnnouncement, 64*4, 32+8+33*4); + node_pks.insert(msg.contents.node_id_1); + node_pks.insert(msg.contents.node_id_2); + let val = slice_to_be64(get_slice!(8)); + let _ = net_graph.update_channel_from_announcement::(&msg, Some(val), None); }, 3 => { - match get_slice!(1)[0] { - 0 => { - net_graph_msg_handler.handle_htlc_fail_channel_update(&msgs::HTLCFailChannelUpdate::ChannelUpdateMessage {msg: decode_msg!(msgs::ChannelUpdate, 136)}); - }, - 1 => { - let short_channel_id = slice_to_be64(get_slice!(8)); - net_graph_msg_handler.handle_htlc_fail_channel_update(&msgs::HTLCFailChannelUpdate::ChannelClosed {short_channel_id, is_permanent: false}); - }, - _ => return, - } + let _ = net_graph.update_channel(&decode_msg!(msgs::ChannelUpdate, 136), None); }, 4 => { - let target = get_pubkey!(); + let short_channel_id = slice_to_be64(get_slice!(8)); + net_graph.close_channel_from_update(short_channel_id, false); + }, + _ if node_pks.is_empty() => {}, + _ => { let mut first_hops_vec = Vec::new(); let first_hops = match get_slice!(1)[0] { 0 => None, - 1 => { - let count = slice_to_be16(get_slice!(2)); + count => { for _ in 0..count { + scid += 1; + let rnid = node_pks.iter().skip(slice_to_be16(get_slice!(2))as usize % node_pks.len()).next().unwrap(); first_hops_vec.push(ChannelDetails { channel_id: [0; 32], - short_channel_id: Some(slice_to_be64(get_slice!(8))), - remote_network_id: get_pubkey!(), - counterparty_features: InitFeatures::empty(), + short_channel_id: Some(scid), + remote_network_id: *rnid, + counterparty_features: InitFeatures::known(), channel_value_satoshis: slice_to_be64(get_slice!(8)), user_id: 0, inbound_capacity_msat: 0, @@ -211,15 +214,16 @@ pub fn do_test(data: &[u8], out: Out) { } Some(&first_hops_vec[..]) }, - _ => return, }; let mut last_hops_vec = Vec::new(); - let last_hops = { - let count = slice_to_be16(get_slice!(2)); + { + let count = get_slice!(1)[0]; for _ in 0..count { + scid += 1; + let rnid = node_pks.iter().skip(slice_to_be16(get_slice!(2))as usize % node_pks.len()).next().unwrap(); last_hops_vec.push(RouteHint { - src_node_id: get_pubkey!(), - short_channel_id: slice_to_be64(get_slice!(8)), + src_node_id: *rnid, + short_channel_id: scid, fees: RoutingFees { base_msat: slice_to_be32(get_slice!(4)), proportional_millionths: slice_to_be32(get_slice!(4)), @@ -228,14 +232,15 @@ pub fn do_test(data: &[u8], out: Out) { htlc_minimum_msat: slice_to_be64(get_slice!(8)), }); } - &last_hops_vec[..] - }; - let _ = get_route(&our_pubkey, &net_graph_msg_handler.network_graph.read().unwrap(), &target, - first_hops.map(|c| c.iter().collect::>()).as_ref().map(|a| a.as_slice()), - &last_hops.iter().collect::>(), - slice_to_be64(get_slice!(8)), slice_to_be32(get_slice!(4)), Arc::clone(&logger)); + } + let last_hops = &last_hops_vec[..]; + for target in node_pks.iter() { + let _ = get_route(&our_pubkey, &net_graph, target, + first_hops.map(|c| c.iter().collect::>()).as_ref().map(|a| a.as_slice()), + &last_hops.iter().collect::>(), + slice_to_be64(get_slice!(8)), slice_to_be32(get_slice!(4)), Arc::clone(&logger)); + } }, - _ => return, } } } diff --git a/lightning/src/routing/network_graph.rs b/lightning/src/routing/network_graph.rs index 308c0526..932f3779 100644 --- a/lightning/src/routing/network_graph.rs +++ b/lightning/src/routing/network_graph.rs @@ -562,9 +562,14 @@ impl NetworkGraph { } } - /// For an already known node (from channel announcements), update its stored properties from a given node announcement + /// For an already known node (from channel announcements), update its stored properties from a given node announcement. + /// + /// You probably don't want to call this directly, instead relying on a NetGraphMsgHandler's + /// RoutingMessageHandler implementation to call it indirectly. This may be useful to accept + /// routing messages without checking their signatures. + /// /// Announcement signatures are checked here only if Secp256k1 object is provided. - fn update_node_from_announcement(&mut self, msg: &msgs::NodeAnnouncement, secp_ctx: Option<&Secp256k1>) -> Result { + pub fn update_node_from_announcement(&mut self, msg: &msgs::NodeAnnouncement, secp_ctx: Option<&Secp256k1>) -> Result { if let Some(sig_verifier) = secp_ctx { let msg_hash = hash_to_message!(&Sha256dHash::hash(&msg.contents.encode()[..])[..]); secp_verify_sig!(sig_verifier, &msg_hash, &msg.signature, &msg.contents.node_id); @@ -594,13 +599,27 @@ impl NetworkGraph { } } - /// For a new or already known (from previous announcement) channel, store or update channel info. - /// Also store nodes (if not stored yet) the channel is between, and make node aware of this channel. - /// Checking utxo on-chain is useful if we receive an update for already known channel id, - /// which is probably result of a reorg. In that case, we update channel info only if the - /// utxo was checked, otherwise stick to the existing update, to prevent DoS risks. + /// Store or update channel info from a channel announcement. + /// + /// You probably don't want to call this directly, instead relying on a NetGraphMsgHandler's + /// RoutingMessageHandler implementation to call it indirectly. This may be useful to accept + /// routing messages without checking their signatures. + /// + /// If the channel has been confirmed to exist on chain (with correctly-formatted scripts on + /// chain), set utxo_value to the value of the output on chain, otherwise leave it as None. + /// The UTXO value is then used in routing calculation if we have no better information on the + /// maximum HTLC value that can be sent over the channel. + /// + /// Further, setting utxo_value to Some indicates that the announcement message is genuine, + /// allowing us to update existing channel data in the case of reorgs or to replace bogus + /// channel data generated by a DoS attacker. + /// /// Announcement signatures are checked here only if Secp256k1 object is provided. - fn update_channel_from_announcement(&mut self, msg: &msgs::ChannelAnnouncement, utxo_value: Option, secp_ctx: Option<&Secp256k1>) -> Result { + pub fn update_channel_from_announcement(&mut self, msg: &msgs::ChannelAnnouncement, utxo_value: Option, secp_ctx: Option<&Secp256k1>) -> Result { + if msg.contents.node_id_1 == msg.contents.node_id_2 || msg.contents.bitcoin_key_1 == msg.contents.bitcoin_key_2 { + return Err(LightningError{err: "Channel announcement node had a channel with itself".to_owned(), action: ErrorAction::IgnoreError}); + } + if let Some(sig_verifier) = secp_ctx { let msg_hash = hash_to_message!(&Sha256dHash::hash(&msg.contents.encode()[..])[..]); secp_verify_sig!(sig_verifier, &msg_hash, &msg.node_signature_1, &msg.contents.node_id_1); @@ -699,8 +718,13 @@ impl NetworkGraph { } /// For an already known (from announcement) channel, update info about one of the directions of a channel. + /// + /// You probably don't want to call this directly, instead relying on a NetGraphMsgHandler's + /// RoutingMessageHandler implementation to call it indirectly. This may be useful to accept + /// routing messages without checking their signatures. + /// /// Announcement signatures are checked here only if Secp256k1 object is provided. - fn update_channel(&mut self, msg: &msgs::ChannelUpdate, secp_ctx: Option<&Secp256k1>) -> Result { + pub fn update_channel(&mut self, msg: &msgs::ChannelUpdate, secp_ctx: Option<&Secp256k1>) -> Result { let dest_node_id; let chan_enabled = msg.contents.flags & (1 << 1) != (1 << 1); let chan_was_enabled; @@ -716,8 +740,8 @@ impl NetworkGraph { if let Some(capacity_sats) = channel.capacity_sats { // It's possible channel capacity is available now, although it wasn't available at announcement (so the field is None). // Don't query UTXO set here to reduce DoS risks. - if htlc_maximum_msat > capacity_sats * 1000 { - return Err(LightningError{err: "htlc_maximum_msat is larger than channel capacity".to_owned(), action: ErrorAction::IgnoreError}); + if capacity_sats > MAX_VALUE_MSAT / 1000 || htlc_maximum_msat > capacity_sats * 1000 { + return Err(LightningError{err: "htlc_maximum_msat is larger than channel capacity or capacity is bogus".to_owned(), action: ErrorAction::IgnoreError}); } } } @@ -1302,7 +1326,7 @@ mod tests { match net_graph_msg_handler.handle_channel_update(&valid_channel_update) { Ok(_) => panic!(), - Err(e) => assert_eq!(e.err, "htlc_maximum_msat is larger than channel capacity") + Err(e) => assert_eq!(e.err, "htlc_maximum_msat is larger than channel capacity or capacity is bogus") }; unsigned_channel_update.htlc_maximum_msat = OptionalField::Absent; diff --git a/lightning/src/routing/router.rs b/lightning/src/routing/router.rs index 64a08e73..490b6b40 100644 --- a/lightning/src/routing/router.rs +++ b/lightning/src/routing/router.rs @@ -237,13 +237,12 @@ pub fn get_route(our_node_id: &PublicKey, network: &NetworkGraph, targ let mut total_fee = $starting_fee_msat as u64; let hm_entry = dist.entry(&$src_node_id); let old_entry = hm_entry.or_insert_with(|| { - let node = network.get_nodes().get(&$src_node_id).unwrap(); let mut fee_base_msat = u32::max_value(); let mut fee_proportional_millionths = u32::max_value(); - if let Some(fees) = node.lowest_inbound_channel_fees { + if let Some(fees) = network.get_nodes().get(&$src_node_id).and_then(|node| node.lowest_inbound_channel_fees) { fee_base_msat = fees.base_msat; fee_proportional_millionths = fees.proportional_millionths; - }; + } (u64::max_value(), fee_base_msat, fee_proportional_millionths, @@ -342,21 +341,26 @@ pub fn get_route(our_node_id: &PublicKey, network: &NetworkGraph, targ } for hop in last_hops.iter() { - if first_hops.is_none() || hop.src_node_id != *our_node_id { // first_hop overrules last_hops - if network.get_nodes().get(&hop.src_node_id).is_some() { - if first_hops.is_some() { - if let Some(&(ref first_hop, ref features)) = first_hop_targets.get(&hop.src_node_id) { - // Currently there are no channel-context features defined, so we are a - // bit lazy here. In the future, we should pull them out via our - // ChannelManager, but there's no reason to waste the space until we - // need them. - add_entry!(first_hop, *our_node_id , hop.src_node_id, dummy_directional_info, features.to_context(), 0); - } - } - // BOLT 11 doesn't allow inclusion of features for the last hop hints, which - // really sucks, cause we're gonna need that eventually. - add_entry!(hop.short_channel_id, hop.src_node_id, target, hop, ChannelFeatures::empty(), 0); - } + let have_hop_src_in_graph = + if let Some(&(ref first_hop, ref features)) = first_hop_targets.get(&hop.src_node_id) { + // If this hop connects to a node with which we have a direct channel, ignore the + // network graph and add both the hop and our direct channel to the candidate set: + // + // Currently there are no channel-context features defined, so we are a + // bit lazy here. In the future, we should pull them out via our + // ChannelManager, but there's no reason to waste the space until we + // need them. + add_entry!(first_hop, *our_node_id , hop.src_node_id, dummy_directional_info, features.to_context(), 0); + true + } else { + // In any other case, only add the hop if the source is in the regular network + // graph: + network.get_nodes().get(&hop.src_node_id).is_some() + }; + if have_hop_src_in_graph { + // BOLT 11 doesn't allow inclusion of features for the last hop hints, which + // really sucks, cause we're gonna need that eventually. + add_entry!(hop.short_channel_id, hop.src_node_id, target, hop, ChannelFeatures::empty(), 0); } } @@ -412,7 +416,7 @@ pub fn get_route(our_node_id: &PublicKey, network: &NetworkGraph, targ #[cfg(test)] mod tests { use routing::router::{get_route, RouteHint, RoutingFees}; - use routing::network_graph::NetGraphMsgHandler; + use routing::network_graph::{NetworkGraph, NetGraphMsgHandler}; use ln::features::{ChannelFeatures, InitFeatures, NodeFeatures}; use ln::msgs::{ErrorAction, LightningError, OptionalField, UnsignedChannelAnnouncement, ChannelAnnouncement, RoutingMessageHandler, NodeAnnouncement, UnsignedNodeAnnouncement, ChannelUpdate, UnsignedChannelUpdate}; @@ -1222,4 +1226,54 @@ mod tests { assert_eq!(route.paths[0][4].node_features.le_flags(), &Vec::::new()); // We dont pass flags in from invoices yet assert_eq!(route.paths[0][4].channel_features.le_flags(), &Vec::::new()); // We can't learn any flags from invoices, sadly } + + #[test] + fn unannounced_path_test() { + // We should be able to send a payment to a destination without any help of a routing graph + // if we have a channel with a common counterparty that appears in the first and last hop + // hints. + let source_node_id = PublicKey::from_secret_key(&Secp256k1::new(), &SecretKey::from_slice(&hex::decode(format!("{:02}", 41).repeat(32)).unwrap()[..]).unwrap()); + let middle_node_id = PublicKey::from_secret_key(&Secp256k1::new(), &SecretKey::from_slice(&hex::decode(format!("{:02}", 42).repeat(32)).unwrap()[..]).unwrap()); + let target_node_id = PublicKey::from_secret_key(&Secp256k1::new(), &SecretKey::from_slice(&hex::decode(format!("{:02}", 43).repeat(32)).unwrap()[..]).unwrap()); + + // If we specify a channel to a middle hop, that overrides our local channel view and that gets used + let last_hops = vec![RouteHint { + src_node_id: middle_node_id, + short_channel_id: 8, + fees: RoutingFees { + base_msat: 1000, + proportional_millionths: 0, + }, + cltv_expiry_delta: (8 << 8) | 1, + htlc_minimum_msat: 0, + }]; + let our_chans = vec![channelmanager::ChannelDetails { + channel_id: [0; 32], + short_channel_id: Some(42), + remote_network_id: middle_node_id, + counterparty_features: InitFeatures::from_le_bytes(vec![0b11]), + channel_value_satoshis: 100000, + user_id: 0, + outbound_capacity_msat: 100000, + inbound_capacity_msat: 100000, + is_live: true, + }]; + let route = get_route(&source_node_id, &NetworkGraph::new(), &target_node_id, Some(&our_chans.iter().collect::>()), &last_hops.iter().collect::>(), 100, 42, Arc::new(test_utils::TestLogger::new())).unwrap(); + + assert_eq!(route.paths[0].len(), 2); + + assert_eq!(route.paths[0][0].pubkey, middle_node_id); + assert_eq!(route.paths[0][0].short_channel_id, 42); + assert_eq!(route.paths[0][0].fee_msat, 1000); + assert_eq!(route.paths[0][0].cltv_expiry_delta, (8 << 8) | 1); + assert_eq!(route.paths[0][0].node_features.le_flags(), &[0b11]); + assert_eq!(route.paths[0][0].channel_features.le_flags(), &[0; 0]); // We can't learn any flags from invoices, sadly + + assert_eq!(route.paths[0][1].pubkey, target_node_id); + assert_eq!(route.paths[0][1].short_channel_id, 8); + assert_eq!(route.paths[0][1].fee_msat, 100); + assert_eq!(route.paths[0][1].cltv_expiry_delta, 42); + assert_eq!(route.paths[0][1].node_features.le_flags(), &[0; 0]); // We dont pass flags in from invoices yet + assert_eq!(route.paths[0][1].channel_features.le_flags(), &[0; 0]); // We can't learn any flags from invoices, sadly + } }