From f566cf1b3b87558fc8319d0302cdd40bdcd269da Mon Sep 17 00:00:00 2001 From: Daniel McCarney Date: Fri, 8 Sep 2023 14:03:37 -0400 Subject: [PATCH] verify_cert: apply path building budget This is intended to be complementary to the signature validation limit fix and addresses briansmith/webpki#276 in the same manner as NSS libmozpkix. --- src/verify_cert.rs | 92 ++++++++++++++++++++++++++++++++++------------ 1 file changed, 69 insertions(+), 23 deletions(-) diff --git a/src/verify_cert.rs b/src/verify_cert.rs index 9085e29b..a70be964 100644 --- a/src/verify_cert.rs +++ b/src/verify_cert.rs @@ -51,6 +51,8 @@ pub fn build_chain( /// for testing). enum InternalError { MaximumSignatureChecksExceeded, + /// The maximum number of internal path building calls has been reached. Path complexity is too great. + MaximumPathBuildCallsExceeded, } enum ErrorOrInternalError { @@ -62,7 +64,8 @@ impl ErrorOrInternalError { fn is_fatal(&self) -> bool { match self { ErrorOrInternalError::Error(_) => false, - ErrorOrInternalError::InternalError(InternalError::MaximumSignatureChecksExceeded) => { + ErrorOrInternalError::InternalError(InternalError::MaximumSignatureChecksExceeded) + | ErrorOrInternalError::InternalError(InternalError::MaximumPathBuildCallsExceeded) => { true } } @@ -185,6 +188,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, @@ -228,6 +232,7 @@ fn check_signatures( struct Budget { signatures: usize, + build_chain_calls: usize, } impl Budget { @@ -239,6 +244,15 @@ impl Budget { .ok_or(InternalError::MaximumSignatureChecksExceeded)?; Ok(()) } + + #[inline] + fn consume_build_chain_call(&mut self) -> Result<(), InternalError> { + self.build_chain_calls = self + .build_chain_calls + .checked_sub(1) + .ok_or(InternalError::MaximumPathBuildCallsExceeded)?; + Ok(()) + } } impl Default for Budget { @@ -249,6 +263,10 @@ impl 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: 200_000, } } } @@ -453,25 +471,35 @@ where Err(Error::UnknownIssuer.into()) } +#[cfg(feature = "alloc")] #[cfg(test)] mod tests { + use alloc::string::ToString; + use alloc::vec::Vec; + use core::convert::TryFrom; - #[test] - #[cfg(feature = "alloc")] - fn test_too_many_signatures() { - use super::*; + use super::*; + use crate::verify_cert::{Budget, InternalError}; + + enum ChainTrustAnchor { + InChain, + NotInChain, + } + + fn build_degenerate_chain( + intermediate_count: usize, + trust_anchor: ChainTrustAnchor, + ) -> ErrorOrInternalError { use crate::ECDSA_P256_SHA256; use crate::{EndEntityCert, Time}; - use alloc::{string::ToString, vec::Vec}; - use core::convert::TryFrom; let alg = &rcgen::PKCS_ECDSA_P256_SHA256; - let make_issuer = || { + let make_issuer = |org_name| { let mut ca_params = rcgen::CertificateParams::new(Vec::new()); ca_params .distinguished_name - .push(rcgen::DnType::OrganizationName, "Bogus Subject"); + .push(rcgen::DnType::OrganizationName, org_name); ca_params.is_ca = rcgen::IsCa::Ca(rcgen::BasicConstraints::Unconstrained); ca_params.key_usages = vec![ rcgen::KeyUsagePurpose::KeyCertSign, @@ -482,13 +510,17 @@ mod tests { 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(101); + let mut intermediates = Vec::with_capacity(intermediate_count); + if let ChainTrustAnchor::InChain = trust_anchor { + intermediates.push(ca_cert_der.to_vec()); + } + let mut issuer = ca_cert; - for _ in 0..101 { - let intermediate = make_issuer(); + for _ in 0..intermediate_count { + let intermediate = make_issuer("Bogus Subject"); let intermediate_der = intermediate.serialize_der_with_signer(&issuer).unwrap(); intermediates.push(intermediate_der); issuer = intermediate; @@ -500,29 +532,43 @@ mod tests { 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 trust_anchor = match trust_anchor { + ChainTrustAnchor::InChain => make_issuer("Bogus Trust Anchor").serialize_der().unwrap(), + ChainTrustAnchor::NotInChain => ca_cert_der.clone(), + }; + + let anchors = &[TrustAnchor::try_from_cert_der(&trust_anchor).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 intermediate_certs = intermediates.iter().map(|x| x.as_ref()).collect::>(); - // TODO: Use `build_chain` when `Error` is made non-exhaustive. - let result = build_chain_inner( + build_chain_inner( EKU_SERVER_AUTH, &[&ECDSA_P256_SHA256], anchors, - intermediate_certs, + &intermediate_certs, cert.inner(), time, 0, &mut Budget::default(), - ); + ) + .unwrap_err() + } + #[test] + fn test_too_many_signatures() { + assert!(matches!( + build_degenerate_chain(5, ChainTrustAnchor::NotInChain), + ErrorOrInternalError::InternalError(InternalError::MaximumSignatureChecksExceeded), + )); + } + + #[test] + fn test_too_many_path_calls() { + let result = build_degenerate_chain(10, ChainTrustAnchor::InChain); assert!(matches!( result, - Err(ErrorOrInternalError::InternalError( - InternalError::MaximumSignatureChecksExceeded - )) + ErrorOrInternalError::InternalError(InternalError::MaximumPathBuildCallsExceeded), )); } }