Skip to content

Commit

Permalink
verify_cert: apply path building budget
Browse files Browse the repository at this point in the history 
This is intended to be complementary to the signature validation limit
fix and addresses #276 in the same manner as NSS
libmozpkix.
  • Loading branch information
@cpu @briansmith
cpu authored and briansmith committed Sep 30, 2023
1 parent 3ee04be commit f566cf1
Showing 1 changed file with 69 additions and 23 deletions.
92 changes: 69 additions & 23 deletions src/verify_cert.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -51,6 +51,8 @@ pub fn build_chain(
/// for testing).
enum InternalError {
MaximumSignatureChecksExceeded,
/// The maximum number of internal path building calls has been reached. Path complexity is too great.
MaximumPathBuildCallsExceeded,
}

enum ErrorOrInternalError {
Expand All @@ -62,7 +64,8 @@ impl ErrorOrInternalError {
fn is_fatal(&self) -> bool {
match self {
ErrorOrInternalError::Error(_) => false,
ErrorOrInternalError::InternalError(InternalError::MaximumSignatureChecksExceeded) => {
ErrorOrInternalError::InternalError(InternalError::MaximumSignatureChecksExceeded)
| ErrorOrInternalError::InternalError(InternalError::MaximumPathBuildCallsExceeded) => {
true
}
}
Expand Down Expand Up @@ -185,6 +188,7 @@ fn build_chain_inner(
UsedAsCa::Yes => sub_ca_count + 1,
};

budget.consume_build_chain_call()?;
build_chain_inner(
required_eku_if_present,
supported_sig_algs,
Expand Down Expand Up @@ -228,6 +232,7 @@ fn check_signatures(

struct Budget {
signatures: usize,
build_chain_calls: usize,
}

impl Budget {
Expand All @@ -239,6 +244,15 @@ impl Budget {
.ok_or(InternalError::MaximumSignatureChecksExceeded)?;
Ok(())
}

#[inline]
fn consume_build_chain_call(&mut self) -> Result<(), InternalError> {
self.build_chain_calls = self
.build_chain_calls
.checked_sub(1)
.ok_or(InternalError::MaximumPathBuildCallsExceeded)?;
Ok(())
}
}

impl Default for Budget {
Expand All @@ -249,6 +263,10 @@ impl Default for Budget {
// being hit in real applications (see <https://github.com/spiffe/spire/issues/1004>).
// So this may actually be too aggressive.
signatures: 100,

// This limit is taken from NSS libmozpkix, see:
// <https://github.com/nss-dev/nss/blob/bb4a1d38dd9e92923525ac6b5ed0288479f3f3fc/lib/mozpkix/lib/pkixbuild.cpp#L381-L393>
build_chain_calls: 200_000,
}
}
}
Expand Down Expand Up @@ -453,25 +471,35 @@ where
Err(Error::UnknownIssuer.into())
}

#[cfg(feature = "alloc")]
#[cfg(test)]
mod tests {
use alloc::string::ToString;
use alloc::vec::Vec;
use core::convert::TryFrom;

#[test]
#[cfg(feature = "alloc")]
fn test_too_many_signatures() {
use super::*;
use super::*;
use crate::verify_cert::{Budget, InternalError};

enum ChainTrustAnchor {
InChain,
NotInChain,
}

fn build_degenerate_chain(
intermediate_count: usize,
trust_anchor: ChainTrustAnchor,
) -> ErrorOrInternalError {
use crate::ECDSA_P256_SHA256;
use crate::{EndEntityCert, Time};
use alloc::{string::ToString, vec::Vec};
use core::convert::TryFrom;

let alg = &rcgen::PKCS_ECDSA_P256_SHA256;

let make_issuer = || {
let make_issuer = |org_name| {
let mut ca_params = rcgen::CertificateParams::new(Vec::new());
ca_params
.distinguished_name
.push(rcgen::DnType::OrganizationName, "Bogus Subject");
.push(rcgen::DnType::OrganizationName, org_name);
ca_params.is_ca = rcgen::IsCa::Ca(rcgen::BasicConstraints::Unconstrained);
ca_params.key_usages = vec![
rcgen::KeyUsagePurpose::KeyCertSign,
Expand All @@ -482,13 +510,17 @@ mod tests {
rcgen::Certificate::from_params(ca_params).unwrap()
};

let ca_cert = make_issuer();
let ca_cert = make_issuer("Bogus Subject");
let ca_cert_der = ca_cert.serialize_der().unwrap();

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

let mut issuer = ca_cert;
for _ in 0..101 {
let intermediate = make_issuer();
for _ in 0..intermediate_count {
let intermediate = make_issuer("Bogus Subject");
let intermediate_der = intermediate.serialize_der_with_signer(&issuer).unwrap();
intermediates.push(intermediate_der);
issuer = intermediate;
Expand All @@ -500,29 +532,43 @@ mod tests {
let ee_cert = rcgen::Certificate::from_params(ee_params).unwrap();
let ee_cert_der = ee_cert.serialize_der_with_signer(&issuer).unwrap();

let anchors = &[TrustAnchor::try_from_cert_der(&ca_cert_der).unwrap()];
let trust_anchor = match trust_anchor {
ChainTrustAnchor::InChain => make_issuer("Bogus Trust Anchor").serialize_der().unwrap(),
ChainTrustAnchor::NotInChain => ca_cert_der.clone(),
};

let anchors = &[TrustAnchor::try_from_cert_der(&trust_anchor).unwrap()];
let time = Time::from_seconds_since_unix_epoch(0x1fed_f00d);
let cert = EndEntityCert::try_from(&ee_cert_der[..]).unwrap();
let intermediates_der: Vec<&[u8]> = intermediates.iter().map(|x| x.as_ref()).collect();
let intermediate_certs: &[&[u8]] = intermediates_der.as_ref();
let intermediate_certs = intermediates.iter().map(|x| x.as_ref()).collect::<Vec<_>>();

// TODO: Use `build_chain` when `Error` is made non-exhaustive.
let result = build_chain_inner(
build_chain_inner(
EKU_SERVER_AUTH,
&[&ECDSA_P256_SHA256],
anchors,
intermediate_certs,
&intermediate_certs,
cert.inner(),
time,
0,
&mut Budget::default(),
);
)
.unwrap_err()
}

#[test]
fn test_too_many_signatures() {
assert!(matches!(
build_degenerate_chain(5, ChainTrustAnchor::NotInChain),
ErrorOrInternalError::InternalError(InternalError::MaximumSignatureChecksExceeded),
));
}

#[test]
fn test_too_many_path_calls() {
let result = build_degenerate_chain(10, ChainTrustAnchor::InChain);
assert!(matches!(
result,
Err(ErrorOrInternalError::InternalError(
InternalError::MaximumSignatureChecksExceeded
))
ErrorOrInternalError::InternalError(InternalError::MaximumPathBuildCallsExceeded),
));
}
}

0 comments on commit f566cf1

Please sign in to comment.