Skip to content

Commit

Permalink
verify_cert: correct handling of fatal errors
Browse files Browse the repository at this point in the history
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).
  • Loading branch information
cpu authored and djc committed Sep 12, 2023
1 parent b166eab commit c255d61
Showing 1 changed file with 19 additions and 3 deletions.
22 changes: 19 additions & 3 deletions src/verify_cert.rs
Expand Up @@ -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,
};

Expand Down Expand Up @@ -604,6 +609,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();
Expand All @@ -625,7 +631,7 @@ mod tests {
&ca_cert_der,
&intermediates,
&make_end_entity(&issuer),
None,
budget,
)
.unwrap_err()
}
Expand All @@ -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
);
}
Expand All @@ -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
);
}
Expand Down

0 comments on commit c255d61

Please sign in to comment.