From f4755962f208665f0506f8c8f7d82cd3d8df5f3a Mon Sep 17 00:00:00 2001 From: Joseph Birr-Pixton Date: Wed, 23 Aug 2023 09:36:55 +0100 Subject: [PATCH 01/28] Pin specific rcgen commit --- Cargo.toml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Cargo.toml b/Cargo.toml index db3e90ae..2bf8772b 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -101,4 +101,4 @@ harness = false [patch.crates-io] # TODO(XXX): Remove this once rcgen has cut a release w/ CRL support included. -rcgen = { git = 'https://github.com/est31/rcgen.git' } +rcgen = { git = 'https://github.com/est31/rcgen.git', rev = '83e548a06848d923eada1ac66d1a912735b67e79' } From b9ec1f14c38a89fed2a9a5924c651bd189358990 Mon Sep 17 00:00:00 2001 From: Daniel McCarney Date: Fri, 28 Jul 2023 16:07:39 -0400 Subject: [PATCH 02/28] misc: clippy fixes --- src/crl.rs | 2 ++ tests/custom_ekus.rs | 2 +- 2 files changed, 3 insertions(+), 1 deletion(-) diff --git a/src/crl.rs b/src/crl.rs index 0c47e4a3..a5598422 100644 --- a/src/crl.rs +++ b/src/crl.rs @@ -631,6 +631,8 @@ mod tests { #[test] #[cfg(feature = "alloc")] + // redundant clone, clone_on_copy allowed to verify derived traits. + #[allow(clippy::redundant_clone, clippy::clone_on_copy)] fn test_derived_traits() { let crl = crate::crl::BorrowedCertRevocationList::from_der(include_bytes!( "../tests/crls/crl.valid.der" diff --git a/tests/custom_ekus.rs b/tests/custom_ekus.rs index 0fc094df..4a9e082c 100644 --- a/tests/custom_ekus.rs +++ b/tests/custom_ekus.rs @@ -31,7 +31,7 @@ pub fn verify_custom_eku_mdoc() { let ee = include_bytes!("misc/mdoc_eku.ee.der"); let ca = include_bytes!("misc/mdoc_eku.ca.der"); - let eku_mdoc = KeyUsage::required(&[(40 * 1) + 0, 129, 140, 93, 5, 1, 2]); + let eku_mdoc = KeyUsage::required(&[40, 129, 140, 93, 5, 1, 2]); check_cert(ee, ca, eku_mdoc, time, Ok(())); check_cert(ee, ca, KeyUsage::server_auth(), time, err); check_cert(ee, ca, eku_mdoc, time, Ok(())); From a65cc14645feba8625a5611f31dc2ba108a83b9f Mon Sep 17 00:00:00 2001 From: Joseph Birr-Pixton Date: Wed, 23 Aug 2023 09:41:31 +0100 Subject: [PATCH 03/28] Fix `expect_fun_call` clippy lints This wasn't an actual issue, but the fix is equivalent. --- tests/better_tls.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/better_tls.rs b/tests/better_tls.rs index 306b283b..b1718b63 100644 --- a/tests/better_tls.rs +++ b/tests/better_tls.rs @@ -15,7 +15,7 @@ pub fn path_building() { let path_building_suite = better_tls .suites .get("pathbuilding") - .expect("missing pathbuilding suite"); + .unwrap_or_else(|| panic!("missing pathbuilding suite")); for testcase in &path_building_suite.test_cases { println!("Testing path building test case {:?}", testcase.id); From 72b07cc562b254eedb7ad14f4d4f5e19afb71749 Mon Sep 17 00:00:00 2001 From: Joseph Birr-Pixton Date: Tue, 5 Sep 2023 10:50:10 +0100 Subject: [PATCH 04/28] 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 1b428337..6149cc50 100644 --- a/src/verify_cert.rs +++ b/src/verify_cert.rs @@ -27,7 +27,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( @@ -35,7 +35,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); @@ -93,7 +93,7 @@ fn build_chain_inner( cert, trust_anchor, opts.crls, - signatures, + budget, )?; Ok(()) @@ -140,7 +140,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) }) } @@ -149,17 +149,14 @@ fn check_signatures( cert_chain: &Cert, trust_anchor: &TrustAnchor, crls: &[&dyn CertRevocationList], - signatures: &mut usize, + budget: &mut Budget, ) -> Result<(), Error> { let mut spki_value = untrusted::Input::from(trust_anchor.spki); let mut issuer_subject = untrusted::Input::from(trust_anchor.subject); 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 !crls.is_empty() { @@ -189,6 +186,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 387afe2635999e3ddbf75d71b8f4c041f5466a09 Mon Sep 17 00:00:00 2001 From: Joseph Birr-Pixton Date: Tue, 5 Sep 2023 11:30:03 +0100 Subject: [PATCH 05/28] 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 6149cc50..d6e55068 100644 --- a/src/verify_cert.rs +++ b/src/verify_cert.rs @@ -203,7 +203,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 cece6391c86cf0f3d327f68040c3aaa7c1cd6931 Mon Sep 17 00:00:00 2001 From: Joseph Birr-Pixton Date: Tue, 5 Sep 2023 11:44:51 +0100 Subject: [PATCH 06/28] Apply budget to number of calls to `build_chain_inner` --- src/error.rs | 6 ++++- src/verify_cert.rs | 64 ++++++++++++++++++++++++++++++++++++++-------- 2 files changed, 59 insertions(+), 11 deletions(-) diff --git a/src/error.rs b/src/error.rs index b129c836..dc417fdf 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, @@ -223,8 +226,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 d6e55068..6deaa869 100644 --- a/src/verify_cert.rs +++ b/src/verify_cert.rs @@ -140,6 +140,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) }) } @@ -188,6 +189,7 @@ fn check_signatures( struct Budget { signatures: usize, + build_chain_calls: usize, } impl Budget { @@ -199,6 +201,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 { @@ -209,6 +220,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, } } } @@ -532,7 +547,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), } } @@ -549,9 +565,17 @@ mod tests { .key_purpose_id_equals(EKU_SERVER_AUTH.oid_value)) } - #[test] #[cfg(feature = "alloc")] - fn test_too_many_signatures() { + enum TrustAnchorIsActualIssuer { + Yes, + No, + } + + #[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}; @@ -575,9 +599,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); @@ -593,21 +617,41 @@ 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::>(); - let result = build_chain( + if let TrustAnchorIsActualIssuer::No = trust_anchor_is_actual_issuer { + intermediate_certs.pop(); + } + + build_chain( &ChainOptions { eku: KeyUsage::server_auth(), supported_sig_algs: &[&ECDSA_P256_SHA256], trust_anchors: anchors, - intermediate_certs, + intermediate_certs: &intermediate_certs, crls: &[], }, 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 575ab1f6dbf70ff49817aae2bab3f9d80ce0e7b8 Mon Sep 17 00:00:00 2001 From: Joseph Birr-Pixton Date: Tue, 5 Sep 2023 12:03:06 +0100 Subject: [PATCH 07/28] Make error ranks more sparse This allows new errors to have minimal diffs. --- src/error.rs | 56 ++++++++++++++++++++++++++-------------------------- 1 file changed, 28 insertions(+), 28 deletions(-) diff --git a/src/error.rs b/src/error.rs index dc417fdf..cf397ca7 100644 --- a/src/error.rs +++ b/src/error.rs @@ -188,43 +188,43 @@ impl Error { pub(crate) fn rank(&self) -> u32 { match &self { // Errors related to certificate validity - Error::CertNotValidYet | Error::CertExpired => 27, - Error::CertNotValidForName => 26, - Error::CertRevoked => 25, - Error::InvalidCrlSignatureForPublicKey | Error::InvalidSignatureForPublicKey => 24, - Error::SignatureAlgorithmMismatch => 23, - Error::RequiredEkuNotFound => 22, - Error::NameConstraintViolation => 21, - Error::PathLenConstraintViolated => 20, - Error::CaUsedAsEndEntity | Error::EndEntityUsedAsCa => 19, - Error::IssuerNotCrlSigner => 18, + Error::CertNotValidYet | Error::CertExpired => 290, + Error::CertNotValidForName => 280, + Error::CertRevoked => 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 => 17, - Error::InvalidNetworkMaskConstraint => 16, - Error::InvalidSerialNumber => 15, - Error::InvalidCrlNumber => 14, + Error::InvalidCertValidity => 190, + Error::InvalidNetworkMaskConstraint => 180, + Error::InvalidSerialNumber => 170, + Error::InvalidCrlNumber => 160, // Errors related to unsupported features. Error::UnsupportedCrlSignatureAlgorithmForPublicKey - | Error::UnsupportedSignatureAlgorithmForPublicKey => 13, - Error::UnsupportedCrlSignatureAlgorithm | Error::UnsupportedSignatureAlgorithm => 12, - Error::UnsupportedCriticalExtension => 11, - Error::UnsupportedCertVersion => 11, - Error::UnsupportedCrlVersion => 10, - Error::UnsupportedDeltaCrl => 9, - Error::UnsupportedIndirectCrl => 8, - Error::UnsupportedRevocationReason => 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, // Errors related to malformed data. - Error::MalformedDnsIdentifier => 6, - Error::MalformedNameConstraint => 5, - Error::MalformedExtensions => 4, - Error::ExtensionValueInvalid => 3, + Error::MalformedDnsIdentifier => 60, + Error::MalformedNameConstraint => 50, + Error::MalformedExtensions => 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 2b2d5d6d010751ab45d66721d2da0f3fa1c792a2 Mon Sep 17 00:00:00 2001 From: Joseph Birr-Pixton Date: Tue, 5 Sep 2023 12:23:54 +0100 Subject: [PATCH 08/28] Introduce and test for `MaximumPathDepthExceeded` error This is executing a TODO when the chain length exceeds 6 issuers deep. --- src/error.rs | 7 ++++ src/verify_cert.rs | 79 ++++++++++++++++++++++++++++++++++++++++++++-- 2 files changed, 84 insertions(+), 2 deletions(-) diff --git a/src/error.rs b/src/error.rs index cf397ca7..0d7bcea0 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, @@ -215,6 +218,10 @@ impl Error { Error::UnsupportedDeltaCrl => 110, Error::UnsupportedIndirectCrl => 100, Error::UnsupportedRevocationReason => 90, + // Reserved for webpki 0.102.0+ usages: + // 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 6deaa869..694f6591 100644 --- a/src/verify_cert.rs +++ b/src/verify_cert.rs @@ -48,8 +48,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 => { @@ -654,4 +653,80 @@ 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( + &ChainOptions { + eku: KeyUsage::server_auth(), + supported_sig_algs: &[&ECDSA_P256_SHA256], + trust_anchors: anchors, + intermediate_certs: &intermediates_der, + crls: &[], + }, + 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)); + } } From 92ad878211d1dff71a78b1e5513ff454f6f6aeef Mon Sep 17 00:00:00 2001 From: Dirkjan Ochtman Date: Tue, 5 Sep 2023 17:57:15 +0200 Subject: [PATCH 09/28] 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 694f6591..48d77005 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::{Cert, EndEntityOrCa}, der, signed_data, subject_name, time, CertRevocationList, Error, SignatureAlgorithm, @@ -211,7 +213,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 667370a35d4af1165b3181bfbc9c8f513f46255b Mon Sep 17 00:00:00 2001 From: Dirkjan Ochtman Date: Tue, 5 Sep 2023 18:04:16 +0200 Subject: [PATCH 10/28] Force all signature verification operations to consume budget --- src/crl.rs | 6 ++++++ src/signed_data.rs | 11 ++++++++++- src/verify_cert.rs | 11 ++++++----- 3 files changed, 22 insertions(+), 6 deletions(-) diff --git a/src/crl.rs b/src/crl.rs index a5598422..25a4997c 100644 --- a/src/crl.rs +++ b/src/crl.rs @@ -15,6 +15,7 @@ use crate::cert::lenient_certificate_serial_number; use crate::der::Tag; use crate::signed_data::{self, SignedData}; +use crate::verify_cert::Budget; use crate::x509::{remember_extension, set_extension_once, Extension}; use crate::{der, Error, SignatureAlgorithm, Time}; @@ -42,6 +43,7 @@ pub trait CertRevocationList: Sealed { &self, supported_sig_algs: &[&SignatureAlgorithm], issuer_spki: &[u8], + budget: &mut Budget, ) -> Result<(), Error>; } @@ -85,11 +87,13 @@ impl CertRevocationList for OwnedCertRevocationList { &self, supported_sig_algs: &[&SignatureAlgorithm], issuer_spki: &[u8], + budget: &mut Budget, ) -> Result<(), Error> { signed_data::verify_signed_data( supported_sig_algs, untrusted::Input::from(issuer_spki), &self.signed_data.borrow(), + budget, ) } } @@ -329,11 +333,13 @@ impl CertRevocationList for BorrowedCertRevocationList<'_> { &self, supported_sig_algs: &[&SignatureAlgorithm], issuer_spki: &[u8], + budget: &mut Budget, ) -> Result<(), Error> { signed_data::verify_signed_data( supported_sig_algs, untrusted::Input::from(issuer_spki), &self.signed_data, + budget, ) } } diff --git a/src/signed_data.rs b/src/signed_data.rs index 0b9856eb..d7d5da4d 100644 --- a/src/signed_data.rs +++ b/src/signed_data.rs @@ -12,9 +12,13 @@ // 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; +#[cfg(feature = "alloc")] +use alloc::vec::Vec; + /// X.509 certificates and related items that are signed are almost always /// encoded in the format "tbs||signatureAlgorithm||signature". This structure /// captures this pattern as an owned data type. @@ -152,7 +156,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, @@ -498,7 +505,8 @@ mod tests { signed_data::verify_signed_data( SUPPORTED_ALGORITHMS_IN_TESTS, spki_value, - &signed_data + &signed_data, + &mut Budget::default() ) ); } @@ -795,6 +803,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 48d77005..a2871e7f 100644 --- a/src/verify_cert.rs +++ b/src/verify_cert.rs @@ -158,8 +158,7 @@ fn check_signatures( let mut issuer_key_usage = None; // TODO(XXX): Consider whether to track TrustAnchor KU. 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)?; if !crls.is_empty() { check_crls( @@ -169,6 +168,7 @@ fn check_signatures( spki_value, issuer_key_usage, crls, + budget, )?; } @@ -188,14 +188,14 @@ fn check_signatures( Ok(()) } -struct Budget { +pub 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) @@ -247,6 +247,7 @@ fn check_crls( issuer_spki: untrusted::Input, issuer_ku: Option, crls: &[&dyn CertRevocationList], + budget: &mut Budget, ) -> Result, Error> { assert_eq!(cert.issuer, issuer_subject); @@ -262,7 +263,7 @@ fn check_crls( // TODO(XXX): consider whether we can refactor so this happens once up-front, instead // of per-lookup. // https://github.com/rustls/webpki/issues/81 - crl.verify_signature(supported_sig_algs, issuer_spki.as_slice_less_safe()) + crl.verify_signature(supported_sig_algs, issuer_spki.as_slice_less_safe(), budget) .map_err(crl_signature_err)?; // Verify that if the issuer has a KeyUsage bitstring it asserts cRLSign. From f50e54ab1b80d74b523eccd2230772c36ebd090c Mon Sep 17 00:00:00 2001 From: Dirkjan Ochtman Date: Tue, 5 Sep 2023 18:04:40 +0200 Subject: [PATCH 11/28] 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 a2871e7f..8f81e7f9 100644 --- a/src/verify_cert.rs +++ b/src/verify_cert.rs @@ -224,7 +224,7 @@ impl Default for Budget { // This limit is taken from NSS libmozpkix, see: // - build_chain_calls: 200000, + build_chain_calls: 200_000, } } } From e0729cfc7ff2897b7d34f7c58b02622209f5c1dc Mon Sep 17 00:00:00 2001 From: Daniel McCarney Date: Thu, 31 Aug 2023 11:34:12 -0400 Subject: [PATCH 12/28] 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 ba6ff31d..f395e765 100644 --- a/src/subject_name/verify.rs +++ b/src/subject_name/verify.rs @@ -86,11 +86,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(()); @@ -107,8 +107,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 3a079857ea3107c0f69e967d2ac1fe5a80b0fdf5 Mon Sep 17 00:00:00 2001 From: Daniel McCarney Date: Fri, 1 Sep 2023 10:07:00 -0400 Subject: [PATCH 13/28] verify_cert: check_signatures -> check_signed_chain This commit renames the `check_signatures` fn, as it is doing more than simply verifying signatures. It also checks revocation status w/ CRLs when appropriate. --- 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 8f81e7f9..a591fac4 100644 --- a/src/verify_cert.rs +++ b/src/verify_cert.rs @@ -89,7 +89,7 @@ fn build_chain_inner( // TODO: check_distrust(trust_anchor_subject, trust_anchor_spki)?; - check_signatures( + check_signed_chain( opts.supported_sig_algs, cert, trust_anchor, @@ -146,7 +146,7 @@ fn build_chain_inner( }) } -fn check_signatures( +fn check_signed_chain( supported_sig_algs: &[&SignatureAlgorithm], cert_chain: &Cert, trust_anchor: &TrustAnchor, From 0ccf679fb99c1fcf9729c5a8fb6b65affd93816d Mon Sep 17 00:00:00 2001 From: Daniel McCarney Date: Tue, 5 Sep 2023 11:11:05 -0400 Subject: [PATCH 14/28] error: alpha sort --- src/error.rs | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/src/error.rs b/src/error.rs index 0d7bcea0..c9f717e1 100644 --- a/src/error.rs +++ b/src/error.rs @@ -93,15 +93,15 @@ pub enum Error { /// invalid labels. MalformedNameConstraint, - /// 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 maximum number of signature checks has been reached. Path complexity is too great. + MaximumSignatureChecksExceeded, + /// The certificate violates one or more name constraints. NameConstraintViolation, From 2c555d9039fa004c4eca68a99f7cacbf5edf669c Mon Sep 17 00:00:00 2001 From: Daniel McCarney Date: Wed, 6 Sep 2023 10:20:38 -0400 Subject: [PATCH 15/28] 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 a591fac4..50dc687b 100644 --- a/src/verify_cert.rs +++ b/src/verify_cert.rs @@ -583,28 +583,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; @@ -664,29 +649,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; @@ -732,4 +701,20 @@ mod tests { fn path_too_long() { assert_eq!(build_linear_chain(7), Err(Error::MaximumPathDepthExceeded)); } + + #[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 315d8165d3ba000073f3a27d357da58605d9d7fa Mon Sep 17 00:00:00 2001 From: Daniel McCarney Date: Wed, 6 Sep 2023 10:24:01 -0400 Subject: [PATCH 16/28] 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 50dc687b..de16f1a2 100644 --- a/src/verify_cert.rs +++ b/src/verify_cert.rs @@ -581,8 +581,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(); @@ -595,15 +593,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 { @@ -647,8 +640,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(); @@ -661,15 +652,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( @@ -717,4 +703,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 e1423ded635db9b9c7e1f963d312a74e86a623f2 Mon Sep 17 00:00:00 2001 From: Daniel McCarney Date: Wed, 6 Sep 2023 10:27:06 -0400 Subject: [PATCH 17/28] verify_cert: pull out `verify_chain` test helper --- src/verify_cert.rs | 75 ++++++++++++++++++++-------------------------- 1 file changed, 33 insertions(+), 42 deletions(-) diff --git a/src/verify_cert.rs b/src/verify_cert.rs index de16f1a2..0c228b33 100644 --- a/src/verify_cert.rs +++ b/src/verify_cert.rs @@ -578,9 +578,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(); @@ -593,28 +590,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( - &ChainOptions { - eku: KeyUsage::server_auth(), - supported_sig_algs: &[&ECDSA_P256_SHA256], - trust_anchors: anchors, - intermediate_certs: &intermediate_certs, - crls: &[], - }, - cert.inner(), - time, - ) - .unwrap_err() + verify_chain(ca_cert_der, intermediates, make_end_entity(&issuer)).unwrap_err() } #[test] @@ -637,9 +617,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(); @@ -652,23 +629,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( - &ChainOptions { - eku: KeyUsage::server_auth(), - supported_sig_algs: &[&ECDSA_P256_SHA256], - trust_anchors: anchors, - intermediate_certs: &intermediates_der, - crls: &[], - }, - cert.inner(), - time, - ) + verify_chain(ca_cert_der, intermediates, make_end_entity(&issuer)) } #[test] @@ -688,6 +649,36 @@ mod tests { assert_eq!(build_linear_chain(7), Err(Error::MaximumPathDepthExceeded)); } + #[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( + &ChainOptions { + eku: KeyUsage::server_auth(), + supported_sig_algs: &[&ECDSA_P256_SHA256], + trust_anchors: anchors, + intermediate_certs: &intermediates_der, + crls: &[], + }, + 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 3401dd1395c2bae949b57bc965ee055b8fe1b9b3 Mon Sep 17 00:00:00 2001 From: Daniel McCarney Date: Tue, 5 Sep 2023 11:28:26 -0400 Subject: [PATCH 18/28] 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 | 4 ++++ src/subject_name/verify.rs | 22 +++++++++++++++++----- src/verify_cert.rs | 24 ++++++++++++++++++++++-- 3 files changed, 43 insertions(+), 7 deletions(-) diff --git a/src/error.rs b/src/error.rs index c9f717e1..8699be51 100644 --- a/src/error.rs +++ b/src/error.rs @@ -93,6 +93,9 @@ pub enum Error { /// invalid labels. MalformedNameConstraint, + /// 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, @@ -236,6 +239,7 @@ impl Error { // Special case errors - not subject to ranking. Error::MaximumSignatureChecksExceeded => 0, Error::MaximumPathBuildCallsExceeded => 0, + Error::MaximumNameConstraintComparisonsExceeded => 0, // Default catch all error - should be renamed in the future. Error::UnknownIssuer => 0, diff --git a/src/subject_name/verify.rs b/src/subject_name/verify.rs index f395e765..50d81159 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, }; #[cfg(feature = "alloc")] use { @@ -86,11 +88,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(()); @@ -107,8 +110,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 { @@ -122,6 +125,7 @@ pub(crate) fn check_name_constraints( name, permitted_subtrees, excluded_subtrees, + budget, ) }, )?; @@ -141,11 +145,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; @@ -157,6 +163,7 @@ fn check_presented_id_conforms_to_constraints( name, Subtrees::ExcludedSubtrees, excluded_subtrees, + budget, ) } @@ -170,6 +177,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), @@ -182,6 +190,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." diff --git a/src/verify_cert.rs b/src/verify_cert.rs index 0c228b33..5e7601c2 100644 --- a/src/verify_cert.rs +++ b/src/verify_cert.rs @@ -84,7 +84,12 @@ 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)?; @@ -133,7 +138,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 { @@ -191,6 +196,7 @@ fn check_signed_chain( pub struct Budget { signatures: usize, build_chain_calls: usize, + name_constraint_comparisons: usize, } impl Budget { @@ -211,6 +217,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 { @@ -225,6 +240,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, } } } @@ -551,6 +570,7 @@ where Ok(()) => return Ok(()), err @ Err(Error::MaximumSignatureChecksExceeded) | err @ Err(Error::MaximumPathBuildCallsExceeded) => return err, + err @ Err(Error::MaximumNameConstraintComparisonsExceeded) => return err, Err(new_error) => error = error.most_specific(new_error), } } From caa516f14cd5ffbf3ad58046e116f3508cabc19d Mon Sep 17 00:00:00 2001 From: Daniel McCarney Date: Tue, 5 Sep 2023 13:45:11 -0400 Subject: [PATCH 19/28] 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 | 169 +++++++++++++++++++++++++++++++++++++++------ 1 file changed, 147 insertions(+), 22 deletions(-) diff --git a/src/verify_cert.rs b/src/verify_cert.rs index 5e7601c2..a7b1a261 100644 --- a/src/verify_cert.rs +++ b/src/verify_cert.rs @@ -81,17 +81,6 @@ 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)?; check_signed_chain( @@ -102,6 +91,13 @@ fn build_chain_inner( budget, )?; + check_signed_chain_name_constraints( + cert, + trust_anchor, + subject_common_name_contents, + budget, + )?; + Ok(()) }, ); @@ -137,10 +133,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, @@ -193,6 +185,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 struct Budget { signatures: usize, build_chain_calls: usize, @@ -569,8 +592,8 @@ where match f(v) { Ok(()) => return Ok(()), err @ Err(Error::MaximumSignatureChecksExceeded) - | err @ Err(Error::MaximumPathBuildCallsExceeded) => return err, - err @ Err(Error::MaximumNameConstraintComparisonsExceeded) => return err, + | err @ Err(Error::MaximumPathBuildCallsExceeded) + | err @ Err(Error::MaximumNameConstraintComparisonsExceeded) => return err, Err(new_error) => error = error.most_specific(new_error), } } @@ -598,13 +621,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; @@ -637,13 +660,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; @@ -669,6 +692,104 @@ mod tests { assert_eq!(build_linear_chain(7), Err(Error::MaximumPathDepthExceeded)); } + #[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( + &ChainOptions { + eku: KeyUsage::server_auth(), + supported_sig_algs: &[&ECDSA_P256_SHA256], + trust_anchors: anchors, + intermediate_certs: &intermediates_der, + crls: &[], + }, + 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( + &ChainOptions { + eku: KeyUsage::server_auth(), + supported_sig_algs: &[&ECDSA_P256_SHA256], + trust_anchors: anchors, + intermediate_certs: &intermediates_der, + crls: &[], + }, + cert.inner(), + time, + 0, + &mut failing_budget, + ); + + assert_eq!(result, Err(Error::MaximumNameConstraintComparisonsExceeded)); + } + #[cfg(feature = "alloc")] fn verify_chain( trust_anchor_der: Vec, @@ -700,7 +821,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 @@ -712,6 +836,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 0e100e2343ad44ae74b4c744eaefbc31ee6c2fe4 Mon Sep 17 00:00:00 2001 From: Eliza Weisman Date: Wed, 6 Sep 2023 12:57:27 -0700 Subject: [PATCH 20/28] Add tests for PrintableString, empty SEQUENCE CNs This commit adds a pair of tests reproducing issue rustls/webpki#167, where the `EndEntityCert::dns_names()` method returns an error incorrectly on some certificate DER encodings. In particular, `dns_names` fails if the CN is a `PrintableString`, or if it's an empty `SEQUENCE`, rather than a `SEQUENCE` containing an empty `SET`. The test for the `PrintableString` common name uses an end-entity certificate generated using `rcgen`, while the test for empty `SEQUENCE` CN required a hand-crafted DER using `ascii2der`. The text file that generated the `ascii2der` cert is also included. --- src/end_entity.rs | 56 ++++++++++ src/lib.rs | 4 + src/test_utils.rs | 36 ++++++ src/verify_cert.rs | 35 +----- tests/misc/empty_sequence_common_name.der | Bin 0 -> 434 bytes tests/misc/empty_sequence_common_name.der.txt | 104 ++++++++++++++++++ 6 files changed, 203 insertions(+), 32 deletions(-) create mode 100644 src/test_utils.rs create mode 100644 tests/misc/empty_sequence_common_name.der create mode 100644 tests/misc/empty_sequence_common_name.der.txt diff --git a/src/end_entity.rs b/src/end_entity.rs index a8619390..eeba34db 100644 --- a/src/end_entity.rs +++ b/src/end_entity.rs @@ -257,3 +257,59 @@ impl<'a> EndEntityCert<'a> { subject_name::list_cert_dns_names(self) } } + +#[cfg(feature = "alloc")] +#[cfg(test)] +mod tests { + use super::*; + use crate::test_utils; + + // This test reproduces https://github.com/rustls/webpki/issues/167 --- an + // end-entity cert where the common name is a `PrintableString` rather than + // a `UTF8String` cannot iterate over its subject alternative names. + #[test] + fn printable_string_common_name() { + const DNS_NAME: &str = "test.example.com"; + + let issuer = test_utils::make_issuer("Test", None); + + let ee_cert_der = { + let mut params = rcgen::CertificateParams::new(vec![DNS_NAME.to_string()]); + // construct a certificate that uses `PrintableString` as the + // common name value, rather than `UTF8String`. + params.distinguished_name.push( + rcgen::DnType::CommonName, + rcgen::DnValue::PrintableString("example.com".to_string()), + ); + params.is_ca = rcgen::IsCa::ExplicitNoCa; + params.alg = test_utils::RCGEN_SIGNATURE_ALG; + let cert = rcgen::Certificate::from_params(params) + .expect("failed to make ee cert (this is a test bug)"); + cert.serialize_der_with_signer(&issuer) + .expect("failed to serialize signed ee cert (this is a test bug)") + }; + + expect_dns_name(&ee_cert_der, DNS_NAME); + } + + // This test reproduces https://github.com/rustls/webpki/issues/167 --- an + // end-entity cert where the common name is an empty SEQUENCE. + #[test] + fn empty_sequence_common_name() { + // handcrafted cert DER produced using `ascii2der`, since `rcgen` is + // unwilling to generate this particular weird cert. + let ee_cert_der = include_bytes!("../tests/misc/empty_sequence_common_name.der").as_slice(); + expect_dns_name(ee_cert_der, "example.com"); + } + + fn expect_dns_name(der: &[u8], name: &str) { + let cert = + EndEntityCert::try_from(der).expect("should parse end entity certificate correctly"); + + let mut names = cert + .dns_names() + .expect("should get all DNS names correctly for end entity cert"); + assert_eq!(names.next().map(<&str>::from), Some(name)); + assert_eq!(names.next().map(<&str>::from), None); + } +} diff --git a/src/lib.rs b/src/lib.rs index 617b43bf..b6229db9 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -60,6 +60,10 @@ mod x509; #[allow(deprecated)] pub use trust_anchor::{TlsClientTrustAnchors, TlsServerTrustAnchors}; + +#[cfg(test)] +pub(crate) mod test_utils; + pub use { cert::{Cert, EndEntityOrCa}, crl::{BorrowedCertRevocationList, BorrowedRevokedCert, CertRevocationList, RevocationReason}, diff --git a/src/test_utils.rs b/src/test_utils.rs new file mode 100644 index 00000000..65dcfd7a --- /dev/null +++ b/src/test_utils.rs @@ -0,0 +1,36 @@ +/// Signature algorithm used by certificates generated using `make_issuer` and +/// `make_end_entity`. This is exported as a constant so that tests can use the +/// same algorithm when generating certificates using `rcgen`. +#[cfg(feature = "alloc")] +pub(crate) static RCGEN_SIGNATURE_ALG: &rcgen::SignatureAlgorithm = &rcgen::PKCS_ECDSA_P256_SHA256; + +#[cfg(feature = "alloc")] +pub(crate) 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 + .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_SIGNATURE_ALG; + ca_params.name_constraints = name_constraints; + rcgen::Certificate::from_params(ca_params).unwrap() +} + +#[cfg(feature = "alloc")] +pub(crate) 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_SIGNATURE_ALG; + rcgen::Certificate::from_params(ee_params) + .unwrap() + .serialize_der_with_signer(issuer) + .unwrap() +} diff --git a/src/verify_cert.rs b/src/verify_cert.rs index a7b1a261..9cfe510a 100644 --- a/src/verify_cert.rs +++ b/src/verify_cert.rs @@ -604,6 +604,9 @@ where mod tests { use super::*; + #[cfg(feature = "alloc")] + use crate::test_utils::{make_end_entity, make_issuer}; + #[test] fn eku_key_purpose_id() { assert!(ExtendedKeyUsage::RequiredIfPresent(EKU_SERVER_AUTH) @@ -819,36 +822,4 @@ mod tests { time, ) } - - #[cfg(feature = "alloc")] - 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 - .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; - ca_params.name_constraints = name_constraints; - 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() - } } diff --git a/tests/misc/empty_sequence_common_name.der b/tests/misc/empty_sequence_common_name.der new file mode 100644 index 0000000000000000000000000000000000000000..30e4f77b50bb7ef393be01bd19a4b894a6230a64 GIT binary patch literal 434 zcmXqLVq9m?#2CDQnTe5!Nu)^R(CKhjizU~rK6P&jbZC;fd{o1Li;Y98&EuRc3p0}e zzag&yHyd*(3%4+fUw&Syft)z6k+FfLftjJXk)eS>lsK<3h-(bx8Za0{8VIwogAHI} zgqp<6$j*=y8|#<$KcDCD$`22BC5l{ge|i7VJ7ve-u(c6ye^;D;@}_s~kFdW_ zS4^m!9=zw$N(+u@1=2ObPv#wZF#X3n6UD`ivke+&81MnzBg@ao_@4#nLe>QavLL=H zih`MPvX9sCx_iSA0;-JM97)E z@7UyHAP-Wk%pzeR)*y0n@vV=t%<*}vH6Fc^j7cxe*s%7Mfh0(QAjmX+1KuX?)QZI1 zf}B*n60z};Zqr0fokwfiSu`gA ol`Q+1XL-VU0ps^?tDak}y0C6ZZe#{SNk;oW*YqX-U-Zug012#+t^fc4 literal 0 HcmV?d00001 diff --git a/tests/misc/empty_sequence_common_name.der.txt b/tests/misc/empty_sequence_common_name.der.txt new file mode 100644 index 00000000..dbe59d44 --- /dev/null +++ b/tests/misc/empty_sequence_common_name.der.txt @@ -0,0 +1,104 @@ +SEQUENCE { + SEQUENCE { + [0] { + INTEGER { 2 } + } + INTEGER { `7214c2cb574538a4d63af28bb25140821cd3c528` } + SEQUENCE { + # ecdsa-with-SHA256 + OBJECT_IDENTIFIER { 1.2.840.10045.4.3.2 } + } + SEQUENCE { + SET { + SEQUENCE { + # organizationUnitName + OBJECT_IDENTIFIER { 2.5.4.11 } + PrintableString { "None" } + } + } + } + SEQUENCE { + UTCTime { "230906172100Z" } + UTCTime { "330903172100Z" } + } + SEQUENCE {} + SEQUENCE { + SEQUENCE { + # ecPublicKey + OBJECT_IDENTIFIER { 1.2.840.10045.2.1 } + # secp256r1 + OBJECT_IDENTIFIER { 1.2.840.10045.3.1.7 } + } + BIT_STRING { `00` `04681e0d5d4e66ff6f0cc3a9f0e1ba6114d647e9dfc2ee23418d56ad58edfb78cfe4ec8dadf856fde5a890799753bcd2a9380896701b7c13e49ec2e097f8ee3421` } + } + [3] { + SEQUENCE { + SEQUENCE { + # keyUsage + OBJECT_IDENTIFIER { 2.5.29.15 } + BOOLEAN { TRUE } + OCTET_STRING { + BIT_STRING { b`101` } + } + } + SEQUENCE { + # extKeyUsage + OBJECT_IDENTIFIER { 2.5.29.37 } + OCTET_STRING { + SEQUENCE { + # serverAuth + OBJECT_IDENTIFIER { 1.3.6.1.5.5.7.3.1 } + # clientAuth + OBJECT_IDENTIFIER { 1.3.6.1.5.5.7.3.2 } + } + } + } + SEQUENCE { + # basicConstraints + OBJECT_IDENTIFIER { 2.5.29.19 } + BOOLEAN { TRUE } + OCTET_STRING { + SEQUENCE {} + } + } + SEQUENCE { + # subjectKeyIdentifier + OBJECT_IDENTIFIER { 2.5.29.14 } + OCTET_STRING { + OCTET_STRING { `8f8c61be7ce4c34689e2618034581e34ef88b24c` } + } + } + SEQUENCE { + # authorityKeyIdentifier + OBJECT_IDENTIFIER { 2.5.29.35 } + OCTET_STRING { + SEQUENCE { + [0 PRIMITIVE] { `c9a3daf11d035f6eab28e2ea195c677568b0adea` } + } + } + } + SEQUENCE { + # subjectAltName + OBJECT_IDENTIFIER { 2.5.29.17 } + BOOLEAN { TRUE } + OCTET_STRING { + SEQUENCE { + [2 PRIMITIVE] { "example.com" } + } + } + } + } + } + } + SEQUENCE { + # ecdsa-with-SHA256 + OBJECT_IDENTIFIER { 1.2.840.10045.4.3.2 } + } + BIT_STRING { + `00` + SEQUENCE { + INTEGER { `00f1891fc58e16c37752bf64d58999f703c8d458b1d22d8291292ce2ad87042990` } + INTEGER { `00a6f16e39c83ba001f7f6aae73aaad0aea46d596800746887fe4567a4ffe88f9b` } + } + } +} From 9fe852ef4ac2312a064c86cffb39e355c5dc7138 Mon Sep 17 00:00:00 2001 From: Eliza Weisman Date: Thu, 7 Sep 2023 09:52:33 -0700 Subject: [PATCH 21/28] Remove tests for common name handling As suggested by @ctz in this comment: https://github.com/rustls/webpki/issues/167#issuecomment-1709684296. --- tests/generate.py | 16 ---------------- tests/tls_server_certs.rs | 22 ---------------------- 2 files changed, 38 deletions(-) diff --git a/tests/generate.py b/tests/generate.py index 3cf55546..0ec9e30b 100755 --- a/tests/generate.py +++ b/tests/generate.py @@ -312,13 +312,6 @@ def tls_server_certs(force: bool) -> None: permitted_subtrees=[x509.DNSName(".example.com")], ) - generate_tls_server_cert_test( - output, - "disallow_subject_common_name", - expected_error="NameConstraintViolation", - subject_common_name="disallowed.example.com", - excluded_subtrees=[x509.DNSName("disallowed.example.com")], - ) generate_tls_server_cert_test( output, "disallow_dns_san", @@ -353,15 +346,6 @@ def tls_server_certs(force: bool) -> None: x509.DNSName("allowed-cn.example.com"), ], ) - generate_tls_server_cert_test( - output, - "allow_dns_san_and_disallow_subject_common_name", - expected_error="NameConstraintViolation", - sans=[x509.DNSName("allowed-san.example.com")], - subject_common_name="disallowed-cn.example.com", - permitted_subtrees=[x509.DNSName("allowed-san.example.com")], - excluded_subtrees=[x509.DNSName("disallowed-cn.example.com")], - ) generate_tls_server_cert_test( output, "disallow_dns_san_and_allow_subject_common_name", diff --git a/tests/tls_server_certs.rs b/tests/tls_server_certs.rs index e0d8f2ef..a71f0e12 100644 --- a/tests/tls_server_certs.rs +++ b/tests/tls_server_certs.rs @@ -89,16 +89,6 @@ fn additional_dns_labels() { ); } -#[test] -fn disallow_subject_common_name() { - let ee = include_bytes!("tls_server_certs/disallow_subject_common_name.ee.der"); - let ca = include_bytes!("tls_server_certs/disallow_subject_common_name.ca.der"); - assert_eq!( - check_cert(ee, ca, &[], &[]), - Err(webpki::Error::NameConstraintViolation) - ); -} - #[test] fn disallow_dns_san() { let ee = include_bytes!("tls_server_certs/disallow_dns_san.ee.der"); @@ -138,18 +128,6 @@ fn allow_dns_san_and_subject_common_name() { ); } -#[test] -fn allow_dns_san_and_disallow_subject_common_name() { - let ee = - include_bytes!("tls_server_certs/allow_dns_san_and_disallow_subject_common_name.ee.der"); - let ca = - include_bytes!("tls_server_certs/allow_dns_san_and_disallow_subject_common_name.ca.der"); - assert_eq!( - check_cert(ee, ca, &[], &[]), - Err(webpki::Error::NameConstraintViolation) - ); -} - #[test] fn disallow_dns_san_and_allow_subject_common_name() { let ee = From 65eb6a0f8a76959848fcd5ec7306afc1c6c300e1 Mon Sep 17 00:00:00 2001 From: Eliza Weisman Date: Thu, 7 Sep 2023 09:53:27 -0700 Subject: [PATCH 22/28] Remove common name parsing from `NameIterator` This commit removes parsing of the subject common name field from `NameIterator`, since `rustls-webpki` does not actually verify subject common names except when enforcing name constraints. This fixes issues with common names in formats that `rustls-webpki` doesn't currently support, by removing this code entirely. Fixes rustls-webpki/webpki#167 --- src/der.rs | 2 -- src/subject_name/mod.rs | 4 +--- src/subject_name/verify.rs | 47 +------------------------------------- src/verify_cert.rs | 24 ++----------------- 4 files changed, 4 insertions(+), 73 deletions(-) diff --git a/src/der.rs b/src/der.rs index 213d7b76..d3a53a70 100644 --- a/src/der.rs +++ b/src/der.rs @@ -26,9 +26,7 @@ pub(crate) enum Tag { OctetString = 0x04, OID = 0x06, Enum = 0x0A, - UTF8String = 0x0C, Sequence = CONSTRUCTED | 0x10, // 0x30 - Set = CONSTRUCTED | 0x11, // 0x31 UTCTime = 0x17, GeneralizedTime = 0x18, diff --git a/src/subject_name/mod.rs b/src/subject_name/mod.rs index 33d64c66..5d2058be 100644 --- a/src/subject_name/mod.rs +++ b/src/subject_name/mod.rs @@ -33,6 +33,4 @@ pub use ip_address::IpAddr; mod verify; #[cfg(feature = "alloc")] pub(super) use verify::list_cert_dns_names; -pub(super) use verify::{ - check_name_constraints, verify_cert_subject_name, SubjectCommonNameContents, -}; +pub(super) use verify::{check_name_constraints, verify_cert_subject_name}; diff --git a/src/subject_name/verify.rs b/src/subject_name/verify.rs index 50d81159..9a882fd2 100644 --- a/src/subject_name/verify.rs +++ b/src/subject_name/verify.rs @@ -38,7 +38,6 @@ pub(crate) fn verify_cert_dns_name( iterate_names( Some(cert.subject), cert.subject_alt_name, - SubjectCommonNameContents::Ignore, Err(Error::CertNotValidForName), &mut |name| { if let GeneralName::DnsName(presented_id) = name { @@ -72,7 +71,6 @@ pub(crate) fn verify_cert_subject_name( // only against Subject Alternative Names. None, cert.inner().subject_alt_name, - SubjectCommonNameContents::Ignore, Err(Error::CertNotValidForName), &mut |name| { if let GeneralName::IpAddress(presented_id) = name { @@ -90,7 +88,6 @@ pub(crate) fn verify_cert_subject_name( pub(crate) fn check_name_constraints( input: Option<&mut untrusted::Reader>, subordinate_certs: &Cert, - subject_common_name_contents: SubjectCommonNameContents, budget: &mut Budget, ) -> Result<(), Error> { let input = match input { @@ -118,7 +115,6 @@ pub(crate) fn check_name_constraints( iterate_names( Some(child.subject), child.subject_alt_name, - subject_common_name_contents, Ok(()), &mut |name| { check_presented_id_conforms_to_constraints( @@ -310,16 +306,9 @@ enum NameIteration { Stop(Result<(), Error>), } -#[derive(Clone, Copy)] -pub(crate) enum SubjectCommonNameContents { - DnsName, - Ignore, -} - fn iterate_names<'names>( subject: Option>, subject_alt_name: Option>, - subject_common_name_contents: SubjectCommonNameContents, result_if_never_stopped_early: Result<(), Error>, f: &mut impl FnMut(GeneralName<'names>) -> NameIteration, ) -> Result<(), Error> { @@ -349,20 +338,7 @@ fn iterate_names<'names>( }; } - if let (SubjectCommonNameContents::DnsName, Some(subject)) = - (subject_common_name_contents, subject) - { - match common_name(subject) { - Ok(Some(cn)) => match f(GeneralName::DnsName(cn)) { - NameIteration::Stop(result) => result, - NameIteration::KeepGoing => result_if_never_stopped_early, - }, - Ok(None) => result_if_never_stopped_early, - Err(err) => Err(err), - } - } else { - result_if_never_stopped_early - } + result_if_never_stopped_early } #[cfg(feature = "alloc")] @@ -375,7 +351,6 @@ pub(crate) fn list_cert_dns_names<'names>( iterate_names( Some(cert.subject), cert.subject_alt_name, - SubjectCommonNameContents::DnsName, Ok(()), &mut |name| { if let GeneralName::DnsName(presented_id) = name { @@ -448,23 +423,3 @@ impl<'a> GeneralName<'a> { }) } } - -static COMMON_NAME: untrusted::Input = untrusted::Input::from(&[85, 4, 3]); - -fn common_name(input: untrusted::Input) -> Result, Error> { - let inner = &mut untrusted::Reader::new(input); - der::nested(inner, der::Tag::Set, Error::BadDer, |tagged| { - der::nested(tagged, der::Tag::Sequence, Error::BadDer, |tagged| { - while !tagged.at_end() { - let name_oid = der::expect_tag_and_get_value(tagged, der::Tag::OID)?; - if name_oid == COMMON_NAME { - return der::expect_tag_and_get_value(tagged, der::Tag::UTF8String).map(Some); - } else { - // discard unused name value - der::read_tag_and_get_value(tagged)?; - } - } - Ok(None) - }) - }) -} diff --git a/src/verify_cert.rs b/src/verify_cert.rs index 9cfe510a..cd3d2f32 100644 --- a/src/verify_cert.rs +++ b/src/verify_cert.rs @@ -58,20 +58,6 @@ fn build_chain_inner( } } - // for the purpose of name constraints checking, only end-entity server certificates - // could plausibly have a DNS name as a subject commonName that could contribute to - // path validity - let subject_common_name_contents = if opts - .eku - .inner - .key_purpose_id_equals(EKU_SERVER_AUTH.oid_value) - && used_as_ca == UsedAsCa::No - { - subject_name::SubjectCommonNameContents::DnsName - } else { - subject_name::SubjectCommonNameContents::Ignore - }; - let result = loop_while_non_fatal_error( Error::UnknownIssuer, opts.trust_anchors, @@ -91,12 +77,7 @@ fn build_chain_inner( budget, )?; - check_signed_chain_name_constraints( - cert, - trust_anchor, - subject_common_name_contents, - budget, - )?; + check_signed_chain_name_constraints(cert, trust_anchor, budget)?; Ok(()) }, @@ -188,7 +169,6 @@ fn check_signed_chain( 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; @@ -199,7 +179,7 @@ fn check_signed_chain_name_constraints( loop { untrusted::read_all_optional(name_constraints, Error::BadDer, |value| { - subject_name::check_name_constraints(value, cert, subject_common_name_contents, budget) + subject_name::check_name_constraints(value, cert, budget) })?; match &cert.ee_or_ca { From 6d8621142ff155da9bc61dc377540e4c2a398935 Mon Sep 17 00:00:00 2001 From: Daniel McCarney Date: Thu, 7 Sep 2023 12:49:16 -0400 Subject: [PATCH 23/28] 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 | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/src/verify_cert.rs b/src/verify_cert.rs index cd3d2f32..70119310 100644 --- a/src/verify_cert.rs +++ b/src/verify_cert.rs @@ -620,7 +620,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] @@ -655,7 +655,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] @@ -775,14 +775,14 @@ 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 intermediates_der = intermediates_der From 7ba21bf97952a23586519b0882aa6a95bc136486 Mon Sep 17 00:00:00 2001 From: Daniel McCarney Date: Thu, 7 Sep 2023 12:55:17 -0400 Subject: [PATCH 24/28] 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 | 69 +++++++++++++++++++--------------------------- 1 file changed, 29 insertions(+), 40 deletions(-) diff --git a/src/verify_cert.rs b/src/verify_cert.rs index 70119310..5e0e94c3 100644 --- a/src/verify_cert.rs +++ b/src/verify_cert.rs @@ -620,7 +620,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] @@ -655,7 +661,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] @@ -678,9 +689,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( @@ -710,18 +718,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. @@ -733,22 +733,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( - &ChainOptions { - eku: KeyUsage::server_auth(), - supported_sig_algs: &[&ECDSA_P256_SHA256], - trust_anchors: anchors, - intermediate_certs: &intermediates_der, - crls: &[], - }, - cert.inner(), - time, - 0, - &mut passing_budget, + verify_chain( + &ca_cert_der, + &intermediates_der, + &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() @@ -756,18 +749,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( - &ChainOptions { - eku: KeyUsage::server_auth(), - supported_sig_algs: &[&ECDSA_P256_SHA256], - trust_anchors: anchors, - intermediate_certs: &intermediates_der, - crls: &[], - }, - cert.inner(), - time, - 0, - &mut failing_budget, + let result = verify_chain( + &ca_cert_der, + &intermediates_der, + &ee_cert, + Some(failing_budget), ); assert_eq!(result, Err(Error::MaximumNameConstraintComparisonsExceeded)); @@ -778,6 +764,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}; @@ -790,7 +777,7 @@ mod tests { .map(|x| x.as_ref()) .collect::>(); - build_chain( + build_chain_inner( &ChainOptions { eku: KeyUsage::server_auth(), supported_sig_algs: &[&ECDSA_P256_SHA256], @@ -800,6 +787,8 @@ mod tests { }, cert.inner(), time, + 0, + &mut budget.unwrap_or_default(), ) } } From b86d82ef05816d02070730f8bfcd7757b2dc4245 Mon Sep 17 00:00:00 2001 From: Daniel McCarney Date: Thu, 7 Sep 2023 12:57:29 -0400 Subject: [PATCH 25/28] 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 | 12 ++++++++++++ src/verify_cert.rs | 7 ++++--- 2 files changed, 16 insertions(+), 3 deletions(-) diff --git a/src/error.rs b/src/error.rs index 8699be51..a8ad9789 100644 --- a/src/error.rs +++ b/src/error.rs @@ -245,6 +245,18 @@ impl Error { Error::UnknownIssuer => 0, } } + + /// 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 { diff --git a/src/verify_cert.rs b/src/verify_cert.rs index 5e0e94c3..5c4261fa 100644 --- a/src/verify_cert.rs +++ b/src/verify_cert.rs @@ -571,9 +571,10 @@ where for v in values { 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 be ranked by specificity and only returned + // once all other path-building options have been exhausted. Err(new_error) => error = error.most_specific(new_error), } } From 1bb7ce0ed7a91086fd8d0f4aa233eb17960b8f8c Mon Sep 17 00:00:00 2001 From: Daniel McCarney Date: Thu, 7 Sep 2023 13:07:41 -0400 Subject: [PATCH 26/28] 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 | 22 +++++++++++++++++++--- 1 file changed, 19 insertions(+), 3 deletions(-) diff --git a/src/verify_cert.rs b/src/verify_cert.rs index 5c4261fa..d878ae72 100644 --- a/src/verify_cert.rs +++ b/src/verify_cert.rs @@ -85,6 +85,11 @@ fn build_chain_inner( let err = match result { Ok(()) => return Ok(()), + // Fatal errors should halt further path building. + res @ Err(err) if err.is_fatal() => return res, + // Non-fatal errors should be carried forward as the default_error for subsequent + // loop_while_non_fatal_error processing and only returned once all other path-building + // options have been exhausted. Err(err) => err, }; @@ -604,6 +609,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(); @@ -625,7 +631,7 @@ mod tests { &ca_cert_der, &intermediates, &make_end_entity(&issuer), - None, + budget, ) .unwrap_err() } @@ -634,7 +640,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 ); } @@ -643,7 +649,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 4969bf62ff72d0b63cb4fbce0994b03930227452 Mon Sep 17 00:00:00 2001 From: Daniel McCarney Date: Thu, 7 Sep 2023 17:37:36 -0400 Subject: [PATCH 27/28] 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 | 80 ++++++++++++++++++++++++++-------------------- 2 files changed, 58 insertions(+), 34 deletions(-) diff --git a/src/error.rs b/src/error.rs index a8ad9789..a40b562e 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)] @@ -259,6 +260,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 d878ae72..9fcd280e 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::{Cert, EndEntityOrCa}, @@ -29,7 +30,10 @@ 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 Budget::default()) + build_chain_inner(opts, cert, time, 0, &mut Budget::default()).map_err(|e| match e { + ControlFlow::Break(err) => err, + ControlFlow::Continue(err) => err, + }) } fn build_chain_inner( @@ -38,7 +42,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(cert, time, used_as_ca, sub_ca_count, opts.eku.inner)?; @@ -50,7 +54,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 => { @@ -64,7 +68,7 @@ fn build_chain_inner( |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)?; @@ -86,11 +90,11 @@ fn build_chain_inner( let err = 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 be carried forward as the default_error for subsequent // loop_while_non_fatal_error processing and only returned once all other path-building // options have been exhausted. - Err(err) => err, + Err(ControlFlow::Continue(err)) => err, }; loop_while_non_fatal_error(err, opts.intermediate_certs, |cert_der| { @@ -98,7 +102,7 @@ fn build_chain_inner( Cert::from_der(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. @@ -107,7 +111,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 => { @@ -135,7 +139,7 @@ fn check_signed_chain( trust_anchor: &TrustAnchor, crls: &[&dyn CertRevocationList], budget: &mut Budget, -) -> Result<(), Error> { +) -> Result<(), ControlFlow> { let mut spki_value = untrusted::Input::from(trust_anchor.spki); let mut issuer_subject = untrusted::Input::from(trust_anchor.subject); let mut issuer_key_usage = None; // TODO(XXX): Consider whether to track TrustAnchor KU. @@ -175,7 +179,7 @@ fn check_signed_chain_name_constraints( cert_chain: &Cert, trust_anchor: &TrustAnchor, budget: &mut Budget, -) -> Result<(), Error> { +) -> Result<(), ControlFlow> { let mut cert = cert_chain; let mut name_constraints = trust_anchor .name_constraints @@ -567,8 +571,8 @@ impl KeyUsageMode { fn loop_while_non_fatal_error( default_error: 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, { @@ -577,13 +581,13 @@ 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 be ranked by specificity and only returned // once all other path-building options have been exhausted. - Err(new_error) => error = error.most_specific(new_error), + Err(ControlFlow::Continue(new_error)) => error = error.most_specific(new_error), } } - Err(error) + Err(error.into()) } #[cfg(test)] @@ -610,7 +614,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(); @@ -639,16 +643,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, @@ -660,12 +664,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(); @@ -689,18 +693,21 @@ 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] #[cfg(feature = "alloc")] fn path_too_long() { - assert_eq!(build_linear_chain(7), Err(Error::MaximumPathDepthExceeded)); + assert!(matches!( + build_linear_chain(7), + Err(ControlFlow::Continue(Error::MaximumPathDepthExceeded)) + )); } #[test] @@ -750,13 +757,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. @@ -773,7 +780,12 @@ mod tests { Some(failing_budget), ); - assert_eq!(result, Err(Error::MaximumNameConstraintComparisonsExceeded)); + assert!(matches!( + result, + Err(ControlFlow::Break( + Error::MaximumNameConstraintComparisonsExceeded + )) + )); } #[cfg(feature = "alloc")] @@ -782,7 +794,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 702d57f444e3f7d743277524e832a2363290ec4d Mon Sep 17 00:00:00 2001 From: Daniel McCarney Date: Fri, 8 Sep 2023 10:58:00 -0400 Subject: [PATCH 28/28] Cargo: bump version 0.101.4 -> 0.101.5 --- Cargo.toml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Cargo.toml b/Cargo.toml index 2bf8772b..0bc5314f 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -21,7 +21,7 @@ license = "ISC" name = "rustls-webpki" readme = "README.md" repository = "https://github.com/rustls/webpki" -version = "0.101.4" +version = "0.101.5" include = [ "Cargo.toml",