diff --git a/src/der.rs b/src/der.rs index 4993275c..33ff6377 100644 --- a/src/der.rs +++ b/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, diff --git a/src/end_entity.rs b/src/end_entity.rs index 10f1f363..ab454bf2 100644 --- a/src/end_entity.rs +++ b/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); + } +} diff --git a/src/lib.rs b/src/lib.rs index 94ce26b8..507b547d 100644 --- a/src/lib.rs +++ b/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}, diff --git a/src/subject_name/mod.rs b/src/subject_name/mod.rs index ad87bb72..1eed404a 100644 --- a/src/subject_name/mod.rs +++ b/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}; diff --git a/src/subject_name/verify.rs b/src/subject_name/verify.rs index 011f9309..a0616a00 100644 --- a/src/subject_name/verify.rs +++ b/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>, subject_directory_name: Option>, - subject_common_name: Option>, } impl<'a> NameIterator<'a> { fn new( subject: Option>, subject_alt_name: Option>, - 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, 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) - }, - ) - }, - ) -} diff --git a/src/test_utils.rs b/src/test_utils.rs new file mode 100644 index 00000000..b30c12d7 --- /dev/null +++ b/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, + name_constraints: Option, +) -> 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(), + ) +} diff --git a/src/verify_cert.rs b/src/verify_cert.rs index af15e635..c706a620 100644 --- a/src/verify_cert.rs +++ b/src/verify_cert.rs @@ -159,20 +159,6 @@ fn build_chain_inner( } } - // for the purpose of name constraints checking, only end-entity server certificates - // could plausibly have a DNS name as a subject commonName that could contribute to - // path validity - let subject_common_name_contents = if opts - .eku - .inner - .key_purpose_id_equals(EKU_SERVER_AUTH.oid_value) - && used_as_ca == UsedAsCa::No - { - subject_name::SubjectCommonNameContents::DnsName - } else { - subject_name::SubjectCommonNameContents::Ignore - }; - let result = loop_while_non_fatal_error( Error::UnknownIssuer, opts.trust_anchors, @@ -192,12 +178,7 @@ fn build_chain_inner( budget, )?; - check_signed_chain_name_constraints( - cert, - trust_anchor, - subject_common_name_contents, - budget, - )?; + check_signed_chain_name_constraints(cert, trust_anchor, budget)?; Ok(()) }, @@ -287,7 +268,6 @@ fn check_signed_chain( fn check_signed_chain_name_constraints( cert_chain: &Cert, trust_anchor: &TrustAnchor, - subject_common_name_contents: subject_name::SubjectCommonNameContents, budget: &mut Budget, ) -> Result<(), Error> { let mut cert = cert_chain; @@ -298,7 +278,7 @@ fn check_signed_chain_name_constraints( loop { untrusted::read_all_optional(name_constraints, Error::BadDer, |value| { - subject_name::check_name_constraints(value, cert, subject_common_name_contents, budget) + subject_name::check_name_constraints(value, cert, budget) })?; match &cert.ee_or_ca { @@ -758,6 +738,8 @@ where #[cfg(test)] mod tests { use super::*; + #[cfg(feature = "alloc")] + use crate::test_utils::{make_end_entity, make_issuer}; use crate::BorrowedCertRevocationList; #[test] @@ -1080,37 +1062,4 @@ mod tests { time, ) } - - #[cfg(feature = "alloc")] - fn make_issuer( - org_name: impl Into, - name_constraints: Option, - ) -> 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::PKCS_ECDSA_P256_SHA256; - ca_params.name_constraints = name_constraints; - rcgen::Certificate::from_params(ca_params).unwrap() - } - - #[cfg(feature = "alloc")] - 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::PKCS_ECDSA_P256_SHA256; - CertificateDer::from( - rcgen::Certificate::from_params(ee_params) - .unwrap() - .serialize_der_with_signer(issuer) - .unwrap(), - ) - } } diff --git a/tests/generate.py b/tests/generate.py index 0c0746aa..aaa333fd 100755 --- a/tests/generate.py +++ b/tests/generate.py @@ -323,13 +323,6 @@ def tls_server_certs(force: bool) -> None: permitted_subtrees=[x509.DNSName(".example.com")], ) - generate_tls_server_cert_test( - output, - "disallow_subject_common_name", - expected_error="NameConstraintViolation", - subject_common_name="disallowed.example.com", - excluded_subtrees=[x509.DNSName("disallowed.example.com")], - ) generate_tls_server_cert_test( output, "disallow_dns_san", @@ -364,15 +357,6 @@ def tls_server_certs(force: bool) -> None: x509.DNSName("allowed-cn.example.com"), ], ) - generate_tls_server_cert_test( - output, - "allow_dns_san_and_disallow_subject_common_name", - expected_error="NameConstraintViolation", - sans=[x509.DNSName("allowed-san.example.com")], - subject_common_name="disallowed-cn.example.com", - permitted_subtrees=[x509.DNSName("allowed-san.example.com")], - excluded_subtrees=[x509.DNSName("disallowed-cn.example.com")], - ) generate_tls_server_cert_test( output, "disallow_dns_san_and_allow_subject_common_name", diff --git a/tests/misc/empty_sequence_common_name.der b/tests/misc/empty_sequence_common_name.der new file mode 100644 index 00000000..30e4f77b Binary files /dev/null and b/tests/misc/empty_sequence_common_name.der differ diff --git a/tests/misc/empty_sequence_common_name.der.txt b/tests/misc/empty_sequence_common_name.der.txt new file mode 100644 index 00000000..dbe59d44 --- /dev/null +++ b/tests/misc/empty_sequence_common_name.der.txt @@ -0,0 +1,104 @@ +SEQUENCE { + SEQUENCE { + [0] { + INTEGER { 2 } + } + INTEGER { `7214c2cb574538a4d63af28bb25140821cd3c528` } + SEQUENCE { + # ecdsa-with-SHA256 + OBJECT_IDENTIFIER { 1.2.840.10045.4.3.2 } + } + SEQUENCE { + SET { + SEQUENCE { + # organizationUnitName + OBJECT_IDENTIFIER { 2.5.4.11 } + PrintableString { "None" } + } + } + } + SEQUENCE { + UTCTime { "230906172100Z" } + UTCTime { "330903172100Z" } + } + SEQUENCE {} + SEQUENCE { + SEQUENCE { + # ecPublicKey + OBJECT_IDENTIFIER { 1.2.840.10045.2.1 } + # secp256r1 + OBJECT_IDENTIFIER { 1.2.840.10045.3.1.7 } + } + BIT_STRING { `00` `04681e0d5d4e66ff6f0cc3a9f0e1ba6114d647e9dfc2ee23418d56ad58edfb78cfe4ec8dadf856fde5a890799753bcd2a9380896701b7c13e49ec2e097f8ee3421` } + } + [3] { + SEQUENCE { + SEQUENCE { + # keyUsage + OBJECT_IDENTIFIER { 2.5.29.15 } + BOOLEAN { TRUE } + OCTET_STRING { + BIT_STRING { b`101` } + } + } + SEQUENCE { + # extKeyUsage + OBJECT_IDENTIFIER { 2.5.29.37 } + OCTET_STRING { + SEQUENCE { + # serverAuth + OBJECT_IDENTIFIER { 1.3.6.1.5.5.7.3.1 } + # clientAuth + OBJECT_IDENTIFIER { 1.3.6.1.5.5.7.3.2 } + } + } + } + SEQUENCE { + # basicConstraints + OBJECT_IDENTIFIER { 2.5.29.19 } + BOOLEAN { TRUE } + OCTET_STRING { + SEQUENCE {} + } + } + SEQUENCE { + # subjectKeyIdentifier + OBJECT_IDENTIFIER { 2.5.29.14 } + OCTET_STRING { + OCTET_STRING { `8f8c61be7ce4c34689e2618034581e34ef88b24c` } + } + } + SEQUENCE { + # authorityKeyIdentifier + OBJECT_IDENTIFIER { 2.5.29.35 } + OCTET_STRING { + SEQUENCE { + [0 PRIMITIVE] { `c9a3daf11d035f6eab28e2ea195c677568b0adea` } + } + } + } + SEQUENCE { + # subjectAltName + OBJECT_IDENTIFIER { 2.5.29.17 } + BOOLEAN { TRUE } + OCTET_STRING { + SEQUENCE { + [2 PRIMITIVE] { "example.com" } + } + } + } + } + } + } + SEQUENCE { + # ecdsa-with-SHA256 + OBJECT_IDENTIFIER { 1.2.840.10045.4.3.2 } + } + BIT_STRING { + `00` + SEQUENCE { + INTEGER { `00f1891fc58e16c37752bf64d58999f703c8d458b1d22d8291292ce2ad87042990` } + INTEGER { `00a6f16e39c83ba001f7f6aae73aaad0aea46d596800746887fe4567a4ffe88f9b` } + } + } +} diff --git a/tests/tls_server_certs.rs b/tests/tls_server_certs.rs index 8d4584d1..fb1d32b2 100644 --- a/tests/tls_server_certs.rs +++ b/tests/tls_server_certs.rs @@ -92,16 +92,6 @@ fn additional_dns_labels() { ); } -#[test] -fn disallow_subject_common_name() { - let ee = include_bytes!("tls_server_certs/disallow_subject_common_name.ee.der"); - let ca = include_bytes!("tls_server_certs/disallow_subject_common_name.ca.der"); - assert_eq!( - check_cert(ee, ca, &[], &[]), - Err(webpki::Error::NameConstraintViolation) - ); -} - #[test] fn disallow_dns_san() { let ee = include_bytes!("tls_server_certs/disallow_dns_san.ee.der"); @@ -141,18 +131,6 @@ fn allow_dns_san_and_subject_common_name() { ); } -#[test] -fn allow_dns_san_and_disallow_subject_common_name() { - let ee = - include_bytes!("tls_server_certs/allow_dns_san_and_disallow_subject_common_name.ee.der"); - let ca = - include_bytes!("tls_server_certs/allow_dns_san_and_disallow_subject_common_name.ca.der"); - assert_eq!( - check_cert(ee, ca, &[], &[]), - Err(webpki::Error::NameConstraintViolation) - ); -} - #[test] fn disallow_dns_san_and_allow_subject_common_name() { let ee =