diff --git a/der/src/arrayvec.rs b/der/src/arrayvec.rs index 04fb81344..6e86ae7d2 100644 --- a/der/src/arrayvec.rs +++ b/der/src/arrayvec.rs @@ -85,6 +85,18 @@ impl ArrayVec { } } +impl AsRef<[Option]> for ArrayVec { + fn as_ref(&self) -> &[Option] { + &self.elements[..self.length] + } +} + +impl AsMut<[Option]> for ArrayVec { + fn as_mut(&mut self) -> &mut [Option] { + &mut self.elements[..self.length] + } +} + impl Default for ArrayVec { fn default() -> Self { Self::new() @@ -114,11 +126,12 @@ impl<'a, T> Iterator for Iter<'a, T> { type Item = &'a T; fn next(&mut self) -> Option<&'a T> { - if let Some(Some(res)) = self.elements.get(self.position) { - self.position = self.position.checked_add(1)?; - Some(res) - } else { - None + match self.elements.get(self.position) { + Some(Some(res)) => { + self.position = self.position.checked_add(1)?; + Some(res) + } + _ => None, } } diff --git a/der/src/asn1/set_of.rs b/der/src/asn1/set_of.rs index 00b88cc68..820bba179 100644 --- a/der/src/asn1/set_of.rs +++ b/der/src/asn1/set_of.rs @@ -1,4 +1,14 @@ //! ASN.1 `SET OF` support. +//! +//! # Ordering Notes +//! +//! Some DER serializer implementations fail to properly sort elements of a +//! `SET OF`. This is technically non-canonical, but occurs frequently +//! enough that most DER decoders tolerate it. Unfortunately because +//! of that, we must also follow suit. +//! +//! However, all types in this module sort elements of a set at decode-time, +//! ensuring they'll be in the proper order if reserialized. use crate::{ arrayvec, ord::iter_cmp, ArrayVec, Decode, DecodeValue, Decoder, DerOrd, Encode, EncodeValue, @@ -90,13 +100,15 @@ where let mut result = Self::new(); while decoder.position() < end_pos { - result.add(decoder.decode()?)?; + result.inner.add(decoder.decode()?)?; } if decoder.position() != end_pos { decoder.error(ErrorKind::Length { tag: Self::TAG }); } + der_sort(result.inner.as_mut())?; + validate(result.inner.as_ref())?; Ok(result) } } @@ -274,17 +286,19 @@ where { fn decode_value(decoder: &mut Decoder<'a>, header: Header) -> Result { let end_pos = (decoder.position() + header.length)?; - let mut result = Self::new(); + let mut inner = Vec::new(); while decoder.position() < end_pos { - result.add(decoder.decode()?)?; + inner.push(decoder.decode()?); } if decoder.position() != end_pos { decoder.error(ErrorKind::Length { tag: Self::TAG }); } - Ok(result) + der_sort(inner.as_mut())?; + validate(inner.as_ref())?; + Ok(Self { inner }) } } @@ -390,6 +404,27 @@ fn der_sort(slice: &mut [T]) -> Result<()> { Ok(()) } +/// Validate the elements of a `SET OF`, ensuring that they are all in order +/// and that there are no duplicates. +fn validate(slice: &[T]) -> Result<()> { + if let Some(len) = slice.len().checked_sub(1) { + for i in 0..len { + let j = i.checked_add(1).ok_or(ErrorKind::Overflow)?; + + match slice.get(i..=j) { + Some([a, b]) => { + if a.der_cmp(b)? != Ordering::Less { + return Err(ErrorKind::SetOrdering.into()); + } + } + _ => return Err(Tag::Set.value_error()), + } + } + } + + Ok(()) +} + #[cfg(all(test, feature = "alloc"))] mod tests { use super::{SetOf, SetOfVec}; diff --git a/der/tests/set_of.rs b/der/tests/set_of.rs index c2b699ca2..ab09bfe33 100644 --- a/der/tests/set_of.rs +++ b/der/tests/set_of.rs @@ -1,12 +1,10 @@ //! `SetOf` tests. -#[cfg(feature = "alloc")] -use { - der::{asn1::SetOfVec, DerOrd}, - proptest::{prelude::*, string::*}, -}; +#![cfg(feature = "alloc")] + +use der::{asn1::SetOfVec, DerOrd}; +use proptest::{prelude::*, string::*}; -#[cfg(feature = "alloc")] proptest! { #[test] fn sort_equiv(bytes in bytes_regex(".{0,64}").unwrap()) { @@ -18,36 +16,36 @@ proptest! { } } +/// Set ordering tests. #[cfg(all(feature = "derive", feature = "oid"))] -mod attr_regression { - #![cfg(all(feature = "derive", feature = "oid"))] - - use core::cmp::Ordering; +mod ordering { use der::{ - asn1::{Any, ObjectIdentifier, SetOf}, - Decode, Result, Sequence, ValueOrd, + asn1::{Any, ObjectIdentifier, SetOf, SetOfVec}, + Decode, Sequence, ValueOrd, }; use hex_literal::hex; - /// Attribute type/value pairs as defined in [RFC 5280 Section 4.1.2.4]. - /// - /// [RFC 5280 Section 4.1.2.4]: https://tools.ietf.org/html/rfc5280#section-4.1.2.4 - #[derive(Copy, Clone, Debug, Eq, PartialEq, PartialOrd, Ord, Sequence)] + /// X.501 `AttributeTypeAndValue` + #[derive(Copy, Clone, Debug, Eq, PartialEq, Sequence, ValueOrd)] pub struct AttributeTypeAndValue<'a> { - /// OID describing the type of the attribute pub oid: ObjectIdentifier, - - /// Value of the attribute pub value: Any<'a>, } - impl ValueOrd for AttributeTypeAndValue<'_> { - fn value_cmp(&self, other: &Self) -> Result { - match self.oid.value_cmp(&other.oid)? { - Ordering::Equal => self.value.value_cmp(&other.value), - other => Ok(other), - } - } + const OUT_OF_ORDER_RDN_EXAMPLE: &[u8] = + &hex!("311F301106035504030C0A4A4F484E20534D495448300A060355040A0C03313233"); + + /// For compatibility reasons, we allow non-canonical DER with out-of-order + /// sets in order to match the behavior of other implementations. + #[test] + fn allow_out_of_order_setof() { + assert!(SetOf::, 2>::from_der(OUT_OF_ORDER_RDN_EXAMPLE).is_ok()); + } + + /// Same as above, with `SetOfVec` instead of `SetOf`. + #[test] + fn allow_out_of_order_setofvec() { + assert!(SetOfVec::>::from_der(OUT_OF_ORDER_RDN_EXAMPLE).is_ok()); } /// Test to ensure ordering is handled correctly. diff --git a/x509/tests/name.rs b/x509/tests/name.rs index 94944be9e..d17ed506a 100644 --- a/x509/tests/name.rs +++ b/x509/tests/name.rs @@ -126,11 +126,11 @@ fn decode_rdn() { // fails when caller adds items not in DER lexicographical order assert!(from_scratch2.0.add(*atav1a).is_err()); - //parsing fails due to incorrect order + // allow out-of-order RDNs (see: RustCrypto/formats#625) assert!(RelativeDistinguishedName::from_der( &hex!("311F301106035504030C0A4A4F484E20534D495448300A060355040A0C03313233")[..], ) - .is_err()); + .is_ok()); } // #[test]