Add support for variable-length onion payload reads using TLV

Author: TheBlueMatt

fuzz/src/msg_targets/gen_target.sh

@@ -34,8 +34,8 @@ GEN_TEST NodeAnnouncement test_msg_exact ""
 
 GEN_TEST UpdateAddHTLC test_msg_hole ", 85, 33"
 GEN_TEST ErrorMessage test_msg_hole ", 32, 2"
-GEN_TEST OnionHopData test_msg_hole ", 1+8+8+4, 12"
 
 GEN_TEST Init test_msg_simple ""
+GEN_TEST OnionHopData test_msg_simple ""
 GEN_TEST Ping test_msg_simple ""
 GEN_TEST Pong test_msg_simple ""

fuzz/src/msg_targets/mod.rs

@@ -20,7 +20,7 @@ pub mod msg_channel_update;
 pub mod msg_node_announcement;
 pub mod msg_update_add_htlc;
 pub mod msg_error_message;
-pub mod msg_onion_hop_data;
 pub mod msg_init;
+pub mod msg_onion_hop_data;
 pub mod msg_ping;
 pub mod msg_pong;

fuzz/src/msg_targets/msg_onion_hop_data.rs

@@ -7,7 +7,7 @@ use msg_targets::utils::VecWriter;
 
 #[inline]
 pub fn do_test(data: &[u8]) {
-	test_msg_hole!(msgs::OnionHopData, data, 1+8+8+4, 12);
+	test_msg_simple!(msgs::OnionHopData, data);
 }
 
 #[no_mangle]

lightning/src/ln/channelmanager.rs

@@ -910,14 +910,17 @@ impl<ChanSigner: ChannelKeys, M: Deref> ChannelManager<ChanSigner, M> where M::T
 				Err(err) => {
 					let error_code = match err {
 						msgs::DecodeError::UnknownVersion => 0x4000 | 1, // unknown realm byte
+						msgs::DecodeError::UnknownRequiredFeature|
+						msgs::DecodeError::InvalidValue|
+						msgs::DecodeError::ShortRead => 0x4000 | 22, // invalid_onion_payload
 						_ => 0x2000 | 2, // Should never happen
 					};
 					return_err!("Unable to decode our hop data", error_code, &[0;0]);
 				},
 				Ok(msg) => {
 					let mut hmac = [0; 32];
 					if let Err(_) = chacha_stream.read_exact(&mut hmac[..]) {
-						return_err!("Unable to decode hop data", 0x4000 | 1, &[0;0]);
+						return_err!("Unable to decode hop data", 0x4000 | 22, &[0;0]);
 					}
 					(msg, hmac)
 				},
@@ -971,6 +974,15 @@ impl<ChanSigner: ChannelKeys, M: Deref> ChannelManager<ChanSigner, M> where M::T
 			} else {
 				let mut new_packet_data = [0; 20*65];
 				let read_pos = chacha_stream.read(&mut new_packet_data).unwrap();
+				#[cfg(debug_assertions)]
+				{
+					// Check two things:
+					// a) that the behavior of our stream here will return Ok(0) even if the TLV
+					//    read above emptied out our buffer and the unwrap() wont needlessly panic
+					// b) that we didn't somehow magically end up with extra data.
+					let mut t = [0; 1];
+					debug_assert!(chacha_stream.read(&mut t).unwrap() == 0);
+				}
 				// Once we've emptied the set of bytes our peer gave us, encrypt 0 bytes until we
 				// fill the onion hop data we'll forward to our next-hop peer.
 				chacha_stream.chacha.process_in_place(&mut new_packet_data[read_pos..]);
@@ -995,10 +1007,18 @@ impl<ChanSigner: ChannelKeys, M: Deref> ChannelManager<ChanSigner, M> where M::T
 					hmac: next_hop_hmac.clone(),
 				};
 
