Merge pull request #748 from TheBlueMatt/2020-11-router-fuzzer
authorMatt Corallo <649246+TheBlueMatt@users.noreply.github.com>
Tue, 24 Nov 2020 16:36:14 +0000 (08:36 -0800)
committerGitHub <noreply@github.com>
Tue, 24 Nov 2020 16:36:14 +0000 (08:36 -0800)
Make router_target a bit easier for fuzzers to explore and fix two found bugs

fuzz/src/router.rs
lightning/src/routing/network_graph.rs
lightning/src/routing/router.rs

index 5f2114b64e169eaa875b18afe6a41f3b3d566de2..ef7669f1eafd7f477182452fb9006610bd95f2e0 100644 (file)
@@ -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<Out: test_logger::Output>(data: &[u8], out: Out) {
        }
 
        let logger: Arc<dyn Logger> = 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<Out: test_logger::Output>(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::<secp256k1::VerifyOnly>(&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::<secp256k1::VerifyOnly>(&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::<secp256k1::VerifyOnly>(&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<Out: test_logger::Output>(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<Out: test_logger::Output>(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::<Vec<_>>()).as_ref().map(|a| a.as_slice()),
-                                       &last_hops.iter().collect::<Vec<_>>(),
-                                       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::<Vec<_>>()).as_ref().map(|a| a.as_slice()),
+                                               &last_hops.iter().collect::<Vec<_>>(),
+                                               slice_to_be64(get_slice!(8)), slice_to_be32(get_slice!(4)), Arc::clone(&logger));
+                               }
                        },
-                       _ => return,
                }
        }
 }
index 308c0526eb58cc84207abce8c27d114a059e0bcc..932f3779deff99d4456a4d81f3ec6c43310810da 100644 (file)
@@ -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<secp256k1::VerifyOnly>>) -> Result<bool, LightningError> {
+       pub fn update_node_from_announcement<T: secp256k1::Verification>(&mut self, msg: &msgs::NodeAnnouncement, secp_ctx: Option<&Secp256k1<T>>) -> Result<bool, LightningError> {
                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<u64>, secp_ctx: Option<&Secp256k1<secp256k1::VerifyOnly>>) -> Result<bool, LightningError> {
+       pub fn update_channel_from_announcement<T: secp256k1::Verification>(&mut self, msg: &msgs::ChannelAnnouncement, utxo_value: Option<u64>, secp_ctx: Option<&Secp256k1<T>>) -> Result<bool, LightningError> {
+               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<secp256k1::VerifyOnly>>) -> Result<bool, LightningError> {
+       pub fn update_channel(&mut self, msg: &msgs::ChannelUpdate, secp_ctx: Option<&Secp256k1<secp256k1::VerifyOnly>>) -> Result<bool, LightningError> {
                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;
 
index 64a08e732ecbd6e032a154eacb46133fadc882a9..490b6b4048cbd33a7ba60e8b578b6dddbfcd4e17 100644 (file)
@@ -237,13 +237,12 @@ pub fn get_route<L: Deref>(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<L: Deref>(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<L: Deref>(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::<u8>::new()); // We dont pass flags in from invoices yet
                assert_eq!(route.paths[0][4].channel_features.le_flags(), &Vec::<u8>::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::<Vec<_>>()), &last_hops.iter().collect::<Vec<_>>(), 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
+       }
 }