Drop need to store pending inbound payments

valentinewallace · valentinewallace · commit 9179991a6966 · 2021-11-29T21:19:38.000-05:00
and replace payment_secret with encrypted metadata

See docs on verify_inbound_payment for details
diff --git a/lightning/src/ln/channelmanager.rs b/lightning/src/ln/channelmanager.rs
@@ -50,7 +50,7 @@ use ln::msgs;
 use ln::msgs::NetAddress;
 use ln::onion_utils;
 use ln::msgs::{ChannelMessageHandler, DecodeError, LightningError, OptionalField};
-use chain::keysinterface::{Sign, KeysInterface, KeysManager, InMemorySigner};
+use chain::keysinterface::{Sign, KeyMaterial, KeysInterface, KeysManager, InMemorySigner};
 use util::config::UserConfig;
 use util::events::{EventHandler, EventsProvider, MessageSendEvent, MessageSendEventsProvider, ClosureReason};
 use util::{byte_utils, events};
@@ -65,6 +65,7 @@ use core::{cmp, mem};
 use core::cell::RefCell;
 use io::{Cursor, Read};
 use sync::{Arc, Condvar, Mutex, MutexGuard, RwLock, RwLockReadGuard};
+use core::convert::TryInto;
 use core::sync::atomic::{AtomicUsize, Ordering};
 use core::time::Duration;
 #[cfg(any(test, feature = "allow_wallclock_use"))]
@@ -642,6 +643,8 @@ pub struct ChannelManager<Signer: Sign, M: Deref, T: Deref, K: Deref, F: Deref,
 	our_network_key: SecretKey,
 	our_network_pubkey: PublicKey,
 
+	inbound_payment_key: Mutex<(KeyMaterial, Option<([u8; 32], [u8; 32], [u8; 32])>)>,
+
 	/// Used to track the last value sent in a node_announcement "timestamp" field. We ensure this
 	/// value increases strictly since we don't assume access to a time source.
 	last_node_announcement_serial: AtomicUsize,
@@ -1342,6 +1345,8 @@ impl<Signer: Sign, M: Deref, T: Deref, K: Deref, F: Deref, L: Deref> ChannelMana
 			our_network_pubkey: PublicKey::from_secret_key(&secp_ctx, &keys_manager.get_node_secret()),
 			secp_ctx,
 
+			inbound_payment_key: Mutex::new((keys_manager.get_inbound_payment_key_material(), None)),
+
 			last_node_announcement_serial: AtomicUsize::new(0),
 			highest_seen_timestamp: AtomicUsize::new(0),
 
@@ -2589,6 +2594,89 @@ impl<Signer: Sign, M: Deref, T: Deref, K: Deref, F: Deref, L: Deref> ChannelMana
 		}
 	}
 
