Drop need to store pending inbound payments

and replace payment_secret with encrypted metadata

See docs on verify_inbound_payment for details

Author: valentinewallace

lightning/src/ln/channelmanager.rs

@@ -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: [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: keys_manager.get_inbound_payment_secret(),
+
 			last_node_announcement_serial: AtomicUsize::new(0),
 			highest_seen_timestamp: AtomicUsize::new(0),
 
@@ -2589,6 +2594,75 @@ impl<Signer: Sign, M: Deref, T: Deref, K: Deref, F: Deref, L: Deref> ChannelMana
 		}
 	}
 
+	// 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 (4 bits) || payment amount (8 bytes - 4 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) = hkdf_extract_expand(&vec![0], &self.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];
+		}
+
+		// Make sure to check to check the HMAC before doing the other checks below, to mitigate DoS.
+		let is_user_payment_hash = metadata_bytes[0] & 1 << 4 != 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));
+		}
+
+		let payment_type: u8 = (metadata_bytes[0] & 0b1111_0000) >> 4;
+		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(());
+		}
+
+		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] &= 0b00001111;
+		let min_amt_msat = u64::from_be_bytes(amt_msat_bytes.try_into().unwrap());
+		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(())
+		}
+
+		let expiry = u64::from_be_bytes(metadata_bytes[8..].try_into().unwrap());
+		if expiry < self.highest_seen_timestamp.load(Ordering::Acquire) as u64 {
+			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 +2938,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) {
@@ -4527,56 +4609,67 @@ impl<Signer: Sign, M: Deref, T: Deref, K: Deref, F: Deref, L: Deref> ChannelMana
 		}
 	}
 
-	fn set_payment_hash_secret_map(&self, payment_hash: PaymentHash, payment_preimage: Option<PaymentPreimage>, min_value_msat: Option<u64>, invoice_expiry_delta_secs: u32) -> Result<PaymentSecret, APIError> {
-		assert!(invoice_expiry_delta_secs <= 60*60*24*365); // Sadly bitcoin timestamps are u32s, so panic before 2106
-
-		let payment_secret = PaymentSecret(self.keys_manager.get_secure_random_bytes());
-
-		let _persistence_guard = PersistenceNotifierGuard::notify_on_drop(&self.total_consistency_lock, &self.persistence_notifier);
-		let mut payment_secrets = self.pending_inbound_payments.lock().unwrap();
-		match payment_secrets.entry(payment_hash) {
-			hash_map::Entry::Vacant(e) => {
-				e.insert(PendingInboundPayment {
-					payment_secret, min_value_msat, payment_preimage,
-					user_payment_id: 0, // For compatibility with version 0.0.103 and earlier
-					// 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.
-					expiry_time: self.highest_seen_timestamp.load(Ordering::Acquire) as u64 + invoice_expiry_delta_secs as u64 + 7200,
-				});
-			},
-			hash_map::Entry::Occupied(_) => return Err(APIError::APIMisuseError { err: "Duplicate payment hash".to_owned() }),
-		}
-		Ok(payment_secret)
-	}
-
 	/// Gets a payment secret and payment hash for use in an invoice given to a third party wishing
 	/// 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 could theoretically determine whether an inbound payment
+	/// was created by `create_inbound_payment` or `create_inbound_payment_for_hash`, by flipping the
+	/// relevant bit in the payment secret and observing the difference in processing time.
+	///
 	/// [`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 payment_preimage = PaymentPreimage(self.keys_manager.get_secure_random_bytes());
-		let payment_hash = PaymentHash(Sha256::hash(&payment_preimage.0).into_inner());
+		let (metadata_key, _, ldk_pmt_hash_key) = hkdf_extract_expand(&vec![0], &self.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();
 
-		(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"))
+		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))
 	}
 
 	/// Gets a [`PaymentSecret`] for a given [`PaymentHash`], for which the payment preimage is
