der: (re-)add Reader::read_value with EOC support (#1895)

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.

Author: tarcieri

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);
+}

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)
                         })?,

der/src/decode.rs

@@ -68,7 +68,7 @@ where
     fn decode<R: Reader<'a>>(reader: &mut R) -> Result<T, <T as DecodeValue<'a>>::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))
     }
 }
 

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<Length>
     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<Length>
     }
 }
 
+/// 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: <https://github.com/RustCrypto/formats/issues/779#issuecomment-2902948789>
         ///
         /// Notably this example contains nested indefinite lengths to ensure the decoder handles
@@ -534,18 +559,15 @@ 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();
         assert_eq!(pos, 2);
 
         // 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);
     }
 }

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")]
@@ -32,6 +32,25 @@ pub trait Reader<'r>: Clone {
         E: From<Error>,
         F: FnOnce(&mut Self) -> Result<T, E>;
 
+    /// 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<T, F, E>(&mut self, header: Header, f: F) -> Result<T, E>
+    where
+        E: From<Error>,
+        F: FnOnce(&mut Self) -> Result<T, E>,
+    {
+        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,
     /// updating the internal cursor position.
     ///
@@ -192,7 +211,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.