Req+check payment secrets for inbound payments pre-PaymentReceived

TheBlueMatt · TheBlueMatt · commit fedd89d0dbeb · 2021-04-23T17:09:53.000Z
Our current PaymentReceived API is incredibly easy to mis-use -
the "obvious" way to implement a client is to always call
`ChannelManager::claim_funds` in response to a `PaymentReceived`
event. However, users are *required* to check the payment secret
and value against the expected values before claiming in order to
avoid a number of potentially funds-losing attacks.

Instead, if we rely on payment secrets being pre-registered with
the ChannelManager before we receive HTLCs for a payment we can
simply check the payment secrets and never generate
`PaymentReceived` events if they do not match. Further, when the
user knows the value to expect in advance, we can have them
register it as well, allowing us to check it for them.

Other implementations already require payment secrets for inbound
payments, so this shouldn't materially lose compatibility.
diff --git a/lightning/src/ln/channelmanager.rs b/lightning/src/ln/channelmanager.rs
@@ -95,7 +95,7 @@ enum PendingHTLCRouting {
 		short_channel_id: u64, // This should be NonZero<u64> eventually when we bump MSRV
 	},
 	Receive {
-		payment_data: Option<msgs::FinalOnionHopData>,
+		payment_data: msgs::FinalOnionHopData,
 		incoming_cltv_expiry: u32, // Used to track when we should expire pending HTLCs that go unclaimed
 	},
 }
@@ -159,7 +159,7 @@ struct ClaimableHTLC {
 	/// 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>,
+	payment_data: msgs::FinalOnionHopData,
 	cltv_expiry: u32,
 }
 
@@ -331,7 +331,7 @@ pub(super) struct ChannelHolder<Signer: Sign> {
 	/// 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!
-	claimable_htlcs: HashMap<(PaymentHash, Option<PaymentSecret>), Vec<ClaimableHTLC>>,
+	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<MessageSendEvent>,
@@ -1237,14 +1237,18 @@ impl<Signer: Sign, M: Deref, T: Deref, K: Deref, F: Deref, L: Deref> ChannelMana
 					msgs::OnionHopDataFormat::FinalNode { payment_data } => payment_data,
 				};
 
+				if payment_data.is_none() {
+					return_err!("We require payment_secrets", 0x4000|0x2000|3, &[0;0]);
+				}
+
 				// 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 {
-						payment_data,
+						payment_data: payment_data.unwrap(),
 						incoming_cltv_expiry: msg.cltv_expiry,
 					},
 					payment_hash: msg.payment_hash.clone(),
