Reject unknown TLVs in blinded payment payloads

Reject unknown odd TLVs and disallowed custom TLVs when decoding
blinded payment and trampoline payloads.

Add a dedicated helper macro for TLV readers with custom decode
callbacks and update the affected tests to cover the stricter
validation paths.

Signed-off-by: Asish Kumar <officialasishkumar@gmail.com>

Author: officialasishkumar

lightning/src/blinded_path/payment.rs

@@ -632,7 +632,7 @@ impl<'a> Writeable for BlindedPaymentTlvsRef<'a> {
 
 impl Readable for BlindedPaymentTlvs {
 	fn read<R: io::Read>(r: &mut R) -> Result<Self, DecodeError> {
-		_init_and_read_tlv_stream!(r, {
+		_init_and_read_tlv_stream_with_custom_tlv_decode!(r, {
 			// Reasoning: Padding refers to filler data added to a packet to increase
 			// its size and obscure its actual length. Since padding contains no meaningful
 			// information, we can safely omit reading it here.
@@ -645,6 +645,14 @@ impl Readable for BlindedPaymentTlvs {
 			(65536, payment_secret, option),
 			(65537, payment_context, option),
 			(65539, is_dummy, option)
+		}, |typ: u64, _reader: &mut FixedLengthReader<_>| -> Result<bool, DecodeError> {
+			if typ == 1 {
+				return Ok(true);
+			}
+			if typ % 2 == 1 {
+				return Err(DecodeError::UnknownRequiredFeature);
+			}
+			Ok(false)
 		});
 
 		match (
@@ -685,14 +693,22 @@ impl Readable for BlindedPaymentTlvs {
 
 impl Readable for BlindedTrampolineTlvs {
 	fn read<R: io::Read>(r: &mut R) -> Result<Self, DecodeError> {
-		_init_and_read_tlv_stream!(r, {
+		_init_and_read_tlv_stream_with_custom_tlv_decode!(r, {
 			(4, next_trampoline, option),
 			(8, next_blinding_override, option),
 			(10, payment_relay, option),
 			(12, payment_constraints, required),
 			(14, features, (option, encoding: (BlindedHopFeatures, WithoutLength))),
 			(65536, payment_secret, option),
 			(65537, payment_context, option),
+		}, |typ: u64, _reader: &mut FixedLengthReader<_>| -> Result<bool, DecodeError> {
+			if typ == 1 {
+				return Ok(true);
+			}
+			if typ % 2 == 1 {
+				return Err(DecodeError::UnknownRequiredFeature);
+			}
+			Ok(false)
 		});
 
 		if let Some(next_trampoline) = next_trampoline {
@@ -964,12 +980,15 @@ impl_writeable_tlv_based!(Bolt12RefundContext, {});
 #[cfg(test)]
 mod tests {
 	use crate::blinded_path::payment::{
-		Bolt12RefundContext, ForwardTlvs, PaymentConstraints, PaymentContext, PaymentForwardNode,
-		PaymentRelay, ReceiveTlvs,
+		BlindedPaymentTlvs, BlindedTrampolineTlvs, Bolt12RefundContext, ForwardTlvs,
+		PaymentConstraints, PaymentContext, PaymentForwardNode, PaymentRelay, ReceiveTlvs,
 	};
+	use crate::io::Cursor;
 	use crate::ln::functional_test_utils::TEST_FINAL_CLTV;
+	use crate::ln::msgs::DecodeError;
 	use crate::types::features::BlindedHopFeatures;
 	use crate::types::payment::PaymentSecret;
+	use crate::util::ser::{BigSize, Readable, Writeable};
 	use bitcoin::secp256k1::PublicKey;
 
 	#[test]
@@ -1237,4 +1256,42 @@ mod tests {
 		.unwrap();
 		assert_eq!(blinded_payinfo.htlc_maximum_msat, 3997);
 	}
+
+	#[test]
+	fn blinded_payment_tlvs_reject_unknown_odd_types() {
+		let receive_tlvs = ReceiveTlvs {
+			payment_secret: PaymentSecret([0; 32]),
+			payment_constraints: PaymentConstraints { max_cltv_expiry: 0, htlc_minimum_msat: 1 },
+			payment_context: PaymentContext::Bolt12Refund(Bolt12RefundContext {}),
+		};
+
+		let mut encoded = Vec::new();
+		receive_tlvs.write(&mut encoded).unwrap();
+		BigSize(65541).write(&mut encoded).unwrap();
+		BigSize(0).write(&mut encoded).unwrap();
+
+		assert!(matches!(
+			BlindedPaymentTlvs::read(&mut Cursor::new(&encoded)),
+			Err(DecodeError::UnknownRequiredFeature),
+		));
+	}
+
+	#[test]
+	fn blinded_trampoline_tlvs_reject_unknown_odd_types() {
+		let receive_tlvs = ReceiveTlvs {
+			payment_secret: PaymentSecret([0; 32]),
+			payment_constraints: PaymentConstraints { max_cltv_expiry: 0, htlc_minimum_msat: 1 },
+			payment_context: PaymentContext::Bolt12Refund(Bolt12RefundContext {}),
+		};
+
+		let mut encoded = Vec::new();
+		receive_tlvs.write(&mut encoded).unwrap();
+		BigSize(65541).write(&mut encoded).unwrap();
+		BigSize(0).write(&mut encoded).unwrap();
+
+		assert!(matches!(
+			BlindedTrampolineTlvs::read(&mut Cursor::new(&encoded)),
+			Err(DecodeError::UnknownRequiredFeature),
+		));
+	}
 }

lightning/src/ln/blinded_payment_tests.rs

@@ -1412,7 +1412,7 @@ fn custom_tlvs_to_blinded_path() {
 	let chan_upd = create_announced_chan_between_nodes_with_value(&nodes, 0, 1, 1_000_000, 0).0.contents;
 
 	let amt_msat = 5000;
-	let (payment_preimage, payment_hash, payment_secret) = get_payment_preimage_hash(&nodes[1], Some(amt_msat), None);
+	let (_payment_preimage, payment_hash, payment_secret) = get_payment_preimage_hash(&nodes[1], Some(amt_msat), None);
 	let payee_tlvs = ReceiveTlvs {
 		payment_secret,
 		payment_constraints: PaymentConstraints {
@@ -1448,12 +1448,23 @@ fn custom_tlvs_to_blinded_path() {
 	let path = &[&nodes[1]];
 	let args = PassAlongPathArgs::new(&nodes[0], path, amt_msat, payment_hash, ev)
 		.with_payment_secret(payment_secret)
-		.with_custom_tlvs(recipient_onion_fields.custom_tlvs.clone());
+		.with_custom_tlvs(recipient_onion_fields.custom_tlvs.clone())
+		.without_clearing_recipient_events()
+		.without_claimable_event();
 	do_pass_along_path(args);
-	claim_payment_along_route(
-		ClaimAlongRouteArgs::new(&nodes[0], &[&[&nodes[1]]], payment_preimage)
-			.with_custom_tlvs(recipient_onion_fields.custom_tlvs.clone())
+	check_added_monitors(&nodes[1], 1);
+
+	expect_htlc_handling_failed_destinations!(
+		nodes[1].node.get_and_clear_pending_events(),
+		&[HTLCHandlingFailureType::InvalidOnion]
 	);
+	let updates = get_htlc_update_msgs(&nodes[1], &nodes[0].node.get_our_node_id());
+	assert_eq!(updates.update_fail_htlcs.len(), 1);
+	nodes[0].node.handle_update_fail_htlc(nodes[1].node.get_our_node_id(), &updates.update_fail_htlcs[0]);
+	do_commitment_signed_dance(&nodes[0], &nodes[1], &updates.commitment_signed, false, false);
+	expect_payment_failed_conditions(&nodes[0], payment_hash, true,
+		PaymentFailedConditions::new()
+			.expected_htlc_error_data(LocalHTLCFailureReason::InvalidOnionPayload, &[0; 0]));
 }
 
 #[test]
@@ -1718,116 +1729,12 @@ fn route_blinding_spec_test_vector() {
 	let bob_update_add = update_add_msg(110_000, 747_500, None, bob_onion);
 	let bob_node_signer = TestEcdhSigner { node_secret: bob_secret };
 	// Can't use the public API here as we need to avoid the CLTV delta checks (test vector uses
-	// < MIN_CLTV_EXPIRY_DELTA).
-	let (bob_peeled_onion, next_packet_details_opt) =
-		match onion_payment::decode_incoming_update_add_htlc_onion(
-			&bob_update_add, &bob_node_signer, &logger, &secp_ctx
-		) {
-			Ok(res) => res,
-			_ => panic!("Unexpected error")
-		};
-	let (carol_packet_bytes, carol_hmac) = if let onion_utils::Hop::BlindedForward {
-		next_hop_data: msgs::InboundOnionBlindedForwardPayload {
-			short_channel_id, payment_relay, payment_constraints, features, intro_node_blinding_point, next_blinding_override
-		}, next_hop_hmac, new_packet_bytes, ..
-	} = bob_peeled_onion {
-		assert_eq!(short_channel_id, 1729);
-		assert!(next_blinding_override.is_none());
-		assert_eq!(intro_node_blinding_point, Some(bob_blinding_point));
-		assert_eq!(payment_relay, PaymentRelay { cltv_expiry_delta: 36, fee_proportional_millionths: 150, fee_base_msat: 10_000 });
-		assert_eq!(features, BlindedHopFeatures::empty());
-		assert_eq!(payment_constraints, PaymentConstraints { max_cltv_expiry: 748_005, htlc_minimum_msat: 1500 });
-		(new_packet_bytes, next_hop_hmac)
-	} else { panic!() };
-
-	let carol_packet_details = next_packet_details_opt.unwrap();
-	let carol_onion = msgs::OnionPacket {
-		version: 0,
-		public_key: carol_packet_details.next_packet_pubkey,
-		hop_data: carol_packet_bytes,
-		hmac: carol_hmac,
-	};
-	let carol_update_add = update_add_msg(
-		carol_packet_details.outgoing_amt_msat, carol_packet_details.outgoing_cltv_value,
-		Some(pubkey_from_hex("034e09f450a80c3d252b258aba0a61215bf60dda3b0dc78ffb0736ea1259dfd8a0")),
-		carol_onion
-	);
-	let carol_node_signer = TestEcdhSigner { node_secret: carol_secret };
-	let (carol_peeled_onion, next_packet_details_opt) =
-		match onion_payment::decode_incoming_update_add_htlc_onion(
-			&carol_update_add, &carol_node_signer, &logger, &secp_ctx
-		) {
-			Ok(res) => res,
-			_ => panic!("Unexpected error")
-		};
-	let (dave_packet_bytes, dave_hmac) = if let onion_utils::Hop::BlindedForward {
-		next_hop_data: msgs::InboundOnionBlindedForwardPayload {
-			short_channel_id, payment_relay, payment_constraints, features, intro_node_blinding_point, next_blinding_override
-		}, next_hop_hmac, new_packet_bytes, ..
-	} = carol_peeled_onion {
-		assert_eq!(short_channel_id, 1105);
-		assert_eq!(next_blinding_override, Some(pubkey_from_hex("031b84c5567b126440995d3ed5aaba0565d71e1834604819ff9c17f5e9d5dd078f")));
-		assert!(intro_node_blinding_point.is_none());
-		assert_eq!(payment_relay, PaymentRelay { cltv_expiry_delta: 48, fee_proportional_millionths: 100, fee_base_msat: 500 });
-		assert_eq!(features, BlindedHopFeatures::empty());
-		assert_eq!(payment_constraints, PaymentConstraints { max_cltv_expiry: 747_969, htlc_minimum_msat: 1500 });
-		(new_packet_bytes, next_hop_hmac)
-	} else { panic!() };
-
-	let dave_packet_details = next_packet_details_opt.unwrap();
-	let dave_onion = msgs::OnionPacket {
-		version: 0,
-		public_key: dave_packet_details.next_packet_pubkey,
-		hop_data: dave_packet_bytes,
-		hmac: dave_hmac,
-	};
-	let dave_update_add = update_add_msg(
-		dave_packet_details.outgoing_amt_msat, dave_packet_details.outgoing_cltv_value,
-		Some(pubkey_from_hex("031b84c5567b126440995d3ed5aaba0565d71e1834604819ff9c17f5e9d5dd078f")),
-		dave_onion
-	);
-	let dave_node_signer = TestEcdhSigner { node_secret: dave_secret };
-	let (dave_peeled_onion, next_packet_details_opt) =
-		match onion_payment::decode_incoming_update_add_htlc_onion(
-			&dave_update_add, &dave_node_signer, &logger, &secp_ctx
-		) {
-			Ok(res) => res,
-			_ => panic!("Unexpected error")
-		};
-	let (eve_packet_bytes, eve_hmac) = if let onion_utils::Hop::BlindedForward {
-		next_hop_data: msgs::InboundOnionBlindedForwardPayload {
-			short_channel_id, payment_relay, payment_constraints, features, intro_node_blinding_point, next_blinding_override
-		}, next_hop_hmac, new_packet_bytes, ..
-	} = dave_peeled_onion {
-		assert_eq!(short_channel_id, 561);
-		assert!(next_blinding_override.is_none());
-		assert!(intro_node_blinding_point.is_none());
-		assert_eq!(payment_relay, PaymentRelay { cltv_expiry_delta: 144, fee_proportional_millionths: 250, fee_base_msat: 0 });
-		assert_eq!(features, BlindedHopFeatures::empty());
-		assert_eq!(payment_constraints, PaymentConstraints { max_cltv_expiry: 747_921, htlc_minimum_msat: 1500 });
-		(new_packet_bytes, next_hop_hmac)
-	} else { panic!() };
-
-	let eve_packet_details = next_packet_details_opt.unwrap();
-	let eve_onion = msgs::OnionPacket {
-		version: 0,
-		public_key: eve_packet_details.next_packet_pubkey,
-		hop_data: eve_packet_bytes,
-		hmac: eve_hmac,
-	};
-	let eve_update_add = update_add_msg(
-		eve_packet_details.outgoing_amt_msat, eve_packet_details.outgoing_cltv_value,
-		Some(pubkey_from_hex("03e09038ee76e50f444b19abf0a555e8697e035f62937168b80adf0931b31ce52a")),
-		eve_onion
-	);
-	let eve_node_signer = TestEcdhSigner { node_secret: eve_secret };
-	// We can't decode the final payload because it contains a path_id and is missing some LDK
-	// specific fields.
+	// < MIN_CLTV_EXPIRY_DELTA). The vector includes an unknown odd encrypted TLV (type 561) in the
+	// blinded payload for Bob, which should now be rejected instead of forwarded.
 	match onion_payment::decode_incoming_update_add_htlc_onion(
-		&eve_update_add, &eve_node_signer, &logger, &secp_ctx
+		&bob_update_add, &bob_node_signer, &logger, &secp_ctx
 	) {
-		Err((HTLCFailureMsg::Malformed(msg), _)) => assert_eq!(msg.failure_code,
-			LocalHTLCFailureReason::InvalidOnionBlinding.failure_code()),
+		Err((HTLCFailureMsg::Relay(_), LocalHTLCFailureReason::InvalidOnionPayload)) => {},
 		_ => panic!("Unexpected error")
 	}
 }