diff --git a/src/crl/mod.rs b/src/crl/mod.rs index d5a47cda..62cfe11d 100644 --- a/src/crl/mod.rs +++ b/src/crl/mod.rs @@ -130,13 +130,35 @@ 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; + } + + if !crl.has_crl_number() { + // From RFC 5280, Section 5.2.3: + // CRL issuers conforming to this profile MUST include this extension in all + // CRLs and MUST mark this extension as non-critical. + // We therefore skip this CRL as invalid. + continue; + } + + if best_crl + .is_none_or(|best| best.same_scope_as(crl) && crl.has_newer_crl_number_than(best)) + { + // Note that once we find a CRL with an applicable scope, we skip any CRLs with + // different, but also applicable scopes. See [`CertRevocationList::authoritative`] + // for scenarios when that would happen. + // TODO(amichalik): optionally check all the scopes. This would require O(n^2) + // algorithm in no-alloc scenario, I think. + 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. diff --git a/src/crl/types.rs b/src/crl/types.rs index 22522eab..7e857ea9 100644 --- a/src/crl/types.rs +++ b/src/crl/types.rs @@ -2,7 +2,7 @@ use alloc::collections::BTreeMap; #[cfg(feature = "alloc")] use alloc::vec::Vec; -use core::fmt::Debug; +use core::{cmp::Ordering, fmt::Debug}; use pki_types::{SignatureVerificationAlgorithm, UnixTime}; @@ -79,6 +79,22 @@ impl CertRevocationList<'_> { } } + pub(crate) fn same_scope_as(&self, other: &Self) -> bool { + self.issuer() == other.issuer() + && self.issuing_distribution_point() == other.issuing_distribution_point() + } + + pub(crate) fn has_crl_number(&self) -> bool { + self.crl_number().is_some() + } + + pub(crate) fn has_newer_crl_number_than(&self, other: &Self) -> bool { + match (self.crl_number(), other.crl_number()) { + (Some(left), Some(right)) => crl_number_cmp(left, right).is_gt(), + _ => false, + } + } + /// Returns true if the CRL can be considered authoritative for the given certificate. /// /// A CRL is considered authoritative for a certificate when: @@ -144,11 +160,7 @@ impl CertRevocationList<'_> { /// Checks the verification time is before the time in the CRL nextUpdate field. pub(crate) fn check_expiration(&self, time: UnixTime) -> Result<(), Error> { - let next_update = match self { - #[cfg(feature = "alloc")] - CertRevocationList::Owned(crl) => crl.next_update, - CertRevocationList::Borrowed(crl) => crl.next_update, - }; + let next_update = self.next_update(); if time >= next_update { return Err(Error::CrlExpired { time, next_update }); @@ -156,6 +168,28 @@ impl CertRevocationList<'_> { Ok(()) } + + fn next_update(&self) -> UnixTime { + match self { + #[cfg(feature = "alloc")] + CertRevocationList::Owned(crl) => crl.next_update, + CertRevocationList::Borrowed(crl) => crl.next_update, + } + } + + fn crl_number(&self) -> Option<&[u8]> { + match self { + #[cfg(feature = "alloc")] + CertRevocationList::Owned(crl) => crl.crl_number.as_deref(), + CertRevocationList::Borrowed(crl) => crl + .crl_number + .map(|crl_number| crl_number.as_slice_less_safe()), + } + } +} + +fn crl_number_cmp(left: &[u8], right: &[u8]) -> Ordering { + left.len().cmp(&right.len()).then_with(|| left.cmp(right)) } /// Owned representation of a RFC 5280[^1] profile Certificate Revocation List (CRL). @@ -175,6 +209,8 @@ pub struct OwnedCertRevocationList { signed_data: OwnedSignedData, next_update: UnixTime, + + crl_number: Option>, } #[cfg(feature = "alloc")] @@ -225,6 +261,8 @@ pub struct BorrowedCertRevocationList<'a> { revoked_certs: untrusted::Input<'a>, next_update: UnixTime, + + crl_number: Option>, } impl<'a> BorrowedCertRevocationList<'a> { @@ -263,6 +301,9 @@ impl<'a> BorrowedCertRevocationList<'a> { .map(|idp| idp.as_slice_less_safe().to_vec()), revoked_certs, next_update: self.next_update, + crl_number: self + .crl_number + .map(|crl_number| crl_number.as_slice_less_safe().to_vec()), }) } @@ -278,18 +319,16 @@ impl<'a> BorrowedCertRevocationList<'a> { // up to 20 octets. Conforming CRL issuers MUST NOT use CRLNumber // values longer than 20 octets. // - extension.value.read_all(Error::InvalidCrlNumber, |der| { - let crl_number = der::nonnegative_integer(der) - .map_err(|_| Error::InvalidCrlNumber)? - .as_slice_less_safe(); - if crl_number.len() <= 20 { + let crl_number = extension.value.read_all(Error::InvalidCrlNumber, |der| { + let crl_number = + der::nonnegative_integer(der).map_err(|_| Error::InvalidCrlNumber)?; + if crl_number.as_slice_less_safe().len() <= 20 { Ok(crl_number) } else { Err(Error::InvalidCrlNumber) } })?; - // We enforce the cRLNumber is sensible, but don't retain the value for use. - Ok(()) + set_extension_once(&mut self.crl_number, || Ok(crl_number)) } // id-ce-deltaCRLIndicator 2.5.29.27 - RFC 5280 §5.2.4 @@ -408,6 +447,7 @@ impl<'a> FromDer<'a> for BorrowedCertRevocationList<'a> { revoked_certs, issuing_distribution_point: None, next_update, + crl_number: None, }; // RFC 5280 §5.1.2.7: @@ -473,6 +513,7 @@ impl core::hash::Hash for BorrowedCertRevocationList<'_> { issuing_distribution_point, revoked_certs, next_update, + crl_number, } = self; signed_data.hash(state); issuer.as_slice_less_safe().hash(state); @@ -481,6 +522,9 @@ impl core::hash::Hash for BorrowedCertRevocationList<'_> { .hash(state); revoked_certs.as_slice_less_safe().hash(state); next_update.hash(state); + crl_number + .map(|crl_number| crl_number.as_slice_less_safe()) + .hash(state); } } diff --git a/tests/client_auth_revocation.rs b/tests/client_auth_revocation.rs index 8d4bc1c9..bfbcad65 100644 --- a/tests/client_auth_revocation.rs +++ b/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(¤t_not_revoked).unwrap(), + ); + let crls = &[&expired_not_revoked, ¤t_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(¤t_revoked).unwrap(), + ); + let crls = &[&expired_not_revoked, ¤t_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, + 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, not_after_secs: Option, +) -> 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, + 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]) -> Vec { } 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, }