Skip to content

Commit

Permalink
verify_cert: fix build_degenerate_chain
Browse files Browse the repository at this point in the history
This commit updates the `build_degenerate_chain` helper to properly
reproduce the path building complexity budget issue that was reported
upstream in briansmith/webpki. The previous implementation only
reproduced the issue when the signature validation budget was
artificially inflated.

The new formulation is able to reproduce the issue with the default
signature validation budget, and default build chain call budget (and
completes in reasonable time). This better demonstrates the both limits
are needed as its possible to make pathological certificate chains that
avoid the one limit and are caught by the other.
  • Loading branch information
cpu committed Sep 15, 2023
1 parent 886da28 commit 0ce4235
Showing 1 changed file with 20 additions and 26 deletions.
46 changes: 20 additions & 26 deletions src/verify_cert.rs
Original file line number Diff line number Diff line change
Expand Up @@ -508,21 +508,25 @@ mod tests {
}

#[cfg(feature = "alloc")]
enum TrustAnchorIsActualIssuer {
Yes,
No,
enum TrustAnchor {
InChain,
NotInChain,
}

#[cfg(feature = "alloc")]
fn build_degenerate_chain(
intermediate_count: usize,
trust_anchor_is_actual_issuer: TrustAnchorIsActualIssuer,
trust_anchor: TrustAnchor,
budget: Option<Budget>,
) -> ControlFlow<Error, Error> {
let ca_cert = make_issuer("Bogus Subject");
let ca_cert_der = CertificateDer::from(ca_cert.serialize_der().unwrap());

let mut intermediates = Vec::with_capacity(intermediate_count);
let mut intermediates = Vec::with_capacity(intermediate_count + 1);
if let TrustAnchor::InChain = trust_anchor {
intermediates.push(ca_cert_der.to_vec());
}

let mut issuer = ca_cert;
for _ in 0..intermediate_count {
let intermediate = make_issuer("Bogus Subject");
Expand All @@ -531,12 +535,16 @@ mod tests {
issuer = intermediate;
}

if let TrustAnchorIsActualIssuer::No = trust_anchor_is_actual_issuer {
intermediates.pop();
}
let trust_anchor = match trust_anchor {
TrustAnchor::InChain => {
let unused_anchor = make_issuer("Bogus Trust Anchor");
CertificateDer::from(unused_anchor.serialize_der().unwrap())
}
TrustAnchor::NotInChain => ca_cert_der,
};

verify_chain(
&ca_cert_der,
&trust_anchor,
&intermediates,
&make_end_entity(&issuer),
budget,
Expand All @@ -548,30 +556,16 @@ mod tests {
#[cfg(feature = "alloc")]
fn test_too_many_signatures() {
assert!(matches!(
build_degenerate_chain(5, TrustAnchorIsActualIssuer::Yes, None),
build_degenerate_chain(5, TrustAnchor::NotInChain, None),
ControlFlow::Break(Error::MaximumSignatureChecksExceeded)
));
}

#[test]
#[cfg(feature = "alloc")]
fn test_too_many_path_calls_mini() {
fn test_too_many_path_calls() {
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,
// We don't use the default build chain budget here. Doing so makes this test
// run slowly (due to testing quadratic runtime up to the limit) without adding
// much value from a testing perspective over using a smaller non-default limit.
build_chain_calls: 100,
..Budget::default()
})
),
dbg!(build_degenerate_chain(10, TrustAnchor::InChain, None)),
ControlFlow::Break(Error::MaximumPathBuildCallsExceeded)
));
}
Expand Down

0 comments on commit 0ce4235

Please sign in to comment.