diff --git a/lightning/src/ln/onion_payment.rs b/lightning/src/ln/onion_payment.rs index 06a9ae07e1e..7c0d32a52a5 100644 --- a/lightning/src/ln/onion_payment.rs +++ b/lightning/src/ln/onion_payment.rs @@ -23,6 +23,7 @@ use crate::prelude::*; use core::ops::Deref; /// Invalid inbound onion payment. +#[derive(Debug)] pub struct InboundOnionErr { /// BOLT 4 error code. pub err_code: u16, @@ -448,7 +449,7 @@ pub(super) fn check_incoming_htlc_cltv( mod tests { use bitcoin::hashes::Hash; use bitcoin::hashes::sha256::Hash as Sha256; - use bitcoin::secp256k1::{PublicKey, SecretKey}; + use bitcoin::secp256k1::{PublicKey, Secp256k1, SecretKey}; use crate::ln::{PaymentPreimage, PaymentHash, PaymentSecret}; use crate::ln::ChannelId; use crate::ln::channelmanager::RecipientOnionFields; @@ -458,6 +459,38 @@ mod tests { use crate::routing::router::{Path, RouteHop}; use crate::util::test_utils; + #[test] + fn fail_construct_onion_on_too_big_payloads() { + // Ensure that if we call `construct_onion_packet` and friends where payloads are too large for + // the allotted packet length, we'll fail to construct. Previously, senders would happily + // construct invalid packets by array-shifting the final node's HMAC out of the packet when + // adding an intermediate onion layer, causing the receiver to error with "final payload + // provided for us as an intermediate node." + let secp_ctx = Secp256k1::new(); + let bob = crate::sign::KeysManager::new(&[2; 32], 42, 42); + let bob_pk = PublicKey::from_secret_key(&secp_ctx, &bob.get_node_secret_key()); + let charlie = crate::sign::KeysManager::new(&[3; 32], 42, 42); + let charlie_pk = PublicKey::from_secret_key(&secp_ctx, &charlie.get_node_secret_key()); + + let ( + session_priv, total_amt_msat, cur_height, mut recipient_onion, keysend_preimage, payment_hash, + prng_seed, hops, .. + ) = payment_onion_args(bob_pk, charlie_pk); + + // Ensure the onion will not fit all the payloads by adding a large custom TLV. + recipient_onion.custom_tlvs.push((13377331, vec![0; 1156])); + + let path = Path { hops, blinded_tail: None, }; + let onion_keys = super::onion_utils::construct_onion_keys(&secp_ctx, &path, &session_priv).unwrap(); + let (onion_payloads, ..) = super::onion_utils::build_onion_payloads( + &path, total_amt_msat, recipient_onion, cur_height + 1, &Some(keysend_preimage) + ).unwrap(); + + assert!(super::onion_utils::construct_onion_packet( + onion_payloads, onion_keys, prng_seed, &payment_hash + ).is_err()); + } + #[test] fn test_peel_payment_onion() { use super::*; diff --git a/lightning/src/ln/onion_utils.rs b/lightning/src/ln/onion_utils.rs index 051e78c46ee..ea906c9e32a 100644 --- a/lightning/src/ln/onion_utils.rs +++ b/lightning/src/ln/onion_utils.rs @@ -323,8 +323,6 @@ fn construct_onion_packet_with_init_noise( let mut pos = 0; for (i, (payload, keys)) in payloads.iter().zip(onion_keys.iter()).enumerate() { - if i == payloads.len() - 1 { break; } - let mut chacha = ChaCha20::new(&keys.rho, &[0u8; 8]); for _ in 0..(packet_data.len() - pos) { // TODO: Batch this. let mut dummy = [0; 1]; @@ -338,6 +336,8 @@ fn construct_onion_packet_with_init_noise( return Err(()); } + if i == payloads.len() - 1 { break; } + res.resize(pos, 0u8); chacha.process_in_place(&mut res); } @@ -1021,18 +1021,20 @@ fn decode_next_hop, N: NextPacketBytes>(shared_secret: [u8 if hmac == [0; 32] { #[cfg(test)] { - // In tests, make sure that the initial onion packet data is, at least, non-0. - // We could do some fancy randomness test here, but, ehh, whatever. - // This checks for the issue where you can calculate the path length given the - // onion data as all the path entries that the originator sent will be here - // as-is (and were originally 0s). - // Of course reverse path calculation is still pretty easy given naive routing - // algorithms, but this fixes the most-obvious case. - let mut next_bytes = [0; 32]; - chacha_stream.read_exact(&mut next_bytes).unwrap(); - assert_ne!(next_bytes[..], [0; 32][..]); - chacha_stream.read_exact(&mut next_bytes).unwrap(); - assert_ne!(next_bytes[..], [0; 32][..]); + if chacha_stream.read.position() < hop_data.len() as u64 - 64 { + // In tests, make sure that the initial onion packet data is, at least, non-0. + // We could do some fancy randomness test here, but, ehh, whatever. + // This checks for the issue where you can calculate the path length given the + // onion data as all the path entries that the originator sent will be here + // as-is (and were originally 0s). + // Of course reverse path calculation is still pretty easy given naive routing + // algorithms, but this fixes the most-obvious case. + let mut next_bytes = [0; 32]; + chacha_stream.read_exact(&mut next_bytes).unwrap(); + assert_ne!(next_bytes[..], [0; 32][..]); + chacha_stream.read_exact(&mut next_bytes).unwrap(); + assert_ne!(next_bytes[..], [0; 32][..]); + } } return Ok((msg, None)); // We are the final destination for this packet } else { diff --git a/lightning/src/ln/payment_tests.rs b/lightning/src/ln/payment_tests.rs index a5654b30f65..01f59528989 100644 --- a/lightning/src/ln/payment_tests.rs +++ b/lightning/src/ln/payment_tests.rs @@ -19,8 +19,9 @@ use crate::events::{ClosureReason, Event, HTLCDestination, MessageSendEvent, Mes use crate::ln::channel::{EXPIRE_PREV_CONFIG_TICKS, commit_tx_fee_msat, get_holder_selected_channel_reserve_satoshis, ANCHOR_OUTPUT_VALUE_SATOSHI}; use crate::ln::channelmanager::{BREAKDOWN_TIMEOUT, MPP_TIMEOUT_TICKS, MIN_CLTV_EXPIRY_DELTA, PaymentId, PaymentSendFailure, RecentPaymentDetails, RecipientOnionFields, HTLCForwardInfo, PendingHTLCRouting, PendingAddHTLCInfo}; use crate::ln::features::{Bolt11InvoiceFeatures, ChannelTypeFeatures}; -use crate::ln::{msgs, ChannelId, PaymentSecret, PaymentPreimage}; +use crate::ln::{msgs, ChannelId, PaymentHash, PaymentSecret, PaymentPreimage}; use crate::ln::msgs::ChannelMessageHandler; +use crate::ln::onion_utils; use crate::ln::outbound_payment::{IDEMPOTENCY_TIMEOUT_TICKS, Retry}; use crate::routing::gossip::{EffectiveCapacity, RoutingFees}; use crate::routing::router::{get_route, Path, PaymentParameters, Route, Router, RouteHint, RouteHintHop, RouteHop, RouteParameters, find_route}; @@ -31,10 +32,14 @@ use crate::util::errors::APIError; use crate::util::ser::Writeable; use crate::util::string::UntrustedString; +use bitcoin::hashes::Hash; +use bitcoin::hashes::sha256::Hash as Sha256; use bitcoin::network::constants::Network; +use bitcoin::secp256k1::{Secp256k1, SecretKey}; use crate::prelude::*; +use crate::ln::functional_test_utils; use crate::ln::functional_test_utils::*; use crate::routing::gossip::NodeId; #[cfg(feature = "std")] @@ -4194,3 +4199,59 @@ fn test_htlc_forward_considers_anchor_outputs_value() { check_closed_broadcast(&nodes[2], 1, true); check_added_monitors(&nodes[2], 1); } + +#[test] +fn peel_payment_onion_custom_tlvs() { + let chanmon_cfgs = create_chanmon_cfgs(2); + let node_cfgs = create_node_cfgs(2, &chanmon_cfgs); + let node_chanmgrs = create_node_chanmgrs(2, &node_cfgs, &[None, None]); + let nodes = create_network(2, &node_cfgs, &node_chanmgrs); + create_announced_chan_between_nodes(&nodes, 0, 1); + let secp_ctx = Secp256k1::new(); + + let amt_msat = 1000; + let payment_params = PaymentParameters::for_keysend(nodes[1].node.get_our_node_id(), + TEST_FINAL_CLTV, false); + let route_params = RouteParameters::from_payment_params_and_value(payment_params, amt_msat); + let route = functional_test_utils::get_route(&nodes[0], &route_params).unwrap(); + let mut recipient_onion = RecipientOnionFields::spontaneous_empty() + .with_custom_tlvs(vec![(414141, vec![42; 1200])]).unwrap(); + let prng_seed = chanmon_cfgs[0].keys_manager.get_secure_random_bytes(); + let session_priv = SecretKey::from_slice(&prng_seed[..]).expect("RNG is busted"); + let keysend_preimage = PaymentPreimage([42; 32]); + let payment_hash = PaymentHash(Sha256::hash(&keysend_preimage.0).to_byte_array()); + + let (onion_routing_packet, first_hop_msat, cltv_expiry) = onion_utils::create_payment_onion( + &secp_ctx, &route.paths[0], &session_priv, amt_msat, recipient_onion.clone(), + nodes[0].best_block_info().1, &payment_hash, &Some(keysend_preimage), prng_seed + ).unwrap(); + + let update_add = msgs::UpdateAddHTLC { + channel_id: ChannelId([0; 32]), + htlc_id: 42, + amount_msat: first_hop_msat, + payment_hash, + cltv_expiry, + skimmed_fee_msat: None, + onion_routing_packet, + blinding_point: None, + }; + let peeled_onion = crate::ln::onion_payment::peel_payment_onion( + &update_add, &&chanmon_cfgs[1].keys_manager, &&chanmon_cfgs[1].logger, &secp_ctx, + nodes[1].best_block_info().1, true, false + ).unwrap(); + assert_eq!(peeled_onion.incoming_amt_msat, Some(amt_msat)); + match peeled_onion.routing { + PendingHTLCRouting::ReceiveKeysend { + payment_data, payment_metadata, custom_tlvs, .. + } => { + #[cfg(not(c_bindings))] + assert_eq!(&custom_tlvs, recipient_onion.custom_tlvs()); + #[cfg(c_bindings)] + assert_eq!(custom_tlvs, recipient_onion.custom_tlvs()); + assert!(payment_metadata.is_none()); + assert!(payment_data.is_none()); + }, + _ => panic!() + } +}