From 5e07c60f9e6ef6d680d1a55011077c548c411c8d Mon Sep 17 00:00:00 2001 From: Matt Corallo Date: Wed, 7 Sep 2022 21:08:22 +0000 Subject: [PATCH 1/2] Correctly handle BADONION onion errors Currently we entirely ignore the BADONION bit when deciding how to handle HTLC failures. This opens us up to an attack where a malicious node always fails HTLCs backwards via `update_fail_malformed_htlc` with an error code of `BADONION|NODE|PERM|X`. In this case, we may decide to interpret this as a permanent node failure for the node encrypting the onion, i.e. the counterparty of the node who sent the `update_fail_malformed_htlc` message and ultimately failed the HTLC. Thus, any node we route through could cause us to fully remove its counterparty from our network graph. Luckily we do not do any persistent tracking of removed nodes, and thus will re-add the removed node once it is re-announced or on restart, however we are likely to add such persistent tracking (at least in-memory) in the future. --- lightning/src/ln/onion_utils.rs | 25 +++++++++++++++++-------- 1 file changed, 17 insertions(+), 8 deletions(-) diff --git a/lightning/src/ln/onion_utils.rs b/lightning/src/ln/onion_utils.rs index 3795ad5ee77..34be2843812 100644 --- a/lightning/src/ln/onion_utils.rs +++ b/lightning/src/ln/onion_utils.rs @@ -425,6 +425,7 @@ pub(super) fn process_onion_failure(secp_ctx: & if fixed_time_eq(&Hmac::from_engine(hmac).into_inner(), &err_packet.hmac) { if let Some(error_code_slice) = err_packet.failuremsg.get(0..2) { + const BADONION: u16 = 0x8000; const PERM: u16 = 0x4000; const NODE: u16 = 0x2000; const UPDATE: u16 = 0x1000; @@ -445,12 +446,24 @@ pub(super) fn process_onion_failure(secp_ctx: & let mut network_update = None; let mut short_channel_id = None; - if error_code & NODE == NODE { + if error_code & BADONION == BADONION { + // If the error code has the BADONION bit set, always blame the channel + // from the node "originating" the error to its next hop. The + // "originator" is ultimately actually claiming that its counterparty + // is the one who is failing the HTLC. + // If the "originator" here isn't lying we should really mark the + // next-hop node as failed entirely, but we can't be confident in that, + // as it would allow any node to get us to completely ban one of its + // counterparties. Instead, we simply remove the channel in question. + network_update = Some(NetworkUpdate::ChannelFailure { + short_channel_id: failing_route_hop.short_channel_id, + is_permanent: true, + }); + } else if error_code & NODE == NODE { let is_permanent = error_code & PERM == PERM; network_update = Some(NetworkUpdate::NodeFailure { node_id: route_hop.pubkey, is_permanent }); short_channel_id = Some(route_hop.short_channel_id); - } - else if error_code & PERM == PERM { + } else if error_code & PERM == PERM { if !payment_failed { network_update = Some(NetworkUpdate::ChannelFailure { short_channel_id: failing_route_hop.short_channel_id, @@ -458,8 +471,7 @@ pub(super) fn process_onion_failure(secp_ctx: & }); short_channel_id = Some(failing_route_hop.short_channel_id); } - } - else if error_code & UPDATE == UPDATE { + } else if error_code & UPDATE == UPDATE { if let Some(update_len_slice) = err_packet.failuremsg.get(debug_field_size+2..debug_field_size+4) { let update_len = u16::from_be_bytes(update_len_slice.try_into().expect("len is 2")) as usize; if let Some(mut update_slice) = err_packet.failuremsg.get(debug_field_size + 4..debug_field_size + 4 + update_len) { @@ -545,9 +557,6 @@ pub(super) fn process_onion_failure(secp_ctx: & short_channel_id = Some(route_hop.short_channel_id); } - // TODO: Here (and a few other places) we assume that BADONION errors - // are always "sourced" from the node previous to the one which failed - // to decode the onion. res = Some((network_update, short_channel_id, !(error_code & PERM == PERM && is_from_final_node))); let (description, title) = errors::get_onion_error_description(error_code); From e6a3c23d10bd110100715db2b7c0410676a6bd9d Mon Sep 17 00:00:00 2001 From: Duncan Dean Date: Mon, 12 Sep 2022 15:24:14 +0200 Subject: [PATCH 2/2] Add test for malformed update error with NODE bit set Some tweaks by Matt Corallo --- lightning/src/ln/functional_tests.rs | 81 +++++++++++++++++++++++++++- 1 file changed, 80 insertions(+), 1 deletion(-) diff --git a/lightning/src/ln/functional_tests.rs b/lightning/src/ln/functional_tests.rs index c41d5402ff3..8f26c236061 100644 --- a/lightning/src/ln/functional_tests.rs +++ b/lightning/src/ln/functional_tests.rs @@ -24,7 +24,7 @@ use ln::channelmanager::{ChannelManager, ChannelManagerReadArgs, PaymentId, RAAC use ln::channel::{Channel, ChannelError}; use ln::{chan_utils, onion_utils}; use ln::chan_utils::{htlc_success_tx_weight, htlc_timeout_tx_weight, HTLCOutputInCommitment}; -use routing::gossip::NetworkGraph; +use routing::gossip::{NetworkGraph, NetworkUpdate}; use routing::router::{PaymentParameters, Route, RouteHop, RouteParameters, find_route, get_route}; use ln::features::{ChannelFeatures, InitFeatures, InvoiceFeatures, NodeFeatures}; use ln::msgs; @@ -7181,6 +7181,85 @@ fn test_update_fulfill_htlc_bolt2_after_malformed_htlc_message_must_forward_upda check_added_monitors!(nodes[1], 1); } +#[test] +fn test_channel_failed_after_message_with_badonion_node_perm_bits_set() { + let chanmon_cfgs = create_chanmon_cfgs(3); + let node_cfgs = create_node_cfgs(3, &chanmon_cfgs); + let node_chanmgrs = create_node_chanmgrs(3, &node_cfgs, &[None, None, None]); + let mut nodes = create_network(3, &node_cfgs, &node_chanmgrs); + create_announced_chan_between_nodes(&nodes, 0, 1, InitFeatures::known(), InitFeatures::known()); + let chan_2 = create_announced_chan_between_nodes(&nodes, 1, 2, InitFeatures::known(), InitFeatures::known()); + + let (route, our_payment_hash, _, our_payment_secret) = get_route_and_payment_hash!(nodes[0], nodes[2], 100_000); + + // First hop + let mut payment_event = { + nodes[0].node.send_payment(&route, our_payment_hash, &Some(our_payment_secret)).unwrap(); + check_added_monitors!(nodes[0], 1); + SendEvent::from_node(&nodes[0]) + }; + + nodes[1].node.handle_update_add_htlc(&nodes[0].node.get_our_node_id(), &payment_event.msgs[0]); + commitment_signed_dance!(nodes[1], nodes[0], payment_event.commitment_msg, false); + expect_pending_htlcs_forwardable!(nodes[1]); + check_added_monitors!(nodes[1], 1); + payment_event = SendEvent::from_node(&nodes[1]); + assert_eq!(payment_event.msgs.len(), 1); + + // Second Hop + payment_event.msgs[0].onion_routing_packet.version = 1; // Trigger an invalid_onion_version error + nodes[2].node.handle_update_add_htlc(&nodes[1].node.get_our_node_id(), &payment_event.msgs[0]); + check_added_monitors!(nodes[2], 0); + commitment_signed_dance!(nodes[2], nodes[1], payment_event.commitment_msg, false, true); + + let events_3 = nodes[2].node.get_and_clear_pending_msg_events(); + assert_eq!(events_3.len(), 1); + match events_3[0] { + MessageSendEvent::UpdateHTLCs { ref updates, .. } => { + let mut update_msg = updates.update_fail_malformed_htlcs[0].clone(); + // Set the NODE bit (BADONION and PERM already set in invalid_onion_version error) + update_msg.failure_code |= 0x2000; + + nodes[1].node.handle_update_fail_malformed_htlc(&nodes[2].node.get_our_node_id(), &update_msg); + commitment_signed_dance!(nodes[1], nodes[2], updates.commitment_signed, false, true); + }, + _ => panic!("Unexpected event"), + } + + expect_pending_htlcs_forwardable_and_htlc_handling_failed!(nodes[1], + vec![HTLCDestination::NextHopChannel { + node_id: Some(nodes[2].node.get_our_node_id()), channel_id: chan_2.2 }]); + let events_4 = nodes[1].node.get_and_clear_pending_msg_events(); + assert_eq!(events_4.len(), 1); + check_added_monitors!(nodes[1], 1); + + match events_4[0] { + MessageSendEvent::UpdateHTLCs { ref updates, .. } => { + nodes[0].node.handle_update_fail_htlc(&nodes[1].node.get_our_node_id(), &updates.update_fail_htlcs[0]); + commitment_signed_dance!(nodes[0], nodes[1], updates.commitment_signed, false, true); + }, + _ => panic!("Unexpected event"), + } + + let events_5 = nodes[0].node.get_and_clear_pending_events(); + assert_eq!(events_5.len(), 1); + + // Expect a PaymentPathFailed event with a ChannelFailure network update for the channel between + // the node originating the error to its next hop. + match events_5[0] { + Event::PaymentPathFailed { network_update: + Some(NetworkUpdate::ChannelFailure { short_channel_id, is_permanent }), error_code, .. + } => { + assert_eq!(short_channel_id, chan_2.0.contents.short_channel_id); + assert!(is_permanent); + assert_eq!(error_code, Some(0x8000|0x4000|0x2000|4)); + }, + _ => panic!("Unexpected event"), + } + + // TODO: Test actual removal of channel from NetworkGraph when it's implemented. +} + fn do_test_failure_delay_dust_htlc_local_commitment(announce_latest: bool) { // Dust-HTLC failure updates must be delayed until failure-trigger tx (in this case local commitment) reach ANTI_REORG_DELAY // We can have at most two valid local commitment tx, so both cases must be covered, and both txs must be checked to get them all as