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

alex · web-flow · commit 4be53bf20cc9 · 2024-02-24T00:40:59.000Z
diff --git a/.github/actions/fetch-vectors/action.yml b/.github/actions/fetch-vectors/action.yml
@@ -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
diff --git a/src/rust/cryptography-x509-verification/src/lib.rs b/src/rust/cryptography-x509-verification/src/lib.rs
@@ -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,17 +143,18 @@ 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() {
             // If there are no applicable constraints, the SAN is considered valid so the default is true.
             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();
diff --git a/src/rust/cryptography-x509-verification/src/policy/mod.rs b/src/rust/cryptography-x509-verification/src/policy/mod.rs
@@ -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)
diff --git a/tests/x509/verification/test_limbo.py b/tests/x509/verification/test_limbo.py
@@ -27,7 +27,10 @@
     # 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 @@
     # 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",
 }
 
 
