Fuzz process onion failure #3683

joostjager · 2025-03-25T21:00:47Z

Adds a fuzzer for process_onion_failure. The fuzzer found this regression: #3686

ldk-reviews-bot · 2025-03-25T21:00:50Z

👋 Thanks for assigning @arik-so 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.

joostjager · 2025-03-25T21:37:55Z

lightning/src/ln/onion_utils.rs

@@ -1129,7 +1142,7 @@ where
 		let mut hmac = HmacEngine::<Sha256>::new(&um);
 		hmac.input(&encrypted_packet.data[32..]);

-		if !fixed_time_eq(&Hmac::from_engine(hmac).to_byte_array(), &encrypted_packet.data[..32]) {
+		if &Hmac::from_engine(hmac).to_byte_array() != &encrypted_packet.data[..32] {


Required for fuzzer, and fixed_time_eq not needed here at the sender side.

I'd kinda rather do this upstream at rust-bitcoin/rust-bitcoin#4289 so we don't have to think about it (tho its also totally the case that our next-hop can probably identify whether we were the sender based on our timing already and there isn't much we can do to fix that, making this constant time is not gonna help lol).

I don't see how using equal time here would help the next hop identify that we are the sender? When we get here, there is no further comm with the next hop anymore. Or you mean the time until the next attempt for this payment that may potentially flow through the same next hop? For that, I'd think there is so much else also happening such as pathfinding, that this really does not matter.

If equal time cmp isn't adding anything for multiple reasons, then I'd say let's do the faster normal cmp?

The upstream issue is of course still useful for fuzzing of other uses of fixed_time_eq that are timing sensitive.

So if making this constant time isn't gonna mitigate detection, what's the issue with just making it a regular comparison? And alternatively, if there's enough value to keep using fixed_time_eq, would it make sense to use one or the other based on a cfg flag so we use regular comparisons only in a fuzz context?

Oops, missed this. I believe we decode inline when processing a message from a peer. Thus they can measure how long it takes us to process the message to figure out which parts of the code they hit. Given we're iterating the onion its vaguely reminiscent of Bleichenbacher's, but we already exit the loop early so its not like we're particularly robust here. Again, the right fix is to do things like this in a background task rather than in a message processing function.

codecov · 2025-03-26T15:01:38Z

Codecov Report

Attention: Patch coverage is 44.44444% with 10 lines in your changes missing coverage. Please review.

Project coverage is 89.22%. Comparing base (030a784) to head (52b665b).

Files with missing lines	Patch %	Lines
lightning/src/ln/onion_utils.rs	44.44%	10 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #3683      +/-   ##
==========================================
- Coverage   89.25%   89.22%   -0.03%     
==========================================
  Files         155      155              
  Lines      119959   119951       -8     
  Branches   119959   119951       -8     
==========================================
- Hits       107069   107029      -40     
- Misses      10276    10298      +22     
- Partials     2614     2624      +10

☔ 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 · 2025-03-26T15:57:58Z

fuzz/src/process_onion_failure.rs

+			pubkey: get_pubkey!(),
+			node_features: NodeFeatures::empty(),
+			short_channel_id: get_u64!(),
+			channel_features: ChannelFeatures::empty(),


I didn't bother setting the features as they aren't used for the onion failure processing. Mainly because it seemed not straight-fwd to do it.

TheBlueMatt

Fuzzer looks good to me, basically. Left some nits where we could maybe tweak things but given enough CPU probably this fuzzer would do fine.

TheBlueMatt · 2025-03-26T18:51:58Z

fuzz/src/process_onion_failure.rs

+	let session_priv = match SecretKey::from_slice(get_slice!(32)) {
+		Ok(val) => val,
+		Err(_) => return,
+	};
+
+	let payment_id = match get_slice!(32).try_into() {
+		Ok(val) => PaymentId(val),
+		Err(_) => return,
+	};


These we might just want to hard-code. It shouldn't be the case that we ever care about the value, and making them a part of the input just gives the fuzzer more stuff to twiddle that does nothing but invalidate the rest of the input.

Hard-coded keys and things that aren't used in process_onion_failure.

fuzz/src/process_onion_failure.rs

TheBlueMatt · 2025-03-26T19:06:57Z

fuzz/src/process_onion_failure.rs

+	};
+
+	let mut hops = Vec::<RouteHop>::new();
+	let hop_count = get_slice!(1)[0] as usize;


I kinda wonder if we shouldn't make this less flexible - either always do 20 (21?) or have a flag that selects between 20 (21?) and 1 or so. Probably there are no bugs that are only reachable if we have 5/7/10/etc hops.

Hm, but then there are also the trampoline hops and blinded path hops. I think I am ok with just %30 all of them

TheBlueMatt · 2025-03-26T19:53:19Z

fuzz/src/process_onion_failure.rs

+	let failure_len = get_u16!();
+	let encrypted_packet = OnionErrorPacket { data: get_slice!(failure_len).into() };


Should we replace this with just reading the OnionErrorPacket using standard Readable from the remainder of the stream, rather than reading a length prefix?

I don't think there is a standard readable defined for OnionErrorPacket?

Ah, I guess we'd have to read an DecodedOnionErrorPacket?

Read that, and then write it out again to get a raw byte array. I'm not sure if it makes it better.

TheBlueMatt

Basically LGTM, needs commit message and squash.

Preparatory commit that exposes types that are related to onion failure processing to the fuzzing targets.

Because this method is used by the sender of the payment after the htlc has been resolved, there is no information that can leak to downstream nodes. Revert to a regular comparison for better performance and easier fuzzing.

fuzz/src/process_onion_failure.rs

Adds a fuzz target that exercises the processing of a failure message that is received by the sender of a payment. Fuzzing this is important because the input data is coming directly off the wire.

arik-so

Looks good to me!

joostjager force-pushed the fuzz-process-onion-failure branch 2 times, most recently from 7bdde66 to fe4bcd7 Compare March 25, 2025 21:35

joostjager commented Mar 25, 2025

View reviewed changes

joostjager force-pushed the fuzz-process-onion-failure branch 2 times, most recently from dba48e0 to 52b665b Compare March 26, 2025 14:52

joostjager force-pushed the fuzz-process-onion-failure branch from 52b665b to 1dd2d94 Compare March 26, 2025 15:55

joostjager commented Mar 26, 2025

View reviewed changes

joostjager force-pushed the fuzz-process-onion-failure branch 2 times, most recently from f37db7b to 4f9618a Compare March 26, 2025 16:04

TheBlueMatt reviewed Mar 26, 2025

View reviewed changes

joostjager force-pushed the fuzz-process-onion-failure branch 2 times, most recently from c10a9e5 to ebf4f89 Compare March 27, 2025 01:48

joostjager requested a review from TheBlueMatt March 27, 2025 11:02

joostjager marked this pull request as ready for review March 27, 2025 12:57

joostjager mentioned this pull request Mar 27, 2025

Attributable failures #2256

Merged

TheBlueMatt reviewed Mar 27, 2025

View reviewed changes

joostjager force-pushed the fuzz-process-onion-failure branch from ebf4f89 to 64cd40c Compare March 27, 2025 18:03

joostjager requested a review from arik-so March 27, 2025 18:04

joostjager added 2 commits March 27, 2025 14:21

Make types available for fuzzing

07c5b99

Preparatory commit that exposes types that are related to onion failure processing to the fuzzing targets.

Use regular hmac comparison in onion failure processing

991caa6

Because this method is used by the sender of the payment after the htlc has been resolved, there is no information that can leak to downstream nodes. Revert to a regular comparison for better performance and easier fuzzing.

joostjager force-pushed the fuzz-process-onion-failure branch from 64cd40c to fcbb658 Compare March 27, 2025 18:23

arik-so reviewed Mar 27, 2025

View reviewed changes

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

Fuzzing for process_onion_failure

17737be

Adds a fuzz target that exercises the processing of a failure message that is received by the sender of a payment. Fuzzing this is important because the input data is coming directly off the wire.

joostjager force-pushed the fuzz-process-onion-failure branch from fcbb658 to 17737be Compare March 27, 2025 19:05

joostjager requested a review from arik-so March 27, 2025 19:06

arik-so approved these changes Mar 27, 2025

View reviewed changes

joostjager merged commit 0d95565 into lightningdevkit:main Mar 28, 2025
26 of 27 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fuzz process onion failure #3683

Fuzz process onion failure #3683

joostjager commented Mar 25, 2025 •

edited

Loading

ldk-reviews-bot commented Mar 25, 2025 •

edited

Loading

joostjager Mar 25, 2025

TheBlueMatt Mar 26, 2025

joostjager Mar 26, 2025

arik-so Mar 27, 2025

TheBlueMatt Mar 28, 2025

codecov bot commented Mar 26, 2025

joostjager Mar 26, 2025

TheBlueMatt left a comment

TheBlueMatt Mar 26, 2025

joostjager Mar 26, 2025

TheBlueMatt Mar 26, 2025

joostjager Mar 26, 2025

TheBlueMatt Mar 26, 2025

joostjager Mar 26, 2025

TheBlueMatt Mar 27, 2025

joostjager Mar 27, 2025

TheBlueMatt left a comment

arik-so left a comment

		let failure_len = get_u16!();
		let encrypted_packet = OnionErrorPacket { data: get_slice!(failure_len).into() };

Fuzz process onion failure #3683

Fuzz process onion failure #3683

Conversation

joostjager commented Mar 25, 2025 • edited Loading

ldk-reviews-bot commented Mar 25, 2025 • edited Loading

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

codecov bot commented Mar 26, 2025

Codecov Report

Choose a reason for hiding this comment

TheBlueMatt 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

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 left a comment

Choose a reason for hiding this comment

arik-so left a comment

Choose a reason for hiding this comment

joostjager commented Mar 25, 2025 •

edited

Loading

ldk-reviews-bot commented Mar 25, 2025 •

edited

Loading