avoid panic when truncating payer_note in UTF-8 code point #3750

TheBlueMatt · 2025-04-26T00:37:15Z

This is #3747 with one trivial code simplification and a fuzzer update which (after a few hundred million iterations) managed to hit the bug.

ldk-reviews-bot · 2025-04-26T00:37:18Z

I've assigned @wpaulino as a reviewer!
I'll wait for their review and will help manage the review process.
Once they submit their review, I'll check if a second reviewer would be helpful.

lightning/src/offers/invoice_request.rs

lightning/src/offers/signer.rs

lightning/src/offers/invoice_request.rs

ldk-reviews-bot · 2025-04-28T00:48:25Z

🔔 1st Reminder

Hey @wpaulino! This PR has been waiting for your review.
Please take a look when you have a chance. If you're unable to review, please let us know so we can find another reviewer.

joostjager

Mostly questions to complete my understanding of this part.

lightning/src/offers/signer.rs

lightning/src/offers/invoice_request.rs

lightning/src/offers/signer.rs

lightning/src/offers/invoice_request.rs

codecov · 2025-04-28T13:36:19Z

Codecov Report

Attention: Patch coverage is 94.54545% with 3 lines in your changes missing coverage. Please review.

Project coverage is 89.38%. Comparing base (5dcd6c4) to head (5d1a197).
Report is 5 commits behind head on main.

Files with missing lines	Patch %	Lines
lightning/src/offers/signer.rs	81.25%	2 Missing and 1 partial ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #3750      +/-   ##
==========================================
+ Coverage   89.13%   89.38%   +0.24%     
==========================================
  Files         157      157              
  Lines      123851   125812    +1961     
  Branches   123851   125812    +1961     
==========================================
+ Hits       110398   112459    +2061     
+ Misses      10779    10713      -66     
+ Partials     2674     2640      -34

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

joostjager

LGTM. Can keep talking in open threads about background info.

jkczyz · 2025-04-28T16:16:41Z

lightning/src/offers/signer.rs

+		// 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.


Could we simply make InvoiceRequestFields accessible from the unverified InvoiceRequest when fuzzing?

I kinda liked the idea of running through the same flow as we do in production, including the verfiy?

Hmmm... but most everything is being skipped. Like we aren't adding any nonce bytes or TLV records as input to the HMAC by returning early. We could at least do that and not change verify_metadata.

Mmm, fair, I pushed an updated version that uses more common path by having a default nonce.

`String::truncate` takes a byte index but panics if we split in the middle of a UTF-8 codepoint. Sadly, in `InvoiceRequest::fields` we want to tuncate the payer note to a maximum of 512 bytes, which may be in the middle of a UTF-8 codepoint and can cause panic. Here we iterate over the bytes in the string until we find one not in the middle of a UTF-8 codepoint and then split the string there.

TheBlueMatt · 2025-04-28T16:49:10Z

Squashed with a cleaned-up test:

$ git diff-tree -U1 d6886c5b2 a4fd1371e
diff --git a/lightning/src/offers/invoice_request.rs b/lightning/src/offers/invoice_request.rs
index 0b0a029b3..2e399738b 100644
--- a/lightning/src/offers/invoice_request.rs
+++ b/lightning/src/offers/invoice_request.rs
@@ -3031,11 +3031,11 @@ mod tests {
 		let s = String::from("❤️");
-		for new_len in 0..(s.len() + 5) {
-			if new_len >= s.len() {
-				assert_eq!(s, string_truncate_safe(s.clone(), new_len));
-			} else if (3..s.len()).contains(&new_len) {
-				assert_eq!("❤", string_truncate_safe(s.clone(), new_len));
-			} else {
-				assert_eq!("", string_truncate_safe(s.clone(), new_len));
-			}
-		}
+		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));

jkczyz · 2025-04-28T17:49:29Z

lightning/src/offers/signer.rs

+		// 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.


Hmmm... but most everything is being skipped. Like we aren't adding any nonce bytes or TLV records as input to the HMAC by returning early. We could at least do that and not change verify_metadata.

jkczyz · 2025-04-28T17:49:47Z

lightning/src/offers/signer.rs

