From f55622a54befabaf3c994a71b987adb4d3ace3ca Mon Sep 17 00:00:00 2001 From: Daniel McCarney Date: Tue, 5 Sep 2023 11:28:26 -0400 Subject: [PATCH] verify_cert: budget for name constraint comparisons This commit updates the name constraint validation done during path building to apply a budget for the maximum allowed number of name constraint checks. We use the same limit that golang crypto/x509 applies by default: 250,000 comparisons. Note: this commit applies the budget during path building in a manner that means certificates _not_ part of the built path can consume comparisons from the budget even though they will not be present in the complete validated path. Similarly name constraints are evaluated before signatures, meaning a certificate that doesn't verify to a trusted root still has its constraints parsed and evaluated. A subsequent commit will adjust these shortcomings. --- src/error.rs | 3 +++ src/subject_name/verify.rs | 30 +++++++++++++++++++++--------- src/verify_cert.rs | 22 ++++++++++++++++++---- 3 files changed, 42 insertions(+), 13 deletions(-) diff --git a/src/error.rs b/src/error.rs index bdd3569e..2b7183a4 100644 --- a/src/error.rs +++ b/src/error.rs @@ -63,6 +63,9 @@ pub enum Error { /// and as recommended by RFC6125. MalformedExtensions, + /// The maximum number of name constraint comparisons has been reached. + MaximumNameConstraintComparisonsExceeded, + /// The maximum number of internal path building calls has been reached. Path complexity is too great. MaximumPathBuildCallsExceeded, diff --git a/src/subject_name/verify.rs b/src/subject_name/verify.rs index 1dd81cad..ea3faecd 100644 --- a/src/subject_name/verify.rs +++ b/src/subject_name/verify.rs @@ -19,7 +19,9 @@ use super::{ }; use crate::{ cert::{Cert, EndEntityOrCa}, - der, Error, + der, + verify_cert::Budget, + Error, }; pub(crate) fn verify_cert_dns_name( @@ -33,7 +35,7 @@ pub(crate) fn verify_cert_dns_name( cert.subject_alt_name, SubjectCommonNameContents::Ignore, Err(Error::CertNotValidForName), - &|name| { + &mut |name| { if let GeneralName::DnsName(presented_id) = name { match dns_name::presented_id_matches_reference_id(presented_id, dns_name) { Some(true) => return NameIteration::Stop(Ok(())), @@ -67,7 +69,7 @@ pub(crate) fn verify_cert_subject_name( cert.inner().subject_alt_name, SubjectCommonNameContents::Ignore, Err(Error::CertNotValidForName), - &|name| { + &mut |name| { if let GeneralName::IpAddress(presented_id) = name { match ip_address::presented_id_matches_reference_id(presented_id, ip_address) { Ok(true) => return NameIteration::Stop(Ok(())), @@ -84,11 +86,12 @@ pub(crate) fn verify_cert_subject_name( // https://tools.ietf.org/html/rfc5280#section-4.2.1.10 pub(crate) fn check_name_constraints( - constraints: Option<&mut untrusted::Reader>, + input: Option<&mut untrusted::Reader>, subordinate_certs: &Cert, subject_common_name_contents: SubjectCommonNameContents, + budget: &mut Budget, ) -> Result<(), Error> { - let constraints = match constraints { + let input = match input { Some(input) => input, None => { return Ok(()); @@ -105,8 +108,8 @@ pub(crate) fn check_name_constraints( der::expect_tag_and_get_value(inner, subtrees_tag).map(Some) } - let permitted_subtrees = parse_subtrees(constraints, der::Tag::ContextSpecificConstructed0)?; - let excluded_subtrees = parse_subtrees(constraints, der::Tag::ContextSpecificConstructed1)?; + let permitted_subtrees = parse_subtrees(input, der::Tag::ContextSpecificConstructed0)?; + let excluded_subtrees = parse_subtrees(input, der::Tag::ContextSpecificConstructed1)?; let mut child = subordinate_certs; loop { @@ -115,11 +118,12 @@ pub(crate) fn check_name_constraints( child.subject_alt_name, subject_common_name_contents, Ok(()), - &|name| { + &mut |name| { check_presented_id_conforms_to_constraints( name, permitted_subtrees, excluded_subtrees, + budget, ) }, )?; @@ -139,11 +143,13 @@ fn check_presented_id_conforms_to_constraints( name: GeneralName, permitted_subtrees: Option, excluded_subtrees: Option, + budget: &mut Budget, ) -> NameIteration { match check_presented_id_conforms_to_constraints_in_subtree( name, Subtrees::PermittedSubtrees, permitted_subtrees, + budget, ) { stop @ NameIteration::Stop(..) => { return stop; @@ -155,6 +161,7 @@ fn check_presented_id_conforms_to_constraints( name, Subtrees::ExcludedSubtrees, excluded_subtrees, + budget, ) } @@ -168,6 +175,7 @@ fn check_presented_id_conforms_to_constraints_in_subtree( name: GeneralName, subtrees: Subtrees, constraints: Option, + budget: &mut Budget, ) -> NameIteration { let mut constraints = match constraints { Some(constraints) => untrusted::Reader::new(constraints), @@ -180,6 +188,10 @@ fn check_presented_id_conforms_to_constraints_in_subtree( let mut has_permitted_subtrees_mismatch = false; while !constraints.at_end() { + if let Err(e) = budget.consume_name_constraint_comparison() { + return NameIteration::Stop(Err(e)); + } + // 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." @@ -307,7 +319,7 @@ fn iterate_names( subject_alt_name: Option, subject_common_name_contents: SubjectCommonNameContents, result_if_never_stopped_early: Result<(), Error>, - f: &dyn Fn(GeneralName) -> NameIteration, + f: &mut dyn FnMut(GeneralName) -> NameIteration, ) -> Result<(), Error> { if let Some(subject_alt_name) = subject_alt_name { let mut subject_alt_name = untrusted::Reader::new(subject_alt_name); diff --git a/src/verify_cert.rs b/src/verify_cert.rs index 55b4adbe..e72563bd 100644 --- a/src/verify_cert.rs +++ b/src/verify_cert.rs @@ -95,9 +95,8 @@ fn build_chain_inner( } let name_constraints = trust_anchor.name_constraints.map(untrusted::Input::from); - untrusted::read_all_optional(name_constraints, Error::BadDer, |value| { - subject_name::check_name_constraints(value, cert, subject_common_name_contents) + subject_name::check_name_constraints(value, cert, subject_common_name_contents, budget) })?; // TODO: check_distrust(trust_anchor_subject, trust_anchor_spki)?; @@ -144,7 +143,7 @@ fn build_chain_inner( } untrusted::read_all_optional(potential_issuer.name_constraints, Error::BadDer, |value| { - subject_name::check_name_constraints(value, cert, subject_common_name_contents) + subject_name::check_name_constraints(value, cert, subject_common_name_contents, budget) })?; let next_sub_ca_count = match used_as_ca { @@ -196,6 +195,7 @@ fn check_signed_chain( pub(crate) struct Budget { signatures: usize, build_chain_calls: usize, + name_constraint_comparisons: usize, } impl Budget { @@ -216,6 +216,15 @@ impl Budget { .ok_or(Error::MaximumPathBuildCallsExceeded)?; Ok(()) } + + #[inline] + pub(crate) fn consume_name_constraint_comparison(&mut self) -> Result<(), Error> { + self.name_constraint_comparisons = self + .name_constraint_comparisons + .checked_sub(1) + .ok_or(Error::MaximumNameConstraintComparisonsExceeded)?; + Ok(()) + } } impl Default for Budget { @@ -230,6 +239,10 @@ impl Default for Budget { // This limit is taken from NSS libmozpkix, see: // build_chain_calls: 200_000, + + // This limit is taken from golang crypto/x509's default, see: + // + name_constraint_comparisons: 250_000, } } } @@ -423,7 +436,8 @@ where match f(v) { Ok(()) => return Ok(()), err @ Err(Error::MaximumSignatureChecksExceeded) - | err @ Err(Error::MaximumPathBuildCallsExceeded) => return err, + | err @ Err(Error::MaximumPathBuildCallsExceeded) + | err @ Err(Error::MaximumNameConstraintComparisonsExceeded) => return err, _ => {} } }