verify_cert: name constraint checking on verified chain

This commit updates the path building process such that name constraints
are only evaluated against a complete path where signatures on the chain
have been checked successfully to a trust anchor. This avoids:

* Parsing name constraints before signatures are validated.
* Evaluating name constraints and consuming name constraint comparison
  budget for certificates that are not part of the
  built path.

In the future it could be possible to interleave the name constraint
checking with the signature checking, however the logic for this is more
complicated. For an initial fix let's prefer a simpler solution that
walks the built + validated path to check name constraints from the
trust anchor to the end entity certificate.

src/verify_cert.rs

@@ -94,25 +94,27 @@ fn build_chain_inner(
             return Err(Error::UnknownIssuer);
         }
 
-        let name_constraints = trust_anchor.name_constraints.map(untrusted::Input::from);
-        untrusted::read_all_optional(name_constraints, Error::BadDer, |value| {
-            subject_name::check_name_constraints(value, cert, subject_common_name_contents, budget)
-        })?;
-
         // TODO: check_distrust(trust_anchor_subject, trust_anchor_spki)?;
 
         let trust_anchor_spki = untrusted::Input::from(trust_anchor.spki);
-
         check_signed_chain(supported_sig_algs, cert, trust_anchor_spki, budget)?;
 
+        check_signed_chain_name_constraints(
+            cert,
+            trust_anchor,
+            subject_common_name_contents,
+            budget,
+        )?;
+
         Ok(())
     });
 
     // If the error is not fatal, then keep going.
     match result {
         Ok(()) => return Ok(()),
         err @ Err(Error::MaximumSignatureChecksExceeded)
-        | err @ Err(Error::MaximumPathBuildCallsExceeded) => return err,
+        | err @ Err(Error::MaximumPathBuildCallsExceeded)
+        | err @ Err(Error::MaximumNameConstraintComparisonsExceeded) => return err,
         _ => {}
     };
 
@@ -142,10 +144,6 @@ fn build_chain_inner(
             }
         }
 
