From 5e07c60f9e6ef6d680d1a55011077c548c411c8d Mon Sep 17 00:00:00 2001 From: Matt Corallo Date: Wed, 7 Sep 2022 21:08:22 +0000 Subject: [PATCH] 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 3795ad5ee..34be28438 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); -- 2.39.5