Skip to content

Fix max-value overflows in set_max_path_length #3522

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 1 commit into from
Jan 13, 2025
Merged

Fix max-value overflows in set_max_path_length #3522

Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
35 changes: 26 additions & 9 deletions lightning/src/ln/onion_utils.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -239,7 +239,7 @@ where
// the intended recipient).
let value_msat = if cur_value_msat == 0 { hop.fee_msat } else { cur_value_msat };
let cltv = if cur_cltv == starting_htlc_offset {
hop.cltv_expiry_delta + starting_htlc_offset
hop.cltv_expiry_delta.saturating_add(starting_htlc_offset)
} else {
cur_cltv
};
Expand Down Expand Up @@ -307,7 +307,7 @@ where
if cur_value_msat >= 21000000 * 100000000 * 1000 {
return Err(APIError::InvalidRoute { err: "Channel fees overflowed?".to_owned() });
}
cur_cltv += hop.cltv_expiry_delta as u32;
cur_cltv = cur_cltv.saturating_add(hop.cltv_expiry_delta as u32);
if cur_cltv >= 500000000 {
return Err(APIError::InvalidRoute { err: "Channel CLTV overflowed?".to_owned() });
}
Expand All @@ -333,10 +333,10 @@ pub(crate) fn set_max_path_length(
.saturating_add(PAYLOAD_HMAC_LEN);

const OVERPAY_ESTIMATE_MULTIPLER: u64 = 3;
let final_value_msat_with_overpay_buffer = core::cmp::max(
route_params.final_value_msat.saturating_mul(OVERPAY_ESTIMATE_MULTIPLER),
MIN_FINAL_VALUE_ESTIMATE_WITH_OVERPAY,
);
let final_value_msat_with_overpay_buffer = route_params
.final_value_msat
.saturating_mul(OVERPAY_ESTIMATE_MULTIPLER)
.clamp(MIN_FINAL_VALUE_ESTIMATE_WITH_OVERPAY, 0x1000_0000);

let blinded_tail_opt = route_params
.payment_params
Expand All @@ -351,13 +351,15 @@ pub(crate) fn set_max_path_length(
excess_final_cltv_expiry_delta: 0,
});

let cltv_expiry_delta =
core::cmp::min(route_params.payment_params.max_total_cltv_expiry_delta, 0x1000_0000);
let unblinded_route_hop = RouteHop {
pubkey: PublicKey::from_slice(&[2; 33]).unwrap(),
node_features: NodeFeatures::empty(),
short_channel_id: 42,
channel_features: ChannelFeatures::empty(),
fee_msat: final_value_msat_with_overpay_buffer,
cltv_expiry_delta: route_params.payment_params.max_total_cltv_expiry_delta,
cltv_expiry_delta,
maybe_announced_channel: false,
};
let mut num_reserved_bytes: usize = 0;
Expand Down Expand Up @@ -1280,7 +1282,7 @@ fn decode_next_hop<T, R: ReadableArgs<T>, N: NextPacketBytes>(
mod tests {
use crate::io;
use crate::ln::msgs;
use crate::routing::router::{Path, Route, RouteHop};
use crate::routing::router::{Path, PaymentParameters, Route, RouteHop};
use crate::types::features::{ChannelFeatures, NodeFeatures};
use crate::types::payment::PaymentHash;
use crate::util::ser::{VecWriter, Writeable, Writer};
Expand All @@ -1292,7 +1294,7 @@ mod tests {
use bitcoin::secp256k1::Secp256k1;
use bitcoin::secp256k1::{PublicKey, SecretKey};

use super::OnionKeys;
use super::*;

fn get_test_session_key() -> SecretKey {
let hex = "4141414141414141414141414141414141414141414141414141414141414141";
Expand Down Expand Up @@ -1607,4 +1609,19 @@ mod tests {
writer.write_all(&self.data[..])
}
}

#[test]
fn max_length_with_no_cltv_limit() {
Copy link
Contributor

Choose a reason for hiding this comment

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

Can this go in max_path_len_tests.rs?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Could do...Generally we haven't shied away from doing unit tests in the same module that we're testing, at least for modules that aren't 10k LoC already 😅...

// While users generally shouldn't do this, we shouldn't overflow when
// `max_total_cltv_expiry_delta` is `u32::MAX`.
let recipient = PublicKey::from_slice(&[2; 33]).unwrap();
let mut route_params = RouteParameters {
payment_params: PaymentParameters::for_keysend(recipient, u32::MAX, true),
final_value_msat: u64::MAX,
max_total_routing_fee_msat: Some(u64::MAX),
};
route_params.payment_params.max_total_cltv_expiry_delta = u32::MAX;
let recipient_onion = RecipientOnionFields::spontaneous_empty();
set_max_path_length(&mut route_params, &recipient_onion, None, None, 42).unwrap();
}
}
Loading