diff --git a/CHANGELOG.md b/CHANGELOG.md index 7cc8a113285..694027f43d1 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -71,7 +71,19 @@ ## Security 0.2.3 fixes several underestimates of the anchor reserves required to ensure we -can reliably close channels and a sanitization issue. +can reliably close channels, several denial-of-service vulnerabilities and a +sanitization issue. + * `Bolt11Invoice::recover_payee_pub_key` no longer panics if called on an + invoice which set an explicit public key, rather than relying on public key + recovery. Note that this method is called from + `PaymentParameters::from_bolt11_invoice` (#4717). + * Maliciously-crafted unpayable invoices which have overflowing feerates will + no longer cause an `unwrap` failure panic (#4716). + * `possiblyrandom` did not properly generate random data except when it was + explicitly configured to. By default this means LDK is vulnerable to various + HashDoS attacks (#4719). + * `OMNameResolver` will no longer panic when looking up payment instructions + which include unicode characters at the start of a TXT record (#4718). * When using the `anchor_channel_reserves` module to calculate reserves required to pay for fees when closing anchor channels, zero-fee-commitment channels were not considered. This could allow a counterparty to open many @@ -80,6 +92,13 @@ can reliably close channels and a sanitization issue. the wallet by ignoring the `TxIn` cost to spend them (#4670). * `PrintableString` did not properly sanitize unicode format characters, allowing an attacker to corrupt the rendering of logs or UI (#4593, #4605). + * RGS data is now limited in how large of a graph it is able to cause a client + to store in memory. Note that RGS data is still considered a DoS vector in + general and you should only use semi-trusted RGS data (#4713). + * Counterparty-provided strings in failure messages are no longer logged in + full, reducing the ability of such a counterparty to spam our logs (#4714). + * Reading a corrupted `ChannelManager` or `ProbabilisticScorer` can no longer + cause us to allocate large amounts of memory (#4712). Thanks to Project Loupe for reporting most of the issues fixed in this release. diff --git a/lightning-background-processor/Cargo.toml b/lightning-background-processor/Cargo.toml index 828a8017574..ffe0b4ce743 100644 --- a/lightning-background-processor/Cargo.toml +++ b/lightning-background-processor/Cargo.toml @@ -1,6 +1,6 @@ [package] name = "lightning-background-processor" -version = "0.2.0" +version = "0.2.3" authors = ["Valentine Wallace "] license = "MIT OR Apache-2.0" repository = "https://github.com/lightningdevkit/rust-lightning" diff --git a/lightning-custom-message/Cargo.toml b/lightning-custom-message/Cargo.toml index 96632f24bc1..8c1bcced08c 100644 --- a/lightning-custom-message/Cargo.toml +++ b/lightning-custom-message/Cargo.toml @@ -1,6 +1,6 @@ [package] name = "lightning-custom-message" -version = "0.2.0" +version = "0.2.3" authors = ["Jeffrey Czyz"] license = "MIT OR Apache-2.0" repository = "https://github.com/lightningdevkit/rust-lightning" diff --git a/lightning-dns-resolver/Cargo.toml b/lightning-dns-resolver/Cargo.toml index 248cb73025a..bdc13f5e41b 100644 --- a/lightning-dns-resolver/Cargo.toml +++ b/lightning-dns-resolver/Cargo.toml @@ -1,6 +1,6 @@ [package] name = "lightning-dns-resolver" -version = "0.3.0" +version = "0.3.1" authors = ["Matt Corallo"] license = "MIT OR Apache-2.0" repository = "https://github.com/lightningdevkit/rust-lightning/" diff --git a/lightning-invoice/Cargo.toml b/lightning-invoice/Cargo.toml index 30bbfa9a3be..ecd1552cfbc 100644 --- a/lightning-invoice/Cargo.toml +++ b/lightning-invoice/Cargo.toml @@ -1,7 +1,7 @@ [package] name = "lightning-invoice" description = "Data structures to parse and serialize BOLT11 lightning invoices" -version = "0.34.0" +version = "0.34.1" authors = ["Sebastian Geisler "] documentation = "https://docs.rs/lightning-invoice/" license = "MIT OR Apache-2.0" diff --git a/lightning-invoice/src/lib.rs b/lightning-invoice/src/lib.rs index 47f929377de..34014594543 100644 --- a/lightning-invoice/src/lib.rs +++ b/lightning-invoice/src/lib.rs @@ -1498,17 +1498,22 @@ impl Bolt11Invoice { self.signed_invoice.features() } - /// Recover the payee's public key (only to be used if none was included in the invoice) + /// Get the invoice's payee public key. + /// + /// This uses the explicitly included payee public key, if present, otherwise it recovers the + /// payee public key from the signature. Prefer [`Self::get_payee_pub_key`] for clarity. pub fn recover_payee_pub_key(&self) -> PublicKey { - self.signed_invoice.recover_payee_pub_key().expect("was checked by constructor").0 + self.get_payee_pub_key() } - /// Recover the payee's public key if one was included in the invoice, otherwise return the - /// recovered public key from the signature + /// Get the invoice's payee public key, preferring an explicitly included payee public key and + /// falling back to recovering the key from the signature. pub fn get_payee_pub_key(&self) -> PublicKey { match self.payee_pub_key() { Some(pk) => *pk, - None => self.recover_payee_pub_key(), + None => { + self.signed_invoice.recover_payee_pub_key().expect("was checked by constructor").0 + }, } } @@ -2044,6 +2049,44 @@ mod test { assert!(new_signed.check_signature()); } + #[test] + fn recover_payee_pub_key_uses_included_payee_pub_key() { + use crate::*; + use bitcoin::secp256k1::ecdsa::{RecoverableSignature, RecoveryId}; + use bitcoin::secp256k1::{PublicKey, Secp256k1, SecretKey}; + use core::time::Duration; + + let secp_ctx = Secp256k1::new(); + let private_key = SecretKey::from_slice(&[42; 32]).unwrap(); + let public_key = PublicKey::from_secret_key(&secp_ctx, &private_key); + + let invoice = InvoiceBuilder::new(Currency::Bitcoin) + .description("Test".to_string()) + .payment_hash(sha256::Hash::from_slice(&[0; 32][..]).unwrap()) + .payment_secret(PaymentSecret([21; 32])) + .payee_pub_key(public_key) + .min_final_cltv_expiry_delta(144) + .duration_since_epoch(Duration::from_secs(1234567)) + .build_signed(|hash| secp_ctx.sign_ecdsa_recoverable(hash, &private_key)) + .unwrap(); + + let signed_raw = invoice.into_signed_raw(); + let (raw_invoice, hash, signature) = signed_raw.into_parts(); + let (_orig_rid, sig_bytes) = signature.0.serialize_compact(); + let bad_rid = RecoveryId::from_i32(2).unwrap(); + let bad_sig = RecoverableSignature::from_compact(&sig_bytes, bad_rid).unwrap(); + let bad_signed_raw = SignedRawBolt11Invoice { + raw_invoice, + hash, + signature: Bolt11InvoiceSignature(bad_sig), + }; + let bad_invoice = Bolt11Invoice::from_signed(bad_signed_raw).unwrap(); + + assert_eq!(bad_invoice.payee_pub_key(), Some(&public_key)); + assert_eq!(bad_invoice.recover_payee_pub_key(), public_key); + assert_eq!(bad_invoice.get_payee_pub_key(), public_key); + } + #[test] fn test_check_feature_bits() { use crate::TaggedField::*; diff --git a/lightning-liquidity/Cargo.toml b/lightning-liquidity/Cargo.toml index a4855957f7a..4d1f8616e6d 100644 --- a/lightning-liquidity/Cargo.toml +++ b/lightning-liquidity/Cargo.toml @@ -1,6 +1,6 @@ [package] name = "lightning-liquidity" -version = "0.2.0" +version = "0.2.3" authors = ["John Cantrell ", "Elias Rohrer "] homepage = "https://lightningdevkit.org/" license = "MIT OR Apache-2.0" diff --git a/lightning-persister/Cargo.toml b/lightning-persister/Cargo.toml index cdd4b3a5086..dfea46d0d91 100644 --- a/lightning-persister/Cargo.toml +++ b/lightning-persister/Cargo.toml @@ -1,6 +1,6 @@ [package] name = "lightning-persister" -version = "0.2.0" +version = "0.2.3" authors = ["Valentine Wallace", "Matt Corallo"] license = "MIT OR Apache-2.0" repository = "https://github.com/lightningdevkit/rust-lightning" diff --git a/lightning-rapid-gossip-sync/Cargo.toml b/lightning-rapid-gossip-sync/Cargo.toml index 695e41a3662..54a73a00f78 100644 --- a/lightning-rapid-gossip-sync/Cargo.toml +++ b/lightning-rapid-gossip-sync/Cargo.toml @@ -1,6 +1,6 @@ [package] name = "lightning-rapid-gossip-sync" -version = "0.2.0" +version = "0.2.3" authors = ["Arik Sosman "] license = "MIT OR Apache-2.0" repository = "https://github.com/lightningdevkit/rust-lightning" diff --git a/lightning-rapid-gossip-sync/src/lib.rs b/lightning-rapid-gossip-sync/src/lib.rs index 429a3560be0..374cffd8330 100644 --- a/lightning-rapid-gossip-sync/src/lib.rs +++ b/lightning-rapid-gossip-sync/src/lib.rs @@ -153,6 +153,10 @@ where /// Sync gossip data from a file. /// Returns the last sync timestamp to be used the next time rapid sync data is queried. /// + /// You should consider the gossip data source as semi-trusted. It is generally the case that it + /// can DoS the client either by omitting data which leads to pathfinding failure or by bloating + /// the graph such that it leads to eventual OOM on the client. + /// /// `network_graph`: The network graph to apply the updates to /// /// `sync_path`: Path to the file where the gossip update data is located @@ -172,6 +176,10 @@ where /// Update network graph from binary data. /// Returns the last sync timestamp to be used the next time rapid sync data is queried. /// + /// You should consider the gossip data source as semi-trusted. It is generally the case that it + /// can DoS the client either by omitting data which leads to pathfinding failure or by bloating + /// the graph such that it leads to eventual OOM on the client. + /// /// `update_data`: `&[u8]` binary stream that comprises the update data #[cfg(feature = "std")] pub fn update_network_graph(&self, update_data: &[u8]) -> Result { @@ -182,6 +190,10 @@ where /// Update network graph from binary data. /// Returns the last sync timestamp to be used the next time rapid sync data is queried. /// + /// You should consider the gossip data source as semi-trusted. It is generally the case that it + /// can DoS the client either by omitting data which leads to pathfinding failure or by bloating + /// the graph such that it leads to eventual OOM on the client. + /// /// `update_data`: `&[u8]` binary stream that comprises the update data /// `current_time_unix`: `Option` optional current timestamp to verify data age pub fn update_network_graph_no_std( diff --git a/lightning-rapid-gossip-sync/src/processing.rs b/lightning-rapid-gossip-sync/src/processing.rs index 8319506b574..ad58c74eb54 100644 --- a/lightning-rapid-gossip-sync/src/processing.rs +++ b/lightning-rapid-gossip-sync/src/processing.rs @@ -9,7 +9,9 @@ use lightning::ln::msgs::{ DecodeError, ErrorAction, LightningError, SocketAddress, UnsignedChannelUpdate, UnsignedNodeAnnouncement, }; -use lightning::routing::gossip::{NetworkGraph, NodeAlias, NodeId}; +use lightning::routing::gossip::{ + NetworkGraph, NodeAlias, NodeId, CHAN_COUNT_ESTIMATE, NODE_COUNT_ESTIMATE, +}; use lightning::util::logger::Logger; use lightning::util::ser::{BigSize, FixedLengthReader, Readable}; use lightning::{log_debug, log_given_level, log_gossip, log_trace, log_warn}; @@ -115,17 +117,27 @@ where } }; + const MAX_NODE_COUNT: u32 = (NODE_COUNT_ESTIMATE as u32) * 10; + const MAX_CHANNEL_COUNT: u64 = (CHAN_COUNT_ESTIMATE as u64) * 10; + let node_id_count: u32 = Readable::read(read_cursor)?; + if node_id_count > MAX_NODE_COUNT { + return Err(LightningError { + err: "RGS data contained nonsense number of nodes to update".to_owned(), + action: ErrorAction::IgnoreError, + } + .into()); + } let mut node_ids: Vec = Vec::with_capacity(core::cmp::min( node_id_count, MAX_INITIAL_NODE_ID_VECTOR_CAPACITY, ) as usize); - let network_graph = &self.network_graph; let mut node_modifications: Vec = Vec::new(); + let read_only_network_graph = network_graph.read_only(); + if parse_node_details { - let read_only_network_graph = network_graph.read_only(); for _ in 0..node_id_count { let mut pubkey_bytes = [0u8; 33]; read_cursor.read_exact(&mut pubkey_bytes)?; @@ -237,9 +249,12 @@ where } } + let original_graph_channel_count = read_only_network_graph.channels().len() as u32; + core::mem::drop(read_only_network_graph); + let mut previous_scid: u64 = 0; let announcement_count: u32 = Readable::read(read_cursor)?; - for _ in 0..announcement_count { + for i in 0..announcement_count { let features = Readable::read(read_cursor)?; // handle SCID @@ -284,6 +299,10 @@ where } } + if (original_graph_channel_count as u64) + (i as u64) > MAX_CHANNEL_COUNT { + continue; + } + let announcement_result = network_graph.add_channel_from_partial_announcement( short_channel_id, funding_sats, @@ -329,6 +348,13 @@ where previous_scid = 0; let update_count: u32 = Readable::read(read_cursor)?; + if update_count as u64 > MAX_CHANNEL_COUNT { + return Err(LightningError { + err: "RGS data contained nonsense number of channels to update".to_owned(), + action: ErrorAction::IgnoreError, + } + .into()); + } log_debug!(self.logger, "Processing RGS update from {} with {} nodes, {} channel announcements and {} channel updates.", latest_seen_timestamp, node_id_count, announcement_count, update_count); if update_count == 0 { diff --git a/lightning-types/Cargo.toml b/lightning-types/Cargo.toml index 32552def61d..588eb151742 100644 --- a/lightning-types/Cargo.toml +++ b/lightning-types/Cargo.toml @@ -1,6 +1,6 @@ [package] name = "lightning-types" -version = "0.3.1" +version = "0.3.2" authors = ["Matt Corallo"] license = "MIT OR Apache-2.0" repository = "https://github.com/lightningdevkit/rust-lightning/" diff --git a/lightning/Cargo.toml b/lightning/Cargo.toml index 2e0ddd389ed..a4a91dfb1eb 100644 --- a/lightning/Cargo.toml +++ b/lightning/Cargo.toml @@ -1,6 +1,6 @@ [package] name = "lightning" -version = "0.2.2" +version = "0.2.3" authors = ["Matt Corallo"] license = "MIT OR Apache-2.0" repository = "https://github.com/lightningdevkit/rust-lightning/" diff --git a/lightning/src/ln/channel.rs b/lightning/src/ln/channel.rs index eb7ab960056..78dd7adf23a 100644 --- a/lightning/src/ln/channel.rs +++ b/lightning/src/ln/channel.rs @@ -1959,9 +1959,12 @@ where let tx_abort = should_ack.then(|| { let logger = WithChannelContext::from(logger, &self.context(), None); - let reason = - types::string::UntrustedString(String::from_utf8_lossy(&msg.data).to_string()); - log_info!(logger, "Counterparty failed interactive transaction negotiation: {reason}"); + let reason = String::from_utf8_lossy(&msg.data); + log_info!( + logger, + "Counterparty failed interactive transaction negotiation: {}", + log_msg!(reason) + ); msgs::TxAbort { channel_id: msg.channel_id, data: "Acknowledged tx_abort".to_string().into_bytes(), diff --git a/lightning/src/ln/invoice_utils.rs b/lightning/src/ln/invoice_utils.rs index 7c0190a23a9..1258e699114 100644 --- a/lightning/src/ln/invoice_utils.rs +++ b/lightning/src/ln/invoice_utils.rs @@ -1288,7 +1288,7 @@ mod test { assert!(!invoice.features().unwrap().supports_basic_mpp()); let payment_params = PaymentParameters::from_node_id( - invoice.recover_payee_pub_key(), + invoice.get_payee_pub_key(), invoice.min_final_cltv_expiry_delta() as u32, ) .with_bolt11_features(invoice.features().unwrap().clone()) @@ -1350,7 +1350,7 @@ mod test { payment_secret, payment_amt, payment_preimage_opt, - invoice.recover_payee_pub_key(), + invoice.get_payee_pub_key(), ); do_claim_payment_along_route(ClaimAlongRouteArgs::new( &nodes[0], diff --git a/lightning/src/ln/peer_handler.rs b/lightning/src/ln/peer_handler.rs index 74f081b03ae..30f8ec35d88 100644 --- a/lightning/src/ln/peer_handler.rs +++ b/lightning/src/ln/peer_handler.rs @@ -45,7 +45,6 @@ use crate::onion_message::packet::OnionMessageContents; use crate::routing::gossip::{NodeAlias, NodeId}; use crate::sign::{NodeSigner, Recipient}; use crate::types::features::{InitFeatures, NodeFeatures}; -use crate::types::string::PrintableString; use crate::util::atomic_counter::AtomicCounter; use crate::util::logger::{Level, Logger, WithContext}; use crate::util::ser::{VecWriter, Writeable, Writer}; @@ -2415,7 +2414,7 @@ where logger, "Got Err message from {}: {}", their_node_id, - PrintableString(&msg.data) + log_msg!(msg.data) ); self.message_handler.chan_handler.handle_error(their_node_id, &msg); if msg.channel_id.is_zero() { @@ -2427,7 +2426,7 @@ where logger, "Got warning message from {}: {}", their_node_id, - PrintableString(&msg.data) + log_msg!(msg.data) ); }, @@ -3213,7 +3212,7 @@ where msgs::ErrorAction::DisconnectPeer { msg } => { if let Some(msg) = msg.as_ref() { log_trace!(logger, "Handling DisconnectPeer HandleError event in peer_handler for node {} with message {}", - node_id, msg.data); + node_id, log_msg!(msg.data)); } else { log_trace!(logger, "Handling DisconnectPeer HandleError event in peer_handler for node {}", node_id); @@ -3228,7 +3227,7 @@ where }, msgs::ErrorAction::DisconnectPeerWithWarning { msg } => { log_trace!(logger, "Handling DisconnectPeer HandleError event in peer_handler for node {} with message {}", - node_id, msg.data); + node_id, log_msg!(msg.data)); // We do not have the peers write lock, so we just store that we're // about to disconnect the peer and do it after we finish // processing most messages. @@ -3254,7 +3253,7 @@ where msgs::ErrorAction::SendErrorMessage { ref msg } => { log_trace!(logger, "Handling SendErrorMessage HandleError event in peer_handler for node {} with message {}", node_id, - msg.data); + log_msg!(msg.data)); self.enqueue_message( &mut *get_peer_for_forwarding!(&node_id)?, msg, @@ -3266,7 +3265,7 @@ where } => { log_given_level!(logger, *log_level, "Handling SendWarningMessage HandleError event in peer_handler for node {} with message {}", node_id, - msg.data); + log_msg!(msg.data)); self.enqueue_message( &mut *get_peer_for_forwarding!(&node_id)?, msg, diff --git a/lightning/src/onion_message/dns_resolution.rs b/lightning/src/onion_message/dns_resolution.rs index 54eb16b5266..c88526c0932 100644 --- a/lightning/src/onion_message/dns_resolution.rs +++ b/lightning/src/onion_message/dns_resolution.rs @@ -520,7 +520,8 @@ impl OMNameResolver { .filter_map(|data| String::from_utf8(data).ok()) .filter(|data_string| data_string.len() > URI_PREFIX.len()) .filter(|data_string| { - data_string[..URI_PREFIX.len()].eq_ignore_ascii_case(URI_PREFIX) + let pfx = &data_string.as_bytes()[..URI_PREFIX.len()]; + pfx.eq_ignore_ascii_case(URI_PREFIX.as_bytes()) }); // Check that there is exactly one TXT record that begins with // bitcoin: as required by BIP 353 (and is valid UTF-8). diff --git a/lightning/src/routing/gossip.rs b/lightning/src/routing/gossip.rs index 80ffbf9fb6c..51a9a59e47f 100644 --- a/lightning/src/routing/gossip.rs +++ b/lightning/src/routing/gossip.rs @@ -1757,14 +1757,16 @@ where } } -// In Jan, 2025 there were about 49K channels. -// We over-allocate by a bit because 20% more is better than the double we get if we're slightly -// too low -const CHAN_COUNT_ESTIMATE: usize = 60_000; -// In Jan, 2025 there were about 15K nodes -// We over-allocate by a bit because 33% more is better than the double we get if we're slightly -// too low -const NODE_COUNT_ESTIMATE: usize = 20_000; +/// In Jan, 2025 there were about 49K channels. +/// +/// We over-allocate by a bit because 20% more is better than the double we get if we're slightly +/// too low +pub const CHAN_COUNT_ESTIMATE: usize = 60_000; +/// In Jan, 2025 there were about 15K nodes +/// +/// We over-allocate by a bit because 33% more is better than the double we get if we're slightly +/// too low +pub const NODE_COUNT_ESTIMATE: usize = 20_000; impl NetworkGraph where diff --git a/lightning/src/routing/router.rs b/lightning/src/routing/router.rs index 8ea3ea068b3..12ba06eff64 100644 --- a/lightning/src/routing/router.rs +++ b/lightning/src/routing/router.rs @@ -980,7 +980,7 @@ impl PaymentParameters { /// [`PaymentParameters::expiry_time`]. pub fn from_bolt11_invoice(invoice: &Bolt11Invoice) -> Self { let mut payment_params = Self::from_node_id( - invoice.recover_payee_pub_key(), + invoice.get_payee_pub_key(), invoice.min_final_cltv_expiry_delta() as u32, ) .with_route_hints(invoice.route_hints()) @@ -2231,10 +2231,12 @@ impl<'a> PaymentPath<'a> { /// contribution this path can make to the final value of the payment. /// May be slightly lower than the actual max due to rounding errors when aggregating fees /// along the path. + /// Returns an error with the index of a later hop to discard if the following hops' aggregate + /// fees overflow. #[rustfmt::skip] fn max_final_value_msat( &self, used_liquidities: &HashMap, channel_saturation_pow_half: u8 - ) -> (usize, u64) { + ) -> Result<(usize, u64), usize> { let mut max_path_contribution = (0, u64::MAX); for (idx, (hop, _)) in self.hops.iter().enumerate() { let hop_effective_capacity_msat = hop.candidate.effective_capacity(); @@ -2250,7 +2252,8 @@ impl<'a> PaymentPath<'a> { // Aggregate the fees of the hops that come after this one, and use those fees to compute the // maximum amount that this hop can contribute to the final value received by the payee. let (next_hops_aggregated_base, next_hops_aggregated_prop) = - crate::blinded_path::payment::compute_aggregated_base_prop_fee(next_hops_feerates_iter).unwrap(); + crate::blinded_path::payment::compute_aggregated_base_prop_fee(next_hops_feerates_iter) + .map_err(|_| idx + 1)?; // floor(((hop_max_msat - agg_base) * 1_000_000) / (1_000_000 + agg_prop)) let hop_max_final_value_contribution = (hop_max_msat as u128) @@ -2267,7 +2270,19 @@ impl<'a> PaymentPath<'a> { } else { debug_assert!(false); } } - max_path_contribution + Ok(max_path_contribution) + } +} + +fn mark_candidate_liquidity_exhausted( + used_liquidities: &mut HashMap, candidate: &CandidateRouteHop, +) { + let exhausted = u64::max_value(); + if let Some(scid) = candidate.short_channel_id() { + *used_liquidities.entry(CandidateHopId::Clear((scid, false))).or_default() = exhausted; + *used_liquidities.entry(CandidateHopId::Clear((scid, true))).or_default() = exhausted; + } else { + *used_liquidities.entry(candidate.id()).or_default() = exhausted; } } @@ -3448,7 +3463,17 @@ where L::Target: Logger { // underpaid htlc_minimum_msat with fees. debug_assert_eq!(payment_path.get_value_msat(), value_contribution_msat); let (lowest_value_contrib_hop, max_path_contribution_msat) = - payment_path.max_final_value_msat(&used_liquidities, channel_saturation_pow_half); + match payment_path.max_final_value_msat(&used_liquidities, channel_saturation_pow_half) { + Ok(contribution) => contribution, + Err(candidate_idx_to_skip) => { + let candidate = &payment_path.hops[candidate_idx_to_skip].0.candidate; + log_trace!(logger, + "Ignoring path because aggregate fees including hop {} overflow.", + LoggedCandidateHop(candidate)); + mark_candidate_liquidity_exhausted(&mut used_liquidities, candidate); + continue 'paths_collection; + } + }; let desired_value_contribution = cmp::min(max_path_contribution_msat, final_value_msat); value_contribution_msat = payment_path.update_value_and_recompute_fees(desired_value_contribution); @@ -3922,8 +3947,10 @@ mod tests { use crate::util::test_utils as ln_test_utils; use bitcoin::amount::Amount; + use bitcoin::bech32::primitives::decode::CheckedHrpstring; + use bitcoin::bech32::{ByteIterExt, Fe32IterExt}; use bitcoin::constants::ChainHash; - use bitcoin::hashes::Hash; + use bitcoin::hashes::{Hash, sha256::Hash as Sha256}; use bitcoin::hex::FromHex; use bitcoin::network::Network; use bitcoin::opcodes; @@ -3932,9 +3959,58 @@ mod tests { use bitcoin::secp256k1::{PublicKey, SecretKey}; use bitcoin::transaction::TxOut; + use lightning_invoice::{Bolt11Bech32, Bolt11Invoice, Currency, InvoiceBuilder}; + use crate::io::Cursor; use crate::prelude::*; use crate::sync::Arc; + use crate::types::payment::PaymentSecret; + + fn invoice_with_included_payee_pub_key_and_bad_recovery_id() -> (Bolt11Invoice, PublicKey) { + let secp_ctx = Secp256k1::new(); + let private_key = SecretKey::from_slice(&[42; 32]).unwrap(); + let public_key = PublicKey::from_secret_key(&secp_ctx, &private_key); + + let invoice = InvoiceBuilder::new(Currency::Bitcoin) + .description("Test".to_string()) + .amount_milli_satoshis(1000) + .payment_hash(Sha256::from_slice(&[0; 32][..]).unwrap()) + .payment_secret(PaymentSecret([21; 32])) + .payee_pub_key(public_key) + .min_final_cltv_expiry_delta(144) + .duration_since_epoch(core::time::Duration::from_secs(1234567)) + .build_signed(|hash| secp_ctx.sign_ecdsa_recoverable(hash, &private_key)) + .unwrap(); + + let invoice_string = invoice.to_string(); + let parsed = CheckedHrpstring::new::(&invoice_string).unwrap(); + let hrp = parsed.hrp(); + let mut data: Vec<_> = parsed.fe32_iter::<&mut dyn Iterator>().collect(); + let signature_start = data.len() - 104; + let mut signature_bytes: Vec = + data[signature_start..].iter().copied().fes_to_bytes().collect(); + signature_bytes[64] = 2; + let signature_data: Vec<_> = signature_bytes.into_iter().bytes_to_fes().collect(); + data.splice(signature_start.., signature_data); + + let bad_invoice_string = data + .into_iter() + .with_checksum::(&hrp) + .chars() + .collect::(); + (bad_invoice_string.parse().unwrap(), public_key) + } + + #[test] + fn payment_params_from_bolt11_invoice_uses_included_payee_pub_key() { + let (invoice, public_key) = invoice_with_included_payee_pub_key_and_bad_recovery_id(); + let payment_params = PaymentParameters::from_bolt11_invoice(&invoice); + + match payment_params.payee { + super::Payee::Clear { node_id, .. } => assert_eq!(node_id, public_key), + super::Payee::Blinded { .. } => panic!("BOLT11 invoice should create a clear payee"), + } + } #[rustfmt::skip] fn get_channel_details(short_channel_id: Option, node_id: PublicKey, @@ -9044,6 +9120,67 @@ mod tests { assert_eq!(route.paths[0].hops[0].short_channel_id, 44); } + #[test] + fn aggregated_prop_fee_overflow_fails_route() { + // If the fee cap is disabled, we may consider invoice hints with very large + // proportional fees. Aggregating those fees can overflow, in which case we should fail + // routing cleanly rather than panic. + let secp_ctx = Secp256k1::new(); + let logger = Arc::new(ln_test_utils::TestLogger::new()); + let network_graph = Arc::new(NetworkGraph::new(Network::Testnet, Arc::clone(&logger))); + let scorer = ln_test_utils::TestScorer::new(); + let random_seed_bytes = [42; 32]; + let config = UserConfig::default(); + + let (_, our_node_id, _, nodes) = get_nodes(&secp_ctx); + let route_hint = RouteHint(vec![ + RouteHintHop { + src_node_id: nodes[0], + short_channel_id: 100, + fees: RoutingFees { base_msat: 0, proportional_millionths: u32::MAX }, + cltv_expiry_delta: 10, + htlc_minimum_msat: None, + htlc_maximum_msat: None, + }, + RouteHintHop { + src_node_id: nodes[1], + short_channel_id: 101, + fees: RoutingFees { base_msat: 0, proportional_millionths: u32::MAX }, + cltv_expiry_delta: 10, + htlc_minimum_msat: None, + htlc_maximum_msat: None, + }, + ]); + + let payment_params = PaymentParameters::from_node_id(nodes[2], 42) + .with_route_hints(vec![route_hint]) + .unwrap() + .with_bolt11_features(channelmanager::provided_bolt11_invoice_features(&config)) + .unwrap(); + let first_hops = [get_channel_details( + Some(1), + nodes[0], + channelmanager::provided_init_features(&config), + 100_000_000, + )]; + let route_params = RouteParameters { + payment_params, + final_value_msat: 1, + max_total_routing_fee_msat: None, + }; + let route = get_route( + &our_node_id, + &route_params, + &network_graph.read_only(), + Some(&first_hops.iter().collect::>()), + Arc::clone(&logger), + &scorer, + &Default::default(), + &random_seed_bytes, + ); + assert!(route.is_err()); + } + #[test] fn prefers_paths_by_cost_amt_ratio() { // Previously, we preferred paths during MPP selection based on their absolute cost, rather diff --git a/lightning/src/util/macro_logger.rs b/lightning/src/util/macro_logger.rs index ec9eb14ba38..fac68f19c59 100644 --- a/lightning/src/util/macro_logger.rs +++ b/lightning/src/util/macro_logger.rs @@ -169,6 +169,33 @@ macro_rules! log_spendable { }; } +/// The maximum number of characters to display in a network message log entry. +pub(crate) const LOG_MSG_MAX_LEN: usize = 512; + +/// Wraps a string slice for Display, truncating to [`LOG_MSG_MAX_LEN`] characters and +/// delegating sanitization to [`crate::types::string::PrintableString`]. +/// Useful for logging counterparty-provided messages. +pub(crate) struct DebugMsg<'a>(pub &'a str); +impl<'a> core::fmt::Display for DebugMsg<'a> { + fn fmt(&self, f: &mut core::fmt::Formatter) -> Result<(), core::fmt::Error> { + let (msg, was_truncated) = match self.0.char_indices().nth(LOG_MSG_MAX_LEN) { + Some((idx, _)) => (&self.0[..idx], true), + None => (self.0, false), + }; + core::fmt::Display::fmt(&crate::types::string::PrintableString(msg), f)?; + if was_truncated { + f.write_str("...")?; + } + Ok(()) + } +} + +macro_rules! log_msg { + ($obj: expr) => { + $crate::util::macro_logger::DebugMsg(&$obj) + }; +} + /// Create a new Record and log it. You probably don't want to use this macro directly, /// but it needs to be exported so `log_trace` etc can use it in external crates. #[doc(hidden)] @@ -226,3 +253,61 @@ macro_rules! log_gossip { $crate::log_given_level!($logger, $crate::util::logger::Level::Gossip, $($arg)*); ) } + +#[cfg(test)] +mod tests { + use super::*; + use alloc::string::ToString; + + #[test] + fn debug_msg_short_string() { + let s = "hello world"; + assert_eq!(DebugMsg(s).to_string(), "hello world"); + } + + #[test] + fn debug_msg_truncates_at_limit() { + let s: String = core::iter::repeat('a').take(LOG_MSG_MAX_LEN + 100).collect(); + let result = DebugMsg(&s).to_string(); + // Should be exactly LOG_MSG_MAX_LEN 'a's followed by "..." + assert_eq!(result.len(), LOG_MSG_MAX_LEN + 3); + assert!(result.ends_with("...")); + } + + #[test] + fn debug_msg_no_truncation_at_exact_limit() { + let s: String = core::iter::repeat('a').take(LOG_MSG_MAX_LEN).collect(); + let result = DebugMsg(&s).to_string(); + assert_eq!(result.len(), LOG_MSG_MAX_LEN); + assert!(!result.ends_with("...")); + } + + #[test] + fn debug_msg_replaces_control_characters() { + let s = "hello\x00world\nfoo"; + let result = DebugMsg(s).to_string(); + assert_eq!(result, "hello\u{FFFD}world\u{FFFD}foo"); + } + + #[test] + fn debug_msg_uses_printable_string_sanitization() { + let s = "safe\u{202E}cipsxe.exe"; + assert_eq!(DebugMsg(s).to_string(), crate::types::string::PrintableString(s).to_string()); + } + + #[test] + fn debug_msg_multibyte_unicode() { + // Each emoji is multiple bytes but one character + let s: String = core::iter::repeat('\u{1F600}').take(LOG_MSG_MAX_LEN + 10).collect(); + let result = DebugMsg(&s).to_string(); + let char_count: usize = result.chars().count(); + // LOG_MSG_MAX_LEN emoji chars + 3 chars for "..." + assert_eq!(char_count, LOG_MSG_MAX_LEN + 3); + assert!(result.ends_with("...")); + } + + #[test] + fn debug_msg_empty_string() { + assert_eq!(DebugMsg("").to_string(), ""); + } +} diff --git a/lightning/src/util/ser.rs b/lightning/src/util/ser.rs index f821aa5afc0..92b1e224ac6 100644 --- a/lightning/src/util/ser.rs +++ b/lightning/src/util/ser.rs @@ -925,7 +925,9 @@ macro_rules! impl_for_map { #[inline] fn read(r: &mut R) -> Result { let len: CollectionLength = Readable::read(r)?; - let mut ret = $constr(len.0 as usize); + let entry_size = ::core::mem::size_of::() + ::core::mem::size_of::(); + let max_alloc = MAX_BUF_SIZE / (entry_size + 1); + let mut ret = $constr(cmp::min(len.0 as usize, max_alloc)); for _ in 0..len.0 { let k = K::read(r)?; let v_opt = V::read(r)?; diff --git a/possiblyrandom/Cargo.toml b/possiblyrandom/Cargo.toml index 4508f690129..91c0f201238 100644 --- a/possiblyrandom/Cargo.toml +++ b/possiblyrandom/Cargo.toml @@ -1,6 +1,6 @@ [package] name = "possiblyrandom" -version = "0.2.0" +version = "0.2.1" authors = ["Matt Corallo"] license = "MIT OR Apache-2.0" repository = "https://github.com/lightningdevkit/rust-lightning/" diff --git a/possiblyrandom/src/lib.rs b/possiblyrandom/src/lib.rs index 9cbbad7f13d..6ddbc6de1a2 100644 --- a/possiblyrandom/src/lib.rs +++ b/possiblyrandom/src/lib.rs @@ -20,16 +20,19 @@ #![no_std] -#[cfg(feature = "getrandom")] +#[cfg(any( + feature = "getrandom", + not(any(target_os = "unknown", target_os = "none")) +))] extern crate getrandom; /// Possibly fills `dest` with random data. May fill it with zeros. #[inline] pub fn getpossiblyrandom(dest: &mut [u8]) { - #[cfg(feature = "getrandom")] - if getrandom::getrandom(dest).is_err() { - dest.fill(0); - } - #[cfg(not(feature = "getrandom"))] dest.fill(0); + #[cfg(any( + feature = "getrandom", + not(any(target_os = "unknown", target_os = "none")) + ))] + let _ = getrandom::getrandom(dest); }