@@ -4598,27 +4691,65 @@ impl<Signer: Sign, M: Deref, T: Deref, K: Deref, F: Deref, L: Deref> ChannelMana
 	/// in excess of the current time. This should roughly match the expiry time set in the invoice.
 	/// After this many seconds, we will remove the inbound payment, resulting in any attempts to
 	/// pay the invoice failing. The BOLT spec suggests 3,600 secs as a default validity time for
-	/// invoices when no timeout is set.
-	///
-	/// Note that we use block header time to time-out pending inbound payments (with some margin
-	/// to compensate for the inaccuracy of block header timestamps). Thus, in practice we will
-	/// accept a payment and generate a [`PaymentReceived`] event for some time after the expiry.
-	/// 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.
+	/// invoices when no timeout is set.  Note that we use block header time to time-out pending
+	/// inbound payments (with some margin to compensate for the inaccuracy of block header
+	/// timestamps). Thus, in practice we will accept a payment and generate a [`PaymentReceived`]
+	/// event for some time after the expiry.  If you need exact expiry semantics, you should enforce
+	/// them upon receipt of [`PaymentReceived`].
 	///
 	/// 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 could theoretically determine whether an inbound payment
+	/// was created by `create_inbound_payment` or `create_inbound_payment_for_hash`, by flipping the
+	/// relevant bit in the payment secret and observing the difference in processing time.
+	///
 	/// [`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> {
-		self.set_payment_hash_secret_map(payment_hash, None, min_value_msat, invoice_expiry_delta_secs)
+		let (metadata_key, user_pmt_hash_key, _) = hkdf_extract_expand(&vec![0], &self.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 4 bits of the metadata bytes) to 0001
+		// indicating the payment type is paying a user-generated payment hash.
+		min_amt_msat_bytes[0] |= 1 << 4;
+
+		// 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))
 	}
 
 	#[cfg(any(test, feature = "fuzztarget", feature = "_test_utils"))]
@@ -5690,6 +5821,7 @@ impl<Signer: Sign, M: Deref, T: Deref, K: Deref, F: Deref, L: Deref> Writeable f
 		}
 		write_tlv_fields!(writer, {
 			(1, pending_outbound_payments_no_retry, required),
+			(2, self.inbound_payment_key, required),
 			(3, pending_outbound_payments, required),
 		});
 
@@ -5985,10 +6117,15 @@ impl<'a, Signer: Sign, M: Deref, T: Deref, K: Deref, F: Deref, L: Deref>
 		// pending_outbound_payments_no_retry is for compatibility with 0.0.101 clients.
 		let mut pending_outbound_payments_no_retry: Option<HashMap<PaymentId, HashSet<[u8; 32]>>> = None;
 		let mut pending_outbound_payments = None;
+		let mut inbound_payment_key = None;
 		read_tlv_fields!(reader, {
 			(1, pending_outbound_payments_no_retry, option),
+			(2, inbound_payment_key, option),
 			(3, pending_outbound_payments, option),
 		});
+		if inbound_payment_key.is_none() {
+			inbound_payment_key = Some(args.keys_manager.get_inbound_payment_secret());
+		}
 		if pending_outbound_payments.is_none() && pending_outbound_payments_no_retry.is_none() {
 			pending_outbound_payments = Some(pending_outbound_payments_compat);
 		} else if pending_outbound_payments.is_none() {
@@ -6066,6 +6203,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: inbound_payment_key.unwrap(),
 			pending_inbound_payments: Mutex::new(pending_inbound_payments),
 			pending_outbound_payments: Mutex::new(pending_outbound_payments.unwrap()),
 
@@ -6099,6 +6237,27 @@ impl<'a, Signer: Sign, M: Deref, T: Deref, K: Deref, F: Deref, L: Deref>
 	}
 }
 
+fn hkdf_extract_expand(salt: &[u8], ikm: &[u8]) -> ([u8; 32], [u8; 32], [u8; 32]) {
+	let mut hmac = HmacEngine::<Sha256>::new(salt);
+	hmac.input(ikm);
+	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;