Remove subject common name parsing

src/der.rs

@@ -62,9 +62,7 @@ pub(crate) enum Tag {
     OctetString = 0x04,
     OID = 0x06,
     Enum = 0x0A,
-    UTF8String = 0x0C,
     Sequence = CONSTRUCTED | 0x10, // 0x30
-    Set = CONSTRUCTED | 0x11,      // 0x31
     UTCTime = 0x17,
     GeneralizedTime = 0x18,
 

src/end_entity.rs

@@ -165,3 +165,64 @@ impl<'a> EndEntityCert<'a> {
         subject_name::list_cert_dns_names(self)
     }
 }
+
+#[cfg(feature = "alloc")]
+#[cfg(test)]
+mod tests {
+    use super::*;
+    use crate::test_utils;
+
+    // This test reproduces https://github.com/rustls/webpki/issues/167 --- an
+    // end-entity cert where the common name is a `PrintableString` rather than
+    // a `UTF8String` cannot iterate over its subject alternative names.
+    #[test]
+    fn printable_string_common_name() {
+        const DNS_NAME: &str = "test.example.com";
+
+        let issuer = test_utils::make_issuer("Test", None);
+
+        let ee_cert_der = {
+            let mut params = rcgen::CertificateParams::new(vec![DNS_NAME.to_string()]);
+            // construct a certificate that uses `PrintableString` as the
+            // common name value, rather than `UTF8String`.
+            params.distinguished_name.push(
+                rcgen::DnType::CommonName,
+                rcgen::DnValue::PrintableString("example.com".to_string()),
+            );
+            params.is_ca = rcgen::IsCa::ExplicitNoCa;
+            params.alg = test_utils::RCGEN_SIGNATURE_ALG;
+            let cert = rcgen::Certificate::from_params(params)
+                .expect("failed to make ee cert (this is a test bug)");
+            let bytes = cert
+                .serialize_der_with_signer(&issuer)
+                .expect("failed to serialize signed ee cert (this is a test bug)");
+            CertificateDer::from(bytes)
+        };
+
+        expect_dns_name(&ee_cert_der, DNS_NAME);
+    }
+
+    // This test reproduces https://github.com/rustls/webpki/issues/167 --- an
+    // end-entity cert where the common name is an empty SEQUENCE.
+    #[test]
+    fn empty_sequence_common_name() {
+        let ee_cert_der = {
+            // handcrafted cert DER produced using `ascii2der`, since `rcgen` is
+            // unwilling to generate this particular weird cert.
+            let bytes = include_bytes!("../tests/misc/empty_sequence_common_name.der");
+            CertificateDer::from(&bytes[..])
+        };
+        expect_dns_name(&ee_cert_der, "example.com");
+    }
+
+    fn expect_dns_name(der: &CertificateDer<'_>, name: &str) {
+        let cert =
+            EndEntityCert::try_from(der).expect("should parse end entity certificate correctly");
+
+        let mut names = cert
+            .dns_names()
+            .expect("should get all DNS names correctly for end entity cert");
+        assert_eq!(names.next().map(<&str>::from), Some(name));
+        assert_eq!(names.next().map(<&str>::from), None);
+    }
+}

src/lib.rs

@@ -60,6 +60,9 @@ mod crl;
 mod verify_cert;
 mod x509;
 
