From 07c5b99a88e7074b5ef9591a5787449b7de38a5a Mon Sep 17 00:00:00 2001 From: Joost Jager Date: Thu, 27 Mar 2025 14:20:21 -0400 Subject: [PATCH 1/3] Make types available for fuzzing Preparatory commit that exposes types that are related to onion failure processing to the fuzzing targets. --- lightning/src/ln/channelmanager.rs | 78 ++++++++++++++++-------------- lightning/src/ln/mod.rs | 3 ++ lightning/src/ln/msgs.rs | 14 +++--- lightning/src/ln/onion_utils.rs | 28 +++++++---- 4 files changed, 71 insertions(+), 52 deletions(-) diff --git a/lightning/src/ln/channelmanager.rs b/lightning/src/ln/channelmanager.rs index fab15bfea28..55a5f0d4aea 100644 --- a/lightning/src/ln/channelmanager.rs +++ b/lightning/src/ln/channelmanager.rs @@ -409,27 +409,6 @@ pub enum BlindedFailure { FromBlindedNode, } -/// Tracks the inbound corresponding to an outbound HTLC -#[derive(Clone, Debug, Hash, PartialEq, Eq)] -pub(crate) struct HTLCPreviousHopData { - // Note that this may be an outbound SCID alias for the associated channel. - short_channel_id: u64, - user_channel_id: Option, - htlc_id: u64, - incoming_packet_shared_secret: [u8; 32], - phantom_shared_secret: Option<[u8; 32]>, - blinded_failure: Option, - channel_id: ChannelId, - - // These fields are consumed by `claim_funds_from_hop()` when updating a force-closed backwards - // channel with a preimage provided by the forward channel. - outpoint: OutPoint, - counterparty_node_id: Option, - /// Used to preserve our backwards channel by failing back in case an HTLC claim in the forward - /// channel remains unconfirmed for too long. - cltv_expiry: Option, -} - #[derive(PartialEq, Eq)] enum OnionPayload { /// Indicates this incoming onion payload is for the purpose of paying an invoice. @@ -674,21 +653,50 @@ impl_writeable_tlv_based_enum!(SentHTLCId, }, ); - -/// Tracks the inbound corresponding to an outbound HTLC -#[allow(clippy::derive_hash_xor_eq)] // Our Hash is faithful to the data, we just don't have SecretKey::hash -#[derive(Clone, Debug, PartialEq, Eq)] -pub(crate) enum HTLCSource { - PreviousHopData(HTLCPreviousHopData), - OutboundRoute { - path: Path, - session_priv: SecretKey, - /// Technically we can recalculate this from the route, but we cache it here to avoid - /// doing a double-pass on route when we get a failure back - first_hop_htlc_msat: u64, - payment_id: PaymentId, - }, +mod fuzzy_channelmanager { + use super::*; + + /// Tracks the inbound corresponding to an outbound HTLC + #[allow(clippy::derive_hash_xor_eq)] // Our Hash is faithful to the data, we just don't have SecretKey::hash + #[derive(Clone, Debug, PartialEq, Eq)] + pub enum HTLCSource { + PreviousHopData(HTLCPreviousHopData), + OutboundRoute { + path: Path, + session_priv: SecretKey, + /// Technically we can recalculate this from the route, but we cache it here to avoid + /// doing a double-pass on route when we get a failure back + first_hop_htlc_msat: u64, + payment_id: PaymentId, + }, + } + + /// Tracks the inbound corresponding to an outbound HTLC + #[derive(Clone, Debug, Hash, PartialEq, Eq)] + pub struct HTLCPreviousHopData { + // Note that this may be an outbound SCID alias for the associated channel. + pub short_channel_id: u64, + pub user_channel_id: Option, + pub htlc_id: u64, + pub incoming_packet_shared_secret: [u8; 32], + pub phantom_shared_secret: Option<[u8; 32]>, + pub blinded_failure: Option, + pub channel_id: ChannelId, + + // These fields are consumed by `claim_funds_from_hop()` when updating a force-closed backwards + // channel with a preimage provided by the forward channel. + pub outpoint: OutPoint, + pub counterparty_node_id: Option, + /// Used to preserve our backwards channel by failing back in case an HTLC claim in the forward + /// channel remains unconfirmed for too long. + pub cltv_expiry: Option, + } } +#[cfg(fuzzing)] +pub use self::fuzzy_channelmanager::*; +#[cfg(not(fuzzing))] +pub(crate) use self::fuzzy_channelmanager::*; + #[allow(clippy::derive_hash_xor_eq)] // Our Hash is faithful to the data, we just don't have SecretKey::hash impl core::hash::Hash for HTLCSource { fn hash(&self, hasher: &mut H) { diff --git a/lightning/src/ln/mod.rs b/lightning/src/ln/mod.rs index dc87200d300..16d3d769f48 100644 --- a/lightning/src/ln/mod.rs +++ b/lightning/src/ln/mod.rs @@ -51,6 +51,9 @@ pub use onion_utils::create_payment_onion; // without the node parameter being mut. This is incorrect, and thus newer rustcs will complain // about an unnecessary mut. Thus, we silence the unused_mut warning in two test modules below. +#[cfg(fuzzing)] +pub use onion_utils::process_onion_failure; + #[cfg(test)] #[allow(unused_mut)] pub mod bolt11_payment_tests; diff --git a/lightning/src/ln/msgs.rs b/lightning/src/ln/msgs.rs index 1887cb18a88..72d79bd5abb 100644 --- a/lightning/src/ln/msgs.rs +++ b/lightning/src/ln/msgs.rs @@ -2241,6 +2241,13 @@ mod fuzzy_internal_msgs { pub(crate) failuremsg: Vec, pub(crate) pad: Vec, } + + #[derive(Clone, Debug, Hash, PartialEq, Eq)] + pub struct OnionErrorPacket { + // This really should be a constant size slice, but the spec lets these things be up to 128KB? + // (TODO) We limit it in decode to much lower... + pub data: Vec, + } } #[cfg(fuzzing)] pub use self::fuzzy_internal_msgs::*; @@ -2349,13 +2356,6 @@ impl Debug for TrampolineOnionPacket { } } -#[derive(Clone, Debug, Hash, PartialEq, Eq)] -pub(crate) struct OnionErrorPacket { - // This really should be a constant size slice, but the spec lets these things be up to 128KB? - // (TODO) We limit it in decode to much lower... - pub(crate) data: Vec, -} - impl From for OnionErrorPacket { fn from(msg: UpdateFailHTLC) -> Self { OnionErrorPacket { diff --git a/lightning/src/ln/onion_utils.rs b/lightning/src/ln/onion_utils.rs index d50063c61c4..78479938536 100644 --- a/lightning/src/ln/onion_utils.rs +++ b/lightning/src/ln/onion_utils.rs @@ -926,18 +926,26 @@ pub(super) fn build_failure_packet( onion_error_packet } -pub(crate) struct DecodedOnionFailure { - pub(crate) network_update: Option, - pub(crate) short_channel_id: Option, - pub(crate) payment_failed_permanently: bool, - pub(crate) failed_within_blinded_path: bool, - #[cfg(any(test, feature = "_test_utils"))] - pub(crate) onion_error_code: Option, - #[cfg(any(test, feature = "_test_utils"))] - pub(crate) onion_error_data: Option>, +mod fuzzy_onion_utils { + use super::*; + + pub struct DecodedOnionFailure { + pub(crate) network_update: Option, + pub(crate) short_channel_id: Option, + pub(crate) payment_failed_permanently: bool, + pub(crate) failed_within_blinded_path: bool, + #[cfg(any(test, feature = "_test_utils"))] + pub(crate) onion_error_code: Option, + #[cfg(any(test, feature = "_test_utils"))] + pub(crate) onion_error_data: Option>, + } } +#[cfg(fuzzing)] +pub use self::fuzzy_onion_utils::*; +#[cfg(not(fuzzing))] +pub(crate) use self::fuzzy_onion_utils::*; -pub(super) fn process_onion_failure( +pub fn process_onion_failure( secp_ctx: &Secp256k1, logger: &L, htlc_source: &HTLCSource, encrypted_packet: OnionErrorPacket, ) -> DecodedOnionFailure From 991caa695390c903fe18730b7e697ad824245937 Mon Sep 17 00:00:00 2001 From: Joost Jager Date: Thu, 27 Mar 2025 14:21:55 -0400 Subject: [PATCH 2/3] Use regular hmac comparison in onion failure processing 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. --- lightning/src/ln/onion_utils.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lightning/src/ln/onion_utils.rs b/lightning/src/ln/onion_utils.rs index 78479938536..d6436b77dc5 100644 --- a/lightning/src/ln/onion_utils.rs +++ b/lightning/src/ln/onion_utils.rs @@ -1160,7 +1160,7 @@ where let mut hmac = HmacEngine::::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] { continue; } From 17737be8dbe17fd481347dc2b9bf93b2b7b94506 Mon Sep 17 00:00:00 2001 From: Joost Jager Date: Tue, 25 Mar 2025 16:04:31 -0400 Subject: [PATCH 3/3] Fuzzing for process_onion_failure 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. --- fuzz/.gitignore | 1 + fuzz/src/bin/gen_target.sh | 1 + fuzz/src/bin/process_onion_failure_target.rs | 120 +++++++++++++++++ fuzz/src/lib.rs | 1 + fuzz/src/process_onion_failure.rs | 135 +++++++++++++++++++ fuzz/targets.h | 1 + 6 files changed, 259 insertions(+) create mode 100644 fuzz/src/bin/process_onion_failure_target.rs create mode 100644 fuzz/src/process_onion_failure.rs diff --git a/fuzz/.gitignore b/fuzz/.gitignore index 8bf27ec5d6c..e8dc6b6e08b 100644 --- a/fuzz/.gitignore +++ b/fuzz/.gitignore @@ -1,3 +1,4 @@ hfuzz_target target hfuzz_workspace +corpus diff --git a/fuzz/src/bin/gen_target.sh b/fuzz/src/bin/gen_target.sh index 717248aa575..0c51e6278dc 100755 --- a/fuzz/src/bin/gen_target.sh +++ b/fuzz/src/bin/gen_target.sh @@ -17,6 +17,7 @@ GEN_TEST bolt11_deser GEN_TEST onion_message GEN_TEST peer_crypt GEN_TEST process_network_graph +GEN_TEST process_onion_failure GEN_TEST refund_deser GEN_TEST router GEN_TEST zbase32 diff --git a/fuzz/src/bin/process_onion_failure_target.rs b/fuzz/src/bin/process_onion_failure_target.rs new file mode 100644 index 00000000000..1d2cdb28593 --- /dev/null +++ b/fuzz/src/bin/process_onion_failure_target.rs @@ -0,0 +1,120 @@ +// This file is Copyright its original authors, visible in version control +// history. +// +// This file is licensed under the Apache License, Version 2.0 or the MIT license +// , at your option. +// You may not use this file except in accordance with one or both of these +// licenses. + +// This file is auto-generated by gen_target.sh based on target_template.txt +// To modify it, modify target_template.txt and run gen_target.sh instead. + +#![cfg_attr(feature = "libfuzzer_fuzz", no_main)] +#![cfg_attr(rustfmt, rustfmt_skip)] + +#[cfg(not(fuzzing))] +compile_error!("Fuzz targets need cfg=fuzzing"); + +#[cfg(not(hashes_fuzz))] +compile_error!("Fuzz targets need cfg=hashes_fuzz"); + +#[cfg(not(secp256k1_fuzz))] +compile_error!("Fuzz targets need cfg=secp256k1_fuzz"); + +extern crate lightning_fuzz; +use lightning_fuzz::process_onion_failure::*; + +#[cfg(feature = "afl")] +#[macro_use] extern crate afl; +#[cfg(feature = "afl")] +fn main() { + fuzz!(|data| { + process_onion_failure_run(data.as_ptr(), data.len()); + }); +} + +#[cfg(feature = "honggfuzz")] +#[macro_use] extern crate honggfuzz; +#[cfg(feature = "honggfuzz")] +fn main() { + loop { + fuzz!(|data| { + process_onion_failure_run(data.as_ptr(), data.len()); + }); + } +} + +#[cfg(feature = "libfuzzer_fuzz")] +#[macro_use] extern crate libfuzzer_sys; +#[cfg(feature = "libfuzzer_fuzz")] +fuzz_target!(|data: &[u8]| { + process_onion_failure_run(data.as_ptr(), data.len()); +}); + +#[cfg(feature = "stdin_fuzz")] +fn main() { + use std::io::Read; + + let mut data = Vec::with_capacity(8192); + std::io::stdin().read_to_end(&mut data).unwrap(); + process_onion_failure_run(data.as_ptr(), data.len()); +} + +#[test] +fn run_test_cases() { + use std::fs; + use std::io::Read; + use lightning_fuzz::utils::test_logger::StringBuffer; + + use std::sync::{atomic, Arc}; + { + let data: Vec = vec![0]; + process_onion_failure_run(data.as_ptr(), data.len()); + } + let mut threads = Vec::new(); + let threads_running = Arc::new(atomic::AtomicUsize::new(0)); + if let Ok(tests) = fs::read_dir("test_cases/process_onion_failure") { + for test in tests { + let mut data: Vec = Vec::new(); + let path = test.unwrap().path(); + fs::File::open(&path).unwrap().read_to_end(&mut data).unwrap(); + threads_running.fetch_add(1, atomic::Ordering::AcqRel); + + let thread_count_ref = Arc::clone(&threads_running); + let main_thread_ref = std::thread::current(); + threads.push((path.file_name().unwrap().to_str().unwrap().to_string(), + std::thread::spawn(move || { + let string_logger = StringBuffer::new(); + + let panic_logger = string_logger.clone(); + let res = if ::std::panic::catch_unwind(move || { + process_onion_failure_test(&data, panic_logger); + }).is_err() { + Some(string_logger.into_string()) + } else { None }; + thread_count_ref.fetch_sub(1, atomic::Ordering::AcqRel); + main_thread_ref.unpark(); + res + }) + )); + while threads_running.load(atomic::Ordering::Acquire) > 32 { + std::thread::park(); + } + } + } + let mut failed_outputs = Vec::new(); + for (test, thread) in threads.drain(..) { + if let Some(output) = thread.join().unwrap() { + println!("\nOutput of {}:\n{}\n", test, output); + failed_outputs.push(test); + } + } + if !failed_outputs.is_empty() { + println!("Test cases which failed: "); + for case in failed_outputs { + println!("{}", case); + } + panic!(); + } +} diff --git a/fuzz/src/lib.rs b/fuzz/src/lib.rs index 123ee88ea31..3a8d9c060a9 100644 --- a/fuzz/src/lib.rs +++ b/fuzz/src/lib.rs @@ -28,6 +28,7 @@ pub mod onion_hop_data; pub mod onion_message; pub mod peer_crypt; pub mod process_network_graph; +pub mod process_onion_failure; pub mod refund_deser; pub mod router; pub mod zbase32; diff --git a/fuzz/src/process_onion_failure.rs b/fuzz/src/process_onion_failure.rs new file mode 100644 index 00000000000..9eb77bd95cc --- /dev/null +++ b/fuzz/src/process_onion_failure.rs @@ -0,0 +1,135 @@ +use std::sync::Arc; + +use bitcoin::{ + key::Secp256k1, + secp256k1::{PublicKey, SecretKey}, +}; +use lightning::{ + blinded_path::BlindedHop, + ln::{ + channelmanager::{HTLCSource, PaymentId}, + msgs::OnionErrorPacket, + }, + routing::router::{BlindedTail, Path, RouteHop, TrampolineHop}, + types::features::{ChannelFeatures, NodeFeatures}, + util::logger::Logger, +}; + +// Imports that need to be added manually +use crate::utils::test_logger::{self}; + +/// Actual fuzz test, method signature and name are fixed +fn do_test(data: &[u8], out: Out) { + let mut read_pos = 0; + macro_rules! get_slice { + ($len: expr) => {{ + let slice_len = $len as usize; + if data.len() < read_pos + slice_len { + return; + } + read_pos += slice_len; + &data[read_pos - slice_len..read_pos] + }}; + } + + macro_rules! get_u16 { + () => { + match get_slice!(2).try_into() { + Ok(val) => u16::from_be_bytes(val), + Err(_) => return, + } + }; + } + + macro_rules! get_bool { + () => { + get_slice!(1)[0] & 1 != 0 + }; + } + + fn usize_to_32_bytes(input: usize) -> [u8; 32] { + let mut bytes = [0u8; 32]; + let input_bytes = input.to_be_bytes(); + bytes[..input_bytes.len()].copy_from_slice(&input_bytes); + bytes + } + + fn usize_to_pubkey(input: usize) -> PublicKey { + let bytes = usize_to_32_bytes(1 + input); + + let secp_ctx = Secp256k1::new(); + let secret_key = SecretKey::from_slice(&bytes).unwrap(); + secret_key.public_key(&secp_ctx) + } + + let secp_ctx = Secp256k1::new(); + let logger: Arc = Arc::new(test_logger::TestLogger::new("".to_owned(), out)); + + let session_priv = SecretKey::from_slice(&usize_to_32_bytes(213127)).unwrap(); + let payment_id = PaymentId(usize_to_32_bytes(232299)); + + let mut hops = Vec::::new(); + let hop_count = get_slice!(1)[0] as usize % 30; + for i in 0..hop_count { + hops.push(RouteHop { + pubkey: usize_to_pubkey(i), + node_features: NodeFeatures::empty(), + short_channel_id: i as u64, + channel_features: ChannelFeatures::empty(), + fee_msat: 0, + cltv_expiry_delta: 0, + maybe_announced_channel: false, + }); + } + + let blinded_tail = if get_bool!() { + let mut trampoline_hops = Vec::::new(); + let trampoline_hop_count = get_slice!(1)[0] as usize % 30; + for i in 0..trampoline_hop_count { + trampoline_hops.push(TrampolineHop { + pubkey: usize_to_pubkey(1000 + i), + node_features: NodeFeatures::empty(), + fee_msat: 0, + cltv_expiry_delta: 0, + }); + } + let mut blinded_hops = Vec::::new(); + let blinded_hop_count = get_slice!(1)[0] as usize % 30; + for i in 0..blinded_hop_count { + blinded_hops.push(BlindedHop { + blinded_node_id: usize_to_pubkey(2000 + i), + encrypted_payload: get_slice!(get_u16!()).to_vec(), + }); + } + Some(BlindedTail { + trampoline_hops, + hops: blinded_hops, + blinding_point: usize_to_pubkey(64354334), + excess_final_cltv_expiry_delta: 0, + final_value_msat: 0, + }) + } else { + None + }; + + let path = Path { hops, blinded_tail }; + + let htlc_source = + HTLCSource::OutboundRoute { path, session_priv, first_hop_htlc_msat: 0, payment_id }; + + let failure_len = get_u16!(); + let encrypted_packet = OnionErrorPacket { data: get_slice!(failure_len).into() }; + + lightning::ln::process_onion_failure(&secp_ctx, &logger, &htlc_source, encrypted_packet); +} + +/// Method that needs to be added manually, {name}_test +pub fn process_onion_failure_test(data: &[u8], out: Out) { + do_test(data, out); +} + +/// Method that needs to be added manually, {name}_run +#[no_mangle] +pub extern "C" fn process_onion_failure_run(data: *const u8, datalen: usize) { + do_test(unsafe { std::slice::from_raw_parts(data, datalen) }, test_logger::DevNull {}); +} diff --git a/fuzz/targets.h b/fuzz/targets.h index b362477a12e..352296027f0 100644 --- a/fuzz/targets.h +++ b/fuzz/targets.h @@ -10,6 +10,7 @@ void bolt11_deser_run(const unsigned char* data, size_t data_len); void onion_message_run(const unsigned char* data, size_t data_len); void peer_crypt_run(const unsigned char* data, size_t data_len); void process_network_graph_run(const unsigned char* data, size_t data_len); +void process_onion_failure_run(const unsigned char* data, size_t data_len); void refund_deser_run(const unsigned char* data, size_t data_len); void router_run(const unsigned char* data, size_t data_len); void zbase32_run(const unsigned char* data, size_t data_len);