Require length limited reader for LN gossip messages

Continuing the work of the last few commits, here we require that some LN
gossip messages be read from a length limiting reader.

The messages' updates are separated into their own commit because they
definitely should use a length limited reader but they also require breaking
out of the ser macros. We can't use the macros because each message contains a
non-TLV field that will always consume the rest of the reader, while the ser
macros only support reading non-TLV fields that know when to stop reading.

Author: valentinewallace

fuzz/src/msg_targets/utils.rs

@@ -71,12 +71,13 @@ macro_rules! test_msg_simple {
 #[macro_export]
 macro_rules! test_msg_exact {
 	($MsgType: path, $data: ident) => {{
-		use lightning::util::ser::{Readable, Writeable};
-		let mut r = ::lightning::io::Cursor::new($data);
-		if let Ok(msg) = <$MsgType as Readable>::read(&mut r) {
+		use lightning::util::ser::{LengthReadable, Writeable};
+		if let Ok(msg) =
+			<$MsgType as LengthReadable>::read_from_fixed_length_buffer(&mut &$data[..])
+		{
 			let mut w = VecWriter(Vec::new());
 			msg.write(&mut w).unwrap();
-			assert_eq!(&r.into_inner()[..], &w.0[..]);
+			assert_eq!(&$data[..], &w.0[..]);
 			assert_eq!(msg.serialized_length(), w.0.len());
 		}
 	}};

fuzz/src/router.rs

@@ -30,7 +30,7 @@ use lightning::routing::utxo::{UtxoFuture, UtxoLookup, UtxoLookupError, UtxoResu
 use lightning::types::features::{BlindedHopFeatures, Bolt12InvoiceFeatures};
 use lightning::util::config::UserConfig;
 use lightning::util::hash_tables::*;
-use lightning::util::ser::Readable;
+use lightning::util::ser::LengthReadable;
 
 use bitcoin::hashes::Hash;
 use bitcoin::network::Network;
@@ -146,10 +146,11 @@ pub fn do_test<Out: test_logger::Output>(data: &[u8], out: Out) {
 
 	macro_rules! decode_msg {
 		($MsgType: path, $len: expr) => {{
-			let mut reader = ::lightning::io::Cursor::new(get_slice!($len));
-			match <$MsgType>::read(&mut reader) {
+			let data = get_slice!($len);
+			let mut reader = &data[..];
+			match <$MsgType>::read_from_fixed_length_buffer(&mut reader) {
 				Ok(msg) => {
-					assert_eq!(reader.position(), $len as u64);
+					assert_eq!(reader.len(), $len);
 					msg
 				},
 				Err(e) => match e {

lightning/src/ln/msgs.rs

@@ -3572,8 +3572,8 @@ impl Writeable for UnsignedChannelAnnouncement {
 	}
 }
 
-impl Readable for UnsignedChannelAnnouncement {
-	fn read<R: Read>(r: &mut R) -> Result<Self, DecodeError> {
+impl LengthReadable for UnsignedChannelAnnouncement {
+	fn read_from_fixed_length_buffer<R: LengthLimitedRead>(r: &mut R) -> Result<Self, DecodeError> {
 		Ok(Self {
 			features: Readable::read(r)?,
 			chain_hash: Readable::read(r)?,
@@ -3587,13 +3587,28 @@ impl Readable for UnsignedChannelAnnouncement {
 	}
 }
 
-impl_writeable!(ChannelAnnouncement, {
-	node_signature_1,
-	node_signature_2,
-	bitcoin_signature_1,
-	bitcoin_signature_2,
-	contents
-});
+impl Writeable for ChannelAnnouncement {
+	fn write<W: Writer>(&self, w: &mut W) -> Result<(), io::Error> {
+		self.node_signature_1.write(w)?;
+		self.node_signature_2.write(w)?;
+		self.bitcoin_signature_1.write(w)?;
+		self.bitcoin_signature_2.write(w)?;
+		self.contents.write(w)?;
+		Ok(())
+	}
+}
+
+impl LengthReadable for ChannelAnnouncement {
+	fn read_from_fixed_length_buffer<R: LengthLimitedRead>(r: &mut R) -> Result<Self, DecodeError> {
+		Ok(Self {
+			node_signature_1: Readable::read(r)?,
+			node_signature_2: Readable::read(r)?,
+			bitcoin_signature_1: Readable::read(r)?,
+			bitcoin_signature_2: Readable::read(r)?,
+			contents: LengthReadable::read_from_fixed_length_buffer(r)?,
+		})
+	}
+}
 
 impl Writeable for UnsignedChannelUpdate {
 	fn write<W: Writer>(&self, w: &mut W) -> Result<(), io::Error> {
@@ -3614,8 +3629,8 @@ impl Writeable for UnsignedChannelUpdate {
 	}
 }
 
-impl Readable for UnsignedChannelUpdate {
-	fn read<R: Read>(r: &mut R) -> Result<Self, DecodeError> {
+impl LengthReadable for UnsignedChannelUpdate {
+	fn read_from_fixed_length_buffer<R: LengthLimitedRead>(r: &mut R) -> Result<Self, DecodeError> {
 		let res = Self {
 			chain_hash: Readable::read(r)?,
 			short_channel_id: Readable::read(r)?,
@@ -3639,10 +3654,22 @@ impl Readable for UnsignedChannelUpdate {
 	}
 }
 
-impl_writeable!(ChannelUpdate, {
-	signature,
-	contents
-});
+impl Writeable for ChannelUpdate {
+	fn write<W: Writer>(&self, w: &mut W) -> Result<(), io::Error> {
+		self.signature.write(w)?;
+		self.contents.write(w)?;
+		Ok(())
+	}
+}
+
+impl LengthReadable for ChannelUpdate {
+	fn read_from_fixed_length_buffer<R: LengthLimitedRead>(r: &mut R) -> Result<Self, DecodeError> {
+		Ok(Self {
+			signature: Readable::read(r)?,
+			contents: LengthReadable::read_from_fixed_length_buffer(r)?,
+		})
+	}
+}
 
 impl Writeable for ErrorMessage {
 	fn write<W: Writer>(&self, w: &mut W) -> Result<(), io::Error> {
@@ -3720,8 +3747,8 @@ impl Writeable for UnsignedNodeAnnouncement {
 	}
 }
 
-impl Readable for UnsignedNodeAnnouncement {
-	fn read<R: Read>(r: &mut R) -> Result<Self, DecodeError> {
+impl LengthReadable for UnsignedNodeAnnouncement {
+	fn read_from_fixed_length_buffer<R: LengthLimitedRead>(r: &mut R) -> Result<Self, DecodeError> {
 		let features: NodeFeatures = Readable::read(r)?;
 		let timestamp: u32 = Readable::read(r)?;
 		let node_id: NodeId = Readable::read(r)?;
@@ -3782,10 +3809,22 @@ impl Readable for UnsignedNodeAnnouncement {
 	}
 }
 
-impl_writeable!(NodeAnnouncement, {
-	signature,
-	contents
-});
+impl Writeable for NodeAnnouncement {
+	fn write<W: Writer>(&self, w: &mut W) -> Result<(), io::Error> {
+		self.signature.write(w)?;
+		self.contents.write(w)?;
+		Ok(())
+	}
+}
+
+impl LengthReadable for NodeAnnouncement {
+	fn read_from_fixed_length_buffer<R: LengthLimitedRead>(r: &mut R) -> Result<Self, DecodeError> {
+		Ok(Self {
+			signature: Readable::read(r)?,
+			contents: LengthReadable::read_from_fixed_length_buffer(r)?,
+		})
+	}
+}
 
 impl Readable for QueryShortChannelIds {
 	fn read<R: Read>(r: &mut R) -> Result<Self, DecodeError> {

lightning/src/ln/wire.rs

@@ -358,10 +358,14 @@ where
 			LengthReadable::read_from_fixed_length_buffer(buffer)?,
 		)),
 		msgs::ChannelAnnouncement::TYPE => {
-			Ok(Message::ChannelAnnouncement(Readable::read(buffer)?))
+			Ok(Message::ChannelAnnouncement(LengthReadable::read_from_fixed_length_buffer(buffer)?))
+		},
+		msgs::NodeAnnouncement::TYPE => {
+			Ok(Message::NodeAnnouncement(LengthReadable::read_from_fixed_length_buffer(buffer)?))
+		},
+		msgs::ChannelUpdate::TYPE => {
+			Ok(Message::ChannelUpdate(LengthReadable::read_from_fixed_length_buffer(buffer)?))
 		},
-		msgs::NodeAnnouncement::TYPE => Ok(Message::NodeAnnouncement(Readable::read(buffer)?)),
-		msgs::ChannelUpdate::TYPE => Ok(Message::ChannelUpdate(Readable::read(buffer)?)),
 		msgs::QueryShortChannelIds::TYPE => {
 			Ok(Message::QueryShortChannelIds(Readable::read(buffer)?))
 		},

lightning/src/routing/gossip.rs

@@ -2730,7 +2730,7 @@ pub(crate) mod tests {
 	use crate::types::features::InitFeatures;
 	use crate::util::config::UserConfig;
 	use crate::util::scid_utils::scid_from_parts;
-	use crate::util::ser::{Hostname, Readable, ReadableArgs, Writeable};
+	use crate::util::ser::{Hostname, LengthReadable, Readable, ReadableArgs, Writeable};
 	use crate::util::test_utils;
 
 	use super::STALE_CHANNEL_UPDATE_AGE_LIMIT_SECS;
@@ -4301,7 +4301,8 @@ pub(crate) mod tests {
 		// 1. Check we can read a valid NodeAnnouncementInfo and fail on an invalid one
 		let announcement_message = <Vec<u8>>::from_hex("d977cb9b53d93a6ff64bb5f1e158b4094b66e798fb12911168a3ccdf80a83096340a6a95da0ae8d9f776528eecdbb747eb6b545495a4319ed5378e35b21e073a000122013413a7031b84c5567b126440995d3ed5aaba0565d71e1834604819ff9c17f5e9d5dd078f2020201010101010101010101010101010101010101010101010101010101010101010000701fffefdfc2607").unwrap();
 		let announcement_message =
-			NodeAnnouncement::read(&mut announcement_message.as_slice()).unwrap();
+			NodeAnnouncement::read_from_fixed_length_buffer(&mut announcement_message.as_slice())
+				.unwrap();
 		let valid_node_ann_info = NodeAnnouncementInfo::Relayed(announcement_message);
 
 		let mut encoded_valid_node_ann_info = Vec::new();

lightning/src/util/ser.rs

@@ -1330,14 +1330,14 @@ impl<T: Writeable> Writeable for Option<T> {
 	}
 }
 
-impl<T: Readable> Readable for Option<T> {
+impl<T: LengthReadable> Readable for Option<T> {
 	fn read<R: Read>(r: &mut R) -> Result<Self, DecodeError> {
 		let len: BigSize = Readable::read(r)?;
 		match len.0 {
 			0 => Ok(None),
 			len => {
 				let mut reader = FixedLengthReader::new(r, len - 1);
-				Ok(Some(Readable::read(&mut reader)?))
+				Ok(Some(LengthReadable::read_from_fixed_length_buffer(&mut reader)?))
 			},
 		}
 	}