@@ -1923,60 +1927,74 @@ impl<Signer: Sign, M: Deref, T: Deref, K: Deref, F: Deref, L: Deref> ChannelMana
 									routing: PendingHTLCRouting::Receive { payment_data, incoming_cltv_expiry },
 									incoming_shared_secret, payment_hash, amt_to_forward, .. },
 									prev_funding_outpoint } => {
-								let prev_hop = HTLCPreviousHopData {
-									short_channel_id: prev_short_channel_id,
-									outpoint: prev_funding_outpoint,
-									htlc_id: prev_htlc_id,
-									incoming_packet_shared_secret: incoming_shared_secret,
-								};
-
-								let mut total_value = 0;
-								let payment_secret_opt =
-									if let &Some(ref data) = &payment_data { Some(data.payment_secret.clone()) } else { None };
-								let htlcs = channel_state.claimable_htlcs.entry((payment_hash, payment_secret_opt))
-									.or_insert(Vec::new());
-								htlcs.push(ClaimableHTLC {
-									prev_hop,
+								let claimable_htlc = ClaimableHTLC {
+									prev_hop: HTLCPreviousHopData {
+										short_channel_id: prev_short_channel_id,
+										outpoint: prev_funding_outpoint,
+										htlc_id: prev_htlc_id,
+										incoming_packet_shared_secret: incoming_shared_secret,
+									},
 									value: amt_to_forward,
 									payment_data: payment_data.clone(),
 									cltv_expiry: incoming_cltv_expiry,
-								});
-								if let &Some(ref data) = &payment_data {
+								};
+
+								macro_rules! fail_htlc {
+									($htlc: expr) => {
+										let mut htlc_msat_height_data = byte_utils::be64_to_array($htlc.value).to_vec();
+										htlc_msat_height_data.extend_from_slice(
+											&byte_utils::be32_to_array(self.best_block.read().unwrap().height()),
+										);
+										failed_forwards.push((HTLCSource::PreviousHopData(HTLCPreviousHopData {
+												short_channel_id: $htlc.prev_hop.short_channel_id,
+												outpoint: prev_funding_outpoint,
+												htlc_id: $htlc.prev_hop.htlc_id,
+												incoming_packet_shared_secret: $htlc.prev_hop.incoming_packet_shared_secret,
+											}), payment_hash,
+											HTLCFailReason::Reason { failure_code: 0x4000 | 15, data: htlc_msat_height_data }
+										));
+									}
+								}
+
+								// Check that the payment hash and secret are known. Note that we
+								// MUST take care to handle the "unknown payment hash" and
+								// "incorrect payment secret" cases here identically or we'd expose
+								// that we are the ultimately recipient of the given payment hash.
+								// Further, we must not expose whether we have any other HTLCs
+								// associated with the same payment_hash pending or not.
+								if {
+									let payment_secrets = self.pending_inbound_payments.lock().unwrap();
+									if let Some(inbound_payment) = payment_secrets.get(&payment_hash) {
+										inbound_payment.payment_secret != payment_data.payment_secret ||
+										(inbound_payment.min_value_msat.is_some() && inbound_payment.min_value_msat.unwrap() > payment_data.total_msat)
+									} else { true }
+								} {
+									log_trace!(self.logger, "Failing new HTLC with payment_hash {} as it didn't match our expected payment secret.", log_bytes!(payment_hash.0));
+									fail_htlc!(claimable_htlc);
+								} else {
+									let mut total_value = 0;
+									let htlcs = channel_state.claimable_htlcs.entry(payment_hash)
+										.or_insert(Vec::new());
+									htlcs.push(claimable_htlc);
 									for htlc in htlcs.iter() {
 										total_value += htlc.value;
-										if htlc.payment_data.as_ref().unwrap().total_msat != data.total_msat {
+										if htlc.payment_data.total_msat != payment_data.total_msat {
 											total_value = msgs::MAX_VALUE_MSAT;
 										}
 										if total_value >= msgs::MAX_VALUE_MSAT { break; }
 									}
-									if total_value >= msgs::MAX_VALUE_MSAT || total_value > data.total_msat  {
+									if total_value >= msgs::MAX_VALUE_MSAT || total_value > payment_data.total_msat {
+										log_trace!(self.logger, "Failing HTLCs with payment_hash {} as the total value {} ran over expected value {}", log_bytes!(payment_hash.0), total_value, payment_data.total_msat);
 										for htlc in htlcs.iter() {
-											let mut htlc_msat_height_data = byte_utils::be64_to_array(htlc.value).to_vec();
-											htlc_msat_height_data.extend_from_slice(
-												&byte_utils::be32_to_array(self.best_block.read().unwrap().height()),
-											);
-											failed_forwards.push((HTLCSource::PreviousHopData(HTLCPreviousHopData {
-													short_channel_id: htlc.prev_hop.short_channel_id,
-													outpoint: prev_funding_outpoint,
-													htlc_id: htlc.prev_hop.htlc_id,
-													incoming_packet_shared_secret: htlc.prev_hop.incoming_packet_shared_secret,
-												}), payment_hash,
-												HTLCFailReason::Reason { failure_code: 0x4000 | 15, data: htlc_msat_height_data }
-											));
+											fail_htlc!(htlc);
 										}
-									} else if total_value == data.total_msat {
+									} else if total_value == payment_data.total_msat {
 										new_events.push(events::Event::PaymentReceived {
 											payment_hash,
-											payment_secret: Some(data.payment_secret),
+											payment_secret: Some(payment_data.payment_secret),
 											amt: total_value,
 										});
 									}
-								} else {
-									new_events.push(events::Event::PaymentReceived {
-										payment_hash,
-										payment_secret: None,
-										amt: amt_to_forward,
-									});
 								}
 							},
 							HTLCForwardInfo::AddHTLC { .. } => {
@@ -2067,11 +2085,7 @@ impl<Signer: Sign, M: Deref, T: Deref, K: Deref, F: Deref, L: Deref> ChannelMana
 		let _persistence_guard = PersistenceNotifierGuard::new(&self.total_consistency_lock, &self.persistence_notifier);
 
 		let mut channel_state = Some(self.channel_state.lock().unwrap());
-		let payment_secrets = self.payment_secrets.lock().unwrap();
-		let payment_secret = if let Some(secret) = payment_secrets.get(&payment_hash) {
-			Some(secret.payment_secret)
-		} else { return false; };
-		let removed_source = channel_state.as_mut().unwrap().claimable_htlcs.remove(&(*payment_hash, payment_secret));
+		let removed_source = channel_state.as_mut().unwrap().claimable_htlcs.remove(payment_hash);
 		if let Some(mut sources) = removed_source {
 			for htlc in sources.drain(..) {
 				if channel_state.is_none() { channel_state = Some(self.channel_state.lock().unwrap()); }
@@ -2253,11 +2267,7 @@ impl<Signer: Sign, M: Deref, T: Deref, K: Deref, F: Deref, L: Deref> ChannelMana
 		let _persistence_guard = PersistenceNotifierGuard::new(&self.total_consistency_lock, &self.persistence_notifier);
 
 		let mut channel_state = Some(self.channel_state.lock().unwrap());
-		let payment_secrets = self.payment_secrets.lock().unwrap();
-		let payment_secret = if let Some(secret) = payment_secrets.get(&payment_hash) {
-			Some(secret.payment_secret)
-		} else { return false; };
-		let removed_source = channel_state.as_mut().unwrap().claimable_htlcs.remove(&(payment_hash, payment_secret));
+		let removed_source = channel_state.as_mut().unwrap().claimable_htlcs.remove(&payment_hash);
 		if let Some(mut sources) = removed_source {
 			assert!(!sources.is_empty());
 
@@ -2273,13 +2283,8 @@ impl<Signer: Sign, M: Deref, T: Deref, K: Deref, F: Deref, L: Deref> ChannelMana
 			// provide the preimage, so worrying too much about the optimal handling isn't worth
 			// it.
 
-			let (is_mpp, mut valid_mpp) = if let &Some(ref data) = &sources[0].payment_data {
-				assert!(payment_secret.is_some());
-				(true, data.total_msat >= expected_amount)
-			} else {
-				assert!(payment_secret.is_none());
-				(false, false)
-			};
+			let is_mpp = true;
+			let mut valid_mpp = sources[0].payment_data.total_msat >= expected_amount;
 
 			for htlc in sources.iter() {
 				if !is_mpp || !valid_mpp { break; }
@@ -3564,7 +3569,7 @@ impl<Signer: Sign, M: Deref, T: Deref, K: Deref, F: Deref, L: Deref> ChannelMana
 			});
 
 			if let Some(height) = height_opt {
-				channel_state.claimable_htlcs.retain(|&(ref payment_hash, _), htlcs| {
+				channel_state.claimable_htlcs.retain(|payment_hash, htlcs| {
 					htlcs.retain(|htlc| {
 						// If height is approaching the number of blocks we think it takes us to get
 						// our commitment transaction confirmed before the HTLC expires, plus the
@@ -4063,7 +4068,8 @@ impl Writeable for PendingHTLCInfo {
 			},
 			&PendingHTLCRouting::Receive { ref payment_data, ref incoming_cltv_expiry } => {
 				1u8.write(writer)?;
-				payment_data.write(writer)?;
+				payment_data.payment_secret.write(writer)?;
+				payment_data.total_msat.write(writer)?;
 				incoming_cltv_expiry.write(writer)?;
 			},
 		}
@@ -4084,7 +4090,10 @@ impl Readable for PendingHTLCInfo {
 					short_channel_id: Readable::read(reader)?,
 				},
 				1u8 => PendingHTLCRouting::Receive {
-					payment_data: Readable::read(reader)?,
+					payment_data: msgs::FinalOnionHopData {
+						payment_secret: Readable::read(reader)?,
+						total_msat: Readable::read(reader)?,
+					},
 					incoming_cltv_expiry: Readable::read(reader)?,
 				},
 				_ => return Err(DecodeError::InvalidValue),
@@ -4156,12 +4165,29 @@ impl_writeable!(HTLCPreviousHopData, 0, {
 	incoming_packet_shared_secret
 });
 
-impl_writeable!(ClaimableHTLC, 0, {
-	prev_hop,
-	value,
-	payment_data,
-	cltv_expiry
-});
+impl Writeable for ClaimableHTLC {
+	fn write<W: Writer>(&self, writer: &mut W) -> Result<(), ::std::io::Error> {
+		self.prev_hop.write(writer)?;
+		self.value.write(writer)?;
+		self.payment_data.payment_secret.write(writer)?;
+		self.payment_data.total_msat.write(writer)?;
+		self.cltv_expiry.write(writer)
+	}
+}
+
+impl Readable for ClaimableHTLC {
+	fn read<R: ::std::io::Read>(reader: &mut R) -> Result<Self, DecodeError> {
+		Ok(ClaimableHTLC {
+			prev_hop: Readable::read(reader)?,
+			value: Readable::read(reader)?,
+			payment_data: msgs::FinalOnionHopData {
+				payment_secret: Readable::read(reader)?,
+				total_msat: Readable::read(reader)?,
+			},
+			cltv_expiry: Readable::read(reader)?,
+		})
+	}
+}
 
 impl Writeable for HTLCSource {
 	fn write<W: Writer>(&self, writer: &mut W) -> Result<(), ::std::io::Error> {
diff --git a/lightning/src/ln/functional_tests.rs b/lightning/src/ln/functional_tests.rs
@@ -5097,22 +5097,25 @@ fn test_onchain_to_onchain_claim() {
 
 #[test]
 fn test_duplicate_payment_hash_one_failure_one_success() {
-	// Topology : A --> B --> C
+	// Topology : A --> B --> C --> D
 	// We route 2 payments with same hash between B and C, one will be timeout, the other successfully claim
-	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 mut nodes = create_network(3, &node_cfgs, &node_chanmgrs);
+	// Note that because C will refuse to generate two payment secrets for the same payment hash,
+	// we forward one of the payments onwards to D.
+	let chanmon_cfgs = create_chanmon_cfgs(4);
+	let node_cfgs = create_node_cfgs(4, &chanmon_cfgs);
+	let node_chanmgrs = create_node_chanmgrs(4, &node_cfgs, &[None, None, None, None]);
+	let mut nodes = create_network(4, &node_cfgs, &node_chanmgrs);
 
 	create_announced_chan_between_nodes(&nodes, 0, 1, InitFeatures::known(), InitFeatures::known());
 	let chan_2 = create_announced_chan_between_nodes(&nodes, 1, 2, InitFeatures::known(), InitFeatures::known());
+	create_announced_chan_between_nodes(&nodes, 2, 3, InitFeatures::known(), InitFeatures::known());
 
 	let (our_payment_preimage, duplicate_payment_hash, _) = route_payment(&nodes[0], &vec!(&nodes[1], &nodes[2])[..], 900000);
 
-	let (_, _, payment_secret) = get_payment_preimage_hash!(nodes[1]);
+	let payment_secret = nodes[3].node.get_payment_secret(duplicate_payment_hash, None, 1008).unwrap();
 	let route = get_route(&nodes[0].node.get_our_node_id(), &nodes[0].net_graph_msg_handler.network_graph.read().unwrap(),
-		&nodes[2].node.get_our_node_id(), Some(InvoiceFeatures::known()), None, &Vec::new(), 900000, TEST_FINAL_CLTV, nodes[0].logger).unwrap();
-	send_along_route_with_secret(&nodes[0], route, &[&[&nodes[1], &nodes[2]]], 900000, duplicate_payment_hash, payment_secret);
+		&nodes[3].node.get_our_node_id(), Some(InvoiceFeatures::known()), None, &Vec::new(), 900000, TEST_FINAL_CLTV, nodes[0].logger).unwrap();
+	send_along_route_with_secret(&nodes[0], route, &[&[&nodes[1], &nodes[2], &nodes[3]]], 900000, duplicate_payment_hash, payment_secret);
 
 	let commitment_txn = get_local_commitment_txn!(nodes[2], chan_2.2);
 	assert_eq!(commitment_txn[0].input.len(), 1);
