Cut 0.2.3/dns-resolver 0.3.1/invoice 0.34.1/types 0.3.2

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.
 

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 <vwallace@protonmail.com>"]
 license = "MIT OR Apache-2.0"
 repository = "https://github.com/lightningdevkit/rust-lightning"

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"

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/"

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 <sgeisler@wh2.tu-dresden.de>"]
 documentation = "https://docs.rs/lightning-invoice/"
 license = "MIT OR Apache-2.0"

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::*;

lightning-liquidity/Cargo.toml

@@ -1,6 +1,6 @@
 [package]
 name = "lightning-liquidity"
-version = "0.2.0"
+version = "0.2.3"
 authors = ["John Cantrell <johncantrell97@gmail.com>", "Elias Rohrer <dev@tnull.de>"]
 homepage = "https://lightningdevkit.org/"
 license = "MIT OR Apache-2.0"

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"

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 <git@arik.io>"]
 license = "MIT OR Apache-2.0"
 repository = "https://github.com/lightningdevkit/rust-lightning"

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<u32, GraphSyncError> {
@@ -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<u64>` optional current timestamp to verify data age
 	pub fn update_network_graph_no_std(

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<NodeId> = 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<UnsignedNodeAnnouncement> = 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 {

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/"

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/"

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(),

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],

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,

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).