Correctly fail back on intro node payload decode failure

Author: valentinewallace

fuzz/src/router.rs

@@ -157,6 +157,7 @@ pub fn do_test<Out: test_logger::Output>(data: &[u8], out: Out) {
 					msgs::DecodeError::ShortRead => panic!("We picked the length..."),
 					msgs::DecodeError::Io(e) => panic!("{:?}", e),
 					msgs::DecodeError::UnsupportedCompression => return,
+					msgs::DecodeError::InvalidIntroNodePayload => unreachable!(),
 				}
 			}
 		}}

lightning/src/ln/blinded_payment_tests.rs

@@ -8,7 +8,7 @@
 // licenses.
 
 use bitcoin::secp256k1::Secp256k1;
-use crate::blinded_path::BlindedPath;
+use crate::blinded_path::{BlindedHop, BlindedPath};
 use crate::blinded_path::payment::{ForwardTlvs, PaymentConstraints, PaymentRelay, ReceiveTlvs};
 use crate::events::{Event, HTLCDestination, MessageSendEvent, MessageSendEventsProvider};
 use crate::ln::channelmanager;
@@ -801,3 +801,76 @@ fn do_outbound_checks_failure(intro_node_fails: bool) {
 	expect_payment_failed_conditions(&nodes[0], payment_hash, false,
 		PaymentFailedConditions::new().expected_htlc_error_data(INVALID_ONION_BLINDING, &[0; 32]));
 }
+
+#[test]
+fn invalid_intro_node_payload() {
+	// Ensure we fail back properly if the intro node's onion payload is bogus.
+	let chanmon_cfgs = create_chanmon_cfgs(3);
+	let node_cfgs = create_node_cfgs(3, &chanmon_cfgs);
+	let node_chanmgrs = create_node_chanmgrs(3, &node_cfgs, &[None, None, None]);
+	let nodes = create_network(3, &node_cfgs, &node_chanmgrs);
+	create_announced_chan_between_nodes_with_value(&nodes, 0, 1, 1_000_000, 0);
+	let chan_upd = create_announced_chan_between_nodes_with_value(&nodes, 1, 2, 1_000_000, 0).0.contents;
+
+	let amt_msat = 5000;
+	let (_, payment_hash, payment_secret) = get_payment_preimage_hash(&nodes[2], Some(amt_msat), None);
+	let intermediate_nodes = vec![(nodes[1].node.get_our_node_id(), ForwardTlvs {
+		short_channel_id: chan_upd.short_channel_id,
+		payment_relay: PaymentRelay {
+			cltv_expiry_delta: chan_upd.cltv_expiry_delta,
+			fee_proportional_millionths: chan_upd.fee_proportional_millionths,
+			fee_base_msat: chan_upd.fee_base_msat,
+		},
+		payment_constraints: PaymentConstraints {
+			max_cltv_expiry: u32::max_value(),
+			htlc_minimum_msat: chan_upd.htlc_minimum_msat,
+		},
+		features: BlindedHopFeatures::empty(),
+	})];
+	let payee_tlvs = ReceiveTlvs {
+		payment_secret,
+		payment_constraints: PaymentConstraints {
+			max_cltv_expiry: u32::max_value(),
+			htlc_minimum_msat: chan_upd.htlc_minimum_msat,
+		},
+	};
+	let mut secp_ctx = Secp256k1::new();
+	let mut blinded_path = BlindedPath::new_for_payment(
+		&intermediate_nodes[..], nodes[2].node.get_our_node_id(), payee_tlvs,
+		chan_upd.htlc_maximum_msat, &chanmon_cfgs[2].keys_manager, &secp_ctx
+	).unwrap();
+	blinded_path.1.blinded_hops[0] = BlindedHop {
+		blinded_node_id: blinded_path.1.blinded_hops[0].blinded_node_id,
+		encrypted_payload: vec![0; 32], // Bogus intro node payload
+	};
+
+	let route_params = RouteParameters {
+		payment_params: PaymentParameters::blinded(vec![blinded_path]),
+		final_value_msat: amt_msat
+	};
+	nodes[0].node.send_payment(payment_hash, RecipientOnionFields::spontaneous_empty(),
+	PaymentId(payment_hash.0), route_params, Retry::Attempts(0)).unwrap();
+	check_added_monitors(&nodes[0], 1);
+	let (update_add, commitment_signed) = {
+		let mut events = nodes[0].node.get_and_clear_pending_msg_events();
+		assert_eq!(events.len(), 1);
+		let payment_ev = SendEvent::from_event(events.pop().unwrap());
+		(payment_ev.msgs[0].clone(), payment_ev.commitment_msg.clone())
+	};
+	nodes[1].node.handle_update_add_htlc(&nodes[0].node.get_our_node_id(), &update_add);
+	do_commitment_signed_dance(&nodes[1], &nodes[0], &commitment_signed, false, true);
+	let (update_fail, commitment_signed) = {
+		let events = nodes[1].node.get_and_clear_pending_msg_events();
+		assert_eq!(events.len(), 1);
+		match &events[0] {
+			MessageSendEvent::UpdateHTLCs { updates, .. } => {
+				(updates.update_fail_htlcs[0].clone(), updates.commitment_signed.clone())
+			},
+			_ => panic!()
+		}
+	};
+	nodes[0].node.handle_update_fail_htlc(&nodes[1].node.get_our_node_id(), &update_fail);
+	do_commitment_signed_dance(&nodes[0], &nodes[1], &commitment_signed, false, true);
+	expect_payment_failed_conditions(&nodes[0], payment_hash, false,
+		PaymentFailedConditions::new().expected_htlc_error_data(INVALID_ONION_BLINDING, &[0; 32]));
+}