-        untrusted::read_all_optional(potential_issuer.name_constraints, Error::BadDer, |value| {
-            subject_name::check_name_constraints(value, cert, subject_common_name_contents, budget)
-        })?;
-
         let next_sub_ca_count = match used_as_ca {
             UsedAsCa::No => sub_ca_count,
             UsedAsCa::Yes => sub_ca_count + 1,
@@ -192,6 +190,37 @@ fn check_signed_chain(
     Ok(())
 }
 
+fn check_signed_chain_name_constraints(
+    cert_chain: &Cert,
+    trust_anchor: &TrustAnchor,
+    subject_common_name_contents: subject_name::SubjectCommonNameContents,
+    budget: &mut Budget,
+) -> Result<(), Error> {
+    let mut cert = cert_chain;
+    let mut name_constraints = trust_anchor
+        .name_constraints
+        .as_ref()
+        .map(|der| untrusted::Input::from(der));
+
+    loop {
+        untrusted::read_all_optional(name_constraints, Error::BadDer, |value| {
+            subject_name::check_name_constraints(value, cert, subject_common_name_contents, budget)
+        })?;
+
+        match &cert.ee_or_ca {
+            EndEntityOrCa::Ca(child_cert) => {
+                name_constraints = cert.name_constraints;
+                cert = child_cert;
+            }
+            EndEntityOrCa::EndEntity => {
+                break;
+            }
+        }
+    }
+
+    Ok(())
+}
+
 pub(crate) struct Budget {
     signatures: usize,
     build_chain_calls: usize,
@@ -460,13 +489,13 @@ mod tests {
         intermediate_count: usize,
         trust_anchor_is_actual_issuer: TrustAnchorIsActualIssuer,
     ) -> Error {
-        let ca_cert = make_issuer("Bogus Subject");
+        let ca_cert = make_issuer("Bogus Subject", None);
         let ca_cert_der = ca_cert.serialize_der().unwrap();
 
         let mut intermediates = Vec::with_capacity(intermediate_count);
         let mut issuer = ca_cert;
         for _ in 0..intermediate_count {
-            let intermediate = make_issuer("Bogus Subject");
+            let intermediate = make_issuer("Bogus Subject", None);
             let intermediate_der = intermediate.serialize_der_with_signer(&issuer).unwrap();
             intermediates.push(intermediate_der);
             issuer = intermediate;
@@ -499,13 +528,13 @@ mod tests {
 
     #[cfg(feature = "alloc")]
     fn build_linear_chain(chain_length: usize) -> Result<(), Error> {
-        let ca_cert = make_issuer(format!("Bogus Subject {chain_length}"));
+        let ca_cert = make_issuer(format!("Bogus Subject {chain_length}"), None);
         let ca_cert_der = ca_cert.serialize_der().unwrap();
 
         let mut intermediates = Vec::with_capacity(chain_length);
         let mut issuer = ca_cert;
         for i in 0..chain_length {
-            let intermediate = make_issuer(format!("Bogus Subject {i}"));
+            let intermediate = make_issuer(format!("Bogus Subject {i}"), None);
             let intermediate_der = intermediate.serialize_der_with_signer(&issuer).unwrap();
             intermediates.push(intermediate_der);
             issuer = intermediate;
@@ -533,6 +562,98 @@ mod tests {
         assert_eq!(build_linear_chain(7), Err(Error::UnknownIssuer));
     }
 
+    #[test]
+    #[cfg(feature = "alloc")]
+    fn name_constraint_budget() {
+        use crate::ECDSA_P256_SHA256;
+        use crate::{EndEntityCert, Time};
+
+        // Issue a trust anchor that imposes name constraints. The constraint should match
+        // the end entity certificate SAN.
+        let ca_cert = make_issuer(
+            "Constrained Root",
+            Some(rcgen::NameConstraints {
+                permitted_subtrees: vec![rcgen::GeneralSubtree::DnsName(".com".into())],
+                excluded_subtrees: vec![],
+            }),
+        );
+        let ca_cert_der = ca_cert.serialize_der().unwrap();
+
+        // Create a series of intermediate issuers. We'll only use one in the actual built path,
+        // helping demonstrate that the name constraint budget is not expended checking certificates
+        // that are not part of the path we compute.
+        const NUM_INTERMEDIATES: usize = 5;
+        let mut intermediates = Vec::with_capacity(NUM_INTERMEDIATES);
+        for i in 0..NUM_INTERMEDIATES {
+            intermediates.push(make_issuer(format!("Intermediate {i}"), None));
+        }
+
+        // Each intermediate should be issued by the trust anchor.
+        let mut intermediates_der = Vec::with_capacity(NUM_INTERMEDIATES);
+        for intermediate in &intermediates {
+            intermediates_der.push(intermediate.serialize_der_with_signer(&ca_cert).unwrap());
+        }
+
+        // Create an end-entity cert that is issued by the last of the intermediates.
+        let ee_cert = make_end_entity(intermediates.last().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[..]).unwrap();
+        let intermediates_der = intermediates_der
+            .iter()
+            .map(|x| x.as_ref())
+            .collect::<Vec<_>>();
+
+        // We use a custom budget to make it easier to write a test, otherwise it is tricky to
+        // stuff enough names/constraints into the potential chains while staying within the path
+        // depth limit and the build chain call limit.
+        let mut passing_budget = Budget {
+            // One comparison against the intermediate's distinguished name.
+            // One comparison against the EE's distinguished name.
+            // One comparison against the EE's SAN.
+            //  = 3 total comparisons.
+            name_constraint_comparisons: 3,
+            ..Budget::default()
+        };
+
+        // Validation should succeed with the name constraint comparison budget allocated above.
+        // This shows that we're not consuming budget on unused intermediates: we didn't budget
+        // enough comparisons for that to pass the overall chain building.
+        build_chain_inner(
+            EKU_SERVER_AUTH,
+            &[&ECDSA_P256_SHA256],
+            anchors,
+            &intermediates_der,
+            cert.inner(),
+            time,
+            0,
+            &mut passing_budget,
+        )
+        .unwrap();
+
+        let mut failing_budget = Budget {
+            // See passing_budget: 2 comparisons is not sufficient.
+            name_constraint_comparisons: 2,
+            ..Budget::default()
+        };
+        // Validation should fail when the budget is smaller than the number of comparisons performed
+        // on the validated path. This demonstrates we properly fail path building when too many
+        // name constraint comparisons occur.
+        let result = build_chain_inner(
+            EKU_SERVER_AUTH,
+            &[&ECDSA_P256_SHA256],
+            anchors,
+            &intermediates_der,
+            cert.inner(),
+            time,
+            0,
+            &mut failing_budget,
+        );
+
+        assert_eq!(result, Err(Error::MaximumNameConstraintComparisonsExceeded));
+    }
+
     #[cfg(feature = "alloc")]
     fn verify_chain(
         trust_anchor_der: Vec<u8>,
@@ -561,7 +682,10 @@ mod tests {
     }
 
     #[cfg(feature = "alloc")]
-    fn make_issuer(org_name: impl Into<String>) -> rcgen::Certificate {
+    fn make_issuer(
+        org_name: impl Into<String>,
+        name_constraints: Option<rcgen::NameConstraints>,
+    ) -> rcgen::Certificate {
         let mut ca_params = rcgen::CertificateParams::new(Vec::new());
         ca_params
             .distinguished_name
@@ -573,6 +697,7 @@ mod tests {
             rcgen::KeyUsagePurpose::CrlSign,
         ];
         ca_params.alg = &rcgen::PKCS_ECDSA_P256_SHA256;
+        ca_params.name_constraints = name_constraints;
         rcgen::Certificate::from_params(ca_params).unwrap()
     }