Skip to content

Avoid probing attacks with same hash lower amounts #395

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 2 commits into from
Nov 18, 2019
Merged

Avoid probing attacks with same hash lower amounts #395

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
6493efe
Check expected amount in claim_funds
Nov 14, 2019
5e047cc
Add test_check_htlc_underpaying
Nov 18, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion fuzz/fuzz_targets/chanmon_fail_consistency.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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));
}
}
},
Expand Down
13 changes: 7 additions & 6 deletions fuzz/fuzz_targets/full_stack_target.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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();
Expand Down Expand Up @@ -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
Expand All @@ -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);
}
},
Expand Down Expand Up @@ -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 {..} => {},
Expand Down
54 changes: 27 additions & 27 deletions src/ln/chanmon_update_fail_tests.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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]);
Expand Down Expand Up @@ -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);
Expand Down Expand Up @@ -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]
Expand Down Expand Up @@ -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]
Expand Down Expand Up @@ -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]
Expand All @@ -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]);
Expand Down Expand Up @@ -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) {
Expand All @@ -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);
Expand Down Expand Up @@ -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]
Expand All @@ -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());
Expand Down Expand Up @@ -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]
Expand All @@ -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());
Expand Down Expand Up @@ -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]
Expand Down Expand Up @@ -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]
Expand Down Expand Up @@ -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]
Expand All @@ -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();
Expand Down Expand Up @@ -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));
Expand Down Expand Up @@ -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]
Expand Down Expand Up @@ -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());

Expand All @@ -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
Expand Down Expand Up @@ -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);
}

Expand Down
25 changes: 19 additions & 6 deletions src/ln/channelmanager.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Grrrr, I think spec changed out from underneath us. In pub fn fail_htlc_backwards we only send back the recvd_value field. Can be fixed in a separate pr.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, I've spotted other onion issues, like PERM|16 and PERM|17 being deprecated, will bundle in one PR.

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 }
Expand Down
12 changes: 6 additions & 6 deletions src/ln/functional_test_utils.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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;
Expand Down Expand Up @@ -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) {
Expand Down
Loading