+#[cfg(test)]
+pub(crate) mod test_utils;
+
 pub use {
     cert::{Cert, EndEntityOrCa},
     crl::{BorrowedCertRevocationList, BorrowedRevokedCert, CertRevocationList, RevocationReason},

src/subject_name/mod.rs

@@ -35,6 +35,4 @@ mod verify;
 pub(super) use verify::list_cert_dns_names;
 #[allow(unused_imports)] // TODO(@cpu): remove once used by cert module.
 pub(crate) use verify::GeneralName;
-pub(super) use verify::{
-    check_name_constraints, verify_cert_subject_name, SubjectCommonNameContents,
-};
+pub(super) use verify::{check_name_constraints, verify_cert_subject_name};

src/subject_name/verify.rs

@@ -31,29 +31,25 @@ pub(crate) fn verify_cert_dns_name(
 ) -> Result<(), Error> {
     let cert = cert.inner();
     let dns_name = untrusted::Input::from(dns_name.as_ref().as_bytes());
-    NameIterator::new(
-        Some(cert.subject),
-        cert.subject_alt_name,
-        SubjectCommonNameContents::Ignore,
-    )
-    .find_map(|result| {
-        let name = match result {
-            Ok(name) => name,
-            Err(err) => return Some(Err(err)),
-        };
+    NameIterator::new(Some(cert.subject), cert.subject_alt_name)
+        .find_map(|result| {
+            let name = match result {
+                Ok(name) => name,
+                Err(err) => return Some(Err(err)),
+            };
 
-        let presented_id = match name {
-            GeneralName::DnsName(presented) => presented,
-            _ => return None,
-        };
+            let presented_id = match name {
+                GeneralName::DnsName(presented) => presented,
+                _ => return None,
+            };
 
-        match dns_name::presented_id_matches_reference_id(presented_id, dns_name) {
-            Ok(true) => Some(Ok(())),
-            Ok(false) | Err(Error::MalformedDnsIdentifier) => None,
-            Err(e) => Some(Err(e)),
-        }
-    })
-    .unwrap_or(Err(Error::CertNotValidForName))
+            match dns_name::presented_id_matches_reference_id(presented_id, dns_name) {
+                Ok(true) => Some(Ok(())),
+                Ok(false) | Err(Error::MalformedDnsIdentifier) => None,
+                Err(e) => Some(Err(e)),
+            }
+        })
+        .unwrap_or(Err(Error::CertNotValidForName))
 }
 
 pub(crate) fn verify_cert_subject_name(
@@ -75,7 +71,6 @@ pub(crate) fn verify_cert_subject_name(
         // only against Subject Alternative Names.
         None,
         cert.inner().subject_alt_name,
-        SubjectCommonNameContents::Ignore,
     )
     .find_map(|result| {
         let name = match result {
@@ -100,7 +95,6 @@ pub(crate) fn verify_cert_subject_name(
 pub(crate) fn check_name_constraints(
     constraints: Option<&mut untrusted::Reader>,
     subordinate_certs: &Cert,
-    subject_common_name_contents: SubjectCommonNameContents,
     budget: &mut Budget,
 ) -> Result<(), Error> {
     let constraints = match constraints {
@@ -123,24 +117,20 @@ pub(crate) fn check_name_constraints(
 
     let mut child = subordinate_certs;
     loop {
-        let result = NameIterator::new(
-            Some(child.subject),
-            child.subject_alt_name,
-            subject_common_name_contents,
-        )
-        .find_map(|result| {
-            let name = match result {
-                Ok(name) => name,
-                Err(err) => return Some(Err(err)),
-            };
+        let result =
+            NameIterator::new(Some(child.subject), child.subject_alt_name).find_map(|result| {
+                let name = match result {
+                    Ok(name) => name,
+                    Err(err) => return Some(Err(err)),
+                };
 
-            check_presented_id_conforms_to_constraints(
-                name,
-                permitted_subtrees,
-                excluded_subtrees,
-                budget,
-            )
-        });
+                check_presented_id_conforms_to_constraints(
+                    name,
+                    permitted_subtrees,
+                    excluded_subtrees,
+                    budget,
+                )
+            });
 
         if let Some(Err(err)) = result {
             return Err(err);
@@ -282,37 +272,21 @@ enum Subtrees {
     ExcludedSubtrees,
 }
 
-#[derive(Clone, Copy)]
-pub(crate) enum SubjectCommonNameContents {
-    DnsName,
-    Ignore,
-}
-
 struct NameIterator<'a> {
     subject_alt_name: Option<untrusted::Reader<'a>>,
     subject_directory_name: Option<untrusted::Input<'a>>,
-    subject_common_name: Option<untrusted::Input<'a>>,
 }
 
 impl<'a> NameIterator<'a> {
     fn new(
         subject: Option<untrusted::Input<'a>>,
         subject_alt_name: Option<untrusted::Input<'a>>,
-        subject_common_name_contents: SubjectCommonNameContents,
     ) -> Self {
-        // If `subject` is present, we always consider it as a `DirectoryName`.
-        // We yield its common name only if the policy in `subject_common_name_contents` allows it.
-        let (subject_directory_name, subject_common_name) =
-            match (subject, subject_common_name_contents) {
-                (Some(input), SubjectCommonNameContents::DnsName) => (Some(input), Some(input)),
-                (Some(input), SubjectCommonNameContents::Ignore) => (Some(input), None),
-                (None, _) => (None, None),
-            };
-
         NameIterator {
             subject_alt_name: subject_alt_name.map(untrusted::Reader::new),
-            subject_directory_name,
-            subject_common_name,
+
+            // If `subject` is present, we always consider it as a `DirectoryName`.
+            subject_directory_name: subject,
         }
     }
 }
@@ -338,7 +312,6 @@ impl<'a> Iterator for NameIterator<'a> {
                 // Make sure we don't yield any items after this error.
                 self.subject_alt_name = None;
                 self.subject_directory_name = None;
-                self.subject_common_name = None;
                 return Some(Err(err));
             } else {
                 self.subject_alt_name = None;
@@ -349,15 +322,6 @@ impl<'a> Iterator for NameIterator<'a> {
             return Some(Ok(GeneralName::DirectoryName(subject_directory_name)));
         }
 
-        if let Some(subject_common_name) = self.subject_common_name.take() {
-            return match common_name(subject_common_name) {
-                Ok(Some(cn)) => Some(Ok(GeneralName::DnsName(cn))),
-                Ok(None) => None,
-                // All the iterator fields should be `None` at this point
-                Err(err) => Some(Err(err)),
-            };
-        }
-
         None
     }
 }
@@ -369,37 +333,33 @@ pub(crate) fn list_cert_dns_names<'names>(
     let cert = &cert.inner();
     let mut names = Vec::new();
 
-    let result = NameIterator::new(
-        Some(cert.subject),
-        cert.subject_alt_name,
-        SubjectCommonNameContents::DnsName,
-    )
-    .find_map(&mut |result| {
-        let name = match result {
-            Ok(name) => name,
-            Err(err) => return Some(err),
-        };
-
-        let presented_id = match name {
-            GeneralName::DnsName(presented) => presented,
-            _ => return None,
-        };
+    let result =
+        NameIterator::new(Some(cert.subject), cert.subject_alt_name).find_map(&mut |result| {
+            let name = match result {
+                Ok(name) => name,
+                Err(err) => return Some(err),
+            };
 
-        let dns_name = DnsNameRef::try_from_ascii(presented_id.as_slice_less_safe())
-            .map(GeneralDnsNameRef::DnsName)
-            .or_else(|_| {
-                WildcardDnsNameRef::try_from_ascii(presented_id.as_slice_less_safe())
-                    .map(GeneralDnsNameRef::Wildcard)
-            });
+            let presented_id = match name {
+                GeneralName::DnsName(presented) => presented,
+                _ => return None,
+            };
 
-        // if the name could be converted to a DNS name, add it; otherwise,
-        // keep going.
-        if let Ok(name) = dns_name {
-            names.push(name)
-        }
+            let dns_name = DnsNameRef::try_from_ascii(presented_id.as_slice_less_safe())
+                .map(GeneralDnsNameRef::DnsName)
+                .or_else(|_| {
+                    WildcardDnsNameRef::try_from_ascii(presented_id.as_slice_less_safe())
+                        .map(GeneralDnsNameRef::Wildcard)
+                });
+
+            // if the name could be converted to a DNS name, add it; otherwise,
+            // keep going.
+            if let Ok(name) = dns_name {
+                names.push(name)
+            }
 
-        None
-    });
+            None
+        });
 
     match result {
         Some(err) => Err(err),
@@ -457,33 +417,3 @@ impl<'a> FromDer<'a> for GeneralName<'a> {
 
     const TYPE_ID: DerTypeId = DerTypeId::GeneralName;
 }
-
-static COMMON_NAME: untrusted::Input = untrusted::Input::from(&[85, 4, 3]);
-
-fn common_name(input: untrusted::Input) -> Result<Option<untrusted::Input>, Error> {
-    let inner = &mut untrusted::Reader::new(input);
-    der::nested(
-        inner,
-        der::Tag::Set,
-        Error::TrailingData(DerTypeId::CommonNameOuter),
-        |tagged| {
-            der::nested(
-                tagged,
-                der::Tag::Sequence,
-                Error::TrailingData(DerTypeId::CommonNameInner),
-                |tagged| {
-                    while !tagged.at_end() {
-                        let name_oid = der::expect_tag(tagged, der::Tag::OID)?;
-                        if name_oid == COMMON_NAME {
-                            return der::expect_tag(tagged, der::Tag::UTF8String).map(Some);
-                        } else {
-                            // discard unused name value
-                            der::read_tag_and_get_value(tagged)?;
-                        }
-                    }
-                    Ok(None)
-                },
-            )
-        },
-    )
-}

src/test_utils.rs

@@ -0,0 +1,41 @@
+#[cfg(feature = "alloc")]
+use crate::types::CertificateDer;
+
+/// Signature algorithm used by certificates generated using `make_issuer` and
+/// `make_end_entity`. This is exported as a constant so that tests can use the
+/// same algorithm when generating certificates using `rcgen`.
+#[cfg(feature = "alloc")]
+pub(crate) static RCGEN_SIGNATURE_ALG: &rcgen::SignatureAlgorithm = &rcgen::PKCS_ECDSA_P256_SHA256;
+
+#[cfg(feature = "alloc")]
+pub(crate) fn make_issuer(
+    org_name: impl Into<String>,
+    name_constraints: Option<rcgen::NameConstraints>,
+) -> rcgen::Certificate {
+    let mut ca_params = rcgen::CertificateParams::new(Vec::new());
+    ca_params
+        .distinguished_name
+        .push(rcgen::DnType::OrganizationName, org_name);
+    ca_params.is_ca = rcgen::IsCa::Ca(rcgen::BasicConstraints::Unconstrained);
+    ca_params.key_usages = vec![
+        rcgen::KeyUsagePurpose::KeyCertSign,
+        rcgen::KeyUsagePurpose::DigitalSignature,
+        rcgen::KeyUsagePurpose::CrlSign,
+    ];
+    ca_params.alg = RCGEN_SIGNATURE_ALG;
+    ca_params.name_constraints = name_constraints;
+    rcgen::Certificate::from_params(ca_params).unwrap()
+}
+
+#[cfg(feature = "alloc")]
+pub(crate) fn make_end_entity(issuer: &rcgen::Certificate) -> CertificateDer<'static> {
+    let mut ee_params = rcgen::CertificateParams::new(vec!["example.com".to_string()]);
+    ee_params.is_ca = rcgen::IsCa::ExplicitNoCa;
+    ee_params.alg = RCGEN_SIGNATURE_ALG;
+    CertificateDer::from(
+        rcgen::Certificate::from_params(ee_params)
+            .unwrap()
+            .serialize_der_with_signer(issuer)
+            .unwrap(),
+    )
+}