Don't panic when a composite sub-handler returns `Ok(None)`
What changed, and why it matters
This commit fixes a bug where a remote peer could crash a Lightning node by sending a specially chosen custom message. The crash happened because a message-routing helper assumed a sub-component would always recognize any message type matching its declared pattern, but sub-components can legitimately decline to decode some types within their pattern. The fix replaces an internal 'this should never happen' crash with a graceful 'unknown message' response.
Treat this as a security fix worth backporting to maintained release branches. Nodes using custom message handlers built with the composite macro should upgrade, as a peer can crash them with a single malformed or reserved custom message type. No immediate on-chain risk is indicated, but denial-of-service against the message-processing thread is plausible.
Security signals we found
Remote-triggered panic (denial of service) in message-processing thread
Violation of `CustomMessageReader` contract assumption in composite handler
Peer-controlled input (`message_type`) used as index/pattern match without graceful fallback
Fix aligns behavior with existing `wire::do_read` custom-message handling
Evidence from the diff
In rust-lightning’s lightning-custom-message crate, the composite_custom_message_handler! macro generated a read implementation that called unreachable!() when a sub-handler’s CustomMessageReader::read returned Ok(None). Because the composite’s match pattern (especially a numeric range) can be broader than the exact set of types the sub-handler decodes, and because message_type is derived from peer input, a remote peer could trigger a panic in the message-processing thread. The patch changes the None arm to return Ok(None), treating the message as unknown, consistent with wire::do_read. A regression test demonstrates a handler reserving types 32768..=32777 but only decoding 32768, verifying that 32770 no longer panics.
Changed components
lightning-custom-message/src/lib.rscomposite_custom_message_handler! macroCustomMessageReader/CustomMessageHandler composite read pathInspect captured patch +71 / −1
diff --git a/lightning-custom-message/src/lib.rs b/lightning-custom-message/src/lib.rs
index 0d70ba0..06e57b4 100644
--- a/lightning-custom-message/src/lib.rs
+++ b/lightning-custom-message/src/lib.rs
@@ -358,7 +358,12 @@ macro_rules! composite_custom_message_handler {
match message_type {
$(
$pattern => match <$type>::read(&self.$field, message_type, buffer)? {
- None => unreachable!(),
+ // A sub-handler returns `None` for a `message_type` it doesn't
+ // recognize. The composite's pattern can be broader than the types
+ // the sub-handler decodes (e.g. a range), and `message_type` is
+ // peer-provided, so report the message as unknown rather than
+ // treating this as unreachable and panicking.
+ None => Ok(None),
Some(message) => Ok(Some($message::$variant(message))),
},
)*
@@ -501,6 +506,71 @@ mod tests {
}
);
+ struct ReservedBlockHandler;
+ impl CustomMessageReader for ReservedBlockHandler {
+ type CustomMessage = Foo;
+ fn read<R: LengthLimitedRead>(
+ &self, message_type: u16, _b: &mut R,
+ ) -> Result<Option<Foo>, DecodeError> {
+ // This build defines only the message at 32768; the rest of the block its
+ // protocol reserved (32768..=32777) is for types future versions may add.
+ // A not-yet-defined type is unknown to this build, so per the
+ // `CustomMessageReader` contract it returns `Ok(None)` -- a newer peer can
+ // send one and this older node will treat it as an unknown message.
+ match message_type {
+ 32768 => Ok(Some(Foo)),
+ _ => Ok(None),
+ }
+ }
+ }
+ impl CustomMessageHandler for ReservedBlockHandler {
+ fn handle_custom_message(&self, _msg: Foo, _: PublicKey) -> Result<(), LightningError> {
+ Ok(())
+ }
+ fn get_and_clear_pending_msg(&self) -> Vec<(PublicKey, Foo)> {
+ vec![]
+ }
+ fn peer_disconnected(&self, _: PublicKey) {}
+ fn peer_connected(&self, _: PublicKey, _: &Init, _: bool) -> Result<(), ()> {
+ Ok(())
+ }
+ fn provided_node_features(&self) -> NodeFeatures {
+ NodeFeatures::empty()
+ }
+ fn provided_init_features(&self, _: PublicKey) -> InitFeatures {
+ InitFeatures::empty()
+ }
+ }
+
+ composite_custom_message_handler!(
+ struct ReservedBlockComposite {
+ proto: ReservedBlockHandler,
+ }
+
+ enum ReservedBlockMessage {
+ Proto(32768..=32777),
+ }
+ );
+
+ #[test]
+ fn read_treats_a_reserved_in_range_type_as_unknown() {
+ // A sub-handler may own a block of type ids (declared here as a range) yet only
+ // decode the subset its build defines, returning `Ok(None)` for reserved or
+ // not-yet-defined types in the block -- exactly what a node does on receiving a
+ // newer peer's message. `read` must surface that as an unknown message, not
+ // panic.
+ let composite = ReservedBlockComposite { proto: ReservedBlockHandler };
+ let mut buffer: &[u8] = &[];
+ // The message this build defines decodes to its variant.
+ assert!(matches!(
+ composite.read(32768, &mut buffer),
+ Ok(Some(ReservedBlockMessage::Proto(_)))
+ ));
+ // A reserved type from the same block is reported unknown, not panicked
+ // (pre-fix the matched arm hit `unreachable!()`).
+ assert!(matches!(composite.read(32770, &mut buffer), Ok(None)));
+ }
+
#[test]
fn peer_connected_failure_does_not_leak_subhandler_state() {
let composite = CompositeHandler {
Why this scored 76/100
Community notes
Notes can correct, qualify, or add evidence to the AI analysis. Every note shown here has been validated by a human moderator.
The AI analysis stands alone for now. Submit a note if you can add evidence or important context.