From 1f6700c72d8b23d53b3d2866993e3d39dc1905a3 Mon Sep 17 00:00:00 2001 From: Valentine Wallace Date: Sat, 19 Feb 2022 16:41:38 -0500 Subject: [PATCH 1/6] Follow-up nits from #1199 (phantom node support) --- fuzz/src/full_stack.rs | 3 +++ lightning/src/util/scid_utils.rs | 11 ++++------- 2 files changed, 7 insertions(+), 7 deletions(-) diff --git a/fuzz/src/full_stack.rs b/fuzz/src/full_stack.rs index 19ec541b942..6800a3fe179 100644 --- a/fuzz/src/full_stack.rs +++ b/fuzz/src/full_stack.rs @@ -390,6 +390,9 @@ pub fn do_test(data: &[u8], logger: &Arc) { best_block: BestBlock::from_genesis(network), }; let channelmanager = Arc::new(ChannelManager::new(fee_est.clone(), monitor.clone(), broadcast.clone(), Arc::clone(&logger), keys_manager.clone(), config, params)); + // Adding new calls to `KeysInterface::get_secure_random_bytes` during startup can change all the + // keys subsequently generated in this test. Rather than regenerating all the messages manually, + // it's easier to just increment the counter here so the keys don't change. keys_manager.counter.fetch_sub(1, Ordering::AcqRel); let our_id = PublicKey::from_secret_key(&Secp256k1::signing_only(), &keys_manager.get_node_secret(Recipient::Node).unwrap()); let network_graph = Arc::new(NetworkGraph::new(genesis_block(network).block_hash())); diff --git a/lightning/src/util/scid_utils.rs b/lightning/src/util/scid_utils.rs index f9dfd1b0320..11b8d0c95e0 100644 --- a/lightning/src/util/scid_utils.rs +++ b/lightning/src/util/scid_utils.rs @@ -104,18 +104,15 @@ pub(crate) mod fake_scid { let rand_bytes = keys_manager.get_secure_random_bytes(); let segwit_activation_height = segwit_activation_height(genesis_hash); - let mut valid_block_range = if highest_seen_blockheight > segwit_activation_height { - highest_seen_blockheight - segwit_activation_height - } else { - 1 - }; + let mut blocks_since_segwit_activation = highest_seen_blockheight.saturating_sub(segwit_activation_height); + // We want to ensure that this fake channel won't conflict with any transactions we haven't // seen yet, in case `highest_seen_blockheight` is updated before we get full information // about transactions confirmed in the given block. - if valid_block_range > BLOCKS_PER_MONTH { valid_block_range -= BLOCKS_PER_MONTH; } + blocks_since_segwit_activation = blocks_since_segwit_activation.saturating_sub(BLOCKS_PER_MONTH); let rand_for_height = u32::from_be_bytes(rand_bytes[..4].try_into().unwrap()); - let fake_scid_height = segwit_activation_height + rand_for_height % valid_block_range; + let fake_scid_height = segwit_activation_height + rand_for_height % (blocks_since_segwit_activation + 1); let rand_for_tx_index = u32::from_be_bytes(rand_bytes[4..8].try_into().unwrap()); let fake_scid_tx_index = rand_for_tx_index % MAX_TX_INDEX; From bafd141d2c10f10cfe824fdf673d6683a420db7d Mon Sep 17 00:00:00 2001 From: Valentine Wallace Date: Thu, 24 Feb 2022 21:18:48 -0500 Subject: [PATCH 2/6] Add phantom shared secret to PendingHTLCRouting::Receive This will be used in upcoming commit(s) to encrypt phantom payment failure packets. --- lightning/src/ln/channelmanager.rs | 21 ++++++++++++--------- 1 file changed, 12 insertions(+), 9 deletions(-) diff --git a/lightning/src/ln/channelmanager.rs b/lightning/src/ln/channelmanager.rs index 0a0407eacff..302760fef0a 100644 --- a/lightning/src/ln/channelmanager.rs +++ b/lightning/src/ln/channelmanager.rs @@ -366,6 +366,7 @@ enum PendingHTLCRouting { Receive { payment_data: msgs::FinalOnionHopData, incoming_cltv_expiry: u32, // Used to track when we should expire pending HTLCs that go unclaimed + phantom_shared_secret: Option<[u8; 32]>, }, ReceiveKeysend { payment_preimage: PaymentPreimage, @@ -2072,7 +2073,7 @@ impl ChannelMana } fn construct_recv_pending_htlc_info(&self, hop_data: msgs::OnionHopData, shared_secret: [u8; 32], - payment_hash: PaymentHash, amt_msat: u64, cltv_expiry: u32) -> Result + payment_hash: PaymentHash, amt_msat: u64, cltv_expiry: u32, phantom_shared_secret: Option<[u8; 32]>) -> Result { // final_incorrect_cltv_expiry if hop_data.outgoing_cltv_value != cltv_expiry { @@ -2129,6 +2130,7 @@ impl ChannelMana PendingHTLCRouting::Receive { payment_data: data, incoming_cltv_expiry: hop_data.outgoing_cltv_value, + phantom_shared_secret, } } else if let Some(payment_preimage) = keysend_preimage { // We need to check that the sender knows the keysend preimage before processing this @@ -2232,7 +2234,7 @@ impl ChannelMana let pending_forward_info = match next_hop { onion_utils::Hop::Receive(next_hop_data) => { // OUR PAYMENT! - match self.construct_recv_pending_htlc_info(next_hop_data, shared_secret, msg.payment_hash, msg.amount_msat, msg.cltv_expiry) { + match self.construct_recv_pending_htlc_info(next_hop_data, shared_secret, msg.payment_hash, msg.amount_msat, msg.cltv_expiry, None) { Ok(info) => { // Note that we could obviously respond immediately with an update_fulfill_htlc // message, however that would leak that we are the recipient of this payment, so @@ -3031,12 +3033,12 @@ impl ChannelMana if let PendingHTLCRouting::Forward { onion_packet, .. } = routing { let phantom_secret_res = self.keys_manager.get_node_secret(Recipient::PhantomNode); if phantom_secret_res.is_ok() && fake_scid::is_valid_phantom(&self.fake_scid_rand_bytes, short_chan_id) { - let shared_secret = { + let phantom_shared_secret = { let mut arr = [0; 32]; arr.copy_from_slice(&SharedSecret::new(&onion_packet.public_key.unwrap(), &phantom_secret_res.unwrap())[..]); arr }; - let next_hop = match onion_utils::decode_next_hop(shared_secret, &onion_packet.hop_data, onion_packet.hmac, payment_hash) { + let next_hop = match onion_utils::decode_next_hop(phantom_shared_secret, &onion_packet.hop_data, onion_packet.hmac, payment_hash) { Ok(res) => res, Err(onion_utils::OnionDecodeErr::Malformed { err_msg, err_code }) => { fail_forward!(err_msg, err_code, Vec::new()); @@ -3047,7 +3049,7 @@ impl ChannelMana }; match next_hop { onion_utils::Hop::Receive(hop_data) => { - match self.construct_recv_pending_htlc_info(hop_data, shared_secret, payment_hash, amt_to_forward, outgoing_cltv_value) { + match self.construct_recv_pending_htlc_info(hop_data, incoming_shared_secret, payment_hash, amt_to_forward, outgoing_cltv_value, Some(phantom_shared_secret)) { Ok(info) => phantom_receives.push((prev_short_channel_id, prev_funding_outpoint, vec![(info, prev_htlc_id)])), Err(ReceiveError { err_code, err_data, msg }) => fail_forward!(msg, err_code, err_data) } @@ -3207,11 +3209,11 @@ impl ChannelMana HTLCForwardInfo::AddHTLC { prev_short_channel_id, prev_htlc_id, forward_info: PendingHTLCInfo { routing, incoming_shared_secret, payment_hash, amt_to_forward, .. }, prev_funding_outpoint } => { - let (cltv_expiry, onion_payload) = match routing { - PendingHTLCRouting::Receive { payment_data, incoming_cltv_expiry } => - (incoming_cltv_expiry, OnionPayload::Invoice(payment_data)), + let (cltv_expiry, onion_payload, phantom_shared_secret) = match routing { + PendingHTLCRouting::Receive { payment_data, incoming_cltv_expiry, phantom_shared_secret } => + (incoming_cltv_expiry, OnionPayload::Invoice(payment_data), phantom_shared_secret), PendingHTLCRouting::ReceiveKeysend { payment_preimage, incoming_cltv_expiry } => - (incoming_cltv_expiry, OnionPayload::Spontaneous(payment_preimage)), + (incoming_cltv_expiry, OnionPayload::Spontaneous(payment_preimage), None), _ => { panic!("short_channel_id == 0 should imply any pending_forward entries are of type Receive"); } @@ -5979,6 +5981,7 @@ impl_writeable_tlv_based_enum!(PendingHTLCRouting, }, (1, Receive) => { (0, payment_data, required), + (1, phantom_shared_secret, option), (2, incoming_cltv_expiry, required), }, (2, ReceiveKeysend) => { From f1aba795217496db6b7e2418fb2edcbc35b71ad2 Mon Sep 17 00:00:00 2001 From: Valentine Wallace Date: Thu, 24 Feb 2022 22:14:02 -0500 Subject: [PATCH 3/6] Add phantom shared secret to HTLCPreviousHopData This also fixes a bug where we were failing back phantom payments with the wrong scid, causing them to never actually be failed backwards (L3022 in channelmanager.rs) This new field will be used in upcoming commit(s) to encrypt phantom payment failure packets. --- lightning/src/ln/channelmanager.rs | 23 +++++++++++++++-------- 1 file changed, 15 insertions(+), 8 deletions(-) diff --git a/lightning/src/ln/channelmanager.rs b/lightning/src/ln/channelmanager.rs index 302760fef0a..a6856b41198 100644 --- a/lightning/src/ln/channelmanager.rs +++ b/lightning/src/ln/channelmanager.rs @@ -420,6 +420,7 @@ pub(crate) struct HTLCPreviousHopData { short_channel_id: u64, htlc_id: u64, incoming_packet_shared_secret: [u8; 32], + phantom_shared_secret: Option<[u8; 32]>, // This field is consumed by `claim_funds_from_hop()` when updating a force-closed backwards // channel with a preimage provided by the forward channel. @@ -3014,17 +3015,18 @@ impl ChannelMana routing, incoming_shared_secret, payment_hash, amt_to_forward, outgoing_cltv_value }, prev_funding_outpoint } => { macro_rules! fail_forward { - ($msg: expr, $err_code: expr, $err_data: expr) => { + ($msg: expr, $err_code: expr, $err_data: expr, $phantom_ss: expr) => { { log_info!(self.logger, "Failed to accept/forward incoming HTLC: {}", $msg); let htlc_source = HTLCSource::PreviousHopData(HTLCPreviousHopData { - short_channel_id: short_chan_id, + short_channel_id: prev_short_channel_id, outpoint: prev_funding_outpoint, htlc_id: prev_htlc_id, incoming_packet_shared_secret: incoming_shared_secret, + phantom_shared_secret: $phantom_ss, }); failed_forwards.push((htlc_source, payment_hash, - HTLCFailReason::Reason { failure_code: $err_code, data: $err_data } + HTLCFailReason::Reason { failure_code: $err_code, data: $err_data } )); continue; } @@ -3041,26 +3043,26 @@ impl ChannelMana let next_hop = match onion_utils::decode_next_hop(phantom_shared_secret, &onion_packet.hop_data, onion_packet.hmac, payment_hash) { Ok(res) => res, Err(onion_utils::OnionDecodeErr::Malformed { err_msg, err_code }) => { - fail_forward!(err_msg, err_code, Vec::new()); + fail_forward!(err_msg, err_code, Vec::new(), None); }, Err(onion_utils::OnionDecodeErr::Relay { err_msg, err_code }) => { - fail_forward!(err_msg, err_code, Vec::new()); + fail_forward!(err_msg, err_code, Vec::new(), Some(phantom_shared_secret)); }, }; match next_hop { onion_utils::Hop::Receive(hop_data) => { match self.construct_recv_pending_htlc_info(hop_data, incoming_shared_secret, payment_hash, amt_to_forward, outgoing_cltv_value, Some(phantom_shared_secret)) { Ok(info) => phantom_receives.push((prev_short_channel_id, prev_funding_outpoint, vec![(info, prev_htlc_id)])), - Err(ReceiveError { err_code, err_data, msg }) => fail_forward!(msg, err_code, err_data) + Err(ReceiveError { err_code, err_data, msg }) => fail_forward!(msg, err_code, err_data, Some(phantom_shared_secret)) } }, _ => panic!(), } } else { - fail_forward!(format!("Unknown short channel id {} for forward HTLC", short_chan_id), 0x4000 | 10, Vec::new()); + fail_forward!(format!("Unknown short channel id {} for forward HTLC", short_chan_id), 0x4000 | 10, Vec::new(), None); } } else { - fail_forward!(format!("Unknown short channel id {} for forward HTLC", short_chan_id), 0x4000 | 10, Vec::new()); + fail_forward!(format!("Unknown short channel id {} for forward HTLC", short_chan_id), 0x4000 | 10, Vec::new(), None); } }, HTLCForwardInfo::FailHTLC { .. } => { @@ -3093,6 +3095,8 @@ impl ChannelMana outpoint: prev_funding_outpoint, htlc_id: prev_htlc_id, incoming_packet_shared_secret: incoming_shared_secret, + // Phantom payments are only PendingHTLCRouting::Receive. + phantom_shared_secret: None, }); match chan.get_mut().send_htlc(amt_to_forward, payment_hash, outgoing_cltv_value, htlc_source.clone(), onion_packet, &self.logger) { Err(e) => { @@ -3224,6 +3228,7 @@ impl ChannelMana outpoint: prev_funding_outpoint, htlc_id: prev_htlc_id, incoming_packet_shared_secret: incoming_shared_secret, + phantom_shared_secret, }, value: amt_to_forward, cltv_expiry, @@ -3241,6 +3246,7 @@ impl ChannelMana outpoint: prev_funding_outpoint, htlc_id: $htlc.prev_hop.htlc_id, incoming_packet_shared_secret: $htlc.prev_hop.incoming_packet_shared_secret, + phantom_shared_secret, }), payment_hash, HTLCFailReason::Reason { failure_code: 0x4000 | 15, data: htlc_msat_height_data } )); @@ -6073,6 +6079,7 @@ impl_writeable_tlv_based_enum!(PendingHTLCStatus, ; impl_writeable_tlv_based!(HTLCPreviousHopData, { (0, short_channel_id, required), + (1, phantom_shared_secret, option), (2, outpoint, required), (4, htlc_id, required), (6, incoming_packet_shared_secret, required) From 3faea334389a2cf92b2122a8aa464d63de21ca70 Mon Sep 17 00:00:00 2001 From: Valentine Wallace Date: Thu, 24 Feb 2022 22:19:20 -0500 Subject: [PATCH 4/6] Fix phantom malformed onion error packet Ensure we fail back phantom malformed payments with an update_fail_htlc s.t. the error contains the sha256 of the onion, per LN protocol. --- lightning/src/ln/channelmanager.rs | 13 ++- lightning/src/ln/onion_route_tests.rs | 114 ++++++++++++++++++++++++-- 2 files changed, 118 insertions(+), 9 deletions(-) diff --git a/lightning/src/ln/channelmanager.rs b/lightning/src/ln/channelmanager.rs index a6856b41198..373b0a19930 100644 --- a/lightning/src/ln/channelmanager.rs +++ b/lightning/src/ln/channelmanager.rs @@ -358,7 +358,7 @@ mod inbound_payment { // our payment, which we can use to decode errors or inform the user that the payment was sent. #[derive(Clone)] // See Channel::revoke_and_ack for why, tl;dr: Rust bug -enum PendingHTLCRouting { +pub(super) enum PendingHTLCRouting { Forward { onion_packet: msgs::OnionPacket, short_channel_id: u64, // This should be NonZero eventually when we bump MSRV @@ -376,8 +376,8 @@ enum PendingHTLCRouting { #[derive(Clone)] // See Channel::revoke_and_ack for why, tl;dr: Rust bug pub(super) struct PendingHTLCInfo { - routing: PendingHTLCRouting, - incoming_shared_secret: [u8; 32], + pub(super) routing: PendingHTLCRouting, + pub(super) incoming_shared_secret: [u8; 32], payment_hash: PaymentHash, pub(super) amt_to_forward: u64, pub(super) outgoing_cltv_value: u32, @@ -3043,7 +3043,12 @@ impl ChannelMana let next_hop = match onion_utils::decode_next_hop(phantom_shared_secret, &onion_packet.hop_data, onion_packet.hmac, payment_hash) { Ok(res) => res, Err(onion_utils::OnionDecodeErr::Malformed { err_msg, err_code }) => { - fail_forward!(err_msg, err_code, Vec::new(), None); + let sha256_of_onion = Sha256::hash(&onion_packet.hop_data).into_inner(); + // In this scenario, the phantom would have sent us an + // `update_fail_malformed_htlc`, meaning here we encrypt the error as + // if it came from us (the second-to-last hop) but contains the sha256 + // of the onion. + fail_forward!(err_msg, err_code, sha256_of_onion.to_vec(), None); }, Err(onion_utils::OnionDecodeErr::Relay { err_msg, err_code }) => { fail_forward!(err_msg, err_code, Vec::new(), Some(phantom_shared_secret)); diff --git a/lightning/src/ln/onion_route_tests.rs b/lightning/src/ln/onion_route_tests.rs index 4feae104d75..4936941ea2d 100644 --- a/lightning/src/ln/onion_route_tests.rs +++ b/lightning/src/ln/onion_route_tests.rs @@ -12,25 +12,28 @@ //! returned errors decode to the correct thing. use chain::channelmonitor::{CLTV_CLAIM_BUFFER, LATENCY_GRACE_PERIOD_BLOCKS}; +use chain::keysinterface::{KeysInterface, Recipient}; use ln::{PaymentHash, PaymentSecret}; -use ln::channelmanager::{HTLCForwardInfo, CLTV_FAR_FAR_AWAY}; +use ln::channelmanager::{HTLCForwardInfo, CLTV_FAR_FAR_AWAY, MIN_CLTV_EXPIRY_DELTA, PendingHTLCInfo, PendingHTLCRouting}; use ln::onion_utils; -use routing::network_graph::NetworkUpdate; -use routing::router::Route; -use ln::features::InitFeatures; +use routing::network_graph::{NetworkUpdate, RoutingFees}; +use routing::router::{get_route, PaymentParameters, Route, RouteHint, RouteHintHop}; +use ln::features::{InitFeatures, InvoiceFeatures}; use ln::msgs; use ln::msgs::{ChannelMessageHandler, ChannelUpdate, OptionalField}; use util::events::{Event, MessageSendEvent, MessageSendEventsProvider}; use util::ser::{Writeable, Writer}; +use util::test_utils; use util::config::UserConfig; use bitcoin::hash_types::BlockHash; use bitcoin::hashes::Hash; +use bitcoin::hashes::sha256::Hash as Sha256; use bitcoin::secp256k1; use bitcoin::secp256k1::Secp256k1; -use bitcoin::secp256k1::key::SecretKey; +use bitcoin::secp256k1::key::{PublicKey, SecretKey}; use io; use prelude::*; @@ -573,3 +576,104 @@ fn test_onion_failure() { nodes[2].node.fail_htlc_backwards(&payment_hash); }, true, Some(23), None, None); } + +macro_rules! get_phantom_route { + ($nodes: expr, $amt: expr, $channel: expr) => {{ + let secp_ctx = Secp256k1::new(); + let phantom_secret = $nodes[1].keys_manager.get_node_secret(Recipient::PhantomNode).unwrap(); + let phantom_pubkey = PublicKey::from_secret_key(&secp_ctx, &phantom_secret); + let phantom_route_hint = $nodes[1].node.get_phantom_route_hints(); + let payment_params = PaymentParameters::from_node_id(phantom_pubkey) + .with_features(InvoiceFeatures::known()) + .with_route_hints(vec![RouteHint(vec![ + RouteHintHop { + src_node_id: $nodes[0].node.get_our_node_id(), + short_channel_id: $channel.0.contents.short_channel_id, + fees: RoutingFees { + base_msat: $channel.0.contents.fee_base_msat, + proportional_millionths: $channel.0.contents.fee_proportional_millionths, + }, + cltv_expiry_delta: $channel.0.contents.cltv_expiry_delta, + htlc_minimum_msat: None, + htlc_maximum_msat: None, + }, + RouteHintHop { + src_node_id: phantom_route_hint.real_node_pubkey, + short_channel_id: phantom_route_hint.phantom_scid, + fees: RoutingFees { + base_msat: 0, + proportional_millionths: 0, + }, + cltv_expiry_delta: MIN_CLTV_EXPIRY_DELTA, + htlc_minimum_msat: None, + htlc_maximum_msat: None, + } + ])]); + let scorer = test_utils::TestScorer::with_penalty(0); + (get_route( + &$nodes[0].node.get_our_node_id(), &payment_params, $nodes[0].network_graph, + Some(&$nodes[0].node.list_usable_channels().iter().collect::>()), + $amt, TEST_FINAL_CLTV, $nodes[0].logger, &scorer + ).unwrap(), phantom_route_hint.phantom_scid) + } +}} + +#[test] +fn test_phantom_onion_hmac_failure() { + 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); + + let channel = create_announced_chan_between_nodes(&nodes, 0, 1, InitFeatures::known(), InitFeatures::known()); + + // Get the route. + let recv_value_msat = 10_000; + let (_, payment_hash, payment_secret) = get_payment_preimage_hash!(nodes[1], Some(recv_value_msat)); + let (route, phantom_scid) = get_phantom_route!(nodes, recv_value_msat, channel); + + // Route the HTLC through to the destination. + nodes[0].node.send_payment(&route, payment_hash.clone(), &Some(payment_secret)).unwrap(); + check_added_monitors!(nodes[0], 1); + let update_0 = get_htlc_update_msgs!(nodes[0], nodes[1].node.get_our_node_id()); + let mut update_add = update_0.update_add_htlcs[0].clone(); + + nodes[1].node.handle_update_add_htlc(&nodes[0].node.get_our_node_id(), &update_add); + commitment_signed_dance!(nodes[1], nodes[0], &update_0.commitment_signed, false, true); + + // Modify the payload so the phantom hop's HMAC is bogus. + let sha256_of_onion = { + let mut channel_state = nodes[1].node.channel_state.lock().unwrap(); + let mut pending_forward = channel_state.forward_htlcs.get_mut(&phantom_scid).unwrap(); + match pending_forward[0] { + HTLCForwardInfo::AddHTLC { + forward_info: PendingHTLCInfo { + routing: PendingHTLCRouting::Forward { ref mut onion_packet, .. }, + .. + }, .. + } => { + onion_packet.hmac[onion_packet.hmac.len() - 1] ^= 1; + Sha256::hash(&onion_packet.hop_data).into_inner().to_vec() + }, + _ => panic!("Unexpected forward"), + } + }; + expect_pending_htlcs_forwardable_ignore!(nodes[1]); + nodes[1].node.process_pending_htlc_forwards(); + expect_pending_htlcs_forwardable_ignore!(nodes[1]); + nodes[1].node.process_pending_htlc_forwards(); + let update_1 = get_htlc_update_msgs!(nodes[1], nodes[0].node.get_our_node_id()); + check_added_monitors!(&nodes[1], 1); + assert!(update_1.update_fail_htlcs.len() == 1); + let fail_msg = update_1.update_fail_htlcs[0].clone(); + nodes[0].node.handle_update_fail_htlc(&nodes[1].node.get_our_node_id(), &fail_msg); + commitment_signed_dance!(nodes[0], nodes[1], update_1.commitment_signed, false); + + // Ensure the payment fails with the expected error. + let mut fail_conditions = PaymentFailedConditions::new() + .blamed_scid(phantom_scid) + .blamed_chan_closed(true) + .expected_htlc_error_data(0x8000 | 0x4000 | 5, &sha256_of_onion); + expect_payment_failed_conditions!(nodes[0], payment_hash, false, fail_conditions); +} + From 26fe87989607ff459b7f30cd676a6a2b9f8cf1b0 Mon Sep 17 00:00:00 2001 From: Valentine Wallace Date: Thu, 24 Feb 2022 22:28:58 -0500 Subject: [PATCH 5/6] Correctly wrap phantom onion errors In any place where fail_htlc_backwards_internal was called for a phantom payment failure, we weren't encoding the onion failure as if the phantom were the one failing. Instead, we were encoding the failure as if it were coming from the second-to-last hop. This caused our failures to not be parsed properly on the payer's side. Places we were encoding failures incorrectly include: * on failure of a call to inbound_payment::verify * on a user call to fail_htlc_backwards Also drop some unnecessary panics when reading OnionHopData objects. This also enables one of the phantom failure tests because we can construct OnionHopDatas with invalid amounts. Lastly, remove a bogus comment --- lightning/src/ln/channelmanager.rs | 15 +- lightning/src/ln/msgs.rs | 7 - lightning/src/ln/onion_route_tests.rs | 275 +++++++++++++++++++++++++- 3 files changed, 283 insertions(+), 14 deletions(-) diff --git a/lightning/src/ln/channelmanager.rs b/lightning/src/ln/channelmanager.rs index 373b0a19930..1c620bf4078 100644 --- a/lightning/src/ln/channelmanager.rs +++ b/lightning/src/ln/channelmanager.rs @@ -3075,9 +3075,6 @@ impl ChannelMana // the channel is now on chain and our counterparty is // trying to broadcast the HTLC-Timeout, but that's their // problem, not ours. - // - // `fail_htlc_backwards_internal` is never called for - // phantom payments, so this is unreachable for them. } } } @@ -3792,12 +3789,18 @@ impl ChannelMana pending_events.push(path_failure); if let Some(ev) = full_failure_ev { pending_events.push(ev); } }, - HTLCSource::PreviousHopData(HTLCPreviousHopData { short_channel_id, htlc_id, incoming_packet_shared_secret, .. }) => { + HTLCSource::PreviousHopData(HTLCPreviousHopData { short_channel_id, htlc_id, incoming_packet_shared_secret, phantom_shared_secret, .. }) => { let err_packet = match onion_error { HTLCFailReason::Reason { failure_code, data } => { log_trace!(self.logger, "Failing HTLC with payment_hash {} backwards from us with code {}", log_bytes!(payment_hash.0), failure_code); - let packet = onion_utils::build_failure_packet(&incoming_packet_shared_secret, failure_code, &data[..]).encode(); - onion_utils::encrypt_failure_packet(&incoming_packet_shared_secret, &packet) + if let Some(phantom_ss) = phantom_shared_secret { + let phantom_packet = onion_utils::build_failure_packet(&phantom_ss, failure_code, &data[..]).encode(); + let encrypted_phantom_packet = onion_utils::encrypt_failure_packet(&phantom_ss, &phantom_packet); + onion_utils::encrypt_failure_packet(&incoming_packet_shared_secret, &encrypted_phantom_packet.data[..]) + } else { + let packet = onion_utils::build_failure_packet(&incoming_packet_shared_secret, failure_code, &data[..]).encode(); + onion_utils::encrypt_failure_packet(&incoming_packet_shared_secret, &packet) + } }, HTLCFailReason::LightningError { err } => { log_trace!(self.logger, "Failing HTLC with payment_hash {} backwards with pre-built LightningError", log_bytes!(payment_hash.0)); diff --git a/lightning/src/ln/msgs.rs b/lightning/src/ln/msgs.rs index 203e2426fec..131c4fb54d2 100644 --- a/lightning/src/ln/msgs.rs +++ b/lightning/src/ln/msgs.rs @@ -1291,10 +1291,6 @@ impl Readable for FinalOnionHopData { impl Writeable for OnionHopData { fn write(&self, w: &mut W) -> Result<(), io::Error> { - // Note that this should never be reachable if Rust-Lightning generated the message, as we - // check values are sane long before we get here, though its possible in the future - // user-generated messages may hit this. - if self.amt_to_forward > MAX_VALUE_MSAT { panic!("We should never be sending infinite/overflow onion payments"); } match self.format { OnionHopDataFormat::Legacy { short_channel_id } => { 0u8.write(w)?; @@ -1311,9 +1307,6 @@ impl Writeable for OnionHopData { }); }, OnionHopDataFormat::FinalNode { ref payment_data, ref keysend_preimage } => { - if let Some(final_data) = payment_data { - if final_data.total_msat > MAX_VALUE_MSAT { panic!("We should never be sending infinite/overflow onion payments"); } - } encode_varint_length_prefixed_tlv!(w, { (2, HighZeroBytesDroppedVarInt(self.amt_to_forward), required), (4, HighZeroBytesDroppedVarInt(self.outgoing_cltv_value), required), diff --git a/lightning/src/ln/onion_route_tests.rs b/lightning/src/ln/onion_route_tests.rs index 4936941ea2d..87fb788446c 100644 --- a/lightning/src/ln/onion_route_tests.rs +++ b/lightning/src/ln/onion_route_tests.rs @@ -23,7 +23,7 @@ use ln::msgs; use ln::msgs::{ChannelMessageHandler, ChannelUpdate, OptionalField}; use util::events::{Event, MessageSendEvent, MessageSendEventsProvider}; use util::ser::{Writeable, Writer}; -use util::test_utils; +use util::{byte_utils, test_utils}; use util::config::UserConfig; use bitcoin::hash_types::BlockHash; @@ -677,3 +677,276 @@ fn test_phantom_onion_hmac_failure() { expect_payment_failed_conditions!(nodes[0], payment_hash, false, fail_conditions); } +#[test] +fn test_phantom_invalid_onion_payload() { + 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 mut nodes = create_network(2, &node_cfgs, &node_chanmgrs); + + let channel = create_announced_chan_between_nodes(&nodes, 0, 1, InitFeatures::known(), InitFeatures::known()); + + // Get the route. + let recv_value_msat = 10_000; + let (_, payment_hash, payment_secret) = get_payment_preimage_hash!(nodes[1], Some(recv_value_msat)); + let (route, phantom_scid) = get_phantom_route!(nodes, recv_value_msat, channel); + + // We'll use the session priv later when constructing an invalid onion packet. + let session_priv = [3; 32]; + *nodes[0].keys_manager.override_session_priv.lock().unwrap() = Some(session_priv); + nodes[0].node.send_payment(&route, payment_hash.clone(), &Some(payment_secret)).unwrap(); + check_added_monitors!(nodes[0], 1); + let update_0 = get_htlc_update_msgs!(nodes[0], nodes[1].node.get_our_node_id()); + let mut update_add = update_0.update_add_htlcs[0].clone(); + + nodes[1].node.handle_update_add_htlc(&nodes[0].node.get_our_node_id(), &update_add); + commitment_signed_dance!(nodes[1], nodes[0], &update_0.commitment_signed, false, true); + + // Modify the onion packet to have an invalid payment amount. + for (_, pending_forwards) in nodes[1].node.channel_state.lock().unwrap().forward_htlcs.iter_mut() { + for f in pending_forwards.iter_mut() { + match f { + &mut HTLCForwardInfo::AddHTLC { + forward_info: PendingHTLCInfo { + routing: PendingHTLCRouting::Forward { ref mut onion_packet, .. }, + .. + }, .. + } => { + // Construct the onion payloads for the entire route and an invalid amount. + let height = nodes[0].best_block_info().1; + let session_priv = SecretKey::from_slice(&session_priv).unwrap(); + let mut onion_keys = onion_utils::construct_onion_keys(&Secp256k1::new(), &route.paths[0], &session_priv).unwrap(); + let (mut onion_payloads, _, _) = onion_utils::build_onion_payloads(&route.paths[0], msgs::MAX_VALUE_MSAT + 1, &Some(payment_secret), height + 1, &None).unwrap(); + // We only want to construct the onion packet for the last hop, not the entire route, so + // remove the first hop's payload and its keys. + onion_keys.remove(0); + onion_payloads.remove(0); + + let new_onion_packet = onion_utils::construct_onion_packet(onion_payloads, onion_keys, [0; 32], &payment_hash); + onion_packet.hop_data = new_onion_packet.hop_data; + onion_packet.hmac = new_onion_packet.hmac; + }, + _ => panic!("Unexpected forward"), + } + } + } + expect_pending_htlcs_forwardable_ignore!(nodes[1]); + nodes[1].node.process_pending_htlc_forwards(); + expect_pending_htlcs_forwardable_ignore!(nodes[1]); + nodes[1].node.process_pending_htlc_forwards(); + let update_1 = get_htlc_update_msgs!(nodes[1], nodes[0].node.get_our_node_id()); + check_added_monitors!(&nodes[1], 1); + assert!(update_1.update_fail_htlcs.len() == 1); + let fail_msg = update_1.update_fail_htlcs[0].clone(); + nodes[0].node.handle_update_fail_htlc(&nodes[1].node.get_our_node_id(), &fail_msg); + commitment_signed_dance!(nodes[0], nodes[1], update_1.commitment_signed, false); + + // Ensure the payment fails with the expected error. + let error_data = Vec::new(); + let mut fail_conditions = PaymentFailedConditions::new() + .blamed_scid(phantom_scid) + .blamed_chan_closed(true) + .expected_htlc_error_data(0x4000 | 22, &error_data); + expect_payment_failed_conditions!(nodes[0], payment_hash, true, fail_conditions); +} + +#[test] +fn test_phantom_final_incorrect_cltv_expiry() { + 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); + + let channel = create_announced_chan_between_nodes(&nodes, 0, 1, InitFeatures::known(), InitFeatures::known()); + + // Get the route. + let recv_value_msat = 10_000; + let (_, payment_hash, payment_secret) = get_payment_preimage_hash!(nodes[1], Some(recv_value_msat)); + let (route, phantom_scid) = get_phantom_route!(nodes, recv_value_msat, channel); + + // Route the HTLC through to the destination. + nodes[0].node.send_payment(&route, payment_hash.clone(), &Some(payment_secret)).unwrap(); + check_added_monitors!(nodes[0], 1); + let update_0 = get_htlc_update_msgs!(nodes[0], nodes[1].node.get_our_node_id()); + let mut update_add = update_0.update_add_htlcs[0].clone(); + + nodes[1].node.handle_update_add_htlc(&nodes[0].node.get_our_node_id(), &update_add); + commitment_signed_dance!(nodes[1], nodes[0], &update_0.commitment_signed, false, true); + + // Modify the payload so the phantom hop's HMAC is bogus. + for (_, pending_forwards) in nodes[1].node.channel_state.lock().unwrap().forward_htlcs.iter_mut() { + for f in pending_forwards.iter_mut() { + match f { + &mut HTLCForwardInfo::AddHTLC { + forward_info: PendingHTLCInfo { ref mut outgoing_cltv_value, .. }, .. + } => { + *outgoing_cltv_value += 1; + }, + _ => panic!("Unexpected forward"), + } + } + } + expect_pending_htlcs_forwardable_ignore!(nodes[1]); + nodes[1].node.process_pending_htlc_forwards(); + expect_pending_htlcs_forwardable_ignore!(nodes[1]); + nodes[1].node.process_pending_htlc_forwards(); + let update_1 = get_htlc_update_msgs!(nodes[1], nodes[0].node.get_our_node_id()); + check_added_monitors!(&nodes[1], 1); + assert!(update_1.update_fail_htlcs.len() == 1); + let fail_msg = update_1.update_fail_htlcs[0].clone(); + nodes[0].node.handle_update_fail_htlc(&nodes[1].node.get_our_node_id(), &fail_msg); + commitment_signed_dance!(nodes[0], nodes[1], update_1.commitment_signed, false); + + // Ensure the payment fails with the expected error. + let expected_cltv = 82; + let error_data = byte_utils::be32_to_array(expected_cltv).to_vec(); + let mut fail_conditions = PaymentFailedConditions::new() + .blamed_scid(phantom_scid) + .expected_htlc_error_data(18, &error_data); + expect_payment_failed_conditions!(nodes[0], payment_hash, false, fail_conditions); +} + +#[test] +fn test_phantom_failure_too_low_cltv() { + 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); + + let channel = create_announced_chan_between_nodes(&nodes, 0, 1, InitFeatures::known(), InitFeatures::known()); + + // Get the route. + let recv_value_msat = 10_000; + let (_, payment_hash, payment_secret) = get_payment_preimage_hash!(nodes[1], Some(recv_value_msat)); + let (mut route, phantom_scid) = get_phantom_route!(nodes, recv_value_msat, channel); + + // Modify the route to have a too-low cltv. + route.paths[0][1].cltv_expiry_delta = 5; + + // Route the HTLC through to the destination. + nodes[0].node.send_payment(&route, payment_hash.clone(), &Some(payment_secret)).unwrap(); + check_added_monitors!(nodes[0], 1); + let update_0 = get_htlc_update_msgs!(nodes[0], nodes[1].node.get_our_node_id()); + let mut update_add = update_0.update_add_htlcs[0].clone(); + + nodes[1].node.handle_update_add_htlc(&nodes[0].node.get_our_node_id(), &update_add); + commitment_signed_dance!(nodes[1], nodes[0], &update_0.commitment_signed, false, true); + + expect_pending_htlcs_forwardable_ignore!(nodes[1]); + nodes[1].node.process_pending_htlc_forwards(); + expect_pending_htlcs_forwardable_ignore!(nodes[1]); + nodes[1].node.process_pending_htlc_forwards(); + let update_1 = get_htlc_update_msgs!(nodes[1], nodes[0].node.get_our_node_id()); + check_added_monitors!(&nodes[1], 1); + assert!(update_1.update_fail_htlcs.len() == 1); + let fail_msg = update_1.update_fail_htlcs[0].clone(); + nodes[0].node.handle_update_fail_htlc(&nodes[1].node.get_our_node_id(), &fail_msg); + commitment_signed_dance!(nodes[0], nodes[1], update_1.commitment_signed, false); + + // Ensure the payment fails with the expected error. + let error_data = Vec::new(); + let mut fail_conditions = PaymentFailedConditions::new() + .blamed_scid(phantom_scid) + .expected_htlc_error_data(17, &error_data); + expect_payment_failed_conditions!(nodes[0], payment_hash, false, fail_conditions); +} + +#[test] +fn test_phantom_failure_too_low_recv_amt() { + 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); + + let channel = create_announced_chan_between_nodes(&nodes, 0, 1, InitFeatures::known(), InitFeatures::known()); + + // Get the route with a too-low amount. + let recv_amt_msat = 10_000; + let bad_recv_amt_msat = recv_amt_msat - 10; + let (_, payment_hash, payment_secret) = get_payment_preimage_hash!(nodes[1], Some(recv_amt_msat)); + let (mut route, phantom_scid) = get_phantom_route!(nodes, bad_recv_amt_msat, channel); + + // Route the HTLC through to the destination. + nodes[0].node.send_payment(&route, payment_hash.clone(), &Some(payment_secret)).unwrap(); + check_added_monitors!(nodes[0], 1); + let update_0 = get_htlc_update_msgs!(nodes[0], nodes[1].node.get_our_node_id()); + let mut update_add = update_0.update_add_htlcs[0].clone(); + + nodes[1].node.handle_update_add_htlc(&nodes[0].node.get_our_node_id(), &update_add); + commitment_signed_dance!(nodes[1], nodes[0], &update_0.commitment_signed, false, true); + + expect_pending_htlcs_forwardable_ignore!(nodes[1]); + nodes[1].node.process_pending_htlc_forwards(); + expect_pending_htlcs_forwardable_ignore!(nodes[1]); + nodes[1].node.process_pending_htlc_forwards(); + expect_pending_htlcs_forwardable_ignore!(nodes[1]); + nodes[1].node.process_pending_htlc_forwards(); + let update_1 = get_htlc_update_msgs!(nodes[1], nodes[0].node.get_our_node_id()); + check_added_monitors!(&nodes[1], 1); + assert!(update_1.update_fail_htlcs.len() == 1); + let fail_msg = update_1.update_fail_htlcs[0].clone(); + nodes[0].node.handle_update_fail_htlc(&nodes[1].node.get_our_node_id(), &fail_msg); + commitment_signed_dance!(nodes[0], nodes[1], update_1.commitment_signed, false); + + // Ensure the payment fails with the expected error. + let mut error_data = byte_utils::be64_to_array(bad_recv_amt_msat).to_vec(); + error_data.extend_from_slice( + &byte_utils::be32_to_array(nodes[1].node.best_block.read().unwrap().height()), + ); + let mut fail_conditions = PaymentFailedConditions::new() + .blamed_scid(phantom_scid) + .expected_htlc_error_data(0x4000 | 15, &error_data); + expect_payment_failed_conditions!(nodes[0], payment_hash, true, fail_conditions); +} + +#[test] +fn test_phantom_failure_reject_payment() { + // Test that the user can successfully fail back a phantom node payment. + 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); + + let channel = create_announced_chan_between_nodes(&nodes, 0, 1, InitFeatures::known(), InitFeatures::known()); + + // Get the route with a too-low amount. + let recv_amt_msat = 10_000; + let (_, payment_hash, payment_secret) = get_payment_preimage_hash!(nodes[1], Some(recv_amt_msat)); + let (mut route, phantom_scid) = get_phantom_route!(nodes, recv_amt_msat, channel); + + // Route the HTLC through to the destination. + nodes[0].node.send_payment(&route, payment_hash.clone(), &Some(payment_secret)).unwrap(); + check_added_monitors!(nodes[0], 1); + let update_0 = get_htlc_update_msgs!(nodes[0], nodes[1].node.get_our_node_id()); + let mut update_add = update_0.update_add_htlcs[0].clone(); + + nodes[1].node.handle_update_add_htlc(&nodes[0].node.get_our_node_id(), &update_add); + commitment_signed_dance!(nodes[1], nodes[0], &update_0.commitment_signed, false, true); + + expect_pending_htlcs_forwardable_ignore!(nodes[1]); + nodes[1].node.process_pending_htlc_forwards(); + expect_pending_htlcs_forwardable_ignore!(nodes[1]); + nodes[1].node.process_pending_htlc_forwards(); + expect_payment_received!(nodes[1], payment_hash, payment_secret, recv_amt_msat); + assert!(nodes[1].node.fail_htlc_backwards(&payment_hash)); + expect_pending_htlcs_forwardable_ignore!(nodes[1]); + nodes[1].node.process_pending_htlc_forwards(); + + let update_1 = get_htlc_update_msgs!(nodes[1], nodes[0].node.get_our_node_id()); + check_added_monitors!(&nodes[1], 1); + assert!(update_1.update_fail_htlcs.len() == 1); + let fail_msg = update_1.update_fail_htlcs[0].clone(); + nodes[0].node.handle_update_fail_htlc(&nodes[1].node.get_our_node_id(), &fail_msg); + commitment_signed_dance!(nodes[0], nodes[1], update_1.commitment_signed, false); + + // Ensure the payment fails with the expected error. + let mut error_data = byte_utils::be64_to_array(recv_amt_msat).to_vec(); + error_data.extend_from_slice( + &byte_utils::be32_to_array(nodes[1].node.best_block.read().unwrap().height()), + ); + let mut fail_conditions = PaymentFailedConditions::new() + .blamed_scid(phantom_scid) + .expected_htlc_error_data(0x4000 | 15, &error_data); + expect_payment_failed_conditions!(nodes[0], payment_hash, true, fail_conditions); +} + From 694ef1ecb9be7e0cc952729e2c7e232036ce8173 Mon Sep 17 00:00:00 2001 From: Valentine Wallace Date: Wed, 23 Feb 2022 22:20:43 -0500 Subject: [PATCH 6/6] Fix bug where we encode flags field into all updates on htlc fail Failing an HTLC with onion error channel_disabled requires encoding a 'flags' field into the failure packet. However, we were encoding this 'flags' field for all failures packets that were failing on update_add_htlc with an update (error 0x1000 UPDATE). Discovered in the course of adding phantom payment failure tests, which also added testing for this bug --- lightning/src/ln/channelmanager.rs | 4 ++- lightning/src/ln/onion_route_tests.rs | 43 +++++++++++++++++++++++++++ 2 files changed, 46 insertions(+), 1 deletion(-) diff --git a/lightning/src/ln/channelmanager.rs b/lightning/src/ln/channelmanager.rs index 1c620bf4078..14e47e71d4b 100644 --- a/lightning/src/ln/channelmanager.rs +++ b/lightning/src/ln/channelmanager.rs @@ -4504,7 +4504,9 @@ impl ChannelMana onion_utils::build_first_hop_failure_packet(incoming_shared_secret, error_code, &{ let mut res = Vec::with_capacity(8 + 128); // TODO: underspecified, follow https://github.com/lightningnetwork/lightning-rfc/issues/791 - res.extend_from_slice(&byte_utils::be16_to_array(0)); + if error_code == 0x1000 | 20 { + res.extend_from_slice(&byte_utils::be16_to_array(0)); + } res.extend_from_slice(&upd.encode_with_len()[..]); res }[..]) diff --git a/lightning/src/ln/onion_route_tests.rs b/lightning/src/ln/onion_route_tests.rs index 87fb788446c..414c98d4e85 100644 --- a/lightning/src/ln/onion_route_tests.rs +++ b/lightning/src/ln/onion_route_tests.rs @@ -899,6 +899,49 @@ fn test_phantom_failure_too_low_recv_amt() { expect_payment_failed_conditions!(nodes[0], payment_hash, true, fail_conditions); } +#[test] +fn test_phantom_dust_exposure_failure() { + // Set the max dust exposure to the dust limit. + let max_dust_exposure = 546; + let mut receiver_config = UserConfig::default(); + receiver_config.channel_options.max_dust_htlc_exposure_msat = max_dust_exposure; + receiver_config.channel_options.announced_channel = true; + + 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, Some(receiver_config)]); + let nodes = create_network(2, &node_cfgs, &node_chanmgrs); + + let channel = create_announced_chan_between_nodes(&nodes, 0, 1, InitFeatures::known(), InitFeatures::known()); + + // Get the route with an amount exceeding the dust exposure threshold of nodes[1]. + let (_, payment_hash, payment_secret) = get_payment_preimage_hash!(nodes[1], Some(max_dust_exposure + 1)); + let (mut route, _) = get_phantom_route!(nodes, max_dust_exposure + 1, channel); + + // Route the HTLC through to the destination. + nodes[0].node.send_payment(&route, payment_hash.clone(), &Some(payment_secret)).unwrap(); + check_added_monitors!(nodes[0], 1); + let update_0 = get_htlc_update_msgs!(nodes[0], nodes[1].node.get_our_node_id()); + let mut update_add = update_0.update_add_htlcs[0].clone(); + + nodes[1].node.handle_update_add_htlc(&nodes[0].node.get_our_node_id(), &update_add); + commitment_signed_dance!(nodes[1], nodes[0], &update_0.commitment_signed, false, true); + + let update_1 = get_htlc_update_msgs!(nodes[1], nodes[0].node.get_our_node_id()); + assert!(update_1.update_fail_htlcs.len() == 1); + let fail_msg = update_1.update_fail_htlcs[0].clone(); + nodes[0].node.handle_update_fail_htlc(&nodes[1].node.get_our_node_id(), &fail_msg); + commitment_signed_dance!(nodes[0], nodes[1], update_1.commitment_signed, false); + + // Ensure the payment fails with the expected error. + let mut error_data = channel.1.encode_with_len(); + let mut fail_conditions = PaymentFailedConditions::new() + .blamed_scid(channel.0.contents.short_channel_id) + .blamed_chan_closed(false) + .expected_htlc_error_data(0x1000 | 7, &error_data); + expect_payment_failed_conditions!(nodes[0], payment_hash, false, fail_conditions); +} + #[test] fn test_phantom_failure_reject_payment() { // Test that the user can successfully fail back a phantom node payment.