From 74588b25192ecb315fd90c6d4f498cfe8d3f5628 Mon Sep 17 00:00:00 2001 From: Matt Corallo Date: Tue, 22 Jan 2019 15:49:29 -0500 Subject: [PATCH] Send back the actual received amount, not expected on HTLC fails This resolves an incorrect implementation of the spec and fixes a major privacy leak. Fixes GH #289. --- fuzz/fuzz_targets/chanmon_fail_consistency.rs | 2 +- fuzz/fuzz_targets/full_stack_target.rs | 4 +-- src/ln/chanmon_update_fail_tests.rs | 4 +-- src/ln/channelmanager.rs | 33 +++++++++++-------- src/ln/functional_test_utils.rs | 2 +- src/ln/functional_tests.rs | 32 +++++++++--------- 6 files changed, 42 insertions(+), 35 deletions(-) diff --git a/fuzz/fuzz_targets/chanmon_fail_consistency.rs b/fuzz/fuzz_targets/chanmon_fail_consistency.rs index 4db158f28..f457a3091 100644 --- a/fuzz/fuzz_targets/chanmon_fail_consistency.rs +++ b/fuzz/fuzz_targets/chanmon_fail_consistency.rs @@ -439,7 +439,7 @@ pub fn do_test(data: &[u8]) { match event { events::Event::PaymentReceived { payment_hash, .. } => { if $fail { - assert!(nodes[$node].fail_htlc_backwards(&payment_hash, 0)); + assert!(nodes[$node].fail_htlc_backwards(&payment_hash)); } else { assert!(nodes[$node].claim_funds(PaymentPreimage(payment_hash.0))); } diff --git a/fuzz/fuzz_targets/full_stack_target.rs b/fuzz/fuzz_targets/full_stack_target.rs index a4697fd93..f0d219377 100644 --- a/fuzz/fuzz_targets/full_stack_target.rs +++ b/fuzz/fuzz_targets/full_stack_target.rs @@ -451,7 +451,7 @@ pub fn do_test(data: &[u8], logger: &Arc) { // fulfill this HTLC, but if they are, we can just take the first byte and // place that anywhere in our preimage. if &payment.0[1..] != &[0; 31] { - channelmanager.fail_htlc_backwards(&payment, 0); + channelmanager.fail_htlc_backwards(&payment); } else { let mut payment_preimage = PaymentPreimage([0; 32]); payment_preimage.0[0] = payment.0[0]; @@ -461,7 +461,7 @@ pub fn do_test(data: &[u8], logger: &Arc) { }, 9 => { for payment in payments_received.drain(..) { - channelmanager.fail_htlc_backwards(&payment, 0); + channelmanager.fail_htlc_backwards(&payment); } }, 10 => { diff --git a/src/ln/chanmon_update_fail_tests.rs b/src/ln/chanmon_update_fail_tests.rs index fc481f8aa..69e056217 100644 --- a/src/ln/chanmon_update_fail_tests.rs +++ b/src/ln/chanmon_update_fail_tests.rs @@ -674,7 +674,7 @@ fn do_test_monitor_update_fail_raa(test_ignore_second_cs: bool) { let (_, payment_hash_1) = route_payment(&nodes[0], &[&nodes[1], &nodes[2]], 1000000); // Fail the payment backwards, failing the monitor update on nodes[1]'s receipt of the RAA - assert!(nodes[2].node.fail_htlc_backwards(&payment_hash_1, 0)); + assert!(nodes[2].node.fail_htlc_backwards(&payment_hash_1)); expect_pending_htlcs_forwardable!(nodes[2]); check_added_monitors!(nodes[2], 1); @@ -1451,7 +1451,7 @@ fn test_monitor_update_on_pending_forwards() { send_payment(&nodes[0], &[&nodes[1], &nodes[2]], 5000000); let (_, payment_hash_1) = route_payment(&nodes[0], &[&nodes[1], &nodes[2]], 1000000); - assert!(nodes[2].node.fail_htlc_backwards(&payment_hash_1, 1000000)); + assert!(nodes[2].node.fail_htlc_backwards(&payment_hash_1)); expect_pending_htlcs_forwardable!(nodes[2]); check_added_monitors!(nodes[2], 1); diff --git a/src/ln/channelmanager.rs b/src/ln/channelmanager.rs index ae5842463..6cee5d694 100644 --- a/src/ln/channelmanager.rs +++ b/src/ln/channelmanager.rs @@ -252,10 +252,12 @@ pub(super) struct ChannelHolder { /// guarantees are made about the existence of a channel with the short id here, nor the short /// ids in the PendingForwardHTLCInfo! pub(super) forward_htlcs: HashMap>, + /// payment_hash -> Vec<(amount_received, htlc_source)> for tracking things that were 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! - pub(super) claimable_htlcs: HashMap>, + pub(super) 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, @@ -265,7 +267,7 @@ pub(super) struct MutChannelHolder<'a> { pub(super) short_to_id: &'a mut HashMap, pub(super) next_forward: &'a mut Instant, pub(super) forward_htlcs: &'a mut HashMap>, - pub(super) claimable_htlcs: &'a mut HashMap>, + pub(super) claimable_htlcs: &'a mut HashMap>, pub(super) pending_msg_events: &'a mut Vec, } impl ChannelHolder { @@ -1308,8 +1310,8 @@ impl ChannelManager { incoming_packet_shared_secret: forward_info.incoming_shared_secret, }; match channel_state.claimable_htlcs.entry(forward_info.payment_hash) { - hash_map::Entry::Occupied(mut entry) => entry.get_mut().push(prev_hop_data), - hash_map::Entry::Vacant(entry) => { entry.insert(vec![prev_hop_data]); }, + hash_map::Entry::Occupied(mut entry) => entry.get_mut().push((forward_info.amt_to_forward, prev_hop_data)), + hash_map::Entry::Vacant(entry) => { entry.insert(vec![(forward_info.amt_to_forward, prev_hop_data)]); }, }; new_events.push(events::Event::PaymentReceived { payment_hash: forward_info.payment_hash, @@ -1354,20 +1356,21 @@ impl ChannelManager { } /// Indicates that the preimage for payment_hash is unknown or the received amount is incorrect - /// after a PaymentReceived event. - /// expected_value is the value you expected the payment to be for (not the amount it actually - /// was for from the PaymentReceived event). - pub fn fail_htlc_backwards(&self, payment_hash: &PaymentHash, expected_value: u64) -> bool { + /// after a PaymentReceived event, failing the HTLC back to its origin and freeing resources + /// along the path (including in our own channel on which we received it). + /// Returns false if no payment was found to fail backwards, true if the process of failing the + /// HTLC backwards has been started. + pub fn fail_htlc_backwards(&self, payment_hash: &PaymentHash) -> bool { let _ = self.total_consistency_lock.read().unwrap(); let mut channel_state = Some(self.channel_state.lock().unwrap()); let removed_source = channel_state.as_mut().unwrap().claimable_htlcs.remove(payment_hash); if let Some(mut sources) = removed_source { - for htlc_with_hash in sources.drain(..) { + for (recvd_value, htlc_with_hash) in sources.drain(..) { if channel_state.is_none() { channel_state = Some(self.channel_state.lock().unwrap()); } self.fail_htlc_backwards_internal(channel_state.take().unwrap(), HTLCSource::PreviousHopData(htlc_with_hash), payment_hash, - HTLCFailReason::Reason { failure_code: 0x4000 | 15, data: byte_utils::be64_to_array(expected_value).to_vec() }); + HTLCFailReason::Reason { failure_code: 0x4000 | 15, data: byte_utils::be64_to_array(recvd_value).to_vec() }); } true } else { false } @@ -1485,7 +1488,10 @@ impl ChannelManager { let mut channel_state = Some(self.channel_state.lock().unwrap()); let removed_source = channel_state.as_mut().unwrap().claimable_htlcs.remove(&payment_hash); if let Some(mut sources) = removed_source { - for htlc_with_hash in sources.drain(..) { + // TODO: We should require the user specify the expected amount so that we can claim + // only payments for the correct amount, and reject payments for incorrect amounts + // (which are probably middle nodes probing to break our privacy). + for (_, htlc_with_hash) in sources.drain(..) { if channel_state.is_none() { channel_state = Some(self.channel_state.lock().unwrap()); } self.claim_funds_internal(channel_state.take().unwrap(), HTLCSource::PreviousHopData(htlc_with_hash), payment_preimage); } @@ -2910,7 +2916,8 @@ impl Writeable for ChannelManager { for (payment_hash, previous_hops) in channel_state.claimable_htlcs.iter() { payment_hash.write(writer)?; (previous_hops.len() as u64).write(writer)?; - for previous_hop in previous_hops { + for &(recvd_amt, ref previous_hop) in previous_hops.iter() { + recvd_amt.write(writer)?; previous_hop.write(writer)?; } } @@ -3047,7 +3054,7 @@ impl<'a, R : ::std::io::Read> ReadableArgs> for (S let previous_hops_len: u64 = Readable::read(reader)?; let mut previous_hops = Vec::with_capacity(cmp::min(previous_hops_len as usize, 2)); for _ in 0..previous_hops_len { - previous_hops.push(Readable::read(reader)?); + previous_hops.push((Readable::read(reader)?, Readable::read(reader)?)); } claimable_htlcs.insert(payment_hash, previous_hops); } diff --git a/src/ln/functional_test_utils.rs b/src/ln/functional_test_utils.rs index 4ce090a28..0ca9f29a8 100644 --- a/src/ln/functional_test_utils.rs +++ b/src/ln/functional_test_utils.rs @@ -713,7 +713,7 @@ pub fn send_payment(origin: &Node, expected_route: &[&Node], recv_value: u64) { } pub fn fail_payment_along_route(origin_node: &Node, expected_route: &[&Node], skip_last: bool, our_payment_hash: PaymentHash) { - assert!(expected_route.last().unwrap().node.fail_htlc_backwards(&our_payment_hash, 0)); + assert!(expected_route.last().unwrap().node.fail_htlc_backwards(&our_payment_hash)); expect_pending_htlcs_forwardable!(expected_route.last().unwrap()); check_added_monitors!(expected_route.last().unwrap(), 1); diff --git a/src/ln/functional_tests.rs b/src/ln/functional_tests.rs index f9a8ac65a..fffdb7fb8 100644 --- a/src/ln/functional_tests.rs +++ b/src/ln/functional_tests.rs @@ -1912,7 +1912,7 @@ fn test_htlc_on_chain_timeout() { // Broadcast legit commitment tx from C on B's chain let commitment_tx = nodes[2].node.channel_state.lock().unwrap().by_id.get(&chan_2.2).unwrap().last_local_commitment_txn.clone(); check_spends!(commitment_tx[0], chan_2.3.clone()); - nodes[2].node.fail_htlc_backwards(&payment_hash, 0); + nodes[2].node.fail_htlc_backwards(&payment_hash); check_added_monitors!(nodes[2], 0); expect_pending_htlcs_forwardable!(nodes[2]); check_added_monitors!(nodes[2], 1); @@ -2093,7 +2093,7 @@ fn do_test_commitment_revoked_fail_backward_exhaustive(deliver_bs_raa: bool, use let (_, second_payment_hash) = route_payment(&nodes[0], &[&nodes[1], &nodes[2]], value); let (_, third_payment_hash) = route_payment(&nodes[0], &[&nodes[1], &nodes[2]], value); - assert!(nodes[2].node.fail_htlc_backwards(&first_payment_hash, 0)); + assert!(nodes[2].node.fail_htlc_backwards(&first_payment_hash)); expect_pending_htlcs_forwardable!(nodes[2]); check_added_monitors!(nodes[2], 1); let updates = get_htlc_update_msgs!(nodes[2], nodes[1].node.get_our_node_id()); @@ -2106,7 +2106,7 @@ fn do_test_commitment_revoked_fail_backward_exhaustive(deliver_bs_raa: bool, use let bs_raa = commitment_signed_dance!(nodes[1], nodes[2], updates.commitment_signed, false, true, false, true); // Drop the last RAA from 3 -> 2 - assert!(nodes[2].node.fail_htlc_backwards(&second_payment_hash, 0)); + assert!(nodes[2].node.fail_htlc_backwards(&second_payment_hash)); expect_pending_htlcs_forwardable!(nodes[2]); check_added_monitors!(nodes[2], 1); let updates = get_htlc_update_msgs!(nodes[2], nodes[1].node.get_our_node_id()); @@ -2123,7 +2123,7 @@ fn do_test_commitment_revoked_fail_backward_exhaustive(deliver_bs_raa: bool, use nodes[2].node.handle_revoke_and_ack(&nodes[1].node.get_our_node_id(), &as_raa).unwrap(); check_added_monitors!(nodes[2], 1); - assert!(nodes[2].node.fail_htlc_backwards(&third_payment_hash, 0)); + assert!(nodes[2].node.fail_htlc_backwards(&third_payment_hash)); expect_pending_htlcs_forwardable!(nodes[2]); check_added_monitors!(nodes[2], 1); let updates = get_htlc_update_msgs!(nodes[2], nodes[1].node.get_our_node_id()); @@ -3769,10 +3769,10 @@ fn do_test_fail_backwards_unrevoked_remote_announce(deliver_last_raa: bool, anno // Now fail back three of the over-dust-limit and three of the under-dust-limit payments in one go. // Fail 0th below-dust, 4th above-dust, 8th above-dust, 10th below-dust HTLCs - assert!(nodes[4].node.fail_htlc_backwards(&payment_hash_1, ds_dust_limit*1000)); - assert!(nodes[4].node.fail_htlc_backwards(&payment_hash_3, 1000000)); - assert!(nodes[4].node.fail_htlc_backwards(&payment_hash_5, 1000000)); - assert!(nodes[4].node.fail_htlc_backwards(&payment_hash_6, ds_dust_limit*1000)); + assert!(nodes[4].node.fail_htlc_backwards(&payment_hash_1)); + assert!(nodes[4].node.fail_htlc_backwards(&payment_hash_3)); + assert!(nodes[4].node.fail_htlc_backwards(&payment_hash_5)); + assert!(nodes[4].node.fail_htlc_backwards(&payment_hash_6)); check_added_monitors!(nodes[4], 0); expect_pending_htlcs_forwardable!(nodes[4]); check_added_monitors!(nodes[4], 1); @@ -3785,8 +3785,8 @@ fn do_test_fail_backwards_unrevoked_remote_announce(deliver_last_raa: bool, anno commitment_signed_dance!(nodes[3], nodes[4], four_removes.commitment_signed, false); // Fail 3rd below-dust and 7th above-dust HTLCs - assert!(nodes[5].node.fail_htlc_backwards(&payment_hash_2, ds_dust_limit*1000)); - assert!(nodes[5].node.fail_htlc_backwards(&payment_hash_4, 1000000)); + assert!(nodes[5].node.fail_htlc_backwards(&payment_hash_2)); + assert!(nodes[5].node.fail_htlc_backwards(&payment_hash_4)); check_added_monitors!(nodes[5], 0); expect_pending_htlcs_forwardable!(nodes[5]); check_added_monitors!(nodes[5], 1); @@ -4079,7 +4079,7 @@ fn do_htlc_claim_previous_remote_commitment_only(use_dust: bool, check_revoke_no // actually revoked. let htlc_value = if use_dust { 50000 } else { 3000000 }; let (_, our_payment_hash) = route_payment(&nodes[0], &[&nodes[1]], htlc_value); - assert!(nodes[1].node.fail_htlc_backwards(&our_payment_hash, htlc_value)); + assert!(nodes[1].node.fail_htlc_backwards(&our_payment_hash)); expect_pending_htlcs_forwardable!(nodes[1]); check_added_monitors!(nodes[1], 1); @@ -4405,7 +4405,7 @@ fn test_onion_failure() { let onion_keys = onion_utils::construct_onion_keys(&Secp256k1::new(), &route, &session_priv).unwrap(); msg.reason = onion_utils::build_first_hop_failure_packet(&onion_keys[1].shared_secret[..], NODE|2, &[0;0]); }, ||{ - nodes[2].node.fail_htlc_backwards(&payment_hash, 0); + nodes[2].node.fail_htlc_backwards(&payment_hash); }, true, Some(NODE|2), Some(msgs::HTLCFailChannelUpdate::NodeFailure{node_id: route.hops[1].pubkey, is_permanent: false})); // intermediate node failure @@ -4423,7 +4423,7 @@ fn test_onion_failure() { let onion_keys = onion_utils::construct_onion_keys(&Secp256k1::new(), &route, &session_priv).unwrap(); msg.reason = onion_utils::build_first_hop_failure_packet(&onion_keys[1].shared_secret[..], PERM|NODE|2, &[0;0]); }, ||{ - nodes[2].node.fail_htlc_backwards(&payment_hash, 0); + nodes[2].node.fail_htlc_backwards(&payment_hash); }, false, Some(PERM|NODE|2), Some(msgs::HTLCFailChannelUpdate::NodeFailure{node_id: route.hops[1].pubkey, is_permanent: true})); // intermediate node failure @@ -4434,7 +4434,7 @@ fn test_onion_failure() { let onion_keys = onion_utils::construct_onion_keys(&Secp256k1::new(), &route, &session_priv).unwrap(); msg.reason = onion_utils::build_first_hop_failure_packet(&onion_keys[0].shared_secret[..], PERM|NODE|3, &[0;0]); }, ||{ - nodes[2].node.fail_htlc_backwards(&payment_hash, 0); + nodes[2].node.fail_htlc_backwards(&payment_hash); }, true, Some(PERM|NODE|3), Some(msgs::HTLCFailChannelUpdate::NodeFailure{node_id: route.hops[0].pubkey, is_permanent: true})); // final node failure @@ -4443,7 +4443,7 @@ fn test_onion_failure() { let onion_keys = onion_utils::construct_onion_keys(&Secp256k1::new(), &route, &session_priv).unwrap(); msg.reason = onion_utils::build_first_hop_failure_packet(&onion_keys[1].shared_secret[..], PERM|NODE|3, &[0;0]); }, ||{ - nodes[2].node.fail_htlc_backwards(&payment_hash, 0); + nodes[2].node.fail_htlc_backwards(&payment_hash); }, false, Some(PERM|NODE|3), Some(msgs::HTLCFailChannelUpdate::NodeFailure{node_id: route.hops[1].pubkey, is_permanent: true})); run_onion_failure_test("invalid_onion_version", 0, &nodes, &route, &payment_hash, |msg| { msg.onion_routing_packet.version = 1; }, ||{}, true, @@ -4510,7 +4510,7 @@ fn test_onion_failure() { }, ||{}, true, Some(UPDATE|14), Some(msgs::HTLCFailChannelUpdate::ChannelUpdateMessage{msg: ChannelUpdate::dummy()})); run_onion_failure_test("unknown_payment_hash", 2, &nodes, &route, &payment_hash, |_| {}, || { - nodes[2].node.fail_htlc_backwards(&payment_hash, 0); + nodes[2].node.fail_htlc_backwards(&payment_hash); }, false, Some(PERM|15), None); run_onion_failure_test("final_expiry_too_soon", 1, &nodes, &route, &payment_hash, |msg| { -- 2.39.5