Skip to content

Commit

Permalink
verify_cert: enforce maximum number of signatures.
Browse files Browse the repository at this point in the history
Pathbuilding complexity can be quadratic, particularly when the set of
intermediates all have subjects matching a trust anchor. In these cases
we need to bound the number of expensive signature validation operations
that are performed to avoid a DoS on CPU usage.

This commit implements a simple maximum signature check limit inspired
by the approach taken in the Golang x509 package. No more than 100
signatures will be evaluated while pathbuilding. This limit works in
practice for Go when processing real world certificate chains and so
should be appropriate for our use case as well.
  • Loading branch information
cpu authored and ctz committed Aug 22, 2023
1 parent 989ef02 commit dcad240
Show file tree
Hide file tree
Showing 2 changed files with 85 additions and 4 deletions.
6 changes: 6 additions & 0 deletions src/error.rs
Original file line number Diff line number Diff line change
Expand Up @@ -93,6 +93,9 @@ pub enum Error {
/// invalid labels.
MalformedNameConstraint,

/// The maximum number of signature checks has been reached. Path complexity is too great.
MaximumSignatureChecksExceeded,

/// The certificate violates one or more name constraints.
NameConstraintViolation,

Expand Down Expand Up @@ -220,6 +223,9 @@ impl Error {
Error::BadDerTime => 2,
Error::BadDer => 1,

// Special case error - not subject to ranking.
Error::MaximumSignatureChecksExceeded => 0,

// Default catch all error - should be renamed in the future.
Error::UnknownIssuer => 0,
}
Expand Down
83 changes: 79 additions & 4 deletions src/verify_cert.rs
Original file line number Diff line number Diff line change
Expand Up @@ -27,14 +27,15 @@ pub(crate) struct ChainOptions<'a> {
}

pub(crate) fn build_chain(opts: &ChainOptions, cert: &Cert, time: time::Time) -> Result<(), Error> {
build_chain_inner(opts, cert, time, 0)
build_chain_inner(opts, cert, time, 0, &mut 0_usize)
}

fn build_chain_inner(
opts: &ChainOptions,
cert: &Cert,
time: time::Time,
sub_ca_count: usize,
signatures: &mut usize,
) -> Result<(), Error> {
let used_as_ca = used_as_ca(&cert.ee_or_ca);

Expand Down Expand Up @@ -87,7 +88,13 @@ fn build_chain_inner(

// TODO: check_distrust(trust_anchor_subject, trust_anchor_spki)?;

check_signatures(opts.supported_sig_algs, cert, trust_anchor, opts.crls)?;
check_signatures(
opts.supported_sig_algs,
cert,
trust_anchor,
opts.crls,
signatures,
)?;

Ok(())
},
Expand Down Expand Up @@ -133,7 +140,7 @@ fn build_chain_inner(
UsedAsCa::Yes => sub_ca_count + 1,
};

build_chain_inner(opts, &potential_issuer, time, next_sub_ca_count)
build_chain_inner(opts, &potential_issuer, time, next_sub_ca_count, signatures)
})
}

Expand All @@ -142,12 +149,17 @@ fn check_signatures(
cert_chain: &Cert,
trust_anchor: &TrustAnchor,
crls: &[&dyn CertRevocationList],
signatures: &mut usize,
) -> Result<(), Error> {
let mut spki_value = untrusted::Input::from(trust_anchor.spki);
let mut issuer_subject = untrusted::Input::from(trust_anchor.subject);
let mut issuer_key_usage = None; // TODO(XXX): Consider whether to track TrustAnchor KU.
let mut cert = cert_chain;
loop {
*signatures += 1;
if *signatures > 100 {
return Err(Error::MaximumSignatureChecksExceeded);
}
signed_data::verify_signed_data(supported_sig_algs, spki_value, &cert.signed_data)?;

if !crls.is_empty() {
Expand Down Expand Up @@ -487,7 +499,7 @@ impl KeyUsageMode {
fn loop_while_non_fatal_error<V>(
default_error: Error,
values: V,
f: impl Fn(V::Item) -> Result<(), Error>,
mut f: impl FnMut(V::Item) -> Result<(), Error>,
) -> Result<(), Error>
where
V: IntoIterator,
Expand All @@ -496,6 +508,7 @@ where
for v in values {
match f(v) {
Ok(()) => return Ok(()),
err @ Err(Error::MaximumSignatureChecksExceeded) => return err,
Err(new_error) => error = error.most_specific(new_error),
}
}
Expand All @@ -511,4 +524,66 @@ mod tests {
assert!(ExtendedKeyUsage::RequiredIfPresent(EKU_SERVER_AUTH)
.key_purpose_id_equals(EKU_SERVER_AUTH.oid_value))
}

#[test]
#[cfg(feature = "alloc")]
fn test_too_many_signatures() {
use crate::ECDSA_P256_SHA256;
use crate::{EndEntityCert, Time};

let alg = &rcgen::PKCS_ECDSA_P256_SHA256;

let make_issuer = || {
let mut ca_params = rcgen::CertificateParams::new(Vec::new());
ca_params
.distinguished_name
.push(rcgen::DnType::OrganizationName, "Bogus Subject");
ca_params.is_ca = rcgen::IsCa::Ca(rcgen::BasicConstraints::Unconstrained);
ca_params.key_usages = vec![
rcgen::KeyUsagePurpose::KeyCertSign,
rcgen::KeyUsagePurpose::DigitalSignature,
rcgen::KeyUsagePurpose::CrlSign,
];
ca_params.alg = alg;
rcgen::Certificate::from_params(ca_params).unwrap()
};

let ca_cert = make_issuer();
let ca_cert_der = ca_cert.serialize_der().unwrap();

let mut intermediates = Vec::with_capacity(101);
let mut issuer = ca_cert;
for _ in 0..101 {
let intermediate = make_issuer();
let intermediate_der = intermediate.serialize_der_with_signer(&issuer).unwrap();
intermediates.push(intermediate_der);
issuer = intermediate;
}

let mut ee_params = rcgen::CertificateParams::new(vec!["example.com".to_string()]);
ee_params.is_ca = rcgen::IsCa::ExplicitNoCa;
ee_params.alg = alg;
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 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 result = build_chain(
&ChainOptions {
eku: KeyUsage::server_auth(),
supported_sig_algs: &[&ECDSA_P256_SHA256],
trust_anchors: anchors,
intermediate_certs,
crls: &[],
},
cert.inner(),
time,
);

assert!(matches!(result, Err(Error::MaximumSignatureChecksExceeded)));
}
}

0 comments on commit dcad240

Please sign in to comment.