From 547aaddc8b1698a674d57c062b3b08f2e325743d Mon Sep 17 00:00:00 2001 From: Dirkjan Ochtman Date: Fri, 28 Jul 2023 22:41:45 +0200 Subject: [PATCH] Replace iterate_names() with a NameIterator --- src/subject_name/verify.rs | 283 ++++++++++++++++++++++++------------- 1 file changed, 184 insertions(+), 99 deletions(-) diff --git a/src/subject_name/verify.rs b/src/subject_name/verify.rs index 5256c89a..a6d040b2 100644 --- a/src/subject_name/verify.rs +++ b/src/subject_name/verify.rs @@ -14,6 +14,7 @@ #[cfg(feature = "alloc")] use alloc::vec::Vec; +use core::mem; use super::dns_name::{self, DnsNameRef}; #[cfg(feature = "alloc")] @@ -30,24 +31,29 @@ 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()); - iterate_names( + NameIterator::new( Some(cert.subject), cert.subject_alt_name, SubjectCommonNameContents::Ignore, - Err(Error::CertNotValidForName), - &mut |name| { - 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)), - } - }, ) + .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, + }; + + 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( @@ -64,25 +70,28 @@ pub(crate) fn verify_cert_subject_name( } }; - iterate_names( - // IP addresses are not compared against the subject field; - // only against Subject Alternative Names. + NameIterator::new( None, cert.inner().subject_alt_name, SubjectCommonNameContents::Ignore, - Err(Error::CertNotValidForName), - &mut |name| { - let presented_id = match name { - GeneralName::IpAddress(presented) => presented, - _ => return None, - }; - - match ip_address::presented_id_matches_reference_id(presented_id, ip_address) { - true => Some(Ok(())), - false => None, - } - }, ) + .find_map(|result| { + let name = match result { + Ok(name) => name, + Err(err) => return Some(Err(err)), + }; + + let presented_id = match name { + GeneralName::IpAddress(presented) => presented, + _ => return None, + }; + + match ip_address::presented_id_matches_reference_id(presented_id, ip_address) { + true => Some(Ok(())), + false => None, + } + }) + .unwrap_or(Err(Error::CertNotValidForName)) } // https://tools.ietf.org/html/rfc5280#section-4.2.1.10 @@ -111,19 +120,23 @@ pub(crate) fn check_name_constraints( let mut child = subordinate_certs; loop { - iterate_names( + let result = NameIterator::new( Some(child.subject), child.subject_alt_name, subject_common_name_contents, - Ok(()), - &mut |name| { - check_presented_id_conforms_to_constraints( - name, - permitted_subtrees, - excluded_subtrees, - ) - }, - )?; + ) + .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) + }); + + if let Some(Err(err)) = result { + return Err(err); + } child = match child.ee_or_ca { EndEntityOrCa::Ca(child_cert) => child_cert, @@ -265,52 +278,116 @@ pub(crate) enum SubjectCommonNameContents { Ignore, } -fn iterate_names<'names>( - subject: Option>, - subject_alt_name: Option>, +struct NameIterator<'a> { + state: NameIteratorState<'a>, subject_common_name_contents: SubjectCommonNameContents, - result_if_never_stopped_early: Result<(), Error>, - f: &mut impl FnMut(GeneralName<'names>) -> Option>, -) -> Result<(), Error> { - if let Some(subject_alt_name) = subject_alt_name { - let mut subject_alt_name = untrusted::Reader::new(subject_alt_name); - // https://bugzilla.mozilla.org/show_bug.cgi?id=1143085: An empty - // subjectAltName is not legal, but some certificates have an empty - // subjectAltName. Since we don't support CN-IDs, the certificate - // will be rejected either way, but checking `at_end` before - // attempting to parse the first entry allows us to return a better - // error code. - while !subject_alt_name.at_end() { - let name = GeneralName::from_der(&mut subject_alt_name)?; - if let Some(result) = f(name) { - return result; - } - } - } +} - let subject = match subject { - Some(subject) => subject, - None => return result_if_never_stopped_early, - }; +impl<'a> NameIterator<'a> { + fn new( + subject: Option>, + subject_alt_name: Option>, + subject_common_name_contents: SubjectCommonNameContents, + ) -> Self { + let state = match (subject_alt_name, subject) { + (Some(subject_alt_name), _) => NameIteratorState::SubjectAltName { + subject_alt_name: untrusted::Reader::new(subject_alt_name), + subject, + }, + (None, Some(subject)) => NameIteratorState::SubjectDirectoryName { subject }, + (None, None) => NameIteratorState::None, + }; - if let Some(result) = f(GeneralName::DirectoryName(subject)) { - return result; + NameIterator { + state, + subject_common_name_contents, + } } +} - if let SubjectCommonNameContents::Ignore = subject_common_name_contents { - return result_if_never_stopped_early; - } +impl<'a> Iterator for NameIterator<'a> { + type Item = Result, Error>; + + fn next(&mut self) -> Option { + let item; + self.state = match mem::replace(&mut self.state, NameIteratorState::None) { + NameIteratorState::SubjectAltName { + mut subject_alt_name, + subject, + } => { + // https://bugzilla.mozilla.org/show_bug.cgi?id=1143085: An empty + // subjectAltName is not legal, but some certificates have an empty + // subjectAltName. Since we don't support CN-IDs, the certificate + // will be rejected either way, but checking `at_end` before + // attempting to parse the first entry allows us to return a better + // error code. + + item = match GeneralName::from_der(&mut subject_alt_name) { + Ok(name) => Some(Ok(name)), + Err(err) => { + self.state = NameIteratorState::None; + return Some(Err(err)); + } + }; + + match subject_alt_name.at_end() { + true => match subject { + Some(subject) => NameIteratorState::SubjectDirectoryName { subject }, + None => NameIteratorState::None, + }, + false => NameIteratorState::SubjectAltName { + subject_alt_name, + subject, + }, + } + } + NameIteratorState::SubjectDirectoryName { subject } => { + item = Some(Ok(GeneralName::DirectoryName(subject))); + match self.subject_common_name_contents { + SubjectCommonNameContents::DnsName => { + NameIteratorState::SubjectCommonName { subject } + } + SubjectCommonNameContents::Ignore => NameIteratorState::None, + } + } + NameIteratorState::SubjectCommonName { subject } => { + item = match common_name(subject) { + Ok(Some(cn)) => match self.subject_common_name_contents { + SubjectCommonNameContents::DnsName => Some(Ok(GeneralName::DnsName(cn))), + SubjectCommonNameContents::Ignore => None, + }, + Ok(None) => None, + Err(err) => { + self.state = NameIteratorState::None; + return Some(Err(err)); + } + }; + NameIteratorState::None + } + NameIteratorState::None => { + item = None; + NameIteratorState::None + } + }; - match common_name(subject) { - Ok(Some(cn)) => match f(GeneralName::DnsName(cn)) { - Some(result) => result, - None => result_if_never_stopped_early, - }, - Ok(None) => result_if_never_stopped_early, - Err(err) => Err(err), + item } } +enum NameIteratorState<'a> { + SubjectAltName { + subject_alt_name: untrusted::Reader<'a>, + subject: Option>, + }, + SubjectDirectoryName { + subject: untrusted::Input<'a>, + }, + SubjectCommonName { + subject: untrusted::Input<'a>, + }, + None, +} + #[cfg(feature = "alloc")] pub(crate) fn list_cert_dns_names<'names>( cert: &'names crate::EndEntityCert<'names>, @@ -318,34 +395,42 @@ pub(crate) fn list_cert_dns_names<'names>( let cert = &cert.inner(); let mut names = Vec::new(); - iterate_names( + let result = NameIterator::new( Some(cert.subject), cert.subject_alt_name, SubjectCommonNameContents::DnsName, - Ok(()), - &mut |name| { - let presented_id = match name { - GeneralName::DnsName(presented) => presented, - _ => return None, - }; + ) + .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) - }); - - // 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 presented_id = match name { + GeneralName::DnsName(presented) => presented, + _ => return None, + }; - None - }, - ) - .map(|_| names.into_iter()) + 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 + }); + + match result { + Some(err) => Err(err), + _ => Ok(names.into_iter()), + } } // It is *not* valid to derive `Eq`, `PartialEq, etc. for this type. In