der: fix handling of non-unique items in SetOf(Vec) (#1066)

The `TryFrom` constructors were not properly tested for the handling of
duplicate items and allowed them to be inserted.

This commit fixes the handling and adds tests. In the event there are
duplicates in a set, a newly added `ErrorKind::SetDuplicate` variant is
returned with the error type.

Author: tarcieri

der/src/asn1/set_of.rs

@@ -51,8 +51,10 @@ where
     pub fn add(&mut self, new_elem: T) -> Result<()> {
         // Ensure set elements are lexicographically ordered
         if let Some(last_elem) = self.inner.last() {
-            if new_elem.der_cmp(last_elem)? != Ordering::Greater {
-                return Err(ErrorKind::SetOrdering.into());
+            match new_elem.der_cmp(last_elem)? {
+                Ordering::Less => return Err(ErrorKind::SetOrdering.into()),
+                Ordering::Equal => return Err(ErrorKind::SetDuplicate.into()),
+                Ordering::Greater => (),
             }
         }
 
@@ -360,7 +362,6 @@ where
     type Error = Error;
 
     fn try_from(mut vec: Vec<T>) -> Result<SetOfVec<T>> {
-        // TODO(tarcieri): use `[T]::sort_by` here?
         der_sort(vec.as_mut_slice())?;
         Ok(SetOfVec { inner: vec })
     }
@@ -422,9 +423,15 @@ fn der_sort<T: DerOrd>(slice: &mut [T]) -> Result<()> {
     for i in 0..slice.len() {
         let mut j = i;
 
-        while j > 0 && slice[j - 1].der_cmp(&slice[j])? == Ordering::Greater {
-            slice.swap(j - 1, j);
-            j -= 1;
+        while j > 0 {
+            match slice[j - 1].der_cmp(&slice[j])? {
+                Ordering::Less => break,
+                Ordering::Equal => return Err(ErrorKind::SetDuplicate.into()),
+                Ordering::Greater => {
+                    slice.swap(j - 1, j);
+                    j -= 1;
+                }
+            }
         }
     }
 
@@ -452,21 +459,28 @@ fn validate<T: DerOrd>(slice: &[T]) -> Result<()> {
     Ok(())
 }
 
-#[cfg(all(test, feature = "alloc"))]
+#[cfg(test)]
 mod tests {
-    use super::{SetOf, SetOfVec};
-    use alloc::vec::Vec;
+    use super::SetOf;
+    #[cfg(feature = "alloc")]
+    use super::SetOfVec;
+    use crate::ErrorKind;
 
     #[test]
     fn setof_tryfrom_array() {
         let arr = [3u16, 2, 1, 65535, 0];
         let set = SetOf::try_from(arr).unwrap();
-        assert_eq!(
-            set.iter().cloned().collect::<Vec<u16>>(),
-            &[0, 1, 2, 3, 65535]
-        );
+        assert!(set.iter().copied().eq([0, 1, 2, 3, 65535]));
     }
 
+    #[test]
+    fn setof_tryfrom_array_reject_duplicates() {
+        let arr = [1u16, 1];
+        let err = SetOf::try_from(arr).err().unwrap();
+        assert_eq!(err.kind(), ErrorKind::SetDuplicate);
+    }
+
+    #[cfg(feature = "alloc")]
     #[test]
     fn setofvec_tryfrom_array() {
         let arr = [3u16, 2, 1, 65535, 0];
@@ -481,4 +495,12 @@ mod tests {
         let set = SetOfVec::try_from(vec).unwrap();
         assert_eq!(set.as_ref(), &[0, 1, 2, 3, 65535]);
     }
+
+    #[cfg(feature = "alloc")]
+    #[test]
+    fn setofvec_tryfrom_vec_reject_duplicates() {
+        let vec = vec![1u16, 1];
+        let err = SetOfVec::try_from(vec).err().unwrap();
+        assert_eq!(err.kind(), ErrorKind::SetDuplicate);
+    }
 }

der/src/error.rs

@@ -227,6 +227,9 @@ pub enum ErrorKind {
         oid: ObjectIdentifier,
     },
 
+    /// `SET` cannot contain duplicates.
+    SetDuplicate,
+
     /// `SET` ordering error: items not in canonical order.
     SetOrdering,
 
@@ -329,6 +332,7 @@ impl fmt::Display for ErrorKind {
             ErrorKind::OidUnknown { oid } => {
                 write!(f, "unknown/unsupported OID: {}", oid)
             }
+            ErrorKind::SetDuplicate => write!(f, "SET OF contains duplicate"),
             ErrorKind::SetOrdering => write!(f, "SET OF ordering error"),
             ErrorKind::Overflow => write!(f, "integer overflow"),
             ErrorKind::Overlength => write!(f, "ASN.1 DER message is too long"),

der/tests/set_of.rs

@@ -4,15 +4,21 @@
 
 use der::{asn1::SetOfVec, DerOrd};
 use proptest::{prelude::*, string::*};
+use std::collections::BTreeSet;
 
 proptest! {
     #[test]
     fn sort_equiv(bytes in bytes_regex(".{0,64}").unwrap()) {
-        let mut expected = bytes.clone();
-        expected.sort_by(|a, b| a.der_cmp(b).unwrap());
+        let mut uniq = BTreeSet::new();
 
-        let set = SetOfVec::try_from(bytes).unwrap();
-        prop_assert_eq!(expected.as_slice(), set.as_slice());
+        // Ensure there are no duplicates
+        if bytes.iter().copied().all(move |x| uniq.insert(x)) {
+            let mut expected = bytes.clone();
+            expected.sort_by(|a, b| a.der_cmp(b).unwrap());
+
+            let set = SetOfVec::try_from(bytes).unwrap();
+            prop_assert_eq!(expected.as_slice(), set.as_slice());
+        }
     }
 }