+	fn get_expanded_inbound_payment_key(&self) -> ([u8; 32], [u8; 32], [u8; 32]) {
+		let mut guard = self.inbound_payment_key.lock().unwrap();
+		let (ikm, keys_opt) = (guard.0, &mut guard.1);
+		if let Some(keys) = keys_opt {
+			return keys.clone()
+		}
+		let keys = hkdf_extract_expand(&vec![0], &ikm);
+		*keys_opt = Some(keys.clone());
+		keys
+	}
+
+	// Check that an inbound payment's `payment_data` field is sane.
+	//
+	// LDK does not store any data for pending inbound payments. Instead, we construct our payment
+	// secret (and, if supplied by LDK, our payment preimage) to include encrypted metadata about the
+	// payment.
+	//
+	// The metadata is constructed as:
+	//   payment method (3 bits) || payment amount (8 bytes - 3 bits) || expiry (8 bytes)
+	//
+	// Then on payment receipt, we verify in this method that the payment preimage and payment secret
+	// match what was constructed.
+	fn verify_inbound_payment_data(&self, payment_hash: PaymentHash, payment_data: msgs::FinalOnionHopData) -> Result<Option<PaymentPreimage>, ()> {
+		let (metadata_key, user_pmt_hash_key, ldk_pmt_hash_key) = self.get_expanded_inbound_payment_key();
+		let (iv_bytes, encrypted_metadata_bytes) = payment_data.payment_secret.0.split_at(16);
+
+		let chacha_block = ChaCha20::get_single_block(&metadata_key, iv_bytes);
+		let mut metadata_bytes: [u8; 16] = [0; 16];
+		for i in 0..16 {
+			metadata_bytes[i] = chacha_block[i] ^ encrypted_metadata_bytes[i];
+		}
+
+		let payment_type: u8 = (metadata_bytes[0] & 0b1110_0000) >> 5;
+		let mut amt_msat_bytes = [0; 8];
+		amt_msat_bytes.copy_from_slice(&metadata_bytes[..8]);
+		// Zero out the bits reserved to indicate the payment type.
+		amt_msat_bytes[0] &= 0b00011111;
+		let min_amt_msat = u64::from_be_bytes(amt_msat_bytes.try_into().unwrap());
+		let expiry = u64::from_be_bytes(metadata_bytes[8..].try_into().unwrap());
+		let current_time = self.highest_seen_timestamp.load(Ordering::Acquire) as u64;
+
+		// Make sure to check to check the HMAC before doing the other checks below, to mitigate timing
+		// attacks.
+		let is_user_payment_hash = metadata_bytes[0] & 1 << 5 != 0;
+		let mut payment_preimage = None;
+		if is_user_payment_hash {
+			let mut hmac = HmacEngine::<Sha256>::new(&user_pmt_hash_key);
+			hmac.input(&metadata_bytes[..]);
+			hmac.input(&payment_hash.0);
+			if !fixed_time_eq(iv_bytes, &Hmac::from_engine(hmac).into_inner().split_at_mut(16).0) {
+				log_trace!(self.logger, "Failing HTLC with user-generated payment_hash {}: unexpected payment_secret", log_bytes!(payment_hash.0));
+				return Err(())
+			}
+		} else {
+			let mut hmac = HmacEngine::<Sha256>::new(&ldk_pmt_hash_key);
+			hmac.input(iv_bytes);
+			hmac.input(&metadata_bytes);
+			let decoded_payment_preimage = Hmac::from_engine(hmac).into_inner();
+			if !fixed_time_eq(&payment_hash.0, &Sha256::hash(&decoded_payment_preimage).into_inner()) {
+				log_trace!(self.logger, "Failing HTLC with payment_hash {}: payment preimage {} did not match", log_bytes!(payment_hash.0), log_bytes!(decoded_payment_preimage));
+				return Err(())
+			}
+			payment_preimage = Some(PaymentPreimage(decoded_payment_preimage));
+		}
+
+		if payment_type > 1 { // We only support payment method types of 0000 or 0001
+			log_trace!(self.logger, "Failing HTLC with payment hash {} due to unknown payment type {}", log_bytes!(payment_hash.0), payment_type);
+			return Err(());
+		}
+
+		if payment_data.total_msat < min_amt_msat {
+			log_trace!(self.logger, "Failing HTLC with payment_hash {} due to total_msat {} being less than the minimum amount of {} msat", log_bytes!(payment_hash.0), payment_data.total_msat, min_amt_msat);
+			return Err(())
+		}
+
+		if expiry < current_time {
+			log_trace!(self.logger, "Failing HTLC with payment_hash {}: expired payment", log_bytes!(payment_hash.0));
+			return Err(())
+		}
+
+		Ok(payment_preimage)
+	}
+
 	/// Processes HTLCs which are pending waiting on random forward delay.
 	///
 	/// Should only really ever be called in response to a PendingHTLCsForwardable event.