lightning/src/ln/channelmanager.rs

@@ -2979,7 +2979,7 @@ where
 					return Err(HTLCFailureMsg::Relay(msgs::UpdateFailHTLC {
 						channel_id: msg.channel_id,
 						htlc_id: msg.htlc_id,
-						reason: HTLCFailReason::reason($err_code, $data.to_vec())
+						reason: HTLCFailReason::reason($err_code, $data)
 							.get_encrypted_failure_packet(&shared_secret, &None),
 					}));
 				}
@@ -2990,7 +2990,7 @@ where
 				if msg.blinding_point.is_some() {
 					return_malformed_err!($msg, INVALID_ONION_BLINDING);
 				} else {
-					return_err!($msg, INVALID_ONION_BLINDING, [0; 32]);
+					return_err!($msg, INVALID_ONION_BLINDING, vec![0; 32]);
 				}
 			}
 		}
@@ -3007,11 +3007,11 @@ where
 					return_malformed_err!(err_msg, err_code);
 				}
 			},
-			Err(onion_utils::OnionDecodeErr::Relay { err_msg, err_code }) => {
+			Err(onion_utils::OnionDecodeErr::Relay { err_msg, err_code, err_data }) => {
 				if msg.blinding_point.is_some() {
 					return_blinded_htlc_err!(err_msg);
 				} else {
-					return_err!(err_msg, err_code, &[0; 0]);
+					return_err!(err_msg, err_code, err_data);
 				}
 			},
 		};
@@ -3059,7 +3059,8 @@ where
 			// inbound channel's state.
 			onion_utils::Hop::Receive { .. } => return Ok((next_hop, shared_secret, None)),
 			onion_utils::Hop::Forward { next_hop_data: msgs::InboundOnionPayload::Receive { .. }, .. } => {
-				return_err!("Final Node OnionHopData provided for us as an intermediary node", 0x4000 | 22, &[0; 0]);
+				return_err!("Final Node OnionHopData provided for us as an intermediary node",
+					0x4000 | 22, Vec::new());
 			},
 			onion_utils::Hop::Forward {
 				next_hop_data: msgs::InboundOnionPayload::BlindedReceive { .. }, ..
@@ -3200,7 +3201,7 @@ where
 				// instead.
 				code = 0x2000 | 2;
 			}
-			return_err!(err, code, &res.0[..]);
+			return_err!(err, code, res.0);
 		}
 		Ok((next_hop, shared_secret, next_packet_pk_opt))
 	}
@@ -4057,8 +4058,8 @@ where
 														// of the onion.
 														failed_payment!(err_msg, err_code, sha256_of_onion.to_vec(), None);
 													},
