Enforce explicit claims on payments with even custom TLVs
authorAlec Chen <alecchendev@gmail.com>
Thu, 8 Jun 2023 17:08:25 +0000 (12:08 -0500)
committerAlec Chen <alecchendev@gmail.com>
Tue, 8 Aug 2023 21:16:45 +0000 (16:16 -0500)
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
lightning/src/ln/channelmanager.rs
lightning/src/ln/functional_test_utils.rs
lightning/src/ln/payment_tests.rs

index d08e563cbf6195e4566bae0691b783e947033156..f8d251c91c7f43616ddcf10d3d5e1653b8b0848a 100644 (file)
@@ -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 {
index 68e5451d8b243d9dc1fbe6fbf787539e8a053196..ae9744cb71c4c3ea66a748b9176418bfcef1d7f7 100644 (file)
@@ -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; }
                };
index 39396685f005abf2e9bf795ad7428724c02186dc..d51eceac21c6b9b40136111e47c84739d37cccfb 100644 (file)
@@ -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] {
index fc1b181e9e74d91ce4e3a4bf09360a4198432a1d..1725fd301fa4ae992d50ac352e8d580189da93e7 100644 (file)
@@ -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]