Support (de)serializing payment_data in onion TLVs and track them

TheBlueMatt · TheBlueMatt · commit b54817397d62 · 2020-04-14T19:54:17.000-04:00
This is the first step in Base AMP support, just tracking the
relevant data in internal datastructures.
diff --git a/lightning/src/ln/channelmanager.rs b/lightning/src/ln/channelmanager.rs
@@ -74,7 +74,9 @@ enum PendingHTLCRouting {
 		onion_packet: msgs::OnionPacket,
 		short_channel_id: u64, // This should be NonZero<u64> eventually when we bump MSRV
 	},
-	Receive {},
+	Receive {
+		payment_data: Option<msgs::FinalOnionHopData>,
+	},
 }
 
 #[derive(Clone)] // See Channel::revoke_and_ack for why, tl;dr: Rust bug
@@ -119,6 +121,16 @@ pub(super) struct HTLCPreviousHopData {
 	incoming_packet_shared_secret: [u8; 32],
 }
 
+struct ClaimableHTLC {
+	prev_hop: HTLCPreviousHopData,
+	value: u64,
+	/// Filled in when the HTLC was received with a payment_secret packet, which contains a
+	/// total_msat (which may differ from value if this is a Multi-Path Payment) and a
+	/// payment_secret which prevents path-probing attacks and can associate different HTLCs which
+	/// are part of the same payment.
+	payment_data: Option<msgs::FinalOnionHopData>,
+}
+
 /// Tracks the inbound corresponding to an outbound HTLC
 #[derive(Clone, PartialEq)]
 pub(super) enum HTLCSource {
@@ -276,12 +288,11 @@ pub(super) struct ChannelHolder<ChanSigner: ChannelKeys> {
 	/// guarantees are made about the existence of a channel with the short id here, nor the short
 	/// ids in the PendingHTLCInfo!
 	pub(super) forward_htlcs: HashMap<u64, Vec<HTLCForwardInfo>>,
-	/// payment_hash -> Vec<(amount_received, htlc_source)> for tracking things that were to us and
-	/// can be failed/claimed by the user
+	/// Tracks HTLCs that were to us and can be failed/claimed by the user
 	/// Note that while this is held in the same mutex as the channels themselves, no consistency
 	/// guarantees are made about the channels given here actually existing anymore by the time you
 	/// go to read them!
-	pub(super) claimable_htlcs: HashMap<PaymentHash, Vec<(u64, HTLCPreviousHopData)>>,
+	claimable_htlcs: HashMap<PaymentHash, Vec<ClaimableHTLC>>,
 	/// Messages to send to peers - pushed to in the same lock that they are generated in (except
 	/// for broadcast messages, where ordering isn't as strict).
 	pub(super) pending_msg_events: Vec<events::MessageSendEvent>,
@@ -1007,13 +1018,19 @@ impl<ChanSigner: ChannelKeys, M: Deref, T: Deref, K: Deref, F: Deref> ChannelMan
 					return_err!("Upstream node set CLTV to the wrong value", 18, &byte_utils::be32_to_array(msg.cltv_expiry));
 				}
 
+				let payment_data = match next_hop_data.format {
+					msgs::OnionHopDataFormat::Legacy { .. } => None,
+					msgs::OnionHopDataFormat::NonFinalNode { .. } => return_err!("Got non final data with an HMAC of 0", 0x4000 | 22, &[0;0]),
+					msgs::OnionHopDataFormat::FinalNode { payment_data } => payment_data,
+				};
+
 				// Note that we could obviously respond immediately with an update_fulfill_htlc
 				// message, however that would leak that we are the recipient of this payment, so
 				// instead we stay symmetric with the forwarding case, only responding (after a
 				// delay) once they've send us a commitment_signed!
 
 				PendingHTLCStatus::Forward(PendingHTLCInfo {
-					routing: PendingHTLCRouting::Receive {},
+					routing: PendingHTLCRouting::Receive { payment_data },
 					payment_hash: msg.payment_hash.clone(),
 					incoming_shared_secret: shared_secret,
 					amt_to_forward: next_hop_data.amt_to_forward,
@@ -1580,17 +1597,18 @@ impl<ChanSigner: ChannelKeys, M: Deref, T: Deref, K: Deref, F: Deref> ChannelMan
 					for forward_info in pending_forwards.drain(..) {
 						match forward_info {
 							HTLCForwardInfo::AddHTLC { prev_short_channel_id, prev_htlc_id, forward_info: PendingHTLCInfo {
-									routing: PendingHTLCRouting::Receive { },
+									routing: PendingHTLCRouting::Receive { payment_data },
 									incoming_shared_secret, payment_hash, amt_to_forward, .. }, } => {
-								let prev_hop_data = HTLCPreviousHopData {
+								let prev_hop = HTLCPreviousHopData {
 									short_channel_id: prev_short_channel_id,
 									htlc_id: prev_htlc_id,
 									incoming_packet_shared_secret: incoming_shared_secret,
 								};
-								match channel_state.claimable_htlcs.entry(payment_hash) {
-									hash_map::Entry::Occupied(mut entry) => entry.get_mut().push((amt_to_forward, prev_hop_data)),
-									hash_map::Entry::Vacant(entry) => { entry.insert(vec![(amt_to_forward, prev_hop_data)]); },
-								};
+								channel_state.claimable_htlcs.entry(payment_hash).or_insert(Vec::new()).push(ClaimableHTLC {
+									prev_hop,
+									value: amt_to_forward,
+									payment_data,
+								});
 								new_events.push(events::Event::PaymentReceived {
 									payment_hash: payment_hash,
 									amt: amt_to_forward,
@@ -1660,11 +1678,11 @@ impl<ChanSigner: ChannelKeys, M: Deref, T: Deref, K: Deref, F: Deref> ChannelMan
 		let mut channel_state = Some(self.channel_state.lock().unwrap());
 		let removed_source = channel_state.as_mut().unwrap().claimable_htlcs.remove(payment_hash);
 		if let Some(mut sources) = removed_source {
-			for (recvd_value, htlc_with_hash) in sources.drain(..) {
+			for htlc in sources.drain(..) {
 				if channel_state.is_none() { channel_state = Some(self.channel_state.lock().unwrap()); }
 				self.fail_htlc_backwards_internal(channel_state.take().unwrap(),
-						HTLCSource::PreviousHopData(htlc_with_hash), payment_hash,
-						HTLCFailReason::Reason { failure_code: 0x4000 | 15, data: byte_utils::be64_to_array(recvd_value).to_vec() });
+						HTLCSource::PreviousHopData(htlc.prev_hop), payment_hash,
+						HTLCFailReason::Reason { failure_code: 0x4000 | 15, data: byte_utils::be64_to_array(htlc.value).to_vec() });
 			}
 			true
 		} else { false }
@@ -1788,17 +1806,17 @@ impl<ChanSigner: ChannelKeys, M: Deref, T: Deref, K: Deref, F: Deref> ChannelMan
 		let mut channel_state = Some(self.channel_state.lock().unwrap());
 		let removed_source = channel_state.as_mut().unwrap().claimable_htlcs.remove(&payment_hash);
 		if let Some(mut sources) = removed_source {
-			for (received_amount, htlc_with_hash) in sources.drain(..) {
+			for htlc in sources.drain(..) {
 				if channel_state.is_none() { channel_state = Some(self.channel_state.lock().unwrap()); }
-				if received_amount < expected_amount || received_amount > expected_amount * 2 {
-					let mut htlc_msat_data = byte_utils::be64_to_array(received_amount).to_vec();
+				if htlc.value < expected_amount || htlc.value > expected_amount * 2 {
+					let mut htlc_msat_data = byte_utils::be64_to_array(htlc.value).to_vec();
 					let mut height_data = byte_utils::be32_to_array(self.latest_block_height.load(Ordering::Acquire) as u32).to_vec();
 					htlc_msat_data.append(&mut height_data);
 					self.fail_htlc_backwards_internal(channel_state.take().unwrap(),
-									 HTLCSource::PreviousHopData(htlc_with_hash), &payment_hash,
+									 HTLCSource::PreviousHopData(htlc.prev_hop), &payment_hash,
 									 HTLCFailReason::Reason { failure_code: 0x4000|15, data: htlc_msat_data });
 				} else {
-					self.claim_funds_internal(channel_state.take().unwrap(), HTLCSource::PreviousHopData(htlc_with_hash), payment_preimage);
+					self.claim_funds_internal(channel_state.take().unwrap(), HTLCSource::PreviousHopData(htlc.prev_hop), payment_preimage);
 				}
 			}
 			true
@@ -3098,8 +3116,9 @@ impl Writeable for PendingHTLCInfo {
 				onion_packet.write(writer)?;
 				short_channel_id.write(writer)?;
 			},
-			&PendingHTLCRouting::Receive { } => {
+			&PendingHTLCRouting::Receive { ref payment_data } => {
 				1u8.write(writer)?;
+				payment_data.write(writer)?;
 			},
 		}
 		self.incoming_shared_secret.write(writer)?;
@@ -3118,7 +3137,9 @@ impl Readable for PendingHTLCInfo {
 					onion_packet: Readable::read(reader)?,
 					short_channel_id: Readable::read(reader)?,
 				},
-				1u8 => PendingHTLCRouting::Receive { },
+				1u8 => PendingHTLCRouting::Receive {
+					payment_data: Readable::read(reader)?,
+				},
 				_ => return Err(DecodeError::InvalidValue),
 			},
 			incoming_shared_secret: Readable::read(reader)?,
@@ -3187,6 +3208,12 @@ impl_writeable!(HTLCPreviousHopData, 0, {
 	incoming_packet_shared_secret
 });
 
+impl_writeable!(ClaimableHTLC, 0, {
+	prev_hop,
+	value,
+	payment_data
+});
+
 impl Writeable for HTLCSource {
 	fn write<W: Writer>(&self, writer: &mut W) -> Result<(), ::std::io::Error> {
 		match self {
@@ -3328,9 +3355,8 @@ impl<ChanSigner: ChannelKeys + Writeable, M: Deref, T: Deref, K: Deref, F: Deref
 		for (payment_hash, previous_hops) in channel_state.claimable_htlcs.iter() {
 			payment_hash.write(writer)?;
 			(previous_hops.len() as u64).write(writer)?;
-			for &(recvd_amt, ref previous_hop) in previous_hops.iter() {
-				recvd_amt.write(writer)?;
-				previous_hop.write(writer)?;
+			for htlc in previous_hops.iter() {
+				htlc.write(writer)?;
 			}
 		}
 
@@ -3507,7 +3533,7 @@ impl<'a, ChanSigner: ChannelKeys + Readable, M: Deref, T: Deref, K: Deref, F: De
 			let previous_hops_len: u64 = Readable::read(reader)?;
 			let mut previous_hops = Vec::with_capacity(cmp::min(previous_hops_len as usize, 2));
 			for _ in 0..previous_hops_len {
-				previous_hops.push((Readable::read(reader)?, Readable::read(reader)?));
+				previous_hops.push(Readable::read(reader)?);
 			}
 			claimable_htlcs.insert(payment_hash, previous_hops);
 		}
diff --git a/lightning/src/ln/msgs.rs b/lightning/src/ln/msgs.rs
@@ -614,6 +614,11 @@ pub trait RoutingMessageHandler : Send + Sync {
 mod fuzzy_internal_msgs {
 	// These types aren't intended to be pub, but are exposed for direct fuzzing (as we deserialize
 	// them from untrusted input):
+	#[derive(Clone)]
+	pub(crate) struct FinalOnionHopData {
+		pub(crate) payment_secret: [u8; 32],
+		pub(crate) total_msat: u64,
+	}
 
 	pub(crate) enum OnionHopDataFormat {
 		Legacy { // aka Realm-0
@@ -622,7 +627,9 @@ mod fuzzy_internal_msgs {
 		NonFinalNode {
 			short_channel_id: u64,
 		},
-		FinalNode,
+		FinalNode {
+			payment_data: Option<FinalOnionHopData>,
+		},
 	}
 
 	pub struct OnionHopData {
@@ -965,6 +972,22 @@ impl_writeable!(UpdateAddHTLC, 32+8+8+32+4+1366, {
 	onion_routing_packet
 });
 
+impl Writeable for FinalOnionHopData {
+	fn write<W: Writer>(&self, w: &mut W) -> Result<(), ::std::io::Error> {
+		w.size_hint(32 + 8 - (self.total_msat.leading_zeros()/8) as usize);
+		self.payment_secret.write(w)?;
+		HighZeroBytesDroppedVarInt(self.total_msat).write(w)
+	}
+}
+
+impl Readable for FinalOnionHopData {
+	fn read<R: Read>(r: &mut R) -> Result<Self, DecodeError> {
+		let payment_secret = Readable::read(r)?;
+		let amt: HighZeroBytesDroppedVarInt<u64> = Readable::read(r)?;
+		Ok(Self { payment_secret, total_msat: amt.0 })
+	}
+}
+
 impl Writeable for OnionHopData {
 	fn write<W: Writer>(&self, w: &mut W) -> Result<(), ::std::io::Error> {
 		w.size_hint(33);
@@ -983,7 +1006,14 @@ impl Writeable for OnionHopData {
 					(6, short_channel_id)
 				});
 			},
-			OnionHopDataFormat::FinalNode => {
+			OnionHopDataFormat::FinalNode { payment_data: Some(ref final_data) } => {
+				encode_varint_length_prefixed_tlv!(w, {
+					(2, HighZeroBytesDroppedVarInt(self.amt_to_forward)),
+					(4, HighZeroBytesDroppedVarInt(self.outgoing_cltv_value)),
+					(8, final_data)
+				});
+			},
+			OnionHopDataFormat::FinalNode { payment_data: None } => {
 				encode_varint_length_prefixed_tlv!(w, {
 					(2, HighZeroBytesDroppedVarInt(self.amt_to_forward)),
 					(4, HighZeroBytesDroppedVarInt(self.outgoing_cltv_value))
@@ -1008,19 +1038,24 @@ impl Readable for OnionHopData {
 			let mut amt = HighZeroBytesDroppedVarInt(0u64);
 			let mut cltv_value = HighZeroBytesDroppedVarInt(0u32);
 			let mut short_id: Option<u64> = None;
+			let mut payment_data: Option<FinalOnionHopData> = None;
 			decode_tlv!(&mut rd, {
 				(2, amt),
 				(4, cltv_value)
 			}, {
-				(6, short_id)
+				(6, short_id),
+				(8, payment_data)
 			});
 			rd.eat_remaining().map_err(|_| DecodeError::ShortRead)?;
 			let format = if let Some(short_channel_id) = short_id {
+				if payment_data.is_some() { return Err(DecodeError::InvalidValue); }
 				OnionHopDataFormat::NonFinalNode {
 					short_channel_id,
 				}
 			} else {
-				OnionHopDataFormat::FinalNode
+				OnionHopDataFormat::FinalNode {
+					payment_data
+				}
 			};
 			(format, amt.0, cltv_value.0)
 		} else {
@@ -1305,7 +1340,7 @@ impl_writeable_len_match!(NodeAnnouncement, {
 mod tests {
 	use hex;
 	use ln::msgs;
-	use ln::msgs::{ChannelFeatures, InitFeatures, NodeFeatures, OptionalField, OnionErrorPacket, OnionHopDataFormat};
+	use ln::msgs::{ChannelFeatures, FinalOnionHopData, InitFeatures, NodeFeatures, OptionalField, OnionErrorPacket, OnionHopDataFormat};
 	use ln::channelmanager::{PaymentPreimage, PaymentHash};
 	use util::ser::{Writeable, Readable};
 
@@ -1998,15 +2033,46 @@ mod tests {
 	#[test]
 	fn encoding_final_onion_hop_data() {
 		let mut msg = msgs::OnionHopData {
-			format: OnionHopDataFormat::FinalNode,
+			format: OnionHopDataFormat::FinalNode {
+				payment_data: None,
+			},
 			amt_to_forward: 0x0badf00d01020304,
 			outgoing_cltv_value: 0xffffffff,
 		};
 		let encoded_value = msg.encode();
 		let target_value = hex::decode("1002080badf00d010203040404ffffffff").unwrap();
 		assert_eq!(encoded_value, target_value);
 		msg = Readable::read(&mut Cursor::new(&target_value[..])).unwrap();
-		if let OnionHopDataFormat::FinalNode = msg.format { } else { panic!(); }
+		if let OnionHopDataFormat::FinalNode { payment_data: None } = msg.format { } else { panic!(); }
+		assert_eq!(msg.amt_to_forward, 0x0badf00d01020304);
+		assert_eq!(msg.outgoing_cltv_value, 0xffffffff);
+	}
+
+	#[test]
+	fn encoding_final_onion_hop_data_with_secret() {
+		let expected_payment_secret = [0x42u8; 32];
+		let mut msg = msgs::OnionHopData {
+			format: OnionHopDataFormat::FinalNode {
+				payment_data: Some(FinalOnionHopData {
+					payment_secret: expected_payment_secret,
+					total_msat: 0x1badca1f
+				}),
+			},
+			amt_to_forward: 0x0badf00d01020304,
+			outgoing_cltv_value: 0xffffffff,
+		};
+		let encoded_value = msg.encode();
+		let target_value = hex::decode("3602080badf00d010203040404ffffffff082442424242424242424242424242424242424242424242424242424242424242421badca1f").unwrap();
+		assert_eq!(encoded_value, target_value);
+		msg = Readable::read(&mut Cursor::new(&target_value[..])).unwrap();
+		if let OnionHopDataFormat::FinalNode {
+			payment_data: Some(FinalOnionHopData {
+				payment_secret,
+				total_msat: 0x1badca1f
+			})
+		} = msg.format {
+			assert_eq!(payment_secret, expected_payment_secret);
+		} else { panic!(); }
 		assert_eq!(msg.amt_to_forward, 0x0badf00d01020304);
 		assert_eq!(msg.outgoing_cltv_value, 0xffffffff);
 	}
diff --git a/lightning/src/ln/onion_utils.rs b/lightning/src/ln/onion_utils.rs
@@ -123,7 +123,9 @@ pub(super) fn build_onion_payloads(route: &Route, starting_htlc_offset: u32) ->
 		res.insert(0, msgs::OnionHopData {
 			format: if hop.node_features.supports_variable_length_onion() {
 				if idx == 0 {
-					msgs::OnionHopDataFormat::FinalNode
+					msgs::OnionHopDataFormat::FinalNode {
+						payment_data: None,
+					}
 				} else {
 					msgs::OnionHopDataFormat::NonFinalNode {
 						short_channel_id: last_short_channel_id,
