Require length limited reader for all LN messages

When deserializing lightning messages, many consume the reader until it is out
of bytes, or could in the future if new fields or TLVs are added. Therefore,
it's safer if they are only provided with readers that limit how much of the
byte stream will be read.

Many of the relevant LN messages were updated in a prior commit when we
transitioned impl_writeable_msg to require length limiting readers, here we
finish the job.

Author: valentinewallace

fuzz/src/msg_targets/utils.rs

@@ -30,16 +30,16 @@ impl Writer for VecWriter {
 #[macro_export]
 macro_rules! test_msg {
 	($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) {
-			let p = r.position() as usize;
+		use lightning::util::ser::{LengthReadable, Writeable};
+		let mut r = &$data[..];
+		if let Ok(msg) = <$MsgType as LengthReadable>::read_from_fixed_length_buffer(&mut r) {
+			let p = $data.len() - r.len() as usize;
 			let mut w = VecWriter(Vec::new());
 			msg.write(&mut w).unwrap();
 
 			assert_eq!(w.0.len(), p);
 			assert_eq!(msg.serialized_length(), p);
-			assert_eq!(&r.into_inner()[..p], &w.0[..p]);
+			assert_eq!(&$data[..p], &w.0[..p]);
 		}
 	}};
 }
@@ -88,17 +88,18 @@ macro_rules! test_msg_exact {
 #[macro_export]
 macro_rules! test_msg_hole {
 	($MsgType: path, $data: ident, $hole: expr, $hole_len: expr) => {{
-		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();
 			let p = w.0.len() as usize;
 			assert_eq!(msg.serialized_length(), p);
 
 			assert_eq!(w.0.len(), p);
-			assert_eq!(&r.get_ref()[..$hole], &w.0[..$hole]);
-			assert_eq!(&r.get_ref()[$hole + $hole_len..p], &w.0[$hole + $hole_len..]);
+			assert_eq!(&$data[..$hole], &w.0[..$hole]);
+			assert_eq!(&$data[$hole + $hole_len..p], &w.0[$hole + $hole_len..]);
 		}
 	}};
 }

fuzz/src/onion_message.rs

@@ -26,20 +26,23 @@ use lightning::onion_message::packet::OnionMessageContents;
 use lightning::sign::{EntropySource, NodeSigner, Recipient, SignerProvider};
 use lightning::types::features::InitFeatures;
 use lightning::util::logger::Logger;
-use lightning::util::ser::{Readable, Writeable, Writer};
+use lightning::util::ser::{LengthReadable, Writeable, Writer};
 use lightning::util::test_channel_signer::TestChannelSigner;
 
 use lightning_invoice::RawBolt11Invoice;
 
 use crate::utils::test_logger;
 
