From 8bf3d8dec26cfd2e3fb4606f73ffaee174a0a403 Mon Sep 17 00:00:00 2001 From: Matt Corallo Date: Fri, 23 Apr 2021 04:04:55 +0000 Subject: [PATCH] Req+check payment secrets for inbound payments pre-PaymentReceived Our current PaymentReceived API is incredibly easy to mis-use - the "obvious" way to implement a client is to always call `ChannelManager::claim_funds` in response to a `PaymentReceived` event. However, users are *required* to check the payment secret and value against the expected values before claiming in order to avoid a number of potentially funds-losing attacks. Instead, if we rely on payment secrets being pre-registered with the ChannelManager before we receive HTLCs for a payment we can simply check the payment secrets and never generate `PaymentReceived` events if they do not match. Further, when the user knows the value to expect in advance, we can have them register it as well, allowing us to check it for them. Other implementations already require payment secrets for inbound payments, so this shouldn't materially lose compatibility. --- lightning/src/ln/channelmanager.rs | 210 +++++++++++++++----------- lightning/src/ln/functional_tests.rs | 19 ++- lightning/src/ln/onion_route_tests.rs | 7 +- 3 files changed, 142 insertions(+), 94 deletions(-) diff --git a/lightning/src/ln/channelmanager.rs b/lightning/src/ln/channelmanager.rs index bfd41d11a..853c50142 100644 --- a/lightning/src/ln/channelmanager.rs +++ b/lightning/src/ln/channelmanager.rs @@ -96,7 +96,7 @@ enum PendingHTLCRouting { short_channel_id: u64, // This should be NonZero eventually when we bump MSRV }, Receive { - payment_data: Option, + payment_data: msgs::FinalOnionHopData, incoming_cltv_expiry: u32, // Used to track when we should expire pending HTLCs that go unclaimed }, } @@ -156,11 +156,10 @@ pub(crate) struct HTLCPreviousHopData { struct ClaimableHTLC { prev_hop: HTLCPreviousHopData, value: u64, - /// Filled in when the HTLC was received with a payment_secret packet, which contains a - /// total_msat (which may differ from value if this is a Multi-Path Payment) and a + /// Contains a total_msat (which may differ from value if this is a Multi-Path Payment) and a /// payment_secret which prevents path-probing attacks and can associate different HTLCs which /// are part of the same payment. - payment_data: Option, + payment_data: msgs::FinalOnionHopData, cltv_expiry: u32, } @@ -327,12 +326,11 @@ pub(super) struct ChannelHolder { /// guarantees are made about the existence of a channel with the short id here, nor the short /// ids in the PendingHTLCInfo! pub(super) forward_htlcs: HashMap>, - /// (payment_hash, payment_secret) -> Vec for tracking HTLCs that - /// were to us and can be failed/claimed by the user + /// Map from payment hash to any HTLCs which are to us and can be failed/claimed by the user. /// Note that while this is held in the same mutex as the channels themselves, no consistency /// guarantees are made about the channels given here actually existing anymore by the time you /// go to read them! - claimable_htlcs: HashMap<(PaymentHash, Option), Vec>, + claimable_htlcs: HashMap>, /// Messages to send to peers - pushed to in the same lock that they are generated in (except /// for broadcast messages, where ordering isn't as strict). pub(super) pending_msg_events: Vec, @@ -1247,6 +1245,10 @@ impl ChannelMana msgs::OnionHopDataFormat::FinalNode { payment_data } => payment_data, }; + if payment_data.is_none() { + return_err!("We require payment_secrets", 0x4000|0x2000|3, &[0;0]); + } + // 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 // instead we stay symmetric with the forwarding case, only responding (after a @@ -1254,7 +1256,7 @@ impl ChannelMana PendingHTLCStatus::Forward(PendingHTLCInfo { routing: PendingHTLCRouting::Receive { - payment_data, + payment_data: payment_data.unwrap(), incoming_cltv_expiry: msg.cltv_expiry, }, payment_hash: msg.payment_hash.clone(), @@ -1948,61 +1950,93 @@ impl ChannelMana routing: PendingHTLCRouting::Receive { payment_data, incoming_cltv_expiry }, incoming_shared_secret, payment_hash, amt_to_forward, .. }, prev_funding_outpoint } => { - let prev_hop = HTLCPreviousHopData { - short_channel_id: prev_short_channel_id, - outpoint: prev_funding_outpoint, - htlc_id: prev_htlc_id, - incoming_packet_shared_secret: incoming_shared_secret, - }; - - let mut total_value = 0; - let payment_secret_opt = - if let &Some(ref data) = &payment_data { Some(data.payment_secret.clone()) } else { None }; - let htlcs = channel_state.claimable_htlcs.entry((payment_hash, payment_secret_opt)) - .or_insert(Vec::new()); - htlcs.push(ClaimableHTLC { - prev_hop, + let claimable_htlc = ClaimableHTLC { + prev_hop: HTLCPreviousHopData { + short_channel_id: prev_short_channel_id, + outpoint: prev_funding_outpoint, + htlc_id: prev_htlc_id, + incoming_packet_shared_secret: incoming_shared_secret, + }, value: amt_to_forward, payment_data: payment_data.clone(), cltv_expiry: incoming_cltv_expiry, - }); - if let &Some(ref data) = &payment_data { - for htlc in htlcs.iter() { - total_value += htlc.value; - if htlc.payment_data.as_ref().unwrap().total_msat != data.total_msat { - total_value = msgs::MAX_VALUE_MSAT; - } - if total_value >= msgs::MAX_VALUE_MSAT { break; } - } - if total_value >= msgs::MAX_VALUE_MSAT || total_value > data.total_msat { - for htlc in htlcs.iter() { - let mut htlc_msat_height_data = byte_utils::be64_to_array(htlc.value).to_vec(); - htlc_msat_height_data.extend_from_slice( - &byte_utils::be32_to_array(self.best_block.read().unwrap().height()), - ); - failed_forwards.push((HTLCSource::PreviousHopData(HTLCPreviousHopData { - short_channel_id: htlc.prev_hop.short_channel_id, - outpoint: prev_funding_outpoint, - htlc_id: htlc.prev_hop.htlc_id, - incoming_packet_shared_secret: htlc.prev_hop.incoming_packet_shared_secret, - }), payment_hash, - HTLCFailReason::Reason { failure_code: 0x4000 | 15, data: htlc_msat_height_data } - )); - } - } else if total_value == data.total_msat { - new_events.push(events::Event::PaymentReceived { - payment_hash, - payment_secret: Some(data.payment_secret), - amt: total_value, - }); + }; + + macro_rules! fail_htlc { + ($htlc: expr) => { + let mut htlc_msat_height_data = byte_utils::be64_to_array($htlc.value).to_vec(); + htlc_msat_height_data.extend_from_slice( + &byte_utils::be32_to_array(self.best_block.read().unwrap().height()), + ); + failed_forwards.push((HTLCSource::PreviousHopData(HTLCPreviousHopData { + short_channel_id: $htlc.prev_hop.short_channel_id, + outpoint: prev_funding_outpoint, + htlc_id: $htlc.prev_hop.htlc_id, + incoming_packet_shared_secret: $htlc.prev_hop.incoming_packet_shared_secret, + }), payment_hash, + HTLCFailReason::Reason { failure_code: 0x4000 | 15, data: htlc_msat_height_data } + )); } - } else { - new_events.push(events::Event::PaymentReceived { - payment_hash, - payment_secret: None, - amt: amt_to_forward, - }); } + + // Check that the payment hash and secret are known. Note that we + // MUST take care to handle the "unknown payment hash" and + // "incorrect payment secret" cases here identically or we'd expose + // that we are the ultimate recipient of the given payment hash. + // Further, we must not expose whether we have any other HTLCs + // associated with the same payment_hash pending or not. + let mut payment_secrets = self.pending_inbound_payments.lock().unwrap(); + match payment_secrets.entry(payment_hash) { + hash_map::Entry::Vacant(_) => { + log_trace!(self.logger, "Failing new HTLC with payment_hash {} as we didn't have a corresponding inbound payment.", log_bytes!(payment_hash.0)); + fail_htlc!(claimable_htlc); + }, + hash_map::Entry::Occupied(inbound_payment) => { + if inbound_payment.get().payment_secret != payment_data.payment_secret { + log_trace!(self.logger, "Failing new HTLC with payment_hash {} as it didn't match our expected payment secret.", log_bytes!(payment_hash.0)); + fail_htlc!(claimable_htlc); + } else if inbound_payment.get().min_value_msat.is_some() && payment_data.total_msat < inbound_payment.get().min_value_msat.unwrap() { + log_trace!(self.logger, "Failing new HTLC with payment_hash {} as it didn't match our minimum value (had {}, needed {}).", + log_bytes!(payment_hash.0), payment_data.total_msat, inbound_payment.get().min_value_msat.unwrap()); + fail_htlc!(claimable_htlc); + } else { + let mut total_value = 0; + let htlcs = channel_state.claimable_htlcs.entry(payment_hash) + .or_insert(Vec::new()); + htlcs.push(claimable_htlc); + for htlc in htlcs.iter() { + total_value += htlc.value; + if htlc.payment_data.total_msat != payment_data.total_msat { + log_trace!(self.logger, "Failing HTLCs with payment_hash {} as the HTLCs had inconsistent total values (eg {} and {})", + log_bytes!(payment_hash.0), payment_data.total_msat, htlc.payment_data.total_msat); + total_value = msgs::MAX_VALUE_MSAT; + } + if total_value >= msgs::MAX_VALUE_MSAT { break; } + } + if total_value >= msgs::MAX_VALUE_MSAT || total_value > payment_data.total_msat { + log_trace!(self.logger, "Failing HTLCs with payment_hash {} as the total value {} ran over expected value {} (or HTLCs were inconsistent)", + log_bytes!(payment_hash.0), total_value, payment_data.total_msat); + for htlc in htlcs.iter() { + fail_htlc!(htlc); + } + } else if total_value == payment_data.total_msat { + new_events.push(events::Event::PaymentReceived { + payment_hash, + payment_secret: Some(payment_data.payment_secret), + amt: total_value, + }); + // Only ever generate at most one PaymentReceived + // per registered payment_hash, even if it isn't + // claimed. + inbound_payment.remove_entry(); + } else { + // Nothing to do - we haven't reached the total + // payment value yet, wait until we receive more + // MPP parts. + } + } + }, + }; }, HTLCForwardInfo::AddHTLC { .. } => { panic!("short_channel_id == 0 should imply any pending_forward entries are of type Receive"); @@ -2092,11 +2126,7 @@ impl ChannelMana let _persistence_guard = PersistenceNotifierGuard::new(&self.total_consistency_lock, &self.persistence_notifier); let mut channel_state = Some(self.channel_state.lock().unwrap()); - let payment_secrets = self.pending_inbound_payments.lock().unwrap(); - let payment_secret = if let Some(secret) = payment_secrets.get(&payment_hash) { - Some(secret.payment_secret) - } else { return false; }; - let removed_source = channel_state.as_mut().unwrap().claimable_htlcs.remove(&(*payment_hash, payment_secret)); + let removed_source = channel_state.as_mut().unwrap().claimable_htlcs.remove(payment_hash); if let Some(mut sources) = removed_source { for htlc in sources.drain(..) { if channel_state.is_none() { channel_state = Some(self.channel_state.lock().unwrap()); } @@ -2278,11 +2308,7 @@ impl ChannelMana let _persistence_guard = PersistenceNotifierGuard::new(&self.total_consistency_lock, &self.persistence_notifier); let mut channel_state = Some(self.channel_state.lock().unwrap()); - let payment_secrets = self.pending_inbound_payments.lock().unwrap(); - let payment_secret = if let Some(secret) = payment_secrets.get(&payment_hash) { - Some(secret.payment_secret) - } else { return false; }; - let removed_source = channel_state.as_mut().unwrap().claimable_htlcs.remove(&(payment_hash, payment_secret)); + let removed_source = channel_state.as_mut().unwrap().claimable_htlcs.remove(&payment_hash); if let Some(mut sources) = removed_source { assert!(!sources.is_empty()); @@ -2298,13 +2324,8 @@ impl ChannelMana // provide the preimage, so worrying too much about the optimal handling isn't worth // it. - let (is_mpp, mut valid_mpp) = if let &Some(ref data) = &sources[0].payment_data { - assert!(payment_secret.is_some()); - (true, data.total_msat >= expected_amount) - } else { - assert!(payment_secret.is_none()); - (false, false) - }; + let is_mpp = true; + let mut valid_mpp = sources[0].payment_data.total_msat >= expected_amount; for htlc in sources.iter() { if !is_mpp || !valid_mpp { break; } @@ -3667,7 +3688,7 @@ where }); if let Some(height) = height_opt { - channel_state.claimable_htlcs.retain(|&(ref payment_hash, _), htlcs| { + channel_state.claimable_htlcs.retain(|payment_hash, htlcs| { htlcs.retain(|htlc| { // If height is approaching the number of blocks we think it takes us to get // our commitment transaction confirmed before the HTLC expires, plus the @@ -4041,7 +4062,8 @@ impl Writeable for PendingHTLCInfo { }, &PendingHTLCRouting::Receive { ref payment_data, ref incoming_cltv_expiry } => { 1u8.write(writer)?; - payment_data.write(writer)?; + payment_data.payment_secret.write(writer)?; + payment_data.total_msat.write(writer)?; incoming_cltv_expiry.write(writer)?; }, } @@ -4062,7 +4084,10 @@ impl Readable for PendingHTLCInfo { short_channel_id: Readable::read(reader)?, }, 1u8 => PendingHTLCRouting::Receive { - payment_data: Readable::read(reader)?, + payment_data: msgs::FinalOnionHopData { + payment_secret: Readable::read(reader)?, + total_msat: Readable::read(reader)?, + }, incoming_cltv_expiry: Readable::read(reader)?, }, _ => return Err(DecodeError::InvalidValue), @@ -4134,12 +4159,29 @@ impl_writeable!(HTLCPreviousHopData, 0, { incoming_packet_shared_secret }); -impl_writeable!(ClaimableHTLC, 0, { - prev_hop, - value, - payment_data, - cltv_expiry -}); +impl Writeable for ClaimableHTLC { + fn write(&self, writer: &mut W) -> Result<(), ::std::io::Error> { + self.prev_hop.write(writer)?; + self.value.write(writer)?; + self.payment_data.payment_secret.write(writer)?; + self.payment_data.total_msat.write(writer)?; + self.cltv_expiry.write(writer) + } +} + +impl Readable for ClaimableHTLC { + fn read(reader: &mut R) -> Result { + Ok(ClaimableHTLC { + prev_hop: Readable::read(reader)?, + value: Readable::read(reader)?, + payment_data: msgs::FinalOnionHopData { + payment_secret: Readable::read(reader)?, + total_msat: Readable::read(reader)?, + }, + cltv_expiry: Readable::read(reader)?, + }) + } +} impl Writeable for HTLCSource { fn write(&self, writer: &mut W) -> Result<(), ::std::io::Error> { diff --git a/lightning/src/ln/functional_tests.rs b/lightning/src/ln/functional_tests.rs index 0ca1c66cf..0e3e3f01e 100644 --- a/lightning/src/ln/functional_tests.rs +++ b/lightning/src/ln/functional_tests.rs @@ -5098,22 +5098,25 @@ fn test_onchain_to_onchain_claim() { #[test] fn test_duplicate_payment_hash_one_failure_one_success() { - // Topology : A --> B --> C + // Topology : A --> B --> C --> D // We route 2 payments with same hash between B and C, one will be timeout, the other successfully claim - let chanmon_cfgs = create_chanmon_cfgs(3); - let node_cfgs = create_node_cfgs(3, &chanmon_cfgs); - let node_chanmgrs = create_node_chanmgrs(3, &node_cfgs, &[None, None, None]); - let mut nodes = create_network(3, &node_cfgs, &node_chanmgrs); + // Note that because C will refuse to generate two payment secrets for the same payment hash, + // we forward one of the payments onwards to D. + let chanmon_cfgs = create_chanmon_cfgs(4); + let node_cfgs = create_node_cfgs(4, &chanmon_cfgs); + let node_chanmgrs = create_node_chanmgrs(4, &node_cfgs, &[None, None, None, None]); + let mut nodes = create_network(4, &node_cfgs, &node_chanmgrs); create_announced_chan_between_nodes(&nodes, 0, 1, InitFeatures::known(), InitFeatures::known()); let chan_2 = create_announced_chan_between_nodes(&nodes, 1, 2, InitFeatures::known(), InitFeatures::known()); + create_announced_chan_between_nodes(&nodes, 2, 3, InitFeatures::known(), InitFeatures::known()); let (our_payment_preimage, duplicate_payment_hash, _) = route_payment(&nodes[0], &vec!(&nodes[1], &nodes[2])[..], 900000); - let (_, _, payment_secret) = get_payment_preimage_hash!(nodes[1]); + let payment_secret = nodes[3].node.create_inbound_payment_for_hash(duplicate_payment_hash, None, 7200).unwrap(); let route = get_route(&nodes[0].node.get_our_node_id(), &nodes[0].net_graph_msg_handler.network_graph.read().unwrap(), - &nodes[2].node.get_our_node_id(), Some(InvoiceFeatures::known()), None, &Vec::new(), 900000, TEST_FINAL_CLTV, nodes[0].logger).unwrap(); - send_along_route_with_secret(&nodes[0], route, &[&[&nodes[1], &nodes[2]]], 900000, duplicate_payment_hash, payment_secret); + &nodes[3].node.get_our_node_id(), Some(InvoiceFeatures::known()), None, &Vec::new(), 900000, TEST_FINAL_CLTV, nodes[0].logger).unwrap(); + send_along_route_with_secret(&nodes[0], route, &[&[&nodes[1], &nodes[2], &nodes[3]]], 900000, duplicate_payment_hash, payment_secret); let commitment_txn = get_local_commitment_txn!(nodes[2], chan_2.2); assert_eq!(commitment_txn[0].input.len(), 1); diff --git a/lightning/src/ln/onion_route_tests.rs b/lightning/src/ln/onion_route_tests.rs index 7c3e13cf1..1de45d689 100644 --- a/lightning/src/ln/onion_route_tests.rs +++ b/lightning/src/ln/onion_route_tests.rs @@ -268,9 +268,8 @@ fn test_onion_failure() { } let channels = [create_announced_chan_between_nodes(&nodes, 0, 1, InitFeatures::known(), InitFeatures::known()), create_announced_chan_between_nodes(&nodes, 1, 2, InitFeatures::known(), InitFeatures::known())]; let (_, payment_hash, payment_secret) = get_payment_preimage_hash!(nodes[2]); - let net_graph_msg_handler = &nodes[0].net_graph_msg_handler; let logger = test_utils::TestLogger::new(); - let route = get_route(&nodes[0].node.get_our_node_id(), &net_graph_msg_handler.network_graph.read().unwrap(), &nodes[2].node.get_our_node_id(), Some(InvoiceFeatures::known()), None, &Vec::new(), 40000, TEST_FINAL_CLTV, &logger).unwrap(); + let route = get_route(&nodes[0].node.get_our_node_id(), &nodes[0].net_graph_msg_handler.network_graph.read().unwrap(), &nodes[2].node.get_our_node_id(), Some(InvoiceFeatures::known()), None, &Vec::new(), 40000, TEST_FINAL_CLTV, &logger).unwrap(); // positve case send_payment(&nodes[0], &vec!(&nodes[1], &nodes[2])[..], 40000, 40_000); @@ -328,6 +327,7 @@ fn test_onion_failure() { }, ||{ nodes[2].node.fail_htlc_backwards(&payment_hash, &None); }, true, Some(NODE|2), Some(msgs::HTLCFailChannelUpdate::NodeFailure{node_id: route.paths[0][1].pubkey, is_permanent: false})); + let (_, payment_hash, payment_secret) = get_payment_preimage_hash!(nodes[2]); // intermediate node failure run_onion_failure_test_with_fail_intercept("permanent_node_failure", 100, &nodes, &route, &payment_hash, &payment_secret, |msg| { @@ -346,6 +346,7 @@ fn test_onion_failure() { }, ||{ nodes[2].node.fail_htlc_backwards(&payment_hash, &None); }, false, Some(PERM|NODE|2), Some(msgs::HTLCFailChannelUpdate::NodeFailure{node_id: route.paths[0][1].pubkey, is_permanent: true})); + let (_, payment_hash, payment_secret) = get_payment_preimage_hash!(nodes[2]); // intermediate node failure run_onion_failure_test_with_fail_intercept("required_node_feature_missing", 100, &nodes, &route, &payment_hash, &payment_secret, |msg| { @@ -366,6 +367,7 @@ fn test_onion_failure() { }, ||{ nodes[2].node.fail_htlc_backwards(&payment_hash, &None); }, false, Some(PERM|NODE|3), Some(msgs::HTLCFailChannelUpdate::NodeFailure{node_id: route.paths[0][1].pubkey, is_permanent: true})); + let (_, payment_hash, payment_secret) = get_payment_preimage_hash!(nodes[2]); run_onion_failure_test("invalid_onion_version", 0, &nodes, &route, &payment_hash, &payment_secret, |msg| { msg.onion_routing_packet.version = 1; }, ||{}, true, Some(BADONION|PERM|4), None); @@ -439,6 +441,7 @@ fn test_onion_failure() { run_onion_failure_test("unknown_payment_hash", 2, &nodes, &route, &payment_hash, &payment_secret, |_| {}, || { nodes[2].node.fail_htlc_backwards(&payment_hash, &None); }, false, Some(PERM|15), None); + let (_, payment_hash, payment_secret) = get_payment_preimage_hash!(nodes[2]); run_onion_failure_test("final_expiry_too_soon", 1, &nodes, &route, &payment_hash, &payment_secret, |msg| { let height = msg.cltv_expiry - CLTV_CLAIM_BUFFER - LATENCY_GRACE_PERIOD_BLOCKS + 1; -- 2.39.5