From a328b56b50f080b3ada6c5abe500a9030fb20cf3 Mon Sep 17 00:00:00 2001 From: Joseph Birr-Pixton Date: Tue, 5 Sep 2023 10:50:10 +0100 Subject: [PATCH 01/21] Track signature limit using `Budget` type --- src/end_entity.rs | 4 --- src/verify_cert.rs | 64 ++++++++++++++++++++++++++++++++++++---------- 2 files changed, 50 insertions(+), 18 deletions(-) diff --git a/src/end_entity.rs b/src/end_entity.rs index 14569f1f..dd0a3147 100644 --- a/src/end_entity.rs +++ b/src/end_entity.rs @@ -97,8 +97,6 @@ impl<'a> EndEntityCert<'a> { intermediate_certs, &self.inner, time, - 0, - &mut 0_usize, ) } @@ -130,8 +128,6 @@ impl<'a> EndEntityCert<'a> { intermediate_certs, &self.inner, time, - 0, - &mut 0_usize, ) } diff --git a/src/verify_cert.rs b/src/verify_cert.rs index 6166218f..9f9276e8 100644 --- a/src/verify_cert.rs +++ b/src/verify_cert.rs @@ -25,8 +25,29 @@ pub(crate) fn build_chain( intermediate_certs: &[&[u8]], cert: &Cert, time: time::Time, +) -> Result<(), Error> { + build_chain_inner( + required_eku_if_present, + supported_sig_algs, + trust_anchors, + intermediate_certs, + cert, + time, + 0, + &mut Budget::default(), + ) +} + +#[allow(clippy::too_many_arguments)] +fn build_chain_inner( + required_eku_if_present: KeyPurposeId, + supported_sig_algs: &[&SignatureAlgorithm], + trust_anchors: &[TrustAnchor], + intermediate_certs: &[&[u8]], + cert: &Cert, + time: time::Time, sub_ca_count: usize, - signatures: &mut usize, + budget: &mut Budget, ) -> Result<(), Error> { let used_as_ca = used_as_ca(&cert.ee_or_ca); @@ -77,11 +98,11 @@ pub(crate) fn build_chain( subject_name::check_name_constraints(value, cert, subject_common_name_contents) })?; - let trust_anchor_spki = untrusted::Input::from(trust_anchor.spki); - // TODO: check_distrust(trust_anchor_subject, trust_anchor_spki)?; - check_signatures(supported_sig_algs, cert, trust_anchor_spki, signatures)?; + let trust_anchor_spki = untrusted::Input::from(trust_anchor.spki); + + check_signatures(supported_sig_algs, cert, trust_anchor_spki, budget)?; Ok(()) }); @@ -128,7 +149,7 @@ pub(crate) fn build_chain( UsedAsCa::Yes => sub_ca_count + 1, }; - build_chain( + build_chain_inner( required_eku_if_present, supported_sig_algs, trust_anchors, @@ -136,7 +157,7 @@ pub(crate) fn build_chain( &potential_issuer, time, next_sub_ca_count, - signatures, + budget, ) }) } @@ -145,16 +166,12 @@ fn check_signatures( supported_sig_algs: &[&SignatureAlgorithm], cert_chain: &Cert, trust_anchor_key: untrusted::Input, - signatures: &mut usize, + budget: &mut Budget, ) -> Result<(), Error> { let mut spki_value = trust_anchor_key; let mut cert = cert_chain; loop { - *signatures += 1; - if *signatures > 100 { - return Err(Error::MaximumSignatureChecksExceeded); - } - + budget.consume_signature()?; signed_data::verify_signed_data(supported_sig_algs, spki_value, &cert.signed_data)?; // TODO: check revocation @@ -173,6 +190,27 @@ fn check_signatures( Ok(()) } +struct Budget { + signatures: usize, +} + +impl Budget { + #[inline] + fn consume_signature(&mut self) -> Result<(), Error> { + self.signatures = self + .signatures + .checked_sub(1) + .ok_or(Error::MaximumSignatureChecksExceeded)?; + Ok(()) + } +} + +impl core::default::Default for Budget { + fn default() -> Self { + Self { signatures: 100 } + } +} + fn check_issuer_independent_properties( cert: &Cert, time: time::Time, @@ -427,8 +465,6 @@ mod tests { intermediate_certs, cert.inner(), time, - 0, - &mut 0_usize, ); assert!(matches!(result, Err(Error::MaximumSignatureChecksExceeded))); From 7fec222fa149388110c518d0c1b22a9033b4f613 Mon Sep 17 00:00:00 2001 From: Joseph Birr-Pixton Date: Tue, 5 Sep 2023 11:30:03 +0100 Subject: [PATCH 02/21] Add comment indicating source of signature budget --- src/verify_cert.rs | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/src/verify_cert.rs b/src/verify_cert.rs index 9f9276e8..796fd52b 100644 --- a/src/verify_cert.rs +++ b/src/verify_cert.rs @@ -207,7 +207,13 @@ impl Budget { impl core::default::Default for Budget { fn default() -> Self { - Self { signatures: 100 } + Self { + // This limit is taken from the remediation for golang CVE-2018-16875. However, + // note that golang subsequently implemented AKID matching due to this limit + // being hit in real applications (see ). + // So this may actually be too aggressive. + signatures: 100, + } } } From cb0e81def89ce5d6c8e61b62ea4939a811a303d4 Mon Sep 17 00:00:00 2001 From: Joseph Birr-Pixton Date: Tue, 5 Sep 2023 11:44:51 +0100 Subject: [PATCH 03/21] Apply budget to number of calls to `build_chain_inner` --- src/error.rs | 3 ++ src/verify_cert.rs | 71 +++++++++++++++++++++++++++++++++++++--------- 2 files changed, 61 insertions(+), 13 deletions(-) diff --git a/src/error.rs b/src/error.rs index fbcf6f74..e26b32b5 100644 --- a/src/error.rs +++ b/src/error.rs @@ -84,6 +84,9 @@ pub enum Error { /// The maximum number of signature checks has been reached. Path complexity is too great. MaximumSignatureChecksExceeded, + /// The maximum number of internal path building calls has been reached. Path complexity is too great. + MaximumPathBuildCallsExceeded, + /// The certificate contains an unsupported critical extension. UnsupportedCriticalExtension, diff --git a/src/verify_cert.rs b/src/verify_cert.rs index 796fd52b..b527824f 100644 --- a/src/verify_cert.rs +++ b/src/verify_cert.rs @@ -110,7 +110,8 @@ fn build_chain_inner( // If the error is not fatal, then keep going. match result { Ok(()) => return Ok(()), - err @ Err(Error::MaximumSignatureChecksExceeded) => return err, + err @ Err(Error::MaximumSignatureChecksExceeded) + | err @ Err(Error::MaximumPathBuildCallsExceeded) => return err, _ => {} }; @@ -149,6 +150,7 @@ fn build_chain_inner( UsedAsCa::Yes => sub_ca_count + 1, }; + budget.consume_build_chain_call()?; build_chain_inner( required_eku_if_present, supported_sig_algs, @@ -192,6 +194,7 @@ fn check_signatures( struct Budget { signatures: usize, + build_chain_calls: usize, } impl Budget { @@ -203,6 +206,15 @@ impl Budget { .ok_or(Error::MaximumSignatureChecksExceeded)?; Ok(()) } + + #[inline] + fn consume_build_chain_call(&mut self) -> Result<(), Error> { + self.build_chain_calls = self + .build_chain_calls + .checked_sub(1) + .ok_or(Error::MaximumPathBuildCallsExceeded)?; + Ok(()) + } } impl core::default::Default for Budget { @@ -213,6 +225,10 @@ impl core::default::Default for Budget { // being hit in real applications (see ). // So this may actually be too aggressive. signatures: 100, + + // This limit is taken from NSS libmozpkix, see: + // + build_chain_calls: 200000, } } } @@ -405,7 +421,8 @@ where // If the error is not fatal, then keep going. match f(v) { Ok(()) => return Ok(()), - err @ Err(Error::MaximumSignatureChecksExceeded) => return err, + err @ Err(Error::MaximumSignatureChecksExceeded) + | err @ Err(Error::MaximumPathBuildCallsExceeded) => return err, _ => {} } } @@ -415,13 +432,21 @@ where #[cfg(test)] mod tests { use super::*; + use core::convert::TryFrom; - #[test] #[cfg(feature = "alloc")] - fn test_too_many_signatures() { - use std::convert::TryFrom; + enum TrustAnchorIsActualIssuer { + Yes, + No, + } - use crate::{EndEntityCert, Time, ECDSA_P256_SHA256}; + #[cfg(feature = "alloc")] + fn build_degenerate_chain( + intermediate_count: usize, + trust_anchor_is_actual_issuer: TrustAnchorIsActualIssuer, + ) -> Error { + use crate::ECDSA_P256_SHA256; + use crate::{EndEntityCert, Time}; let alg = &rcgen::PKCS_ECDSA_P256_SHA256; @@ -443,9 +468,9 @@ mod tests { let ca_cert = make_issuer(); let ca_cert_der = ca_cert.serialize_der().unwrap(); - let mut intermediates = Vec::with_capacity(101); + let mut intermediates = Vec::with_capacity(intermediate_count); let mut issuer = ca_cert; - for _ in 0..101 { + for _ in 0..intermediate_count { let intermediate = make_issuer(); let intermediate_der = intermediate.serialize_der_with_signer(&issuer).unwrap(); intermediates.push(intermediate_der); @@ -461,18 +486,38 @@ mod tests { let anchors = &[TrustAnchor::try_from_cert_der(&ca_cert_der).unwrap()]; let time = Time::from_seconds_since_unix_epoch(0x1fed_f00d); let cert = EndEntityCert::try_from(&ee_cert_der[..]).unwrap(); - let intermediates_der: Vec<&[u8]> = intermediates.iter().map(|x| x.as_ref()).collect(); - let intermediate_certs: &[&[u8]] = intermediates_der.as_ref(); + let mut intermediate_certs = intermediates.iter().map(|x| x.as_ref()).collect::>(); + + if let TrustAnchorIsActualIssuer::No = trust_anchor_is_actual_issuer { + intermediate_certs.pop(); + } - let result = build_chain( + build_chain( EKU_SERVER_AUTH, &[&ECDSA_P256_SHA256], anchors, - intermediate_certs, + &intermediate_certs, cert.inner(), time, + ) + .unwrap_err() + } + + #[test] + #[cfg(feature = "alloc")] + fn test_too_many_signatures() { + assert_eq!( + build_degenerate_chain(5, TrustAnchorIsActualIssuer::Yes), + Error::MaximumSignatureChecksExceeded ); + } - assert!(matches!(result, Err(Error::MaximumSignatureChecksExceeded))); + #[test] + #[cfg(feature = "alloc")] + fn test_too_many_path_calls() { + assert_eq!( + build_degenerate_chain(10, TrustAnchorIsActualIssuer::No), + Error::MaximumPathBuildCallsExceeded + ); } } From 0b139abeb12e0ce25897ca7e5a2f4c3183ee50a9 Mon Sep 17 00:00:00 2001 From: Joseph Birr-Pixton Date: Tue, 5 Sep 2023 12:23:54 +0100 Subject: [PATCH 04/21] Introduce and test for `MaximumPathDepthExceeded` error This is executing a TODO when the chain length exceeds 6 issuers deep. --- src/error.rs | 3 ++ src/verify_cert.rs | 77 +++++++++++++++++++++++++++++++++++++++++++++- 2 files changed, 79 insertions(+), 1 deletion(-) diff --git a/src/error.rs b/src/error.rs index e26b32b5..1402bd75 100644 --- a/src/error.rs +++ b/src/error.rs @@ -87,6 +87,9 @@ pub enum Error { /// The maximum number of internal path building calls has been reached. Path complexity is too great. MaximumPathBuildCallsExceeded, + /// The path search was terminated because it became too deep. + MaximumPathDepthExceeded, + /// The certificate contains an unsupported critical extension. UnsupportedCriticalExtension, diff --git a/src/verify_cert.rs b/src/verify_cert.rs index b527824f..25254bb9 100644 --- a/src/verify_cert.rs +++ b/src/verify_cert.rs @@ -66,7 +66,7 @@ fn build_chain_inner( const MAX_SUB_CA_COUNT: usize = 6; if sub_ca_count >= MAX_SUB_CA_COUNT { - return Err(Error::UnknownIssuer); + return Err(Error::MaximumPathDepthExceeded); } } UsedAsCa::No => { @@ -520,4 +520,79 @@ mod tests { Error::MaximumPathBuildCallsExceeded ); } + + #[cfg(feature = "alloc")] + fn build_linear_chain(chain_length: usize) -> Result<(), Error> { + use crate::ECDSA_P256_SHA256; + use crate::{EndEntityCert, Time}; + + let alg = &rcgen::PKCS_ECDSA_P256_SHA256; + + let make_issuer = |index: usize| { + let mut ca_params = rcgen::CertificateParams::new(Vec::new()); + ca_params.distinguished_name.push( + rcgen::DnType::OrganizationName, + format!("Bogus Subject {index}"), + ); + 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 = alg; + rcgen::Certificate::from_params(ca_params).unwrap() + }; + + let ca_cert = make_issuer(chain_length); + let ca_cert_der = ca_cert.serialize_der().unwrap(); + + let mut intermediates = Vec::with_capacity(chain_length); + let mut issuer = ca_cert; + for i in 0..chain_length { + let intermediate = make_issuer(i); + let intermediate_der = intermediate.serialize_der_with_signer(&issuer).unwrap(); + intermediates.push(intermediate_der); + issuer = intermediate; + } + + let mut ee_params = rcgen::CertificateParams::new(vec!["example.com".to_string()]); + ee_params.is_ca = rcgen::IsCa::ExplicitNoCa; + ee_params.alg = alg; + let ee_cert = rcgen::Certificate::from_params(ee_params).unwrap(); + let ee_cert_der = ee_cert.serialize_der_with_signer(&issuer).unwrap(); + + let anchors = &[TrustAnchor::try_from_cert_der(&ca_cert_der).unwrap()]; + let time = Time::from_seconds_since_unix_epoch(0x1fed_f00d); + let cert = EndEntityCert::try_from(&ee_cert_der[..]).unwrap(); + let intermediates_der = intermediates.iter().map(|x| x.as_ref()).collect::>(); + + build_chain( + EKU_SERVER_AUTH, + &[&ECDSA_P256_SHA256], + anchors, + &intermediates_der, + cert.inner(), + time, + ) + } + + #[test] + #[cfg(feature = "alloc")] + fn longest_allowed_path() { + assert_eq!(build_linear_chain(1), Ok(())); + assert_eq!(build_linear_chain(2), Ok(())); + assert_eq!(build_linear_chain(3), Ok(())); + assert_eq!(build_linear_chain(4), Ok(())); + assert_eq!(build_linear_chain(5), Ok(())); + assert_eq!(build_linear_chain(6), Ok(())); + } + + #[test] + #[cfg(feature = "alloc")] + fn path_too_long() { + // Note: webpki 0.101.x and earlier surface all non-fatal errors as UnknownIssuer, + // eating the more specific MaximumPathDepthExceeded error. + assert_eq!(build_linear_chain(7), Err(Error::UnknownIssuer)); + } } From abcbdae2c53aca844512bd73e5bf379f91d1b65f Mon Sep 17 00:00:00 2001 From: Dirkjan Ochtman Date: Tue, 5 Sep 2023 17:57:15 +0200 Subject: [PATCH 05/21] Import Default trait --- src/verify_cert.rs | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/src/verify_cert.rs b/src/verify_cert.rs index 25254bb9..995c20f7 100644 --- a/src/verify_cert.rs +++ b/src/verify_cert.rs @@ -12,6 +12,8 @@ // ACTION OF CONTRACT, NEGLIGENCE OR OTHER TORTIOUS ACTION, ARISING OUT OF // OR IN CONNECTION WITH THE USE OR PERFORMANCE OF THIS SOFTWARE. +use core::default::Default; + use crate::{ cert::{self, Cert, EndEntityOrCa}, der, signed_data, subject_name, time, Error, SignatureAlgorithm, TrustAnchor, @@ -217,7 +219,7 @@ impl Budget { } } -impl core::default::Default for Budget { +impl Default for Budget { fn default() -> Self { Self { // This limit is taken from the remediation for golang CVE-2018-16875. However, From d2d070c80175b2b547049db349be0be6f502664b Mon Sep 17 00:00:00 2001 From: Dirkjan Ochtman Date: Tue, 5 Sep 2023 18:04:16 +0200 Subject: [PATCH 06/21] Force all signature verification operations to consume budget --- src/signed_data.rs | 8 +++++++- src/verify_cert.rs | 7 +++---- 2 files changed, 10 insertions(+), 5 deletions(-) diff --git a/src/signed_data.rs b/src/signed_data.rs index eb36c44f..f29cfbca 100644 --- a/src/signed_data.rs +++ b/src/signed_data.rs @@ -12,6 +12,7 @@ // ACTION OF CONTRACT, NEGLIGENCE OR OTHER TORTIOUS ACTION, ARISING OUT OF // OR IN CONNECTION WITH THE USE OR PERFORMANCE OF THIS SOFTWARE. +use crate::verify_cert::Budget; use crate::{der, Error}; use ring::signature; @@ -96,7 +97,10 @@ pub(crate) fn verify_signed_data( supported_algorithms: &[&SignatureAlgorithm], spki_value: untrusted::Input, signed_data: &SignedData, + budget: &mut Budget, ) -> Result<(), Error> { + budget.consume_signature()?; + // We need to verify the signature in `signed_data` using the public key // in `public_key`. In order to know which *ring* signature verification // algorithm to use, we need to know the public key algorithm (ECDSA, @@ -438,7 +442,8 @@ mod tests { signed_data::verify_signed_data( SUPPORTED_ALGORITHMS_IN_TESTS, spki_value, - &signed_data + &signed_data, + &mut Budget::default() ) ); } @@ -735,6 +740,7 @@ mod tests { } } + use crate::verify_cert::Budget; use alloc::str::Lines; fn read_pem_section(lines: &mut Lines, section_name: &str) -> Vec { diff --git a/src/verify_cert.rs b/src/verify_cert.rs index 995c20f7..f76d5d6e 100644 --- a/src/verify_cert.rs +++ b/src/verify_cert.rs @@ -175,8 +175,7 @@ fn check_signatures( let mut spki_value = trust_anchor_key; let mut cert = cert_chain; loop { - budget.consume_signature()?; - signed_data::verify_signed_data(supported_sig_algs, spki_value, &cert.signed_data)?; + signed_data::verify_signed_data(supported_sig_algs, spki_value, &cert.signed_data, budget)?; // TODO: check revocation @@ -194,14 +193,14 @@ fn check_signatures( Ok(()) } -struct Budget { +pub(crate) struct Budget { signatures: usize, build_chain_calls: usize, } impl Budget { #[inline] - fn consume_signature(&mut self) -> Result<(), Error> { + pub(crate) fn consume_signature(&mut self) -> Result<(), Error> { self.signatures = self .signatures .checked_sub(1) From 32d8b1c4bd73381aec54bf86a9fb1abbde7c0155 Mon Sep 17 00:00:00 2001 From: Dirkjan Ochtman Date: Tue, 5 Sep 2023 18:04:40 +0200 Subject: [PATCH 07/21] Improve readability of build chain calls budget --- src/verify_cert.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/verify_cert.rs b/src/verify_cert.rs index f76d5d6e..f7e54c77 100644 --- a/src/verify_cert.rs +++ b/src/verify_cert.rs @@ -229,7 +229,7 @@ impl Default for Budget { // This limit is taken from NSS libmozpkix, see: // - build_chain_calls: 200000, + build_chain_calls: 200_000, } } } From 6b78410bd914ed29d39e92a942e5a17ef64b9587 Mon Sep 17 00:00:00 2001 From: Daniel McCarney Date: Thu, 31 Aug 2023 11:34:12 -0400 Subject: [PATCH 08/21] subject_name: clarify var name in check_name_constraints --- src/subject_name/verify.rs | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/src/subject_name/verify.rs b/src/subject_name/verify.rs index 0f50e8a0..1dd81cad 100644 --- a/src/subject_name/verify.rs +++ b/src/subject_name/verify.rs @@ -84,11 +84,11 @@ pub(crate) fn verify_cert_subject_name( // https://tools.ietf.org/html/rfc5280#section-4.2.1.10 pub(crate) fn check_name_constraints( - input: Option<&mut untrusted::Reader>, + constraints: Option<&mut untrusted::Reader>, subordinate_certs: &Cert, subject_common_name_contents: SubjectCommonNameContents, ) -> Result<(), Error> { - let input = match input { + let constraints = match constraints { Some(input) => input, None => { return Ok(()); @@ -105,8 +105,8 @@ pub(crate) fn check_name_constraints( der::expect_tag_and_get_value(inner, subtrees_tag).map(Some) } - let permitted_subtrees = parse_subtrees(input, der::Tag::ContextSpecificConstructed0)?; - let excluded_subtrees = parse_subtrees(input, der::Tag::ContextSpecificConstructed1)?; + let permitted_subtrees = parse_subtrees(constraints, der::Tag::ContextSpecificConstructed0)?; + let excluded_subtrees = parse_subtrees(constraints, der::Tag::ContextSpecificConstructed1)?; let mut child = subordinate_certs; loop { From 7627dff4fabce61a6db391f65108991c6264bbe0 Mon Sep 17 00:00:00 2001 From: Daniel McCarney Date: Fri, 1 Sep 2023 10:07:00 -0400 Subject: [PATCH 09/21] verify_cert: check_signatures -> check_signed_chain This commit renames the `check_signatures` fn, as it will soon be used to do more than simply verifying signatures. --- src/verify_cert.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/verify_cert.rs b/src/verify_cert.rs index f7e54c77..6a888948 100644 --- a/src/verify_cert.rs +++ b/src/verify_cert.rs @@ -104,7 +104,7 @@ fn build_chain_inner( let trust_anchor_spki = untrusted::Input::from(trust_anchor.spki); - check_signatures(supported_sig_algs, cert, trust_anchor_spki, budget)?; + check_signed_chain(supported_sig_algs, cert, trust_anchor_spki, budget)?; Ok(()) }); @@ -166,7 +166,7 @@ fn build_chain_inner( }) } -fn check_signatures( +fn check_signed_chain( supported_sig_algs: &[&SignatureAlgorithm], cert_chain: &Cert, trust_anchor_key: untrusted::Input, From f86f3e26586a014cc3f82ce1a15afb7ce30e87e7 Mon Sep 17 00:00:00 2001 From: Daniel McCarney Date: Tue, 5 Sep 2023 11:11:05 -0400 Subject: [PATCH 10/21] error: alpha sort --- src/error.rs | 58 ++++++++++++++++++++++++++-------------------------- 1 file changed, 29 insertions(+), 29 deletions(-) diff --git a/src/error.rs b/src/error.rs index 1402bd75..bdd3569e 100644 --- a/src/error.rs +++ b/src/error.rs @@ -48,23 +48,44 @@ pub enum Error { /// the notAfter time is earlier than the notBefore time. InvalidCertValidity, + /// A iPAddress name constraint was invalid: + /// - it had a sparse network mask (ie, cannot be written in CIDR form). + /// - it was too long or short + InvalidNetworkMaskConstraint, + /// The signature is invalid for the given public key. InvalidSignatureForPublicKey, + /// The certificate extensions are malformed. + /// + /// In particular, webpki requires the DNS name(s) be in the subjectAltName + /// extension as required by the CA/Browser Forum Baseline Requirements + /// and as recommended by RFC6125. + MalformedExtensions, + + /// The maximum number of internal path building calls has been reached. Path complexity is too great. + MaximumPathBuildCallsExceeded, + + /// The path search was terminated because it became too deep. + MaximumPathDepthExceeded, + + /// The maximum number of signature checks has been reached. Path complexity is too great. + MaximumSignatureChecksExceeded, + /// The certificate violates one or more name constraints. NameConstraintViolation, /// The certificate violates one or more path length constraints. PathLenConstraintViolated, - /// The algorithm in the TBSCertificate "signature" field of a certificate - /// does not match the algorithm in the signature of the certificate. - SignatureAlgorithmMismatch, - /// The certificate is not valid for the Extended Key Usage for which it is /// being validated. RequiredEkuNotFound, + /// The algorithm in the TBSCertificate "signature" field of a certificate + /// does not match the algorithm in the signature of the certificate. + SignatureAlgorithmMismatch, + /// A valid issuer for the certificate could not be found. UnknownIssuer, @@ -74,25 +95,13 @@ pub enum Error { /// is malformed. UnsupportedCertVersion, - /// The certificate extensions are malformed. - /// - /// In particular, webpki requires the DNS name(s) be in the subjectAltName - /// extension as required by the CA/Browser Forum Baseline Requirements - /// and as recommended by RFC6125. - MalformedExtensions, - - /// The maximum number of signature checks has been reached. Path complexity is too great. - MaximumSignatureChecksExceeded, - - /// The maximum number of internal path building calls has been reached. Path complexity is too great. - MaximumPathBuildCallsExceeded, - - /// The path search was terminated because it became too deep. - MaximumPathDepthExceeded, - /// The certificate contains an unsupported critical extension. UnsupportedCriticalExtension, + /// The signature algorithm for a signature is not in the set of supported + /// signature algorithms given. + UnsupportedSignatureAlgorithm, + /// The signature's algorithm does not match the algorithm of the public /// key it is being validated for. This may be because the public key /// algorithm's OID isn't recognized (e.g. DSA), or the public key @@ -101,15 +110,6 @@ pub enum Error { /// algorithm and the signature algorithm simply don't match (e.g. /// verifying an RSA signature with an ECC public key). UnsupportedSignatureAlgorithmForPublicKey, - - /// The signature algorithm for a signature is not in the set of supported - /// signature algorithms given. - UnsupportedSignatureAlgorithm, - - /// A iPAddress name constraint was invalid: - /// - it had a sparse network mask (ie, cannot be written in CIDR form). - /// - it was too long or short - InvalidNetworkMaskConstraint, } impl fmt::Display for Error { From c6d1d1e10f18ed623d40e337947dcab9bc8c5412 Mon Sep 17 00:00:00 2001 From: Daniel McCarney Date: Wed, 6 Sep 2023 10:20:38 -0400 Subject: [PATCH 11/21] verify_cert: pull out `make_issuer` test helper --- src/verify_cert.rs | 55 +++++++++++++++++----------------------------- 1 file changed, 20 insertions(+), 35 deletions(-) diff --git a/src/verify_cert.rs b/src/verify_cert.rs index 6a888948..e2afedbe 100644 --- a/src/verify_cert.rs +++ b/src/verify_cert.rs @@ -451,28 +451,13 @@ mod tests { let alg = &rcgen::PKCS_ECDSA_P256_SHA256; - let make_issuer = || { - let mut ca_params = rcgen::CertificateParams::new(Vec::new()); - ca_params - .distinguished_name - .push(rcgen::DnType::OrganizationName, "Bogus Subject"); - 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 = alg; - rcgen::Certificate::from_params(ca_params).unwrap() - }; - - let ca_cert = make_issuer(); + let ca_cert = make_issuer("Bogus Subject"); let ca_cert_der = ca_cert.serialize_der().unwrap(); let mut intermediates = Vec::with_capacity(intermediate_count); let mut issuer = ca_cert; for _ in 0..intermediate_count { - let intermediate = make_issuer(); + let intermediate = make_issuer("Bogus Subject"); let intermediate_der = intermediate.serialize_der_with_signer(&issuer).unwrap(); intermediates.push(intermediate_der); issuer = intermediate; @@ -529,29 +514,13 @@ mod tests { let alg = &rcgen::PKCS_ECDSA_P256_SHA256; - let make_issuer = |index: usize| { - let mut ca_params = rcgen::CertificateParams::new(Vec::new()); - ca_params.distinguished_name.push( - rcgen::DnType::OrganizationName, - format!("Bogus Subject {index}"), - ); - 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 = alg; - rcgen::Certificate::from_params(ca_params).unwrap() - }; - - let ca_cert = make_issuer(chain_length); + let ca_cert = make_issuer(format!("Bogus Subject {chain_length}")); let ca_cert_der = ca_cert.serialize_der().unwrap(); let mut intermediates = Vec::with_capacity(chain_length); let mut issuer = ca_cert; for i in 0..chain_length { - let intermediate = make_issuer(i); + let intermediate = make_issuer(format!("Bogus Subject {i}")); let intermediate_der = intermediate.serialize_der_with_signer(&issuer).unwrap(); intermediates.push(intermediate_der); issuer = intermediate; @@ -596,4 +565,20 @@ mod tests { // eating the more specific MaximumPathDepthExceeded error. assert_eq!(build_linear_chain(7), Err(Error::UnknownIssuer)); } + + #[cfg(feature = "alloc")] + fn make_issuer(org_name: impl Into) -> 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; + rcgen::Certificate::from_params(ca_params).unwrap() + } } From c9f1f6b93158ea26adbfe2c5be348f695e30dab9 Mon Sep 17 00:00:00 2001 From: Daniel McCarney Date: Wed, 6 Sep 2023 10:24:01 -0400 Subject: [PATCH 12/21] verify_cert: pull out `make_end_entity` test helper --- src/verify_cert.rs | 34 ++++++++++++++++------------------ 1 file changed, 16 insertions(+), 18 deletions(-) diff --git a/src/verify_cert.rs b/src/verify_cert.rs index e2afedbe..103dba7b 100644 --- a/src/verify_cert.rs +++ b/src/verify_cert.rs @@ -449,8 +449,6 @@ mod tests { use crate::ECDSA_P256_SHA256; use crate::{EndEntityCert, Time}; - let alg = &rcgen::PKCS_ECDSA_P256_SHA256; - let ca_cert = make_issuer("Bogus Subject"); let ca_cert_der = ca_cert.serialize_der().unwrap(); @@ -463,15 +461,10 @@ mod tests { issuer = intermediate; } - let mut ee_params = rcgen::CertificateParams::new(vec!["example.com".to_string()]); - ee_params.is_ca = rcgen::IsCa::ExplicitNoCa; - ee_params.alg = alg; - let ee_cert = rcgen::Certificate::from_params(ee_params).unwrap(); - let ee_cert_der = ee_cert.serialize_der_with_signer(&issuer).unwrap(); - + let ee_cert_der = make_end_entity(&issuer); + let cert = EndEntityCert::try_from(&ee_cert_der[..]).unwrap(); let anchors = &[TrustAnchor::try_from_cert_der(&ca_cert_der).unwrap()]; let time = Time::from_seconds_since_unix_epoch(0x1fed_f00d); - let cert = EndEntityCert::try_from(&ee_cert_der[..]).unwrap(); let mut intermediate_certs = intermediates.iter().map(|x| x.as_ref()).collect::>(); if let TrustAnchorIsActualIssuer::No = trust_anchor_is_actual_issuer { @@ -512,8 +505,6 @@ mod tests { use crate::ECDSA_P256_SHA256; use crate::{EndEntityCert, Time}; - let alg = &rcgen::PKCS_ECDSA_P256_SHA256; - let ca_cert = make_issuer(format!("Bogus Subject {chain_length}")); let ca_cert_der = ca_cert.serialize_der().unwrap(); @@ -526,15 +517,10 @@ mod tests { issuer = intermediate; } - let mut ee_params = rcgen::CertificateParams::new(vec!["example.com".to_string()]); - ee_params.is_ca = rcgen::IsCa::ExplicitNoCa; - ee_params.alg = alg; - let ee_cert = rcgen::Certificate::from_params(ee_params).unwrap(); - let ee_cert_der = ee_cert.serialize_der_with_signer(&issuer).unwrap(); - + let ee_cert_der = make_end_entity(&issuer); + let cert = EndEntityCert::try_from(&ee_cert_der[..]).unwrap(); let anchors = &[TrustAnchor::try_from_cert_der(&ca_cert_der).unwrap()]; let time = Time::from_seconds_since_unix_epoch(0x1fed_f00d); - let cert = EndEntityCert::try_from(&ee_cert_der[..]).unwrap(); let intermediates_der = intermediates.iter().map(|x| x.as_ref()).collect::>(); build_chain( @@ -581,4 +567,16 @@ mod tests { ca_params.alg = &rcgen::PKCS_ECDSA_P256_SHA256; rcgen::Certificate::from_params(ca_params).unwrap() } + + #[cfg(feature = "alloc")] + fn make_end_entity(issuer: &rcgen::Certificate) -> Vec { + 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; + + rcgen::Certificate::from_params(ee_params) + .unwrap() + .serialize_der_with_signer(issuer) + .unwrap() + } } From d82c740df07d69a8779fd78b1daa045bbc6fb3a5 Mon Sep 17 00:00:00 2001 From: Daniel McCarney Date: Wed, 6 Sep 2023 10:27:06 -0400 Subject: [PATCH 13/21] verify_cert: pull out `verify_chain` test helper --- src/verify_cert.rs | 66 +++++++++++++++++++++------------------------- 1 file changed, 30 insertions(+), 36 deletions(-) diff --git a/src/verify_cert.rs b/src/verify_cert.rs index 103dba7b..55b4adbe 100644 --- a/src/verify_cert.rs +++ b/src/verify_cert.rs @@ -446,9 +446,6 @@ mod tests { intermediate_count: usize, trust_anchor_is_actual_issuer: TrustAnchorIsActualIssuer, ) -> Error { - use crate::ECDSA_P256_SHA256; - use crate::{EndEntityCert, Time}; - let ca_cert = make_issuer("Bogus Subject"); let ca_cert_der = ca_cert.serialize_der().unwrap(); @@ -461,25 +458,11 @@ mod tests { issuer = intermediate; } - let ee_cert_der = make_end_entity(&issuer); - let cert = EndEntityCert::try_from(&ee_cert_der[..]).unwrap(); - let anchors = &[TrustAnchor::try_from_cert_der(&ca_cert_der).unwrap()]; - let time = Time::from_seconds_since_unix_epoch(0x1fed_f00d); - let mut intermediate_certs = intermediates.iter().map(|x| x.as_ref()).collect::>(); - if let TrustAnchorIsActualIssuer::No = trust_anchor_is_actual_issuer { - intermediate_certs.pop(); + intermediates.pop(); } - build_chain( - EKU_SERVER_AUTH, - &[&ECDSA_P256_SHA256], - anchors, - &intermediate_certs, - cert.inner(), - time, - ) - .unwrap_err() + verify_chain(ca_cert_der, intermediates, make_end_entity(&issuer)).unwrap_err() } #[test] @@ -502,9 +485,6 @@ mod tests { #[cfg(feature = "alloc")] fn build_linear_chain(chain_length: usize) -> Result<(), Error> { - use crate::ECDSA_P256_SHA256; - use crate::{EndEntityCert, Time}; - let ca_cert = make_issuer(format!("Bogus Subject {chain_length}")); let ca_cert_der = ca_cert.serialize_der().unwrap(); @@ -517,20 +497,7 @@ mod tests { issuer = intermediate; } - let ee_cert_der = make_end_entity(&issuer); - let cert = EndEntityCert::try_from(&ee_cert_der[..]).unwrap(); - let anchors = &[TrustAnchor::try_from_cert_der(&ca_cert_der).unwrap()]; - let time = Time::from_seconds_since_unix_epoch(0x1fed_f00d); - let intermediates_der = intermediates.iter().map(|x| x.as_ref()).collect::>(); - - build_chain( - EKU_SERVER_AUTH, - &[&ECDSA_P256_SHA256], - anchors, - &intermediates_der, - cert.inner(), - time, - ) + verify_chain(ca_cert_der, intermediates, make_end_entity(&issuer)) } #[test] @@ -552,6 +519,33 @@ mod tests { assert_eq!(build_linear_chain(7), Err(Error::UnknownIssuer)); } + #[cfg(feature = "alloc")] + fn verify_chain( + trust_anchor_der: Vec, + intermediates_der: Vec>, + ee_cert_der: Vec, + ) -> Result<(), Error> { + use crate::ECDSA_P256_SHA256; + use crate::{EndEntityCert, Time}; + + let anchors = &[TrustAnchor::try_from_cert_der(&trust_anchor_der).unwrap()]; + let time = Time::from_seconds_since_unix_epoch(0x1fed_f00d); + let cert = EndEntityCert::try_from(&ee_cert_der[..]).unwrap(); + let intermediates_der = intermediates_der + .iter() + .map(|x| x.as_ref()) + .collect::>(); + + build_chain( + EKU_SERVER_AUTH, + &[&ECDSA_P256_SHA256], + anchors, + &intermediates_der, + cert.inner(), + time, + ) + } + #[cfg(feature = "alloc")] fn make_issuer(org_name: impl Into) -> rcgen::Certificate { let mut ca_params = rcgen::CertificateParams::new(Vec::new()); From 2b3857fc076991b84a8adbc39685b0c77a408d1c Mon Sep 17 00:00:00 2001 From: Daniel McCarney Date: Tue, 5 Sep 2023 11:28:26 -0400 Subject: [PATCH 14/21] 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, _ => {} } } From debea51dd00828ec5dbfc56d111cf4b2051bbbc1 Mon Sep 17 00:00:00 2001 From: Daniel McCarney Date: Tue, 5 Sep 2023 13:45:11 -0400 Subject: [PATCH 15/21] verify_cert: name constraint checking on verified chain This commit updates the path building process such that name constraints are only evaluated against a complete path where signatures on the chain have been checked successfully to a trust anchor. This avoids: * Parsing name constraints before signatures are validated. * Evaluating name constraints and consuming name constraint comparison budget for certificates that are not part of the built path. In the future it could be possible to interleave the name constraint checking with the signature checking, however the logic for this is more complicated. For an initial fix let's prefer a simpler solution that walks the built + validated path to check name constraints from the trust anchor to the end entity certificate. --- src/verify_cert.rs | 157 ++++++++++++++++++++++++++++++++++++++++----- 1 file changed, 141 insertions(+), 16 deletions(-) diff --git a/src/verify_cert.rs b/src/verify_cert.rs index e72563bd..433c6e36 100644 --- a/src/verify_cert.rs +++ b/src/verify_cert.rs @@ -94,17 +94,18 @@ fn build_chain_inner( return Err(Error::UnknownIssuer); } - 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, budget) - })?; - // TODO: check_distrust(trust_anchor_subject, trust_anchor_spki)?; let trust_anchor_spki = untrusted::Input::from(trust_anchor.spki); - check_signed_chain(supported_sig_algs, cert, trust_anchor_spki, budget)?; + check_signed_chain_name_constraints( + cert, + trust_anchor, + subject_common_name_contents, + budget, + )?; + Ok(()) }); @@ -112,7 +113,8 @@ fn build_chain_inner( match result { Ok(()) => return Ok(()), err @ Err(Error::MaximumSignatureChecksExceeded) - | err @ Err(Error::MaximumPathBuildCallsExceeded) => return err, + | err @ Err(Error::MaximumPathBuildCallsExceeded) + | err @ Err(Error::MaximumNameConstraintComparisonsExceeded) => return err, _ => {} }; @@ -142,10 +144,6 @@ 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, budget) - })?; - let next_sub_ca_count = match used_as_ca { UsedAsCa::No => sub_ca_count, UsedAsCa::Yes => sub_ca_count + 1, @@ -192,6 +190,37 @@ fn check_signed_chain( Ok(()) } +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; + let mut name_constraints = trust_anchor + .name_constraints + .as_ref() + .map(|der| untrusted::Input::from(der)); + + loop { + untrusted::read_all_optional(name_constraints, Error::BadDer, |value| { + subject_name::check_name_constraints(value, cert, subject_common_name_contents, budget) + })?; + + match &cert.ee_or_ca { + EndEntityOrCa::Ca(child_cert) => { + name_constraints = cert.name_constraints; + cert = child_cert; + } + EndEntityOrCa::EndEntity => { + break; + } + } + } + + Ok(()) +} + pub(crate) struct Budget { signatures: usize, build_chain_calls: usize, @@ -460,13 +489,13 @@ mod tests { intermediate_count: usize, trust_anchor_is_actual_issuer: TrustAnchorIsActualIssuer, ) -> Error { - let ca_cert = make_issuer("Bogus Subject"); + let ca_cert = make_issuer("Bogus Subject", None); let ca_cert_der = ca_cert.serialize_der().unwrap(); let mut intermediates = Vec::with_capacity(intermediate_count); let mut issuer = ca_cert; for _ in 0..intermediate_count { - let intermediate = make_issuer("Bogus Subject"); + let intermediate = make_issuer("Bogus Subject", None); let intermediate_der = intermediate.serialize_der_with_signer(&issuer).unwrap(); intermediates.push(intermediate_der); issuer = intermediate; @@ -499,13 +528,13 @@ mod tests { #[cfg(feature = "alloc")] fn build_linear_chain(chain_length: usize) -> Result<(), Error> { - let ca_cert = make_issuer(format!("Bogus Subject {chain_length}")); + let ca_cert = make_issuer(format!("Bogus Subject {chain_length}"), None); let ca_cert_der = ca_cert.serialize_der().unwrap(); let mut intermediates = Vec::with_capacity(chain_length); let mut issuer = ca_cert; for i in 0..chain_length { - let intermediate = make_issuer(format!("Bogus Subject {i}")); + let intermediate = make_issuer(format!("Bogus Subject {i}"), None); let intermediate_der = intermediate.serialize_der_with_signer(&issuer).unwrap(); intermediates.push(intermediate_der); issuer = intermediate; @@ -533,6 +562,98 @@ mod tests { assert_eq!(build_linear_chain(7), Err(Error::UnknownIssuer)); } + #[test] + #[cfg(feature = "alloc")] + fn name_constraint_budget() { + use crate::ECDSA_P256_SHA256; + use crate::{EndEntityCert, Time}; + + // Issue a trust anchor that imposes name constraints. The constraint should match + // the end entity certificate SAN. + let ca_cert = make_issuer( + "Constrained Root", + Some(rcgen::NameConstraints { + permitted_subtrees: vec![rcgen::GeneralSubtree::DnsName(".com".into())], + excluded_subtrees: vec![], + }), + ); + let ca_cert_der = ca_cert.serialize_der().unwrap(); + + // Create a series of intermediate issuers. We'll only use one in the actual built path, + // helping demonstrate that the name constraint budget is not expended checking certificates + // that are not part of the path we compute. + const NUM_INTERMEDIATES: usize = 5; + let mut intermediates = Vec::with_capacity(NUM_INTERMEDIATES); + for i in 0..NUM_INTERMEDIATES { + intermediates.push(make_issuer(format!("Intermediate {i}"), None)); + } + + // Each intermediate should be issued by the trust anchor. + let mut intermediates_der = Vec::with_capacity(NUM_INTERMEDIATES); + for intermediate in &intermediates { + intermediates_der.push(intermediate.serialize_der_with_signer(&ca_cert).unwrap()); + } + + // Create an end-entity cert that is issued by the last of the intermediates. + let ee_cert = make_end_entity(intermediates.last().unwrap()); + + let anchors = &[TrustAnchor::try_from_cert_der(&ca_cert_der).unwrap()]; + let time = Time::from_seconds_since_unix_epoch(0x1fed_f00d); + let cert = EndEntityCert::try_from(&ee_cert[..]).unwrap(); + let intermediates_der = intermediates_der + .iter() + .map(|x| x.as_ref()) + .collect::>(); + + // We use a custom budget to make it easier to write a test, otherwise it is tricky to + // stuff enough names/constraints into the potential chains while staying within the path + // depth limit and the build chain call limit. + let mut passing_budget = Budget { + // One comparison against the intermediate's distinguished name. + // One comparison against the EE's distinguished name. + // One comparison against the EE's SAN. + // = 3 total comparisons. + name_constraint_comparisons: 3, + ..Budget::default() + }; + + // Validation should succeed with the name constraint comparison budget allocated above. + // This shows that we're not consuming budget on unused intermediates: we didn't budget + // enough comparisons for that to pass the overall chain building. + build_chain_inner( + EKU_SERVER_AUTH, + &[&ECDSA_P256_SHA256], + anchors, + &intermediates_der, + cert.inner(), + time, + 0, + &mut passing_budget, + ) + .unwrap(); + + let mut failing_budget = Budget { + // See passing_budget: 2 comparisons is not sufficient. + name_constraint_comparisons: 2, + ..Budget::default() + }; + // Validation should fail when the budget is smaller than the number of comparisons performed + // on the validated path. This demonstrates we properly fail path building when too many + // name constraint comparisons occur. + let result = build_chain_inner( + EKU_SERVER_AUTH, + &[&ECDSA_P256_SHA256], + anchors, + &intermediates_der, + cert.inner(), + time, + 0, + &mut failing_budget, + ); + + assert_eq!(result, Err(Error::MaximumNameConstraintComparisonsExceeded)); + } + #[cfg(feature = "alloc")] fn verify_chain( trust_anchor_der: Vec, @@ -561,7 +682,10 @@ mod tests { } #[cfg(feature = "alloc")] - fn make_issuer(org_name: impl Into) -> rcgen::Certificate { + 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 @@ -573,6 +697,7 @@ mod tests { rcgen::KeyUsagePurpose::CrlSign, ]; ca_params.alg = &rcgen::PKCS_ECDSA_P256_SHA256; + ca_params.name_constraints = name_constraints; rcgen::Certificate::from_params(ca_params).unwrap() } From f34fc7cd8761a6229484828ffb3da0bb3420d9e5 Mon Sep 17 00:00:00 2001 From: Daniel McCarney Date: Thu, 7 Sep 2023 12:49:16 -0400 Subject: [PATCH 16/21] verify_cert: take references in verify_chain helper This commit adjusts the arguments to the `verify_chain` test helper to take references instead of moving the arguments. This makes it easier to use the same inputs for multiple `verify_chain` invocations. --- src/verify_cert.rs | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-) diff --git a/src/verify_cert.rs b/src/verify_cert.rs index 433c6e36..690e72ff 100644 --- a/src/verify_cert.rs +++ b/src/verify_cert.rs @@ -505,7 +505,7 @@ mod tests { intermediates.pop(); } - verify_chain(ca_cert_der, intermediates, make_end_entity(&issuer)).unwrap_err() + verify_chain(&ca_cert_der, &intermediates, &make_end_entity(&issuer)).unwrap_err() } #[test] @@ -540,7 +540,7 @@ mod tests { issuer = intermediate; } - verify_chain(ca_cert_der, intermediates, make_end_entity(&issuer)) + verify_chain(&ca_cert_der, &intermediates, &make_end_entity(&issuer)) } #[test] @@ -656,16 +656,16 @@ mod tests { #[cfg(feature = "alloc")] fn verify_chain( - trust_anchor_der: Vec, - intermediates_der: Vec>, - ee_cert_der: Vec, + trust_anchor_der: &[u8], + intermediates_der: &[Vec], + ee_cert_der: &[u8], ) -> Result<(), Error> { use crate::ECDSA_P256_SHA256; use crate::{EndEntityCert, Time}; - let anchors = &[TrustAnchor::try_from_cert_der(&trust_anchor_der).unwrap()]; + let anchors = &[TrustAnchor::try_from_cert_der(trust_anchor_der).unwrap()]; let time = Time::from_seconds_since_unix_epoch(0x1fed_f00d); - let cert = EndEntityCert::try_from(&ee_cert_der[..]).unwrap(); + let cert = EndEntityCert::try_from(ee_cert_der).unwrap(); let intermediates_der = intermediates_der .iter() .map(|x| x.as_ref()) From 2f25bf36a60a4cc856b067f38d94e6a5be2db00b Mon Sep 17 00:00:00 2001 From: Daniel McCarney Date: Thu, 7 Sep 2023 12:55:17 -0400 Subject: [PATCH 17/21] verify_cert: optional `Budget` arg for `verify_chain` helper This commit updates the `verify_chain` helper to allow providing an optional `Budget` argument (using the default if not provided). This makes it easier to write tests that need to customize the path building budget (e.g. `name_constraint_budget`). --- src/verify_cert.rs | 59 +++++++++++++++++++++------------------------- 1 file changed, 27 insertions(+), 32 deletions(-) diff --git a/src/verify_cert.rs b/src/verify_cert.rs index 690e72ff..c77c8e5f 100644 --- a/src/verify_cert.rs +++ b/src/verify_cert.rs @@ -505,7 +505,13 @@ mod tests { intermediates.pop(); } - verify_chain(&ca_cert_der, &intermediates, &make_end_entity(&issuer)).unwrap_err() + verify_chain( + &ca_cert_der, + &intermediates, + &make_end_entity(&issuer), + None, + ) + .unwrap_err() } #[test] @@ -540,7 +546,12 @@ mod tests { issuer = intermediate; } - verify_chain(&ca_cert_der, &intermediates, &make_end_entity(&issuer)) + verify_chain( + &ca_cert_der, + &intermediates, + &make_end_entity(&issuer), + None, + ) } #[test] @@ -565,9 +576,6 @@ mod tests { #[test] #[cfg(feature = "alloc")] fn name_constraint_budget() { - use crate::ECDSA_P256_SHA256; - use crate::{EndEntityCert, Time}; - // Issue a trust anchor that imposes name constraints. The constraint should match // the end entity certificate SAN. let ca_cert = make_issuer( @@ -597,18 +605,10 @@ mod tests { // Create an end-entity cert that is issued by the last of the intermediates. let ee_cert = make_end_entity(intermediates.last().unwrap()); - let anchors = &[TrustAnchor::try_from_cert_der(&ca_cert_der).unwrap()]; - let time = Time::from_seconds_since_unix_epoch(0x1fed_f00d); - let cert = EndEntityCert::try_from(&ee_cert[..]).unwrap(); - let intermediates_der = intermediates_der - .iter() - .map(|x| x.as_ref()) - .collect::>(); - // We use a custom budget to make it easier to write a test, otherwise it is tricky to // stuff enough names/constraints into the potential chains while staying within the path // depth limit and the build chain call limit. - let mut passing_budget = Budget { + let passing_budget = Budget { // One comparison against the intermediate's distinguished name. // One comparison against the EE's distinguished name. // One comparison against the EE's SAN. @@ -620,19 +620,15 @@ mod tests { // Validation should succeed with the name constraint comparison budget allocated above. // This shows that we're not consuming budget on unused intermediates: we didn't budget // enough comparisons for that to pass the overall chain building. - build_chain_inner( - EKU_SERVER_AUTH, - &[&ECDSA_P256_SHA256], - anchors, + verify_chain( + &ca_cert_der, &intermediates_der, - cert.inner(), - time, - 0, - &mut passing_budget, + &ee_cert, + Some(passing_budget), ) .unwrap(); - let mut failing_budget = Budget { + let failing_budget = Budget { // See passing_budget: 2 comparisons is not sufficient. name_constraint_comparisons: 2, ..Budget::default() @@ -640,15 +636,11 @@ mod tests { // Validation should fail when the budget is smaller than the number of comparisons performed // on the validated path. This demonstrates we properly fail path building when too many // name constraint comparisons occur. - let result = build_chain_inner( - EKU_SERVER_AUTH, - &[&ECDSA_P256_SHA256], - anchors, + let result = verify_chain( + &ca_cert_der, &intermediates_der, - cert.inner(), - time, - 0, - &mut failing_budget, + &ee_cert, + Some(failing_budget), ); assert_eq!(result, Err(Error::MaximumNameConstraintComparisonsExceeded)); @@ -659,6 +651,7 @@ mod tests { trust_anchor_der: &[u8], intermediates_der: &[Vec], ee_cert_der: &[u8], + budget: Option, ) -> Result<(), Error> { use crate::ECDSA_P256_SHA256; use crate::{EndEntityCert, Time}; @@ -671,13 +664,15 @@ mod tests { .map(|x| x.as_ref()) .collect::>(); - build_chain( + build_chain_inner( EKU_SERVER_AUTH, &[&ECDSA_P256_SHA256], anchors, &intermediates_der, cert.inner(), time, + 0, + &mut budget.unwrap_or_default(), ) } From 0f64dc18654c2fab3d88b71b24e5ed1f811d327b Mon Sep 17 00:00:00 2001 From: Daniel McCarney Date: Thu, 7 Sep 2023 12:57:29 -0400 Subject: [PATCH 18/21] error: add is_fatal helper, use in verify_cert This commit adds a method to `Error` for testing whether an error should be considered fatal, e.g. should stop any further path building progress. The existing consideration of fatal errors in `loop_while_non_fatal_error` is updated to use the `is_fatal` fn. Having this in a central place means we can avoid duplicating the match arms in multiple places, where they are likely to fall out-of-sync. --- src/error.rs | 14 ++++++++++++++ src/verify_cert.rs | 6 +++--- 2 files changed, 17 insertions(+), 3 deletions(-) diff --git a/src/error.rs b/src/error.rs index 2b7183a4..c450b89f 100644 --- a/src/error.rs +++ b/src/error.rs @@ -115,6 +115,20 @@ pub enum Error { UnsupportedSignatureAlgorithmForPublicKey, } +impl Error { + /// Returns true for errors that should be considered fatal during path building. Errors of + /// this class should halt any further path building and be returned immediately. + #[inline] + pub(crate) fn is_fatal(&self) -> bool { + matches!( + self, + Error::MaximumSignatureChecksExceeded + | Error::MaximumPathBuildCallsExceeded + | Error::MaximumNameConstraintComparisonsExceeded + ) + } +} + impl fmt::Display for Error { fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result { write!(f, "{:?}", self) diff --git a/src/verify_cert.rs b/src/verify_cert.rs index c77c8e5f..cc977cf1 100644 --- a/src/verify_cert.rs +++ b/src/verify_cert.rs @@ -464,9 +464,9 @@ where // If the error is not fatal, then keep going. match f(v) { Ok(()) => return Ok(()), - err @ Err(Error::MaximumSignatureChecksExceeded) - | err @ Err(Error::MaximumPathBuildCallsExceeded) - | err @ Err(Error::MaximumNameConstraintComparisonsExceeded) => return err, + // Fatal errors should halt further looping. + res @ Err(err) if err.is_fatal() => return res, + // Non-fatal errors should allow looping to continue. _ => {} } } From e592bc00b717c6c3183832a3026e20b74bc6b0dd Mon Sep 17 00:00:00 2001 From: Daniel McCarney Date: Thu, 7 Sep 2023 13:07:41 -0400 Subject: [PATCH 19/21] verify_cert: correct handling of fatal errors Previously the handling of fatal path building errors (e.g. those that should halt all further exploration of the path space) was mishandled such that we could hit the maximum signature budget and still pursue additional path building. This was demonstrated by the `test_too_many_path_calls` unit test which was hitting a `MaximumSignatureChecksExceeded` error, but yet proceeding until hitting a `MaximumPathBuildCallsExceeded` error. This commit updates the error handling between the first and second `loop_while_non_fatal_error` calls to properly terminate the search when a fatal error is encountered, instead of proceeding with further search. The existing `test_too_many_path_calls` test is updated to use an artificially large signature check budget so that we can focus on testing the limit we care about for that test without needing to invest in more complicated test case generation. This avoids hitting a `MaximumSignatureChecksExceeded` error early in the test (which now terminates further path building), instead allowing execution to continue until the maximum path building call budget is expended (matching the previous behaviour and intent of the original test). --- src/verify_cert.rs | 23 +++++++++++++++++------ 1 file changed, 17 insertions(+), 6 deletions(-) diff --git a/src/verify_cert.rs b/src/verify_cert.rs index cc977cf1..5e2f0b6c 100644 --- a/src/verify_cert.rs +++ b/src/verify_cert.rs @@ -112,9 +112,9 @@ fn build_chain_inner( // If the error is not fatal, then keep going. match result { Ok(()) => return Ok(()), - err @ Err(Error::MaximumSignatureChecksExceeded) - | err @ Err(Error::MaximumPathBuildCallsExceeded) - | err @ Err(Error::MaximumNameConstraintComparisonsExceeded) => return err, + // Fatal errors should halt further path building. + res @ Err(err) if err.is_fatal() => return res, + // Non-fatal errors should allow path building to continue. _ => {} }; @@ -488,6 +488,7 @@ mod tests { fn build_degenerate_chain( intermediate_count: usize, trust_anchor_is_actual_issuer: TrustAnchorIsActualIssuer, + budget: Option, ) -> Error { let ca_cert = make_issuer("Bogus Subject", None); let ca_cert_der = ca_cert.serialize_der().unwrap(); @@ -509,7 +510,7 @@ mod tests { &ca_cert_der, &intermediates, &make_end_entity(&issuer), - None, + budget, ) .unwrap_err() } @@ -518,7 +519,7 @@ mod tests { #[cfg(feature = "alloc")] fn test_too_many_signatures() { assert_eq!( - build_degenerate_chain(5, TrustAnchorIsActualIssuer::Yes), + build_degenerate_chain(5, TrustAnchorIsActualIssuer::Yes, None), Error::MaximumSignatureChecksExceeded ); } @@ -527,7 +528,17 @@ mod tests { #[cfg(feature = "alloc")] fn test_too_many_path_calls() { assert_eq!( - build_degenerate_chain(10, TrustAnchorIsActualIssuer::No), + build_degenerate_chain( + 10, + TrustAnchorIsActualIssuer::No, + Some(Budget { + // Crafting a chain that will expend the build chain calls budget without + // first expending the signature checks budget is tricky, so we artificially + // inflate the signature limit to make this test easier to write. + signatures: usize::MAX, + ..Budget::default() + }) + ), Error::MaximumPathBuildCallsExceeded ); } From f56fcbf342cfea16f821d6e0fec91cb35ba9c237 Mon Sep 17 00:00:00 2001 From: Daniel McCarney Date: Thu, 7 Sep 2023 17:37:36 -0400 Subject: [PATCH 20/21] verify_cert: use enum for build chain error The `loop_while_non_fatal_error` helper can return one of three things: * success, when a validated chain to a trust anchor was built. * a fatal error, e.g. when a `Budget` has been exceeded and no further path building should occur because we've exhausted a budget. * a non-fatal error, when a candidate chain results in an error condition, but other paths could be considered if the options are not exhausted. This commit attempts to express this in the type system, centralizing a check for what is/isn't a fatal error and ensuring that downstream callers to `loop_while_non_fatal_error` handle the fatal case appropriately. --- src/error.rs | 12 +++++++ src/verify_cert.rs | 79 +++++++++++++++++++++++++++------------------- 2 files changed, 58 insertions(+), 33 deletions(-) diff --git a/src/error.rs b/src/error.rs index c450b89f..e35994b3 100644 --- a/src/error.rs +++ b/src/error.rs @@ -13,6 +13,7 @@ // OR IN CONNECTION WITH THE USE OR PERFORMANCE OF THIS SOFTWARE. use core::fmt; +use core::ops::ControlFlow; /// An error that occurs during certificate validation or name validation. #[derive(Clone, Copy, Debug, PartialEq, Eq)] @@ -129,6 +130,17 @@ impl Error { } } +impl From for ControlFlow { + fn from(value: Error) -> Self { + match value { + // If an error is fatal, we've exhausted the potential for continued search. + err if err.is_fatal() => Self::Break(err), + // Otherwise we've rejected one candidate chain, but may continue to search for others. + err => Self::Continue(err), + } + } +} + impl fmt::Display for Error { fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result { write!(f, "{:?}", self) diff --git a/src/verify_cert.rs b/src/verify_cert.rs index 5e2f0b6c..5bb2bab1 100644 --- a/src/verify_cert.rs +++ b/src/verify_cert.rs @@ -13,6 +13,7 @@ // OR IN CONNECTION WITH THE USE OR PERFORMANCE OF THIS SOFTWARE. use core::default::Default; +use core::ops::ControlFlow; use crate::{ cert::{self, Cert, EndEntityOrCa}, @@ -38,6 +39,10 @@ pub(crate) fn build_chain( 0, &mut Budget::default(), ) + .map_err(|e| match e { + ControlFlow::Break(err) => err, + ControlFlow::Continue(err) => err, + }) } #[allow(clippy::too_many_arguments)] @@ -50,7 +55,7 @@ fn build_chain_inner( time: time::Time, sub_ca_count: usize, budget: &mut Budget, -) -> Result<(), Error> { +) -> Result<(), ControlFlow> { let used_as_ca = used_as_ca(&cert.ee_or_ca); check_issuer_independent_properties( @@ -68,7 +73,7 @@ fn build_chain_inner( const MAX_SUB_CA_COUNT: usize = 6; if sub_ca_count >= MAX_SUB_CA_COUNT { - return Err(Error::MaximumPathDepthExceeded); + return Err(Error::MaximumPathDepthExceeded.into()); } } UsedAsCa::No => { @@ -91,7 +96,7 @@ fn build_chain_inner( let result = loop_while_non_fatal_error(trust_anchors, |trust_anchor: &TrustAnchor| { let trust_anchor_subject = untrusted::Input::from(trust_anchor.subject); if cert.issuer != trust_anchor_subject { - return Err(Error::UnknownIssuer); + return Err(Error::UnknownIssuer.into()); } // TODO: check_distrust(trust_anchor_subject, trust_anchor_spki)?; @@ -113,9 +118,9 @@ fn build_chain_inner( match result { Ok(()) => return Ok(()), // Fatal errors should halt further path building. - res @ Err(err) if err.is_fatal() => return res, + res @ Err(ControlFlow::Break(_)) => return res, // Non-fatal errors should allow path building to continue. - _ => {} + Err(ControlFlow::Continue(_)) => {} }; loop_while_non_fatal_error(intermediate_certs, |cert_der| { @@ -123,7 +128,7 @@ fn build_chain_inner( cert::parse_cert(untrusted::Input::from(cert_der), EndEntityOrCa::Ca(cert))?; if potential_issuer.subject != cert.issuer { - return Err(Error::UnknownIssuer); + return Err(Error::UnknownIssuer.into()); } // Prevent loops; see RFC 4158 section 5.2. @@ -132,7 +137,7 @@ fn build_chain_inner( if potential_issuer.spki.value() == prev.spki.value() && potential_issuer.subject == prev.subject { - return Err(Error::UnknownIssuer); + return Err(Error::UnknownIssuer.into()); } match &prev.ee_or_ca { EndEntityOrCa::EndEntity => { @@ -168,7 +173,7 @@ fn check_signed_chain( cert_chain: &Cert, trust_anchor_key: untrusted::Input, budget: &mut Budget, -) -> Result<(), Error> { +) -> Result<(), ControlFlow> { let mut spki_value = trust_anchor_key; let mut cert = cert_chain; loop { @@ -195,7 +200,7 @@ fn check_signed_chain_name_constraints( trust_anchor: &TrustAnchor, subject_common_name_contents: subject_name::SubjectCommonNameContents, budget: &mut Budget, -) -> Result<(), Error> { +) -> Result<(), ControlFlow> { let mut cert = cert_chain; let mut name_constraints = trust_anchor .name_constraints @@ -455,8 +460,8 @@ fn check_eku( fn loop_while_non_fatal_error( values: V, - mut f: impl FnMut(V::Item) -> Result<(), Error>, -) -> Result<(), Error> + mut f: impl FnMut(V::Item) -> Result<(), ControlFlow>, +) -> Result<(), ControlFlow> where V: IntoIterator, { @@ -465,12 +470,12 @@ where match f(v) { Ok(()) => return Ok(()), // Fatal errors should halt further looping. - res @ Err(err) if err.is_fatal() => return res, + res @ Err(ControlFlow::Break(_)) => return res, // Non-fatal errors should allow looping to continue. - _ => {} + Err(ControlFlow::Continue(_)) => {} } } - Err(Error::UnknownIssuer) + Err(Error::UnknownIssuer.into()) } #[cfg(test)] @@ -489,7 +494,7 @@ mod tests { intermediate_count: usize, trust_anchor_is_actual_issuer: TrustAnchorIsActualIssuer, budget: Option, - ) -> Error { + ) -> ControlFlow { let ca_cert = make_issuer("Bogus Subject", None); let ca_cert_der = ca_cert.serialize_der().unwrap(); @@ -518,16 +523,16 @@ mod tests { #[test] #[cfg(feature = "alloc")] fn test_too_many_signatures() { - assert_eq!( + assert!(matches!( build_degenerate_chain(5, TrustAnchorIsActualIssuer::Yes, None), - Error::MaximumSignatureChecksExceeded - ); + ControlFlow::Break(Error::MaximumSignatureChecksExceeded) + )); } #[test] #[cfg(feature = "alloc")] fn test_too_many_path_calls() { - assert_eq!( + assert!(matches!( build_degenerate_chain( 10, TrustAnchorIsActualIssuer::No, @@ -539,12 +544,12 @@ mod tests { ..Budget::default() }) ), - Error::MaximumPathBuildCallsExceeded - ); + ControlFlow::Break(Error::MaximumPathBuildCallsExceeded) + )); } #[cfg(feature = "alloc")] - fn build_linear_chain(chain_length: usize) -> Result<(), Error> { + fn build_linear_chain(chain_length: usize) -> Result<(), ControlFlow> { let ca_cert = make_issuer(format!("Bogus Subject {chain_length}"), None); let ca_cert_der = ca_cert.serialize_der().unwrap(); @@ -568,12 +573,12 @@ mod tests { #[test] #[cfg(feature = "alloc")] fn longest_allowed_path() { - assert_eq!(build_linear_chain(1), Ok(())); - assert_eq!(build_linear_chain(2), Ok(())); - assert_eq!(build_linear_chain(3), Ok(())); - assert_eq!(build_linear_chain(4), Ok(())); - assert_eq!(build_linear_chain(5), Ok(())); - assert_eq!(build_linear_chain(6), Ok(())); + assert!(build_linear_chain(1).is_ok()); + assert!(build_linear_chain(2).is_ok()); + assert!(build_linear_chain(3).is_ok()); + assert!(build_linear_chain(4).is_ok()); + assert!(build_linear_chain(5).is_ok()); + assert!(build_linear_chain(6).is_ok()); } #[test] @@ -581,7 +586,10 @@ mod tests { fn path_too_long() { // Note: webpki 0.101.x and earlier surface all non-fatal errors as UnknownIssuer, // eating the more specific MaximumPathDepthExceeded error. - assert_eq!(build_linear_chain(7), Err(Error::UnknownIssuer)); + assert!(matches!( + build_linear_chain(7), + Err(ControlFlow::Continue(Error::UnknownIssuer)) + )); } #[test] @@ -631,13 +639,13 @@ mod tests { // Validation should succeed with the name constraint comparison budget allocated above. // This shows that we're not consuming budget on unused intermediates: we didn't budget // enough comparisons for that to pass the overall chain building. - verify_chain( + assert!(verify_chain( &ca_cert_der, &intermediates_der, &ee_cert, Some(passing_budget), ) - .unwrap(); + .is_ok()); let failing_budget = Budget { // See passing_budget: 2 comparisons is not sufficient. @@ -654,7 +662,12 @@ mod tests { Some(failing_budget), ); - assert_eq!(result, Err(Error::MaximumNameConstraintComparisonsExceeded)); + assert!(matches!( + result, + Err(ControlFlow::Break( + Error::MaximumNameConstraintComparisonsExceeded + )) + )); } #[cfg(feature = "alloc")] @@ -663,7 +676,7 @@ mod tests { intermediates_der: &[Vec], ee_cert_der: &[u8], budget: Option, - ) -> Result<(), Error> { + ) -> Result<(), ControlFlow> { use crate::ECDSA_P256_SHA256; use crate::{EndEntityCert, Time}; From 8a7bf1ac0df9c2bb1ea94e51bdff2613b85da953 Mon Sep 17 00:00:00 2001 From: Daniel McCarney Date: Fri, 8 Sep 2023 12:58:30 -0400 Subject: [PATCH 21/21] Cargo: bump version 0.100.2 -> 0.100.3. --- Cargo.toml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Cargo.toml b/Cargo.toml index dd739ad3..ff975ec0 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -21,7 +21,7 @@ license-file = "LICENSE" name = "rustls-webpki" readme = "README.md" repository = "https://github.com/rustls/webpki" -version = "0.100.2" +version = "0.100.3" include = [ "Cargo.toml",