From 4dff2bbac78b34fa799c6d0fea9e1448e3227ee3 Mon Sep 17 00:00:00 2001 From: Tony Arcieri Date: Wed, 2 Jul 2025 13:46:39 -0600 Subject: [PATCH 1/2] Revert "der: remove `Reader::read_value` (#1887)" This reverts commit 5277fcfa0a30fbf9089bcd1222e348414d3e1b1b. --- der/src/asn1/internal_macros.rs | 6 +++--- der/src/decode.rs | 2 +- der/src/reader.rs | 14 +++++++++++++- 3 files changed, 17 insertions(+), 5 deletions(-) diff --git a/der/src/asn1/internal_macros.rs b/der/src/asn1/internal_macros.rs index e1667db1d..89adc9c84 100644 --- a/der/src/asn1/internal_macros.rs +++ b/der/src/asn1/internal_macros.rs @@ -151,8 +151,8 @@ macro_rules! impl_custom_class { return Err(reader.error(header.tag.non_canonical_error()).into()); } - // read_nested checks if header matches decoded length - let value = reader.read_nested(header.length, |reader| { + // read_value checks if header matches decoded length + let value = reader.read_value(header, |reader| { // Decode inner IMPLICIT value T::decode_value(reader, header) })?; @@ -192,7 +192,7 @@ macro_rules! impl_custom_class { Tag::$class_enum_name { number, .. } => Ok(Self { tag_number: number, tag_mode: TagMode::default(), - value: reader.read_nested(header.length, |reader| { + value: reader.read_value(header, |reader| { // Decode inner tag-length-value of EXPLICIT T::decode(reader) })?, diff --git a/der/src/decode.rs b/der/src/decode.rs index 4129a0ec4..a44a9687f 100644 --- a/der/src/decode.rs +++ b/der/src/decode.rs @@ -68,7 +68,7 @@ where fn decode>(reader: &mut R) -> Result>::Error> { let header = Header::decode(reader)?; header.tag.assert_eq(T::TAG)?; - reader.read_nested(header.length, |r| T::decode_value(r, header)) + reader.read_value(header, |r| T::decode_value(r, header)) } } diff --git a/der/src/reader.rs b/der/src/reader.rs index 9630d8dcc..0bbeeb566 100644 --- a/der/src/reader.rs +++ b/der/src/reader.rs @@ -32,6 +32,18 @@ pub trait Reader<'r>: Clone { E: From, F: FnOnce(&mut Self) -> Result; + /// Read a value (i.e. the "V" part of a "TLV" field) using the provided header. + /// + /// This calls the provided function `f` with a nested reader created using + /// [`Reader::read_nested`]. + fn read_value(&mut self, header: Header, f: F) -> Result + where + E: From, + F: FnOnce(&mut Self) -> Result, + { + self.read_nested(header.length, f) + } + /// Attempt to read data borrowed directly from the input as a slice, /// updating the internal cursor position. /// @@ -192,7 +204,7 @@ pub trait Reader<'r>: Clone { { let header = Header::decode(self)?; header.tag.assert_eq(Tag::Sequence)?; - self.read_nested(header.length, f) + self.read_value(header, f) } /// Obtain a slice of bytes containing a complete TLV production suitable for parsing later. From 586e4c4bc782bc875d3201cfb4adbcd28b0b8af4 Mon Sep 17 00:00:00 2001 From: Tony Arcieri Date: Wed, 2 Jul 2025 14:40:35 -0600 Subject: [PATCH 2/2] der: (re-)add `Reader::read_value` with EOC support One of the blockers for addressing indefinite length handling (#779) has been where to consume the EOC tag. This commit (re)introduces `Reader::read_value` (added in #1877, removed in #1887) and handles decoding the EOC there. This gives us a single place where EOC can be handled for all constructed messages. With these changes, the decoder is able to parse `cms_ber.bin` from `cms/tests`, from which the `cms_der.bin` file has been translated. This file provides a real-world example of nested indefinite lengths from CMS. Note, however, that the example contains a constructed `Any` which isn't yet being correctly handled. A `TODO` for ensuring the BER and DER decode identically has been added. --- cms/tests/tests_from_pkcs7_crate.rs | 11 +++++ der/src/length.rs | 66 +++++++++++++++++++---------- der/src/reader.rs | 11 ++++- 3 files changed, 64 insertions(+), 24 deletions(-) diff --git a/cms/tests/tests_from_pkcs7_crate.rs b/cms/tests/tests_from_pkcs7_crate.rs index e422b141f..bcd5c6723 100644 --- a/cms/tests/tests_from_pkcs7_crate.rs +++ b/cms/tests/tests_from_pkcs7_crate.rs @@ -141,3 +141,14 @@ fn cms_decode_signed_der() { // should match the original assert_eq!(reencoded_der_signed_data_in_ci, der_signed_data_in_ci) } + +#[test] +fn cms_decode_signed_ber() { + let cms_ber = include_bytes!("../tests/examples/cms_ber.bin"); + let _ci_ber = ContentInfo::from_ber(cms_ber).unwrap(); + + // TODO(tarcieri): ensure BER and DER decode identically + // let cms_der = include_bytes!("../tests/examples/cms_der.bin"); + // let ci_der = ContentInfo::from_der(cms_der).unwrap(); + // assert_eq!(ci_ber, ci_der); +} diff --git a/der/src/length.rs b/der/src/length.rs index a6627e33f..4ad53f41c 100644 --- a/der/src/length.rs +++ b/der/src/length.rs @@ -40,6 +40,9 @@ impl Length { /// Maximum length (`u32::MAX`). pub const MAX: Self = Self::new(u32::MAX); + /// Length of end-of-content octets (i.e. `00 00`). + pub(crate) const EOC_LEN: Self = Self::new(2); + /// Maximum number of octets in a DER encoding of a [`Length`] using the /// rules implemented by this crate. pub(crate) const MAX_SIZE: usize = 5; @@ -92,6 +95,26 @@ impl Length { Self::new(self.inner.saturating_sub(rhs.inner)) } + /// If the length is indefinite, compute a length with the EOC marker removed + /// (i.e. the final two bytes `00 00`). + /// + /// Otherwise (as should always be the case with DER), the length is unchanged. + /// + /// This method notably preserves the `indefinite` flag when performing arithmetic. + pub(crate) fn sans_eoc(self) -> Self { + if self.indefinite { + // We expect EOC to be present when this is called. + debug_assert!(self >= Self::EOC_LEN); + + Self { + inner: self.saturating_sub(Self::EOC_LEN).inner, + indefinite: true, + } + } else { + self + } + } + /// Get initial octet of the encoded length (if one is required). /// /// From X.690 Section 8.1.3.5: @@ -379,22 +402,12 @@ fn decode_indefinite_length<'a, R: Reader<'a>>(reader: &mut R) -> Result let start_pos = reader.position(); loop { - let current_pos = reader.position(); - // Look for the end-of-contents marker if reader.peek_byte() == Some(EOC_TAG) { - // Drain the end-of-contents tag - reader.drain(Length::ONE)?; - - // Read the length byte and ensure it's zero (i.e. the full EOC is `00 00`) - let length_byte = reader.read_byte()?; - - if length_byte != 0 { - return Err(reader.error(ErrorKind::IndefiniteLength)); - } + read_eoc(reader)?; // Compute how much we read and flag the decoded length as indefinite - let mut ret = (current_pos - start_pos)?; + let mut ret = (reader.position() - start_pos)?; ret.indefinite = true; return Ok(ret); } @@ -404,6 +417,21 @@ fn decode_indefinite_length<'a, R: Reader<'a>>(reader: &mut R) -> Result } } +/// Read an expected end-of-contents (EOC) marker: `00 00`. +/// +/// # Errors +/// +/// - Returns `ErrorKind::IndefiniteLength` if the EOC marker isn't present as expected. +pub(crate) fn read_eoc<'a>(reader: &mut impl Reader<'a>) -> Result<()> { + for _ in 0..Length::EOC_LEN.inner as usize { + if reader.read_byte()? != 0 { + return Err(reader.error(ErrorKind::IndefiniteLength)); + } + } + + Ok(()) +} + #[cfg(test)] #[allow(clippy::unwrap_used)] mod tests { @@ -507,9 +535,6 @@ mod tests { /// Length of example in octets. const EXAMPLE_LEN: usize = 68; - /// Length of end-of-content octets (i.e. `00 00`). - const EOC_LEN: usize = 2; - /// Test vector from: /// /// Notably this example contains nested indefinite lengths to ensure the decoder handles @@ -534,7 +559,7 @@ mod tests { // Decode indefinite length let length = Length::decode(&mut reader).unwrap(); - assert!(length.indefinite); + assert!(length.is_indefinite()); // Decoding the length should leave the position at the end of the indefinite length octet let pos = usize::try_from(reader.position()).unwrap(); @@ -542,10 +567,7 @@ mod tests { // The first two bytes are the header and the rest is the length of the message. // The last four are two end-of-content markers (2 * 2 bytes). - assert_eq!( - usize::try_from(length).unwrap(), - EXAMPLE_LEN - pos - (EOC_LEN * 2) - ); + assert_eq!(usize::try_from(length).unwrap(), EXAMPLE_LEN - pos); // Read OID reader.tlv_bytes().unwrap(); @@ -564,7 +586,7 @@ mod tests { // Parse the inner indefinite length let length = Length::decode(&mut reader).unwrap(); - assert!(length.indefinite); - assert_eq!(usize::try_from(length).unwrap(), 18); + assert!(length.is_indefinite()); + assert_eq!(usize::try_from(length).unwrap(), 20); } } diff --git a/der/src/reader.rs b/der/src/reader.rs index 0bbeeb566..fc748657f 100644 --- a/der/src/reader.rs +++ b/der/src/reader.rs @@ -9,7 +9,7 @@ mod position; use crate::{ Decode, DecodeValue, Encode, EncodingRules, Error, ErrorKind, FixedTag, Header, Length, Tag, - TagMode, TagNumber, asn1::ContextSpecific, + TagMode, TagNumber, asn1::ContextSpecific, length::read_eoc, }; #[cfg(feature = "alloc")] @@ -41,7 +41,14 @@ pub trait Reader<'r>: Clone { E: From, F: FnOnce(&mut Self) -> Result, { - self.read_nested(header.length, f) + let ret = self.read_nested(header.length.sans_eoc(), f)?; + + // Consume EOC marker if the length is indefinite. + if header.length.is_indefinite() { + read_eoc(self)?; + } + + Ok(ret) } /// Attempt to read data borrowed directly from the input as a slice,