der: remove decode-time SET OF ordering checks

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.

This commit builds an `ArrayVec` or `Vec` of the incoming decoded set
elements, and then unconditionally sorts it retroactively, ensuring all
`SET OF` types internally reflect the proper order, but out-of-order
`SET OF` messages are tolerated when coming off-the-wire.

The sort implementation is merge sort, which should be efficient for
already-in-order or mostly-in-order sets.

Author: tarcieri

der/src/arrayvec.rs

@@ -85,6 +85,18 @@ impl<T, const N: usize> ArrayVec<T, N> {
     }
 }
 
+impl<T, const N: usize> AsRef<[Option<T>]> for ArrayVec<T, N> {
+    fn as_ref(&self) -> &[Option<T>] {
+        &self.elements[..self.length]
+    }
+}
+
+impl<T, const N: usize> AsMut<[Option<T>]> for ArrayVec<T, N> {
+    fn as_mut(&mut self) -> &mut [Option<T>] {
+        &mut self.elements[..self.length]
+    }
+}
+
 impl<T, const N: usize> Default for ArrayVec<T, N> {
     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,
         }
     }
 

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<Self> {
         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<T: DerOrd>(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<T: DerOrd>(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};

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<Ordering> {
-            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::<AttributeTypeAndValue<'_>, 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::<AttributeTypeAndValue<'_>>::from_der(OUT_OF_ORDER_RDN_EXAMPLE).is_ok());
     }
 
     /// Test to ensure ordering is handled correctly.

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]