Encrypt payment_metadata when we build the payment secret

In 657ac8f we started committing
to the `payment_metadata` in the `payment_secret`. We'd largely
assumed that downstream code could simply encrypt the
`payment_metadata` itself before passing it to `lightning` and
decrypt before reading it from `lightning`. However, this presents
a challenge - we'd very much love for that downstream code to avoid
adding any extra bytes to its `payment_metadata` if at all
possible, but it doesn't have a great way to get a decent IV
without simply shoving it in the encrypted `payment_metadata`.

Instead, here, we encrypt and decrypt the `payment_metadata`
internally in `lightning`. This allows us to reuse the IV that is
used for `lightning`-generated `payment_hash`es as the IV for the
encrypted `payment_metadata` as well. Sadly, we don't have any
similar IV for user-provided `payment_hash`es. In that case, we
simply accept the limitations and document that users must avoid
encrypting multiple `payment_metadata`s for payments with the same
`payment_hash`. This avoids padding the size of the
`payment_metadata` and should generally not be a material concern -
`payment_hash` reuse should generally not exist anyway, and if it
does it should only be in cases where its "the same payment" being
retried after failure, at which point `payment_metadata` should
hopefully be the same.

Author: TheBlueMatt

fuzz/src/chanmon_consistency.rs

@@ -1369,7 +1369,7 @@ impl PaymentTracker {
 		let mut payment_preimage = PaymentPreimage([0; 32]);
 		payment_preimage.0[0..8].copy_from_slice(&self.payment_ctr.to_be_bytes());
 		let hash = PaymentHash(Sha256::hash(&payment_preimage.0).to_byte_array());
-		let secret = dest
+		let (secret, _no_metadata) = dest
 			.create_inbound_payment_for_hash(hash, None, 3600, None, None)
 			.expect("create_inbound_payment_for_hash failed");
 		assert!(self.payment_preimages.insert(hash, payment_preimage).is_none());

lightning-liquidity/tests/lsps2_integration_tests.rs

@@ -120,7 +120,7 @@ fn create_jit_invoice(
 ) -> Result<Bolt11Invoice, ()> {
 	// LSPS2 requires min_final_cltv_expiry_delta to be at least 2 more than usual.
 	let min_final_cltv_expiry_delta = MIN_FINAL_CLTV_EXPIRY_DELTA + 2;
-	let (payment_hash, payment_secret) = node
+	let (payment_hash, payment_secret, _) = node
 		.node
 		.create_inbound_payment(None, expiry_secs, Some(min_final_cltv_expiry_delta), None)
 		.map_err(|e| {

lightning/src/crypto/utils.rs

@@ -22,7 +22,7 @@ macro_rules! hkdf_extract_expand {
 		let (k1, k2, _) = hkdf_extract_expand!($salt, $ikm);
 		(k1, k2)
 	}};
-	($salt: expr, $ikm: expr, 7) => {{
+	($salt: expr, $ikm: expr, 8) => {{
 		let (k1, k2, prk) = hkdf_extract_expand!($salt, $ikm);
 
 		let mut hmac = HmacEngine::<Sha256>::new(&prk[..]);
@@ -50,18 +50,23 @@ macro_rules! hkdf_extract_expand {
 		hmac.input(&[7; 1]);
 		let k7 = Hmac::from_engine(hmac).to_byte_array();
 
-		(k1, k2, k3, k4, k5, k6, k7)
+		let mut hmac = HmacEngine::<Sha256>::new(&prk[..]);
+		hmac.input(&k7);
+		hmac.input(&[8; 1]);
+		let k8 = Hmac::from_engine(hmac).to_byte_array();
+
+		(k1, k2, k3, k4, k5, k6, k7, k8)
 	}};
 }
 
 pub fn hkdf_extract_expand_twice(salt: &[u8], ikm: &[u8]) -> ([u8; 32], [u8; 32]) {
 	hkdf_extract_expand!(salt, ikm, 2)
 }
 
-pub fn hkdf_extract_expand_7x(
+pub fn hkdf_extract_expand_8x(
 	salt: &[u8], ikm: &[u8],
-) -> ([u8; 32], [u8; 32], [u8; 32], [u8; 32], [u8; 32], [u8; 32], [u8; 32]) {
-	hkdf_extract_expand!(salt, ikm, 7)
+) -> ([u8; 32], [u8; 32], [u8; 32], [u8; 32], [u8; 32], [u8; 32], [u8; 32], [u8; 32]) {
+	hkdf_extract_expand!(salt, ikm, 8)
 }
 
 #[inline]

lightning/src/ln/bolt11_payment_tests.rs

@@ -30,8 +30,10 @@ fn payment_metadata_end_to_end_for_invoice_with_amount() {
 
 	let payment_metadata = vec![42, 43, 44, 45, 46, 47, 48, 49, 42];
 
-	let (payment_hash, payment_secret) =
-		nodes[1].node.create_inbound_payment(None, 7200, None, Some(&payment_metadata)).unwrap();
+	let (payment_hash, payment_secret, encrypted_metadata) = nodes[1]
+		.node
+		.create_inbound_payment(None, 7200, None, Some(payment_metadata.clone()))
+		.unwrap();
 
 	let timestamp = SystemTime::now().duration_since(SystemTime::UNIX_EPOCH).unwrap();
 	let invoice = InvoiceBuilder::new(Currency::Bitcoin)
@@ -41,7 +43,7 @@ fn payment_metadata_end_to_end_for_invoice_with_amount() {
 		.duration_since_epoch(timestamp)
 		.min_final_cltv_expiry_delta(144)
 		.amount_milli_satoshis(50_000)
-		.payment_metadata(payment_metadata.clone())
+		.payment_metadata(encrypted_metadata.unwrap())
 		.build_raw()
 		.unwrap();
 	let sig = nodes[1].keys_manager.backing.sign_invoice(&invoice, Recipient::Node).unwrap();
@@ -97,8 +99,10 @@ fn payment_metadata_end_to_end_for_invoice_with_no_amount() {
 
 	let payment_metadata = vec![42, 43, 44, 45, 46, 47, 48, 49, 42];
 
-	let (payment_hash, payment_secret) =
-		nodes[1].node.create_inbound_payment(None, 7200, None, Some(&payment_metadata)).unwrap();
+	let (payment_hash, payment_secret, encrypted_metadata) = nodes[1]
+		.node
+		.create_inbound_payment(None, 7200, None, Some(payment_metadata.clone()))
+		.unwrap();
 
 	let timestamp = SystemTime::now().duration_since(SystemTime::UNIX_EPOCH).unwrap();
 	let invoice = InvoiceBuilder::new(Currency::Bitcoin)
@@ -107,7 +111,7 @@ fn payment_metadata_end_to_end_for_invoice_with_no_amount() {
 		.payment_secret(payment_secret)
 		.duration_since_epoch(timestamp)
 		.min_final_cltv_expiry_delta(144)
-		.payment_metadata(payment_metadata.clone())
+		.payment_metadata(encrypted_metadata.unwrap())
 		.build_raw()
 		.unwrap();
 	let sig = nodes[1].keys_manager.backing.sign_invoice(&invoice, Recipient::Node).unwrap();

lightning/src/ln/channelmanager.rs

@@ -8462,7 +8462,7 @@ impl<
 						payment_data,
 						payment_context,
 						phantom_shared_secret,
-						onion_fields,
+						mut onion_fields,
 						has_recipient_created_payment_secret,
 						invoice_request_opt,
 						trampoline_shared_secret,
@@ -8603,7 +8603,7 @@ impl<
 							let verify_res = inbound_payment::verify(
 								payment_hash,
 								&payment_data,
-								onion_fields.payment_metadata.as_deref(),
+								onion_fields.payment_metadata.as_mut(),
 								self.highest_seen_timestamp.load(Ordering::Acquire) as u64,
 								&self.inbound_payment_key,
 								&self.logger,
@@ -14372,24 +14372,24 @@ This indicates a bug inside LDK. Please report this error at https://github.com/
 			}
 		}
 
-		let (payment_hash, payment_secret) = match payment_hash {
+		let (payment_hash, payment_secret, payment_metadata) = match payment_hash {
 			Some(payment_hash) => {
-				let payment_secret = self
+				let (payment_secret, payment_metadata) = self
 					.create_inbound_payment_for_hash(
 						payment_hash, amount_msats,
 						invoice_expiry_delta_secs.unwrap_or(DEFAULT_EXPIRY_TIME as u32),
 						min_final_cltv_expiry_delta,
-						payment_metadata.as_deref(),
+						payment_metadata,
 					)
 					.map_err(|()| SignOrCreationError::CreationError(CreationError::InvalidAmount))?;
-				(payment_hash, payment_secret)
+				(payment_hash, payment_secret, payment_metadata)
 			},
 			None => {
 				self
 					.create_inbound_payment(
 						amount_msats, invoice_expiry_delta_secs.unwrap_or(DEFAULT_EXPIRY_TIME as u32),
 						min_final_cltv_expiry_delta,
-						payment_metadata.as_deref(),
+						payment_metadata,
 					)
 					.map_err(|()| SignOrCreationError::CreationError(CreationError::InvalidAmount))?
 			},
@@ -14516,8 +14516,7 @@ pub struct Bolt11InvoiceParameters {
 	/// onion by the sender, available as [`RecipientOnionFields::payment_metadata`] via
 	/// [`Event::PaymentClaimable::onion_fields`].
 	///
-	/// Note that because it is exposed to the sender in the invoice you should consider encrypting
-	/// it. It is committed to, however, so cannot be modified by the sender.
+	/// The metadata itself is encrypted and HMAC'd before being stored in the BOLT 11 invoice.
 	pub payment_metadata: Option<Vec<u8>>,
 }
 
@@ -15023,6 +15022,7 @@ impl<
 			|amount_msats, relative_expiry| {
 				self.create_inbound_payment(Some(amount_msats), relative_expiry, None, None)
 					.map_err(|()| Bolt12SemanticError::InvalidAmount)
+					.map(|(preimage, secret, _no_metadata)| (preimage, secret))
 			},
 			None,
 		)?;
@@ -15033,8 +15033,8 @@ impl<
 		Ok(invoice)
 	}
 
-	/// Gets a payment secret and payment hash for use in an invoice given to a third party wishing
-	/// to pay us.
+	/// Gets a payment secret, payment hash, and encrypts the `payment_metadata` 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.
@@ -15065,8 +15065,8 @@ impl<
 	/// [`create_inbound_payment_for_hash`]: Self::create_inbound_payment_for_hash
 	pub fn create_inbound_payment(
 		&self, min_value_msat: Option<u64>, invoice_expiry_delta_secs: u32,
-		min_final_cltv_expiry_delta: Option<u16>, payment_metadata: Option<&[u8]>,
-	) -> Result<(PaymentHash, PaymentSecret), ()> {
+		min_final_cltv_expiry_delta: Option<u16>, payment_metadata: Option<Vec<u8>>,
+	) -> Result<(PaymentHash, PaymentSecret, Option<Vec<u8>>), ()> {
 		inbound_payment::create(
 			&self.inbound_payment_key,
 			min_value_msat,
@@ -15078,8 +15078,8 @@ impl<
 		)
 	}
 
-	/// Gets a [`PaymentSecret`] for a given [`PaymentHash`], for which the payment preimage is
-	/// stored external to LDK.
+	/// Gets a [`PaymentSecret`] for a given [`PaymentHash`] (for which the payment preimage is
+	/// stored external to LDK) and encrypts the `payment_metadata`.
 	///
 	/// A [`PaymentClaimable`] event will only be generated if the [`PaymentSecret`] matches a
 	/// payment secret fetched via this method or [`create_inbound_payment`], and which is at least
@@ -15115,41 +15115,34 @@ impl<
 	/// 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.
-	///
 	/// Errors if `min_value_msat` is greater than total bitcoin supply.
 	///
-	/// If `min_final_cltv_expiry_delta` is set to some value, then the payment will not be receivable
-	/// on versions of LDK prior to 0.0.114.
-	///
 	/// [`create_inbound_payment`]: Self::create_inbound_payment
 	/// [`PaymentClaimable`]: events::Event::PaymentClaimable
 	pub fn create_inbound_payment_for_hash(
 		&self, payment_hash: PaymentHash, min_value_msat: Option<u64>,
 		invoice_expiry_delta_secs: u32, min_final_cltv_expiry: Option<u16>,
-		payment_metadata: Option<&[u8]>,
-	) -> Result<PaymentSecret, ()> {
+		payment_metadata: Option<Vec<u8>>,
+	) -> Result<(PaymentSecret, Option<Vec<u8>>), ()> {
 		inbound_payment::create_from_hash(
 			&self.inbound_payment_key,
 			min_value_msat,
 			payment_hash,
 			invoice_expiry_delta_secs,
+			&self.entropy_source,
 			self.highest_seen_timestamp.load(Ordering::Acquire) as u64,
 			min_final_cltv_expiry,
 			payment_metadata,
 		)
 	}
 
-	/// Gets an LDK-generated payment preimage from a payment hash, metadata and secret that were
-	/// previously returned from [`create_inbound_payment`].
+	/// Gets an LDK-generated payment preimage from a payment hash and secret and decrypts the
+	/// metadata (if any) that were previously returned from [`create_inbound_payment`].
 	///
 	/// [`create_inbound_payment`]: Self::create_inbound_payment
-	pub fn get_payment_preimage(
+	pub fn get_payment_preimage_decrypt_metadata(
 		&self, payment_hash: PaymentHash, payment_secret: PaymentSecret,
-		payment_metadata: Option<&[u8]>,
+		payment_metadata: Option<&mut [u8]>,
 	) -> Result<PaymentPreimage, APIError> {
 		let expanded_key = &self.inbound_payment_key;
 		inbound_payment::get_payment_preimage(
@@ -17235,7 +17228,9 @@ impl<
 						relative_expiry,
 						None,
 						None,
-					).map_err(|_| Bolt12SemanticError::InvalidAmount)
+					)
+					.map_err(|_| Bolt12SemanticError::InvalidAmount)
+					.map(|(preimage, secret, _no_metadata)| (preimage, secret))
 				};
 
 				let (result, context) = match invoice_request {
@@ -22137,7 +22132,8 @@ pub mod bench {
 				payment_preimage.0[0..8].copy_from_slice(&payment_count.to_le_bytes());
 				payment_count += 1;
 				let payment_hash = PaymentHash(Sha256::hash(&payment_preimage.0[..]).to_byte_array());
-				let payment_secret = $node_b.create_inbound_payment_for_hash(payment_hash, None, 7200, None, None).unwrap();
+				let (payment_secret, _no_payment_metadata) =
+					$node_b.create_inbound_payment_for_hash(payment_hash, None, 7200, None, None).unwrap();
 
 				$node_a.send_payment(payment_hash, RecipientOnionFields::secret_only(payment_secret, 10_000),
 					PaymentId(payment_hash.0),

lightning/src/ln/functional_test_utils.rs

@@ -2800,7 +2800,7 @@ pub fn get_payment_preimage_hash(
 	let payment_preimage = PaymentPreimage([*payment_count; 32]);
 	*payment_count += 1;
 	let payment_hash = PaymentHash(Sha256::hash(&payment_preimage.0[..]).to_byte_array());
-	let payment_secret = recipient
+	let (payment_secret, _) = recipient
 		.node
 		.create_inbound_payment_for_hash(
 			payment_hash,

lightning/src/ln/functional_tests.rs

@@ -293,7 +293,7 @@ pub fn test_duplicate_htlc_different_direction_onchain() {
 	let (payment_preimage, payment_hash, ..) = route_payment(&nodes[0], &[&nodes[1]], 900_000);
 
 	let (route, _, _, _) = get_route_and_payment_hash!(nodes[1], nodes[0], payment_value_msats);
-	let node_a_payment_secret = nodes[0]
+	let (node_a_payment_secret, _) = nodes[0]
 		.node
 		.create_inbound_payment_for_hash(payment_hash, None, 7200, None, None)
 		.unwrap();
@@ -4159,7 +4159,7 @@ pub fn test_duplicate_payment_hash_one_failure_one_success() {
 	let (our_payment_preimage, dup_payment_hash, ..) =
 		route_payment(&nodes[0], &[&nodes[1], &nodes[2], &nodes[3]], 900_000);
 
-	let payment_secret = nodes[4]
+	let (payment_secret, _) = nodes[4]
 		.node
 		.create_inbound_payment_for_hash(dup_payment_hash, None, 7200, None, None)
 		.unwrap();
@@ -4428,13 +4428,13 @@ fn do_test_fail_backwards_unrevoked_remote_announce(deliver_last_raa: bool, anno
 
 	// 2nd HTLC (not added - smaller than dust limit + HTLC tx fee):
 	let path_5: &[&[_]] = &[&[&nodes[2], &nodes[3], &nodes[5]]];
-	let payment_secret =
+	let (payment_secret, _) =
 		nodes[5].node.create_inbound_payment_for_hash(hash_1, None, 7200, None, None).unwrap();
 	let route = route_to_5.clone();
 	send_along_route_with_secret(&nodes[1], route, path_5, dust_limit_msat, hash_1, payment_secret);
 
 	// 3rd HTLC (not added - smaller than dust limit + HTLC tx fee):
-	let payment_secret =
+	let (payment_secret, _) =
 		nodes[5].node.create_inbound_payment_for_hash(hash_2, None, 7200, None, None).unwrap();
 	let route = route_to_5;
 	send_along_route_with_secret(&nodes[1], route, path_5, dust_limit_msat, hash_2, payment_secret);
@@ -4447,12 +4447,12 @@ fn do_test_fail_backwards_unrevoked_remote_announce(deliver_last_raa: bool, anno
 	let (route, _, _, _) = get_route_and_payment_hash!(nodes[1], nodes[5], 1000000);
 
 	// 6th HTLC:
-	let payment_secret =
+	let (payment_secret, _) =
 		nodes[5].node.create_inbound_payment_for_hash(hash_3, None, 7200, None, None).unwrap();
 	send_along_route_with_secret(&nodes[1], route.clone(), path_5, 1000000, hash_3, payment_secret);
 
 	// 7th HTLC:
-	let payment_secret =
+	let (payment_secret, _) =
 		nodes[5].node.create_inbound_payment_for_hash(hash_4, None, 7200, None, None).unwrap();
 	send_along_route_with_secret(&nodes[1], route, path_5, 1000000, hash_4, payment_secret);
 
@@ -4461,7 +4461,7 @@ fn do_test_fail_backwards_unrevoked_remote_announce(deliver_last_raa: bool, anno
 
 	// 9th HTLC (not added - smaller than dust limit + HTLC tx fee):
 	let (route, _, _, _) = get_route_and_payment_hash!(nodes[1], nodes[5], dust_limit_msat);
-	let payment_secret =
+	let (payment_secret, _) =
 		nodes[5].node.create_inbound_payment_for_hash(hash_5, None, 7200, None, None).unwrap();
 	send_along_route_with_secret(&nodes[1], route, path_5, dust_limit_msat, hash_5, payment_secret);
 
@@ -4470,7 +4470,7 @@ fn do_test_fail_backwards_unrevoked_remote_announce(deliver_last_raa: bool, anno
 
 	// 11th HTLC:
 	let (route, _, _, _) = get_route_and_payment_hash!(nodes[1], nodes[5], 1000000);
-	let payment_secret =
+	let (payment_secret, _) =
 		nodes[5].node.create_inbound_payment_for_hash(hash_6, None, 7200, None, None).unwrap();
 	send_along_route_with_secret(&nodes[1], route, path_5, 1000000, hash_6, payment_secret);
 
@@ -6064,7 +6064,7 @@ pub fn test_check_htlc_underpaying() {
 	.unwrap();
 
 	let (_, our_payment_hash, _) = get_payment_preimage_hash(&nodes[0], None, None);
-	let our_payment_secret = nodes[1]
+	let (our_payment_secret, _) = nodes[1]
 		.node
 		.create_inbound_payment_for_hash(our_payment_hash, Some(100_000), 7200, None, None)
 		.unwrap();
@@ -7233,7 +7233,7 @@ pub fn test_preimage_storage() {
 	create_announced_chan_between_nodes(&nodes, 0, 1);
 
 	{
-		let (payment_hash, payment_secret) =
+		let (payment_hash, payment_secret, _) =
 			nodes[1].node.create_inbound_payment(Some(100_000), 7200, None, None).unwrap();
 		let (route, _, _, _) = get_route_and_payment_hash!(nodes[0], nodes[1], 100_000);
 		let onion = RecipientOnionFields::secret_only(payment_secret, 100_000);
@@ -7278,7 +7278,7 @@ pub fn test_bad_secret_hash() {
 
 	let random_hash = PaymentHash([42; 32]);
 	let random_secret = PaymentSecret([43; 32]);
-	let (our_payment_hash, our_payment_secret) =
+	let (our_payment_hash, our_payment_secret, _) =
 		nodes[1].node.create_inbound_payment(Some(100_000), 2, None, None).unwrap();
 	let (route, _, _, _) = get_route_and_payment_hash!(nodes[0], nodes[1], 100_000);
 
@@ -9496,13 +9496,16 @@ fn do_payment_with_custom_min_final_cltv_expiry(valid_delta: bool, use_user_hash
 			get_payment_preimage_hash(&nodes[1], Some(recv_value), Some(min_cltv_expiry_delta));
 		(hash, payment_preimage, payment_secret)
 	} else {
-		let (hash, payment_secret) = nodes[1]
+		let (hash, payment_secret, _) = nodes[1]
 			.node
 			.create_inbound_payment(Some(recv_value), 7200, Some(min_cltv_expiry_delta), None)
 			.unwrap();
 		(
 			hash,
-			nodes[1].node.get_payment_preimage(hash, payment_secret, None).unwrap(),
+			nodes[1]
+				.node
+				.get_payment_preimage_decrypt_metadata(hash, payment_secret, None)
+				.unwrap(),
 			payment_secret,
 		)
 	};