+				let short_channel_id = match next_hop_data.format {
+					msgs::OnionHopDataFormat::Legacy { short_channel_id } => short_channel_id,
+					msgs::OnionHopDataFormat::NonFinalNode { short_channel_id } => short_channel_id,
+					msgs::OnionHopDataFormat::FinalNode => {
+						return_err!("Final Node OnionHopData provided for us as an intermediary node", 0x4000 | 22, &[0;0]);
+					},
+				};
+
 				PendingHTLCStatus::Forward(PendingForwardHTLCInfo {
 					onion_packet: Some(outgoing_packet),
 					payment_hash: msg.payment_hash.clone(),
-					short_channel_id: next_hop_data.short_channel_id,
+					short_channel_id: short_channel_id,
 					incoming_shared_secret: shared_secret,
 					amt_to_forward: next_hop_data.amt_to_forward,
 					outgoing_cltv_value: next_hop_data.outgoing_cltv_value,

lightning/src/ln/features.rs

@@ -182,13 +182,21 @@ impl<T: sealed::Context> Features<T> {
 
 	pub(crate) fn requires_unknown_bits(&self) -> bool {
 		self.flags.iter().enumerate().any(|(idx, &byte)| {
-			( idx != 0 && (byte & 0x55) != 0 ) || ( idx == 0 && (byte & 0x14) != 0 )
+			(match idx {
+				0 => (byte & 0b00010100),
+				1 => (byte & 0b01010100),
+				_ => (byte & 0b01010101),
+			}) != 0
 		})
 	}
 
 	pub(crate) fn supports_unknown_bits(&self) -> bool {
 		self.flags.iter().enumerate().any(|(idx, &byte)| {
-			( idx != 0 && byte != 0 ) || ( idx == 0 && (byte & 0xc4) != 0 )
+			(match idx {
+				0 => (byte & 0b11000100),
+				1 => (byte & 0b11111100),
+				_ => byte,
+			}) != 0
 		})
 	}
 

lightning/src/ln/functional_tests.rs

@@ -5023,7 +5023,7 @@ fn test_onion_failure() {
 		// describing a length-1 TLV payload, which is obviously bogus.
 		new_payloads[0].data[0] = 1;
 		msg.onion_routing_packet = onion_utils::construct_onion_packet_bogus_hopdata(new_payloads, onion_keys, [0; 32], &payment_hash);
-	}, ||{}, true, Some(PERM|1), Some(msgs::HTLCFailChannelUpdate::ChannelClosed{short_channel_id: channels[1].0.contents.short_channel_id, is_permanent: true}));//XXX incremented channels idx here
+	}, ||{}, true, Some(PERM|22), Some(msgs::HTLCFailChannelUpdate::ChannelClosed{short_channel_id: channels[1].0.contents.short_channel_id, is_permanent: true}));//XXX incremented channels idx here
 
 	// final node failure
 	run_onion_failure_test("invalid_realm", 3, &nodes, &route, &payment_hash, |msg| {
@@ -5039,7 +5039,7 @@ fn test_onion_failure() {
 		// length-1 TLV payload, which is obviously bogus.
 		new_payloads[1].data[0] = 1;
 		msg.onion_routing_packet = onion_utils::construct_onion_packet_bogus_hopdata(new_payloads, onion_keys, [0; 32], &payment_hash);
-	}, ||{}, false, Some(PERM|1), Some(msgs::HTLCFailChannelUpdate::ChannelClosed{short_channel_id: channels[1].0.contents.short_channel_id, is_permanent: true}));
+	}, ||{}, false, Some(PERM|22), Some(msgs::HTLCFailChannelUpdate::ChannelClosed{short_channel_id: channels[1].0.contents.short_channel_id, is_permanent: true}));
 
 	// the following three with run_onion_failure_test_with_fail_intercept() test only the origin node
 	// receiving simulated fail messages

lightning/src/ln/msgs.rs

@@ -29,7 +29,7 @@ use std::io::Read;
 use std::result::Result;
 
 use util::events;
-use util::ser::{Readable, Writeable, Writer};
+use util::ser::{Readable, Writeable, Writer, FixedLengthReader, HighZeroBytesDroppedVarInt};
 
 use ln::channelmanager::{PaymentPreimage, PaymentHash};
 
