verify_cert: correct handling of fatal errors.

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)]
@@ -257,6 +258,29 @@ 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 From<Error> for ControlFlow<Error, Error> {
+    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 {

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<Error, Error>> {
     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)?;
@@ -205,22 +209,27 @@ fn build_chain_inner(
 
     let err = match result {
         Ok(()) => return Ok(()),
-        Err(err) => err,
+        // Fatal errors should halt further path building.
+        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(ControlFlow::Continue(err)) => err,
     };
 
     loop_while_non_fatal_error(err, opts.intermediate_certs, |cert_der| {
         let potential_issuer =
             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 => {
@@ -248,7 +257,7 @@ fn check_signed_chain(
     trust_anchor: &TrustAnchor,
     revocation: Option<RevocationOptions>,
     budget: &mut Budget,
-) -> Result<(), Error> {
+) -> Result<(), ControlFlow<Error, Error>> {
     let mut spki_value = untrusted::Input::from(trust_anchor.subject_public_key_info.as_ref());
     let mut issuer_subject = untrusted::Input::from(trust_anchor.subject.as_ref());
     let mut issuer_key_usage = None; // TODO(XXX): Consider whether to track TrustAnchor KU.
@@ -289,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<Error, Error>> {
     let mut cert = cert_chain;
     let mut name_constraints = trust_anchor
         .name_constraints
@@ -737,22 +746,23 @@ impl KeyUsageMode {
 fn loop_while_non_fatal_error<V>(
     default_error: Error,
     values: V,
-    mut f: impl FnMut(V::Item) -> Result<(), Error>,
-) -> Result<(), Error>
+    mut f: impl FnMut(V::Item) -> Result<(), ControlFlow<Error, Error>>,
+) -> Result<(), ControlFlow<Error, Error>>
 where
     V: IntoIterator,
 {
     let mut error = default_error;
     for v in values {
         match f(v) {
             Ok(()) => return Ok(()),
-            err @ Err(Error::MaximumSignatureChecksExceeded)
-            | err @ Err(Error::MaximumPathBuildCallsExceeded)
-            | err @ Err(Error::MaximumNameConstraintComparisonsExceeded) => return err,
-            Err(new_error) => error = error.most_specific(new_error),
+            // Fatal errors should halt further looping.
+            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(ControlFlow::Continue(new_error)) => error = error.most_specific(new_error),
         }
     }
-    Err(error)
+    Err(error.into())
 }
 
 #[cfg(test)]
@@ -881,7 +891,8 @@ mod tests {
     fn build_degenerate_chain(
         intermediate_count: usize,
         trust_anchor_is_actual_issuer: TrustAnchorIsActualIssuer,
-    ) -> Error {
+        budget: Option<Budget>,
+    ) -> ControlFlow<Error, Error> {
         let ca_cert = make_issuer("Bogus Subject", None);
         let ca_cert_der = CertificateDer::from(ca_cert.serialize_der().unwrap());
 
@@ -898,29 +909,45 @@ 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),
+            budget,
+        )
+        .unwrap_err()
     }
 
     #[test]
     #[cfg(feature = "alloc")]
     fn test_too_many_signatures() {
-        assert_eq!(
-            build_degenerate_chain(5, TrustAnchorIsActualIssuer::Yes),
-            Error::MaximumSignatureChecksExceeded
-        );
+        assert!(matches!(
+            build_degenerate_chain(5, TrustAnchorIsActualIssuer::Yes, None),
+            ControlFlow::Break(Error::MaximumSignatureChecksExceeded)
+        ));
     }
 
     #[test]
     #[cfg(feature = "alloc")]
     fn test_too_many_path_calls() {
-        assert_eq!(
-            build_degenerate_chain(10, TrustAnchorIsActualIssuer::No),
-            Error::MaximumPathBuildCallsExceeded
-        );
+        assert!(matches!(
+            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()
+                })
+            ),
+            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<Error, Error>> {
         let ca_cert = make_issuer(format!("Bogus Subject {chain_length}"), None);
         let ca_cert_der = CertificateDer::from(ca_cert.serialize_der().unwrap());
 
@@ -933,32 +960,37 @@ 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]
     #[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]
     #[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 +1020,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::<Vec<_>>();
-
         // 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,64 +1035,56 @@ 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,
+        assert!(verify_chain(
+            &ca_cert_der,
+            &intermediates_der,
+            &ee_cert,
+            Some(passing_budget),
         )
-        .unwrap();
+        .is_ok());
 
-        let mut failing_budget = Budget {
+        let 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,
-                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));
+        assert!(matches!(
+            result,
+            Err(ControlFlow::Break(
+                Error::MaximumNameConstraintComparisonsExceeded
+            ))
+        ));
     }
 
     #[cfg(feature = "alloc")]
     fn verify_chain(
-        trust_anchor: CertificateDer<'_>,
-        intermediates_der: Vec<Vec<u8>>,
-        ee_cert: CertificateDer<'_>,
-    ) -> Result<(), Error> {
+        trust_anchor: &CertificateDer<'_>,
+        intermediates_der: &[Vec<u8>],
+        ee_cert: &CertificateDer<'_>,
+        budget: Option<Budget>,
+    ) -> Result<(), ControlFlow<Error, 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()))
             .collect::<Vec<_>>();
 
-        build_chain(
+        build_chain_inner(
             &ChainOptions {
                 eku: KeyUsage::server_auth(),
                 supported_sig_algs: &[ECDSA_P256_SHA256],
@@ -1078,6 +1094,8 @@ mod tests {
             },
             cert.inner(),
             time,
+            0,
+            &mut budget.unwrap_or_default(),
         )
     }