Select CRLs by cRLNumber

Choose the newest authoritative CRL within the same issuer/IDP scope before checking revocation status.

Author: xyzzyz

src/crl/mod.rs

@@ -130,13 +130,31 @@ impl RevocationOptions<'_> {
             return Ok(None);
         }
 
-        let crl = self
-            .crls
-            .iter()
-            .find(|candidate_crl| candidate_crl.authoritative(path));
+        let mut best_crl: Option<&CertRevocationList<'_>> = None;
+
+        for crl in self.crls.iter().copied() {
+            if !crl.authoritative(path) {
+                continue;
+            }
+
+            let best_so_far = best_crl.get_or_insert(crl);
+
+            // Now check if `crl` supersedes `best_so_far`.
+            // First, check if the scope is the same
+            if crl.issuer() != best_so_far.issuer()
+                || crl.issuing_distribution_point() != best_so_far.issuing_distribution_point()
+            {
+                continue;
+            }
+
+            // Then, check if it has newer CRL number.
+            if crl.crl_number() > best_so_far.crl_number() {
+                best_crl = Some(crl);
+            }
+        }
 
         use UnknownStatusPolicy::*;
-        let crl = match (crl, self.status_policy) {
+        let crl = match (best_crl, self.status_policy) {
             (Some(crl), _) => crl,
             // If the policy allows unknown, return Ok(None) to indicate that the certificate
             // was not confirmed as CertNotRevoked, but that this isn't an error condition.

src/crl/types.rs

@@ -2,6 +2,7 @@
 use alloc::collections::BTreeMap;
 #[cfg(feature = "alloc")]
 use alloc::vec::Vec;
+use core::cmp::Ordering;
 use core::fmt::Debug;
 
 use pki_types::{SignatureVerificationAlgorithm, UnixTime};
@@ -156,6 +157,36 @@ impl CertRevocationList<'_> {
 
         Ok(())
     }
+
+    pub(crate) fn crl_number(&self) -> CrlNumber<'_> {
+        match self {
+            #[cfg(feature = "alloc")]
+            CertRevocationList::Owned(crl) => CrlNumber {
+                value: &crl.crl_number,
+            },
+            CertRevocationList::Borrowed(crl) => CrlNumber {
+                value: crl.crl_number.as_slice_less_safe(),
+            },
+        }
+    }
+}
+
+#[derive(Debug, Eq, PartialEq)]
+pub(crate) struct CrlNumber<'a> {
+    /// Parsed nonnegative INTEGER bytes.
+    /// It has no DER sign padding, and no unnecessary leading zeroes.
+    value: &'a [u8],
+}
+
+impl PartialOrd for CrlNumber<'_> {
+    fn partial_cmp(&self, other: &Self) -> Option<Ordering> {
+        Some(
+            self.value
+                .len()
+                .cmp(&other.value.len())
+                .then_with(|| self.value.cmp(other.value)),
+        )
+    }
 }
 
 /// Owned representation of a RFC 5280[^1] profile Certificate Revocation List (CRL).

tests/client_auth_revocation.rs

