From d16d7aa738d5b33e9fa13031fcd805f4475b5a8b Mon Sep 17 00:00:00 2001
From: Daniel McCarney <daniel@binaryparadox.net>
Date: Thu, 7 Sep 2023 11:53:02 -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 centralizes the determination of whether an error is fatal
or not to a new `Error.is_fatal` fn, allowing the logic to be used in
the two places we need to evaluate an error for fatal status in
`verify_cert.rs`. We now correctly terminate path building for fatal
errors after evaluating a path to a trust anchor, instead of proceeding
for further path building.

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/error.rs       | 12 ++++++++++++
 src/verify_cert.rs | 24 ++++++++++++++++++++----
 2 files changed, 32 insertions(+), 4 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 dec31ecf..181d7e64 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 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,
     };
 
@@ -746,9 +751,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),
         }
     }
@@ -921,7 +927,17 @@ mod tests {
     #[cfg(feature = "alloc")]
     fn test_too_many_path_calls() {
         assert_eq!(
-            build_degenerate_chain(10, TrustAnchorIsActualIssuer::No, None),
+            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
         );
     }