Merge pull request #748 from TheBlueMatt/2020-11-router-fuzzer

TheBlueMatt · web-flow · commit fc7df54f8dc3 · 2020-11-24T08:36:14.000-08:00
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
@@ -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,
 		}
 	}
 }
diff --git 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<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;
 
diff --git a/lightning/src/routing/router.rs b/lightning/src/routing/router.rs
@@ -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
+	}
 }