@@ -39,7 +39,7 @@ pub enum DecodeError {
 	/// A version byte specified something we don't know how to handle.
 	/// Includes unknown realm byte in an OnionHopData packet
 	UnknownVersion,
-	/// Unknown feature mandating we fail to parse message
+	/// Unknown feature mandating we fail to parse message (eg TLV with an even, unknown type)
 	UnknownRequiredFeature,
 	/// Value was invalid, eg a byte which was supposed to be a bool was something other than a 0
 	/// or 1, a public key/private key/signature was invalid, text wasn't UTF-8, etc
@@ -611,15 +611,20 @@ mod fuzzy_internal_msgs {
 	// them from untrusted input):
 
 	pub(crate) enum OnionHopDataFormat {
-		Legacy, // aka Realm-0
+		Legacy { // aka Realm-0
+			short_channel_id: u64,
+		},
+		NonFinalNode {
+			short_channel_id: u64,
+		},
+		FinalNode,
 	}
 
 	pub struct OnionHopData {
 		pub(crate) format: OnionHopDataFormat,
-		pub(crate) short_channel_id: u64,
 		pub(crate) amt_to_forward: u64,
 		pub(crate) outgoing_cltv_value: u32,
-		// 12 bytes of 0-padding
+		// 12 bytes of 0-padding for Legacy format
 	}
 
 	pub struct DecodedOnionErrorPacket {
@@ -960,33 +965,79 @@ impl Writeable for OnionHopData {
 	fn write<W: Writer>(&self, w: &mut W) -> Result<(), ::std::io::Error> {
 		w.size_hint(33);
 		match self.format {
-			OnionHopDataFormat::Legacy => 0u8.write(w)?,
+			OnionHopDataFormat::Legacy { short_channel_id } => {
+				0u8.write(w)?;
+				short_channel_id.write(w)?;
+				self.amt_to_forward.write(w)?;
+				self.outgoing_cltv_value.write(w)?;
+			},
+			OnionHopDataFormat::NonFinalNode { short_channel_id } => {
+				encode_varint_length_prefixed_tlv!(w, {
+					(2, HighZeroBytesDroppedVarInt(self.amt_to_forward)),
+					(4, HighZeroBytesDroppedVarInt(self.outgoing_cltv_value)),
+					(6, short_channel_id)
+				});
+			},
+			OnionHopDataFormat::FinalNode => {
+				encode_varint_length_prefixed_tlv!(w, {
+					(2, HighZeroBytesDroppedVarInt(self.amt_to_forward)),
+					(4, HighZeroBytesDroppedVarInt(self.outgoing_cltv_value))
+				});
+			},
+		}
+		match self.format {
+			OnionHopDataFormat::Legacy { .. } => {
+				w.write_all(&[0;12])?;
+			},
+			_ => {},
 		}
-		self.short_channel_id.write(w)?;
-		self.amt_to_forward.write(w)?;
-		self.outgoing_cltv_value.write(w)?;
-		w.write_all(&[0;12])?;
 		Ok(())
 	}
 }
 
 impl<R: Read> Readable<R> for OnionHopData {
-	fn read(r: &mut R) -> Result<Self, DecodeError> {
-		Ok(OnionHopData {
-			format: {
-				let r: u8 = Readable::read(r)?;
-				if r != 0 {
-					return Err(DecodeError::UnknownVersion);
+	fn read(mut r: &mut R) -> Result<Self, DecodeError> {
+		use bitcoin::consensus::encode::{Decodable, Error, VarInt};
+		let v: VarInt = Decodable::consensus_decode(&mut r)
+			.map_err(|e| match e {
+				Error::Io(ioe) => DecodeError::from(ioe),
+				_ => DecodeError::InvalidValue
+			})?;
+		const LEGACY_ONION_HOP_FLAG: u64 = 0;
+		let (format, amt, cltv_value) = if v.0 != LEGACY_ONION_HOP_FLAG {
+			let mut rd = FixedLengthReader::new(r, v.0);
+			let mut amt = HighZeroBytesDroppedVarInt(0u64);
+			let mut cltv_value = HighZeroBytesDroppedVarInt(0u32);
+			let mut short_id: Option<u64> = None;
+			decode_tlv!(&mut rd, {
+				(2, amt),
+				(4, cltv_value)
+			}, {
+				(6, short_id)
+			});
+			rd.eat_remaining().map_err(|_| DecodeError::ShortRead)?;
+			let format = if let Some(short_channel_id) = short_id {
+				OnionHopDataFormat::NonFinalNode {
+					short_channel_id,
 				}
-				OnionHopDataFormat::Legacy
-			},
-			short_channel_id: Readable::read(r)?,
-			amt_to_forward: Readable::read(r)?,
-			outgoing_cltv_value: {
-				let v: u32 = Readable::read(r)?;
-				r.read_exact(&mut [0; 12])?;
-				v
-			},
+			} else {
+				OnionHopDataFormat::FinalNode
+			};
+			(format, amt.0, cltv_value.0)
+		} else {
+			let format = OnionHopDataFormat::Legacy {
+				short_channel_id: Readable::read(r)?,
+			};
+			let amt: u64 = Readable::read(r)?;
+			let cltv_value: u32 = Readable::read(r)?;
+			r.read_exact(&mut [0; 12])?;
+			(format, amt, cltv_value)
+		};
+
+		Ok(OnionHopData {
+			format,
+			amt_to_forward: amt,
+			outgoing_cltv_value: cltv_value,
 		})
 	}
 }
@@ -1272,9 +1323,9 @@ impl_writeable_len_match!(NodeAnnouncement, {
 mod tests {
 	use hex;
 	use ln::msgs;
-	use ln::msgs::{ChannelFeatures, InitFeatures, NodeFeatures, OptionalField, OnionErrorPacket};
+	use ln::msgs::{ChannelFeatures, InitFeatures, NodeFeatures, OptionalField, OnionErrorPacket, OnionHopDataFormat};
 	use ln::channelmanager::{PaymentPreimage, PaymentHash};
-	use util::ser::Writeable;
+	use util::ser::{Writeable, Readable};
 
 	use bitcoin_hashes::sha256d::Hash as Sha256dHash;
 	use bitcoin_hashes::hex::FromHex;
@@ -1286,6 +1337,8 @@ mod tests {
 	use secp256k1::key::{PublicKey,SecretKey};
 	use secp256k1::{Secp256k1, Message};
 
+	use std::io::Cursor;
+
 	#[test]
 	fn encoding_channel_reestablish_no_secret() {
 		let cr = msgs::ChannelReestablish {
@@ -1925,4 +1978,54 @@ mod tests {
 		let target_value = hex::decode("004000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000").unwrap();
 		assert_eq!(encoded_value, target_value);
 	}
+
+	#[test]
+	fn encoding_legacy_onion_hop_data() {
+		let msg = msgs::OnionHopData {
+			format: OnionHopDataFormat::Legacy {
+				short_channel_id: 0xdeadbeef1bad1dea,
+			},
+			amt_to_forward: 0x0badf00d01020304,
+			outgoing_cltv_value: 0xffffffff,
+		};
+		let encoded_value = msg.encode();
+		let target_value = hex::decode("00deadbeef1bad1dea0badf00d01020304ffffffff000000000000000000000000").unwrap();
+		assert_eq!(encoded_value, target_value);
+	}
+
+	#[test]
+	fn encoding_nonfinal_onion_hop_data() {
+		let mut msg = msgs::OnionHopData {
+			format: OnionHopDataFormat::NonFinalNode {
+				short_channel_id: 0xdeadbeef1bad1dea,
+			},
+			amt_to_forward: 0x0badf00d01020304,
+			outgoing_cltv_value: 0xffffffff,
+		};
+		let encoded_value = msg.encode();
+		let target_value = hex::decode("1a02080badf00d010203040404ffffffff0608deadbeef1bad1dea").unwrap();
+		assert_eq!(encoded_value, target_value);
+		msg = Readable::read(&mut Cursor::new(&target_value[..])).unwrap();
+		if let OnionHopDataFormat::NonFinalNode { short_channel_id } = msg.format {
+			assert_eq!(short_channel_id, 0xdeadbeef1bad1dea);
+		} else { panic!(); }
+		assert_eq!(msg.amt_to_forward, 0x0badf00d01020304);
+		assert_eq!(msg.outgoing_cltv_value, 0xffffffff);
+	}
+
+	#[test]
+	fn encoding_final_onion_hop_data() {
+		let mut msg = msgs::OnionHopData {
+			format: OnionHopDataFormat::FinalNode,
+			amt_to_forward: 0x0badf00d01020304,
+			outgoing_cltv_value: 0xffffffff,
+		};
+		let encoded_value = msg.encode();
+		let target_value = hex::decode("1002080badf00d010203040404ffffffff").unwrap();
+		assert_eq!(encoded_value, target_value);
+		msg = Readable::read(&mut Cursor::new(&target_value[..])).unwrap();
+		if let OnionHopDataFormat::FinalNode = msg.format { } else { panic!(); }
+		assert_eq!(msg.amt_to_forward, 0x0badf00d01020304);
+		assert_eq!(msg.outgoing_cltv_value, 0xffffffff);
+	}
 }