-													Err(onion_utils::OnionDecodeErr::Relay { err_msg, err_code }) => {
-														failed_payment!(err_msg, err_code, Vec::new(), Some(phantom_shared_secret));
+													Err(onion_utils::OnionDecodeErr::Relay { err_msg, err_code, err_data }) => {
+														failed_payment!(err_msg, err_code, err_data, Some(phantom_shared_secret));
 													},
 												};
 												match next_hop {

lightning/src/ln/msgs.rs

@@ -83,6 +83,9 @@ pub enum DecodeError {
 	Io(io::ErrorKind),
 	/// The message included zlib-compressed values, which we don't support.
 	UnsupportedCompression,
+	/// We are the intro node to a blinded path and failed to decode our onion payload. Separated out
+	/// from [`Self::InvalidValue`] to ensure we abide by the spec when failing backwards.
+	InvalidIntroNodePayload,
 }
 
 /// An [`init`] message to be sent to or received from a peer.
@@ -1570,6 +1573,7 @@ impl fmt::Display for DecodeError {
 			DecodeError::BadLengthDescriptor => f.write_str("A length descriptor in the packet didn't describe the later data correctly"),
 			DecodeError::Io(ref e) => fmt::Debug::fmt(e, f),
 			DecodeError::UnsupportedCompression => f.write_str("We don't support receiving messages with zlib-compressed fields"),
+			DecodeError::InvalidIntroNodePayload => f.write_str("Failed to decode blinded payment intro node onion payload"),
 		}
 	}
 }
