Merge pull request #3750 from TheBlueMatt/2025-04-utf8-split-panic

avoid panic when truncating payer_note in UTF-8 code point

Author: TheBlueMatt

fuzz/src/invoice_request_deser.rs

@@ -85,16 +85,26 @@ fn build_response<T: secp256k1::Signing + secp256k1::Verification>(
 	let expanded_key = ExpandedKey::new([42; 32]);
 	let entropy_source = Randomness {};
 	let nonce = Nonce::from_entropy_source(&entropy_source);
+
+	let invoice_request_fields =
+		if let Ok(ver) = invoice_request.clone().verify_using_metadata(&expanded_key, secp_ctx) {
+			// Previously we had a panic where we'd truncate the payer note possibly cutting a
+			// Unicode character in two here, so try to fetch fields if we can validate.
+			ver.fields()
+		} else {
+			InvoiceRequestFields {
+				payer_signing_pubkey: invoice_request.payer_signing_pubkey(),
+				quantity: invoice_request.quantity(),
+				payer_note_truncated: invoice_request
+					.payer_note()
+					.map(|s| UntrustedString(s.to_string())),
+				human_readable_name: None,
+			}
+		};
+
 	let payment_context = PaymentContext::Bolt12Offer(Bolt12OfferContext {
 		offer_id: OfferId([42; 32]),
-		invoice_request: InvoiceRequestFields {
-			payer_signing_pubkey: invoice_request.payer_signing_pubkey(),
-			quantity: invoice_request.quantity(),
-			payer_note_truncated: invoice_request
-				.payer_note()
-				.map(|s| UntrustedString(s.to_string())),
-			human_readable_name: None,
-		},
+		invoice_request: invoice_request_fields,
 	});
 	let payee_tlvs = UnauthenticatedReceiveTlvs {
 		payment_secret: PaymentSecret([42; 32]),

lightning/src/offers/invoice_request.rs

@@ -989,7 +989,14 @@ impl VerifiedInvoiceRequest {
 		InvoiceWithDerivedSigningPubkeyBuilder
 	);
 
-	pub(crate) fn fields(&self) -> InvoiceRequestFields {
+	/// Fetch the [`InvoiceRequestFields`] for this verified invoice.
+	///
+	/// These are fields which we expect to be useful when receiving a payment for this invoice
+	/// request, and include the returned [`InvoiceRequestFields`] in the
+	/// [`PaymentContext::Bolt12Offer`].
+	///
+	/// [`PaymentContext::Bolt12Offer`]: crate::blinded_path::payment::PaymentContext::Bolt12Offer
+	pub fn fields(&self) -> InvoiceRequestFields {
 		let InvoiceRequestContents {
 			payer_signing_pubkey,
 			inner: InvoiceRequestContentsWithoutPayerSigningPubkey { quantity, payer_note, .. },
@@ -998,15 +1005,37 @@ impl VerifiedInvoiceRequest {
 		InvoiceRequestFields {
 			payer_signing_pubkey: *payer_signing_pubkey,
 			quantity: *quantity,
-			payer_note_truncated: payer_note.clone().map(|mut s| {
-				s.truncate(PAYER_NOTE_LIMIT);
-				UntrustedString(s)
-			}),
+			payer_note_truncated: payer_note
+				.clone()
+				// Truncate the payer note to `PAYER_NOTE_LIMIT` bytes, rounding
+				// down to the nearest valid UTF-8 code point boundary.
+				.map(|s| UntrustedString(string_truncate_safe(s, PAYER_NOTE_LIMIT))),
 			human_readable_name: self.offer_from_hrn().clone(),
 		}
 	}
 }
 
+/// `String::truncate(new_len)` panics if you split inside a UTF-8 code point,
+/// which would leave the `String` containing invalid UTF-8. This function will
+/// instead truncate the string to the next smaller code point boundary so the
+/// truncated string always remains valid UTF-8.
+///
+/// This can still split a grapheme cluster, but that's probably fine.
+/// We'd otherwise have to pull in the `unicode-segmentation` crate and its big
+/// unicode tables to find the next smaller grapheme cluster boundary.
+fn string_truncate_safe(mut s: String, new_len: usize) -> String {
+	// Finds the largest byte index `x` not exceeding byte index `index` where
+	// `s.is_char_boundary(x)` is true.
+	// TODO(phlip9): remove when `std::str::floor_char_boundary` stabilizes.
+	let truncated_len = if new_len >= s.len() {
+		s.len()
+	} else {
+		(0..=new_len).rev().find(|idx| s.is_char_boundary(*idx)).unwrap_or(0)
+	};
+	s.truncate(truncated_len);
+	s
+}
+
 impl InvoiceRequestContents {
 	pub(super) fn metadata(&self) -> &[u8] {
 		self.inner.metadata()
@@ -1382,8 +1411,13 @@ pub struct InvoiceRequestFields {
 }
 
 /// The maximum number of characters included in [`InvoiceRequestFields::payer_note_truncated`].
+#[cfg(not(fuzzing))]
 pub const PAYER_NOTE_LIMIT: usize = 512;
 
+/// The maximum number of characters included in [`InvoiceRequestFields::payer_note_truncated`].
+#[cfg(fuzzing)]
+pub const PAYER_NOTE_LIMIT: usize = 8;
+
 impl Writeable for InvoiceRequestFields {
 	fn write<W: Writer>(&self, writer: &mut W) -> Result<(), io::Error> {
 		write_tlv_fields!(writer, {
@@ -1426,6 +1460,7 @@ mod tests {
 	use crate::ln::inbound_payment::ExpandedKey;
 	use crate::ln::msgs::{DecodeError, MAX_VALUE_MSAT};
 	use crate::offers::invoice::{Bolt12Invoice, SIGNATURE_TAG as INVOICE_SIGNATURE_TAG};
+	use crate::offers::invoice_request::string_truncate_safe;
 	use crate::offers::merkle::{self, SignatureTlvStreamRef, TaggedHash, TlvStream};
 	use crate::offers::nonce::Nonce;
 	#[cfg(not(c_bindings))]
@@ -2947,14 +2982,20 @@ mod tests {
 			.unwrap();
 		assert_eq!(offer.issuer_signing_pubkey(), Some(node_id));
 
+		// UTF-8 payer note that we can't naively `.truncate(PAYER_NOTE_LIMIT)`
+		// because it would split a multi-byte UTF-8 code point.
+		let payer_note = "❤️".repeat(86);
+		assert_eq!(payer_note.len(), PAYER_NOTE_LIMIT + 4);
+		let expected_payer_note = "❤️".repeat(85);
+
 		let invoice_request = offer
 			.request_invoice(&expanded_key, nonce, &secp_ctx, payment_id)
 			.unwrap()
 			.chain(Network::Testnet)
 			.unwrap()
 			.quantity(1)
 			.unwrap()
-			.payer_note("0".repeat(PAYER_NOTE_LIMIT * 2))
+			.payer_note(payer_note)
 			.build_and_sign()
 			.unwrap();
 		match invoice_request.verify_using_metadata(&expanded_key, &secp_ctx) {
@@ -2966,7 +3007,7 @@ mod tests {
 					InvoiceRequestFields {
 						payer_signing_pubkey: invoice_request.payer_signing_pubkey(),
 						quantity: Some(1),
-						payer_note_truncated: Some(UntrustedString("0".repeat(PAYER_NOTE_LIMIT))),
+						payer_note_truncated: Some(UntrustedString(expected_payer_note)),
 						human_readable_name: None,
 					}
 				);
@@ -2981,4 +3022,31 @@ mod tests {
 			Err(_) => panic!("unexpected error"),
 		}
 	}
+
+	#[test]
+	fn test_string_truncate_safe() {
+		// We'll correctly truncate to the nearest UTF-8 code point boundary:
+		// ❤      variation-selector
+		// e29da4 efb88f
+		let s = String::from("❤️");
+		assert_eq!(s.len(), 6);
+		assert_eq!(s, string_truncate_safe(s.clone(), 7));
+		assert_eq!(s, string_truncate_safe(s.clone(), 6));
+		assert_eq!("❤", string_truncate_safe(s.clone(), 5));
+		assert_eq!("❤", string_truncate_safe(s.clone(), 4));
+		assert_eq!("❤", string_truncate_safe(s.clone(), 3));
+		assert_eq!("", string_truncate_safe(s.clone(), 2));
+		assert_eq!("", string_truncate_safe(s.clone(), 1));
+		assert_eq!("", string_truncate_safe(s.clone(), 0));
+
+		// Every byte in an ASCII string is also a full UTF-8 code point.
+		let s = String::from("my ASCII string!");
+		for new_len in 0..(s.len() + 5) {
+			if new_len >= s.len() {
+				assert_eq!(s, string_truncate_safe(s.clone(), new_len));
+			} else {
+				assert_eq!(s[..new_len], string_truncate_safe(s.clone(), new_len));
+			}
+		}
+	}
 }

lightning/src/offers/signer.rs

@@ -393,36 +393,51 @@ fn verify_metadata<T: secp256k1::Signing>(
 			secp_ctx,
 			&SecretKey::from_slice(hmac.as_byte_array()).unwrap(),
 		);
-		if fixed_time_eq(&signing_pubkey.serialize(), &derived_keys.public_key().serialize()) {
+		#[allow(unused_mut)]
+		let mut ok = fixed_time_eq(&signing_pubkey.serialize(), &derived_keys.public_key().serialize());
+		#[cfg(fuzzing)]
+		if metadata[0] & 1 == 0 {
+			ok = true;
+		}
+		if ok {
 			Ok(Some(derived_keys))
 		} else {
 			Err(())
 		}
-	} else if metadata[Nonce::LENGTH..].len() == Sha256::LEN {
-		if fixed_time_eq(&metadata[Nonce::LENGTH..], &hmac.to_byte_array()) {
+	} else {
+		#[allow(unused_mut)]
+		let mut ok = metadata.len() == Nonce::LENGTH + Sha256::LEN
+			&& fixed_time_eq(&metadata[Nonce::LENGTH..], &hmac.to_byte_array());
+		#[cfg(fuzzing)]
+		if metadata.is_empty() || metadata[0] & 1 == 0 {
+			ok = true;
+		}
+		if ok {
 			Ok(None)
 		} else {
 			Err(())
 		}
-	} else {
-		Err(())
 	}
 }
 
 fn hmac_for_message<'a>(
 	metadata: &[u8], expanded_key: &ExpandedKey, iv_bytes: &[u8; IV_LEN],
 	tlv_stream: impl core::iter::Iterator<Item = TlvRecord<'a>>,
 ) -> Result<HmacEngine<Sha256>, ()> {
-	if metadata.len() < Nonce::LENGTH {
-		return Err(());
-	}
-
-	let nonce = match Nonce::try_from(&metadata[..Nonce::LENGTH]) {
-		Ok(nonce) => nonce,
-		Err(_) => return Err(()),
-	};
 	let mut hmac = expanded_key.hmac_for_offer();
 	hmac.input(iv_bytes);
+
+	let nonce = if metadata.len() < Nonce::LENGTH {
+		// In fuzzing its relatively challenging for the fuzzer to find cases where we have issues
+		// in a BOLT 12 object but also have a right-sized nonce. So instead we allow any size
+		// nonce.
+		if !cfg!(fuzzing) {
+			return Err(());
+		}
+		Nonce::try_from(&[42; Nonce::LENGTH][..]).unwrap()
+	} else {
+		Nonce::try_from(&metadata[..Nonce::LENGTH])?
+	};
 	hmac.input(&nonce.0);
 
 	for record in tlv_stream {