Peer storage can send more than wire limit size

[TheBlueMatt]

Copied verbatim from project loupe. Note that this is behind the peer_storage cfg flag so not currently reachable, but needs fixing.

Finding

• repo: lightningdevkit/rust-lightning (https://github.com/lightningdevkit/rust-lightning.git)
• reviewed revision: 5049f7c0277ad6dc53c75b37833dcf7714984abf
• title: Peer storage size cap ignores encryption overhead, can exceed wire limit and panic
• severity: medium
• location: lightning/src/chain/chainmonitor.rs:988-1063
• cwe: CWE-131
• scanner: llm-code-review
• fingerprint: ccd2318909fb0c5040f0faf9d28c7ffc652a525babde631eb741a5533d54a2e4

Description

ChainMonitor::send_peer_storage (cfg peer_storage) accumulates PeerStorageMonitorHolders, enforcing MAX_PEER_STORAGE_SIZE = 65531 against current_size — the plaintext serialized length (chainmonitor.rs:988, 1045). But the bytes placed on the wire are the encrypted blob: monitors_list.encode() adds a 2-byte CollectionLength prefix, and DecryptedOurPeerStorage::encrypt appends a 16-byte ChaCha20Poly1305 tag plus a 32-byte nonce seed (our_peer_storage.rs) — 50 bytes of overhead that are never counted. So PeerStorage.data (line 1060) can reach ~65581 bytes, exceeding BOLT #1's 65531-byte peer_storage maximum (limit confirmed via the bkb BOLT #1 lookup in a prior session). Worse, once the encoded message crosses BOLT #8's LN_MAX_MSG_LEN = 65535, the transport hits an unconditional panic!("Attempted to encrypt message longer than 65535 bytes!") (peer_channel_encryptor.rs:520-521, 647-648), crashing the node — a self-inflicted DoS that fires whenever a node accrues ~65KB of monitor data and send_peer_storage runs (per peer, on block updates).

PoC: a regression test binary-searches a destination_script length so one monitor's serialized length sits just under the plaintext cap, watches it, calls send_peer_storage, and asserts the emitted msg.data.len() <= 65531. It fails on HEAD (blob = len + 50) and passes once the cap accounts for the overhead (excluding the over-large monitor). Caveats: gated behind the experimental --cfg peer_storage; no Rust toolchain in env, so not compile/run-verified. Queried prior findings ("peer_storage blob size...", "send_peer_storage MAX_PEER_STORAGE_SIZE...") — no matches.

Proof of Concept

--- a/lightning/src/chain/channelmonitor.rs
+++ b/lightning/src/chain/channelmonitor.rs
@@ -6973,6 +6973,16 @@
 pub(super) fn dummy_monitor<S: EcdsaChannelSigner + 'static>(
 	channel_id: ChannelId, wrap_signer: impl FnOnce(crate::sign::InMemorySigner) -> S,
 ) -> ChannelMonitor<S> {
+	dummy_monitor_with_destination_script(channel_id, ScriptBuf::new(), wrap_signer)
+}
+
+/// Like [`dummy_monitor`] but allows setting an arbitrary `destination_script`. This is useful for
+/// tests that need to control the serialized size of the resulting [`ChannelMonitor`].
+#[cfg(test)]
+pub(super) fn dummy_monitor_with_destination_script<S: EcdsaChannelSigner + 'static>(
+	channel_id: ChannelId, destination_script: ScriptBuf,
+	wrap_signer: impl FnOnce(crate::sign::InMemorySigner) -> S,
+) -> ChannelMonitor<S> {
 	use crate::ln::chan_utils::{ChannelPublicKeys, CounterpartyChannelTransactionParameters};
 	use crate::sign::{ChannelSigner, InMemorySigner};
 	use bitcoin::network::Network;
@@ -7022,7 +7032,7 @@
 		signer,
 		Some(shutdown_script.into_inner()),
 		0,
-		&ScriptBuf::new(),
+		&destination_script,
 		&channel_parameters,
 		true,
 		0,
--- a/lightning/src/chain/chainmonitor.rs
+++ b/lightning/src/chain/chainmonitor.rs
@@ -2215,4 +2215,119 @@
 		assert_eq!(chain_monitor_a.pending_operation_count(), 0);
 		assert_eq!(chain_monitor_b.pending_operation_count(), 0);
 	}
