diff --git a/src/subject_name/verify.rs b/src/subject_name/verify.rs index d430de1b..f9391fdd 100644 --- a/src/subject_name/verify.rs +++ b/src/subject_name/verify.rs @@ -30,22 +30,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| { - if let GeneralName::DnsName(presented_id) = name { - match dns_name::presented_id_matches_reference_id(presented_id, dns_name) { - Ok(true) => return NameIteration::Stop(Ok(())), - Ok(false) | Err(Error::MalformedDnsIdentifier) => (), - Err(e) => return NameIteration::Stop(Err(e)), - } - } - NameIteration::KeepGoing - }, ) + .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( @@ -62,23 +69,30 @@ pub(crate) fn verify_cert_subject_name( } }; - iterate_names( + NameIterator::new( // IP addresses are not compared against the subject field; // only against Subject Alternative Names. None, cert.inner().subject_alt_name, SubjectCommonNameContents::Ignore, - Err(Error::CertNotValidForName), - &mut |name| { - if let GeneralName::IpAddress(presented_id) = name { - match ip_address::presented_id_matches_reference_id(presented_id, ip_address) { - true => return NameIteration::Stop(Ok(())), - false => (), - } - } - NameIteration::KeepGoing - }, ) + .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 @@ -89,9 +103,7 @@ pub(crate) fn check_name_constraints( ) -> Result<(), Error> { let input = match input { Some(input) => input, - None => { - return Ok(()); - } + None => return Ok(()), }; fn parse_subtrees<'b>( @@ -109,19 +121,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, @@ -138,161 +154,121 @@ fn check_presented_id_conforms_to_constraints( name: GeneralName, permitted_subtrees: Option, excluded_subtrees: Option, -) -> NameIteration { - match check_presented_id_conforms_to_constraints_in_subtree( - name, - Subtrees::PermittedSubtrees, - permitted_subtrees, - ) { - stop @ NameIteration::Stop(..) => { - return stop; - } - NameIteration::KeepGoing => (), - }; - - check_presented_id_conforms_to_constraints_in_subtree( - name, - Subtrees::ExcludedSubtrees, - excluded_subtrees, - ) -} - -#[derive(Clone, Copy)] -enum Subtrees { - PermittedSubtrees, - ExcludedSubtrees, -} - -fn check_presented_id_conforms_to_constraints_in_subtree( - name: GeneralName, - subtrees: Subtrees, - constraints: Option, -) -> NameIteration { - let mut constraints = match constraints { - Some(constraints) => untrusted::Reader::new(constraints), - None => { - return NameIteration::KeepGoing; - } - }; - - let mut has_permitted_subtrees_match = false; - let mut has_permitted_subtrees_mismatch = false; - - while !constraints.at_end() { - // http://tools.ietf.org/html/rfc5280#section-4.2.1.10: "Within this - // profile, the minimum and maximum fields are not used with any name - // forms, thus, the minimum MUST be zero, and maximum MUST be absent." - // - // Since the default value isn't allowed to be encoded according to the - // DER encoding rules for DEFAULT, this is equivalent to saying that - // neither minimum or maximum must be encoded. - fn general_subtree<'b>( - input: &mut untrusted::Reader<'b>, - ) -> Result, Error> { - let general_subtree = der::expect_tag_and_get_value(input, der::Tag::Sequence)?; - general_subtree.read_all(Error::BadDer, GeneralName::from_der) - } +) -> Option> { + let subtrees = [ + (Subtrees::PermittedSubtrees, permitted_subtrees), + (Subtrees::ExcludedSubtrees, excluded_subtrees), + ]; + + fn general_subtree<'b>(input: &mut untrusted::Reader<'b>) -> Result, Error> { + der::expect_tag_and_get_value(input, der::Tag::Sequence)? + .read_all(Error::BadDer, GeneralName::from_der) + } - let base = match general_subtree(&mut constraints) { - Ok(base) => base, - Err(err) => { - return NameIteration::Stop(Err(err)); - } + for (subtrees, constraints) in subtrees { + let mut constraints = match constraints { + Some(constraints) => untrusted::Reader::new(constraints), + None => continue, }; - let matches = match (name, base) { - (GeneralName::DnsName(name), GeneralName::DnsName(base)) => { - dns_name::presented_id_matches_constraint(name, base) - } - - (GeneralName::DirectoryName(name), GeneralName::DirectoryName(base)) => Ok( - presented_directory_name_matches_constraint(name, base, subtrees), - ), - - (GeneralName::IpAddress(name), GeneralName::IpAddress(base)) => { - ip_address::presented_id_matches_constraint(name, base) - } + let mut has_permitted_subtrees_match = false; + let mut has_permitted_subtrees_mismatch = false; + while !constraints.at_end() { + // http://tools.ietf.org/html/rfc5280#section-4.2.1.10: "Within this + // profile, the minimum and maximum fields are not used with any name + // forms, thus, the minimum MUST be zero, and maximum MUST be absent." + // + // Since the default value isn't allowed to be encoded according to the + // DER encoding rules for DEFAULT, this is equivalent to saying that + // neither minimum or maximum must be encoded. + let base = match general_subtree(&mut constraints) { + Ok(base) => base, + Err(err) => return Some(Err(err)), + }; + + let matches = match (name, base) { + (GeneralName::DnsName(name), GeneralName::DnsName(base)) => { + dns_name::presented_id_matches_constraint(name, base) + } - // RFC 4280 says "If a name constraints extension that is marked as - // critical imposes constraints on a particular name form, and an - // instance of that name form appears in the subject field or - // subjectAltName extension of a subsequent certificate, then the - // application MUST either process the constraint or reject the - // certificate." Later, the CABForum agreed to support non-critical - // constraints, so it is important to reject the cert without - // considering whether the name constraint it critical. - (GeneralName::Unsupported(name_tag), GeneralName::Unsupported(base_tag)) - if name_tag == base_tag => - { - Err(Error::NameConstraintViolation) - } + (GeneralName::DirectoryName(_), GeneralName::DirectoryName(_)) => Ok( + // Reject any uses of directory name constraints; we don't implement this. + // + // Rejecting everything technically confirms to RFC5280: + // + // "If a name constraints extension that is marked as critical imposes constraints + // on a particular name form, and an instance of that name form appears in the + // subject field or subjectAltName extension of a subsequent certificate, then + // the application MUST either process the constraint or _reject the certificate_." + // + // TODO: rustls/webpki#19 + // + // Rejection is achieved by not matching any PermittedSubtrees, and matching all + // ExcludedSubtrees. + match subtrees { + Subtrees::PermittedSubtrees => false, + Subtrees::ExcludedSubtrees => true, + }, + ), + + (GeneralName::IpAddress(name), GeneralName::IpAddress(base)) => { + ip_address::presented_id_matches_constraint(name, base) + } - _ => { - // mismatch between constraint and name types; continue with current - // name and next constraint - continue; - } - }; + // RFC 4280 says "If a name constraints extension that is marked as + // critical imposes constraints on a particular name form, and an + // instance of that name form appears in the subject field or + // subjectAltName extension of a subsequent certificate, then the + // application MUST either process the constraint or reject the + // certificate." Later, the CABForum agreed to support non-critical + // constraints, so it is important to reject the cert without + // considering whether the name constraint it critical. + (GeneralName::Unsupported(name_tag), GeneralName::Unsupported(base_tag)) + if name_tag == base_tag => + { + Err(Error::NameConstraintViolation) + } - match (subtrees, matches) { - (Subtrees::PermittedSubtrees, Ok(true)) => { - has_permitted_subtrees_match = true; - } + _ => { + // mismatch between constraint and name types; continue with current + // name and next constraint + continue; + } + }; - (Subtrees::PermittedSubtrees, Ok(false)) => { - has_permitted_subtrees_mismatch = true; - } + match (subtrees, matches) { + (Subtrees::PermittedSubtrees, Ok(true)) => { + has_permitted_subtrees_match = true; + } - (Subtrees::ExcludedSubtrees, Ok(true)) => { - return NameIteration::Stop(Err(Error::NameConstraintViolation)); - } + (Subtrees::PermittedSubtrees, Ok(false)) => { + has_permitted_subtrees_mismatch = true; + } - (Subtrees::ExcludedSubtrees, Ok(false)) => (), + (Subtrees::ExcludedSubtrees, Ok(true)) => { + return Some(Err(Error::NameConstraintViolation)); + } - (_, Err(err)) => { - return NameIteration::Stop(Err(err)); + (Subtrees::ExcludedSubtrees, Ok(false)) => (), + (_, Err(err)) => return Some(Err(err)), } } - } - if has_permitted_subtrees_mismatch && !has_permitted_subtrees_match { - // If there was any entry of the given type in permittedSubtrees, then - // it required that at least one of them must match. Since none of them - // did, we have a failure. - NameIteration::Stop(Err(Error::NameConstraintViolation)) - } else { - NameIteration::KeepGoing + if has_permitted_subtrees_mismatch && !has_permitted_subtrees_match { + // If there was any entry of the given type in permittedSubtrees, then + // it required that at least one of them must match. Since none of them + // did, we have a failure. + return Some(Err(Error::NameConstraintViolation)); + } } -} -fn presented_directory_name_matches_constraint( - _name: untrusted::Input, - _constraint: untrusted::Input, - subtrees: Subtrees, -) -> bool { - // Reject any uses of directory name constraints; we don't implement this. - // - // Rejecting everything technically confirms to RFC5280: - // - // "If a name constraints extension that is marked as critical imposes constraints - // on a particular name form, and an instance of that name form appears in the - // subject field or subjectAltName extension of a subsequent certificate, then - // the application MUST either process the constraint or _reject the certificate_." - // - // TODO: rustls/webpki#19 - // - // Rejection is achieved by not matching any PermittedSubtrees, and matching all - // ExcludedSubtrees. - match subtrees { - Subtrees::PermittedSubtrees => false, - Subtrees::ExcludedSubtrees => true, - } + None } #[derive(Clone, Copy)] -enum NameIteration { - KeepGoing, - Stop(Result<(), Error>), +enum Subtrees { + PermittedSubtrees, + ExcludedSubtrees, } #[derive(Clone, Copy)] @@ -301,52 +277,77 @@ pub(crate) enum SubjectCommonNameContents { Ignore, } -fn iterate_names<'names>( - subject: Option>, - subject_alt_name: Option>, - subject_common_name_contents: SubjectCommonNameContents, - result_if_never_stopped_early: Result<(), Error>, - f: &mut impl FnMut(GeneralName<'names>) -> NameIteration, -) -> 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)?; - match f(name) { - NameIteration::Stop(result) => { - return result; - } - NameIteration::KeepGoing => (), - } +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 let Some(subject) = subject { - match f(GeneralName::DirectoryName(subject)) { - NameIteration::Stop(result) => return result, - NameIteration::KeepGoing => (), - }; - } +impl<'a> Iterator for NameIterator<'a> { + type Item = Result, Error>; + + fn next(&mut self) -> Option { + if let Some(subject_alt_name) = &mut self.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. + + if !subject_alt_name.at_end() { + let err = match GeneralName::from_der(subject_alt_name) { + Ok(name) => return Some(Ok(name)), + Err(err) => err, + }; + + // 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; + } + } - if let (SubjectCommonNameContents::DnsName, Some(subject)) = - (subject_common_name_contents, subject) - { - match common_name(subject) { - Ok(Some(cn)) => match f(GeneralName::DnsName(cn)) { - NameIteration::Stop(result) => result, - NameIteration::KeepGoing => result_if_never_stopped_early, - }, - Ok(None) => result_if_never_stopped_early, - Err(err) => Err(err), + if let Some(subject_directory_name) = self.subject_directory_name.take() { + return Some(Ok(GeneralName::DirectoryName(subject_directory_name))); } - } else { - result_if_never_stopped_early + + 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 } } @@ -357,30 +358,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| { - if let GeneralName::DnsName(presented_id) = 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) - } - } - NameIteration::KeepGoing - }, ) - .map(|_| names.into_iter()) + .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 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