From 50a293091c1e4c1b1cab8c147fbb0981671f971b Mon Sep 17 00:00:00 2001
From: Daniel McCarney <daniel@binaryparadox.net>
Date: Thu, 7 Sep 2023 13:07:41 -0400
Subject: [PATCH] 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 | 23 +++++++++++++++++------
 1 file changed, 17 insertions(+), 6 deletions(-)

diff --git a/src/verify_cert.rs b/src/verify_cert.rs
index cc977cf1..5e2f0b6c 100644
--- a/src/verify_cert.rs
+++ b/src/verify_cert.rs
@@ -112,9 +112,9 @@ fn build_chain_inner(
     // If the error is not fatal, then keep going.
     match result {
         Ok(()) => return Ok(()),
-        err @ Err(Error::MaximumSignatureChecksExceeded)
-        | err @ Err(Error::MaximumPathBuildCallsExceeded)
-        | err @ Err(Error::MaximumNameConstraintComparisonsExceeded) => return err,
+        // Fatal errors should halt further path building.
+        res @ Err(err) if err.is_fatal() => return res,
+        // Non-fatal errors should allow path building to continue.
         _ => {}
     };
 
@@ -488,6 +488,7 @@ mod tests {
     fn build_degenerate_chain(
         intermediate_count: usize,
         trust_anchor_is_actual_issuer: TrustAnchorIsActualIssuer,
+        budget: Option<Budget>,
     ) -> Error {
         let ca_cert = make_issuer("Bogus Subject", None);
         let ca_cert_der = ca_cert.serialize_der().unwrap();
@@ -509,7 +510,7 @@ mod tests {
             &ca_cert_der,
             &intermediates,
             &make_end_entity(&issuer),
-            None,
+            budget,
         )
         .unwrap_err()
     }
@@ -518,7 +519,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
         );
     }
@@ -527,7 +528,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
         );
     }