Return error when sending onion messages over exceedingly long paths

Previously, we would panic when sending onion messages over paths with
an excessive number of hops. Here we opt to rather error out and don't
panic if something goes wrong during OM packet construction.

Author: tnull

lightning/src/ln/channelmanager.rs

@@ -2731,7 +2731,12 @@ where
 		if onion_utils::route_size_insane(&onion_payloads) {
 			return Err(APIError::InvalidRoute{err: "Route size too large considering onion data".to_owned()});
 		}
-		let onion_packet = onion_utils::construct_onion_packet(onion_payloads, onion_keys, prng_seed, payment_hash);
+		let onion_packet = match onion_utils::construct_onion_packet(onion_payloads, onion_keys, prng_seed, payment_hash) {
+			Ok(packet) => packet,
+			Err(()) => {
+				return Err(APIError::InvalidRoute{err: "Failed to create onion packet".to_owned()});
+			}
+		};
 
 		let err: Result<(), _> = loop {
 			let (counterparty_node_id, id) = match self.short_to_chan_info.read().unwrap().get(&path.hops.first().unwrap().short_channel_id) {

lightning/src/ln/functional_tests.rs

@@ -1383,7 +1383,7 @@ fn test_fee_spike_violation_fails_htlc() {
 	let onion_keys = onion_utils::construct_onion_keys(&secp_ctx, &route.paths[0], &session_priv).unwrap();
 	let (onion_payloads, htlc_msat, htlc_cltv) = onion_utils::build_onion_payloads(&route.paths[0],
 		3460001, RecipientOnionFields::secret_only(payment_secret), cur_height, &None).unwrap();
-	let onion_packet = onion_utils::construct_onion_packet(onion_payloads, onion_keys, [0; 32], &payment_hash);
+	let onion_packet = onion_utils::construct_onion_packet(onion_payloads, onion_keys, [0; 32], &payment_hash).unwrap();
 	let msg = msgs::UpdateAddHTLC {
 		channel_id: chan.2,
 		htlc_id: 0,
@@ -1571,7 +1571,7 @@ fn test_chan_reserve_violation_inbound_htlc_outbound_channel() {
 	let onion_keys = onion_utils::construct_onion_keys(&secp_ctx, &route.paths[0], &session_priv).unwrap();
 	let (onion_payloads, htlc_msat, htlc_cltv) = onion_utils::build_onion_payloads(&route.paths[0],
 		700_000, RecipientOnionFields::secret_only(payment_secret), cur_height, &None).unwrap();
-	let onion_packet = onion_utils::construct_onion_packet(onion_payloads, onion_keys, [0; 32], &payment_hash);
+	let onion_packet = onion_utils::construct_onion_packet(onion_payloads, onion_keys, [0; 32], &payment_hash).unwrap();
 	let msg = msgs::UpdateAddHTLC {
 		channel_id: chan.2,
 		htlc_id: MIN_AFFORDABLE_HTLC_COUNT as u64,
@@ -1745,7 +1745,7 @@ fn test_chan_reserve_violation_inbound_htlc_inbound_chan() {
 	let onion_keys = onion_utils::construct_onion_keys(&secp_ctx, &route_2.paths[0], &session_priv).unwrap();
 	let (onion_payloads, htlc_msat, htlc_cltv) = onion_utils::build_onion_payloads(
 		&route_2.paths[0], recv_value_2, RecipientOnionFields::spontaneous_empty(), cur_height, &None).unwrap();
-	let onion_packet = onion_utils::construct_onion_packet(onion_payloads, onion_keys, [0; 32], &our_payment_hash_1);
+	let onion_packet = onion_utils::construct_onion_packet(onion_payloads, onion_keys, [0; 32], &our_payment_hash_1).unwrap();
 	let msg = msgs::UpdateAddHTLC {
 		channel_id: chan.2,
 		htlc_id: 1,
@@ -3398,7 +3398,7 @@ fn fail_backward_pending_htlc_upon_channel_failure() {
 		let (onion_payloads, _amount_msat, cltv_expiry) = onion_utils::build_onion_payloads(
 			&route.paths[0], 50_000, RecipientOnionFields::secret_only(payment_secret), current_height, &None).unwrap();
 		let onion_keys = onion_utils::construct_onion_keys(&secp_ctx, &route.paths[0], &session_priv).unwrap();
-		let onion_routing_packet = onion_utils::construct_onion_packet(onion_payloads, onion_keys, [0; 32], &payment_hash);
+		let onion_routing_packet = onion_utils::construct_onion_packet(onion_payloads, onion_keys, [0; 32], &payment_hash).unwrap();
 
 		// Send a 0-msat update_add_htlc to fail the channel.
 		let update_add_htlc = msgs::UpdateAddHTLC {
@@ -6267,7 +6267,7 @@ fn test_update_add_htlc_bolt2_receiver_check_max_htlc_limit() {
 	let onion_keys = onion_utils::construct_onion_keys(&Secp256k1::signing_only(), &route.paths[0], &session_priv).unwrap();
 	let (onion_payloads, _htlc_msat, htlc_cltv) = onion_utils::build_onion_payloads(
 		&route.paths[0], 3999999, RecipientOnionFields::secret_only(our_payment_secret), cur_height, &None).unwrap();
-	let onion_packet = onion_utils::construct_onion_packet(onion_payloads, onion_keys, [0; 32], &our_payment_hash);
+	let onion_packet = onion_utils::construct_onion_packet(onion_payloads, onion_keys, [0; 32], &our_payment_hash).unwrap();
 
 	let mut msg = msgs::UpdateAddHTLC {
 		channel_id: chan.2,
@@ -8087,7 +8087,7 @@ fn test_onion_value_mpp_set_calculation() {
 			// Edit amt_to_forward to simulate the sender having set
 			// the final amount and the routing node taking less fee
 			onion_payloads[1].amt_to_forward = 99_000;
-			let new_onion_packet = onion_utils::construct_onion_packet(onion_payloads, onion_keys, [0; 32], &our_payment_hash);
+			let new_onion_packet = onion_utils::construct_onion_packet(onion_payloads, onion_keys, [0; 32], &our_payment_hash).unwrap();
 			payment_event.msgs[0].onion_routing_packet = new_onion_packet;
 		}
 

lightning/src/ln/onion_route_tests.rs

@@ -359,7 +359,7 @@ fn test_onion_failure() {
 		// break the first (non-final) hop payload by swapping the realm (0) byte for a byte
 		// describing a length-1 TLV payload, which is obviously bogus.
 		new_payloads[0].data[0] = 1;
-		msg.onion_routing_packet = onion_utils::construct_onion_packet_with_writable_hopdata(new_payloads, onion_keys, [0; 32], &payment_hash);
+		msg.onion_routing_packet = onion_utils::construct_onion_packet_with_writable_hopdata(new_payloads, onion_keys, [0; 32], &payment_hash).unwrap();
 	}, ||{}, true, Some(PERM|22), Some(NetworkUpdate::ChannelFailure{short_channel_id, is_permanent: true}), Some(short_channel_id));
 
 	// final node failure
@@ -377,7 +377,7 @@ fn test_onion_failure() {
 		// break the last-hop payload by swapping the realm (0) byte for a byte describing a
 		// length-1 TLV payload, which is obviously bogus.
 		new_payloads[1].data[0] = 1;
-		msg.onion_routing_packet = onion_utils::construct_onion_packet_with_writable_hopdata(new_payloads, onion_keys, [0; 32], &payment_hash);
+		msg.onion_routing_packet = onion_utils::construct_onion_packet_with_writable_hopdata(new_payloads, onion_keys, [0; 32], &payment_hash).unwrap();
 	}, ||{}, false, Some(PERM|22), Some(NetworkUpdate::ChannelFailure{short_channel_id, is_permanent: true}), Some(short_channel_id));
 
 	// the following three with run_onion_failure_test_with_fail_intercept() test only the origin node
@@ -607,7 +607,7 @@ fn test_onion_failure() {
 		let onion_keys = onion_utils::construct_onion_keys(&Secp256k1::new(), &route.paths[0], &session_priv).unwrap();
 		let (onion_payloads, _, htlc_cltv) = onion_utils::build_onion_payloads(
 			&route.paths[0], 40000, RecipientOnionFields::spontaneous_empty(), height, &None).unwrap();
-		let onion_packet = onion_utils::construct_onion_packet(onion_payloads, onion_keys, [0; 32], &payment_hash);
+		let onion_packet = onion_utils::construct_onion_packet(onion_payloads, onion_keys, [0; 32], &payment_hash).unwrap();
 		msg.cltv_expiry = htlc_cltv;
 		msg.onion_routing_packet = onion_packet;
 	}, ||{}, true, Some(21), Some(NetworkUpdate::NodeFailure{node_id: route.paths[0].hops[0].pubkey, is_permanent: true}), Some(route.paths[0].hops[0].short_channel_id));
@@ -1106,7 +1106,7 @@ fn test_phantom_invalid_onion_payload() {
 					onion_keys.remove(0);
 					onion_payloads.remove(0);
 
-					let new_onion_packet = onion_utils::construct_onion_packet(onion_payloads, onion_keys, [0; 32], &payment_hash);
+					let new_onion_packet = onion_utils::construct_onion_packet(onion_payloads, onion_keys, [0; 32], &payment_hash).unwrap();
 					onion_packet.hop_data = new_onion_packet.hop_data;
 					onion_packet.hmac = new_onion_packet.hmac;
 				},

lightning/src/ln/onion_utils.rs

@@ -222,8 +222,7 @@ pub(super) fn route_size_insane(payloads: &Vec<msgs::OnionHopData>) -> bool {
 	false
 }
 
-/// panics if route_size_insane(payloads)
-pub(super) fn construct_onion_packet(payloads: Vec<msgs::OnionHopData>, onion_keys: Vec<OnionKeys>, prng_seed: [u8; 32], associated_data: &PaymentHash) -> msgs::OnionPacket {
+pub(super) fn construct_onion_packet(payloads: Vec<msgs::OnionHopData>, onion_keys: Vec<OnionKeys>, prng_seed: [u8; 32], associated_data: &PaymentHash) -> Result<msgs::OnionPacket, ()> {
 	let mut packet_data = [0; ONION_DATA_LEN];
 
 	let mut chacha = ChaCha20::new(&prng_seed, &[0; 8]);
@@ -236,7 +235,7 @@ pub(super) fn construct_onion_packet(payloads: Vec<msgs::OnionHopData>, onion_ke
 #[cfg(test)]
 /// Used in testing to write bogus `BogusOnionHopData` as well as `RawOnionHopData`, which is
 /// otherwise not representable in `msgs::OnionHopData`.
-pub(super) fn construct_onion_packet_with_writable_hopdata<HD: Writeable>(payloads: Vec<HD>, onion_keys: Vec<OnionKeys>, prng_seed: [u8; 32], associated_data: &PaymentHash) -> msgs::OnionPacket {
+pub(super) fn construct_onion_packet_with_writable_hopdata<HD: Writeable>(payloads: Vec<HD>, onion_keys: Vec<OnionKeys>, prng_seed: [u8; 32], associated_data: &PaymentHash) -> Result<msgs::OnionPacket, ()> {
 	let mut packet_data = [0; ONION_DATA_LEN];
 
 	let mut chacha = ChaCha20::new(&prng_seed, &[0; 8]);
@@ -268,9 +267,8 @@ pub(crate) fn payloads_serialized_length<HD: Writeable>(payloads: &Vec<HD>) -> u
 	payloads.iter().map(|p| p.serialized_length() + 32 /* HMAC */).sum()
 }
 
-/// panics if payloads_serialized_length(payloads) > packet_data_len
 pub(crate) fn construct_onion_message_packet<HD: Writeable, P: Packet<Data = Vec<u8>>>(
-	payloads: Vec<HD>, onion_keys: Vec<OnionKeys>, prng_seed: [u8; 32], packet_data_len: usize) -> P
+	payloads: Vec<HD>, onion_keys: Vec<OnionKeys>, prng_seed: [u8; 32], packet_data_len: usize) -> Result<P, ()>
 {
 	let mut packet_data = vec![0; packet_data_len];
 
@@ -280,9 +278,8 @@ pub(crate) fn construct_onion_message_packet<HD: Writeable, P: Packet<Data = Vec
 	construct_onion_packet_with_init_noise::<_, _>(payloads, onion_keys, packet_data, None)
 }
 
-/// panics if payloads_serialized_length(payloads) > packet_data.len()
 fn construct_onion_packet_with_init_noise<HD: Writeable, P: Packet>(
-	mut payloads: Vec<HD>, onion_keys: Vec<OnionKeys>, mut packet_data: P::Data, associated_data: Option<&PaymentHash>) -> P
+	mut payloads: Vec<HD>, onion_keys: Vec<OnionKeys>, mut packet_data: P::Data, associated_data: Option<&PaymentHash>) -> Result<P, ()>
 {
 	let filler = {
 		let packet_data = packet_data.as_mut();
@@ -300,7 +297,7 @@ fn construct_onion_packet_with_init_noise<HD: Writeable, P: Packet>(
 			}
 
 			let mut payload_len = LengthCalculatingWriter(0);
-			payload.write(&mut payload_len).expect("Failed to calculate length");
+			payload.write(&mut payload_len).map_err(|_| ())?;
 			pos += payload_len.0 + 32;
 			assert!(pos <= packet_data.len());
 
@@ -313,7 +310,7 @@ fn construct_onion_packet_with_init_noise<HD: Writeable, P: Packet>(
 	let mut hmac_res = [0; 32];
 	for (i, (payload, keys)) in payloads.iter_mut().zip(onion_keys.iter()).rev().enumerate() {
 		let mut payload_len = LengthCalculatingWriter(0);
-		payload.write(&mut payload_len).expect("Failed to calculate length");
+		payload.write(&mut payload_len).map_err(|_| ())?;
 
 		let packet_data = packet_data.as_mut();
 		shift_slice_right(packet_data, payload_len.0 + 32);
@@ -324,7 +321,9 @@ fn construct_onion_packet_with_init_noise<HD: Writeable, P: Packet>(
 		chacha.process_in_place(packet_data);
 
 		if i == 0 {
-			packet_data[ONION_DATA_LEN - filler.len()..ONION_DATA_LEN].copy_from_slice(&filler[..]);
+			let start_index = ONION_DATA_LEN.checked_sub(filler.len()).ok_or(())?;
+			let stop_index = ONION_DATA_LEN;
+			packet_data[start_index..stop_index].copy_from_slice(&filler[..]);
 		}
 
 		let mut hmac = HmacEngine::<Sha256>::new(&keys.mu);
@@ -335,7 +334,7 @@ fn construct_onion_packet_with_init_noise<HD: Writeable, P: Packet>(
 		hmac_res = Hmac::from_engine(hmac).into_inner();
 	}
 
-	P::new(onion_keys.first().unwrap().ephemeral_pubkey, packet_data, hmac_res)
+	Ok(P::new(onion_keys.first().unwrap().ephemeral_pubkey, packet_data, hmac_res))
 }
 
 /// Encrypts a failure packet. raw_packet can either be a
@@ -1081,7 +1080,7 @@ mod tests {
 
 		let pad_keytype_seed = super::gen_pad_from_shared_secret(&get_test_session_key().secret_bytes());
 
-		let packet: msgs::OnionPacket = super::construct_onion_packet_with_writable_hopdata::<_>(payloads, onion_keys, pad_keytype_seed, &PaymentHash([0x42; 32]));
+		let packet: msgs::OnionPacket = super::construct_onion_packet_with_writable_hopdata::<_>(payloads, onion_keys, pad_keytype_seed, &PaymentHash([0x42; 32])).unwrap();
 
 		assert_eq!(packet.encode(), hex::decode("0002EEC7245D6B7D2CCB30380BFBE2A3648CD7A942653F5AA340EDCEA1F283686619F7F3416A5AA36DC7EEB3EC6D421E9615471AB870A33AC07FA5D5A51DF0A8823AABE3FEA3F90D387529D4F72837F9E687230371CCD8D263072206DBED0234F6505E21E282ABD8C0E4F5B9FF8042800BBAB065036EADD0149B37F27DDE664725A49866E052E809D2B0198AB9610FAA656BBF4EC516763A59F8F42C171B179166BA38958D4F51B39B3E98706E2D14A2DAFD6A5DF808093ABFCA5AEAACA16EDED5DB7D21FB0294DD1A163EDF0FB445D5C8D7D688D6DD9C541762BF5A5123BF9939D957FE648416E88F1B0928BFA034982B22548E1A4D922690EECF546275AFB233ACF4323974680779F1A964CFE687456035CC0FBA8A5428430B390F0057B6D1FE9A8875BFA89693EEB838CE59F09D207A503EE6F6299C92D6361BC335FCBF9B5CD44747AADCE2CE6069CFDC3D671DAEF9F8AE590CF93D957C9E873E9A1BC62D9640DC8FC39C14902D49A1C80239B6C5B7FD91D05878CBF5FFC7DB2569F47C43D6C0D27C438ABFF276E87364DEB8858A37E5A62C446AF95D8B786EAF0B5FCF78D98B41496794F8DCAAC4EEF34B2ACFB94C7E8C32A9E9866A8FA0B6F2A06F00A1CCDE569F97EEC05C803BA7500ACC96691D8898D73D8E6A47B8F43C3D5DE74458D20EDA61474C426359677001FBD75A74D7D5DB6CB4FEB83122F133206203E4E2D293F838BF8C8B3A29ACB321315100B87E80E0EDB272EE80FDA944E3FB6084ED4D7F7C7D21C69D9DA43D31A90B70693F9B0CC3EAC74C11AB8FF655905688916CFA4EF0BD04135F2E50B7C689A21D04E8E981E74C6058188B9B1F9DFC3EEC6838E9FFBCF22CE738D8A177C19318DFFEF090CEE67E12DE1A3E2A39F61247547BA5257489CBC11D7D91ED34617FCC42F7A9DA2E3CF31A94A210A1018143173913C38F60E62B24BF0D7518F38B5BAB3E6A1F8AEB35E31D6442C8ABB5178EFC892D2E787D79C6AD9E2FC271792983FA9955AC4D1D84A36C024071BC6E431B625519D556AF38185601F70E29035EA6A09C8B676C9D88CF7E05E0F17098B584C4168735940263F940033A220F40BE4C85344128B14BEB9E75696DB37014107801A59B13E89CD9D2258C169D523BE6D31552C44C82FF4BB18EC9F099F3BF0E5B1BB2BA9A87D7E26F98D294927B600B5529C47E04D98956677CBCEE8FA2B60F49776D8B8C367465B7C626DA53700684FB6C918EAD0EAB8360E4F60EDD25B4F43816A75ECF70F909301825B512469F8389D79402311D8AECB7B3EF8599E79485A4388D87744D899F7C47EE644361E17040A7958C8911BE6F463AB6A9B2AFACD688EC55EF517B38F1339EFC54487232798BB25522FF4572FF68567FE830F92F7B8113EFCE3E98C3FFFBAEDCE4FD8B50E41DA97C0C08E423A72689CC68E68F752A5E3A9003E64E35C957CA2E1C48BB6F64B05F56B70B575AD2F278D57850A7AD568C24A4D32A3D74B29F03DC125488BC7C637DA582357F40B0A52D16B3B40BB2C2315D03360BC24209E20972C200566BCF3BBE5C5B0AEDD83132A8A4D5B4242BA370B6D67D9B67EB01052D132C7866B9CB502E44796D9D356E4E3CB47CC527322CD24976FE7C9257A2864151A38E568EF7A79F10D6EF27CC04CE382347A2488B1F404FDBF407FE1CA1C9D0D5649E34800E25E18951C98CAE9F43555EEF65FEE1EA8F15828807366C3B612CD5753BF9FB8FCED08855F742CDDD6F765F74254F03186683D646E6F09AC2805586C7CF11998357CAFC5DF3F285329366F475130C928B2DCEBA4AA383758E7A9D20705C4BB9DB619E2992F608A1BA65DB254BB389468741D0502E2588AEB54390AC600C19AF5C8E61383FC1BEBE0029E4474051E4EF908828DB9CCA13277EF65DB3FD47CCC2179126AAEFB627719F421E20").unwrap());
 	}

lightning/src/onion_message/functional_tests.rs

@@ -264,3 +264,45 @@ fn peer_buffer_full() {
 	let err = nodes[0].messenger.send_onion_message(&[], Destination::Node(nodes[1].get_node_pk()), OnionMessageContents::Custom(test_msg), None).unwrap_err();
 	assert_eq!(err, SendError::BufferFull);
 }
+
+#[test]
+fn many_hops() {
+	// Check we can send over a long route with 16 hops.
+	let num_nodes: usize = 16;
+	let nodes = create_nodes(num_nodes as u8);
+	let test_msg = OnionMessageContents::Custom(TestCustomMessage {});
+
+	let mut intermediates = vec![];
+	for i in 1..(num_nodes-1) {
+		intermediates.push(nodes[i].get_node_pk());
+	}
+
+	nodes[0].messenger.send_onion_message(&intermediates, Destination::Node(nodes[num_nodes-1].get_node_pk()), test_msg, None).unwrap();
+	pass_along_path(&nodes, None);
+
+	// Check we can send over a long route with 17 hops.
+	let num_nodes: usize = 17;
+	let nodes = create_nodes(num_nodes as u8);
+	let test_msg = OnionMessageContents::Custom(TestCustomMessage {});
+
+	let mut intermediates = vec![];
+	for i in 1..(num_nodes-1) {
+		intermediates.push(nodes[i].get_node_pk());
+	}
+
+	nodes[0].messenger.send_onion_message(&intermediates, Destination::Node(nodes[num_nodes-1].get_node_pk()), test_msg, None).unwrap();
+	pass_along_path(&nodes, None);
+
+	// Check we fail to send over a longer route with 18 hops, but receive an error instead of
+	// panicking.
+	let num_nodes: usize = 18;
+	let nodes = create_nodes(num_nodes as u8);
+	let test_msg = OnionMessageContents::Custom(TestCustomMessage {});
+
+	let mut intermediates = vec![];
+	for i in 1..(num_nodes-1) {
+		intermediates.push(nodes[i].get_node_pk());
+	}
+
+	assert_eq!(nodes[0].messenger.send_onion_message(&intermediates, Destination::Node(nodes[num_nodes-1].get_node_pk()), test_msg, None), Err(SendError::TooBigPacket));
+}

lightning/src/onion_message/messenger.rs

@@ -567,6 +567,6 @@ fn construct_onion_message_packet<T: CustomOnionMessageContents>(payloads: Vec<(
 		BIG_PACKET_HOP_DATA_LEN
 	} else { return Err(()) };
 
-	Ok(onion_utils::construct_onion_message_packet::<_, _>(
-		payloads, onion_keys, prng_seed, hop_data_len))
+	onion_utils::construct_onion_message_packet::<_, _>(
+		payloads, onion_keys, prng_seed, hop_data_len)
 }