Negative test case for constructed, definite-length octet string

Ensure octet strings with constructed, definite-length method
return an error, rather than an incorrect decoding.

Author: dwhjames

der/src/asn1/any.rs

@@ -77,10 +77,7 @@ impl<'a> AnyRef<'a> {
             return Err(self.tag.unexpected_error(None).to_error().into());
         }
 
-        let header = Header {
-            tag: self.tag,
-            length: self.value.len(),
-        };
+        let header = Header::new(self.tag, self.value.len())?;
 
         let mut decoder = SliceReader::new_with_encoding_rules(self.value(), encoding)?;
         let result = T::decode_value(&mut decoder, header)?;

der/src/asn1/bit_string.rs

@@ -139,10 +139,7 @@ impl<'a> DecodeValue<'a> for BitStringRef<'a> {
     type Error = Error;
 
     fn decode_value<R: Reader<'a>>(reader: &mut R, header: Header) -> Result<Self> {
-        let header = Header {
-            tag: header.tag,
-            length: (header.length - Length::ONE)?,
-        };
+        let header = header.copy_with_length((header.length - Length::ONE)?)?;
 
         let unused_bits = reader.read_byte()?;
         let inner = <&'a BytesRef>::decode_value(reader, header)?;

der/src/asn1/octet_string.rs

@@ -159,7 +159,7 @@ pub use self::allocating::OctetString;
 #[cfg(feature = "alloc")]
 mod allocating {
     use super::*;
-    use crate::{BytesOwned, referenced::*};
+    use crate::{referenced::*, BytesOwned};
     use alloc::{borrow::Cow, boxed::Box, vec::Vec};
 
     /// ASN.1 `OCTET STRING` type: owned form.
@@ -375,7 +375,25 @@ mod tests {
 
     #[test]
     #[cfg(feature = "alloc")]
-    fn decode_ber() {
+    fn decode_ber_primitive_definite() {
+        use crate::{Decode, asn1::OctetString};
+        use hex_literal::hex;
+
+        const EXAMPLE: &[u8] = &hex!(
+            "040c" // primitive definite length OCTET STRING
+            "48656c6c6f2c20776f726c64" // "Hello, world"
+        );
+
+        let decoded = OctetString::from_ber(EXAMPLE).unwrap();
+        assert_eq!(decoded.as_bytes(), b"Hello, world");
+
+        let decoded = OctetString::from_der(EXAMPLE).unwrap();
+        assert_eq!(decoded.as_bytes(), b"Hello, world");
+    }
+
+    #[test]
+    #[cfg(feature = "alloc")]
+    fn decode_ber_constructed_indefinite() {
         use crate::{Decode, asn1::OctetString};
         use hex_literal::hex;
 
@@ -390,6 +408,28 @@ mod tests {
         assert_eq!(decoded.as_bytes(), b"Hello, world");
     }
 
+    #[test]
+    #[cfg(feature = "alloc")]
+    fn decode_ber_constructed_definite() {
+        use crate::{Decode, Error, ErrorKind, Length, Tag, asn1::OctetString};
+        use hex_literal::hex;
+
+        const EXAMPLE_BER: &[u8] = &hex!(
+            "2410" // Constructed definite length OCTET STRING
+            "040648656c6c6f2c" // Segment containing "Hello,"
+            "040620776f726c64" // Segment containing " world"
+        );
+
+        let err = OctetString::from_ber(EXAMPLE_BER).err().unwrap();
+        let expected = Error::new(
+            ErrorKind::Noncanonical {
+                tag: Tag::OctetString,
+            },
+            Length::new(1),
+        );
+        assert_eq!(expected, err);
+    }
+
     #[test]
     #[cfg(feature = "alloc")]
     fn decode_context_specific_ber_explicit() {

der/src/bytes.rs

@@ -118,8 +118,8 @@ impl<'a> arbitrary::Arbitrary<'a> for &'a BytesRef {
 pub(crate) mod allocating {
     use super::BytesRef;
     use crate::{
-        DecodeValue, DerOrd, EncodeValue, EncodingRules, Error, Header, Length, Reader, Result,
-        Tag, Writer, length::indefinite::read_constructed_vec,
+        DecodeValue, DerOrd, EncodeValue, EncodingRules, Error, ErrorKind, Header, Length, Reader,
+        Result, Tag, Writer, length::indefinite::read_constructed_vec,
     };
     use alloc::{borrow::ToOwned, boxed::Box, vec::Vec};
     use core::{borrow::Borrow, cmp::Ordering, ops::Deref};
@@ -152,11 +152,22 @@ pub(crate) mod allocating {
             inner_tag: Tag,
         ) -> Result<Self> {
             // Reassemble indefinite length string types
-            if reader.encoding_rules() == EncodingRules::Ber
-                && header.length.is_indefinite()
-                && !inner_tag.is_constructed()
-            {
-                return Self::new(read_constructed_vec(reader, header.length, inner_tag)?);
+            if header.is_constructed() {
+                if header.length.is_indefinite() && reader.encoding_rules() == EncodingRules::Ber {
+                    return Self::new(read_constructed_vec(reader, header.length, inner_tag)?);
+                } else {
+                    // NOTE:
+                    // constructed strings with definite length unsupported
+                    // See discussion
+                    //   - https://github.com/RustCrypto/formats/issues/779#issuecomment-3049869721
+                    //
+                    // NOTE: this repositions the error to be at the end of the header
+                    // rather than at the beginning of the value
+                    return Err(Error::new(
+                        ErrorKind::Noncanonical { tag: header.tag },
+                        reader.position().saturating_sub(Length::ONE),
+                    ));
+                }
             }
 
             Self::decode_value(reader, header)

der/src/header.rs

@@ -13,6 +13,8 @@ pub struct Header {
 
     /// Length of the encoded value
     pub length: Length,
+
+    constructed: bool,
 }
 
 impl Header {
@@ -21,7 +23,22 @@ impl Header {
     /// Returns an error if the length exceeds the limits of [`Length`].
     pub fn new(tag: Tag, length: impl TryInto<Length>) -> Result<Self> {
         let length = length.try_into().map_err(|_| ErrorKind::Overflow)?;
-        Ok(Self { tag, length })
+        Ok(Self { tag, length , constructed: length.is_indefinite() })
+    }
+
+    /// TODO.
+    pub fn copy_with_length(&self, length: impl TryInto<Length>) -> Result<Self> {
+        let length = length.try_into().map_err(|_| ErrorKind::Overflow)?;
+        Ok(Self {
+            tag: self.tag,
+            length,
+            constructed: self.constructed
+        })
+    }
+
+    /// TODO.
+    pub fn is_constructed(&self) -> bool {
+        self.constructed
     }
 
     /// Peek forward in the reader, attempting to decode a [`Header`] at the current position.
@@ -51,7 +68,7 @@ impl<'a> Decode<'a> for Header {
             return Err(reader.error(ErrorKind::IndefiniteLength));
         }
 
-        Ok(Self { tag, length })
+        Ok(Self { tag, length, constructed: is_constructed })
     }
 }