From cc78b77c715d6ef62693d4c1bc7190da990ec0fa Mon Sep 17 00:00:00 2001 From: Matt Corallo Date: Tue, 26 Mar 2024 15:08:20 +0000 Subject: [PATCH 1/3] Fix unknown handling in `impl_writeable_tlv_based_enum_upgradable` `impl_writeable_tlv_based_enum_upgradable` professed to supporting upgrades by returning `None` from `MaybeReadable` when unknown variants written by newer versions of LDK were read. However, it generally didn't support this as it didn't discard bytes for unknown types, resulting in corrupt reading. This is fixed here for enum variants written as a TLV stream, however we don't have a length prefix for tuple enum variants, so the documentation on the macro is updated to mention that downgrades are not supported for tuple variants. --- lightning/src/util/ser_macros.rs | 13 ++++++++++++- 1 file changed, 12 insertions(+), 1 deletion(-) diff --git a/lightning/src/util/ser_macros.rs b/lightning/src/util/ser_macros.rs index 84d9f7a180b..5d6988ba1f1 100644 --- a/lightning/src/util/ser_macros.rs +++ b/lightning/src/util/ser_macros.rs @@ -1065,6 +1065,10 @@ macro_rules! impl_writeable_tlv_based_enum { /// when [`MaybeReadable`] is practical instead of just [`Readable`] as it provides an upgrade path for /// new variants to be added which are simply ignored by existing clients. /// +/// Note that only struct and unit variants (not tuple variants) will support downgrading, thus any +/// new odd variants MUST be non-tuple (i.e. described using `$variant_id` and `$variant_name` not +/// `$tuple_variant_id` and `$tuple_variant_name`). +/// /// [`MaybeReadable`]: crate::util::ser::MaybeReadable /// [`Writeable`]: crate::util::ser::Writeable /// [`DecodeError::UnknownRequiredFeature`]: crate::ln::msgs::DecodeError::UnknownRequiredFeature @@ -1102,7 +1106,14 @@ macro_rules! impl_writeable_tlv_based_enum_upgradable { $($($tuple_variant_id => { Ok(Some($st::$tuple_variant_name(Readable::read(reader)?))) }),*)* - _ if id % 2 == 1 => Ok(None), + _ if id % 2 == 1 => { + // Assume that a $variant_id was written, not a $tuple_variant_id, and read + // the length prefix and discard the correct number of bytes. + let tlv_len: $crate::util::ser::BigSize = $crate::util::ser::Readable::read(reader)?; + let mut rd = $crate::util::ser::FixedLengthReader::new(reader, tlv_len.0); + rd.eat_remaining().map_err(|_| $crate::ln::msgs::DecodeError::ShortRead)?; + Ok(None) + }, _ => Err($crate::ln::msgs::DecodeError::UnknownRequiredFeature), } } From d6770d4a74348c91c91dfe734a1c38b7fa26d81e Mon Sep 17 00:00:00 2001 From: Matt Corallo Date: Thu, 28 Mar 2024 22:02:09 +0000 Subject: [PATCH 2/3] Ensure we read the full TLV stream length when maybe-reading `None` If we are reading an object that is `MaybeReadable` in a TLV stream using `upgradable_required`, it may return early with `Ok(None)`. In this case, it will not read any further TLVs from the TLV stream. This is fine, except that we generally expect `MaybeReadable` always consume the correct number of bytes for the full object, even if it doesn't understand it. This could pose a problem, for example, in cases where we're reading a TLV-stream `MaybeReadable` object inside another TLV-stream object. In that case, the `MaybeReadable` object may return `Ok(None)` and not consume all the available bytes, causing the outer TLV read to fail as the TLV length does not match. --- lightning/src/lib.rs | 6 ++++- lightning/src/util/ser_macros.rs | 45 +++++++++++++++++++------------- 2 files changed, 32 insertions(+), 19 deletions(-) diff --git a/lightning/src/lib.rs b/lightning/src/lib.rs index 1adf3786b76..c9ae3f26eeb 100644 --- a/lightning/src/lib.rs +++ b/lightning/src/lib.rs @@ -94,7 +94,9 @@ pub use std::io; pub use core2::io; #[cfg(not(feature = "std"))] -mod io_extras { +#[doc(hidden)] +/// IO utilities public only for use by in-crate macros. These should not be used externally +pub mod io_extras { use core2::io::{self, Read, Write}; /// A writer which will move data into the void. @@ -154,6 +156,8 @@ mod io_extras { } #[cfg(feature = "std")] +#[doc(hidden)] +/// IO utilities public only for use by in-crate macros. These should not be used externally mod io_extras { pub fn read_to_end(mut d: D) -> Result, ::std::io::Error> { let mut buf = Vec::new(); diff --git a/lightning/src/util/ser_macros.rs b/lightning/src/util/ser_macros.rs index 5d6988ba1f1..d1e152feb09 100644 --- a/lightning/src/util/ser_macros.rs +++ b/lightning/src/util/ser_macros.rs @@ -354,25 +354,25 @@ macro_rules! _check_missing_tlv { #[doc(hidden)] #[macro_export] macro_rules! _decode_tlv { - ($reader: expr, $field: ident, (default_value, $default: expr)) => {{ - $crate::_decode_tlv!($reader, $field, required) + ($outer_reader: expr, $reader: expr, $field: ident, (default_value, $default: expr)) => {{ + $crate::_decode_tlv!($outer_reader, $reader, $field, required) }}; - ($reader: expr, $field: ident, (static_value, $value: expr)) => {{ + ($outer_reader: expr, $reader: expr, $field: ident, (static_value, $value: expr)) => {{ }}; - ($reader: expr, $field: ident, required) => {{ + ($outer_reader: expr, $reader: expr, $field: ident, required) => {{ $field = $crate::util::ser::Readable::read(&mut $reader)?; }}; - ($reader: expr, $field: ident, (required: $trait: ident $(, $read_arg: expr)?)) => {{ + ($outer_reader: expr, $reader: expr, $field: ident, (required: $trait: ident $(, $read_arg: expr)?)) => {{ $field = $trait::read(&mut $reader $(, $read_arg)*)?; }}; - ($reader: expr, $field: ident, required_vec) => {{ + ($outer_reader: expr, $reader: expr, $field: ident, required_vec) => {{ let f: $crate::util::ser::WithoutLength> = $crate::util::ser::Readable::read(&mut $reader)?; $field = f.0; }}; - ($reader: expr, $field: ident, option) => {{ + ($outer_reader: expr, $reader: expr, $field: ident, option) => {{ $field = Some($crate::util::ser::Readable::read(&mut $reader)?); }}; - ($reader: expr, $field: ident, optional_vec) => {{ + ($outer_reader: expr, $reader: expr, $field: ident, optional_vec) => {{ let f: $crate::util::ser::WithoutLength> = $crate::util::ser::Readable::read(&mut $reader)?; $field = Some(f.0); }}; @@ -380,32 +380,41 @@ macro_rules! _decode_tlv { // without backwards compat. We'll error if the field is missing, and return `Ok(None)` if the // field is present but we can no longer understand it. // Note that this variant can only be used within a `MaybeReadable` read. - ($reader: expr, $field: ident, upgradable_required) => {{ + ($outer_reader: expr, $reader: expr, $field: ident, upgradable_required) => {{ $field = match $crate::util::ser::MaybeReadable::read(&mut $reader)? { Some(res) => res, - _ => return Ok(None) + None => { + // If we successfully read a value but we don't know how to parse it, we give up + // and immediately return `None`. However, we need to make sure we read the correct + // number of bytes for this TLV stream, which is implicitly the end of the stream. + // Thus, we consume everything left in the `$outer_reader` here, ensuring that if + // we're being read as a part of another TLV stream we don't spuriously fail to + // deserialize the outer object due to a TLV length mismatch. + $crate::io_extras::copy($outer_reader, &mut $crate::io_extras::sink()).unwrap(); + return Ok(None) + }, }; }}; // `upgradable_option` indicates we're reading an Option-al TLV that may have been upgraded // without backwards compat. $field will be None if the TLV is missing or if the field is present // but we can no longer understand it. - ($reader: expr, $field: ident, upgradable_option) => {{ + ($outer_reader: expr, $reader: expr, $field: ident, upgradable_option) => {{ $field = $crate::util::ser::MaybeReadable::read(&mut $reader)?; }}; - ($reader: expr, $field: ident, (option: $trait: ident $(, $read_arg: expr)?)) => {{ + ($outer_reader: expr, $reader: expr, $field: ident, (option: $trait: ident $(, $read_arg: expr)?)) => {{ $field = Some($trait::read(&mut $reader $(, $read_arg)*)?); }}; - ($reader: expr, $field: ident, (option, encoding: ($fieldty: ty, $encoding: ident, $encoder:ty))) => {{ - $crate::_decode_tlv!($reader, $field, (option, encoding: ($fieldty, $encoding))); + ($outer_reader: expr, $reader: expr, $field: ident, (option, encoding: ($fieldty: ty, $encoding: ident, $encoder:ty))) => {{ + $crate::_decode_tlv!($outer_reader, $reader, $field, (option, encoding: ($fieldty, $encoding))); }}; - ($reader: expr, $field: ident, (option, encoding: ($fieldty: ty, $encoding: ident))) => {{ + ($outer_reader: expr, $reader: expr, $field: ident, (option, encoding: ($fieldty: ty, $encoding: ident))) => {{ $field = { let field: $encoding<$fieldty> = ser::Readable::read(&mut $reader)?; Some(field.0) }; }}; - ($reader: expr, $field: ident, (option, encoding: $fieldty: ty)) => {{ - $crate::_decode_tlv!($reader, $field, option); + ($outer_reader: expr, $reader: expr, $field: ident, (option, encoding: $fieldty: ty)) => {{ + $crate::_decode_tlv!($outer_reader, $reader, $field, option); }}; } @@ -539,7 +548,7 @@ macro_rules! _decode_tlv_stream_range { let mut s = ser::FixedLengthReader::new(&mut stream_ref, length.0); match typ.0 { $(_t if $crate::_decode_tlv_stream_match_check!(_t, $type, $fieldty) => { - $crate::_decode_tlv!(s, $field, $fieldty); + $crate::_decode_tlv!($stream, s, $field, $fieldty); if s.bytes_remain() { s.eat_remaining()?; // Return ShortRead if there's actually not enough bytes return Err(DecodeError::InvalidValue); From f852d16d924a9c547c8c4995a8e61cda711711af Mon Sep 17 00:00:00 2001 From: Matt Corallo Date: Thu, 28 Mar 2024 22:06:36 +0000 Subject: [PATCH 3/3] Allow `MaybeReadable` to not fully read in `upgradable_option` Whils this is generally not supported, issues in our `MaybeReadable` implementations may occur, and we should try to be robust against them. --- lightning/src/util/ser_macros.rs | 11 +++++++++++ 1 file changed, 11 insertions(+) diff --git a/lightning/src/util/ser_macros.rs b/lightning/src/util/ser_macros.rs index d1e152feb09..e1f4762ecbb 100644 --- a/lightning/src/util/ser_macros.rs +++ b/lightning/src/util/ser_macros.rs @@ -400,6 +400,17 @@ macro_rules! _decode_tlv { // but we can no longer understand it. ($outer_reader: expr, $reader: expr, $field: ident, upgradable_option) => {{ $field = $crate::util::ser::MaybeReadable::read(&mut $reader)?; + if $field.is_none() { + #[cfg(not(debug_assertions))] { + // In general, MaybeReadable implementations are required to consume all the bytes + // of the object even if they don't understand it, but due to a bug in the + // serialization format for `impl_writeable_tlv_based_enum_upgradable` we sometimes + // don't know how many bytes that is. In such cases, we'd like to spuriously allow + // TLV length mismatches, which we do here by calling `eat_remaining` so that the + // `s.bytes_remain()` check in `_decode_tlv_stream_range` doesn't fail. + $reader.eat_remaining()?; + } + } }}; ($outer_reader: expr, $reader: expr, $field: ident, (option: $trait: ident $(, $read_arg: expr)?)) => {{ $field = Some($trait::read(&mut $reader $(, $read_arg)*)?);