@@ -2864,9 +2952,17 @@ impl<Signer: Sign, M: Deref, T: Deref, K: Deref, F: Deref, L: Deref> ChannelMana
 								match payment_secrets.entry(payment_hash) {
 									hash_map::Entry::Vacant(_) => {
 										match claimable_htlc.onion_payload {
-											OnionPayload::Invoice(_) => {
-												log_trace!(self.logger, "Failing new HTLC with payment_hash {} as we didn't have a corresponding inbound payment.", log_bytes!(payment_hash.0));
-												fail_htlc!(claimable_htlc);
+											OnionPayload::Invoice(ref payment_data) => {
+												let payment_preimage = match self.verify_inbound_payment_data(payment_hash, payment_data.clone()) {
+													Ok(payment_preimage) => payment_preimage,
+													Err(()) => {
+														fail_htlc!(claimable_htlc);
+														continue
+													}
+												};
+												let payment_data_total_msat = payment_data.total_msat;
+												let payment_secret = payment_data.payment_secret.clone();
+												check_total_value!(payment_data_total_msat, payment_secret, payment_preimage);
 											},
 											OnionPayload::Spontaneous(preimage) => {
 												match channel_state.claimable_htlcs.entry(payment_hash) {
@@ -4558,25 +4654,82 @@ impl<Signer: Sign, M: Deref, T: Deref, K: Deref, F: Deref, L: Deref> ChannelMana
 	/// to pay us.
 	///
 	/// This differs from [`create_inbound_payment_for_hash`] only in that it generates the
-	/// [`PaymentHash`] and [`PaymentPreimage`] for you, returning the first and storing the second.
+	/// [`PaymentHash`] and [`PaymentPreimage`] for you.
 	///
 	/// The [`PaymentPreimage`] will ultimately be returned to you in the [`PaymentReceived`], which
 	/// will have the [`PaymentReceived::payment_preimage`] field filled in. That should then be
 	/// passed directly to [`claim_funds`].
 	///
 	/// See [`create_inbound_payment_for_hash`] for detailed documentation on behavior and requirements.
 	///
+	/// Note that a malicious eavesdropper can intuit whether an inbound payment was created by
+	/// `create_inbound_payment` or `create_inbound_payment_for_hash` based on runtime.
+	///
+	/// # Note
+	///
+	/// If you register an inbound payment with this method, then serialize the `ChannelManager`, then
+	/// deserialize it with a node running 0.0.103 and earlier, the payment will fail to be received.
+	///
 	/// [`claim_funds`]: Self::claim_funds
 	/// [`PaymentReceived`]: events::Event::PaymentReceived
 	/// [`PaymentReceived::payment_preimage`]: events::Event::PaymentReceived::payment_preimage
 	/// [`create_inbound_payment_for_hash`]: Self::create_inbound_payment_for_hash
+	// For details on the implementation of this method, see `verify_inbound_payment_data`.
 	pub fn create_inbound_payment(&self, min_value_msat: Option<u64>, invoice_expiry_delta_secs: u32) -> (PaymentHash, PaymentSecret) {
+		let (metadata_key, _, ldk_pmt_hash_key) = self.get_expanded_inbound_payment_key();
+		let min_amt_msat_bytes: [u8; 8] = match min_value_msat {
+			Some(amt) => amt.to_be_bytes(),
+			None => [0; 8],
+		};
+		// We assume that highest_seen_timestamp is pretty close to the current time - its updated when
+		// we receive a new block with the maximum time we've seen in a header. It should never be more
+		// than two hours in the future.  Thus, we add two hours here as a buffer to ensure we
+		// absolutely never fail a payment too early.
+		// Note that we assume that received blocks have reasonably up-to-date timestamps.
+		let expiry_bytes = (self.highest_seen_timestamp.load(Ordering::Acquire) as u64 + invoice_expiry_delta_secs as u64 + 7200).to_be_bytes();
+		let mut metadata_bytes: [u8; 16] = [0; 16];
+		{
+			let (min_amt_msat_slice, expiry_slice) = metadata_bytes.split_at_mut(8);
+			min_amt_msat_slice.copy_from_slice(&min_amt_msat_bytes);
+			expiry_slice.copy_from_slice(&expiry_bytes);
+		}
+
+		let rand_bytes = self.keys_manager.get_secure_random_bytes();
+		let iv_bytes = &rand_bytes[..16];
+
+		let mut hmac = HmacEngine::<Sha256>::new(&ldk_pmt_hash_key);
+		hmac.input(iv_bytes);
+		hmac.input(&metadata_bytes);
+		let payment_preimage_bytes = Hmac::from_engine(hmac).into_inner();
+
+		let mut payment_secret_bytes: [u8; 32] = [0; 32];
+		{
+			let (iv_slice, encrypted_metadata_slice) = payment_secret_bytes.split_at_mut(16);
+			iv_slice.copy_from_slice(iv_bytes);
+
+			let chacha_block = ChaCha20::get_single_block(&metadata_key, iv_bytes);
+			for i in 0..16 {
+				encrypted_metadata_slice[i] = chacha_block[i] ^ metadata_bytes[i];
+			}
+		}
+
+		let payment_hash = PaymentHash(Sha256::hash(&payment_preimage_bytes).into_inner());
+		(payment_hash, PaymentSecret(payment_secret_bytes))
+	}
+
+	/// Legacy version of [`create_inbound_payment`]. Use this method if you wish to share
+	/// serialized state with LDK node(s) running 0.0.103 and earlier.
+	///
+	/// # Note
+	/// This method will be deprecated in the next few versions.
+	///
+	/// [`create_inbound_payment`]: Self::create_inbound_payment
+	pub fn create_inbound_payment_legacy(&self, min_value_msat: Option<u64>, invoice_expiry_delta_secs: u32) -> (PaymentHash, PaymentSecret) {
 		let payment_preimage = PaymentPreimage(self.keys_manager.get_secure_random_bytes());
 		let payment_hash = PaymentHash(Sha256::hash(&payment_preimage.0).into_inner());
-
 		(payment_hash,
-			self.set_payment_hash_secret_map(payment_hash, Some(payment_preimage), min_value_msat, invoice_expiry_delta_secs)
-				.expect("RNG Generated Duplicate PaymentHash"))
+		 self.set_payment_hash_secret_map(payment_hash, Some(payment_preimage), min_value_msat, invoice_expiry_delta_secs)
+		     .expect("RNG Generated Duplicate PaymentHash"))
 	}
 
 	/// Gets a [`PaymentSecret`] for a given [`PaymentHash`], for which the payment preimage is
@@ -4606,18 +4759,73 @@ impl<Signer: Sign, M: Deref, T: Deref, K: Deref, F: Deref, L: Deref> ChannelMana
 	/// If you need exact expiry semantics, you should enforce them upon receipt of
 	/// [`PaymentReceived`].
 	///
-	/// Pending inbound payments are stored in memory and in serialized versions of this
-	/// [`ChannelManager`]. If potentially unbounded numbers of inbound payments may exist and
-	/// space is limited, you may wish to rate-limit inbound payment creation.
-	///
 	/// May panic if `invoice_expiry_delta_secs` is greater than one year.
 	///
 	/// Note that invoices generated for inbound payments should have their `min_final_cltv_expiry`
 	/// set to at least [`MIN_FINAL_CLTV_EXPIRY`].
 	///
+	/// Note that a malicious eavesdropper can intuit whether an inbound payment was created by
+	/// `create_inbound_payment` or `create_inbound_payment_for_hash` based on runtime.
+	///
+	/// # Note
+	///
+	/// If you register an inbound payment with this method, then serialize the `ChannelManager`, then
+	/// deserialize it with a node running 0.0.103 and earlier, the payment will fail to be received.
+	///
 	/// [`create_inbound_payment`]: Self::create_inbound_payment
 	/// [`PaymentReceived`]: events::Event::PaymentReceived
+	// For details on the implementation of this method, see `verify_inbound_payment_data`.
 	pub fn create_inbound_payment_for_hash(&self, payment_hash: PaymentHash, min_value_msat: Option<u64>, invoice_expiry_delta_secs: u32) -> Result<PaymentSecret, APIError> {
+		let (metadata_key, user_pmt_hash_key, _) = self.get_expanded_inbound_payment_key();
+		let mut min_amt_msat_bytes: [u8; 8] = match min_value_msat {
+			Some(amt) => amt.to_be_bytes(),
+			None => [0; 8],
+		};
+		// Set the payment method bits (the top 3 bits of the metadata bytes) to 001
+		// indicating the payment type is paying a user-generated payment hash.
+		min_amt_msat_bytes[0] |= 1 << 5;
+
+		// We assume that highest_seen_timestamp is pretty close to the current time - its updated when
+		// we receive a new block with the maximum time we've seen in a header. It should never be more
+		// than two hours in the future.  Thus, we add two hours here as a buffer to ensure we
+		// absolutely never fail a payment too early.
+		// Note that we assume that received blocks have reasonably up-to-date timestamps.
+		let expiry_bytes = (self.highest_seen_timestamp.load(Ordering::Acquire) as u64 + invoice_expiry_delta_secs as u64 + 7200).to_be_bytes();
+
+		let mut metadata_bytes: [u8; 16] = [0; 16];
+		{
+			let (min_amt_msat_slice, expiry_slice) = metadata_bytes.split_at_mut(8);
+			min_amt_msat_slice.copy_from_slice(&min_amt_msat_bytes);
+			expiry_slice.copy_from_slice(&expiry_bytes);
+		}
+
+		let mut hmac = HmacEngine::<Sha256>::new(&user_pmt_hash_key);
+		hmac.input(&metadata_bytes);
+		hmac.input(&payment_hash.0);
+		let hmac_bytes = Hmac::from_engine(hmac).into_inner();
+		let iv_bytes = &hmac_bytes[..16];
+
+		let mut payment_secret_bytes: [u8; 32] = [0; 32];
+		{
+			let (iv_slice, encrypted_metadata_slice) = payment_secret_bytes.split_at_mut(16);
+			iv_slice.copy_from_slice(iv_bytes);
+
+			let chacha_block = ChaCha20::get_single_block(&metadata_key, iv_bytes);
+			for i in 0..16 {
+				encrypted_metadata_slice[i] = chacha_block[i] ^ metadata_bytes[i];
+			}
+		}
+		Ok(PaymentSecret(payment_secret_bytes))
+	}
+
+	/// Legacy version of [`create_inbound_payment_for_hash`]. Use this method if you wish to share
+	/// serialized state with LDK node(s) running 0.0.103 and earlier.
+	///
+	/// # Note
+	/// This method will be deprecated in the next few versions.
+	///
+	/// [`create_inbound_payment_for_hash`]: Self::create_inbound_payment_for_hash
+	pub fn create_inbound_payment_for_hash_legacy(&self, payment_hash: PaymentHash, min_value_msat: Option<u64>, invoice_expiry_delta_secs: u32) -> Result<PaymentSecret, APIError> {
 		self.set_payment_hash_secret_map(payment_hash, None, min_value_msat, invoice_expiry_delta_secs)
 	}
 
@@ -6066,6 +6274,7 @@ impl<'a, Signer: Sign, M: Deref, T: Deref, K: Deref, F: Deref, L: Deref>
 				claimable_htlcs,
 				pending_msg_events: Vec::new(),
 			}),
+			inbound_payment_key: Mutex::new((args.keys_manager.get_inbound_payment_key_material(), None)),
 			pending_inbound_payments: Mutex::new(pending_inbound_payments),
 			pending_outbound_payments: Mutex::new(pending_outbound_payments.unwrap()),
 
@@ -6099,6 +6308,27 @@ impl<'a, Signer: Sign, M: Deref, T: Deref, K: Deref, F: Deref, L: Deref>
 	}
 }
 
+fn hkdf_extract_expand(salt: &[u8], ikm: &KeyMaterial) -> ([u8; 32], [u8; 32], [u8; 32]) {
+	let mut hmac = HmacEngine::<Sha256>::new(salt);
+	hmac.input(&ikm.0);
+	let prk = Hmac::from_engine(hmac).into_inner();
+	let mut hmac = HmacEngine::<Sha256>::new(&prk[..]);
+	hmac.input(&[1; 1]);
+	let t1 = Hmac::from_engine(hmac).into_inner();
+
+	let mut hmac = HmacEngine::<Sha256>::new(&prk[..]);
+	hmac.input(&t1);
+	hmac.input(&[2; 1]);
+	let t2 = Hmac::from_engine(hmac).into_inner();
+
+	let mut hmac = HmacEngine::<Sha256>::new(&prk[..]);
+	hmac.input(&t2);
+	hmac.input(&[3; 1]);
+	let t3 = Hmac::from_engine(hmac).into_inner();
+
+	(t1, t2, t3)
+}
+
 #[cfg(test)]
 mod tests {
 	use bitcoin::hashes::Hash;
diff --git a/lightning/src/ln/functional_tests.rs b/lightning/src/ln/functional_tests.rs
@@ -8102,19 +8102,20 @@ fn test_preimage_storage() {
 
 #[test]
 fn test_secret_timeout() {
-	// Simple test of payment secret storage time outs
+	// Simple test of payment secret storage time outs. After
+	// `create_inbound_payment(_for_hash)_legacy` is removed, this test will be removed as well.
 	let chanmon_cfgs = create_chanmon_cfgs(2);
 	let node_cfgs = create_node_cfgs(2, &chanmon_cfgs);
 	let node_chanmgrs = create_node_chanmgrs(2, &node_cfgs, &[None, None]);
 	let nodes = create_network(2, &node_cfgs, &node_chanmgrs);
 
 	create_announced_chan_between_nodes(&nodes, 0, 1, InitFeatures::known(), InitFeatures::known()).0.contents.short_channel_id;
 
-	let (payment_hash, payment_secret_1) = nodes[1].node.create_inbound_payment(Some(100_000), 2);
+	let (payment_hash, payment_secret_1) = nodes[1].node.create_inbound_payment_legacy(Some(100_000), 2);
 
 	// We should fail to register the same payment hash twice, at least until we've connected a
 	// block with time 7200 + CHAN_CONFIRM_DEPTH + 1.
-	if let Err(APIError::APIMisuseError { err }) = nodes[1].node.create_inbound_payment_for_hash(payment_hash, Some(100_000), 2) {
+	if let Err(APIError::APIMisuseError { err }) = nodes[1].node.create_inbound_payment_for_hash_legacy(payment_hash, Some(100_000), 2) {
 		assert_eq!(err, "Duplicate payment hash");
 	} else { panic!(); }
 	let mut block = {
@@ -8129,7 +8130,7 @@ fn test_secret_timeout() {
 		}
 	};
 	connect_block(&nodes[1], &block);
-	if let Err(APIError::APIMisuseError { err }) = nodes[1].node.create_inbound_payment_for_hash(payment_hash, Some(100_000), 2) {
+	if let Err(APIError::APIMisuseError { err }) = nodes[1].node.create_inbound_payment_for_hash_legacy(payment_hash, Some(100_000), 2) {
 		assert_eq!(err, "Duplicate payment hash");
 	} else { panic!(); }
 
@@ -8138,7 +8139,7 @@ fn test_secret_timeout() {
 	block.header.prev_blockhash = block.header.block_hash();
 	block.header.time += 1;
 	connect_block(&nodes[1], &block);
-	let our_payment_secret = nodes[1].node.create_inbound_payment_for_hash(payment_hash, Some(100_000), 2).unwrap();
+	let our_payment_secret = nodes[1].node.create_inbound_payment_for_hash_legacy(payment_hash, Some(100_000), 2).unwrap();
 	assert_ne!(payment_secret_1, our_payment_secret);
 
 	{
diff --git a/lightning/src/util/events.rs b/lightning/src/util/events.rs
@@ -183,7 +183,8 @@ pub enum Event {
 	/// [`ChannelManager::claim_funds`]: crate::ln::channelmanager::ChannelManager::claim_funds
 	/// [`ChannelManager::fail_htlc_backwards`]: crate::ln::channelmanager::ChannelManager::fail_htlc_backwards
 	PaymentReceived {
-		/// The hash for which the preimage should be handed to the ChannelManager.
+		/// The hash for which the preimage should be handed to the ChannelManager. Note that LDK will
+		/// not stop you from registering duplicate payment hashes for inbound payments.
 		payment_hash: PaymentHash,
 		/// The value, in thousandths of a satoshi, that this payment is for.
 		amt: u64,
