From dec3fb316a7905284d64bb5bad88a898d04744d1 Mon Sep 17 00:00:00 2001 From: Alec Chen Date: Thu, 8 Jun 2023 12:08:25 -0500 Subject: [PATCH] Enforce explicit claims on payments with even custom TLVs Because we don't know which custom TLV type numbers the user is expecting (and it would be cumbersome for them to tell us), instead of failing unknown even custom TLVs on deserialization, we accept all custom TLVs, and pass them to the user to check whether they recognize them and choose to fail back if they don't. However, a user may not check for custom TLVs, in which case we should reject any even custom TLVs as unknown. This commit makes sure a user must explicitly accept a payment with even custom TLVs, by (1) making the default `ChannelManager::claim_funds` fail if the payment had even custom TLVs and (2) adding a new function `ChannelManager::claim_funds_with_known_custom_tlvs` that accepts them. This commit also refactors our custom TLVs test and updates various documentation to account for this. --- lightning/src/events/mod.rs | 18 +++++++++-- lightning/src/ln/channelmanager.rs | 39 +++++++++++++++++++++++ lightning/src/ln/functional_test_utils.rs | 3 ++ lightning/src/ln/payment_tests.rs | 35 ++++++++++++++++---- 4 files changed, 86 insertions(+), 9 deletions(-) diff --git a/lightning/src/events/mod.rs b/lightning/src/events/mod.rs index d08e563cb..f8d251c91 100644 --- a/lightning/src/events/mod.rs +++ b/lightning/src/events/mod.rs @@ -356,9 +356,19 @@ pub enum Event { /// Note that if the preimage is not known, you should call /// [`ChannelManager::fail_htlc_backwards`] or [`ChannelManager::fail_htlc_backwards_with_reason`] /// to free up resources for this HTLC and avoid network congestion. - /// If you fail to call either [`ChannelManager::claim_funds`], [`ChannelManager::fail_htlc_backwards`], - /// or [`ChannelManager::fail_htlc_backwards_with_reason`] within the HTLC's timeout, the HTLC will be - /// automatically failed. + /// + /// If [`Event::PaymentClaimable::onion_fields`] is `Some`, and includes custom TLVs with even type + /// numbers, you should use [`ChannelManager::fail_htlc_backwards_with_reason`] with + /// [`FailureCode::InvalidOnionPayload`] if you fail to understand and handle the contents, or + /// [`ChannelManager::claim_funds_with_known_custom_tlvs`] upon successful handling. + /// If you don't intend to check for custom TLVs, you can simply use + /// [`ChannelManager::claim_funds`], which will automatically fail back even custom TLVs. + /// + /// If you fail to call [`ChannelManager::claim_funds`], + /// [`ChannelManager::claim_funds_with_known_custom_tlvs`], + /// [`ChannelManager::fail_htlc_backwards`], or + /// [`ChannelManager::fail_htlc_backwards_with_reason`] within the HTLC's timeout, the HTLC will + /// be automatically failed. /// /// # Note /// LDK will not stop an inbound payment from being paid multiple times, so multiple @@ -370,6 +380,8 @@ pub enum Event { /// This event used to be called `PaymentReceived` in LDK versions 0.0.112 and earlier. /// /// [`ChannelManager::claim_funds`]: crate::ln::channelmanager::ChannelManager::claim_funds + /// [`ChannelManager::claim_funds_with_known_custom_tlvs`]: crate::ln::channelmanager::ChannelManager::claim_funds_with_known_custom_tlvs + /// [`FailureCode::InvalidOnionPayload`]: crate::ln::channelmanager::FailureCode::InvalidOnionPayload /// [`ChannelManager::fail_htlc_backwards`]: crate::ln::channelmanager::ChannelManager::fail_htlc_backwards /// [`ChannelManager::fail_htlc_backwards_with_reason`]: crate::ln::channelmanager::ChannelManager::fail_htlc_backwards_with_reason PaymentClaimable { diff --git a/lightning/src/ln/channelmanager.rs b/lightning/src/ln/channelmanager.rs index 68e5451d8..ae9744cb7 100644 --- a/lightning/src/ln/channelmanager.rs +++ b/lightning/src/ln/channelmanager.rs @@ -4759,13 +4759,35 @@ where /// event matches your expectation. If you fail to do so and call this method, you may provide /// the sender "proof-of-payment" when they did not fulfill the full expected payment. /// + /// This function will fail the payment if it has custom TLVs with even type numbers, as we + /// will assume they are unknown. If you intend to accept even custom TLVs, you should use + /// [`claim_funds_with_known_custom_tlvs`]. + /// /// [`Event::PaymentClaimable`]: crate::events::Event::PaymentClaimable /// [`Event::PaymentClaimable::claim_deadline`]: crate::events::Event::PaymentClaimable::claim_deadline /// [`Event::PaymentClaimed`]: crate::events::Event::PaymentClaimed /// [`process_pending_events`]: EventsProvider::process_pending_events /// [`create_inbound_payment`]: Self::create_inbound_payment /// [`create_inbound_payment_for_hash`]: Self::create_inbound_payment_for_hash + /// [`claim_funds_with_known_custom_tlvs`]: Self::claim_funds_with_known_custom_tlvs pub fn claim_funds(&self, payment_preimage: PaymentPreimage) { + self.claim_payment_internal(payment_preimage, false); + } + + /// This is a variant of [`claim_funds`] that allows accepting a payment with custom TLVs with + /// even type numbers. + /// + /// # Note + /// + /// You MUST check you've understood all even TLVs before using this to + /// claim, otherwise you may unintentionally agree to some protocol you do not understand. + /// + /// [`claim_funds`]: Self::claim_funds + pub fn claim_funds_with_known_custom_tlvs(&self, payment_preimage: PaymentPreimage) { + self.claim_payment_internal(payment_preimage, true); + } + + fn claim_payment_internal(&self, payment_preimage: PaymentPreimage, custom_tlvs_known: bool) { let payment_hash = PaymentHash(Sha256::hash(&payment_preimage.0).into_inner()); let _persistence_guard = PersistenceNotifierGuard::notify_on_drop(self); @@ -4792,6 +4814,23 @@ where log_error!(self.logger, "Got a duplicate pending claimable event on payment hash {}! Please report this bug", log_bytes!(payment_hash.0)); } + + if let Some(RecipientOnionFields { ref custom_tlvs, .. }) = payment.onion_fields { + if !custom_tlvs_known && custom_tlvs.iter().any(|(typ, _)| typ % 2 == 0) { + log_info!(self.logger, "Rejecting payment with payment hash {} as we cannot accept payment with unknown even TLVs: {}", + log_bytes!(payment_hash.0), log_iter!(custom_tlvs.iter().map(|(typ, _)| typ).filter(|typ| *typ % 2 == 0))); + claimable_payments.pending_claiming_payments.remove(&payment_hash); + mem::drop(claimable_payments); + for htlc in payment.htlcs { + let reason = self.get_htlc_fail_reason_from_failure_code(FailureCode::InvalidOnionPayload(None), &htlc); + let source = HTLCSource::PreviousHopData(htlc.prev_hop); + let receiver = HTLCDestination::FailedPayment { payment_hash }; + self.fail_htlc_backwards_internal(&source, &payment_hash, &reason, receiver); + } + return; + } + } + payment.htlcs } else { return; } }; diff --git a/lightning/src/ln/functional_test_utils.rs b/lightning/src/ln/functional_test_utils.rs index 39396685f..d51eceac2 100644 --- a/lightning/src/ln/functional_test_utils.rs +++ b/lightning/src/ln/functional_test_utils.rs @@ -2250,7 +2250,10 @@ pub fn do_claim_payment_along_route_with_extra_penultimate_hop_fees<'a, 'b, 'c>( assert_eq!(path.last().unwrap().node.get_our_node_id(), expected_paths[0].last().unwrap().node.get_our_node_id()); } expected_paths[0].last().unwrap().node.claim_funds(our_payment_preimage); + pass_claimed_payment_along_route(origin_node, expected_paths, expected_extra_fees, skip_last, our_payment_preimage) +} +pub fn pass_claimed_payment_along_route<'a, 'b, 'c>(origin_node: &Node<'a, 'b, 'c>, expected_paths: &[&[&Node<'a, 'b, 'c>]], expected_extra_fees: &[u32], skip_last: bool, our_payment_preimage: PaymentPreimage) -> u64 { let claim_event = expected_paths[0].last().unwrap().node.get_and_clear_pending_events(); assert_eq!(claim_event.len(), 1); match claim_event[0] { diff --git a/lightning/src/ln/payment_tests.rs b/lightning/src/ln/payment_tests.rs index fc1b181e9..1725fd301 100644 --- a/lightning/src/ln/payment_tests.rs +++ b/lightning/src/ln/payment_tests.rs @@ -3386,12 +3386,20 @@ fn claim_from_closed_chan() { } #[test] -fn test_custom_tlvs() { - do_test_custom_tlvs(true); - do_test_custom_tlvs(false); +fn test_custom_tlvs_basic() { + do_test_custom_tlvs(false, false, false); + do_test_custom_tlvs(true, false, false); } -fn do_test_custom_tlvs(spontaneous: bool) { +#[test] +fn test_custom_tlvs_explicit_claim() { + // Test that when receiving even custom TLVs the user must explicitly accept in case they + // are unknown. + do_test_custom_tlvs(false, true, false); + do_test_custom_tlvs(false, true, true); +} + +fn do_test_custom_tlvs(spontaneous: bool, even_tlvs: bool, known_tlvs: bool) { 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; 2]); @@ -3403,7 +3411,7 @@ fn do_test_custom_tlvs(spontaneous: bool) { let (mut route, our_payment_hash, our_payment_preimage, our_payment_secret) = get_route_and_payment_hash!(&nodes[0], &nodes[1], amt_msat); let payment_id = PaymentId(our_payment_hash.0); let custom_tlvs = vec![ - (5482373483, vec![1, 2, 3, 4]), + (if even_tlvs { 5482373482 } else { 5482373483 }, vec![1, 2, 3, 4]), (5482373487, vec![0x42u8; 16]), ]; let onion_fields = RecipientOnionFields { @@ -3446,7 +3454,22 @@ fn do_test_custom_tlvs(spontaneous: bool) { _ => panic!("Unexpected event"), } - claim_payment(&nodes[0], &[&nodes[1]], our_payment_preimage); + match (known_tlvs, even_tlvs) { + (true, _) => { + nodes[1].node.claim_funds_with_known_custom_tlvs(our_payment_preimage); + let expected_total_fee_msat = pass_claimed_payment_along_route(&nodes[0], &[&[&nodes[1]]], &[0; 1], false, our_payment_preimage); + expect_payment_sent!(&nodes[0], our_payment_preimage, Some(expected_total_fee_msat)); + }, + (false, false) => { + claim_payment(&nodes[0], &[&nodes[1]], our_payment_preimage); + }, + (false, true) => { + nodes[1].node.claim_funds(our_payment_preimage); + let expected_destinations = vec![HTLCDestination::FailedPayment { payment_hash: our_payment_hash }]; + expect_pending_htlcs_forwardable_and_htlc_handling_failed!(nodes[1], expected_destinations); + pass_failed_payment_back(&nodes[0], &[&[&nodes[1]]], false, our_payment_hash, PaymentFailureReason::RecipientRejected); + } + } } #[test] -- 2.39.5