From 95285b32db0496bca360e61460653b3cb3d96c07 Mon Sep 17 00:00:00 2001 From: Joseph Birr-Pixton Date: Tue, 5 Sep 2023 10:50:10 +0100 Subject: [PATCH 1/5] Track signature limit using `Budget` type --- src/verify_cert.rs | 36 +++++++++++++++++++++++++++--------- 1 file changed, 27 insertions(+), 9 deletions(-) diff --git a/src/verify_cert.rs b/src/verify_cert.rs index e6e078ba..03a3e85e 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); @@ -199,7 +199,7 @@ fn build_chain_inner( cert, trust_anchor, opts.revocation, - signatures, + budget, )?; Ok(()) @@ -244,7 +244,7 @@ fn build_chain_inner( UsedAsCa::Yes => sub_ca_count + 1, }; - build_chain_inner(opts, &potential_issuer, time, next_sub_ca_count, signatures) + 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,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 } + } +} + // 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(()); From a78457cd7a9627c8dab62364a2ea5e34cdeb3ca4 Mon Sep 17 00:00:00 2001 From: Joseph Birr-Pixton Date: Tue, 5 Sep 2023 11:30:03 +0100 Subject: [PATCH 2/5] 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 03a3e85e..d298dbf9 100644 --- a/src/verify_cert.rs +++ b/src/verify_cert.rs @@ -307,7 +307,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 b0d27373034943959da1eb7283091eb2139adc9d Mon Sep 17 00:00:00 2001 From: Joseph Birr-Pixton Date: Tue, 5 Sep 2023 11:44:51 +0100 Subject: [PATCH 3/5] Apply budget to number of calls to `build_chain_inner` --- src/error.rs | 6 ++++- src/verify_cert.rs | 62 +++++++++++++++++++++++++++++++++++++++------- 2 files changed, 58 insertions(+), 10 deletions(-) diff --git a/src/error.rs b/src/error.rs index 987d0a4d..a344ba6c 100644 --- a/src/error.rs +++ b/src/error.rs @@ -96,6 +96,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 violates one or more name constraints. NameConstraintViolation, @@ -238,8 +241,9 @@ impl Error { Error::BadDerTime => 2, Error::BadDer => 1, - // 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 d298dbf9..65eb6aa5 100644 --- a/src/verify_cert.rs +++ b/src/verify_cert.rs @@ -244,6 +244,7 @@ fn build_chain_inner( UsedAsCa::Yes => sub_ca_count + 1, }; + budget.consume_build_chain_call()?; build_chain_inner(opts, &potential_issuer, time, next_sub_ca_count, budget) }) } @@ -292,6 +293,7 @@ fn check_signatures( struct Budget { signatures: usize, + build_chain_calls: usize, } impl Budget { @@ -303,6 +305,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 { @@ -313,6 +324,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, } } } @@ -692,7 +707,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), } } @@ -815,10 +831,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}; @@ -842,9 +865,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); @@ -860,12 +883,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], @@ -875,8 +902,25 @@ 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 ); + } - 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 41be0e6767d023e5085238b8f14fc7afd581e6e3 Mon Sep 17 00:00:00 2001 From: Joseph Birr-Pixton Date: Tue, 5 Sep 2023 12:03:06 +0100 Subject: [PATCH 4/5] Make error ranks more sparse This allows new errors to have minimal diffs. --- src/error.rs | 60 ++++++++++++++++++++++++++-------------------------- 1 file changed, 30 insertions(+), 30 deletions(-) diff --git a/src/error.rs b/src/error.rs index a344ba6c..fd63a403 100644 --- a/src/error.rs +++ b/src/error.rs @@ -201,45 +201,45 @@ 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, // 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 errors - not subject to ranking. Error::MaximumSignatureChecksExceeded => 0, From 1af0ee0a6454178f6fb00c7d6375680b2f86eb92 Mon Sep 17 00:00:00 2001 From: Joseph Birr-Pixton Date: Tue, 5 Sep 2023 12:23:54 +0100 Subject: [PATCH 5/5] Introduce and test for `MaximumPathDepthExceeded` error This is executing a TODO when the chain length exceeds 6 issuers deep. --- src/error.rs | 4 +++ src/verify_cert.rs | 82 ++++++++++++++++++++++++++++++++++++++++++++-- 2 files changed, 84 insertions(+), 2 deletions(-) diff --git a/src/error.rs b/src/error.rs index fd63a403..f3abcf70 100644 --- a/src/error.rs +++ b/src/error.rs @@ -99,6 +99,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 violates one or more name constraints. NameConstraintViolation, @@ -230,6 +233,7 @@ impl Error { Error::UnsupportedRevocationReason => 90, Error::UnsupportedRevocationReasonsPartitioning => 80, Error::UnsupportedCrlIssuingDistributionPoint => 70, + Error::MaximumPathDepthExceeded => 61, // Errors related to malformed data. Error::MalformedDnsIdentifier => 60, diff --git a/src/verify_cert.rs b/src/verify_cert.rs index 65eb6aa5..5c70ae0d 100644 --- a/src/verify_cert.rs +++ b/src/verify_cert.rs @@ -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 => { @@ -923,4 +922,83 @@ mod tests { Error::MaximumPathBuildCallsExceeded ); } + + #[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)); + } }