Check expected amount in claim_funds

Require to specify expected amount so that we can claim only
payment for thhe correct amount, and reject payments for incorrect
amounts (which are probably middle nodes probing to break our privacy).

Send back incorrect_or_unknown_payments_details (PERM|15) to avoid
the probe node learning that final node is waiting a payment with
the routed hash.

Author: Antoine Riard

fuzz/fuzz_targets/chanmon_fail_consistency.rs

@@ -609,7 +609,7 @@ pub fn do_test(data: &[u8]) {
 								if $fail {
 									assert!(nodes[$node].fail_htlc_backwards(&payment_hash));
 								} else {
-									assert!(nodes[$node].claim_funds(PaymentPreimage(payment_hash.0)));
+									assert!(nodes[$node].claim_funds(PaymentPreimage(payment_hash.0), 5_000_000));
 								}
 							}
 						},

fuzz/fuzz_targets/full_stack_target.rs

@@ -346,6 +346,7 @@ pub fn do_test(data: &[u8], logger: &Arc<Logger>) {
 	let mut pending_funding_generation: Vec<([u8; 32], u64, Script)> = Vec::new();
 	let mut pending_funding_signatures = HashMap::new();
 	let mut pending_funding_relay = Vec::new();
+	let mut values = HashMap::new();
 
 	loop {
 		match get_slice!(1)[0] {
@@ -399,6 +400,7 @@ pub fn do_test(data: &[u8], logger: &Arc<Logger>) {
 				sha.input(&payment_hash.0[..]);
 				payment_hash.0 = Sha256::from_engine(sha).into_inner();
 				payments_sent += 1;
+				values.insert(payment_hash, value);
 				match channelmanager.send_payment(route, payment_hash) {
 					Ok(_) => {},
 					Err(_) => return,
@@ -436,7 +438,9 @@ pub fn do_test(data: &[u8], logger: &Arc<Logger>) {
 					} else {
 						let mut payment_preimage = PaymentPreimage([0; 32]);
 						payment_preimage.0[0] = payment.0[0];
-						channelmanager.claim_funds(payment_preimage);
+						if let Some(value) = values.get(&payment) {
+							channelmanager.claim_funds(payment_preimage, *value);
+						}
 					}
 				}
 			},

src/ln/chanmon_update_fail_tests.rs

@@ -91,7 +91,7 @@ fn do_test_simple_monitor_temporary_update_fail(disconnect: bool) {
 		_ => panic!("Unexpected event"),
 	}
 
-	claim_payment(&nodes[0], &[&nodes[1]], payment_preimage_1);
+	claim_payment(&nodes[0], &[&nodes[1]], payment_preimage_1, 1_000_000);
 
 	// Now set it to failed again...
 	let (_, payment_hash_2) = get_payment_preimage_hash!(nodes[0]);
@@ -166,7 +166,7 @@ fn do_test_monitor_temporary_update_fail(disconnect_count: usize) {
 
 	// Claim the previous payment, which will result in a update_fulfill_htlc/CS from nodes[1]
 	// but nodes[0] won't respond since it is frozen.
-	assert!(nodes[1].node.claim_funds(payment_preimage_1));
+	assert!(nodes[1].node.claim_funds(payment_preimage_1, 1_000_000));
 	check_added_monitors!(nodes[1], 1);
 	let events_2 = nodes[1].node.get_and_clear_pending_msg_events();
 	assert_eq!(events_2.len(), 1);
@@ -440,7 +440,7 @@ fn do_test_monitor_temporary_update_fail(disconnect_count: usize) {
 		_ => panic!("Unexpected event"),
 	}
 
-	claim_payment(&nodes[0], &[&nodes[1]], payment_preimage_2);
+	claim_payment(&nodes[0], &[&nodes[1]], payment_preimage_2, 1_000_000);
 }
 
 #[test]
@@ -544,7 +544,7 @@ fn test_monitor_update_fail_cs() {
 		_ => panic!("Unexpected event"),
 	};
 
-	claim_payment(&nodes[0], &[&nodes[1]], payment_preimage);
+	claim_payment(&nodes[0], &[&nodes[1]], payment_preimage, 1_000_000);
 }
 
 #[test]
@@ -587,7 +587,7 @@ fn test_monitor_update_fail_no_rebroadcast() {
 		_ => panic!("Unexpected event"),
 	}
 
-	claim_payment(&nodes[0], &[&nodes[1]], payment_preimage_1);
+	claim_payment(&nodes[0], &[&nodes[1]], payment_preimage_1, 1_000_000);
 }
 
 #[test]
@@ -597,7 +597,7 @@ fn test_monitor_update_raa_while_paused() {
 	let mut nodes = create_network(2, &[None, None]);
 	create_announced_chan_between_nodes(&nodes, 0, 1, LocalFeatures::new(), LocalFeatures::new());
 
-	send_payment(&nodes[0], &[&nodes[1]], 5000000);
+	send_payment(&nodes[0], &[&nodes[1]], 5000000, 5_000_000);
 
 	let route = nodes[0].router.get_route(&nodes[1].node.get_our_node_id(), None, &Vec::new(), 1000000, TEST_FINAL_CLTV).unwrap();
 	let (payment_preimage_1, our_payment_hash_1) = get_payment_preimage_hash!(nodes[0]);
@@ -655,8 +655,8 @@ fn test_monitor_update_raa_while_paused() {
 	expect_pending_htlcs_forwardable!(nodes[1]);
 	expect_payment_received!(nodes[1], our_payment_hash_1, 1000000);
 
-	claim_payment(&nodes[0], &[&nodes[1]], payment_preimage_1);
-	claim_payment(&nodes[1], &[&nodes[0]], payment_preimage_2);
+	claim_payment(&nodes[0], &[&nodes[1]], payment_preimage_1, 1_000_000);
+	claim_payment(&nodes[1], &[&nodes[0]], payment_preimage_2, 1_000_000);
 }
 
 fn do_test_monitor_update_fail_raa(test_ignore_second_cs: bool) {
@@ -666,7 +666,7 @@ fn do_test_monitor_update_fail_raa(test_ignore_second_cs: bool) {
 	let chan_2 = create_announced_chan_between_nodes(&nodes, 1, 2, LocalFeatures::new(), LocalFeatures::new());
 
 	// Rebalance a bit so that we can send backwards from 2 to 1.
-	send_payment(&nodes[0], &[&nodes[1], &nodes[2]], 5000000);
+	send_payment(&nodes[0], &[&nodes[1], &nodes[2]], 5000000, 5_000_000);
 
 	// Route a first payment that we'll fail backwards
 	let (_, payment_hash_1) = route_payment(&nodes[0], &[&nodes[1], &nodes[2]], 1000000);
@@ -897,10 +897,10 @@ fn do_test_monitor_update_fail_raa(test_ignore_second_cs: bool) {
 			Event::PaymentReceived { payment_hash, .. } => assert_eq!(payment_hash, payment_hash_4.unwrap()),
 			_ => panic!("Unexpected event"),
 		};
-		claim_payment(&nodes[2], &[&nodes[1], &nodes[0]], payment_preimage_4.unwrap());
+		claim_payment(&nodes[2], &[&nodes[1], &nodes[0]], payment_preimage_4.unwrap(), 1_000_000);
 	}
 
-	claim_payment(&nodes[0], &[&nodes[1], &nodes[2]], payment_preimage_2);
+	claim_payment(&nodes[0], &[&nodes[1], &nodes[2]], payment_preimage_2, 1_000_000);
 }
 
 #[test]
@@ -923,7 +923,7 @@ fn test_monitor_update_fail_reestablish() {
 	nodes[1].node.peer_disconnected(&nodes[0].node.get_our_node_id(), false);
 	nodes[0].node.peer_disconnected(&nodes[1].node.get_our_node_id(), false);
 
-	assert!(nodes[2].node.claim_funds(our_payment_preimage));
+	assert!(nodes[2].node.claim_funds(our_payment_preimage, 1_000_000));
 	check_added_monitors!(nodes[2], 1);
 	let mut updates = get_htlc_update_msgs!(nodes[2], nodes[1].node.get_our_node_id());
 	assert!(updates.update_add_htlcs.is_empty());
@@ -1092,9 +1092,9 @@ fn raa_no_response_awaiting_raa_state() {
 	expect_pending_htlcs_forwardable!(nodes[1]);
 	expect_payment_received!(nodes[1], payment_hash_3, 1000000);
 
-	claim_payment(&nodes[0], &[&nodes[1]], payment_preimage_1);
-	claim_payment(&nodes[0], &[&nodes[1]], payment_preimage_2);
-	claim_payment(&nodes[0], &[&nodes[1]], payment_preimage_3);
+	claim_payment(&nodes[0], &[&nodes[1]], payment_preimage_1, 1_000_000);
+	claim_payment(&nodes[0], &[&nodes[1]], payment_preimage_2, 1_000_000);
+	claim_payment(&nodes[0], &[&nodes[1]], payment_preimage_3, 1_000_000);
 }
 
 #[test]
@@ -1114,7 +1114,7 @@ fn claim_while_disconnected_monitor_update_fail() {
 	nodes[0].node.peer_disconnected(&nodes[1].node.get_our_node_id(), false);
 	nodes[1].node.peer_disconnected(&nodes[0].node.get_our_node_id(), false);
 
-	assert!(nodes[1].node.claim_funds(payment_preimage_1));
+	assert!(nodes[1].node.claim_funds(payment_preimage_1, 1_000_000));
 	check_added_monitors!(nodes[1], 1);
 
 	nodes[0].node.peer_connected(&nodes[1].node.get_our_node_id());
@@ -1211,7 +1211,7 @@ fn claim_while_disconnected_monitor_update_fail() {
 		_ => panic!("Unexpected event"),
 	}
 
-	claim_payment(&nodes[0], &[&nodes[1]], payment_preimage_2);
+	claim_payment(&nodes[0], &[&nodes[1]], payment_preimage_2, 1_000_000);
 }
 
 #[test]
@@ -1271,7 +1271,7 @@ fn monitor_failed_no_reestablish_response() {
 	expect_pending_htlcs_forwardable!(nodes[1]);
 	expect_payment_received!(nodes[1], payment_hash_1, 1000000);
 
-	claim_payment(&nodes[0], &[&nodes[1]], payment_preimage_1);
+	claim_payment(&nodes[0], &[&nodes[1]], payment_preimage_1, 1_000_000);
 }
 
 #[test]
@@ -1360,8 +1360,8 @@ fn first_message_on_recv_ordering() {
 	expect_pending_htlcs_forwardable!(nodes[1]);
 	expect_payment_received!(nodes[1], payment_hash_2, 1000000);
 
-	claim_payment(&nodes[0], &[&nodes[1]], payment_preimage_1);
-	claim_payment(&nodes[0], &[&nodes[1]], payment_preimage_2);
+	claim_payment(&nodes[0], &[&nodes[1]], payment_preimage_1, 1_000_000);
+	claim_payment(&nodes[0], &[&nodes[1]], payment_preimage_2, 1_000_000);
 }
 
 #[test]
@@ -1376,12 +1376,12 @@ fn test_monitor_update_fail_claim() {
 	create_announced_chan_between_nodes(&nodes, 1, 2, LocalFeatures::new(), LocalFeatures::new());
 
 	// Rebalance a bit so that we can send backwards from 3 to 2.
-	send_payment(&nodes[0], &[&nodes[1], &nodes[2]], 5000000);
+	send_payment(&nodes[0], &[&nodes[1], &nodes[2]], 5000000, 5_000_000);
 
 	let (payment_preimage_1, _) = route_payment(&nodes[0], &[&nodes[1]], 1000000);
 
 	*nodes[1].chan_monitor.update_ret.lock().unwrap() = Err(ChannelMonitorUpdateErr::TemporaryFailure);
-	assert!(nodes[1].node.claim_funds(payment_preimage_1));
+	assert!(nodes[1].node.claim_funds(payment_preimage_1, 1_000_000));
 	check_added_monitors!(nodes[1], 1);
 
 	let route = nodes[2].router.get_route(&nodes[0].node.get_our_node_id(), None, &Vec::new(), 1000000, TEST_FINAL_CLTV).unwrap();
@@ -1446,7 +1446,7 @@ fn test_monitor_update_on_pending_forwards() {
 	create_announced_chan_between_nodes(&nodes, 1, 2, LocalFeatures::new(), LocalFeatures::new());
 
 	// Rebalance a bit so that we can send backwards from 3 to 1.
-	send_payment(&nodes[0], &[&nodes[1], &nodes[2]], 5000000);
+	send_payment(&nodes[0], &[&nodes[1], &nodes[2]], 5000000, 5_000_000);
 
 	let (_, payment_hash_1) = route_payment(&nodes[0], &[&nodes[1], &nodes[2]], 1000000);
 	assert!(nodes[2].node.fail_htlc_backwards(&payment_hash_1));
@@ -1496,7 +1496,7 @@ fn test_monitor_update_on_pending_forwards() {
 	nodes[0].node.process_pending_htlc_forwards();
 	expect_payment_received!(nodes[0], payment_hash_2, 1000000);
 
-	claim_payment(&nodes[2], &[&nodes[1], &nodes[0]], payment_preimage_2);
+	claim_payment(&nodes[2], &[&nodes[1], &nodes[0]], payment_preimage_2, 1_000_000);
 }
 
 #[test]
@@ -1524,7 +1524,7 @@ fn monitor_update_claim_fail_no_response() {
 	let as_raa = commitment_signed_dance!(nodes[1], nodes[0], payment_event.commitment_msg, false, true, false, true);
 
 	*nodes[1].chan_monitor.update_ret.lock().unwrap() = Err(ChannelMonitorUpdateErr::TemporaryFailure);
-	assert!(nodes[1].node.claim_funds(payment_preimage_1));
+	assert!(nodes[1].node.claim_funds(payment_preimage_1, 1_000_000));
 	check_added_monitors!(nodes[1], 1);
 	assert!(nodes[1].node.get_and_clear_pending_msg_events().is_empty());
 
@@ -1551,7 +1551,7 @@ fn monitor_update_claim_fail_no_response() {
 		_ => panic!("Unexpected event"),
 	}
 
-	claim_payment(&nodes[0], &[&nodes[1]], payment_preimage_2);
+	claim_payment(&nodes[0], &[&nodes[1]], payment_preimage_2, 1_000_000);
 }
 
 // Note that restore_between_fails with !fail_on_generate is useless
@@ -1671,7 +1671,7 @@ fn do_during_funding_monitor_fail(fail_on_generate: bool, restore_between_fails:
 		node.router.handle_channel_update(&bs_update).unwrap();
 	}
 
-	send_payment(&nodes[0], &[&nodes[1]], 8000000);
+	send_payment(&nodes[0], &[&nodes[1]], 8000000, 8_000_000);
 	close_channel(&nodes[0], &nodes[1], &channel_id, funding_tx, true);
 }
 

src/ln/channelmanager.rs

@@ -1574,21 +1574,34 @@ impl ChannelManager {
 	/// generating message events for the net layer to claim the payment, if possible. Thus, you
 	/// should probably kick the net layer to go send messages if this returns true!
 	///
+	/// You must specify the expected amounts for this HTLC, and we will only claim HTLCs
+	/// available within a few percent of the expected amount. This is critical for several
+	/// reasons : a) it avoids providing senders with `proof-of-payment` (in the form of the
+	/// payment_preimage without having provided the full value and b) it avoids certain
+	/// privacy-breaking recipient-probing attacks which may reveal payment activity to
+	/// motivated attackers.
+	///
 	/// May panic if called except in response to a PaymentReceived event.
-	pub fn claim_funds(&self, payment_preimage: PaymentPreimage) -> bool {
+	pub fn claim_funds(&self, payment_preimage: PaymentPreimage, expected_amount: u64) -> bool {
 		let payment_hash = PaymentHash(Sha256::hash(&payment_preimage.0).into_inner());
 
 		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 {
-			// 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(..) {
+			for (received_amount, 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);
+				if received_amount < expected_amount || received_amount * 2 > expected_amount {
+					let mut htlc_msat_data = byte_utils::be64_to_array(received_amount).to_vec();
+					let mut height_data = byte_utils::be32_to_array(self.latest_block_height.load(Ordering::Acquire) as u32).to_vec();
+					htlc_msat_data.append(&mut height_data);
+					self.fail_htlc_backwards_internal(channel_state.take().unwrap(),
+									 HTLCSource::PreviousHopData(htlc_with_hash), &payment_hash,
+									 HTLCFailReason::Reason { failure_code: 0x4000|15, data: htlc_msat_data });
+				} else {
+					self.claim_funds_internal(channel_state.take().unwrap(), HTLCSource::PreviousHopData(htlc_with_hash), payment_preimage);
+				}
 			}
 			true
 		} else { false }

src/ln/functional_test_utils.rs

@@ -636,8 +636,8 @@ pub fn send_along_route(origin_node: &Node, route: Route, expected_route: &[&Nod
 	(our_payment_preimage, our_payment_hash)
 }
 
-pub fn claim_payment_along_route(origin_node: &Node, expected_route: &[&Node], skip_last: bool, our_payment_preimage: PaymentPreimage) {
-	assert!(expected_route.last().unwrap().node.claim_funds(our_payment_preimage));
+pub fn claim_payment_along_route(origin_node: &Node, expected_route: &[&Node], skip_last: bool, our_payment_preimage: PaymentPreimage, expected_amount: u64) {
+	assert!(expected_route.last().unwrap().node.claim_funds(our_payment_preimage, expected_amount));
 	check_added_monitors!(expected_route.last().unwrap(), 1);
 
 	let mut next_msgs: Option<(msgs::UpdateFulfillHTLC, msgs::CommitmentSigned)> = None;
@@ -714,8 +714,8 @@ pub fn claim_payment_along_route(origin_node: &Node, expected_route: &[&Node], s
 	}
 }
 
-pub fn claim_payment(origin_node: &Node, expected_route: &[&Node], our_payment_preimage: PaymentPreimage) {
-	claim_payment_along_route(origin_node, expected_route, false, our_payment_preimage);
+pub fn claim_payment(origin_node: &Node, expected_route: &[&Node], our_payment_preimage: PaymentPreimage, expected_amount: u64) {
+	claim_payment_along_route(origin_node, expected_route, false, our_payment_preimage, expected_amount);
 }
 
 pub const TEST_FINAL_CLTV: u32 = 32;
@@ -746,9 +746,9 @@ pub fn route_over_limit(origin_node: &Node, expected_route: &[&Node], recv_value
 	};
 }
 
-pub fn send_payment(origin: &Node, expected_route: &[&Node], recv_value: u64) {
+pub fn send_payment(origin: &Node, expected_route: &[&Node], recv_value: u64, expected_value: u64) {
 	let our_payment_preimage = route_payment(&origin, expected_route, recv_value).0;
-	claim_payment(&origin, expected_route, our_payment_preimage);
+	claim_payment(&origin, expected_route, our_payment_preimage, expected_value);
 }
 
 pub fn fail_payment_along_route(origin_node: &Node, expected_route: &[&Node], skip_last: bool, our_payment_hash: PaymentHash) {