From 9f5e574b0b0f0f87a48f79d3150e93570957965c Mon Sep 17 00:00:00 2001 From: Valentine Wallace Date: Sat, 12 Aug 2023 19:02:56 -0400 Subject: [PATCH 1/7] Clean up process_onion_failure Get rid of a bunch of indentation and be more idiomatic. --- lightning/src/ln/onion_utils.rs | 409 ++++++++++++++++---------------- 1 file changed, 209 insertions(+), 200 deletions(-) diff --git a/lightning/src/ln/onion_utils.rs b/lightning/src/ln/onion_utils.rs index 7e0ccbe96..31f1d5064 100644 --- a/lightning/src/ln/onion_utils.rs +++ b/lightning/src/ln/onion_utils.rs @@ -385,224 +385,233 @@ pub(super) fn build_first_hop_failure_packet(shared_secret: &[u8], failure_type: /// Returns update, a boolean indicating that the payment itself failed, the short channel id of /// the responsible channel, and the error code. #[inline] -pub(super) fn process_onion_failure(secp_ctx: &Secp256k1, logger: &L, htlc_source: &HTLCSource, mut packet_decrypted: Vec) -> (Option, Option, bool, Option, Option>) where L::Target: Logger { - if let &HTLCSource::OutboundRoute { ref path, ref session_priv, ref first_hop_htlc_msat, .. } = htlc_source { - let mut res = None; - let mut htlc_msat = *first_hop_htlc_msat; - let mut error_code_ret = None; - let mut error_packet_ret = None; - let mut is_from_final_node = false; +pub(super) fn process_onion_failure( + secp_ctx: &Secp256k1, logger: &L, htlc_source: &HTLCSource, mut packet_decrypted: Vec +) -> (Option, Option, bool, Option, Option>) +where L::Target: Logger { + let (path, session_priv, first_hop_htlc_msat) = if let &HTLCSource::OutboundRoute { + ref path, ref session_priv, ref first_hop_htlc_msat, .. + } = htlc_source { + (path, session_priv, first_hop_htlc_msat) + } else { unreachable!() }; + let mut res = None; + let mut htlc_msat = *first_hop_htlc_msat; + let mut error_code_ret = None; + let mut error_packet_ret = None; + let mut is_from_final_node = false; - // Handle packed channel/node updates for passing back for the route handler - construct_onion_keys_callback(secp_ctx, &path.hops, session_priv, |shared_secret, _, _, route_hop, route_hop_idx| { - if res.is_some() { return; } + // Handle packed channel/node updates for passing back for the route handler + construct_onion_keys_callback(secp_ctx, &path.hops, session_priv, |shared_secret, _, _, route_hop, route_hop_idx| { + if res.is_some() { return; } - let amt_to_forward = htlc_msat - route_hop.fee_msat; - htlc_msat = amt_to_forward; + let amt_to_forward = htlc_msat - route_hop.fee_msat; + htlc_msat = amt_to_forward; - let ammag = gen_ammag_from_shared_secret(shared_secret.as_ref()); + let ammag = gen_ammag_from_shared_secret(shared_secret.as_ref()); - let mut decryption_tmp = Vec::with_capacity(packet_decrypted.len()); - decryption_tmp.resize(packet_decrypted.len(), 0); - let mut chacha = ChaCha20::new(&ammag, &[0u8; 8]); - chacha.process(&packet_decrypted, &mut decryption_tmp[..]); - packet_decrypted = decryption_tmp; + let mut decryption_tmp = Vec::with_capacity(packet_decrypted.len()); + decryption_tmp.resize(packet_decrypted.len(), 0); + let mut chacha = ChaCha20::new(&ammag, &[0u8; 8]); + chacha.process(&packet_decrypted, &mut decryption_tmp[..]); + packet_decrypted = decryption_tmp; - // The failing hop includes either the inbound channel to the recipient or the outbound - // channel from the current hop (i.e., the next hop's inbound channel). - is_from_final_node = route_hop_idx + 1 == path.hops.len(); - let failing_route_hop = if is_from_final_node { route_hop } else { &path.hops[route_hop_idx + 1] }; + // The failing hop includes either the inbound channel to the recipient or the outbound + // channel from the current hop (i.e., the next hop's inbound channel). + is_from_final_node = route_hop_idx + 1 == path.hops.len(); + let failing_route_hop = if is_from_final_node { route_hop } else { &path.hops[route_hop_idx + 1] }; - if let Ok(err_packet) = msgs::DecodedOnionErrorPacket::read(&mut Cursor::new(&packet_decrypted)) { - let um = gen_um_from_shared_secret(shared_secret.as_ref()); - let mut hmac = HmacEngine::::new(&um); - hmac.input(&err_packet.encode()[32..]); + let err_packet = match msgs::DecodedOnionErrorPacket::read(&mut Cursor::new(&packet_decrypted)) { + Ok(p) => p, + Err(_) => return + }; + let um = gen_um_from_shared_secret(shared_secret.as_ref()); + let mut hmac = HmacEngine::::new(&um); + hmac.input(&err_packet.encode()[32..]); - 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; + if !fixed_time_eq(&Hmac::from_engine(hmac).into_inner(), &err_packet.hmac) { return } + let error_code_slice = match err_packet.failuremsg.get(0..2) { + Some(s) => s, + None => { + // Useless packet that we can't use but it passed HMAC, so it + // definitely came from the peer in question + let network_update = Some(NetworkUpdate::NodeFailure { + node_id: route_hop.pubkey, + is_permanent: true, + }); + let short_channel_id = Some(route_hop.short_channel_id); + res = Some((network_update, short_channel_id, !is_from_final_node)); + return + } + }; + const BADONION: u16 = 0x8000; + const PERM: u16 = 0x4000; + const NODE: u16 = 0x2000; + const UPDATE: u16 = 0x1000; - let error_code = u16::from_be_bytes(error_code_slice.try_into().expect("len is 2")); - error_code_ret = Some(error_code); - error_packet_ret = Some(err_packet.failuremsg[2..].to_vec()); + let error_code = u16::from_be_bytes(error_code_slice.try_into().expect("len is 2")); + error_code_ret = Some(error_code); + error_packet_ret = Some(err_packet.failuremsg[2..].to_vec()); - let (debug_field, debug_field_size) = errors::get_onion_debug_field(error_code); + let (debug_field, debug_field_size) = errors::get_onion_debug_field(error_code); - // indicate that payment parameter has failed and no need to - // update Route object - let payment_failed = match error_code & 0xff { - 15|16|17|18|19|23 => true, - _ => false, - } && is_from_final_node; // PERM bit observed below even if this error is from the intermediate nodes + // indicate that payment parameter has failed and no need to + // update Route object + let payment_failed = match error_code & 0xff { + 15|16|17|18|19|23 => true, + _ => false, + } && is_from_final_node; // PERM bit observed below even if this error is from the intermediate nodes - let mut network_update = None; - let mut short_channel_id = None; + let mut network_update = None; + let mut short_channel_id = None; - 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 { - if !payment_failed { - network_update = Some(NetworkUpdate::ChannelFailure { - short_channel_id: failing_route_hop.short_channel_id, - is_permanent: true, - }); - short_channel_id = Some(failing_route_hop.short_channel_id); - } - } 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) { - // Historically, the BOLTs were unclear if the message type - // bytes should be included here or not. The BOLTs have now - // been updated to indicate that they *are* included, but many - // nodes still send messages without the type bytes, so we - // support both here. - // TODO: Switch to hard require the type prefix, as the current - // permissiveness introduces the (although small) possibility - // that we fail to decode legitimate channel updates that - // happen to start with ChannelUpdate::TYPE, i.e., [0x01, 0x02]. - if update_slice.len() > 2 && update_slice[0..2] == msgs::ChannelUpdate::TYPE.to_be_bytes() { - update_slice = &update_slice[2..]; - } else { - log_trace!(logger, "Failure provided features a channel update without type prefix. Deprecated, but allowing for now."); - } - let update_opt = msgs::ChannelUpdate::read(&mut Cursor::new(&update_slice)); - if update_opt.is_ok() || update_slice.is_empty() { - // if channel_update should NOT have caused the failure: - // MAY treat the channel_update as invalid. - let is_chan_update_invalid = match error_code & 0xff { - 7 => false, - 11 => update_opt.is_ok() && - amt_to_forward > - update_opt.as_ref().unwrap().contents.htlc_minimum_msat, - 12 => update_opt.is_ok() && amt_to_forward - .checked_mul(update_opt.as_ref().unwrap() - .contents.fee_proportional_millionths as u64) - .map(|prop_fee| prop_fee / 1_000_000) - .and_then(|prop_fee| prop_fee.checked_add( - update_opt.as_ref().unwrap().contents.fee_base_msat as u64)) - .map(|fee_msats| route_hop.fee_msat >= fee_msats) - .unwrap_or(false), - 13 => update_opt.is_ok() && - route_hop.cltv_expiry_delta as u16 >= - update_opt.as_ref().unwrap().contents.cltv_expiry_delta, - 14 => false, // expiry_too_soon; always valid? - 20 => update_opt.as_ref().unwrap().contents.flags & 2 == 0, - _ => false, // unknown error code; take channel_update as valid - }; - if is_chan_update_invalid { - // This probably indicates the node which forwarded - // to the node in question corrupted something. - network_update = Some(NetworkUpdate::ChannelFailure { - short_channel_id: route_hop.short_channel_id, - is_permanent: true, - }); - } else { - if let Ok(chan_update) = update_opt { - // Make sure the ChannelUpdate contains the expected - // short channel id. - if failing_route_hop.short_channel_id == chan_update.contents.short_channel_id { - short_channel_id = Some(failing_route_hop.short_channel_id); - } else { - log_info!(logger, "Node provided a channel_update for which it was not authoritative, ignoring."); - } - network_update = Some(NetworkUpdate::ChannelUpdateMessage { - msg: chan_update, - }) - } else { - network_update = Some(NetworkUpdate::ChannelFailure { - short_channel_id: route_hop.short_channel_id, - is_permanent: false, - }); - } - }; - } else { - // If the channel_update had a non-zero length (i.e. was - // present) but we couldn't read it, treat it as a total - // node failure. - log_info!(logger, - "Failed to read a channel_update of len {} in an onion", - update_slice.len()); - } - } - } - if network_update.is_none() { - // They provided an UPDATE which was obviously bogus, not worth - // trying to relay through them anymore. - network_update = Some(NetworkUpdate::NodeFailure { - node_id: route_hop.pubkey, - is_permanent: true, - }); - } - if short_channel_id.is_none() { - short_channel_id = Some(route_hop.short_channel_id); - } - } else if payment_failed { - // Only blame the hop when a value in the HTLC doesn't match the - // corresponding value in the onion. - short_channel_id = match error_code & 0xff { - 18|19 => Some(route_hop.short_channel_id), - _ => None, - }; - } else { - // We can't understand their error messages and they failed to - // forward...they probably can't understand our forwards so its - // really not worth trying any further. - network_update = Some(NetworkUpdate::NodeFailure { - node_id: route_hop.pubkey, - is_permanent: true, - }); - short_channel_id = Some(route_hop.short_channel_id); - } - - 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); - if debug_field_size > 0 && err_packet.failuremsg.len() >= 4 + debug_field_size { - log_info!(logger, "Onion Error[from {}: {}({:#x}) {}({})] {}", route_hop.pubkey, title, error_code, debug_field, log_bytes!(&err_packet.failuremsg[4..4+debug_field_size]), description); - } - else { - log_info!(logger, "Onion Error[from {}: {}({:#x})] {}", route_hop.pubkey, title, error_code, description); - } + 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 { + if !payment_failed { + network_update = Some(NetworkUpdate::ChannelFailure { + short_channel_id: failing_route_hop.short_channel_id, + is_permanent: true, + }); + short_channel_id = Some(failing_route_hop.short_channel_id); + } + } 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) { + // Historically, the BOLTs were unclear if the message type + // bytes should be included here or not. The BOLTs have now + // been updated to indicate that they *are* included, but many + // nodes still send messages without the type bytes, so we + // support both here. + // TODO: Switch to hard require the type prefix, as the current + // permissiveness introduces the (although small) possibility + // that we fail to decode legitimate channel updates that + // happen to start with ChannelUpdate::TYPE, i.e., [0x01, 0x02]. + if update_slice.len() > 2 && update_slice[0..2] == msgs::ChannelUpdate::TYPE.to_be_bytes() { + update_slice = &update_slice[2..]; } else { - // Useless packet that we can't use but it passed HMAC, so it - // definitely came from the peer in question - let network_update = Some(NetworkUpdate::NodeFailure { - node_id: route_hop.pubkey, - is_permanent: true, - }); - let short_channel_id = Some(route_hop.short_channel_id); - res = Some((network_update, short_channel_id, !is_from_final_node)); + log_trace!(logger, "Failure provided features a channel update without type prefix. Deprecated, but allowing for now."); + } + let update_opt = msgs::ChannelUpdate::read(&mut Cursor::new(&update_slice)); + if update_opt.is_ok() || update_slice.is_empty() { + // if channel_update should NOT have caused the failure: + // MAY treat the channel_update as invalid. + let is_chan_update_invalid = match error_code & 0xff { + 7 => false, + 11 => update_opt.is_ok() && + amt_to_forward > + update_opt.as_ref().unwrap().contents.htlc_minimum_msat, + 12 => update_opt.is_ok() && amt_to_forward + .checked_mul(update_opt.as_ref().unwrap() + .contents.fee_proportional_millionths as u64) + .map(|prop_fee| prop_fee / 1_000_000) + .and_then(|prop_fee| prop_fee.checked_add( + update_opt.as_ref().unwrap().contents.fee_base_msat as u64)) + .map(|fee_msats| route_hop.fee_msat >= fee_msats) + .unwrap_or(false), + 13 => update_opt.is_ok() && + route_hop.cltv_expiry_delta as u16 >= + update_opt.as_ref().unwrap().contents.cltv_expiry_delta, + 14 => false, // expiry_too_soon; always valid? + 20 => update_opt.as_ref().unwrap().contents.flags & 2 == 0, + _ => false, // unknown error code; take channel_update as valid + }; + if is_chan_update_invalid { + // This probably indicates the node which forwarded + // to the node in question corrupted something. + network_update = Some(NetworkUpdate::ChannelFailure { + short_channel_id: route_hop.short_channel_id, + is_permanent: true, + }); + } else { + if let Ok(chan_update) = update_opt { + // Make sure the ChannelUpdate contains the expected + // short channel id. + if failing_route_hop.short_channel_id == chan_update.contents.short_channel_id { + short_channel_id = Some(failing_route_hop.short_channel_id); + } else { + log_info!(logger, "Node provided a channel_update for which it was not authoritative, ignoring."); + } + network_update = Some(NetworkUpdate::ChannelUpdateMessage { + msg: chan_update, + }) + } else { + network_update = Some(NetworkUpdate::ChannelFailure { + short_channel_id: route_hop.short_channel_id, + is_permanent: false, + }); + } + }; + } else { + // If the channel_update had a non-zero length (i.e. was + // present) but we couldn't read it, treat it as a total + // node failure. + log_info!(logger, + "Failed to read a channel_update of len {} in an onion", + update_slice.len()); } } } - }).expect("Route that we sent via spontaneously grew invalid keys in the middle of it?"); - if let Some((channel_update, short_channel_id, payment_retryable)) = res { - (channel_update, short_channel_id, payment_retryable, error_code_ret, error_packet_ret) + if network_update.is_none() { + // They provided an UPDATE which was obviously bogus, not worth + // trying to relay through them anymore. + network_update = Some(NetworkUpdate::NodeFailure { + node_id: route_hop.pubkey, + is_permanent: true, + }); + } + if short_channel_id.is_none() { + short_channel_id = Some(route_hop.short_channel_id); + } + } else if payment_failed { + // Only blame the hop when a value in the HTLC doesn't match the + // corresponding value in the onion. + short_channel_id = match error_code & 0xff { + 18|19 => Some(route_hop.short_channel_id), + _ => None, + }; } else { - // only not set either packet unparseable or hmac does not match with any - // payment not retryable only when garbage is from the final node - (None, None, !is_from_final_node, None, None) + // We can't understand their error messages and they failed to + // forward...they probably can't understand our forwards so its + // really not worth trying any further. + network_update = Some(NetworkUpdate::NodeFailure { + node_id: route_hop.pubkey, + is_permanent: true, + }); + short_channel_id = Some(route_hop.short_channel_id); } - } else { unreachable!(); } + + 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); + if debug_field_size > 0 && err_packet.failuremsg.len() >= 4 + debug_field_size { + log_info!(logger, "Onion Error[from {}: {}({:#x}) {}({})] {}", route_hop.pubkey, title, error_code, debug_field, log_bytes!(&err_packet.failuremsg[4..4+debug_field_size]), description); + } else { + log_info!(logger, "Onion Error[from {}: {}({:#x})] {}", route_hop.pubkey, title, error_code, description); + } + }).expect("Route that we sent via spontaneously grew invalid keys in the middle of it?"); + if let Some((channel_update, short_channel_id, payment_retryable)) = res { + (channel_update, short_channel_id, payment_retryable, error_code_ret, error_packet_ret) + } else { + // only not set either packet unparseable or hmac does not match with any + // payment not retryable only when garbage is from the final node + (None, None, !is_from_final_node, None, None) + } } #[derive(Clone)] // See Channel::revoke_and_ack for why, tl;dr: Rust bug From 6144e30c0e99ff52f4febed652d62b48a9f4c924 Mon Sep 17 00:00:00 2001 From: Valentine Wallace Date: Sat, 12 Aug 2023 19:11:07 -0400 Subject: [PATCH 2/7] Wrap process_onion_failure comments at 100chars And fix an its vs it's grammar --- lightning/src/ln/onion_utils.rs | 35 +++++++++++++++------------------ 1 file changed, 16 insertions(+), 19 deletions(-) diff --git a/lightning/src/ln/onion_utils.rs b/lightning/src/ln/onion_utils.rs index 31f1d5064..88b40ebd8 100644 --- a/lightning/src/ln/onion_utils.rs +++ b/lightning/src/ln/onion_utils.rs @@ -415,8 +415,8 @@ where L::Target: Logger { chacha.process(&packet_decrypted, &mut decryption_tmp[..]); packet_decrypted = decryption_tmp; - // The failing hop includes either the inbound channel to the recipient or the outbound - // channel from the current hop (i.e., the next hop's inbound channel). + // The failing hop includes either the inbound channel to the recipient or the outbound channel + // from the current hop (i.e., the next hop's inbound channel). is_from_final_node = route_hop_idx + 1 == path.hops.len(); let failing_route_hop = if is_from_final_node { route_hop } else { &path.hops[route_hop_idx + 1] }; @@ -432,8 +432,8 @@ where L::Target: Logger { let error_code_slice = match err_packet.failuremsg.get(0..2) { Some(s) => s, None => { - // Useless packet that we can't use but it passed HMAC, so it - // definitely came from the peer in question + // Useless packet that we can't use but it passed HMAC, so it definitely came from the peer + // in question let network_update = Some(NetworkUpdate::NodeFailure { node_id: route_hop.pubkey, is_permanent: true, @@ -454,8 +454,7 @@ where L::Target: Logger { let (debug_field, debug_field_size) = errors::get_onion_debug_field(error_code); - // indicate that payment parameter has failed and no need to - // update Route object + // indicate that payment parameter has failed and no need to update Route object let payment_failed = match error_code & 0xff { 15|16|17|18|19|23 => true, _ => false, @@ -465,14 +464,13 @@ where L::Target: Logger { let mut short_channel_id = None; 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. + // 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, @@ -579,16 +577,15 @@ where L::Target: Logger { short_channel_id = Some(route_hop.short_channel_id); } } else if payment_failed { - // Only blame the hop when a value in the HTLC doesn't match the - // corresponding value in the onion. + // Only blame the hop when a value in the HTLC doesn't match the corresponding value in the + // onion. short_channel_id = match error_code & 0xff { 18|19 => Some(route_hop.short_channel_id), _ => None, }; } else { - // We can't understand their error messages and they failed to - // forward...they probably can't understand our forwards so its - // really not worth trying any further. + // We can't understand their error messages and they failed to forward...they probably can't + // understand our forwards so it's really not worth trying any further. network_update = Some(NetworkUpdate::NodeFailure { node_id: route_hop.pubkey, is_permanent: true, From cf13f78cd1957753f694642361fae1e0daa5b98a Mon Sep 17 00:00:00 2001 From: Valentine Wallace Date: Fri, 7 Jul 2023 13:35:42 -0400 Subject: [PATCH 3/7] Blinded paths: support constructing onion keys + handling onion errors We don't bother actually parsing errors from within a blinded path, since all errors should be wiped by the introduction node by the time it gets back to us (the sender). --- lightning/src/ln/onion_utils.rs | 47 ++++++++++++++++++++++++++------- 1 file changed, 37 insertions(+), 10 deletions(-) diff --git a/lightning/src/ln/onion_utils.rs b/lightning/src/ln/onion_utils.rs index 88b40ebd8..03ba88771 100644 --- a/lightning/src/ln/onion_utils.rs +++ b/lightning/src/ln/onion_utils.rs @@ -104,12 +104,23 @@ pub(crate) fn next_hop_packet_pubkey (secp_ctx: &Secp256k1, path: &Vec, session_priv: &SecretKey, mut callback: FType) -> Result<(), secp256k1::Error> { +pub(super) fn construct_onion_keys_callback( + secp_ctx: &Secp256k1, path: &Path, session_priv: &SecretKey, mut callback: FType +) -> Result<(), secp256k1::Error> +where + T: secp256k1::Signing, + FType: FnMut(SharedSecret, [u8; 32], PublicKey, Option<&RouteHop>, usize) +{ let mut blinded_priv = session_priv.clone(); let mut blinded_pub = PublicKey::from_secret_key(secp_ctx, &blinded_priv); - for (idx, hop) in path.iter().enumerate() { - let shared_secret = SharedSecret::new(&hop.pubkey, &blinded_priv); + let unblinded_hops_iter = path.hops.iter().map(|h| (&h.pubkey, Some(h))); + let blinded_pks_iter = path.blinded_tail.as_ref() + .map(|t| t.hops.iter()).unwrap_or([].iter()) + .skip(1) // Skip the intro node because it's included in the unblinded hops + .map(|h| (&h.blinded_node_id, None)); + for (idx, (pubkey, route_hop_opt)) in unblinded_hops_iter.chain(blinded_pks_iter).enumerate() { + let shared_secret = SharedSecret::new(pubkey, &blinded_priv); let mut sha = Sha256::engine(); sha.input(&blinded_pub.serialize()[..]); @@ -121,7 +132,7 @@ pub(super) fn construct_onion_keys_callback(secp_ctx: &Secp256k1, path: &Path, session_priv: &SecretKey) -> Result, secp256k1::Error> { let mut res = Vec::with_capacity(path.hops.len()); - construct_onion_keys_callback(secp_ctx, &path.hops, session_priv, |shared_secret, _blinding_factor, ephemeral_pubkey, _, _| { + construct_onion_keys_callback(secp_ctx, &path, session_priv, + |shared_secret, _blinding_factor, ephemeral_pubkey, _, _| + { let (rho, mu) = gen_rho_mu_from_shared_secret(shared_secret.as_ref()); res.push(OnionKeys { @@ -400,10 +413,28 @@ where L::Target: Logger { let mut error_packet_ret = None; let mut is_from_final_node = false; + const BADONION: u16 = 0x8000; + const PERM: u16 = 0x4000; + const NODE: u16 = 0x2000; + const UPDATE: u16 = 0x1000; + // Handle packed channel/node updates for passing back for the route handler - construct_onion_keys_callback(secp_ctx, &path.hops, session_priv, |shared_secret, _, _, route_hop, route_hop_idx| { + construct_onion_keys_callback(secp_ctx, &path, session_priv, + |shared_secret, _, _, route_hop_opt, route_hop_idx| + { if res.is_some() { return; } + let route_hop = match route_hop_opt { + Some(hop) => hop, + None => { + // Got an error from within a blinded route. + error_code_ret = Some(BADONION | PERM | 24); // invalid_onion_blinding + error_packet_ret = Some(vec![0; 32]); + is_from_final_node = false; + return + }, + }; + let amt_to_forward = htlc_msat - route_hop.fee_msat; htlc_msat = amt_to_forward; @@ -443,10 +474,6 @@ where L::Target: Logger { return } }; - const BADONION: u16 = 0x8000; - const PERM: u16 = 0x4000; - const NODE: u16 = 0x2000; - const UPDATE: u16 = 0x1000; let error_code = u16::from_be_bytes(error_code_slice.try_into().expect("len is 2")); error_code_ret = Some(error_code); From ec5e837cc2390573fa4273381c182a174ebfe027 Mon Sep 17 00:00:00 2001 From: Valentine Wallace Date: Tue, 27 Jun 2023 12:35:03 -0400 Subject: [PATCH 4/7] Generalize next_hop_packet_pubkey onion util Useful for generating a next hop blinding point when forwarding a blinded payment. --- lightning/src/blinded_path/mod.rs | 14 +++----------- lightning/src/ln/channelmanager.rs | 4 ++-- lightning/src/ln/onion_utils.rs | 11 +++++++---- lightning/src/onion_message/messenger.rs | 15 +++++---------- 4 files changed, 17 insertions(+), 27 deletions(-) diff --git a/lightning/src/blinded_path/mod.rs b/lightning/src/blinded_path/mod.rs index dabfb79c7..c52df1651 100644 --- a/lightning/src/blinded_path/mod.rs +++ b/lightning/src/blinded_path/mod.rs @@ -11,9 +11,7 @@ pub(crate) mod utils; -use bitcoin::hashes::{Hash, HashEngine}; -use bitcoin::hashes::sha256::Hash as Sha256; -use bitcoin::secp256k1::{self, PublicKey, Scalar, Secp256k1, SecretKey}; +use bitcoin::secp256k1::{self, PublicKey, Secp256k1, SecretKey}; use crate::sign::{EntropySource, NodeSigner, Recipient}; use crate::onion_message::ControlTlvs; @@ -97,14 +95,8 @@ impl BlindedPath { let mut new_blinding_point = match next_blinding_override { Some(blinding_point) => blinding_point, None => { - let blinding_factor = { - let mut sha = Sha256::engine(); - sha.input(&self.blinding_point.serialize()[..]); - sha.input(control_tlvs_ss.as_ref()); - Sha256::from_engine(sha).into_inner() - }; - self.blinding_point.mul_tweak(secp_ctx, &Scalar::from_be_bytes(blinding_factor).unwrap()) - .map_err(|_| ())? + onion_utils::next_hop_pubkey(secp_ctx, self.blinding_point, + control_tlvs_ss.as_ref()).map_err(|_| ())? } }; mem::swap(&mut self.blinding_point, &mut new_blinding_point); diff --git a/lightning/src/ln/channelmanager.rs b/lightning/src/ln/channelmanager.rs index 961f25cc6..5c2d392ba 100644 --- a/lightning/src/ln/channelmanager.rs +++ b/lightning/src/ln/channelmanager.rs @@ -2868,9 +2868,9 @@ where short_channel_id, amt_to_forward, outgoing_cltv_value }, .. } => { - let next_pk = onion_utils::next_hop_packet_pubkey(&self.secp_ctx, + let next_packet_pk = onion_utils::next_hop_pubkey(&self.secp_ctx, msg.onion_routing_packet.public_key.unwrap(), &shared_secret); - (short_channel_id, amt_to_forward, outgoing_cltv_value, Some(next_pk)) + (short_channel_id, amt_to_forward, outgoing_cltv_value, Some(next_packet_pk)) }, // We'll do receive checks in [`Self::construct_pending_htlc_info`] so we have access to the // inbound channel's state. diff --git a/lightning/src/ln/onion_utils.rs b/lightning/src/ln/onion_utils.rs index 03ba88771..688a9c961 100644 --- a/lightning/src/ln/onion_utils.rs +++ b/lightning/src/ln/onion_utils.rs @@ -91,15 +91,18 @@ pub(super) fn gen_pad_from_shared_secret(shared_secret: &[u8]) -> [u8; 32] { Hmac::from_engine(hmac).into_inner() } -pub(crate) fn next_hop_packet_pubkey(secp_ctx: &Secp256k1, packet_pubkey: PublicKey, packet_shared_secret: &[u8; 32]) -> Result { +/// Calculates a pubkey for the next hop, such as the next hop's packet pubkey or blinding point. +pub(crate) fn next_hop_pubkey( + secp_ctx: &Secp256k1, curr_pubkey: PublicKey, shared_secret: &[u8] +) -> Result { let blinding_factor = { let mut sha = Sha256::engine(); - sha.input(&packet_pubkey.serialize()[..]); - sha.input(packet_shared_secret); + sha.input(&curr_pubkey.serialize()[..]); + sha.input(shared_secret); Sha256::from_engine(sha).into_inner() }; - packet_pubkey.mul_tweak(secp_ctx, &Scalar::from_be_bytes(blinding_factor).unwrap()) + curr_pubkey.mul_tweak(secp_ctx, &Scalar::from_be_bytes(blinding_factor).unwrap()) } // can only fail if an intermediary hop has an invalid public key or session_priv is invalid diff --git a/lightning/src/onion_message/messenger.rs b/lightning/src/onion_message/messenger.rs index a3613605c..7d5f4a22e 100644 --- a/lightning/src/onion_message/messenger.rs +++ b/lightning/src/onion_message/messenger.rs @@ -490,7 +490,7 @@ where // unwrapping the onion layers to get to the final payload. Since we don't have the option // of creating blinded paths with dummy hops currently, we should be ok to not handle this // for now. - let new_pubkey = match onion_utils::next_hop_packet_pubkey(&self.secp_ctx, msg.onion_routing_packet.public_key, &onion_decode_ss) { + let new_pubkey = match onion_utils::next_hop_pubkey(&self.secp_ctx, msg.onion_routing_packet.public_key, &onion_decode_ss) { Ok(pk) => pk, Err(e) => { log_trace!(self.logger, "Failed to compute next hop packet pubkey: {}", e); @@ -507,21 +507,16 @@ where blinding_point: match next_blinding_override { Some(blinding_point) => blinding_point, None => { - let blinding_factor = { - let mut sha = Sha256::engine(); - sha.input(&msg.blinding_point.serialize()[..]); - sha.input(control_tlvs_ss.as_ref()); - Sha256::from_engine(sha).into_inner() - }; - let next_blinding_point = msg.blinding_point; - match next_blinding_point.mul_tweak(&self.secp_ctx, &Scalar::from_be_bytes(blinding_factor).unwrap()) { + match onion_utils::next_hop_pubkey( + &self.secp_ctx, msg.blinding_point, control_tlvs_ss.as_ref() + ) { Ok(bp) => bp, Err(e) => { log_trace!(self.logger, "Failed to compute next blinding point: {}", e); return } } - }, + } }, onion_routing_packet: outgoing_packet, }; From cd2ebb769cf1221b3c23aedd731dd77b46e20415 Mon Sep 17 00:00:00 2001 From: Valentine Wallace Date: Thu, 20 Jul 2023 13:42:12 -0400 Subject: [PATCH 5/7] Add missing test coverage for bogus err packet with valid hmac --- lightning/src/ln/onion_route_tests.rs | 29 +++++++++++++++++++++++++-- 1 file changed, 27 insertions(+), 2 deletions(-) diff --git a/lightning/src/ln/onion_route_tests.rs b/lightning/src/ln/onion_route_tests.rs index c6ac77ac3..8550532c6 100644 --- a/lightning/src/ln/onion_route_tests.rs +++ b/lightning/src/ln/onion_route_tests.rs @@ -31,7 +31,8 @@ use crate::util::errors::APIError; use bitcoin::hash_types::BlockHash; -use bitcoin::hashes::Hash; +use bitcoin::hashes::{Hash, HashEngine}; +use bitcoin::hashes::hmac::{Hmac, HmacEngine}; use bitcoin::hashes::sha256::Hash as Sha256; use bitcoin::secp256k1; @@ -57,7 +58,12 @@ fn run_onion_failure_test(_name: &str, test_case: u8, nodes: &Vec, // 3: final node fails backward (but tamper onion payloads from node0) // 100: trigger error in the intermediate node and tamper returning fail_htlc // 200: trigger error in the final node and tamper returning fail_htlc -fn run_onion_failure_test_with_fail_intercept(_name: &str, test_case: u8, nodes: &Vec, route: &Route, payment_hash: &PaymentHash, payment_secret: &PaymentSecret, mut callback_msg: F1, mut callback_fail: F2, mut callback_node: F3, expected_retryable: bool, expected_error_code: Option, expected_channel_update: Option, expected_short_channel_id: Option) +fn run_onion_failure_test_with_fail_intercept( + _name: &str, test_case: u8, nodes: &Vec, route: &Route, payment_hash: &PaymentHash, + payment_secret: &PaymentSecret, mut callback_msg: F1, mut callback_fail: F2, + mut callback_node: F3, expected_retryable: bool, expected_error_code: Option, + expected_channel_update: Option, expected_short_channel_id: Option +) where F1: for <'a> FnMut(&'a mut msgs::UpdateAddHTLC), F2: for <'a> FnMut(&'a mut msgs::UpdateFailHTLC), F3: FnMut(), @@ -620,6 +626,25 @@ fn test_onion_failure() { }, ||{ nodes[2].node.fail_htlc_backwards(&payment_hash); }, true, Some(23), None, None); + + run_onion_failure_test_with_fail_intercept("bogus err packet with valid hmac", 200, &nodes, + &route, &payment_hash, &payment_secret, |_msg| {}, |msg| { + let session_priv = SecretKey::from_slice(&[3; 32]).unwrap(); + let onion_keys = onion_utils::construct_onion_keys(&Secp256k1::new(), &route.paths[0], &session_priv).unwrap(); + let mut decoded_err_packet = msgs::DecodedOnionErrorPacket { + failuremsg: vec![0], + pad: vec![0; 255], + hmac: [0; 32], + }; + let um = onion_utils::gen_um_from_shared_secret(&onion_keys[1].shared_secret.as_ref()); + let mut hmac = HmacEngine::::new(&um); + hmac.input(&decoded_err_packet.encode()[32..]); + decoded_err_packet.hmac = Hmac::from_engine(hmac).into_inner(); + msg.reason = onion_utils::encrypt_failure_packet( + &onion_keys[1].shared_secret.as_ref(), &decoded_err_packet.encode()[..]) + }, || nodes[2].node.fail_htlc_backwards(&payment_hash), false, None, + Some(NetworkUpdate::NodeFailure { node_id: route.paths[0].hops[1].pubkey, is_permanent: true }), + Some(channels[1].0.contents.short_channel_id)); } #[test] From fb9ad5686ef2ad3f9ef06ad19b9ef1c8b60f8c1f Mon Sep 17 00:00:00 2001 From: Valentine Wallace Date: Sat, 12 Aug 2023 20:27:35 -0400 Subject: [PATCH 6/7] Document and test 0-len channel update onion error case --- lightning/src/ln/onion_route_tests.rs | 24 ++++++++++++++++++++++++ lightning/src/ln/onion_utils.rs | 2 ++ 2 files changed, 26 insertions(+) diff --git a/lightning/src/ln/onion_route_tests.rs b/lightning/src/ln/onion_route_tests.rs index 8550532c6..785c11304 100644 --- a/lightning/src/ln/onion_route_tests.rs +++ b/lightning/src/ln/onion_route_tests.rs @@ -645,6 +645,30 @@ fn test_onion_failure() { }, || nodes[2].node.fail_htlc_backwards(&payment_hash), false, None, Some(NetworkUpdate::NodeFailure { node_id: route.paths[0].hops[1].pubkey, is_permanent: true }), Some(channels[1].0.contents.short_channel_id)); + run_onion_failure_test_with_fail_intercept("0-length channel update in UPDATE onion failure", 200, &nodes, + &route, &payment_hash, &payment_secret, |_msg| {}, |msg| { + let session_priv = SecretKey::from_slice(&[3; 32]).unwrap(); + let onion_keys = onion_utils::construct_onion_keys(&Secp256k1::new(), &route.paths[0], &session_priv).unwrap(); + let mut decoded_err_packet = msgs::DecodedOnionErrorPacket { + failuremsg: vec![ + 0x10, 0x7, // UPDATE|7 + 0x0, 0x0 // 0-len channel update + ], + pad: vec![0; 255 - 4 /* 4-byte error message */], + hmac: [0; 32], + }; + let um = onion_utils::gen_um_from_shared_secret(&onion_keys[1].shared_secret.as_ref()); + let mut hmac = HmacEngine::::new(&um); + hmac.input(&decoded_err_packet.encode()[32..]); + decoded_err_packet.hmac = Hmac::from_engine(hmac).into_inner(); + msg.reason = onion_utils::encrypt_failure_packet( + &onion_keys[1].shared_secret.as_ref(), &decoded_err_packet.encode()[..]) + }, || nodes[2].node.fail_htlc_backwards(&payment_hash), true, Some(0x1000|7), + Some(NetworkUpdate::ChannelFailure { + short_channel_id: channels[1].0.contents.short_channel_id, + is_permanent: false, + }), + Some(channels[1].0.contents.short_channel_id)); } #[test] diff --git a/lightning/src/ln/onion_utils.rs b/lightning/src/ln/onion_utils.rs index 688a9c961..64a1e3fa3 100644 --- a/lightning/src/ln/onion_utils.rs +++ b/lightning/src/ln/onion_utils.rs @@ -579,6 +579,8 @@ where L::Target: Logger { msg: chan_update, }) } else { + // The node in question intentionally encoded a 0-length channel update. This is + // likely due to https://github.com/ElementsProject/lightning/issues/6200. network_update = Some(NetworkUpdate::ChannelFailure { short_channel_id: route_hop.short_channel_id, is_permanent: false, From 7b317125572781a2fd04502a87f914a444c91821 Mon Sep 17 00:00:00 2001 From: Valentine Wallace Date: Thu, 20 Jul 2023 13:59:23 -0400 Subject: [PATCH 7/7] Struct-ify decoded onion failures To avoid several long hard-to-read tuple return values. --- lightning/src/ln/onion_utils.rs | 47 ++++++++++++++++++++++------ lightning/src/ln/outbound_payment.rs | 9 ++++-- 2 files changed, 43 insertions(+), 13 deletions(-) diff --git a/lightning/src/ln/onion_utils.rs b/lightning/src/ln/onion_utils.rs index 64a1e3fa3..d55077770 100644 --- a/lightning/src/ln/onion_utils.rs +++ b/lightning/src/ln/onion_utils.rs @@ -396,15 +396,22 @@ pub(super) fn build_first_hop_failure_packet(shared_secret: &[u8], failure_type: encrypt_failure_packet(shared_secret, &failure_packet.encode()[..]) } +pub(crate) struct DecodedOnionFailure { + pub(crate) network_update: Option, + pub(crate) short_channel_id: Option, + pub(crate) payment_retryable: bool, + #[cfg(test)] + pub(crate) onion_error_code: Option, + #[cfg(test)] + pub(crate) onion_error_data: Option>, +} + /// Process failure we got back from upstream on a payment we sent (implying htlc_source is an /// OutboundRoute). -/// Returns update, a boolean indicating that the payment itself failed, the short channel id of -/// the responsible channel, and the error code. #[inline] pub(super) fn process_onion_failure( secp_ctx: &Secp256k1, logger: &L, htlc_source: &HTLCSource, mut packet_decrypted: Vec -) -> (Option, Option, bool, Option, Option>) -where L::Target: Logger { +) -> DecodedOnionFailure where L::Target: Logger { let (path, session_priv, first_hop_htlc_msat) = if let &HTLCSource::OutboundRoute { ref path, ref session_priv, ref first_hop_htlc_msat, .. } = htlc_source { @@ -634,12 +641,24 @@ where L::Target: Logger { log_info!(logger, "Onion Error[from {}: {}({:#x})] {}", route_hop.pubkey, title, error_code, description); } }).expect("Route that we sent via spontaneously grew invalid keys in the middle of it?"); - if let Some((channel_update, short_channel_id, payment_retryable)) = res { - (channel_update, short_channel_id, payment_retryable, error_code_ret, error_packet_ret) + if let Some((network_update, short_channel_id, payment_retryable)) = res { + DecodedOnionFailure { + network_update, short_channel_id, payment_retryable, + #[cfg(test)] + onion_error_code: error_code_ret, + #[cfg(test)] + onion_error_data: error_packet_ret + } } else { // only not set either packet unparseable or hmac does not match with any // payment not retryable only when garbage is from the final node - (None, None, !is_from_final_node, None, None) + DecodedOnionFailure { + network_update: None, short_channel_id: None, payment_retryable: !is_from_final_node, + #[cfg(test)] + onion_error_code: None, + #[cfg(test)] + onion_error_data: None + } } } @@ -763,12 +782,12 @@ impl HTLCFailReason { pub(super) fn decode_onion_failure( &self, secp_ctx: &Secp256k1, logger: &L, htlc_source: &HTLCSource - ) -> (Option, Option, bool, Option, Option>) - where L::Target: Logger { + ) -> DecodedOnionFailure where L::Target: Logger { match self.0 { HTLCFailReasonRepr::LightningError { ref err } => { process_onion_failure(secp_ctx, logger, &htlc_source, err.data.clone()) }, + #[allow(unused)] HTLCFailReasonRepr::Reason { ref failure_code, ref data, .. } => { // we get a fail_malformed_htlc from the first hop // TODO: We'd like to generate a NetworkUpdate for temporary @@ -776,7 +795,15 @@ impl HTLCFailReason { // generally ignores its view of our own channels as we provide them via // ChannelDetails. if let &HTLCSource::OutboundRoute { ref path, .. } = htlc_source { - (None, Some(path.hops[0].short_channel_id), true, Some(*failure_code), Some(data.clone())) + DecodedOnionFailure { + network_update: None, + payment_retryable: true, + short_channel_id: Some(path.hops[0].short_channel_id), + #[cfg(test)] + onion_error_code: Some(*failure_code), + #[cfg(test)] + onion_error_data: Some(data.clone()), + } } else { unreachable!(); } } } diff --git a/lightning/src/ln/outbound_payment.rs b/lightning/src/ln/outbound_payment.rs index 9c8d66458..d0851373b 100644 --- a/lightning/src/ln/outbound_payment.rs +++ b/lightning/src/ln/outbound_payment.rs @@ -17,7 +17,7 @@ use crate::sign::{EntropySource, NodeSigner, Recipient}; use crate::events::{self, PaymentFailureReason}; use crate::ln::{PaymentHash, PaymentPreimage, PaymentSecret}; use crate::ln::channelmanager::{ChannelDetails, EventCompletionAction, HTLCSource, IDEMPOTENCY_TIMEOUT_TICKS, PaymentId}; -use crate::ln::onion_utils::HTLCFailReason; +use crate::ln::onion_utils::{DecodedOnionFailure, HTLCFailReason}; use crate::routing::router::{InFlightHtlcs, Path, PaymentParameters, Route, RouteParameters, Router}; use crate::util::errors::APIError; use crate::util::logger::Logger; @@ -1293,9 +1293,12 @@ impl OutboundPayments { pending_events: &Mutex)>>, logger: &L, ) -> bool where L::Target: Logger { #[cfg(test)] - let (network_update, short_channel_id, payment_retryable, onion_error_code, onion_error_data) = onion_error.decode_onion_failure(secp_ctx, logger, &source); + let DecodedOnionFailure { + network_update, short_channel_id, payment_retryable, onion_error_code, onion_error_data + } = onion_error.decode_onion_failure(secp_ctx, logger, &source); #[cfg(not(test))] - let (network_update, short_channel_id, payment_retryable, _, _) = onion_error.decode_onion_failure(secp_ctx, logger, &source); + let DecodedOnionFailure { network_update, short_channel_id, payment_retryable } = + onion_error.decode_onion_failure(secp_ctx, logger, &source); let payment_is_probe = payment_is_probe(payment_hash, &payment_id, probing_cookie_secret); let mut session_priv_bytes = [0; 32];