-use lightning::io::{self, Cursor};
+use lightning::io;
 use std::sync::atomic::{AtomicU64, Ordering};
 
 #[inline]
 /// Actual fuzz test, method signature and name are fixed
 pub fn do_test<L: Logger>(data: &[u8], logger: &L) {
-	if let Ok(msg) = <msgs::OnionMessage as Readable>::read(&mut Cursor::new(data)) {
+	let mut reader = &data[..];
+	if let Ok(msg) =
+		<msgs::OnionMessage as LengthReadable>::read_from_fixed_length_buffer(&mut reader)
+	{
 		let mut secret_bytes = [1; 32];
 		secret_bytes[31] = 2;
 		let secret = SecretKey::from_slice(&secret_bytes).unwrap();

lightning/src/ln/msgs.rs

@@ -2412,8 +2412,8 @@ impl Writeable for AcceptChannel {
 	}
 }
 
-impl Readable for AcceptChannel {
-	fn read<R: Read>(r: &mut R) -> Result<Self, DecodeError> {
+impl LengthReadable for AcceptChannel {
+	fn read_from_fixed_length_buffer<R: LengthLimitedRead>(r: &mut R) -> Result<Self, DecodeError> {
 		let temporary_channel_id: ChannelId = Readable::read(r)?;
 		let dust_limit_satoshis: u64 = Readable::read(r)?;
 		let max_htlc_value_in_flight_msat: u64 = Readable::read(r)?;
@@ -2497,8 +2497,8 @@ impl Writeable for AcceptChannelV2 {
 	}
 }
 
-impl Readable for AcceptChannelV2 {
-	fn read<R: Read>(r: &mut R) -> Result<Self, DecodeError> {
+impl LengthReadable for AcceptChannelV2 {
+	fn read_from_fixed_length_buffer<R: LengthLimitedRead>(r: &mut R) -> Result<Self, DecodeError> {
 		let temporary_channel_id: ChannelId = Readable::read(r)?;
 		let funding_satoshis: u64 = Readable::read(r)?;
 		let dust_limit_satoshis: u64 = Readable::read(r)?;
@@ -2760,8 +2760,8 @@ impl Writeable for Init {
 	}
 }
 
-impl Readable for Init {
-	fn read<R: Read>(r: &mut R) -> Result<Self, DecodeError> {
+impl LengthReadable for Init {
+	fn read_from_fixed_length_buffer<R: LengthLimitedRead>(r: &mut R) -> Result<Self, DecodeError> {
 		let global_features: InitFeatures = Readable::read(r)?;
 		let features: InitFeatures = Readable::read(r)?;
 		let mut remote_network_address: Option<SocketAddress> = None;
@@ -2806,8 +2806,8 @@ impl Writeable for OpenChannel {
 	}
 }
 
-impl Readable for OpenChannel {
-	fn read<R: Read>(r: &mut R) -> Result<Self, DecodeError> {
+impl LengthReadable for OpenChannel {
+	fn read_from_fixed_length_buffer<R: LengthLimitedRead>(r: &mut R) -> Result<Self, DecodeError> {
 		let chain_hash: ChainHash = Readable::read(r)?;
 		let temporary_channel_id: ChannelId = Readable::read(r)?;
 		let funding_satoshis: u64 = Readable::read(r)?;
@@ -2890,8 +2890,8 @@ impl Writeable for OpenChannelV2 {
 	}
 }
 
-impl Readable for OpenChannelV2 {
-	fn read<R: Read>(r: &mut R) -> Result<Self, DecodeError> {
+impl LengthReadable for OpenChannelV2 {
+	fn read_from_fixed_length_buffer<R: LengthLimitedRead>(r: &mut R) -> Result<Self, DecodeError> {
 		let chain_hash: ChainHash = Readable::read(r)?;
 		let temporary_channel_id: ChannelId = Readable::read(r)?;
 		let funding_feerate_sat_per_1000_weight: u32 = Readable::read(r)?;
@@ -3052,8 +3052,8 @@ impl_writeable_msg!(UpdateAddHTLC, {
 	(65537, skimmed_fee_msat, option)
 });
 
-impl Readable for OnionMessage {
-	fn read<R: Read>(r: &mut R) -> Result<Self, DecodeError> {
+impl LengthReadable for OnionMessage {
+	fn read_from_fixed_length_buffer<R: LengthLimitedRead>(r: &mut R) -> Result<Self, DecodeError> {
 		let blinding_point: PublicKey = Readable::read(r)?;
 		let len: u16 = Readable::read(r)?;
 		let mut packet_reader = FixedLengthReader::new(r, len as u64);
@@ -3526,8 +3526,8 @@ impl Writeable for Ping {
 	}
 }
 
-impl Readable for Ping {
-	fn read<R: Read>(r: &mut R) -> Result<Self, DecodeError> {
+impl LengthReadable for Ping {
+	fn read_from_fixed_length_buffer<R: LengthLimitedRead>(r: &mut R) -> Result<Self, DecodeError> {
 		Ok(Ping {
 			ponglen: Readable::read(r)?,
 			byteslen: {
@@ -3546,8 +3546,8 @@ impl Writeable for Pong {
 	}
 }
 
-impl Readable for Pong {
-	fn read<R: Read>(r: &mut R) -> Result<Self, DecodeError> {
+impl LengthReadable for Pong {
+	fn read_from_fixed_length_buffer<R: LengthLimitedRead>(r: &mut R) -> Result<Self, DecodeError> {
 		Ok(Pong {
 			byteslen: {
 				let byteslen = Readable::read(r)?;
@@ -3680,8 +3680,8 @@ impl Writeable for ErrorMessage {
 	}
 }
 
-impl Readable for ErrorMessage {
-	fn read<R: Read>(r: &mut R) -> Result<Self, DecodeError> {
+impl LengthReadable for ErrorMessage {
+	fn read_from_fixed_length_buffer<R: LengthLimitedRead>(r: &mut R) -> Result<Self, DecodeError> {
 		Ok(Self {
 			channel_id: Readable::read(r)?,
 			data: {
@@ -3707,8 +3707,8 @@ impl Writeable for WarningMessage {
 	}
 }
 
-impl Readable for WarningMessage {
-	fn read<R: Read>(r: &mut R) -> Result<Self, DecodeError> {
+impl LengthReadable for WarningMessage {
+	fn read_from_fixed_length_buffer<R: LengthLimitedRead>(r: &mut R) -> Result<Self, DecodeError> {
 		Ok(Self {
 			channel_id: Readable::read(r)?,
 			data: {
@@ -3826,8 +3826,8 @@ impl LengthReadable for NodeAnnouncement {
 	}
 }
 
-impl Readable for QueryShortChannelIds {
-	fn read<R: Read>(r: &mut R) -> Result<Self, DecodeError> {
+impl LengthReadable for QueryShortChannelIds {
+	fn read_from_fixed_length_buffer<R: LengthLimitedRead>(r: &mut R) -> Result<Self, DecodeError> {
 		let chain_hash: ChainHash = Readable::read(r)?;
 
 		let encoding_len: u16 = Readable::read(r)?;
@@ -3902,8 +3902,8 @@ impl_writeable_msg!(QueryChannelRange, {
 	number_of_blocks
 }, {});
 
-impl Readable for ReplyChannelRange {
-	fn read<R: Read>(r: &mut R) -> Result<Self, DecodeError> {
+impl LengthReadable for ReplyChannelRange {
+	fn read_from_fixed_length_buffer<R: LengthLimitedRead>(r: &mut R) -> Result<Self, DecodeError> {
 		let chain_hash: ChainHash = Readable::read(r)?;
 		let first_blocknum: u32 = Readable::read(r)?;
 		let number_of_blocks: u32 = Readable::read(r)?;
@@ -5484,7 +5484,7 @@ mod tests {
 			let encoded_value = reply_channel_range.encode();
 			assert_eq!(encoded_value, target_value);
 
-			reply_channel_range = Readable::read(&mut Cursor::new(&target_value[..])).unwrap();
+			reply_channel_range = LengthReadable::read_from_fixed_length_buffer(&mut &target_value[..]).unwrap();
 			assert_eq!(reply_channel_range.chain_hash, expected_chain_hash);
 			assert_eq!(reply_channel_range.first_blocknum, 756230);
 			assert_eq!(reply_channel_range.number_of_blocks, 1500);
@@ -5494,7 +5494,7 @@ mod tests {
 			assert_eq!(reply_channel_range.short_channel_ids[2], 0x000000000045a6c4);
 		} else {
 			target_value.append(&mut <Vec<u8>>::from_hex("001601789c636000833e08659309a65878be010010a9023a").unwrap());
-			let result: Result<msgs::ReplyChannelRange, msgs::DecodeError> = Readable::read(&mut Cursor::new(&target_value[..]));
+			let result: Result<msgs::ReplyChannelRange, msgs::DecodeError> = LengthReadable::read_from_fixed_length_buffer(&mut &target_value[..]);
 			assert!(result.is_err(), "Expected decode failure with unsupported zlib encoding");
 		}
 	}
@@ -5518,14 +5518,14 @@ mod tests {
 			let encoded_value = query_short_channel_ids.encode();
 			assert_eq!(encoded_value, target_value);
 
-			query_short_channel_ids = Readable::read(&mut Cursor::new(&target_value[..])).unwrap();
+			query_short_channel_ids = LengthReadable::read_from_fixed_length_buffer(&mut &target_value[..]).unwrap();
 			assert_eq!(query_short_channel_ids.chain_hash, expected_chain_hash);
 			assert_eq!(query_short_channel_ids.short_channel_ids[0], 0x000000000000008e);
 			assert_eq!(query_short_channel_ids.short_channel_ids[1], 0x0000000000003c69);
 			assert_eq!(query_short_channel_ids.short_channel_ids[2], 0x000000000045a6c4);
 		} else {
 			target_value.append(&mut <Vec<u8>>::from_hex("001601789c636000833e08659309a65878be010010a9023a").unwrap());
-			let result: Result<msgs::QueryShortChannelIds, msgs::DecodeError> = Readable::read(&mut Cursor::new(&target_value[..]));
+			let result: Result<msgs::QueryShortChannelIds, msgs::DecodeError> = LengthReadable::read_from_fixed_length_buffer(&mut &target_value[..]);
 			assert!(result.is_err(), "Expected decode failure with unsupported zlib encoding");
 		}
 	}

lightning/src/ln/wire.rs

@@ -257,21 +257,39 @@ where
 	H::Target: CustomMessageReader<CustomMessage = T>,
 {
 	match message_type {
-		msgs::Init::TYPE => Ok(Message::Init(Readable::read(buffer)?)),
-		msgs::ErrorMessage::TYPE => Ok(Message::Error(Readable::read(buffer)?)),
-		msgs::WarningMessage::TYPE => Ok(Message::Warning(Readable::read(buffer)?)),
-		msgs::Ping::TYPE => Ok(Message::Ping(Readable::read(buffer)?)),
-		msgs::Pong::TYPE => Ok(Message::Pong(Readable::read(buffer)?)),
+		msgs::Init::TYPE => {
+			Ok(Message::Init(LengthReadable::read_from_fixed_length_buffer(buffer)?))
+		},
+		msgs::ErrorMessage::TYPE => {
+			Ok(Message::Error(LengthReadable::read_from_fixed_length_buffer(buffer)?))
+		},
+		msgs::WarningMessage::TYPE => {
+			Ok(Message::Warning(LengthReadable::read_from_fixed_length_buffer(buffer)?))
+		},
+		msgs::Ping::TYPE => {
+			Ok(Message::Ping(LengthReadable::read_from_fixed_length_buffer(buffer)?))
+		},
+		msgs::Pong::TYPE => {
+			Ok(Message::Pong(LengthReadable::read_from_fixed_length_buffer(buffer)?))
+		},
 		msgs::PeerStorage::TYPE => {
 			Ok(Message::PeerStorage(LengthReadable::read_from_fixed_length_buffer(buffer)?))
 		},
 		msgs::PeerStorageRetrieval::TYPE => Ok(Message::PeerStorageRetrieval(
 			LengthReadable::read_from_fixed_length_buffer(buffer)?,
 		)),
-		msgs::OpenChannel::TYPE => Ok(Message::OpenChannel(Readable::read(buffer)?)),
-		msgs::OpenChannelV2::TYPE => Ok(Message::OpenChannelV2(Readable::read(buffer)?)),
-		msgs::AcceptChannel::TYPE => Ok(Message::AcceptChannel(Readable::read(buffer)?)),
-		msgs::AcceptChannelV2::TYPE => Ok(Message::AcceptChannelV2(Readable::read(buffer)?)),
+		msgs::OpenChannel::TYPE => {
+			Ok(Message::OpenChannel(LengthReadable::read_from_fixed_length_buffer(buffer)?))
+		},
+		msgs::OpenChannelV2::TYPE => {
+			Ok(Message::OpenChannelV2(LengthReadable::read_from_fixed_length_buffer(buffer)?))
+		},
+		msgs::AcceptChannel::TYPE => {
+			Ok(Message::AcceptChannel(LengthReadable::read_from_fixed_length_buffer(buffer)?))
+		},
+		msgs::AcceptChannelV2::TYPE => {
+			Ok(Message::AcceptChannelV2(LengthReadable::read_from_fixed_length_buffer(buffer)?))
+		},
 		msgs::FundingCreated::TYPE => {
 			Ok(Message::FundingCreated(LengthReadable::read_from_fixed_length_buffer(buffer)?))
 		},
@@ -329,7 +347,9 @@ where
 		msgs::ClosingSigned::TYPE => {
 			Ok(Message::ClosingSigned(LengthReadable::read_from_fixed_length_buffer(buffer)?))
 		},
-		msgs::OnionMessage::TYPE => Ok(Message::OnionMessage(Readable::read(buffer)?)),
+		msgs::OnionMessage::TYPE => {
+			Ok(Message::OnionMessage(LengthReadable::read_from_fixed_length_buffer(buffer)?))
+		},
 		msgs::UpdateAddHTLC::TYPE => {
 			Ok(Message::UpdateAddHTLC(LengthReadable::read_from_fixed_length_buffer(buffer)?))
 		},
@@ -366,16 +386,18 @@ where
 		msgs::ChannelUpdate::TYPE => {
 			Ok(Message::ChannelUpdate(LengthReadable::read_from_fixed_length_buffer(buffer)?))
 		},
-		msgs::QueryShortChannelIds::TYPE => {
-			Ok(Message::QueryShortChannelIds(Readable::read(buffer)?))
-		},
+		msgs::QueryShortChannelIds::TYPE => Ok(Message::QueryShortChannelIds(
+			LengthReadable::read_from_fixed_length_buffer(buffer)?,
+		)),
 		msgs::ReplyShortChannelIdsEnd::TYPE => Ok(Message::ReplyShortChannelIdsEnd(
 			LengthReadable::read_from_fixed_length_buffer(buffer)?,
 		)),
 		msgs::QueryChannelRange::TYPE => {
 			Ok(Message::QueryChannelRange(LengthReadable::read_from_fixed_length_buffer(buffer)?))
 		},
-		msgs::ReplyChannelRange::TYPE => Ok(Message::ReplyChannelRange(Readable::read(buffer)?)),
+		msgs::ReplyChannelRange::TYPE => {
+			Ok(Message::ReplyChannelRange(LengthReadable::read_from_fixed_length_buffer(buffer)?))
+		},
 		msgs::GossipTimestampFilter::TYPE => Ok(Message::GossipTimestampFilter(
 			LengthReadable::read_from_fixed_length_buffer(buffer)?,
 		)),