@@ -2058,8 +2062,6 @@ impl Writeable for OutboundOnionPayload {
 
 impl<NS: Deref> ReadableArgs<(Option<PublicKey>, &NS)> for InboundOnionPayload where NS::Target: NodeSigner {
 	fn read<R: Read>(r: &mut R, args: (Option<PublicKey>, &NS)) -> Result<Self, DecodeError> {
-		// TODO if we are the intro node and hit an error here, we won't correctly return the malformed
-		// error atm
 		let (update_add_blinding_point, node_signer) = args;
 		let mut amt = None;
 		let mut cltv_value = None;
@@ -2093,27 +2095,35 @@ impl<NS: Deref> ReadableArgs<(Option<PublicKey>, &NS)> for InboundOnionPayload w
 			Ok(true)
 		});
 
+		let err = intro_node_blinding_point.map_or(DecodeError::InvalidValue,
+			|_| { DecodeError::InvalidIntroNodePayload });
 		if update_add_blinding_point.is_some() && intro_node_blinding_point.is_some() {
-			return Err(DecodeError::InvalidValue)
+			// This should be unreachable because we would've attempted to use the
+			// update_add_blinding_point to tweak to our onion packet pubkey and subsequently failed the
+			// HMAC check.
+			debug_assert!(false);
+			return Err(err)
 		}
-		if amt.unwrap_or(0) > MAX_VALUE_MSAT { return Err(DecodeError::InvalidValue) }
+		if amt.unwrap_or(0) > MAX_VALUE_MSAT { return Err(err) }
 
 		if let Some(blinding_point) = update_add_blinding_point.or(intro_node_blinding_point) {
 			if short_id.is_some() || payment_data.is_some() || payment_metadata.is_some() {
-				return Err(DecodeError::InvalidValue)
+				return Err(err)
 			}
-			let enc_tlvs = encrypted_tlvs_opt.ok_or(DecodeError::InvalidValue)?.0;
+			let enc_tlvs = encrypted_tlvs_opt.ok_or_else(|| err.clone())?.0;
 			let enc_tlvs_ss = node_signer.ecdh(Recipient::Node, &blinding_point, None)
-				.map_err(|_| DecodeError::InvalidValue)?;
+				.map_err(|_| err.clone())?;
 			let rho = onion_utils::gen_rho_from_shared_secret(&enc_tlvs_ss.secret_bytes());
 			let mut s = Cursor::new(&enc_tlvs);
 			let mut reader = FixedLengthReader::new(&mut s, enc_tlvs.len() as u64);
-			match ChaChaPolyReadAdapter::read(&mut reader, rho)? {
+			let decoded_blinded_tlvs = ChaChaPolyReadAdapter::read(&mut reader, rho).map_err(|e|
+				if intro_node_blinding_point.is_some() { DecodeError::InvalidIntroNodePayload } else { e })?;
+			match decoded_blinded_tlvs {
 				ChaChaPolyReadAdapter { readable: BlindedPaymentTlvs::Forward(ForwardTlvs {
 					short_channel_id, payment_relay, payment_constraints, features
 				})} => {
 					if amt.is_some() || cltv_value.is_some() || total_msat.is_some() {
-						return Err(DecodeError::InvalidValue)
+						return Err(err)
 					}
 					Ok(Self::BlindedForward {
 						short_channel_id,
@@ -2126,6 +2136,8 @@ impl<NS: Deref> ReadableArgs<(Option<PublicKey>, &NS)> for InboundOnionPayload w
 				ChaChaPolyReadAdapter { readable: BlindedPaymentTlvs::Receive(ReceiveTlvs {
 					payment_secret, payment_constraints
 				})} => {
+					// Per the spec, if we're the intro node and the final node we should error as if we're
+					// not part of a blinded path.
 					if total_msat.unwrap_or(0) > MAX_VALUE_MSAT { return Err(DecodeError::InvalidValue) }
 					Ok(Self::BlindedReceive {
 						amt_msat: amt.ok_or(DecodeError::InvalidValue)?,

lightning/src/ln/onion_utils.rs

@@ -894,6 +894,7 @@ pub(crate) enum OnionDecodeErr {
 	Relay {
 		err_msg: &'static str,
 		err_code: u16,
+		err_data: Vec<u8>,
 	},
 }
 
@@ -936,16 +937,16 @@ fn decode_next_hop<T, R: ReadableArgs<T>, N: NextPacketBytes>(shared_secret: [u8
 	let mut chacha_stream = ChaChaReader { chacha: &mut chacha, read: Cursor::new(&hop_data[..]) };
 	match R::read(&mut chacha_stream, read_args) {
 		Err(err) => {
-			let error_code = match err {
-				msgs::DecodeError::UnknownVersion => 0x4000 | 1, // unknown realm byte
+			let (err_code, err_data) = match err {
+				msgs::DecodeError::UnknownVersion => (0x4000 | 1, Vec::new()), // unknown realm byte
 				msgs::DecodeError::UnknownRequiredFeature|
 				msgs::DecodeError::InvalidValue|
-				msgs::DecodeError::ShortRead => 0x4000 | 22, // invalid_onion_payload
-				_ => 0x2000 | 2, // Should never happen
+				msgs::DecodeError::ShortRead => (0x4000 | 22, Vec::new()), // invalid_onion_payload
+				msgs::DecodeError::InvalidIntroNodePayload => (INVALID_ONION_BLINDING, vec![0; 32]),
+				_ => (0x2000 | 2, Vec::new()), // Should never happen
 			};
 			return Err(OnionDecodeErr::Relay {
-				err_msg: "Unable to decode our hop data",
-				err_code: error_code,
+				err_msg: "Unable to decode our hop data", err_code, err_data,
 			});
 		},
 		Ok(msg) => {
@@ -954,6 +955,7 @@ fn decode_next_hop<T, R: ReadableArgs<T>, N: NextPacketBytes>(shared_secret: [u8
 				return Err(OnionDecodeErr::Relay {
 					err_msg: "Unable to decode our hop data",
 					err_code: 0x4000 | 22,
+					err_data: Vec::new(),
 				});
 			}
 			if hmac == [0; 32] {

lightning/src/ln/peer_handler.rs

@@ -1447,6 +1447,10 @@ impl<Descriptor: SocketDescriptor, CM: Deref, RM: Deref, OM: Deref, L: Deref, CM
 												}
 												(msgs::DecodeError::BadLengthDescriptor, _) => return Err(PeerHandleError { }),
 												(msgs::DecodeError::Io(_), _) => return Err(PeerHandleError { }),
+												(msgs::DecodeError::InvalidIntroNodePayload, _) => {
+													debug_assert!(false); // This error can only be hit when decoding an onion payload
+													return Err(PeerHandleError { })
+												},
 											}
 										}
 									};