+		let mut ok = metadata.len() == Nonce::LENGTH + Sha256::LEN
+			&& fixed_time_eq(&metadata[Nonce::LENGTH..], &hmac.to_byte_array());
+		#[cfg(fuzzing)]
+		if metadata[0] & 1 == 0 {


Won't this panic if the metadata is empty?

Grr, rewrote it too many times...

In the next commit we attempt to verify `InvoiceRequest`s when fuzzing so that we can test fetching the `InvoiceRequestFields`, but its useful to allow the verification to succeed more often first, which we do here.

TheBlueMatt · 2025-04-28T18:14:21Z

$ git diff-tree -U1 a4fd1371e 5d1a197cf
diff --git a/lightning/src/offers/signer.rs b/lightning/src/offers/signer.rs
index d958b68b1..329b90d20 100644
--- a/lightning/src/offers/signer.rs
+++ b/lightning/src/offers/signer.rs
@@ -411,3 +411,3 @@ fn verify_metadata<T: secp256k1::Signing>(
 		#[cfg(fuzzing)]
-		if metadata[0] & 1 == 0 {
+		if metadata.is_empty() || metadata[0] & 1 == 0 {
 			ok = true;
@@ -429,3 +429,3 @@ fn hmac_for_message<'a>(

-	if metadata.len() < Nonce::LENGTH {
+	let nonce = if metadata.len() < Nonce::LENGTH {
 		// In fuzzing its relatively challenging for the fuzzer to find cases where we have issues
@@ -433,12 +433,8 @@ fn hmac_for_message<'a>(
 		// nonce.
-		if cfg!(fuzzing) {
-			return Ok(hmac);
-		} else {
+		if !cfg!(fuzzing) {
 			return Err(());
 		}
-	}
-
-	let nonce = match Nonce::try_from(&metadata[..Nonce::LENGTH]) {
-		Ok(nonce) => nonce,
-		Err(_) => return Err(()),
+		Nonce::try_from(&[42; Nonce::LENGTH][..]).unwrap()
+	} else {
+		Nonce::try_from(&metadata[..Nonce::LENGTH])?
 	};

fuzz/src/invoice_request_deser.rs

jkczyz

LGTM. Feel free to squash.

This should allow us to reach the panic from two commits ago from the fuzzer.

TheBlueMatt · 2025-04-28T20:26:26Z

Squashed again, thanks.

joostjager · 2025-04-28T22:27:05Z

fuzz/src/invoice_request_deser.rs

+		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()


Somehow I expected this call not here directly, but a fuzzer that indirectly hits it via channel manager. But that would be a whole different fuzz test.

Yea, hitting something that requires eg 3 specific TLVs to line up as a part of a much, much larger target is probably an infeasible level of complexity for a fuzzer.

TheBlueMatt · 2025-04-29T20:08:50Z

Backported in #3757

v0.1.3 - Apr 30, 2025 - "Routing Unicode in 2025" Bug Fixes ========= * `Event::InvoiceReceived` is now only generated once for each `Bolt12Invoice` received matching a pending outbound payment. Previously it would be provided each time we received an invoice, which may happen many times if the sender sends redundant messages to improve success rates (lightningdevkit#3658). * LDK's router now more fully saturates paths which are subject to HTLC maximum restrictions after the first hop. In some rare cases this can result in finding paths when it would previously spuriously decide it cannot find enough diverse paths (lightningdevkit#3707, lightningdevkit#3755). Security ======== 0.1.3 fixes a denial-of-service vulnerability which cause a crash of an LDK-based node if an attacker has access to a valid `Bolt12Offer` which the LDK-based node created. * A malicious payer which requests a BOLT 12 Invoice from an LDK-based node (via the `Bolt12InvoiceRequest` message) can cause the panic of the LDK-based node due to the way `String::truncate` handles UTF-8 codepoints. The codepath can only be reached once the received `Botlt12InvoiceRequest` has been authenticated to be based on a valid `Bolt12Offer` which the same LDK-based node issued (lightningdevkit#3747, lightningdevkit#3750).

TheBlueMatt added the backport 0.1 label Apr 26, 2025

TheBlueMatt mentioned this pull request Apr 26, 2025

offers: avoid panic when truncating payer_note in UTF-8 code point #3747

Closed

TheBlueMatt force-pushed the 2025-04-utf8-split-panic branch from 06a6e0e to 92f1bf4 Compare April 26, 2025 00:42

ldk-reviews-bot requested a review from wpaulino April 26, 2025 00:47

joostjager reviewed Apr 26, 2025

View reviewed changes

lightning/src/offers/invoice_request.rs Show resolved Hide resolved

lightning/src/offers/signer.rs Outdated Show resolved Hide resolved

lightning/src/offers/signer.rs Show resolved Hide resolved

lightning/src/offers/invoice_request.rs Show resolved Hide resolved

TheBlueMatt force-pushed the 2025-04-utf8-split-panic branch from 92f1bf4 to 20c5ac3 Compare April 28, 2025 00:42

joostjager reviewed Apr 28, 2025

View reviewed changes

TheBlueMatt force-pushed the 2025-04-utf8-split-panic branch from 20c5ac3 to d6886c5 Compare April 28, 2025 13:27

joostjager previously approved these changes Apr 28, 2025

View reviewed changes

jkczyz reviewed Apr 28, 2025

View reviewed changes

TheBlueMatt dismissed joostjager’s stale review via a4fd137 April 28, 2025 16:48

TheBlueMatt force-pushed the 2025-04-utf8-split-panic branch from d6886c5 to a4fd137 Compare April 28, 2025 16:48

joostjager previously approved these changes Apr 28, 2025

View reviewed changes

wpaulino previously approved these changes Apr 28, 2025

View reviewed changes

jkczyz reviewed Apr 28, 2025

View reviewed changes

Make it easier for the fuzzer to get a VerifiedInvoiceRequest

eb7f6d9

In the next commit we attempt to verify `InvoiceRequest`s when fuzzing so that we can test fetching the `InvoiceRequestFields`, but its useful to allow the verification to succeed more often first, which we do here.

TheBlueMatt dismissed stale reviews from wpaulino and joostjager via 5d1a197 April 28, 2025 18:13

TheBlueMatt force-pushed the 2025-04-utf8-split-panic branch from a4fd137 to 5d1a197 Compare April 28, 2025 18:13

jkczyz reviewed Apr 28, 2025

View reviewed changes

fuzz/src/invoice_request_deser.rs Outdated Show resolved Hide resolved

jkczyz reviewed Apr 28, 2025

View reviewed changes

Fuzz fetching InvoiceRequestFields from VerifiedInvoiceRequests

5309176

This should allow us to reach the panic from two commits ago from the fuzzer.

TheBlueMatt force-pushed the 2025-04-utf8-split-panic branch from 3e9d966 to 5309176 Compare April 28, 2025 20:26

wpaulino approved these changes Apr 28, 2025

View reviewed changes

jkczyz approved these changes Apr 28, 2025

View reviewed changes

joostjager approved these changes Apr 28, 2025

View reviewed changes

TheBlueMatt merged commit 559a784 into lightningdevkit:main Apr 28, 2025
26 of 27 checks passed

TheBlueMatt removed the backport 0.1 label Apr 29, 2025

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

avoid panic when truncating payer_note in UTF-8 code point #3750

avoid panic when truncating payer_note in UTF-8 code point #3750

TheBlueMatt commented Apr 26, 2025

ldk-reviews-bot commented Apr 26, 2025 •

edited

Loading

ldk-reviews-bot commented Apr 28, 2025

joostjager left a comment

codecov bot commented Apr 28, 2025 •

edited

Loading

joostjager left a comment

jkczyz Apr 28, 2025

TheBlueMatt Apr 28, 2025

jkczyz Apr 28, 2025

TheBlueMatt Apr 28, 2025

TheBlueMatt commented Apr 28, 2025

jkczyz Apr 28, 2025

jkczyz Apr 28, 2025

TheBlueMatt Apr 28, 2025

TheBlueMatt commented Apr 28, 2025

jkczyz left a comment

TheBlueMatt commented Apr 28, 2025

joostjager Apr 28, 2025

TheBlueMatt Apr 28, 2025

TheBlueMatt commented Apr 29, 2025

avoid panic when truncating payer_note in UTF-8 code point #3750

avoid panic when truncating payer_note in UTF-8 code point #3750

Conversation

TheBlueMatt commented Apr 26, 2025

ldk-reviews-bot commented Apr 26, 2025 • edited Loading

ldk-reviews-bot commented Apr 28, 2025

joostjager left a comment

Choose a reason for hiding this comment

codecov bot commented Apr 28, 2025 • edited Loading

Codecov Report

joostjager left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

TheBlueMatt commented Apr 28, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

TheBlueMatt commented Apr 28, 2025

jkczyz left a comment

Choose a reason for hiding this comment

TheBlueMatt commented Apr 28, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

TheBlueMatt commented Apr 29, 2025

ldk-reviews-bot commented Apr 26, 2025 •

edited

Loading

codecov bot commented Apr 28, 2025 •

edited

Loading