Negative test case for constructed, definite-length octet string (#1931)

dwhjames · web-flow · commit 858f3af9df83 · 2025-07-31T11:24:18.000-06:00
Ensure octet strings with constructed, definite-length method
return an error, rather than an incorrect decoding.
diff --git a/Cargo.lock b/Cargo.lock
diff --git a/Cargo.toml b/Cargo.toml
@@ -59,3 +59,5 @@ tls_codec_derive = { path = "./tls_codec/derive" }
 x509-tsp = { path = "./x509-tsp" }
 x509-cert = { path = "./x509-cert" }
 x509-ocsp = { path = "./x509-ocsp" }
+
+ecdsa = { git = "https://github.com/dwhjames/RustCrypto-signatures", branch = "tmp_der_header_refactor" }
diff --git a/der/src/asn1/any.rs b/der/src/asn1/any.rs
@@ -56,10 +56,7 @@ impl<'a> AnyRef<'a> {
 
     /// Returns [`Tag`] and [`Length`] of self.
     pub fn header(&self) -> Header {
-        Header {
-            tag: self.tag,
-            length: self.value.len(),
-        }
+        Header::new(self.tag, self.value.len())
     }
 
     /// Attempt to decode this [`AnyRef`] type into the inner value.
@@ -128,7 +125,7 @@ impl<'a> DecodeValue<'a> for AnyRef<'a> {
 
     fn decode_value<R: Reader<'a>>(reader: &mut R, header: Header) -> Result<Self, Error> {
         Ok(Self {
-            tag: header.tag,
+            tag: header.tag(),
             value: <&'a BytesRef>::decode_value(reader, header)?,
         })
     }
@@ -207,10 +204,7 @@ mod allocating {
 
         /// Returns [`Tag`] and [`Length`] of self.
         pub fn header(&self) -> Header {
-            Header {
-                tag: self.tag,
-                length: self.value.len(),
-            }
+            Header::new(self.tag, self.value.len())
         }
 
         /// Attempt to decode this [`Any`] type into the inner value.
@@ -290,7 +284,7 @@ mod allocating {
 
         fn decode_value<R: Reader<'a>>(reader: &mut R, header: Header) -> Result<Self, Error> {
             Ok(Self {
-                tag: header.tag,
+                tag: header.tag(),
                 value: BytesOwned::decode_value(reader, header)?,
             })
         }
diff --git a/der/src/asn1/bit_string.rs b/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.with_length((header.length() - Length::ONE)?);
 
         let unused_bits = reader.read_byte()?;
         let inner = <&'a BytesRef>::decode_value(reader, header)?;
@@ -354,7 +351,7 @@ mod allocating {
         type Error = Error;
 
         fn decode_value<R: Reader<'a>>(reader: &mut R, header: Header) -> Result<Self> {
-            let inner_len = (header.length - Length::ONE)?;
+            let inner_len = (header.length() - Length::ONE)?;
             let unused_bits = reader.read_byte()?;
             let inner = reader.read_vec(inner_len)?;
             Self::new(unused_bits, inner)
diff --git a/der/src/asn1/bmp_string.rs b/der/src/asn1/bmp_string.rs
@@ -93,7 +93,7 @@ impl<'a> DecodeValue<'a> for BmpString {
     type Error = Error;
 
     fn decode_value<R: Reader<'a>>(reader: &mut R, header: Header) -> Result<Self> {
-        Self::from_ucs2(reader.read_vec(header.length)?)
+        Self::from_ucs2(reader.read_vec(header.length())?)
     }
 }
 
diff --git a/der/src/asn1/boolean.rs b/der/src/asn1/boolean.rs
@@ -18,7 +18,7 @@ impl<'a> DecodeValue<'a> for bool {
     type Error = Error;
 
     fn decode_value<R: Reader<'a>>(reader: &mut R, header: Header) -> Result<Self> {
-        if header.length != Length::ONE {
+        if header.length() != Length::ONE {
             return Err(reader.error(ErrorKind::Length { tag: Self::TAG }));
         }
 
diff --git a/der/src/asn1/generalized_time.rs b/der/src/asn1/generalized_time.rs
@@ -78,7 +78,7 @@ impl<'a> DecodeValue<'a> for GeneralizedTime {
     type Error = Error;
 
     fn decode_value<R: Reader<'a>>(reader: &mut R, header: Header) -> Result<Self> {
-        if Self::LENGTH != usize::try_from(header.length)? {
+        if Self::LENGTH != usize::try_from(header.length())? {
             return Err(reader.error(Self::TAG.value_error()));
         }
 
diff --git a/der/src/asn1/integer/int.rs b/der/src/asn1/integer/int.rs
@@ -18,7 +18,7 @@ macro_rules! impl_encoding_traits {
 
                 fn decode_value<R: Reader<'a>>(reader: &mut R, header: Header) -> $crate::Result<Self> {
                     let mut buf = [0u8; Self::BITS as usize / 8];
-                    let max_length = u32::from(header.length) as usize;
+                    let max_length = u32::from(header.length()) as usize;
 
                     if max_length == 0 {
                         return Err(reader.error(Tag::Integer.length_error()));
@@ -39,7 +39,7 @@ macro_rules! impl_encoding_traits {
                     };
 
                     // Ensure we compute the same encoded length as the original any value
-                    if header.length != result.value_len()? {
+                    if header.length() != result.value_len()? {
                         return Err(reader.error(Self::TAG.non_canonical_error()));
                     }
 
@@ -143,7 +143,7 @@ impl<'a> DecodeValue<'a> for IntRef<'a> {
         let result = Self::new(bytes.as_slice())?;
 
         // Ensure we compute the same encoded length as the original any value.
-        if result.value_len()? != header.length {
+        if result.value_len()? != header.length() {
             return Err(reader.error(Self::TAG.non_canonical_error()));
         }
 
@@ -237,7 +237,7 @@ mod allocating {
             let result = Self::new(bytes.as_slice())?;
 
             // Ensure we compute the same encoded length as the original any value.
-            if result.value_len()? != header.length {
+            if result.value_len()? != header.length() {
                 return Err(reader.error(Self::TAG.non_canonical_error()));
             }
 
diff --git a/der/src/asn1/integer/uint.rs b/der/src/asn1/integer/uint.rs
@@ -22,7 +22,7 @@ macro_rules! impl_encoding_traits {
                     const UNSIGNED_HEADROOM: usize = 1;
 
                     let mut buf = [0u8; (Self::BITS as usize / 8) + UNSIGNED_HEADROOM];
-                    let max_length = u32::from(header.length) as usize;
+                    let max_length = u32::from(header.length()) as usize;
 
                     if max_length == 0 {
                         return Err(reader.error(Tag::Integer.length_error()));
@@ -36,7 +36,7 @@ macro_rules! impl_encoding_traits {
                     let result = Self::from_be_bytes(decode_to_array(bytes)?);
 
                     // Ensure we compute the same encoded length as the original any value
-                    if header.length != result.value_len()? {
+                    if header.length() != result.value_len()? {
                         return Err(reader.error(Self::TAG.non_canonical_error()));
                     }
 
@@ -126,7 +126,7 @@ impl<'a> DecodeValue<'a> for UintRef<'a> {
         let result = Self::new(decode_to_slice(bytes)?)?;
 
         // Ensure we compute the same encoded length as the original any value.
-        if result.value_len()? != header.length {
+        if result.value_len()? != header.length() {
             return Err(reader.error(Self::TAG.non_canonical_error()));
         }
 
@@ -221,7 +221,7 @@ mod allocating {
             let result = Self::new(decode_to_slice(bytes.as_slice())?)?;
 
             // Ensure we compute the same encoded length as the original any value.
-            if result.value_len()? != header.length {
+            if result.value_len()? != header.length() {
                 return Err(reader.error(Self::TAG.non_canonical_error()));
             }
 
diff --git a/der/src/asn1/internal_macros.rs b/der/src/asn1/internal_macros.rs
@@ -148,9 +148,9 @@ macro_rules! impl_custom_class {
                 let header = Header::decode(reader)?;
 
                 // the encoding shall be constructed if the base encoding is constructed
-                if header.tag.is_constructed() != T::CONSTRUCTED
+                if header.tag().is_constructed() != T::CONSTRUCTED
                     && reader.encoding_rules().is_der() {
-                    return Err(reader.error(header.tag.non_canonical_error()).into());
+                    return Err(reader.error(header.tag().non_canonical_error()).into());
                 }
 
                 // read_value checks if header matches decoded length
@@ -184,10 +184,10 @@ macro_rules! impl_custom_class {
                 let header = Header::decode(reader)?;
 
                 // encoding shall be constructed
-                if !header.tag.is_constructed() {
-                    return Err(reader.error(header.tag.non_canonical_error()).into());
+                if !header.tag().is_constructed() {
+                    return Err(reader.error(header.tag().non_canonical_error()).into());
                 }
-                match header.tag {
+                match header.tag() {
                     Tag::$class_enum_name { number, .. } => Ok(Self {
                         tag_number: number,
                         tag_mode: TagMode::default(),
diff --git a/der/src/asn1/null.rs b/der/src/asn1/null.rs
@@ -15,7 +15,7 @@ impl<'a> DecodeValue<'a> for Null {
     type Error = Error;
 
     fn decode_value<R: Reader<'a>>(reader: &mut R, header: Header) -> Result<Self> {
-        if header.length.is_zero() {
+        if header.length().is_zero() {
             Ok(Null)
         } else {
             Err(reader.error(ErrorKind::Length { tag: Self::TAG }))
diff --git a/der/src/asn1/octet_string.rs b/der/src/asn1/octet_string.rs
@@ -375,7 +375,25 @@ mod tests {
 
     #[test]
     #[cfg(all(feature = "alloc", feature = "ber"))]
-    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(all(feature = "alloc", feature = "ber"))]
+    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(all(feature = "alloc", feature = "ber"))]
+    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(all(feature = "alloc", feature = "ber"))]
     fn decode_context_specific_ber_explicit() {
diff --git a/der/src/asn1/oid.rs b/der/src/asn1/oid.rs
@@ -15,11 +15,11 @@ impl<'a, const MAX_SIZE: usize> DecodeValue<'a> for ObjectIdentifier<MAX_SIZE> {
     fn decode_value<R: Reader<'a>>(reader: &mut R, header: Header) -> Result<Self> {
         let mut buf = [0u8; MAX_SIZE];
         let slice = buf
-            .get_mut(..header.length.try_into()?)
+            .get_mut(..header.length().try_into()?)
             .ok_or_else(|| Self::TAG.length_error())?;
 
         let actual_len = reader.read_into(slice)?.len();
-        debug_assert_eq!(actual_len, header.length.try_into()?);
+        debug_assert_eq!(actual_len, header.length().try_into()?);
         Ok(ObjectIdentifierRef::from_bytes(slice)?.try_into()?)
     }
 }
diff --git a/der/src/asn1/real.rs b/der/src/asn1/real.rs
@@ -20,7 +20,7 @@ impl<'a> DecodeValue<'a> for f64 {
     fn decode_value<R: Reader<'a>>(reader: &mut R, header: Header) -> Result<Self> {
         let bytes = <&'a BytesRef>::decode_value(reader, header)?.as_slice();
 
-        if header.length == Length::ZERO {
+        if header.length() == Length::ZERO {
             Ok(0.0)
         } else if is_nth_bit_one::<7>(bytes) {
             // Binary encoding from section 8.5.7 applies
diff --git a/der/src/asn1/utc_time.rs b/der/src/asn1/utc_time.rs
@@ -85,7 +85,7 @@ impl<'a> DecodeValue<'a> for UtcTime {
     type Error = Error;
 
     fn decode_value<R: Reader<'a>>(reader: &mut R, header: Header) -> Result<Self> {
-        if Self::LENGTH != usize::try_from(header.length)? {
+        if Self::LENGTH != usize::try_from(header.length())? {
             return Err(reader.error(Self::TAG.value_error()));
         }
 
diff --git a/der/src/asn1/utf8_string.rs b/der/src/asn1/utf8_string.rs
@@ -141,7 +141,7 @@ impl<'a> DecodeValue<'a> for String {
     type Error = Error;
 
     fn decode_value<R: Reader<'a>>(reader: &mut R, header: Header) -> Result<Self> {
-        Ok(String::from_utf8(reader.read_vec(header.length)?)?)
+        Ok(String::from_utf8(reader.read_vec(header.length())?)?)
     }
 }
 
diff --git a/der/src/bytes.rs b/der/src/bytes.rs
@@ -70,7 +70,7 @@ impl<'a> DecodeValue<'a> for &'a BytesRef {
     type Error = Error;
 
     fn decode_value<R: Reader<'a>>(reader: &mut R, header: Header) -> Result<Self> {
-        BytesRef::new(reader.read_slice(header.length)?)
+        BytesRef::new(reader.read_slice(header.length())?)
     }
 }
 
@@ -118,7 +118,7 @@ impl<'a> arbitrary::Arbitrary<'a> for &'a BytesRef {
 pub(crate) mod allocating {
     use super::BytesRef;
     #[cfg(feature = "ber")]
-    use crate::length::indefinite::read_constructed_vec;
+    use crate::{ErrorKind, length::indefinite::read_constructed_vec};
 
     use crate::{
         DecodeValue, DerOrd, EncodeValue, Error, Header, Length, Reader, Result, Tag, Writer,
@@ -154,13 +154,24 @@ pub(crate) mod allocating {
             header: Header,
             inner_tag: Tag,
         ) -> Result<Self> {
-            // Reassemble indefinite length string types
             #[cfg(feature = "ber")]
-            if reader.encoding_rules().is_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().is_ber() {
+                    // Reassemble indefinite length string types
+                    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),
+                    ));
+                }
             }
 
             #[cfg(not(feature = "ber"))]
@@ -206,7 +217,7 @@ pub(crate) mod allocating {
         type Error = Error;
 
         fn decode_value<R: Reader<'a>>(reader: &mut R, header: Header) -> Result<Self> {
-            reader.read_vec(header.length).and_then(Self::new)
+            reader.read_vec(header.length()).and_then(Self::new)
         }
     }
 
diff --git a/der/src/decode.rs b/der/src/decode.rs
@@ -72,7 +72,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)?;
+        header.tag().assert_eq(T::TAG)?;
         read_value(reader, header, T::decode_value)
     }
 }
diff --git a/der/src/document.rs b/der/src/document.rs
@@ -149,7 +149,7 @@ impl<'a> Decode<'a> for Document {
 
     fn decode<R: Reader<'a>>(reader: &mut R) -> Result<Document, Error> {
         let header = Header::peek(reader)?;
-        let length = (header.encoded_len()? + header.length)?;
+        let length = (header.encoded_len()? + header.length())?;
         let bytes = reader.read_slice(length)?;
 
         Ok(Self {
@@ -324,9 +324,9 @@ impl ZeroizeOnDrop for SecretDocument {}
 /// entire sequence including the header.
 fn decode_sequence<'a>(decoder: &mut SliceReader<'a>) -> Result<&'a [u8], Error> {
     let header = Header::peek(decoder)?;
-    header.tag.assert_eq(Tag::Sequence)?;
+    header.tag().assert_eq(Tag::Sequence)?;
 
-    let len = (header.encoded_len()? + header.length)?;
+    let len = (header.encoded_len()? + header.length())?;
     decoder.read_slice(len)
 }
 
diff --git a/der/src/encode.rs b/der/src/encode.rs
diff --git a/der/src/header.rs b/der/src/header.rs
diff --git a/der/src/length/indefinite.rs b/der/src/length/indefinite.rs
diff --git a/der/src/reader.rs b/der/src/reader.rs
diff --git a/der/src/writer/slice.rs b/der/src/writer/slice.rs
diff --git a/pkcs5/src/pbes2/kdf/salt.rs b/pkcs5/src/pbes2/kdf/salt.rs
diff --git a/x509-cert/src/ext/pkix/name/dirstr.rs b/x509-cert/src/ext/pkix/name/dirstr.rs

Original file line number	Diff line number	Diff line change
`@@ -93,7 +93,7 @@ impl<'a> DecodeValue<'a> for BmpString {`
`93`	`93`	`type Error = Error;`
`94`	`94`
`95`	`95`	`fn decode_value<R: Reader<'a>>(reader: &mut R, header: Header) -> Result<Self> {`
`96`		`- Self::from_ucs2(reader.read_vec(header.length)?)`
	`96`	`+ Self::from_ucs2(reader.read_vec(header.length())?)`
`97`	`97`	`}`
`98`	`98`	`}`
`99`	`99`
Original file line number	Diff line number	Diff line change
`@@ -18,7 +18,7 @@ impl<'a> DecodeValue<'a> for bool {`
`18`	`18`	`type Error = Error;`
`19`	`19`
`20`	`20`	`fn decode_value<R: Reader<'a>>(reader: &mut R, header: Header) -> Result<Self> {`
`21`		`- if header.length != Length::ONE {`
	`21`	`+ if header.length() != Length::ONE {`
`22`	`22`	`return Err(reader.error(ErrorKind::Length { tag: Self::TAG }));`
`23`	`23`	`}`
`24`	`24`
Original file line number	Diff line number	Diff line change
`@@ -78,7 +78,7 @@ impl<'a> DecodeValue<'a> for GeneralizedTime {`
`78`	`78`	`type Error = Error;`
`79`	`79`
`80`	`80`	`fn decode_value<R: Reader<'a>>(reader: &mut R, header: Header) -> Result<Self> {`
`81`		`- if Self::LENGTH != usize::try_from(header.length)? {`
	`81`	`+ if Self::LENGTH != usize::try_from(header.length())? {`
`82`	`82`	`return Err(reader.error(Self::TAG.value_error()));`
`83`	`83`	`}`
`84`	`84`