saymrwulf/cryptography
1
0
Fork 0
mirror of https://github.com/saymrwulf/cryptography.git synced 2026-05-14 20:37:55 +00:00
Code Issues Projects Releases Packages Wiki Activity Actions

Added a budget for NC checks to protect against DoS (#10467) (#10468)

Browse source
This commit is contained in:
Alex Gaynor 2024-02-23 19:40:59 -05:00 committed by GitHub
parent 8e9de309f8
commit 4be53bf20c
No known key found for this signature in database
GPG key ID: B5690EEEBB952194
4 changed files with 62 additions and 9 deletions
Show stats Download patch file Download diff file Expand all files Collapse all files

4
.github/actions/fetch-vectors/action.yml vendored
View file

@ -16,5 +16,5 @@ runs:
with:
repository: "C2SP/x509-limbo"
path: "x509-limbo"
# Latest commit on the x509-limbo main branch, as of Jan 23, 2024.
ref: "cf66142f5c27b64c987c6f0aa4c10b8c9677b41c" # x509-limbo-ref
# Latest commit on the x509-limbo main branch, as of Feb 23, 2024.
ref: "c8f6a4f4946076db55778ed7b3cffdab082a1a12" # x509-limbo-ref

47
src/rust/cryptography-x509-verification/src/lib.rs
View file

@ -33,9 +33,35 @@ pub enum ValidationError {
CandidatesExhausted(Box<ValidationError>),
Malformed(asn1::ParseError),
DuplicateExtension(DuplicateExtensionsError),
FatalError(&'static str),
Other(String),
}
struct Budget {
name_constraint_checks: usize,
}
impl Budget {
// Same limit as other validators
const DEFAULT_NAME_CONSTRAINT_CHECK_LIMIT: usize = 1 << 20;
fn new() -> Budget {
Budget {
name_constraint_checks: Self::DEFAULT_NAME_CONSTRAINT_CHECK_LIMIT,
}
}
fn name_constraint_check(&mut self) -> Result<(), ValidationError> {
self.name_constraint_checks =
self.name_constraint_checks
.checked_sub(1)
.ok_or(ValidationError::FatalError(
"Exceeded maximum name constraint check limit",
))?;
Ok(())
}
}
impl From<asn1::ParseError> for ValidationError {
fn from(value: asn1::ParseError) -> Self {
Self::Malformed(value)
@ -76,7 +102,10 @@ impl<'a, 'chain> NameChain<'a, 'chain> {
&self,
constraint: &GeneralName<'chain>,
san: &GeneralName<'chain>,
budget: &mut Budget,
) -> Result<ApplyNameConstraintStatus, ValidationError> {
budget.name_constraint_check()?;
match (constraint, san) {
(GeneralName::DNSName(pattern), GeneralName::DNSName(name)) => {
match (DNSConstraint::new(pattern.0), DNSName::new(name.0)) {
@ -114,9 +143,10 @@ impl<'a, 'chain> NameChain<'a, 'chain> {
fn evaluate_constraints(
&self,
constraints: &NameConstraints<'chain>,
budget: &mut Budget,
) -> Result<(), ValidationError> {
if let Some(child) = self.child {
child.evaluate_constraints(constraints)?;
child.evaluate_constraints(constraints, budget)?;
}
for san in self.sans.clone() {
@ -124,7 +154,7 @@ impl<'a, 'chain> NameChain<'a, 'chain> {
let mut permit = true;
if let Some(permitted_subtrees) = &constraints.permitted_subtrees {
for p in permitted_subtrees.unwrap_read().clone() {
let status = self.evaluate_single_constraint(&p.base, &san)?;
let status = self.evaluate_single_constraint(&p.base, &san, budget)?;
if status.is_applied() {
permit = status.is_match();
if permit {
@ -142,7 +172,7 @@ impl<'a, 'chain> NameChain<'a, 'chain> {
if let Some(excluded_subtrees) = &constraints.excluded_subtrees {
for e in excluded_subtrees.unwrap_read().clone() {
let status = self.evaluate_single_constraint(&e.base, &san)?;
let status = self.evaluate_single_constraint(&e.base, &san, budget)?;
if status.is_match() {
return Err(ValidationError::Other(
"excluded name constraint matched SAN".into(),
@ -166,7 +196,8 @@ pub fn verify<'chain, B: CryptoOps>(
) -> Result<Chain<'chain, B>, ValidationError> {
let builder = ChainBuilder::new(intermediates.into_iter().collect(), policy, store);
builder.build_chain(leaf)
let mut budget = Budget::new();
builder.build_chain(leaf, &mut budget)
}
struct ChainBuilder<'a, 'chain, B: CryptoOps> {
@ -227,9 +258,10 @@ impl<'a, 'chain, B: CryptoOps> ChainBuilder<'a, 'chain, B> {
current_depth: u8,
working_cert_extensions: &Extensions<'chain>,
name_chain: NameChain<'_, 'chain>,
budget: &mut Budget,
) -> Result<Chain<'chain, B>, ValidationError> {
if let Some(nc) = working_cert_extensions.get_extension(&NAME_CONSTRAINTS_OID) {
name_chain.evaluate_constraints(&nc.value()?)?;
name_chain.evaluate_constraints(&nc.value()?, budget)?;
}
// Look in the store's root set to see if the working cert is listed.
@ -295,11 +327,14 @@ impl<'a, 'chain, B: CryptoOps> ChainBuilder<'a, 'chain, B> {
// candidate (which is a non-leaf by definition) isn't self-issued.
cert_is_self_issued(issuing_cert_candidate.certificate()),
)?,
budget,
) {
Ok(mut chain) => {
chain.push(working_cert.clone());
return Ok(chain);
}
// Immediately return on fatal error.
Err(e @ ValidationError::FatalError(..)) => return Err(e),
Err(e) => last_err = Some(e),
};
}
@ -326,6 +361,7 @@ impl<'a, 'chain, B: CryptoOps> ChainBuilder<'a, 'chain, B> {
fn build_chain(
&self,
leaf: &VerificationCertificate<'chain, B>,
budget: &mut Budget,
) -> Result<Chain<'chain, B>, ValidationError> {
// Before anything else, check whether the given leaf cert
// is well-formed according to our policy (and its underlying
@ -342,6 +378,7 @@ impl<'a, 'chain, B: CryptoOps> ChainBuilder<'a, 'chain, B> {
0,
&leaf_extensions,
NameChain::new(None, &leaf_extensions, false)?,
budget,
)?;
// We build the chain in reverse order, fix it now.
chain.reverse();

9
src/rust/cryptography-x509-verification/src/policy/mod.rs
View file

@ -468,9 +468,11 @@ impl<'a, B: CryptoOps> Policy<'a, B> {
self.permits_ca(issuer.certificate(), current_depth, issuer_extensions)?;
// CA/B 7.1.3.1 SubjectPublicKeyInfo
// NOTE: We check the issuer's SPKI here, since the issuer is
// definitionally a CA and thus subject to CABF key requirements.
if !self
.permitted_public_key_algorithms
.contains(&child.tbs_cert.spki.algorithm)
.contains(&issuer.certificate().tbs_cert.spki.algorithm)
{
return Err(ValidationError::Other(format!(
"Forbidden public key algorithm: {:?}",
@ -479,6 +481,11 @@ impl<'a, B: CryptoOps> Policy<'a, B> {
}
// CA/B 7.1.3.2 Signature AlgorithmIdentifier
// NOTE: We check the child's signature here, since the issuer's
// signature is not necessarily subject to signature checks (e.g.
// if it's a root). This works out transitively, as any non root-issuer
// will be checked in its recursive step (where it'll be in the child
// position).
if !self
.permitted_signature_algorithms
.contains(&child.signature_alg)

11
tests/x509/verification/test_limbo.py
View file

@ -27,7 +27,10 @@ LIMBO_UNSUPPORTED_FEATURES = {
# Our support for custom EKUs is limited, and we (like most impls.) don't
# handle all EKU conditions under CABF.
"pedantic-webpki-eku",
# Similarly: contains tests that fail based on a strict reading of RFC 5280
# Most CABF validators do not enforce the CABF key requirements on
# subscriber keys (i.e., in the leaf certificate).
"pedantic-webpki-subscriber-key",
# Tests that fail based on a strict reading of RFC 5280
# but are widely ignored by validators.
"pedantic-rfc5280",
# In rare circumstances, CABF relaxes RFC 5280's prescriptions in
@ -62,11 +65,17 @@ LIMBO_SKIP_TESTCASES = {
# forbidden under CABF. This is consistent with what
# Go's crypto/x509 and Rust's webpki crate do.
"webpki::aki::root-with-aki-ski-mismatch",
# We allow RSA keys that aren't divisible by 8, which is technically
# forbidden under CABF. No other implementation checks this either.
"webpki::forbidden-rsa-not-divisable-by-8-in-root",
# We disallow CAs in the leaf position, which is explicitly forbidden
# by CABF (but implicitly permitted under RFC 5280). This is consistent
# with what webpki and rustls do, but inconsistent with Go and OpenSSL.
"rfc5280::ca-as-leaf",
"pathlen::validation-ignores-pathlen-in-leaf",
# Implemented on main, but not backported to this branch.
"webpki::forbidden-p192-root",
"webpki::forbidden-weak-rsa-key-in-root",
}