diff --git a/fuzz/src/bin/gen_target.sh b/fuzz/src/bin/gen_target.sh index 34cae5107..fe17e4bab 100755 --- a/fuzz/src/bin/gen_target.sh +++ b/fuzz/src/bin/gen_target.sh @@ -20,6 +20,7 @@ GEN_TEST refund_deser GEN_TEST router GEN_TEST zbase32 GEN_TEST indexedmap +GEN_TEST onion_hop_data GEN_TEST msg_accept_channel msg_targets:: GEN_TEST msg_announcement_signatures msg_targets:: @@ -51,7 +52,6 @@ GEN_TEST msg_update_add_htlc msg_targets:: GEN_TEST msg_error_message msg_targets:: GEN_TEST msg_channel_update msg_targets:: -GEN_TEST msg_onion_hop_data msg_targets:: GEN_TEST msg_ping msg_targets:: GEN_TEST msg_pong msg_targets:: diff --git a/fuzz/src/bin/msg_onion_hop_data_target.rs b/fuzz/src/bin/onion_hop_data_target.rs similarity index 86% rename from fuzz/src/bin/msg_onion_hop_data_target.rs rename to fuzz/src/bin/onion_hop_data_target.rs index ae21e9bd9..b8a357229 100644 --- a/fuzz/src/bin/msg_onion_hop_data_target.rs +++ b/fuzz/src/bin/onion_hop_data_target.rs @@ -16,14 +16,14 @@ compile_error!("Fuzz targets need cfg=fuzzing"); extern crate lightning_fuzz; -use lightning_fuzz::msg_targets::msg_onion_hop_data::*; +use lightning_fuzz::onion_hop_data::*; #[cfg(feature = "afl")] #[macro_use] extern crate afl; #[cfg(feature = "afl")] fn main() { fuzz!(|data| { - msg_onion_hop_data_run(data.as_ptr(), data.len()); + onion_hop_data_run(data.as_ptr(), data.len()); }); } @@ -33,7 +33,7 @@ fn main() { fn main() { loop { fuzz!(|data| { - msg_onion_hop_data_run(data.as_ptr(), data.len()); + onion_hop_data_run(data.as_ptr(), data.len()); }); } } @@ -42,7 +42,7 @@ fn main() { #[macro_use] extern crate libfuzzer_sys; #[cfg(feature = "libfuzzer_fuzz")] fuzz_target!(|data: &[u8]| { - msg_onion_hop_data_run(data.as_ptr(), data.len()); + onion_hop_data_run(data.as_ptr(), data.len()); }); #[cfg(feature = "stdin_fuzz")] @@ -51,7 +51,7 @@ fn main() { let mut data = Vec::with_capacity(8192); std::io::stdin().read_to_end(&mut data).unwrap(); - msg_onion_hop_data_run(data.as_ptr(), data.len()); + onion_hop_data_run(data.as_ptr(), data.len()); } #[test] @@ -63,11 +63,11 @@ fn run_test_cases() { use std::sync::{atomic, Arc}; { let data: Vec = vec![0]; - msg_onion_hop_data_run(data.as_ptr(), data.len()); + onion_hop_data_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/msg_onion_hop_data") { + if let Ok(tests) = fs::read_dir("test_cases/onion_hop_data") { for test in tests { let mut data: Vec = Vec::new(); let path = test.unwrap().path(); @@ -82,7 +82,7 @@ fn run_test_cases() { let panic_logger = string_logger.clone(); let res = if ::std::panic::catch_unwind(move || { - msg_onion_hop_data_test(&data, panic_logger); + onion_hop_data_test(&data, panic_logger); }).is_err() { Some(string_logger.into_string()) } else { None }; diff --git a/fuzz/src/lib.rs b/fuzz/src/lib.rs index 92142e564..6cdeb8ab5 100644 --- a/fuzz/src/lib.rs +++ b/fuzz/src/lib.rs @@ -28,5 +28,6 @@ pub mod process_network_graph; pub mod refund_deser; pub mod router; pub mod zbase32; +pub mod onion_hop_data; pub mod msg_targets; diff --git a/fuzz/src/msg_targets/gen_target.sh b/fuzz/src/msg_targets/gen_target.sh index 3937c5001..d89df19d9 100755 --- a/fuzz/src/msg_targets/gen_target.sh +++ b/fuzz/src/msg_targets/gen_target.sh @@ -20,7 +20,6 @@ GEN_TEST lightning::ln::msgs::ChannelReady test_msg_simple "" GEN_TEST lightning::ln::msgs::FundingSigned test_msg_simple "" GEN_TEST lightning::ln::msgs::GossipTimestampFilter test_msg_simple "" GEN_TEST lightning::ln::msgs::Init test_msg_simple "" -GEN_TEST lightning::ln::msgs::OnionHopData test_msg_simple "" GEN_TEST lightning::ln::msgs::OpenChannel test_msg_simple "" GEN_TEST lightning::ln::msgs::Ping test_msg_simple "" GEN_TEST lightning::ln::msgs::Pong test_msg_simple "" diff --git a/fuzz/src/msg_targets/mod.rs b/fuzz/src/msg_targets/mod.rs index fe3bd14a7..302dda440 100644 --- a/fuzz/src/msg_targets/mod.rs +++ b/fuzz/src/msg_targets/mod.rs @@ -8,7 +8,6 @@ pub mod msg_channel_ready; pub mod msg_funding_signed; pub mod msg_gossip_timestamp_filter; pub mod msg_init; -pub mod msg_onion_hop_data; pub mod msg_open_channel; pub mod msg_ping; pub mod msg_pong; diff --git a/fuzz/src/msg_targets/msg_onion_hop_data.rs b/fuzz/src/onion_hop_data.rs similarity index 59% rename from fuzz/src/msg_targets/msg_onion_hop_data.rs rename to fuzz/src/onion_hop_data.rs index 59b3674f9..54b283ab0 100644 --- a/fuzz/src/msg_targets/msg_onion_hop_data.rs +++ b/fuzz/src/onion_hop_data.rs @@ -10,16 +10,19 @@ // This file is auto-generated by gen_target.sh based on msg_target_template.txt // To modify it, modify msg_target_template.txt and run gen_target.sh instead. -use crate::msg_targets::utils::VecWriter; use crate::utils::test_logger; #[inline] -pub fn msg_onion_hop_data_test(data: &[u8], _out: Out) { - test_msg_simple!(lightning::ln::msgs::OnionHopData, data); +pub fn onion_hop_data_test(data: &[u8], _out: Out) { + use lightning::util::ser::Readable; + let mut r = ::std::io::Cursor::new(data); + let _ = ::read(&mut r); } #[no_mangle] -pub extern "C" fn msg_onion_hop_data_run(data: *const u8, datalen: usize) { +pub extern "C" fn onion_hop_data_run(data: *const u8, datalen: usize) { + use lightning::util::ser::Readable; let data = unsafe { std::slice::from_raw_parts(data, datalen) }; - test_msg_simple!(lightning::ln::msgs::OnionHopData, data); + let mut r = ::std::io::Cursor::new(data); + let _ = ::read(&mut r); } diff --git a/fuzz/targets.h b/fuzz/targets.h index eb8d66f41..9b5a6d455 100644 --- a/fuzz/targets.h +++ b/fuzz/targets.h @@ -13,6 +13,7 @@ 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); void indexedmap_run(const unsigned char* data, size_t data_len); +void onion_hop_data_run(const unsigned char* data, size_t data_len); void msg_accept_channel_run(const unsigned char* data, size_t data_len); void msg_announcement_signatures_run(const unsigned char* data, size_t data_len); void msg_channel_reestablish_run(const unsigned char* data, size_t data_len); @@ -40,7 +41,6 @@ void msg_gossip_timestamp_filter_run(const unsigned char* data, size_t data_len) void msg_update_add_htlc_run(const unsigned char* data, size_t data_len); void msg_error_message_run(const unsigned char* data, size_t data_len); void msg_channel_update_run(const unsigned char* data, size_t data_len); -void msg_onion_hop_data_run(const unsigned char* data, size_t data_len); void msg_ping_run(const unsigned char* data, size_t data_len); void msg_pong_run(const unsigned char* data, size_t data_len); void msg_channel_details_run(const unsigned char* data, size_t data_len); diff --git a/lightning/src/ln/channel.rs b/lightning/src/ln/channel.rs index fd0d94d3e..1978467ba 100644 --- a/lightning/src/ln/channel.rs +++ b/lightning/src/ln/channel.rs @@ -3119,9 +3119,9 @@ impl Channel { let mut htlc_updates = Vec::new(); mem::swap(&mut htlc_updates, &mut self.context.holding_cell_htlc_updates); - let mut update_add_htlcs = Vec::with_capacity(htlc_updates.len()); - let mut update_fulfill_htlcs = Vec::with_capacity(htlc_updates.len()); - let mut update_fail_htlcs = Vec::with_capacity(htlc_updates.len()); + let mut update_add_count = 0; + let mut update_fulfill_count = 0; + let mut update_fail_count = 0; let mut htlcs_to_fail = Vec::new(); for htlc_update in htlc_updates.drain(..) { // Note that this *can* fail, though it should be due to rather-rare conditions on @@ -3137,7 +3137,7 @@ impl Channel { match self.send_htlc(amount_msat, *payment_hash, cltv_expiry, source.clone(), onion_routing_packet.clone(), false, skimmed_fee_msat, fee_estimator, logger) { - Ok(update_add_msg_option) => update_add_htlcs.push(update_add_msg_option.unwrap()), + Ok(_) => update_add_count += 1, Err(e) => { match e { ChannelError::Ignore(ref msg) => { @@ -3164,11 +3164,11 @@ impl Channel { // not fail - any in between attempts to claim the HTLC will have resulted // in it hitting the holding cell again and we cannot change the state of a // holding cell HTLC from fulfill to anything else. - let (update_fulfill_msg_option, mut additional_monitor_update) = - if let UpdateFulfillFetch::NewClaim { msg, monitor_update, .. } = self.get_update_fulfill_htlc(htlc_id, *payment_preimage, logger) { - (msg, monitor_update) - } else { unreachable!() }; - update_fulfill_htlcs.push(update_fulfill_msg_option.unwrap()); + let mut additional_monitor_update = + if let UpdateFulfillFetch::NewClaim { monitor_update, .. } = + self.get_update_fulfill_htlc(htlc_id, *payment_preimage, logger) + { monitor_update } else { unreachable!() }; + update_fulfill_count += 1; monitor_update.updates.append(&mut additional_monitor_update.updates); }, &HTLCUpdateAwaitingACK::FailHTLC { htlc_id, ref err_packet } => { @@ -3179,7 +3179,8 @@ impl Channel { // not fail - we should never end up in a state where we double-fail // an HTLC or fail-then-claim an HTLC as it indicates we didn't wait // for a full revocation before failing. - update_fail_htlcs.push(update_fail_msg_option.unwrap()) + debug_assert!(update_fail_msg_option.is_some()); + update_fail_count += 1; }, Err(e) => { if let ChannelError::Ignore(_) = e {} @@ -3191,7 +3192,7 @@ impl Channel { }, } } - if update_add_htlcs.is_empty() && update_fulfill_htlcs.is_empty() && update_fail_htlcs.is_empty() && self.context.holding_cell_update_fee.is_none() { + if update_add_count == 0 && update_fulfill_count == 0 && update_fail_count == 0 && self.context.holding_cell_update_fee.is_none() { return (None, htlcs_to_fail); } let update_fee = if let Some(feerate) = self.context.holding_cell_update_fee.take() { @@ -3208,7 +3209,7 @@ impl Channel { log_debug!(logger, "Freeing holding cell in channel {} resulted in {}{} HTLCs added, {} HTLCs fulfilled, and {} HTLCs failed.", log_bytes!(self.context.channel_id()), if update_fee.is_some() { "a fee update, " } else { "" }, - update_add_htlcs.len(), update_fulfill_htlcs.len(), update_fail_htlcs.len()); + update_add_count, update_fulfill_count, update_fail_count); self.monitor_updating_paused(false, true, false, Vec::new(), Vec::new(), Vec::new()); (self.push_ret_blockable_mon_update(monitor_update), htlcs_to_fail) diff --git a/lightning/src/ln/channelmanager.rs b/lightning/src/ln/channelmanager.rs index 6cd4799ff..3757b06b3 100644 --- a/lightning/src/ln/channelmanager.rs +++ b/lightning/src/ln/channelmanager.rs @@ -341,7 +341,7 @@ impl HTLCSource { } } -struct ReceiveError { +struct InboundOnionErr { err_code: u16, err_data: Vec, msg: &'static str, @@ -2631,14 +2631,64 @@ where } } + fn construct_fwd_pending_htlc_info( + &self, msg: &msgs::UpdateAddHTLC, hop_data: msgs::InboundOnionPayload, hop_hmac: [u8; 32], + new_packet_bytes: [u8; onion_utils::ONION_DATA_LEN], shared_secret: [u8; 32], + next_packet_pubkey_opt: Option> + ) -> Result { + debug_assert!(next_packet_pubkey_opt.is_some()); + let outgoing_packet = msgs::OnionPacket { + version: 0, + public_key: next_packet_pubkey_opt.unwrap_or(Err(secp256k1::Error::InvalidPublicKey)), + hop_data: new_packet_bytes, + hmac: hop_hmac, + }; + + let (short_channel_id, amt_to_forward, outgoing_cltv_value) = match hop_data { + msgs::InboundOnionPayload::Forward { short_channel_id, amt_to_forward, outgoing_cltv_value } => + (short_channel_id, amt_to_forward, outgoing_cltv_value), + msgs::InboundOnionPayload::Receive { .. } => + return Err(InboundOnionErr { + msg: "Final Node OnionHopData provided for us as an intermediary node", + err_code: 0x4000 | 22, + err_data: Vec::new(), + }), + }; + + Ok(PendingHTLCInfo { + routing: PendingHTLCRouting::Forward { + onion_packet: outgoing_packet, + short_channel_id, + }, + payment_hash: msg.payment_hash, + incoming_shared_secret: shared_secret, + incoming_amt_msat: Some(msg.amount_msat), + outgoing_amt_msat: amt_to_forward, + outgoing_cltv_value, + skimmed_fee_msat: None, + }) + } + fn construct_recv_pending_htlc_info( - &self, hop_data: msgs::OnionHopData, shared_secret: [u8; 32], payment_hash: PaymentHash, + &self, hop_data: msgs::InboundOnionPayload, shared_secret: [u8; 32], payment_hash: PaymentHash, amt_msat: u64, cltv_expiry: u32, phantom_shared_secret: Option<[u8; 32]>, allow_underpay: bool, counterparty_skimmed_fee_msat: Option, - ) -> Result { + ) -> Result { + let (payment_data, keysend_preimage, onion_amt_msat, outgoing_cltv_value, payment_metadata) = match hop_data { + msgs::InboundOnionPayload::Receive { + payment_data, keysend_preimage, amt_msat, outgoing_cltv_value, payment_metadata, .. + } => + (payment_data, keysend_preimage, amt_msat, outgoing_cltv_value, payment_metadata), + _ => + return Err(InboundOnionErr { + err_code: 0x4000|22, + err_data: Vec::new(), + msg: "Got non final data with an HMAC of 0", + }), + }; // final_incorrect_cltv_expiry - if hop_data.outgoing_cltv_value > cltv_expiry { - return Err(ReceiveError { + if outgoing_cltv_value > cltv_expiry { + return Err(InboundOnionErr { msg: "Upstream node set CLTV to less than the CLTV set by the sender", err_code: 18, err_data: cltv_expiry.to_be_bytes().to_vec() @@ -2652,85 +2702,74 @@ where // payment logic has enough time to fail the HTLC backward before our onchain logic triggers a // channel closure (see HTLC_FAIL_BACK_BUFFER rationale). let current_height: u32 = self.best_block.read().unwrap().height(); - if (hop_data.outgoing_cltv_value as u64) <= current_height as u64 + HTLC_FAIL_BACK_BUFFER as u64 + 1 { + if (outgoing_cltv_value as u64) <= current_height as u64 + HTLC_FAIL_BACK_BUFFER as u64 + 1 { let mut err_data = Vec::with_capacity(12); err_data.extend_from_slice(&amt_msat.to_be_bytes()); err_data.extend_from_slice(¤t_height.to_be_bytes()); - return Err(ReceiveError { + return Err(InboundOnionErr { err_code: 0x4000 | 15, err_data, msg: "The final CLTV expiry is too soon to handle", }); } - if (!allow_underpay && hop_data.amt_to_forward > amt_msat) || - (allow_underpay && hop_data.amt_to_forward > + if (!allow_underpay && onion_amt_msat > amt_msat) || + (allow_underpay && onion_amt_msat > amt_msat.saturating_add(counterparty_skimmed_fee_msat.unwrap_or(0))) { - return Err(ReceiveError { + return Err(InboundOnionErr { err_code: 19, err_data: amt_msat.to_be_bytes().to_vec(), msg: "Upstream node sent less than we were supposed to receive in payment", }); } - let routing = match hop_data.format { - msgs::OnionHopDataFormat::NonFinalNode { .. } => { - return Err(ReceiveError { + let routing = if let Some(payment_preimage) = keysend_preimage { + // We need to check that the sender knows the keysend preimage before processing this + // payment further. Otherwise, an intermediary routing hop forwarding non-keysend-HTLC X + // could discover the final destination of X, by probing the adjacent nodes on the route + // with a keysend payment of identical payment hash to X and observing the processing + // time discrepancies due to a hash collision with X. + let hashed_preimage = PaymentHash(Sha256::hash(&payment_preimage.0).into_inner()); + if hashed_preimage != payment_hash { + return Err(InboundOnionErr { err_code: 0x4000|22, err_data: Vec::new(), - msg: "Got non final data with an HMAC of 0", + msg: "Payment preimage didn't match payment hash", }); - }, - msgs::OnionHopDataFormat::FinalNode { payment_data, keysend_preimage, payment_metadata } => { - if let Some(payment_preimage) = keysend_preimage { - // We need to check that the sender knows the keysend preimage before processing this - // payment further. Otherwise, an intermediary routing hop forwarding non-keysend-HTLC X - // could discover the final destination of X, by probing the adjacent nodes on the route - // with a keysend payment of identical payment hash to X and observing the processing - // time discrepancies due to a hash collision with X. - let hashed_preimage = PaymentHash(Sha256::hash(&payment_preimage.0).into_inner()); - if hashed_preimage != payment_hash { - return Err(ReceiveError { - err_code: 0x4000|22, - err_data: Vec::new(), - msg: "Payment preimage didn't match payment hash", - }); - } - if !self.default_configuration.accept_mpp_keysend && payment_data.is_some() { - return Err(ReceiveError { - err_code: 0x4000|22, - err_data: Vec::new(), - msg: "We don't support MPP keysend payments", - }); - } - PendingHTLCRouting::ReceiveKeysend { - payment_data, - payment_preimage, - payment_metadata, - incoming_cltv_expiry: hop_data.outgoing_cltv_value, - } - } else if let Some(data) = payment_data { - PendingHTLCRouting::Receive { - payment_data: data, - payment_metadata, - incoming_cltv_expiry: hop_data.outgoing_cltv_value, - phantom_shared_secret, - } - } else { - return Err(ReceiveError { - err_code: 0x4000|0x2000|3, - err_data: Vec::new(), - msg: "We require payment_secrets", - }); - } - }, + } + if !self.default_configuration.accept_mpp_keysend && payment_data.is_some() { + return Err(InboundOnionErr { + err_code: 0x4000|22, + err_data: Vec::new(), + msg: "We don't support MPP keysend payments", + }); + } + PendingHTLCRouting::ReceiveKeysend { + payment_data, + payment_preimage, + payment_metadata, + incoming_cltv_expiry: outgoing_cltv_value, + } + } else if let Some(data) = payment_data { + PendingHTLCRouting::Receive { + payment_data: data, + payment_metadata, + incoming_cltv_expiry: outgoing_cltv_value, + phantom_shared_secret, + } + } else { + return Err(InboundOnionErr { + err_code: 0x4000|0x2000|3, + err_data: Vec::new(), + msg: "We require payment_secrets", + }); }; Ok(PendingHTLCInfo { routing, payment_hash, incoming_shared_secret: shared_secret, incoming_amt_msat: Some(amt_msat), - outgoing_amt_msat: hop_data.amt_to_forward, - outgoing_cltv_value: hop_data.outgoing_cltv_value, + outgoing_amt_msat: onion_amt_msat, + outgoing_cltv_value, skimmed_fee_msat: counterparty_skimmed_fee_msat, }) } @@ -2794,9 +2833,8 @@ where }; let (outgoing_scid, outgoing_amt_msat, outgoing_cltv_value, next_packet_pk_opt) = match next_hop { onion_utils::Hop::Forward { - next_hop_data: msgs::OnionHopData { - format: msgs::OnionHopDataFormat::NonFinalNode { short_channel_id }, amt_to_forward, - outgoing_cltv_value, + next_hop_data: msgs::InboundOnionPayload::Forward { + short_channel_id, amt_to_forward, outgoing_cltv_value }, .. } => { let next_pk = onion_utils::next_hop_packet_pubkey(&self.secp_ctx, @@ -2806,9 +2844,7 @@ where // We'll do receive checks in [`Self::construct_pending_htlc_info`] so we have access to the // inbound channel's state. onion_utils::Hop::Receive { .. } => return Ok((next_hop, shared_secret, None)), - onion_utils::Hop::Forward { - next_hop_data: msgs::OnionHopData { format: msgs::OnionHopDataFormat::FinalNode { .. }, .. }, .. - } => { + onion_utils::Hop::Forward { next_hop_data: msgs::InboundOnionPayload::Receive { .. }, .. } => { return_err!("Final Node OnionHopData provided for us as an intermediary node", 0x4000 | 22, &[0; 0]); } }; @@ -2979,37 +3015,15 @@ where // delay) once they've send us a commitment_signed! PendingHTLCStatus::Forward(info) }, - Err(ReceiveError { err_code, err_data, msg }) => return_err!(msg, err_code, &err_data) + Err(InboundOnionErr { err_code, err_data, msg }) => return_err!(msg, err_code, &err_data) } }, onion_utils::Hop::Forward { next_hop_data, next_hop_hmac, new_packet_bytes } => { - debug_assert!(next_packet_pubkey_opt.is_some()); - let outgoing_packet = msgs::OnionPacket { - version: 0, - public_key: next_packet_pubkey_opt.unwrap_or(Err(secp256k1::Error::InvalidPublicKey)), - hop_data: new_packet_bytes, - hmac: next_hop_hmac.clone(), - }; - - let short_channel_id = match next_hop_data.format { - msgs::OnionHopDataFormat::NonFinalNode { short_channel_id } => short_channel_id, - msgs::OnionHopDataFormat::FinalNode { .. } => { - return_err!("Final Node OnionHopData provided for us as an intermediary node", 0x4000 | 22, &[0;0]); - }, - }; - - PendingHTLCStatus::Forward(PendingHTLCInfo { - routing: PendingHTLCRouting::Forward { - onion_packet: outgoing_packet, - short_channel_id, - }, - payment_hash: msg.payment_hash.clone(), - incoming_shared_secret: shared_secret, - incoming_amt_msat: Some(msg.amount_msat), - outgoing_amt_msat: next_hop_data.amt_to_forward, - outgoing_cltv_value: next_hop_data.outgoing_cltv_value, - skimmed_fee_msat: None, - }) + match self.construct_fwd_pending_htlc_info(msg, next_hop_data, next_hop_hmac, + new_packet_bytes, shared_secret, next_packet_pubkey_opt) { + Ok(info) => PendingHTLCStatus::Forward(info), + Err(InboundOnionErr { err_code, err_data, msg }) => return_err!(msg, err_code, &err_data) + } } } } @@ -3814,7 +3828,7 @@ where outgoing_cltv_value, Some(phantom_shared_secret), false, None) { Ok(info) => phantom_receives.push((prev_short_channel_id, prev_funding_outpoint, prev_user_channel_id, vec![(info, prev_htlc_id)])), - Err(ReceiveError { err_code, err_data, msg }) => failed_payment!(msg, err_code, err_data, Some(phantom_shared_secret)) + Err(InboundOnionErr { err_code, err_data, msg }) => failed_payment!(msg, err_code, err_data, Some(phantom_shared_secret)) } }, _ => panic!(), @@ -10070,20 +10084,18 @@ mod tests { let node = create_network(1, &node_cfg, &node_chanmgr); let sender_intended_amt_msat = 100; let extra_fee_msat = 10; - let hop_data = msgs::OnionHopData { - amt_to_forward: 100, + let hop_data = msgs::InboundOnionPayload::Receive { + amt_msat: 100, outgoing_cltv_value: 42, - format: msgs::OnionHopDataFormat::FinalNode { - keysend_preimage: None, - payment_metadata: None, - payment_data: Some(msgs::FinalOnionHopData { - payment_secret: PaymentSecret([0; 32]), total_msat: sender_intended_amt_msat, - }), - } + payment_metadata: None, + keysend_preimage: None, + payment_data: Some(msgs::FinalOnionHopData { + payment_secret: PaymentSecret([0; 32]), total_msat: sender_intended_amt_msat, + }), }; // Check that if the amount we received + the penultimate hop extra fee is less than the sender // intended amount, we fail the payment. - if let Err(crate::ln::channelmanager::ReceiveError { err_code, .. }) = + if let Err(crate::ln::channelmanager::InboundOnionErr { err_code, .. }) = node[0].node.construct_recv_pending_htlc_info(hop_data, [0; 32], PaymentHash([0; 32]), sender_intended_amt_msat - extra_fee_msat - 1, 42, None, true, Some(extra_fee_msat)) { @@ -10091,16 +10103,14 @@ mod tests { } else { panic!(); } // If amt_received + extra_fee is equal to the sender intended amount, we're fine. - let hop_data = msgs::OnionHopData { // This is the same hop_data as above, OnionHopData doesn't implement Clone - amt_to_forward: 100, + let hop_data = msgs::InboundOnionPayload::Receive { // This is the same payload as above, InboundOnionPayload doesn't implement Clone + amt_msat: 100, outgoing_cltv_value: 42, - format: msgs::OnionHopDataFormat::FinalNode { - keysend_preimage: None, - payment_metadata: None, - payment_data: Some(msgs::FinalOnionHopData { - payment_secret: PaymentSecret([0; 32]), total_msat: sender_intended_amt_msat, - }), - } + payment_metadata: None, + keysend_preimage: None, + payment_data: Some(msgs::FinalOnionHopData { + payment_secret: PaymentSecret([0; 32]), total_msat: sender_intended_amt_msat, + }), }; assert!(node[0].node.construct_recv_pending_htlc_info(hop_data, [0; 32], PaymentHash([0; 32]), sender_intended_amt_msat - extra_fee_msat, 42, None, true, Some(extra_fee_msat)).is_ok()); diff --git a/lightning/src/ln/functional_tests.rs b/lightning/src/ln/functional_tests.rs index 0f7f4bfbd..546c23247 100644 --- a/lightning/src/ln/functional_tests.rs +++ b/lightning/src/ln/functional_tests.rs @@ -8067,7 +8067,9 @@ fn test_onion_value_mpp_set_calculation() { RecipientOnionFields::secret_only(our_payment_secret), height + 1, &None).unwrap(); // Edit amt_to_forward to simulate the sender having set // the final amount and the routing node taking less fee - onion_payloads[1].amt_to_forward = 99_000; + if let msgs::OutboundOnionPayload::Receive { ref mut amt_msat, .. } = onion_payloads[1] { + *amt_msat = 99_000; + } else { panic!() } let new_onion_packet = onion_utils::construct_onion_packet(onion_payloads, onion_keys, [0; 32], &our_payment_hash).unwrap(); payment_event.msgs[0].onion_routing_packet = new_onion_packet; } diff --git a/lightning/src/ln/msgs.rs b/lightning/src/ln/msgs.rs index 0c23d4425..aedd1e76e 100644 --- a/lightning/src/ln/msgs.rs +++ b/lightning/src/ln/msgs.rs @@ -1423,30 +1423,43 @@ mod fuzzy_internal_msgs { // These types aren't intended to be pub, but are exposed for direct fuzzing (as we deserialize // them from untrusted input): #[derive(Clone)] - pub(crate) struct FinalOnionHopData { - pub(crate) payment_secret: PaymentSecret, + pub struct FinalOnionHopData { + pub payment_secret: PaymentSecret, /// The total value, in msat, of the payment as received by the ultimate recipient. /// Message serialization may panic if this value is more than 21 million Bitcoin. - pub(crate) total_msat: u64, + pub total_msat: u64, } - pub(crate) enum OnionHopDataFormat { - NonFinalNode { + pub enum InboundOnionPayload { + Forward { short_channel_id: u64, + /// The value, in msat, of the payment after this hop's fee is deducted. + amt_to_forward: u64, + outgoing_cltv_value: u32, }, - FinalNode { + Receive { payment_data: Option, payment_metadata: Option>, keysend_preimage: Option, + amt_msat: u64, + outgoing_cltv_value: u32, }, } - pub struct OnionHopData { - pub(crate) format: OnionHopDataFormat, - /// The value, in msat, of the payment after this hop's fee is deducted. - /// Message serialization may panic if this value is more than 21 million Bitcoin. - pub(crate) amt_to_forward: u64, - pub(crate) outgoing_cltv_value: u32, + pub(crate) enum OutboundOnionPayload { + Forward { + short_channel_id: u64, + /// The value, in msat, of the payment after this hop's fee is deducted. + amt_to_forward: u64, + outgoing_cltv_value: u32, + }, + Receive { + payment_data: Option, + payment_metadata: Option>, + keysend_preimage: Option, + amt_msat: u64, + outgoing_cltv_value: u32, + }, } pub struct DecodedOnionErrorPacket { @@ -1955,20 +1968,22 @@ impl Readable for FinalOnionHopData { } } -impl Writeable for OnionHopData { +impl Writeable for OutboundOnionPayload { fn write(&self, w: &mut W) -> Result<(), io::Error> { - match self.format { - OnionHopDataFormat::NonFinalNode { short_channel_id } => { + match self { + Self::Forward { short_channel_id, amt_to_forward, outgoing_cltv_value } => { _encode_varint_length_prefixed_tlv!(w, { - (2, HighZeroBytesDroppedBigSize(self.amt_to_forward), required), - (4, HighZeroBytesDroppedBigSize(self.outgoing_cltv_value), required), + (2, HighZeroBytesDroppedBigSize(*amt_to_forward), required), + (4, HighZeroBytesDroppedBigSize(*outgoing_cltv_value), required), (6, short_channel_id, required) }); }, - OnionHopDataFormat::FinalNode { ref payment_data, ref payment_metadata, ref keysend_preimage } => { + Self::Receive { + ref payment_data, ref payment_metadata, ref keysend_preimage, amt_msat, outgoing_cltv_value + } => { _encode_varint_length_prefixed_tlv!(w, { - (2, HighZeroBytesDroppedBigSize(self.amt_to_forward), required), - (4, HighZeroBytesDroppedBigSize(self.outgoing_cltv_value), required), + (2, HighZeroBytesDroppedBigSize(*amt_msat), required), + (4, HighZeroBytesDroppedBigSize(*outgoing_cltv_value), required), (8, payment_data, option), (16, payment_metadata.as_ref().map(|m| WithoutLength(m)), option), (5482373484, keysend_preimage, option) @@ -1979,7 +1994,7 @@ impl Writeable for OnionHopData { } } -impl Readable for OnionHopData { +impl Readable for InboundOnionPayload { fn read(r: &mut R) -> Result { let mut amt = HighZeroBytesDroppedBigSize(0u64); let mut cltv_value = HighZeroBytesDroppedBigSize(0u32); @@ -1997,39 +2012,35 @@ impl Readable for OnionHopData { (5482373484, keysend_preimage, option) }); - let format = if let Some(short_channel_id) = short_id { - if payment_data.is_some() { return Err(DecodeError::InvalidValue); } + if amt.0 > MAX_VALUE_MSAT { return Err(DecodeError::InvalidValue) } + if let Some(short_channel_id) = short_id { + if payment_data.is_some() { return Err(DecodeError::InvalidValue) } if payment_metadata.is_some() { return Err(DecodeError::InvalidValue); } - OnionHopDataFormat::NonFinalNode { + Ok(Self::Forward { short_channel_id, - } + amt_to_forward: amt.0, + outgoing_cltv_value: cltv_value.0, + }) } else { if let Some(data) = &payment_data { if data.total_msat > MAX_VALUE_MSAT { return Err(DecodeError::InvalidValue); } } - OnionHopDataFormat::FinalNode { + Ok(Self::Receive { payment_data, payment_metadata: payment_metadata.map(|w| w.0), keysend_preimage, - } - }; - - if amt.0 > MAX_VALUE_MSAT { - return Err(DecodeError::InvalidValue); + amt_msat: amt.0, + outgoing_cltv_value: cltv_value.0, + }) } - Ok(OnionHopData { - format, - amt_to_forward: amt.0, - outgoing_cltv_value: cltv_value.0, - }) } } // ReadableArgs because we need onion_utils::decode_next_hop to accommodate payment packets and // onion message packets. -impl ReadableArgs<()> for OnionHopData { +impl ReadableArgs<()> for InboundOnionPayload { fn read(r: &mut R, _arg: ()) -> Result { ::read(r) } @@ -2447,7 +2458,7 @@ mod tests { use hex; use crate::ln::{PaymentPreimage, PaymentHash, PaymentSecret}; use crate::ln::features::{ChannelFeatures, ChannelTypeFeatures, InitFeatures, NodeFeatures}; - use crate::ln::msgs::{self, FinalOnionHopData, OnionErrorPacket, OnionHopDataFormat}; + use crate::ln::msgs::{self, FinalOnionHopData, OnionErrorPacket}; use crate::routing::gossip::{NodeAlias, NodeId}; use crate::util::ser::{Writeable, Readable, Hostname, TransactionU16LenLimited}; @@ -3530,75 +3541,74 @@ mod tests { #[test] fn encoding_nonfinal_onion_hop_data() { - let mut msg = msgs::OnionHopData { - format: OnionHopDataFormat::NonFinalNode { - short_channel_id: 0xdeadbeef1bad1dea, - }, + let outbound_msg = msgs::OutboundOnionPayload::Forward { + short_channel_id: 0xdeadbeef1bad1dea, amt_to_forward: 0x0badf00d01020304, outgoing_cltv_value: 0xffffffff, }; - let encoded_value = msg.encode(); + let encoded_value = outbound_msg.encode(); let target_value = hex::decode("1a02080badf00d010203040404ffffffff0608deadbeef1bad1dea").unwrap(); assert_eq!(encoded_value, target_value); - msg = Readable::read(&mut Cursor::new(&target_value[..])).unwrap(); - if let OnionHopDataFormat::NonFinalNode { short_channel_id } = msg.format { + + let inbound_msg = Readable::read(&mut Cursor::new(&target_value[..])).unwrap(); + if let msgs::InboundOnionPayload::Forward { short_channel_id, amt_to_forward, outgoing_cltv_value } = inbound_msg { assert_eq!(short_channel_id, 0xdeadbeef1bad1dea); + assert_eq!(amt_to_forward, 0x0badf00d01020304); + assert_eq!(outgoing_cltv_value, 0xffffffff); } else { panic!(); } - assert_eq!(msg.amt_to_forward, 0x0badf00d01020304); - assert_eq!(msg.outgoing_cltv_value, 0xffffffff); } #[test] fn encoding_final_onion_hop_data() { - let mut msg = msgs::OnionHopData { - format: OnionHopDataFormat::FinalNode { - payment_data: None, - payment_metadata: None, - keysend_preimage: None, - }, - amt_to_forward: 0x0badf00d01020304, + let outbound_msg = msgs::OutboundOnionPayload::Receive { + payment_data: None, + payment_metadata: None, + keysend_preimage: None, + amt_msat: 0x0badf00d01020304, outgoing_cltv_value: 0xffffffff, }; - let encoded_value = msg.encode(); + let encoded_value = outbound_msg.encode(); let target_value = hex::decode("1002080badf00d010203040404ffffffff").unwrap(); assert_eq!(encoded_value, target_value); - msg = Readable::read(&mut Cursor::new(&target_value[..])).unwrap(); - if let OnionHopDataFormat::FinalNode { payment_data: None, .. } = msg.format { } else { panic!(); } - assert_eq!(msg.amt_to_forward, 0x0badf00d01020304); - assert_eq!(msg.outgoing_cltv_value, 0xffffffff); + + let inbound_msg = Readable::read(&mut Cursor::new(&target_value[..])).unwrap(); + if let msgs::InboundOnionPayload::Receive { payment_data: None, amt_msat, outgoing_cltv_value, .. } = inbound_msg { + assert_eq!(amt_msat, 0x0badf00d01020304); + assert_eq!(outgoing_cltv_value, 0xffffffff); + } else { panic!(); } } #[test] fn encoding_final_onion_hop_data_with_secret() { let expected_payment_secret = PaymentSecret([0x42u8; 32]); - let mut msg = msgs::OnionHopData { - format: OnionHopDataFormat::FinalNode { - payment_data: Some(FinalOnionHopData { - payment_secret: expected_payment_secret, - total_msat: 0x1badca1f - }), - payment_metadata: None, - keysend_preimage: None, - }, - amt_to_forward: 0x0badf00d01020304, - outgoing_cltv_value: 0xffffffff, - }; - let encoded_value = msg.encode(); - let target_value = hex::decode("3602080badf00d010203040404ffffffff082442424242424242424242424242424242424242424242424242424242424242421badca1f").unwrap(); - assert_eq!(encoded_value, target_value); - msg = Readable::read(&mut Cursor::new(&target_value[..])).unwrap(); - if let OnionHopDataFormat::FinalNode { + let outbound_msg = msgs::OutboundOnionPayload::Receive { payment_data: Some(FinalOnionHopData { - payment_secret, + payment_secret: expected_payment_secret, total_msat: 0x1badca1f }), payment_metadata: None, keysend_preimage: None, - } = msg.format { + amt_msat: 0x0badf00d01020304, + outgoing_cltv_value: 0xffffffff, + }; + let encoded_value = outbound_msg.encode(); + let target_value = hex::decode("3602080badf00d010203040404ffffffff082442424242424242424242424242424242424242424242424242424242424242421badca1f").unwrap(); + assert_eq!(encoded_value, target_value); + + let inbound_msg = Readable::read(&mut Cursor::new(&target_value[..])).unwrap(); + if let msgs::InboundOnionPayload::Receive { + payment_data: Some(FinalOnionHopData { + payment_secret, + total_msat: 0x1badca1f + }), + amt_msat, outgoing_cltv_value, + payment_metadata: None, + keysend_preimage: None, + } = inbound_msg { assert_eq!(payment_secret, expected_payment_secret); + assert_eq!(amt_msat, 0x0badf00d01020304); + assert_eq!(outgoing_cltv_value, 0xffffffff); } else { panic!(); } - assert_eq!(msg.amt_to_forward, 0x0badf00d01020304); - assert_eq!(msg.outgoing_cltv_value, 0xffffffff); } #[test] @@ -3748,25 +3758,23 @@ mod tests { // payload length to be encoded over multiple bytes rather than a single u8. let big_payload = encode_big_payload().unwrap(); let mut rd = Cursor::new(&big_payload[..]); - ::read(&mut rd).unwrap(); + ::read(&mut rd).unwrap(); } // see above test, needs to be a separate method for use of the serialization macros. fn encode_big_payload() -> Result, io::Error> { use crate::util::ser::HighZeroBytesDroppedBigSize; - let payload = msgs::OnionHopData { - format: OnionHopDataFormat::NonFinalNode { - short_channel_id: 0xdeadbeef1bad1dea, - }, + let payload = msgs::OutboundOnionPayload::Forward { + short_channel_id: 0xdeadbeef1bad1dea, amt_to_forward: 1000, outgoing_cltv_value: 0xffffffff, }; let mut encoded_payload = Vec::new(); let test_bytes = vec![42u8; 1000]; - if let OnionHopDataFormat::NonFinalNode { short_channel_id } = payload.format { + if let msgs::OutboundOnionPayload::Forward { short_channel_id, amt_to_forward, outgoing_cltv_value } = payload { _encode_varint_length_prefixed_tlv!(&mut encoded_payload, { (1, test_bytes, required_vec), - (2, HighZeroBytesDroppedBigSize(payload.amt_to_forward), required), - (4, HighZeroBytesDroppedBigSize(payload.outgoing_cltv_value), required), + (2, HighZeroBytesDroppedBigSize(amt_to_forward), required), + (4, HighZeroBytesDroppedBigSize(outgoing_cltv_value), required), (6, short_channel_id, required) }); } diff --git a/lightning/src/ln/onion_route_tests.rs b/lightning/src/ln/onion_route_tests.rs index 01dac6843..888ac7dca 100644 --- a/lightning/src/ln/onion_route_tests.rs +++ b/lightning/src/ln/onion_route_tests.rs @@ -252,7 +252,7 @@ struct BogusOnionHopData { data: Vec } impl BogusOnionHopData { - fn new(orig: msgs::OnionHopData) -> Self { + fn new(orig: msgs::OutboundOnionPayload) -> Self { Self { data: orig.encode() } } } @@ -875,15 +875,15 @@ fn test_always_create_tlv_format_onion_payloads() { let (onion_payloads, _htlc_msat, _htlc_cltv) = onion_utils::build_onion_payloads( &route.paths[0], 40000, RecipientOnionFields::spontaneous_empty(), cur_height, &None).unwrap(); - match onion_payloads[0].format { - msgs::OnionHopDataFormat::NonFinalNode {..} => {}, + match onion_payloads[0] { + msgs::OutboundOnionPayload::Forward {..} => {}, _ => { panic!( "Should have generated a `msgs::OnionHopDataFormat::NonFinalNode` payload for `hops[0]`, despite that the features signals no support for variable length onions" )} } - match onion_payloads[1].format { - msgs::OnionHopDataFormat::FinalNode {..} => {}, + match onion_payloads[1] { + msgs::OutboundOnionPayload::Receive {..} => {}, _ => {panic!( "Should have generated a `msgs::OnionHopDataFormat::FinalNode` payload for `hops[1]`, despite that the features signals no support for variable length onions" diff --git a/lightning/src/ln/onion_utils.rs b/lightning/src/ln/onion_utils.rs index 52eb7bcb5..f3579a56b 100644 --- a/lightning/src/ln/onion_utils.rs +++ b/lightning/src/ln/onion_utils.rs @@ -149,11 +149,11 @@ pub(super) fn construct_onion_keys(secp_ctx: &Secp256k1) -> Result<(Vec, u64, u32), APIError> { +pub(super) fn build_onion_payloads(path: &Path, total_msat: u64, mut recipient_onion: RecipientOnionFields, starting_htlc_offset: u32, keysend_preimage: &Option) -> Result<(Vec, u64, u32), APIError> { let mut cur_value_msat = 0u64; let mut cur_cltv = starting_htlc_offset; let mut last_short_channel_id = 0; - let mut res: Vec = Vec::with_capacity(path.hops.len()); + let mut res: Vec = Vec::with_capacity(path.hops.len()); for (idx, hop) in path.hops.iter().rev().enumerate() { // First hop gets special values so that it can check, on receipt, that everything is @@ -161,25 +161,25 @@ pub(super) fn build_onion_payloads(path: &Path, total_msat: u64, mut recipient_o // the intended recipient). let value_msat = if cur_value_msat == 0 { hop.fee_msat } else { cur_value_msat }; let cltv = if cur_cltv == starting_htlc_offset { hop.cltv_expiry_delta + starting_htlc_offset } else { cur_cltv }; - res.insert(0, msgs::OnionHopData { - format: if idx == 0 { - msgs::OnionHopDataFormat::FinalNode { - payment_data: if let Some(secret) = recipient_onion.payment_secret.take() { - Some(msgs::FinalOnionHopData { - payment_secret: secret, - total_msat, - }) - } else { None }, - payment_metadata: recipient_onion.payment_metadata.take(), - keysend_preimage: *keysend_preimage, - } - } else { - msgs::OnionHopDataFormat::NonFinalNode { - short_channel_id: last_short_channel_id, - } - }, - amt_to_forward: value_msat, - outgoing_cltv_value: cltv, + res.insert(0, if idx == 0 { + msgs::OutboundOnionPayload::Receive { + payment_data: if let Some(secret) = recipient_onion.payment_secret.take() { + Some(msgs::FinalOnionHopData { + payment_secret: secret, + total_msat, + }) + } else { None }, + payment_metadata: recipient_onion.payment_metadata.take(), + keysend_preimage: *keysend_preimage, + amt_msat: value_msat, + outgoing_cltv_value: cltv, + } + } else { + msgs::OutboundOnionPayload::Forward { + short_channel_id: last_short_channel_id, + amt_to_forward: value_msat, + outgoing_cltv_value: cltv, + } }); cur_value_msat += hop.fee_msat; if cur_value_msat >= 21000000 * 100000000 * 1000 { @@ -208,7 +208,10 @@ fn shift_slice_right(arr: &mut [u8], amt: usize) { } } -pub(super) fn construct_onion_packet(payloads: Vec, onion_keys: Vec, prng_seed: [u8; 32], associated_data: &PaymentHash) -> Result { +pub(super) fn construct_onion_packet( + payloads: Vec, onion_keys: Vec, prng_seed: [u8; 32], + associated_data: &PaymentHash +) -> Result { let mut packet_data = [0; ONION_DATA_LEN]; let mut chacha = ChaCha20::new(&prng_seed, &[0; 8]); @@ -763,11 +766,11 @@ impl NextPacketBytes for Vec { pub(crate) enum Hop { /// This onion payload was for us, not for forwarding to a next-hop. Contains information for /// verifying the incoming payment. - Receive(msgs::OnionHopData), + Receive(msgs::InboundOnionPayload), /// This onion payload needs to be forwarded to a next-hop. Forward { /// Onion payload data used in forwarding the payment. - next_hop_data: msgs::OnionHopData, + next_hop_data: msgs::InboundOnionPayload, /// HMAC of the next hop's onion packet. next_hop_hmac: [u8; 32], /// Bytes of the onion packet we're forwarding. @@ -988,10 +991,8 @@ mod tests { // with raw hex instead of our in-memory enums, as the payloads contains custom types, and // we have no way of representing that with our enums. let payloads = vec!( - RawOnionHopData::new(msgs::OnionHopData { - format: msgs::OnionHopDataFormat::NonFinalNode { - short_channel_id: 1, - }, + RawOnionHopData::new(msgs::OutboundOnionPayload::Forward { + short_channel_id: 1, amt_to_forward: 15000, outgoing_cltv_value: 1500, }), @@ -1013,17 +1014,13 @@ mod tests { RawOnionHopData { data: hex::decode("52020236b00402057806080000000000000002fd02013c0102030405060708090a0b0c0d0e0f0102030405060708090a0b0c0d0e0f0102030405060708090a0b0c0d0e0f0102030405060708090a0b0c0d0e0f").unwrap(), }, - RawOnionHopData::new(msgs::OnionHopData { - format: msgs::OnionHopDataFormat::NonFinalNode { - short_channel_id: 3, - }, + RawOnionHopData::new(msgs::OutboundOnionPayload::Forward { + short_channel_id: 3, amt_to_forward: 12500, outgoing_cltv_value: 1250, }), - RawOnionHopData::new(msgs::OnionHopData { - format: msgs::OnionHopDataFormat::NonFinalNode { - short_channel_id: 4, - }, + RawOnionHopData::new(msgs::OutboundOnionPayload::Forward { + short_channel_id: 4, amt_to_forward: 10000, outgoing_cltv_value: 1000, }), @@ -1101,7 +1098,7 @@ mod tests { data: Vec } impl RawOnionHopData { - fn new(orig: msgs::OnionHopData) -> Self { + fn new(orig: msgs::OutboundOnionPayload) -> Self { Self { data: orig.encode() } } }