Merge pull request #395 from ariard/2019-11-expected-amt-claim-funds

Avoid probing attacks with same hash lower amounts

Author: TheBlueMatt

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

@@ -341,7 +341,7 @@ pub fn do_test(data: &[u8], logger: &Arc<Logger>) {
 	}, our_network_key, &[0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 15, 0], Arc::clone(&logger)));
 
 	let mut should_forward = false;
-	let mut payments_received: Vec<PaymentHash> = Vec::new();
+	let mut payments_received: Vec<(PaymentHash, u64)> = Vec::new();
 	let mut payments_sent = 0;
 	let mut pending_funding_generation: Vec<([u8; 32], u64, Script)> = Vec::new();
 	let mut pending_funding_signatures = HashMap::new();
@@ -426,7 +426,7 @@ pub fn do_test(data: &[u8], logger: &Arc<Logger>) {
 				}
 			},
 			8 => {
-				for payment in payments_received.drain(..) {
+				for (payment, amt) in payments_received.drain(..) {
 					// SHA256 is defined as XOR of all input bytes placed in the first byte, and 0s
 					// for the remaining bytes. Thus, if not all remaining bytes are 0s we cannot
 					// fulfill this HTLC, but if they are, we can just take the first byte and
@@ -436,12 +436,12 @@ 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);
+						channelmanager.claim_funds(payment_preimage, amt);
 					}
 				}
 			},
 			9 => {
-				for payment in payments_received.drain(..) {
+				for (payment, _) in payments_received.drain(..) {
 					channelmanager.fail_htlc_backwards(&payment);
 				}
 			},
@@ -516,8 +516,9 @@ pub fn do_test(data: &[u8], logger: &Arc<Logger>) {
 				Event::FundingBroadcastSafe { funding_txo, .. } => {
 					pending_funding_relay.push(pending_funding_signatures.remove(&funding_txo).unwrap());
 				},
-				Event::PaymentReceived { payment_hash, .. } => {
-					payments_received.push(payment_hash);
+				Event::PaymentReceived { payment_hash, amt } => {
+					//TODO: enhance by fetching random amounts from fuzz input?
+					payments_received.push((payment_hash, amt));
 				},
 				Event::PaymentSent {..} => {},
 				Event::PaymentFailed {..} => {},

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

@@ -1606,21 +1606,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 > expected_amount * 2 {
+					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) {