From d2256301e89a05b8f76c6a080a0ee68abd5c762e Mon Sep 17 00:00:00 2001 From: Matt Corallo Date: Tue, 8 Mar 2022 21:55:02 +0000 Subject: [PATCH] Use the correct SCID when failing HTLCs to aliased channels When we fail an HTLC which was destined for a channel that the HTLC sender didn't know the real SCID for, we should ensure we continue to use the alias in the channel_update we provide them. Otherwise we will leak the channel's real SCID to HTLC senders. --- lightning/src/ln/channelmanager.rs | 43 ++++++++-- lightning/src/ln/functional_test_utils.rs | 4 +- lightning/src/ln/onion_route_tests.rs | 1 - lightning/src/ln/priv_short_conf_tests.rs | 98 ++++++++++++++++++++++- 4 files changed, 134 insertions(+), 12 deletions(-) diff --git a/lightning/src/ln/channelmanager.rs b/lightning/src/ln/channelmanager.rs index 0ca18da34..a483421db 100644 --- a/lightning/src/ln/channelmanager.rs +++ b/lightning/src/ln/channelmanager.rs @@ -2413,7 +2413,6 @@ impl ChannelMana }; let (chan_update_opt, forwardee_cltv_expiry_delta) = if let Some(forwarding_id) = forwarding_id_opt { let chan = channel_state.as_mut().unwrap().by_id.get_mut(&forwarding_id).unwrap(); - let chan_update_opt = self.get_channel_update_for_broadcast(chan).ok(); if !chan.should_announce() && !self.default_configuration.accept_forwards_to_priv_channels { // Note that the behavior here should be identical to the above block - we // should NOT reveal the existence or non-existence of a private channel if @@ -2426,6 +2425,7 @@ impl ChannelMana // we don't have the channel here. break Some(("Refusing to forward over real channel SCID as our counterparty requested.", 0x4000 | 10, None)); } + let chan_update_opt = self.get_channel_update_for_onion(*short_channel_id, chan).ok(); // Note that we could technically not return an error yet here and just hope // that the connection is reestablished or monitor updated by the time we get @@ -2525,6 +2525,10 @@ impl ChannelMana Some(id) => id, }; + self.get_channel_update_for_onion(short_channel_id, chan) + } + fn get_channel_update_for_onion(&self, short_channel_id: u64, chan: &Channel) -> Result { + log_trace!(self.logger, "Generating channel update for channel {}", log_bytes!(chan.channel_id())); let were_node_one = PublicKey::from_secret_key(&self.secp_ctx, &self.our_network_key).serialize()[..] < chan.get_counterparty_node_id().serialize()[..]; let unsigned = msgs::UnsignedChannelUpdate { @@ -3214,7 +3218,7 @@ impl ChannelMana } else { panic!("Stated return value requirements in send_htlc() were not met"); } - let (failure_code, data) = self.get_htlc_temp_fail_err_and_data(0x1000|7, chan.get()); + let (failure_code, data) = self.get_htlc_temp_fail_err_and_data(0x1000|7, short_chan_id, chan.get()); failed_forwards.push((htlc_source, payment_hash, HTLCFailReason::Reason { failure_code, data } )); @@ -3714,9 +3718,32 @@ impl ChannelMana /// Gets an HTLC onion failure code and error data for an `UPDATE` error, given the error code /// that we want to return and a channel. - fn get_htlc_temp_fail_err_and_data(&self, desired_err_code: u16, chan: &Channel) -> (u16, Vec) { + /// + /// This is for failures on the channel on which the HTLC was *received*, not failures + /// forwarding + fn get_htlc_inbound_temp_fail_err_and_data(&self, desired_err_code: u16, chan: &Channel) -> (u16, Vec) { + // We can't be sure what SCID was used when relaying inbound towards us, so we have to + // guess somewhat. If its a public channel, we figure best to just use the real SCID (as + // we're not leaking that we have a channel with the counterparty), otherwise we try to use + // an inbound SCID alias before the real SCID. + let scid_pref = if chan.should_announce() { + chan.get_short_channel_id().or(chan.latest_inbound_scid_alias()) + } else { + chan.latest_inbound_scid_alias().or(chan.get_short_channel_id()) + }; + if let Some(scid) = scid_pref { + self.get_htlc_temp_fail_err_and_data(desired_err_code, scid, chan) + } else { + (0x4000|10, Vec::new()) + } + } + + + /// Gets an HTLC onion failure code and error data for an `UPDATE` error, given the error code + /// that we want to return and a channel. + fn get_htlc_temp_fail_err_and_data(&self, desired_err_code: u16, scid: u64, chan: &Channel) -> (u16, Vec) { debug_assert_eq!(desired_err_code & 0x1000, 0x1000); - if let Ok(upd) = self.get_channel_update_for_unicast(chan) { + if let Ok(upd) = self.get_channel_update_for_onion(scid, chan) { let mut enc = VecWriter(Vec::with_capacity(upd.serialized_length() + 4)); if desired_err_code == 0x1000 | 20 { // TODO: underspecified, follow https://github.com/lightning/bolts/issues/791 @@ -3744,7 +3771,7 @@ impl ChannelMana let (failure_code, onion_failure_data) = match self.channel_state.lock().unwrap().by_id.entry(channel_id) { hash_map::Entry::Occupied(chan_entry) => { - self.get_htlc_temp_fail_err_and_data(0x1000|7, &chan_entry.get()) + self.get_htlc_inbound_temp_fail_err_and_data(0x1000|7, &chan_entry.get()) }, hash_map::Entry::Vacant(_) => (0x4000|10, Vec::new()) }; @@ -4634,7 +4661,7 @@ impl ChannelMana match pending_forward_info { PendingHTLCStatus::Forward(PendingHTLCInfo { ref incoming_shared_secret, .. }) => { let reason = if (error_code & 0x1000) != 0 { - let (real_code, error_data) = self.get_htlc_temp_fail_err_and_data(error_code, chan); + let (real_code, error_data) = self.get_htlc_inbound_temp_fail_err_and_data(error_code, chan); onion_utils::build_first_hop_failure_packet(incoming_shared_secret, real_code, &error_data) } else { onion_utils::build_first_hop_failure_packet(incoming_shared_secret, error_code, &[]) @@ -5627,8 +5654,8 @@ where let res = f(channel); if let Ok((funding_locked_opt, mut timed_out_pending_htlcs, announcement_sigs)) = res { for (source, payment_hash) in timed_out_pending_htlcs.drain(..) { - let (failure_code, data) = self.get_htlc_temp_fail_err_and_data(0x1000|14 /* expiry_too_soon */, &channel); - timed_out_htlcs.push((source, payment_hash, HTLCFailReason::Reason { + let (failure_code, data) = self.get_htlc_inbound_temp_fail_err_and_data(0x1000|14 /* expiry_too_soon */, &channel); + timed_out_htlcs.push((source, payment_hash, HTLCFailReason::Reason { failure_code, data, })); } diff --git a/lightning/src/ln/functional_test_utils.rs b/lightning/src/ln/functional_test_utils.rs index c95ac5bb0..d65bf2ee3 100644 --- a/lightning/src/ln/functional_test_utils.rs +++ b/lightning/src/ln/functional_test_utils.rs @@ -14,7 +14,7 @@ use chain::{BestBlock, Confirm, Listen, Watch, keysinterface::KeysInterface}; use chain::channelmonitor::ChannelMonitor; use chain::transaction::OutPoint; use ln::{PaymentPreimage, PaymentHash, PaymentSecret}; -use ln::channelmanager::{ChainParameters, ChannelManager, ChannelManagerReadArgs, RAACommitmentOrder, PaymentSendFailure, PaymentId}; +use ln::channelmanager::{ChainParameters, ChannelManager, ChannelManagerReadArgs, RAACommitmentOrder, PaymentSendFailure, PaymentId, MIN_CLTV_EXPIRY_DELTA}; use routing::network_graph::{NetGraphMsgHandler, NetworkGraph}; use routing::router::{PaymentParameters, Route, get_route}; use ln::features::{InitFeatures, InvoiceFeatures}; @@ -1848,7 +1848,7 @@ pub fn test_default_channel_config() -> UserConfig { let mut default_config = UserConfig::default(); // Set cltv_expiry_delta slightly lower to keep the final CLTV values inside one byte in our // tests so that our script-length checks don't fail (see ACCEPTED_HTLC_SCRIPT_WEIGHT). - default_config.channel_options.cltv_expiry_delta = 6*6; + default_config.channel_options.cltv_expiry_delta = MIN_CLTV_EXPIRY_DELTA; default_config.channel_options.announced_channel = true; default_config.peer_channel_config_limits.force_announced_channel_preference = false; // When most of our tests were written, the default HTLC minimum was fixed at 1000. diff --git a/lightning/src/ln/onion_route_tests.rs b/lightning/src/ln/onion_route_tests.rs index 070d88dda..706fcb6c2 100644 --- a/lightning/src/ln/onion_route_tests.rs +++ b/lightning/src/ln/onion_route_tests.rs @@ -993,4 +993,3 @@ fn test_phantom_failure_reject_payment() { .expected_htlc_error_data(0x4000 | 15, &error_data); expect_payment_failed_conditions!(nodes[0], payment_hash, true, fail_conditions); } - diff --git a/lightning/src/ln/priv_short_conf_tests.rs b/lightning/src/ln/priv_short_conf_tests.rs index 806f4f206..996d64f37 100644 --- a/lightning/src/ln/priv_short_conf_tests.rs +++ b/lightning/src/ln/priv_short_conf_tests.rs @@ -13,12 +13,13 @@ use chain::Watch; use chain::channelmonitor::ChannelMonitor; +use chain::keysinterface::{Recipient, KeysInterface}; use ln::channelmanager::{ChannelManager, ChannelManagerReadArgs, MIN_CLTV_EXPIRY_DELTA}; use routing::network_graph::RoutingFees; use routing::router::{RouteHint, RouteHintHop}; use ln::features::InitFeatures; use ln::msgs; -use ln::msgs::{ChannelMessageHandler, RoutingMessageHandler}; +use ln::msgs::{ChannelMessageHandler, RoutingMessageHandler, OptionalField}; use util::enforcing_trait_impls::EnforcingSigner; use util::events::{Event, MessageSendEvent, MessageSendEventsProvider}; use util::config::UserConfig; @@ -30,7 +31,12 @@ use core::default::Default; use ln::functional_test_utils::*; +use bitcoin::blockdata::constants::genesis_block; use bitcoin::hash_types::BlockHash; +use bitcoin::hashes::Hash; +use bitcoin::hashes::sha256d::Hash as Sha256dHash; +use bitcoin::network::constants::Network; +use bitcoin::secp256k1::Secp256k1; #[test] fn test_priv_forwarding_rejection() { @@ -445,3 +451,93 @@ fn test_inbound_scid_privacy() { PaymentFailedConditions::new().blamed_scid(last_hop[0].short_channel_id.unwrap()) .blamed_chan_closed(true).expected_htlc_error_data(0x4000|10, &[0; 0])); } + +#[test] +fn test_scid_alias_returned() { + // Tests that when we fail an HTLC (in this case due to attempting to forward more than the + // channel's available balance) we use the correct (in this case the aliased) SCID in the + // channel_update which is returned in the onion to the sender. + let chanmon_cfgs = create_chanmon_cfgs(3); + let node_cfgs = create_node_cfgs(3, &chanmon_cfgs); + let mut accept_forward_cfg = test_default_channel_config(); + accept_forward_cfg.accept_forwards_to_priv_channels = true; + let node_chanmgrs = create_node_chanmgrs(3, &node_cfgs, &[None, Some(accept_forward_cfg), None]); + let nodes = create_network(3, &node_cfgs, &node_chanmgrs); + + create_announced_chan_between_nodes_with_value(&nodes, 0, 1, 10_000_000, 0, InitFeatures::known(), InitFeatures::known()); + create_unannounced_chan_between_nodes_with_value(&nodes, 1, 2, 10_000, 0, InitFeatures::known(), InitFeatures::known()); + + let last_hop = nodes[2].node.list_usable_channels(); + let mut hop_hints = vec![RouteHint(vec![RouteHintHop { + src_node_id: nodes[1].node.get_our_node_id(), + short_channel_id: last_hop[0].inbound_scid_alias.unwrap(), + fees: RoutingFees { + base_msat: last_hop[0].counterparty.forwarding_info.as_ref().unwrap().fee_base_msat, + proportional_millionths: last_hop[0].counterparty.forwarding_info.as_ref().unwrap().fee_proportional_millionths, + }, + cltv_expiry_delta: last_hop[0].counterparty.forwarding_info.as_ref().unwrap().cltv_expiry_delta, + htlc_maximum_msat: None, + htlc_minimum_msat: None, + }])]; + let (mut route, payment_hash, _, payment_secret) = get_route_and_payment_hash!(nodes[0], nodes[2], hop_hints, 10_000, 42); + assert_eq!(route.paths[0][1].short_channel_id, nodes[2].node.list_usable_channels()[0].inbound_scid_alias.unwrap()); + + route.paths[0][1].fee_msat = 10_000_000; // Overshoot the last channel's value + + // Route the HTLC through to the destination. + nodes[0].node.send_payment(&route, payment_hash, &Some(payment_secret)).unwrap(); + check_added_monitors!(nodes[0], 1); + let as_updates = get_htlc_update_msgs!(nodes[0], nodes[1].node.get_our_node_id()); + nodes[1].node.handle_update_add_htlc(&nodes[0].node.get_our_node_id(), &as_updates.update_add_htlcs[0]); + commitment_signed_dance!(nodes[1], nodes[0], &as_updates.commitment_signed, false, true); + + expect_pending_htlcs_forwardable!(nodes[1]); + expect_pending_htlcs_forwardable!(nodes[1]); + check_added_monitors!(nodes[1], 1); + + let bs_updates = get_htlc_update_msgs!(nodes[1], nodes[0].node.get_our_node_id()); + nodes[0].node.handle_update_fail_htlc(&nodes[1].node.get_our_node_id(), &bs_updates.update_fail_htlcs[0]); + commitment_signed_dance!(nodes[0], nodes[1], bs_updates.commitment_signed, false, true); + + // Build the expected channel update + let contents = msgs::UnsignedChannelUpdate { + chain_hash: genesis_block(Network::Testnet).header.block_hash(), + short_channel_id: last_hop[0].inbound_scid_alias.unwrap(), + timestamp: 21, + flags: 1, + cltv_expiry_delta: accept_forward_cfg.channel_options.cltv_expiry_delta, + htlc_minimum_msat: 1_000, + htlc_maximum_msat: OptionalField::Present(1_000_000), // Defaults to 10% of the channel value + fee_base_msat: last_hop[0].counterparty.forwarding_info.as_ref().unwrap().fee_base_msat, + fee_proportional_millionths: last_hop[0].counterparty.forwarding_info.as_ref().unwrap().fee_proportional_millionths, + excess_data: Vec::new(), + }; + let msg_hash = Sha256dHash::hash(&contents.encode()[..]); + let signature = Secp256k1::new().sign(&hash_to_message!(&msg_hash[..]), &nodes[1].keys_manager.get_node_secret(Recipient::Node).unwrap()); + let msg = msgs::ChannelUpdate { signature, contents }; + + expect_payment_failed_conditions!(nodes[0], payment_hash, false, + PaymentFailedConditions::new().blamed_scid(last_hop[0].inbound_scid_alias.unwrap()) + .blamed_chan_closed(false).expected_htlc_error_data(0x1000|7, &msg.encode_with_len())); + + route.paths[0][1].fee_msat = 10_000; // Reset to the correct payment amount + route.paths[0][0].fee_msat = 0; // But set fee paid to the middle hop to 0 + + // Route the HTLC through to the destination. + nodes[0].node.send_payment(&route, payment_hash, &Some(payment_secret)).unwrap(); + check_added_monitors!(nodes[0], 1); + let as_updates = get_htlc_update_msgs!(nodes[0], nodes[1].node.get_our_node_id()); + nodes[1].node.handle_update_add_htlc(&nodes[0].node.get_our_node_id(), &as_updates.update_add_htlcs[0]); + commitment_signed_dance!(nodes[1], nodes[0], &as_updates.commitment_signed, false, true); + + let bs_updates = get_htlc_update_msgs!(nodes[1], nodes[0].node.get_our_node_id()); + nodes[0].node.handle_update_fail_htlc(&nodes[1].node.get_our_node_id(), &bs_updates.update_fail_htlcs[0]); + commitment_signed_dance!(nodes[0], nodes[1], bs_updates.commitment_signed, false, true); + + let mut err_data = Vec::new(); + err_data.extend_from_slice(&10_000u64.to_be_bytes()); + err_data.extend_from_slice(&msg.encode_with_len()); + expect_payment_failed_conditions!(nodes[0], payment_hash, false, + PaymentFailedConditions::new().blamed_scid(last_hop[0].inbound_scid_alias.unwrap()) + .blamed_chan_closed(false).expected_htlc_error_data(0x1000|12, &err_data)); +} -- 2.39.5