From 910b9ca142679c5d6c3f2f45cb3e202c904eeab5 Mon Sep 17 00:00:00 2001 From: Daniel McCarney Date: Thu, 7 Sep 2023 12:49:16 -0400 Subject: [PATCH 1/5] verify_cert: take references in verify_chain helper This commit adjusts the arguments to the `verify_chain` test helper to take references instead of moving the arguments. This makes it easier to use the same inputs for multiple `verify_chain` invocations. --- src/verify_cert.rs | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-) diff --git a/src/verify_cert.rs b/src/verify_cert.rs index af15e635..20781dc8 100644 --- a/src/verify_cert.rs +++ b/src/verify_cert.rs @@ -898,7 +898,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] @@ -933,7 +933,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] @@ -1053,16 +1053,16 @@ mod tests { #[cfg(feature = "alloc")] fn verify_chain( - trust_anchor: CertificateDer<'_>, - intermediates_der: Vec>, - ee_cert: CertificateDer<'_>, + trust_anchor: &CertificateDer<'_>, + intermediates_der: &[Vec], + ee_cert: &CertificateDer<'_>, ) -> Result<(), Error> { use crate::{extract_trust_anchor, ECDSA_P256_SHA256}; use crate::{EndEntityCert, Time}; - let anchors = &[extract_trust_anchor(&trust_anchor).unwrap()]; + let anchors = &[extract_trust_anchor(trust_anchor).unwrap()]; let time = Time::from_seconds_since_unix_epoch(0x1fed_f00d); - let cert = EndEntityCert::try_from(&ee_cert).unwrap(); + let cert = EndEntityCert::try_from(ee_cert).unwrap(); let intermediates_der = intermediates_der .iter() .map(|x| CertificateDer::from(x.as_ref())) From 026f9e75ce9bad8dfea3116bd904c32cce8608f2 Mon Sep 17 00:00:00 2001 From: Daniel McCarney Date: Thu, 7 Sep 2023 12:55:17 -0400 Subject: [PATCH 2/5] 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 20781dc8..a534187a 100644 --- a/src/verify_cert.rs +++ b/src/verify_cert.rs @@ -898,7 +898,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] @@ -933,7 +939,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] @@ -956,9 +967,6 @@ mod tests { #[test] #[cfg(feature = "alloc")] fn name_constraint_budget() { - use crate::{extract_trust_anchor, 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( @@ -988,18 +996,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 = &[extract_trust_anchor(&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| CertificateDer::from(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. @@ -1011,22 +1011,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, - revocation: None, - }, - 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() @@ -1034,18 +1027,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, - revocation: None, - }, - 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)); @@ -1056,6 +1042,7 @@ mod tests { trust_anchor: &CertificateDer<'_>, intermediates_der: &[Vec], ee_cert: &CertificateDer<'_>, + budget: Option, ) -> Result<(), Error> { use crate::{extract_trust_anchor, ECDSA_P256_SHA256}; use crate::{EndEntityCert, Time}; @@ -1068,7 +1055,7 @@ mod tests { .map(|x| CertificateDer::from(x.as_ref())) .collect::>(); - build_chain( + build_chain_inner( &ChainOptions { eku: KeyUsage::server_auth(), supported_sig_algs: &[ECDSA_P256_SHA256], @@ -1078,6 +1065,8 @@ mod tests { }, cert.inner(), time, + 0, + &mut budget.unwrap_or_default(), ) } From bcdd68053e1e994a571473862bc3b5c44ce0002b Mon Sep 17 00:00:00 2001 From: Daniel McCarney Date: Thu, 7 Sep 2023 12:57:29 -0400 Subject: [PATCH 3/5] 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 5a8efe1c..3593e7f2 100644 --- a/src/error.rs +++ b/src/error.rs @@ -257,6 +257,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 a534187a..d9b34757 100644 --- a/src/verify_cert.rs +++ b/src/verify_cert.rs @@ -746,9 +746,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 b5af2cebd621ceeb3aeca44edd6181ab97b3b334 Mon Sep 17 00:00:00 2001 From: Daniel McCarney Date: Thu, 7 Sep 2023 13:07:41 -0400 Subject: [PATCH 4/5] 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 d9b34757..b27465d8 100644 --- a/src/verify_cert.rs +++ b/src/verify_cert.rs @@ -205,6 +205,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, }; @@ -882,6 +887,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 = CertificateDer::from(ca_cert.serialize_der().unwrap()); @@ -903,7 +909,7 @@ mod tests { &ca_cert_der, &intermediates, &make_end_entity(&issuer), - None, + budget, ) .unwrap_err() } @@ -912,7 +918,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 ); } @@ -921,7 +927,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 c69804ec761866633333150f0bb5f2c4a244d07a Mon Sep 17 00:00:00 2001 From: Daniel McCarney Date: Thu, 7 Sep 2023 17:37:36 -0400 Subject: [PATCH 5/5] 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 3593e7f2..a0b037df 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)] @@ -271,6 +272,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 b27465d8..42466279 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 pki_types::{CertificateDer, SignatureVerificationAlgorithm, TrustAnchor}; @@ -130,7 +131,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( @@ -139,7 +143,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)?; @@ -151,7 +155,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 => { @@ -179,7 +183,7 @@ fn build_chain_inner( |trust_anchor: &TrustAnchor| { let trust_anchor_subject = untrusted::Input::from(trust_anchor.subject.as_ref()); if cert.issuer != trust_anchor_subject { - return Err(Error::UnknownIssuer); + return Err(Error::UnknownIssuer.into()); } // TODO: check_distrust(trust_anchor_subject, trust_anchor_spki)?; @@ -206,11 +210,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| { @@ -218,14 +222,14 @@ 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. let mut prev = cert; loop { if potential_issuer.spki == prev.spki && potential_issuer.subject == prev.subject { - return Err(Error::UnknownIssuer); + return Err(Error::UnknownIssuer.into()); } match &prev.ee_or_ca { EndEntityOrCa::EndEntity => { @@ -253,7 +257,7 @@ fn check_signed_chain( trust_anchor: &TrustAnchor, revocation: Option, budget: &mut Budget, -) -> Result<(), Error> { +) -> Result<(), ControlFlow> { let mut spki_value = untrusted::Input::from(trust_anchor.subject_public_key_info.as_ref()); let mut issuer_subject = untrusted::Input::from(trust_anchor.subject.as_ref()); let mut issuer_key_usage = None; // TODO(XXX): Consider whether to track TrustAnchor KU. @@ -294,7 +298,7 @@ fn check_signed_chain_name_constraints( trust_anchor: &TrustAnchor, subject_common_name_contents: subject_name::SubjectCommonNameContents, budget: &mut Budget, -) -> Result<(), Error> { +) -> Result<(), ControlFlow> { let mut cert = cert_chain; let mut name_constraints = trust_anchor .name_constraints @@ -742,8 +746,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, { @@ -752,13 +756,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)] @@ -888,7 +892,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 = CertificateDer::from(ca_cert.serialize_der().unwrap()); @@ -917,16 +921,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, @@ -938,12 +942,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 = CertificateDer::from(ca_cert.serialize_der().unwrap()); @@ -967,18 +971,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] @@ -1028,13 +1035,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. @@ -1051,7 +1058,12 @@ mod tests { Some(failing_budget), ); - assert_eq!(result, Err(Error::MaximumNameConstraintComparisonsExceeded)); + assert!(matches!( + result, + Err(ControlFlow::Break( + Error::MaximumNameConstraintComparisonsExceeded + )) + )); } #[cfg(feature = "alloc")] @@ -1060,7 +1072,7 @@ mod tests { intermediates_der: &[Vec], ee_cert: &CertificateDer<'_>, budget: Option, - ) -> Result<(), Error> { + ) -> Result<(), ControlFlow> { use crate::{extract_trust_anchor, ECDSA_P256_SHA256}; use crate::{EndEntityCert, Time};