+
+	/// Regression test for peer-storage blob size accounting.
+	///
+	/// `send_peer_storage` caps the *plaintext* payload at `MAX_PEER_STORAGE_SIZE` (65531), but the
+	/// message put on the wire is the *encrypted* blob, which is 50 bytes larger: a 2-byte vec-length
+	/// prefix plus a 16-byte AEAD tag and a 32-byte nonce seed appended during encryption. As a
+	/// result the emitted `PeerStorage` message can exceed BOLT #1's 65531-byte limit (and even
+	/// BOLT #8's 65535-byte transport limit, which makes the outbound message un-encryptable and
+	/// panics the node).
+	#[test]
+	#[cfg(peer_storage)]
+	fn test_peer_storage_blob_within_size_limit() {
+		use crate::chain::channelmonitor::{
+			dummy_monitor_with_destination_script, write_chanmon_internal,
+		};
+		use crate::ln::our_peer_storage::PeerStorageMonitorHolder;
+		use crate::util::ser::{VecWriter, Writeable};
+		use bitcoin::script::ScriptBuf;
+
+		const MAX_PEER_STORAGE_SIZE: usize = 65531;
+		// 2-byte `CollectionLength` vec prefix + 16-byte AEAD tag + 32-byte nonce seed.
+		const OVERHEAD: usize = 2 + 16 + 32;
+
+		let broadcaster = TestBroadcaster::new(Network::Testnet);
+		let fee_est = TestFeeEstimator::new(253);
+		let logger = TestLogger::new();
+		let persister = TestPersister::new();
+		let chain_source = TestChainSource::new(Network::Testnet);
+		let keys = TestKeysInterface::new(&[0; 32], Network::Testnet);
+
+		// Non-deferred so `watch_channel` inserts into the monitor set synchronously.
+		let chain_monitor = ChainMonitor::new(
+			Some(&chain_source),
+			&broadcaster,
+			&logger,
+			&fee_est,
+			&persister,
+			&keys,
+			keys.get_peer_storage_key(),
+			false,
+		);
+
+		let chan = ChannelId::from_bytes([7u8; 32]);
+
+		// Computes the exact per-monitor `serialized_length` that `send_peer_storage` accounts for,
+		// for a `dummy_monitor` whose `destination_script` is `script_len` bytes. This mirrors
+		// `send_peer_storage`'s accounting but is independent of its (buggy) size-capping logic.
+		let holder_len = |script_len: usize| -> usize {
+			let monitor = dummy_monitor_with_destination_script(
+				chan,
+				ScriptBuf::from(vec![0u8; script_len]),
+				|keys| TestChannelSigner::new(DynSigner::new(keys)),
+			);
+			let mut serialized_channel = VecWriter(Vec::new());
+			{
+				let inner_lock = monitor.inner.lock().unwrap();
+				write_chanmon_internal(&inner_lock, true, &mut serialized_channel).unwrap();
+			}
+			let holder = PeerStorageMonitorHolder {
+				channel_id: chan,
+				min_seen_secret: monitor.get_min_seen_secret(),
+				counterparty_node_id: monitor.get_counterparty_node_id(),
+				monitor_bytes: serialized_channel.0,
+			};
+			holder.serialized_length()
+		};
+
+		// Binary-search the largest `destination_script` length whose monitor still fits under the
+		// plaintext cap (so `send_peer_storage` includes it). `serialized_length` is monotonic in
+		// `script_len`, so this lands the single monitor's serialized length just below the cap.
+		let (mut lo, mut hi) = (0usize, 65500usize);
+		assert!(holder_len(hi) > MAX_PEER_STORAGE_SIZE, "search upper bound too low");
+		while lo < hi {
+			let mid = (lo + hi + 1) / 2;
+			if holder_len(mid) <= MAX_PEER_STORAGE_SIZE {
+				lo = mid;
+			} else {
+				hi = mid - 1;
+			}
+		}
+		let script_len = lo;
+		let monitor_len = holder_len(script_len);
+		// The monitor is includable (<= cap) yet large enough that the encryption/framing overhead
+		// pushes the blob over the limit.
+		assert!(
+			monitor_len <= MAX_PEER_STORAGE_SIZE && monitor_len + OVERHEAD > MAX_PEER_STORAGE_SIZE,
+			"calibration failed: monitor serialized length {} not in expected window",
+			monitor_len,
+		);
+
+		let monitor = dummy_monitor_with_destination_script(
+			chan,
+			ScriptBuf::from(vec![0u8; script_len]),
+			|keys| TestChannelSigner::new(DynSigner::new(keys)),
+		);
+		let counterparty_node_id = monitor.get_counterparty_node_id();
+		assert!(Watch::watch_channel(&chain_monitor, chan, monitor).is_ok());
+
+		chain_monitor.send_peer_storage(counterparty_node_id);
+
+		let msg_events = chain_monitor.get_and_clear_pending_msg_events();
+		let mut saw_peer_storage = false;
+		for ev in msg_events {
+			if let MessageSendEvent::SendPeerStorage { msg, .. } = ev {
+				saw_peer_storage = true;
+				assert!(
+					msg.data.len() <= MAX_PEER_STORAGE_SIZE,
+					"peer_storage blob length {} exceeds BOLT #1 limit of {}",
+					msg.data.len(),
+					MAX_PEER_STORAGE_SIZE,
+				);
+			}
+		}
+		assert!(saw_peer_storage, "expected a SendPeerStorage message");
+	}
 }

This finding was discovered by Project Loupe.