diff --git a/src/error.rs b/src/error.rs index 987d0a4d..f3abcf70 100644 --- a/src/error.rs +++ b/src/error.rs @@ -96,6 +96,12 @@ 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 path search was terminated because it became too deep. + MaximumPathDepthExceeded, + /// The certificate violates one or more name constraints. NameConstraintViolation, @@ -198,48 +204,50 @@ impl Error { pub(crate) fn rank(&self) -> u32 { match &self { // Errors related to certificate validity - Error::CertNotValidYet | Error::CertExpired => 29, - Error::CertNotValidForName => 28, - Error::CertRevoked | Error::UnknownRevocationStatus => 27, - Error::InvalidCrlSignatureForPublicKey | Error::InvalidSignatureForPublicKey => 26, - Error::SignatureAlgorithmMismatch => 25, - Error::RequiredEkuNotFound => 24, - Error::NameConstraintViolation => 23, - Error::PathLenConstraintViolated => 22, - Error::CaUsedAsEndEntity | Error::EndEntityUsedAsCa => 21, - Error::IssuerNotCrlSigner => 20, + Error::CertNotValidYet | Error::CertExpired => 290, + Error::CertNotValidForName => 280, + Error::CertRevoked | Error::UnknownRevocationStatus => 270, + Error::InvalidCrlSignatureForPublicKey | Error::InvalidSignatureForPublicKey => 260, + Error::SignatureAlgorithmMismatch => 250, + Error::RequiredEkuNotFound => 240, + Error::NameConstraintViolation => 230, + Error::PathLenConstraintViolated => 220, + Error::CaUsedAsEndEntity | Error::EndEntityUsedAsCa => 210, + Error::IssuerNotCrlSigner => 200, // Errors related to supported features used in an invalid way. - Error::InvalidCertValidity => 19, - Error::InvalidNetworkMaskConstraint => 18, - Error::InvalidSerialNumber => 17, - Error::InvalidCrlNumber => 16, + Error::InvalidCertValidity => 190, + Error::InvalidNetworkMaskConstraint => 180, + Error::InvalidSerialNumber => 170, + Error::InvalidCrlNumber => 160, // Errors related to unsupported features. Error::UnsupportedCrlSignatureAlgorithmForPublicKey - | Error::UnsupportedSignatureAlgorithmForPublicKey => 15, - Error::UnsupportedCrlSignatureAlgorithm | Error::UnsupportedSignatureAlgorithm => 14, - Error::UnsupportedCriticalExtension => 13, - Error::UnsupportedCertVersion => 13, - Error::UnsupportedCrlVersion => 12, - Error::UnsupportedDeltaCrl => 11, - Error::UnsupportedIndirectCrl => 10, - Error::UnsupportedRevocationReason => 9, - Error::UnsupportedRevocationReasonsPartitioning => 8, - Error::UnsupportedCrlIssuingDistributionPoint => 7, + | Error::UnsupportedSignatureAlgorithmForPublicKey => 150, + Error::UnsupportedCrlSignatureAlgorithm | Error::UnsupportedSignatureAlgorithm => 140, + Error::UnsupportedCriticalExtension => 130, + Error::UnsupportedCertVersion => 130, + Error::UnsupportedCrlVersion => 120, + Error::UnsupportedDeltaCrl => 110, + Error::UnsupportedIndirectCrl => 100, + Error::UnsupportedRevocationReason => 90, + Error::UnsupportedRevocationReasonsPartitioning => 80, + Error::UnsupportedCrlIssuingDistributionPoint => 70, + Error::MaximumPathDepthExceeded => 61, // Errors related to malformed data. - Error::MalformedDnsIdentifier => 6, - Error::MalformedNameConstraint => 5, - Error::MalformedExtensions | Error::TrailingData(_) => 4, - Error::ExtensionValueInvalid => 3, + Error::MalformedDnsIdentifier => 60, + Error::MalformedNameConstraint => 50, + Error::MalformedExtensions | Error::TrailingData(_) => 40, + Error::ExtensionValueInvalid => 30, // Generic DER errors. - Error::BadDerTime => 2, - Error::BadDer => 1, + Error::BadDerTime => 20, + Error::BadDer => 10, - // Special case error - not subject to ranking. + // Special case errors - not subject to ranking. Error::MaximumSignatureChecksExceeded => 0, + Error::MaximumPathBuildCallsExceeded => 0, // Default catch all error - should be renamed in the future. Error::UnknownIssuer => 0, diff --git a/src/verify_cert.rs b/src/verify_cert.rs index e6e078ba..5c70ae0d 100644 --- a/src/verify_cert.rs +++ b/src/verify_cert.rs @@ -130,7 +130,7 @@ pub(crate) struct ChainOptions<'a> { } pub(crate) fn build_chain(opts: &ChainOptions, cert: &Cert, time: time::Time) -> Result<(), Error> { - build_chain_inner(opts, cert, time, 0, &mut 0_usize) + build_chain_inner(opts, cert, time, 0, &mut Budget::default()) } fn build_chain_inner( @@ -138,7 +138,7 @@ fn build_chain_inner( 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); @@ -151,8 +151,7 @@ fn build_chain_inner( const MAX_SUB_CA_COUNT: usize = 6; if sub_ca_count >= MAX_SUB_CA_COUNT { - // TODO(XXX): Candidate for a more specific error - Error::PathTooDeep? - return Err(Error::UnknownIssuer); + return Err(Error::MaximumPathDepthExceeded); } } UsedAsCa::No => { @@ -199,7 +198,7 @@ fn build_chain_inner( cert, trust_anchor, opts.revocation, - signatures, + budget, )?; Ok(()) @@ -244,7 +243,8 @@ fn build_chain_inner( UsedAsCa::Yes => sub_ca_count + 1, }; - build_chain_inner(opts, &potential_issuer, time, next_sub_ca_count, signatures) + budget.consume_build_chain_call()?; + build_chain_inner(opts, &potential_issuer, time, next_sub_ca_count, budget) }) } @@ -253,17 +253,14 @@ fn check_signatures( cert_chain: &Cert, trust_anchor: &TrustAnchor, revocation: Option, - signatures: &mut usize, + budget: &mut Budget, ) -> Result<(), Error> { let mut spki_value = untrusted::Input::from(trust_anchor.subject_public_key_info.as_ref()); let mut issuer_subject = untrusted::Input::from(trust_anchor.subject.as_ref()); let mut issuer_key_usage = None; // TODO(XXX): Consider whether to track TrustAnchor KU. 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)?; if let Some(revocation_opts) = &revocation { @@ -293,6 +290,47 @@ fn check_signatures( Ok(()) } +struct Budget { + signatures: usize, + build_chain_calls: usize, +} + +impl Budget { + #[inline] + fn consume_signature(&mut self) -> Result<(), Error> { + self.signatures = self + .signatures + .checked_sub(1) + .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 { + fn default() -> Self { + 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, + + // This limit is taken from NSS libmozpkix, see: + // + build_chain_calls: 200000, + } + } +} + // Zero-sized marker type representing positive assertion that revocation status was checked // for a certificate and the result was that the certificate is not revoked. struct CertNotRevoked(()); @@ -668,7 +706,8 @@ where for v in values { match f(v) { Ok(()) => return Ok(()), - err @ Err(Error::MaximumSignatureChecksExceeded) => return err, + err @ Err(Error::MaximumSignatureChecksExceeded) + | err @ Err(Error::MaximumPathBuildCallsExceeded) => return err, Err(new_error) => error = error.most_specific(new_error), } } @@ -791,10 +830,17 @@ mod tests { assert!(crl_authoritative(&crl, &ee)); } - #[test] #[cfg(feature = "alloc")] - fn test_too_many_signatures() { - use crate::types::CertificateDer; + enum TrustAnchorIsActualIssuer { + Yes, + No, + } + + #[cfg(feature = "alloc")] + fn build_degenerate_chain( + intermediate_count: usize, + trust_anchor_is_actual_issuer: TrustAnchorIsActualIssuer, + ) -> Error { use crate::{extract_trust_anchor, ECDSA_P256_SHA256}; use crate::{EndEntityCert, Time}; @@ -818,9 +864,9 @@ mod tests { let ca_cert = make_issuer(); let ca_cert_der = CertificateDer::from(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); @@ -836,12 +882,16 @@ mod tests { let anchors = &[extract_trust_anchor(&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 + let mut intermediates_der = intermediates .iter() .map(|x| CertificateDer::from(x.as_ref())) .collect::>(); - let result = build_chain( + if let TrustAnchorIsActualIssuer::No = trust_anchor_is_actual_issuer { + intermediates_der.pop(); + } + + build_chain( &ChainOptions { eku: KeyUsage::server_auth(), supported_sig_algs: &[ECDSA_P256_SHA256], @@ -851,8 +901,104 @@ mod tests { }, cert.inner(), time, + ) + .unwrap_err() + } + + #[test] + #[cfg(feature = "alloc")] + fn test_too_many_signatures() { + assert_eq!( + build_degenerate_chain(5, TrustAnchorIsActualIssuer::Yes), + Error::MaximumSignatureChecksExceeded ); + } + + #[test] + #[cfg(feature = "alloc")] + fn test_too_many_path_calls() { + assert_eq!( + build_degenerate_chain(10, TrustAnchorIsActualIssuer::No), + Error::MaximumPathBuildCallsExceeded + ); + } - assert!(matches!(result, Err(Error::MaximumSignatureChecksExceeded))); + #[cfg(feature = "alloc")] + fn build_linear_chain(chain_length: usize) -> Result<(), Error> { + use crate::{extract_trust_anchor, 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 = CertificateDer::from(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 = CertificateDer::from(ee_cert.serialize_der_with_signer(&issuer).unwrap()); + + let anchors = &[extract_trust_anchor(&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| CertificateDer::from(x.as_ref())) + .collect::>(); + + build_chain( + &ChainOptions { + eku: KeyUsage::server_auth(), + supported_sig_algs: &[ECDSA_P256_SHA256], + trust_anchors: anchors, + intermediate_certs: &intermediates_der, + revocation: None, + }, + 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() { + assert_eq!(build_linear_chain(7), Err(Error::MaximumPathDepthExceeded)); } }