From 2d563a31ca43c4bb3488d110b982092679239fab Mon Sep 17 00:00:00 2001 From: Daniel McCarney Date: Thu, 21 Sep 2023 08:56:31 -0400 Subject: [PATCH] only consume budgets during path building Previously we updated the `signed_data::verify_signed_data` fn to take a `Budget` so that it could directly consume the signature check budget at the place signature verification is done. While generally a good idea, in practice it means we can't expose signature validation operations on CRLs to crate external consumers that might want to use this functionality (e.g. to verify a CRL on disk has a valid issuer). The budget type is crate-private, and while we could open that up it would be awkward to require providing a budget in the context of validating a single item's signature. The budget concept is really only meaningful in the context of path building. This commit removes the `Budget` arg from the signature validation function, and the CRL trait's signature validation function, preferring to consume budget at the call-sites in `verify_cert` where we're doing path building. --- src/alg_tests.rs | 8 +------- src/crl/mod.rs | 3 ++- src/crl/types.rs | 7 +------ src/signed_data.rs | 4 ---- src/verify_cert.rs | 4 ++-- 5 files changed, 6 insertions(+), 20 deletions(-) diff --git a/src/alg_tests.rs b/src/alg_tests.rs index adfe6a74..25dd43e3 100644 --- a/src/alg_tests.rs +++ b/src/alg_tests.rs @@ -17,7 +17,6 @@ use base64::{engine::general_purpose, Engine as _}; use crate::error::{DerTypeId, Error}; -use crate::verify_cert::Budget; use crate::{der, signed_data}; use alloc::{string::String, vec::Vec}; @@ -84,12 +83,7 @@ fn test_verify_signed_data(file_contents: &[u8], expected_result: Result<(), Err assert_eq!( expected_result, - signed_data::verify_signed_data( - SUPPORTED_ALGORITHMS_IN_TESTS, - spki_value, - &signed_data, - &mut Budget::default(), - ) + signed_data::verify_signed_data(SUPPORTED_ALGORITHMS_IN_TESTS, spki_value, &signed_data,) ); } diff --git a/src/crl/mod.rs b/src/crl/mod.rs index 93424b79..7428e17b 100644 --- a/src/crl/mod.rs +++ b/src/crl/mod.rs @@ -140,7 +140,8 @@ impl<'a> RevocationOptions<'a> { // TODO(XXX): consider whether we can refactor so this happens once up-front, instead // of per-lookup. // https://github.com/rustls/webpki/issues/81 - crl.verify_signature(supported_sig_algs, issuer_spki.as_slice_less_safe(), budget) + budget.consume_signature()?; + crl.verify_signature(supported_sig_algs, issuer_spki.as_slice_less_safe()) .map_err(crl_signature_err)?; // Verify that if the issuer has a KeyUsage bitstring it asserts cRLSign. diff --git a/src/crl/types.rs b/src/crl/types.rs index 9028754e..9dbcf620 100644 --- a/src/crl/types.rs +++ b/src/crl/types.rs @@ -5,7 +5,7 @@ use crate::der::{self, DerIterator, FromDer, Tag, CONSTRUCTED, CONTEXT_SPECIFIC} use crate::error::{DerTypeId, Error}; use crate::signed_data::{self, SignedData}; use crate::subject_name::GeneralName; -use crate::verify_cert::{Budget, PathNode}; +use crate::verify_cert::PathNode; use crate::x509::{remember_extension, set_extension_once, DistributionPointName, Extension}; #[cfg(feature = "alloc")] @@ -38,7 +38,6 @@ pub trait CertRevocationList: Sealed + Debug { &self, supported_sig_algs: &[&dyn SignatureVerificationAlgorithm], issuer_spki: &[u8], - budget: &mut Budget, ) -> Result<(), Error>; } @@ -86,13 +85,11 @@ impl CertRevocationList for OwnedCertRevocationList { &self, supported_sig_algs: &[&dyn SignatureVerificationAlgorithm], issuer_spki: &[u8], - budget: &mut Budget, ) -> Result<(), Error> { signed_data::verify_signed_data( supported_sig_algs, untrusted::Input::from(issuer_spki), &self.signed_data.borrow(), - budget, ) } } @@ -230,13 +227,11 @@ impl CertRevocationList for BorrowedCertRevocationList<'_> { &self, supported_sig_algs: &[&dyn SignatureVerificationAlgorithm], issuer_spki: &[u8], - budget: &mut Budget, ) -> Result<(), Error> { signed_data::verify_signed_data( supported_sig_algs, untrusted::Input::from(issuer_spki), &self.signed_data, - budget, ) } } diff --git a/src/signed_data.rs b/src/signed_data.rs index a1800898..e684d022 100644 --- a/src/signed_data.rs +++ b/src/signed_data.rs @@ -14,7 +14,6 @@ use crate::der::{self, FromDer}; use crate::error::{DerTypeId, Error}; -use crate::verify_cert::Budget; use pki_types::{AlgorithmIdentifier, SignatureVerificationAlgorithm}; @@ -157,10 +156,7 @@ pub(crate) fn verify_signed_data( supported_algorithms: &[&dyn SignatureVerificationAlgorithm], spki_value: untrusted::Input, signed_data: &SignedData, - budget: &mut Budget, ) -> Result<(), Error> { - budget.consume_signature()?; - // We need to verify the signature in `signed_data` using the public key // in `public_key`. In order to know which *ring* signature verification // algorithm to use, we need to know the public key algorithm (ECDSA, diff --git a/src/verify_cert.rs b/src/verify_cert.rs index 9bf2d35c..18553aa0 100644 --- a/src/verify_cert.rs +++ b/src/verify_cert.rs @@ -133,11 +133,11 @@ impl<'a> ChainOptions<'a> { 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. for path in path.iter() { + budget.consume_signature()?; signed_data::verify_signed_data( self.supported_sig_algs, spki_value, &path.cert.signed_data, - budget, )?; if let Some(revocation_opts) = &self.revocation { @@ -181,7 +181,7 @@ fn check_signed_chain_name_constraints( Ok(()) } -pub struct Budget { +pub(crate) struct Budget { signatures: usize, build_chain_calls: usize, name_constraint_comparisons: usize,