From: Jeffrey Czyz Date: Fri, 30 Sep 2022 20:50:12 +0000 (-0500) Subject: Limit TLV stream decoding to type ranges X-Git-Tag: v0.0.113~36^2~1 X-Git-Url: http://git.bitcoin.ninja/index.cgi?a=commitdiff_plain;h=94a07d9caee6d38d42954ac783c49afe1cf89697;p=rust-lightning Limit TLV stream decoding to type ranges BOLT 12 messages are limited to a range of TLV record types. Refactor decode_tlv_stream into a decode_tlv_stream_range macro for limiting which types are parsed. Requires a SeekReadable trait for rewinding when a type outside of the range is seen. This allows for composing TLV streams of different ranges. Updates offer parsing accordingly and adds a test demonstrating failure if a type outside of the range is included. --- diff --git a/lightning/src/offers/offer.rs b/lightning/src/offers/offer.rs index e57334f31..6b2dfe2aa 100644 --- a/lightning/src/offers/offer.rs +++ b/lightning/src/offers/offer.rs @@ -76,9 +76,9 @@ use core::time::Duration; use crate::io; use crate::ln::features::OfferFeatures; use crate::ln::msgs::MAX_VALUE_MSAT; -use crate::offers::parse::{Bech32Encode, ParseError, SemanticError}; +use crate::offers::parse::{Bech32Encode, ParseError, ParsedMessage, SemanticError}; use crate::onion_message::BlindedPath; -use crate::util::ser::{HighZeroBytesDroppedBigSize, Readable, WithoutLength, Writeable, Writer}; +use crate::util::ser::{HighZeroBytesDroppedBigSize, WithoutLength, Writeable, Writer}; use crate::util::string::PrintableString; use crate::prelude::*; @@ -394,15 +394,6 @@ impl Writeable for OfferContents { } } -impl TryFrom> for Offer { - type Error = ParseError; - - fn try_from(bytes: Vec) -> Result { - let tlv_stream: OfferTlvStream = Readable::read(&mut &bytes[..])?; - Offer::try_from((bytes, tlv_stream)) - } -} - /// The minimum amount required for an item in an [`Offer`], denominated in either bitcoin or /// another currency. #[derive(Clone, Debug, PartialEq)] @@ -449,7 +440,7 @@ impl Quantity { } } -tlv_stream!(OfferTlvStream, OfferTlvStreamRef, { +tlv_stream!(OfferTlvStream, OfferTlvStreamRef, 1..80, { (2, chains: (Vec, WithoutLength)), (4, metadata: (Vec, WithoutLength)), (6, currency: CurrencyCode), @@ -467,8 +458,6 @@ impl Bech32Encode for Offer { const BECH32_HRP: &'static str = "lno"; } -type ParsedOffer = (Vec, OfferTlvStream); - impl FromStr for Offer { type Err = ParseError; @@ -477,11 +466,12 @@ impl FromStr for Offer { } } -impl TryFrom for Offer { +impl TryFrom> for Offer { type Error = ParseError; - fn try_from(offer: ParsedOffer) -> Result { - let (bytes, tlv_stream) = offer; + fn try_from(bytes: Vec) -> Result { + let offer = ParsedMessage::::try_from(bytes)?; + let ParsedMessage { bytes, tlv_stream } = offer; let contents = OfferContents::try_from(tlv_stream)?; Ok(Offer { bytes, contents }) } @@ -551,10 +541,10 @@ mod tests { use core::num::NonZeroU64; use core::time::Duration; use crate::ln::features::OfferFeatures; - use crate::ln::msgs::MAX_VALUE_MSAT; + use crate::ln::msgs::{DecodeError, MAX_VALUE_MSAT}; use crate::offers::parse::{ParseError, SemanticError}; use crate::onion_message::{BlindedHop, BlindedPath}; - use crate::util::ser::Writeable; + use crate::util::ser::{BigSize, Writeable}; use crate::util::string::PrintableString; fn pubkey(byte: u8) -> PublicKey { @@ -1003,6 +993,22 @@ mod tests { }, } } + + #[test] + fn fails_parsing_offer_with_extra_tlv_records() { + let offer = OfferBuilder::new("foo".into(), pubkey(42)).build().unwrap(); + + let mut encoded_offer = Vec::new(); + offer.write(&mut encoded_offer).unwrap(); + BigSize(80).write(&mut encoded_offer).unwrap(); + BigSize(32).write(&mut encoded_offer).unwrap(); + [42u8; 32].write(&mut encoded_offer).unwrap(); + + match Offer::try_from(encoded_offer) { + Ok(_) => panic!("expected error"), + Err(e) => assert_eq!(e, ParseError::Decode(DecodeError::InvalidValue)), + } + } } #[cfg(test)] diff --git a/lightning/src/offers/parse.rs b/lightning/src/offers/parse.rs index 032e95b3c..19d7d74ed 100644 --- a/lightning/src/offers/parse.rs +++ b/lightning/src/offers/parse.rs @@ -13,7 +13,9 @@ use bitcoin::bech32; use bitcoin::bech32::{FromBase32, ToBase32}; use core::convert::TryFrom; use core::fmt; +use crate::io; use crate::ln::msgs::DecodeError; +use crate::util::ser::SeekReadable; use crate::prelude::*; @@ -74,6 +76,30 @@ impl<'a> AsRef for Bech32String<'a> { } } +/// A wrapper for reading a message as a TLV stream `T` from a byte sequence, while still +/// maintaining ownership of the bytes for later use. +pub(crate) struct ParsedMessage { + pub bytes: Vec, + pub tlv_stream: T, +} + +impl TryFrom> for ParsedMessage { + type Error = DecodeError; + + fn try_from(bytes: Vec) -> Result { + let mut cursor = io::Cursor::new(bytes); + let tlv_stream: T = SeekReadable::read(&mut cursor)?; + + // Ensure that there are no more TLV records left to parse. + if cursor.position() < cursor.get_ref().len() as u64 { + return Err(DecodeError::InvalidValue); + } + + let bytes = cursor.into_inner(); + Ok(Self { bytes, tlv_stream }) + } +} + /// Error when parsing a bech32 encoded message using [`str::parse`]. #[derive(Debug, PartialEq)] pub enum ParseError { diff --git a/lightning/src/util/ser.rs b/lightning/src/util/ser.rs index 7c4ba7fd5..04dc04e55 100644 --- a/lightning/src/util/ser.rs +++ b/lightning/src/util/ser.rs @@ -11,7 +11,7 @@ //! as ChannelsManagers and ChannelMonitors. use crate::prelude::*; -use crate::io::{self, Read, Write}; +use crate::io::{self, Read, Seek, Write}; use crate::io_extras::{copy, sink}; use core::hash::Hash; use crate::sync::Mutex; @@ -219,6 +219,13 @@ pub trait Readable fn read(reader: &mut R) -> Result; } +/// A trait that various rust-lightning types implement allowing them to be read in from a +/// `Read + Seek`. +pub(crate) trait SeekReadable where Self: Sized { + /// Reads a Self in from the given Read + fn read(reader: &mut R) -> Result; +} + /// A trait that various higher-level rust-lightning types implement allowing them to be read in /// from a Read given some additional set of arguments which is required to deserialize. /// diff --git a/lightning/src/util/ser_macros.rs b/lightning/src/util/ser_macros.rs index 129fb8f40..db7553d5f 100644 --- a/lightning/src/util/ser_macros.rs +++ b/lightning/src/util/ser_macros.rs @@ -201,6 +201,17 @@ macro_rules! decode_tlv { // `Ok(false)` if the message type is unknown, and `Err(DecodeError)` if parsing fails. macro_rules! decode_tlv_stream { ($stream: expr, {$(($type: expr, $field: ident, $fieldty: tt)),* $(,)*} + $(, $decode_custom_tlv: expr)?) => { { + let rewind = |_, _| { unreachable!() }; + use core::ops::RangeBounds; + decode_tlv_stream_range!( + $stream, .., rewind, {$(($type, $field, $fieldty)),*} $(, $decode_custom_tlv)? + ); + } } +} + +macro_rules! decode_tlv_stream_range { + ($stream: expr, $range: expr, $rewind: ident, {$(($type: expr, $field: ident, $fieldty: tt)),* $(,)*} $(, $decode_custom_tlv: expr)?) => { { use $crate::ln::msgs::DecodeError; let mut last_seen_type: Option = None; @@ -215,7 +226,7 @@ macro_rules! decode_tlv_stream { // UnexpectedEof. This should in every case be largely cosmetic, but its nice to // pass the TLV test vectors exactly, which requre this distinction. let mut tracking_reader = ser::ReadTrackingReader::new(&mut stream_ref); - match $crate::util::ser::Readable::read(&mut tracking_reader) { + match <$crate::util::ser::BigSize as $crate::util::ser::Readable>::read(&mut tracking_reader) { Err(DecodeError::ShortRead) => { if !tracking_reader.have_read { break 'tlv_read; @@ -224,7 +235,15 @@ macro_rules! decode_tlv_stream { } }, Err(e) => return Err(e), - Ok(t) => t, + Ok(t) => if $range.contains(&t.0) { t } else { + drop(tracking_reader); + + // Assumes the type id is minimally encoded, which is enforced on read. + use $crate::util::ser::Writeable; + let bytes_read = t.serialized_length(); + $rewind(stream_ref, bytes_read); + break 'tlv_read; + }, } }; @@ -472,7 +491,7 @@ macro_rules! impl_writeable_tlv_based { /// [`Readable`]: crate::util::ser::Readable /// [`Writeable`]: crate::util::ser::Writeable macro_rules! tlv_stream { - ($name:ident, $nameref:ident, { + ($name:ident, $nameref:ident, $range:expr, { $(($type:expr, $field:ident : $fieldty:tt)),* $(,)* }) => { #[derive(Debug)] @@ -497,12 +516,15 @@ macro_rules! tlv_stream { } } - impl $crate::util::ser::Readable for $name { - fn read(reader: &mut R) -> Result { + impl $crate::util::ser::SeekReadable for $name { + fn read(reader: &mut R) -> Result { $( init_tlv_field_var!($field, option); )* - decode_tlv_stream!(reader, { + let rewind = |cursor: &mut R, offset: usize| { + cursor.seek($crate::io::SeekFrom::Current(-(offset as i64))).expect(""); + }; + decode_tlv_stream_range!(reader, $range, rewind, { $(($type, $field, (option, encoding: $fieldty))),* });