@@ -26,9 +26,9 @@ use rcgen::{
 #[cfg(feature = "alloc")]
 use webpki::OwnedCertRevocationList;
 use webpki::{
-    BorrowedCertRevocationList, CertRevocationList, ExtendedKeyUsage, PathBuilder,
-    RevocationCheckDepth, RevocationOptions, RevocationOptionsBuilder, UnknownStatusPolicy,
-    anchor_from_trusted_cert,
+    BorrowedCertRevocationList, CertRevocationList, ExpirationPolicy, ExtendedKeyUsage,
+    PathBuilder, RevocationCheckDepth, RevocationOptions, RevocationOptionsBuilder,
+    UnknownStatusPolicy, anchor_from_trusted_cert,
 };
 use x509_parser::oid_registry;
 
@@ -990,7 +990,7 @@ fn ee_revoked_expired_crl() {
         .unwrap()
         .with_depth(RevocationCheckDepth::EndEntity)
         .with_status_policy(UnknownStatusPolicy::Allow)
-        .with_expiration_policy(webpki::ExpirationPolicy::Enforce)
+        .with_expiration_policy(ExpirationPolicy::Enforce)
         .build();
 
     assert!(matches!(
@@ -1004,6 +1004,180 @@ fn ee_revoked_expired_crl() {
     ));
 }
 
+#[test]
+fn expired_crl_does_not_shadow_current_crl_when_enforcing_expiration() {
+    let chain = CertChain::no_key_usages("expired_first_enforce");
+    let expired_not_revoked = chain.int_a.generate_crl_with_updates_and_number(
+        SerialNumber::from(0xFFFF),
+        None,
+        0x1FED_F00D - 120,
+        0x1FED_F00D - 60,
+        1,
+    );
+    let current_not_revoked = chain.int_a.generate_crl_with_updates_and_number(
+        SerialNumber::from(0xFFFF),
+        None,
+        0x1FED_F00D - 60,
+        0x1FED_F00D + 60,
+        2,
+    );
+
+    let expired_not_revoked = CertRevocationList::Borrowed(
+        BorrowedCertRevocationList::from_der(&expired_not_revoked).unwrap(),
+    );
+    let current_not_revoked = CertRevocationList::Borrowed(
+        BorrowedCertRevocationList::from_der(&current_not_revoked).unwrap(),
+    );
+    let crls = &[&expired_not_revoked, &current_not_revoked];
+
+    let revocation = RevocationOptionsBuilder::new(crls)
+        .unwrap()
+        .with_depth(RevocationCheckDepth::EndEntity)
+        .with_status_policy(UnknownStatusPolicy::Allow)
+        .with_expiration_policy(ExpirationPolicy::Enforce)
+        .build();
+
+    assert_eq!(
+        check_cert(
+            chain.ee_cert.der(),
+            &chain.intermediates(),
+            chain.root.der(),
+            Some(revocation),
+        ),
+        Ok(())
+    );
+}
+
+#[test]
+fn expired_crl_does_not_shadow_newer_revocation_when_ignoring_expiration() {
+    let chain = CertChain::no_key_usages("expired_first_ignore");
+    let expired_not_revoked = chain.int_a.generate_crl_with_updates_and_number(
+        SerialNumber::from(0xFFFF),
+        None,
+        0x1FED_F00D - 120,
+        0x1FED_F00D - 60,
+        1,
+    );
+    let current_revoked = chain.int_a.generate_crl_with_updates_and_number(
+        chain.ee_serial.clone(),
+        None,
+        0x1FED_F00D - 60,
+        0x1FED_F00D + 60,
+        2,
+    );
+
+    let expired_not_revoked = CertRevocationList::Borrowed(
+        BorrowedCertRevocationList::from_der(&expired_not_revoked).unwrap(),
+    );
+    let current_revoked = CertRevocationList::Borrowed(
+        BorrowedCertRevocationList::from_der(&current_revoked).unwrap(),
+    );
+    let crls = &[&expired_not_revoked, &current_revoked];
+
+    let revocation = RevocationOptionsBuilder::new(crls)
+        .unwrap()
+        .with_depth(RevocationCheckDepth::EndEntity)
+        .with_status_policy(UnknownStatusPolicy::Allow)
+        .with_expiration_policy(ExpirationPolicy::Ignore)
+        .build();
+
+    assert_eq!(
+        check_cert(
+            chain.ee_cert.der(),
+            &chain.intermediates(),
+            chain.root.der(),
+            Some(revocation),
+        ),
+        Err(webpki::Error::CertRevoked)
+    );
+}
+
+#[test]
+fn crl_number_in_other_partition_does_not_shadow_revoked_partition() {
+    let chain = CertChain::with_crl_dps("partitioned_crl_order", vec![MATCHING_URI.to_string()]);
+    let other_partition_not_revoked = chain.int_a.generate_crl_with_updates_and_number(
+        SerialNumber::from(0xFFFF),
+        Some(idp(NON_MATCHING_URI)),
+        NOT_BEFORE_SECS,
+        NOT_AFTER_SECS,
+        100,
+    );
+    let revoked_partition = chain.int_a.generate_crl_with_updates_and_number(
+        chain.ee_serial.clone(),
+        Some(idp(MATCHING_URI)),
+        NOT_BEFORE_SECS,
+        NOT_AFTER_SECS,
+        1,
+    );
+
+    let other_partition_not_revoked = CertRevocationList::Borrowed(
+        BorrowedCertRevocationList::from_der(&other_partition_not_revoked).unwrap(),
+    );
+    let revoked_partition = CertRevocationList::Borrowed(
+        BorrowedCertRevocationList::from_der(&revoked_partition).unwrap(),
+    );
+    let crls = &[&other_partition_not_revoked, &revoked_partition];
+
+    let revocation = RevocationOptionsBuilder::new(crls)
+        .unwrap()
+        .with_depth(RevocationCheckDepth::EndEntity)
+        .with_status_policy(UnknownStatusPolicy::Allow)
+        .build();
+
+    assert_eq!(
+        check_cert(
+            chain.ee_cert.der(),
+            &chain.intermediates(),
+            chain.root.der(),
+            Some(revocation),
+        ),
+        Err(webpki::Error::CertRevoked)
+    );
+}
+
+#[test]
+fn crl_number_order_uses_integer_value_not_lexicographic_bytes() {
+    let chain = CertChain::no_key_usages("crl_number_order");
+    let crl_255_not_revoked = chain.int_a.generate_crl_with_updates_and_number(
+        SerialNumber::from(0xFFFF),
+        None,
+        NOT_BEFORE_SECS,
+        NOT_AFTER_SECS,
+        0xFF,
+    );
+    let crl_256_revoked = chain.int_a.generate_crl_with_updates_and_number(
+        chain.ee_serial.clone(),
+        None,
+        NOT_BEFORE_SECS,
+        NOT_AFTER_SECS,
+        0x0100,
+    );
+
+    let crl_255_not_revoked = CertRevocationList::Borrowed(
+        BorrowedCertRevocationList::from_der(&crl_255_not_revoked).unwrap(),
+    );
+    let crl_256_revoked = CertRevocationList::Borrowed(
+        BorrowedCertRevocationList::from_der(&crl_256_revoked).unwrap(),
+    );
+    let crls = &[&crl_255_not_revoked, &crl_256_revoked];
+
+    let revocation = RevocationOptionsBuilder::new(crls)
+        .unwrap()
+        .with_depth(RevocationCheckDepth::EndEntity)
+        .with_status_policy(UnknownStatusPolicy::Allow)
+        .build();
+
+    assert_eq!(
+        check_cert(
+            chain.ee_cert.der(),
+            &chain.intermediates(),
+            chain.root.der(),
+            Some(revocation),
+        ),
+        Err(webpki::Error::CertRevoked)
+    );
+}
+
 // Cert has two normal DPs; first doesn't match IDP, second does.
 // Proves the outer cert_dp loop continues to the next DP when URIs don't match.
 #[test]
@@ -1312,6 +1486,26 @@ impl Intermediate {
             .into()
     }
 
+    fn generate_crl_with_updates_and_number(
+        &self,
+        serial: SerialNumber,
+        issuing_dp: Option<CrlIssuingDistributionPoint>,
+        this_update_secs: u64,
+        next_update_secs: u64,
+        crl_number: u64,
+    ) -> CertificateRevocationListDer<'static> {
+        crl_params_with_updates_and_number(
+            serial,
+            issuing_dp,
+            this_update_secs,
+            next_update_secs,
+            crl_number,
+        )
+        .signed_by(&self.issuer)
+        .unwrap()
+        .into()
+    }
+
     fn generate_crl_with_crl_sign(
         &self,
         serial: SerialNumber,
@@ -1336,12 +1530,27 @@ fn crl_params(
     serial_number: SerialNumber,
     issuing_distribution_point: Option<CrlIssuingDistributionPoint>,
     not_after_secs: Option<u64>,
+) -> CertificateRevocationListParams {
+    crl_params_with_updates_and_number(
+        serial_number,
+        issuing_distribution_point,
+        NOT_BEFORE_SECS,
+        not_after_secs.unwrap_or(NOT_AFTER_SECS),
+        1234,
+    )
+}
+
+fn crl_params_with_updates_and_number(
+    serial_number: SerialNumber,
+    issuing_distribution_point: Option<CrlIssuingDistributionPoint>,
+    this_update_secs: u64,
+    next_update_secs: u64,
+    crl_number: u64,
 ) -> CertificateRevocationListParams {
     CertificateRevocationListParams {
-        this_update: date_time_ymd(1970, 1, 1) + Duration::from_secs(NOT_BEFORE_SECS),
-        next_update: date_time_ymd(1970, 1, 1)
-            + Duration::from_secs(not_after_secs.unwrap_or(NOT_AFTER_SECS)),
-        crl_number: SerialNumber::from(1234),
+        this_update: date_time_ymd(1970, 1, 1) + Duration::from_secs(this_update_secs),
+        next_update: date_time_ymd(1970, 1, 1) + Duration::from_secs(next_update_secs),
+        crl_number: SerialNumber::from(crl_number),
         issuing_distribution_point,
         key_identifier_method: KeyIdMethod::Sha256,
         revoked_certs: vec![RevokedCertParams {
@@ -1516,9 +1725,13 @@ fn build_crl_dps_extension(dps: &[Vec<u8>]) -> Vec<u8> {
 }
 
 fn matching_idp() -> CrlIssuingDistributionPoint {
+    idp(MATCHING_URI)
+}
+
+fn idp(uri: &str) -> CrlIssuingDistributionPoint {
     CrlIssuingDistributionPoint {
         distribution_point: CrlDistributionPoint {
-            uris: vec![MATCHING_URI.to_string()],
+            uris: vec![uri.to_string()],
         },
         scope: None,
     }