diff --git a/fuzz/src/invoice_request_deser.rs b/fuzz/src/invoice_request_deser.rs index 37668c1d801..beff5c21a0b 100644 --- a/fuzz/src/invoice_request_deser.rs +++ b/fuzz/src/invoice_request_deser.rs @@ -85,16 +85,26 @@ fn build_response( 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]), diff --git a/lightning/src/offers/invoice_request.rs b/lightning/src/offers/invoice_request.rs index cdd9cff2177..2e399738bae 100644 --- a/lightning/src/offers/invoice_request.rs +++ b/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(&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,6 +2982,12 @@ 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() @@ -2954,7 +2995,7 @@ mod tests { .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)); + } + } + } } diff --git a/lightning/src/offers/signer.rs b/lightning/src/offers/signer.rs index 84540fc13e0..329b90d2076 100644 --- a/lightning/src/offers/signer.rs +++ b/lightning/src/offers/signer.rs @@ -393,19 +393,30 @@ fn verify_metadata( 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(()) } } @@ -413,16 +424,20 @@ fn hmac_for_message<'a>( metadata: &[u8], expanded_key: &ExpandedKey, iv_bytes: &[u8; IV_LEN], tlv_stream: impl core::iter::Iterator>, ) -> Result, ()> { - 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 {