Further limits on expensive path building

src/error.rs

@@ -96,6 +96,12 @@ pub enum Error {
     /// The maximum number of signature checks has been reached. Path complexity is too great.
     MaximumSignatureChecksExceeded,
 
+    /// The maximum number of internal path building calls has been reached. Path complexity is too great.
+    MaximumPathBuildCallsExceeded,
+
+    /// The path search was terminated because it became too deep.
+    MaximumPathDepthExceeded,
+
     /// The certificate violates one or more name constraints.
     NameConstraintViolation,
 
@@ -198,48 +204,50 @@ impl Error {
     pub(crate) fn rank(&self) -> u32 {
         match &self {
             // Errors related to certificate validity
-            Error::CertNotValidYet | Error::CertExpired => 29,
-            Error::CertNotValidForName => 28,
-            Error::CertRevoked | Error::UnknownRevocationStatus => 27,
-            Error::InvalidCrlSignatureForPublicKey | Error::InvalidSignatureForPublicKey => 26,
-            Error::SignatureAlgorithmMismatch => 25,
-            Error::RequiredEkuNotFound => 24,
-            Error::NameConstraintViolation => 23,
-            Error::PathLenConstraintViolated => 22,
-            Error::CaUsedAsEndEntity | Error::EndEntityUsedAsCa => 21,
-            Error::IssuerNotCrlSigner => 20,
+            Error::CertNotValidYet | Error::CertExpired => 290,
+            Error::CertNotValidForName => 280,
+            Error::CertRevoked | Error::UnknownRevocationStatus => 270,
+            Error::InvalidCrlSignatureForPublicKey | Error::InvalidSignatureForPublicKey => 260,
+            Error::SignatureAlgorithmMismatch => 250,
+            Error::RequiredEkuNotFound => 240,
+            Error::NameConstraintViolation => 230,
+            Error::PathLenConstraintViolated => 220,
+            Error::CaUsedAsEndEntity | Error::EndEntityUsedAsCa => 210,
+            Error::IssuerNotCrlSigner => 200,
 
             // Errors related to supported features used in an invalid way.
-            Error::InvalidCertValidity => 19,
-            Error::InvalidNetworkMaskConstraint => 18,
-            Error::InvalidSerialNumber => 17,
-            Error::InvalidCrlNumber => 16,
+            Error::InvalidCertValidity => 190,
+            Error::InvalidNetworkMaskConstraint => 180,
+            Error::InvalidSerialNumber => 170,
+            Error::InvalidCrlNumber => 160,
 
             // Errors related to unsupported features.
             Error::UnsupportedCrlSignatureAlgorithmForPublicKey
-            | Error::UnsupportedSignatureAlgorithmForPublicKey => 15,
-            Error::UnsupportedCrlSignatureAlgorithm | Error::UnsupportedSignatureAlgorithm => 14,
-            Error::UnsupportedCriticalExtension => 13,
-            Error::UnsupportedCertVersion => 13,
-            Error::UnsupportedCrlVersion => 12,
-            Error::UnsupportedDeltaCrl => 11,
-            Error::UnsupportedIndirectCrl => 10,
-            Error::UnsupportedRevocationReason => 9,
-            Error::UnsupportedRevocationReasonsPartitioning => 8,
-            Error::UnsupportedCrlIssuingDistributionPoint => 7,
+            | Error::UnsupportedSignatureAlgorithmForPublicKey => 150,
+            Error::UnsupportedCrlSignatureAlgorithm | Error::UnsupportedSignatureAlgorithm => 140,
+            Error::UnsupportedCriticalExtension => 130,
+            Error::UnsupportedCertVersion => 130,
+            Error::UnsupportedCrlVersion => 120,
+            Error::UnsupportedDeltaCrl => 110,
+            Error::UnsupportedIndirectCrl => 100,
+            Error::UnsupportedRevocationReason => 90,
+            Error::UnsupportedRevocationReasonsPartitioning => 80,
+            Error::UnsupportedCrlIssuingDistributionPoint => 70,
+            Error::MaximumPathDepthExceeded => 61,
 
             // Errors related to malformed data.
-            Error::MalformedDnsIdentifier => 6,
-            Error::MalformedNameConstraint => 5,
-            Error::MalformedExtensions | Error::TrailingData(_) => 4,
-            Error::ExtensionValueInvalid => 3,
+            Error::MalformedDnsIdentifier => 60,
+            Error::MalformedNameConstraint => 50,
+            Error::MalformedExtensions | Error::TrailingData(_) => 40,
+            Error::ExtensionValueInvalid => 30,
 
             // Generic DER errors.
-            Error::BadDerTime => 2,
-            Error::BadDer => 1,
+            Error::BadDerTime => 20,
+            Error::BadDer => 10,
 
-            // Special case error - not subject to ranking.
+            // Special case errors - not subject to ranking.
             Error::MaximumSignatureChecksExceeded => 0,
+            Error::MaximumPathBuildCallsExceeded => 0,
 
             // Default catch all error - should be renamed in the future.
             Error::UnknownIssuer => 0,

src/verify_cert.rs

@@ -130,15 +130,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, &mut 0_usize)
+    build_chain_inner(opts, cert, time, 0, &mut Budget::default())
 }
 
 fn build_chain_inner(
     opts: &ChainOptions,
     cert: &Cert,
     time: time::Time,
     sub_ca_count: usize,
-    signatures: &mut usize,
+    budget: &mut Budget,
 ) -> Result<(), Error> {
     let used_as_ca = used_as_ca(&cert.ee_or_ca);
 
@@ -151,8 +151,7 @@ fn build_chain_inner(
             const MAX_SUB_CA_COUNT: usize = 6;
 
             if sub_ca_count >= MAX_SUB_CA_COUNT {
-                // TODO(XXX): Candidate for a more specific error - Error::PathTooDeep?
-                return Err(Error::UnknownIssuer);
+                return Err(Error::MaximumPathDepthExceeded);
             }
         }
         UsedAsCa::No => {
@@ -199,7 +198,7 @@ fn build_chain_inner(
                 cert,
                 trust_anchor,
                 opts.revocation,
-                signatures,
+                budget,
             )?;
 
             Ok(())
@@ -244,7 +243,8 @@ fn build_chain_inner(
             UsedAsCa::Yes => sub_ca_count + 1,
         };
 
-        build_chain_inner(opts, &potential_issuer, time, next_sub_ca_count, signatures)
+        budget.consume_build_chain_call()?;
+        build_chain_inner(opts, &potential_issuer, time, next_sub_ca_count, budget)
     })
 }
 
@@ -253,17 +253,14 @@ fn check_signatures(
     cert_chain: &Cert,
     trust_anchor: &TrustAnchor,
     revocation: Option<RevocationOptions>,
-    signatures: &mut usize,
+    budget: &mut Budget,
 ) -> Result<(), Error> {
     let mut spki_value = untrusted::Input::from(trust_anchor.subject_public_key_info.as_ref());
     let mut issuer_subject = untrusted::Input::from(trust_anchor.subject.as_ref());
     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);
-        }
+        budget.consume_signature()?;
         signed_data::verify_signed_data(supported_sig_algs, spki_value, &cert.signed_data)?;
 
         if let Some(revocation_opts) = &revocation {
@@ -293,6 +290,47 @@ fn check_signatures(
     Ok(())
 }
 
+struct Budget {
+    signatures: usize,
+    build_chain_calls: usize,
+}
+
+impl Budget {
+    #[inline]
+    fn consume_signature(&mut self) -> Result<(), Error> {
+        self.signatures = self
+            .signatures
+            .checked_sub(1)
+            .ok_or(Error::MaximumSignatureChecksExceeded)?;
+        Ok(())
+    }
+
+    #[inline]
+    fn consume_build_chain_call(&mut self) -> Result<(), Error> {
+        self.build_chain_calls = self
+            .build_chain_calls
+            .checked_sub(1)
+            .ok_or(Error::MaximumPathBuildCallsExceeded)?;
+        Ok(())
+    }
+}
+
+impl core::default::Default for Budget {
+    fn default() -> Self {
+        Self {
+            // This limit is taken from the remediation for golang CVE-2018-16875.  However,
+            // note that golang subsequently implemented AKID matching due to this limit
+            // 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: 200000,
+        }
+    }
+}
+
 // Zero-sized marker type representing positive assertion that revocation status was checked
 // for a certificate and the result was that the certificate is not revoked.
 struct CertNotRevoked(());
@@ -668,7 +706,8 @@ where
     for v in values {
         match f(v) {
             Ok(()) => return Ok(()),
-            err @ Err(Error::MaximumSignatureChecksExceeded) => return err,
+            err @ Err(Error::MaximumSignatureChecksExceeded)
+            | err @ Err(Error::MaximumPathBuildCallsExceeded) => return err,
             Err(new_error) => error = error.most_specific(new_error),
         }
     }
@@ -791,10 +830,17 @@ mod tests {
         assert!(crl_authoritative(&crl, &ee));
     }
 
-    #[test]
     #[cfg(feature = "alloc")]
-    fn test_too_many_signatures() {
-        use crate::types::CertificateDer;
+    enum TrustAnchorIsActualIssuer {
+        Yes,
+        No,
+    }
+
+    #[cfg(feature = "alloc")]
+    fn build_degenerate_chain(
+        intermediate_count: usize,
+        trust_anchor_is_actual_issuer: TrustAnchorIsActualIssuer,
+    ) -> Error {
         use crate::{extract_trust_anchor, ECDSA_P256_SHA256};
         use crate::{EndEntityCert, Time};
 
@@ -818,9 +864,9 @@ mod tests {
         let ca_cert = make_issuer();
         let ca_cert_der = CertificateDer::from(ca_cert.serialize_der().unwrap());
 
-        let mut intermediates = Vec::with_capacity(101);
+        let mut intermediates = Vec::with_capacity(intermediate_count);
         let mut issuer = ca_cert;
-        for _ in 0..101 {
+        for _ in 0..intermediate_count {
             let intermediate = make_issuer();
             let intermediate_der = intermediate.serialize_der_with_signer(&issuer).unwrap();
             intermediates.push(intermediate_der);
@@ -836,12 +882,16 @@ mod tests {
         let anchors = &[extract_trust_anchor(&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 = intermediates
+        let mut intermediates_der = intermediates
             .iter()
             .map(|x| CertificateDer::from(x.as_ref()))
             .collect::<Vec<_>>();
 
-        let result = build_chain(
+        if let TrustAnchorIsActualIssuer::No = trust_anchor_is_actual_issuer {
+            intermediates_der.pop();
+        }
+
+        build_chain(
             &ChainOptions {
                 eku: KeyUsage::server_auth(),
                 supported_sig_algs: &[ECDSA_P256_SHA256],
@@ -851,8 +901,104 @@ mod tests {
             },
             cert.inner(),
             time,
+        )
+        .unwrap_err()
+    }
+
+    #[test]
+    #[cfg(feature = "alloc")]
+    fn test_too_many_signatures() {
+        assert_eq!(
+            build_degenerate_chain(5, TrustAnchorIsActualIssuer::Yes),
+            Error::MaximumSignatureChecksExceeded
         );
+    }
+
+    #[test]
+    #[cfg(feature = "alloc")]
+    fn test_too_many_path_calls() {
+        assert_eq!(
+            build_degenerate_chain(10, TrustAnchorIsActualIssuer::No),
+            Error::MaximumPathBuildCallsExceeded
+        );
+    }
 
-        assert!(matches!(result, Err(Error::MaximumSignatureChecksExceeded)));
+    #[cfg(feature = "alloc")]
+    fn build_linear_chain(chain_length: usize) -> Result<(), Error> {
+        use crate::{extract_trust_anchor, ECDSA_P256_SHA256};
+        use crate::{EndEntityCert, Time};
+
+        let alg = &rcgen::PKCS_ECDSA_P256_SHA256;
+
+        let make_issuer = |index: usize| {
+            let mut ca_params = rcgen::CertificateParams::new(Vec::new());
+            ca_params.distinguished_name.push(
+                rcgen::DnType::OrganizationName,
+                format!("Bogus Subject {index}"),
+            );
+            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(chain_length);
+        let ca_cert_der = CertificateDer::from(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(i);
+            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 = CertificateDer::from(ee_cert.serialize_der_with_signer(&issuer).unwrap());
+
+        let anchors = &[extract_trust_anchor(&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 = intermediates
+            .iter()
+            .map(|x| CertificateDer::from(x.as_ref()))
+            .collect::<Vec<_>>();
+
+        build_chain(
+            &ChainOptions {
+                eku: KeyUsage::server_auth(),
+                supported_sig_algs: &[ECDSA_P256_SHA256],
+                trust_anchors: anchors,
+                intermediate_certs: &intermediates_der,
+                revocation: None,
+            },
+            cert.inner(),
+            time,
+        )
+    }
+
+    #[test]
+    #[cfg(feature = "alloc")]
+    fn longest_allowed_path() {
+        assert_eq!(build_linear_chain(1), Ok(()));
+        assert_eq!(build_linear_chain(2), Ok(()));
+        assert_eq!(build_linear_chain(3), Ok(()));
+        assert_eq!(build_linear_chain(4), Ok(()));
+        assert_eq!(build_linear_chain(5), Ok(()));
+        assert_eq!(build_linear_chain(6), Ok(()));
+    }
+
+    #[test]
+    #[cfg(feature = "alloc")]
+    fn path_too_long() {
+        assert_eq!(build_linear_chain(7), Err(Error::MaximumPathDepthExceeded));
     }
 }