Skip to content

Commit

Permalink
Add support for enforcing CRL expiration using nextUpdate field
Browse files Browse the repository at this point in the history
  • Loading branch information
jasperpatterson authored and cpu committed May 16, 2024
1 parent fe294b3 commit 3f6eb0d
Show file tree
Hide file tree
Showing 4 changed files with 102 additions and 4 deletions.
48 changes: 47 additions & 1 deletion src/crl/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,7 @@
// ACTION OF CONTRACT, NEGLIGENCE OR OTHER TORTIOUS ACTION, ARISING OUT OF
// OR IN CONNECTION WITH THE USE OR PERFORMANCE OF THIS SOFTWARE.

use pki_types::SignatureVerificationAlgorithm;
use pki_types::{SignatureVerificationAlgorithm, UnixTime};

use crate::error::Error;
use crate::verify_cert::{Budget, PathNode, Role};
Expand All @@ -35,6 +35,8 @@ pub struct RevocationOptionsBuilder<'a> {
depth: RevocationCheckDepth,

status_policy: UnknownStatusPolicy,

expiration_policy: ExpirationPolicy,
}

impl<'a> RevocationOptionsBuilder<'a> {
Expand All @@ -50,6 +52,10 @@ impl<'a> RevocationOptionsBuilder<'a> {
/// By default revocation checking will fail if the revocation status of a certificate cannot
/// be determined. This can be customized using the
/// [RevocationOptionsBuilder::with_status_policy] method.
///
/// By default revocation checking will *not* fail if the verification time is beyond the time
/// in the CRL nextUpdate field. This can be customized using the
/// [RevocationOptionsBuilder::with_expiration_policy] method.
pub fn new(crls: &'a [&'a CertRevocationList<'a>]) -> Result<Self, CrlsRequired> {
if crls.is_empty() {
return Err(CrlsRequired(()));
Expand All @@ -59,6 +65,7 @@ impl<'a> RevocationOptionsBuilder<'a> {
crls,
depth: RevocationCheckDepth::Chain,
status_policy: UnknownStatusPolicy::Deny,
expiration_policy: ExpirationPolicy::Ignore,
})
}

Expand All @@ -76,12 +83,19 @@ impl<'a> RevocationOptionsBuilder<'a> {
self
}

/// Customize whether the CRL nextUpdate field (i.e. expiration) is enforced.
pub fn with_expiration_policy(mut self, policy: ExpirationPolicy) -> Self {
self.expiration_policy = policy;
self
}

/// Construct a [RevocationOptions] instance based on the builder's configuration.
pub fn build(self) -> RevocationOptions<'a> {
RevocationOptions {
crls: self.crls,
depth: self.depth,
status_policy: self.status_policy,
expiration_policy: self.expiration_policy,
}
}
}
Expand All @@ -93,6 +107,7 @@ pub struct RevocationOptions<'a> {
pub(crate) crls: &'a [&'a CertRevocationList<'a>],
pub(crate) depth: RevocationCheckDepth,
pub(crate) status_policy: UnknownStatusPolicy,
pub(crate) expiration_policy: ExpirationPolicy,
}

impl<'a> RevocationOptions<'a> {
Expand All @@ -104,6 +119,7 @@ impl<'a> RevocationOptions<'a> {
issuer_ku: Option<untrusted::Input>,
supported_sig_algs: &[&dyn SignatureVerificationAlgorithm],
budget: &mut Budget,
time: UnixTime,
) -> Result<Option<CertNotRevoked>, Error> {
assert!(public_values_eq(path.cert.issuer, issuer_subject));

Expand All @@ -128,6 +144,10 @@ impl<'a> RevocationOptions<'a> {
(None, _) => return Err(Error::UnknownRevocationStatus),
};

if self.expiration_policy == ExpirationPolicy::Enforce {
crl.check_expiration(time)?;
}

// Verify the CRL signature with the issuer SPKI.
// TODO(XXX): consider whether we can refactor so this happens once up-front, instead
// of per-lookup.
Expand Down Expand Up @@ -218,6 +238,16 @@ pub enum UnknownStatusPolicy {
Deny,
}

/// Describes how to handle the nextUpdate field of the CRL (i.e. expiration).
#[derive(Debug, Copy, Clone, PartialEq, Eq)]
pub enum ExpirationPolicy {
/// Enforce the verification time is before the time in the nextUpdate field.
/// Treats an expired CRL as an error condition yielding [Error::CrlExpired].
Enforce,
/// Ignore the CRL nextUpdate field.
Ignore,
}

// 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.
pub(crate) struct CertNotRevoked(());
Expand Down Expand Up @@ -269,6 +299,7 @@ mod tests {
let opts = builder.build();
assert_eq!(opts.depth, RevocationCheckDepth::Chain);
assert_eq!(opts.status_policy, UnknownStatusPolicy::Deny);
assert_eq!(opts.expiration_policy, ExpirationPolicy::Ignore);
assert_eq!(opts.crls.len(), 1);

// It should be possible to build a revocation options builder with custom depth.
Expand All @@ -278,6 +309,7 @@ mod tests {
.build();
assert_eq!(opts.depth, RevocationCheckDepth::EndEntity);
assert_eq!(opts.status_policy, UnknownStatusPolicy::Deny);
assert_eq!(opts.expiration_policy, ExpirationPolicy::Ignore);
assert_eq!(opts.crls.len(), 1);

// It should be possible to build a revocation options builder that allows unknown
Expand All @@ -288,6 +320,7 @@ mod tests {
.build();
assert_eq!(opts.depth, RevocationCheckDepth::Chain);
assert_eq!(opts.status_policy, UnknownStatusPolicy::Allow);
assert_eq!(opts.expiration_policy, ExpirationPolicy::Ignore);
assert_eq!(opts.crls.len(), 1);

// It should be possible to specify both depth and unknown status policy together.
Expand All @@ -298,6 +331,7 @@ mod tests {
.build();
assert_eq!(opts.depth, RevocationCheckDepth::EndEntity);
assert_eq!(opts.status_policy, UnknownStatusPolicy::Allow);
assert_eq!(opts.expiration_policy, ExpirationPolicy::Ignore);
assert_eq!(opts.crls.len(), 1);

// The same should be true for explicitly forbidding unknown status.
Expand All @@ -308,6 +342,18 @@ mod tests {
.build();
assert_eq!(opts.depth, RevocationCheckDepth::EndEntity);
assert_eq!(opts.status_policy, UnknownStatusPolicy::Deny);
assert_eq!(opts.expiration_policy, ExpirationPolicy::Ignore);
assert_eq!(opts.crls.len(), 1);

// It should be possible to build a revocation options builder that allows unknown
// revocation status.
let opts = RevocationOptionsBuilder::new(&crls)
.unwrap()
.with_expiration_policy(ExpirationPolicy::Enforce)
.build();
assert_eq!(opts.depth, RevocationCheckDepth::Chain);
assert_eq!(opts.status_policy, UnknownStatusPolicy::Deny);
assert_eq!(opts.expiration_policy, ExpirationPolicy::Enforce);
assert_eq!(opts.crls.len(), 1);

// Built revocation options should be debug and clone when alloc is enabled.
Expand Down
48 changes: 47 additions & 1 deletion src/crl/types.rs
Original file line number Diff line number Diff line change
Expand Up @@ -140,6 +140,21 @@ impl<'a> CertRevocationList<'a> {
)
.map_err(crl_signature_err)
}

/// 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,
};

if time >= next_update {
return Err(Error::CrlExpired);
}

Ok(())
}
}

/// Owned representation of a RFC 5280[^1] profile Certificate Revocation List (CRL).
Expand All @@ -157,6 +172,8 @@ pub struct OwnedCertRevocationList {
issuing_distribution_point: Option<Vec<u8>>,

signed_data: signed_data::OwnedSignedData,

next_update: UnixTime,
}

#[cfg(feature = "alloc")]
Expand Down Expand Up @@ -205,6 +222,8 @@ pub struct BorrowedCertRevocationList<'a> {

/// List of certificates revoked by the issuer in this CRL.
revoked_certs: untrusted::Input<'a>,

next_update: UnixTime,
}

impl<'a> BorrowedCertRevocationList<'a> {
Expand Down Expand Up @@ -242,6 +261,7 @@ impl<'a> BorrowedCertRevocationList<'a> {
.issuing_distribution_point
.map(|idp| idp.as_slice_less_safe().to_vec()),
revoked_certs,
next_update: self.next_update,
})
}

Expand Down Expand Up @@ -363,7 +383,7 @@ impl<'a> FromDer<'a> for BorrowedCertRevocationList<'a> {
// Conforming CRL issuers MUST include the nextUpdate field in all CRLs.
// We do not presently enforce the correct choice of UTCTime or GeneralizedTime based on
// whether the date is post 2050.
UnixTime::from_der(tbs_cert_list)?;
let next_update = UnixTime::from_der(tbs_cert_list)?;

// RFC 5280 §5.1.2.6:
// When there are no revoked certificates, the revoked certificates list
Expand All @@ -384,6 +404,7 @@ impl<'a> FromDer<'a> for BorrowedCertRevocationList<'a> {
issuer,
revoked_certs,
issuing_distribution_point: None,
next_update,
};

// RFC 5280 §5.1.2.7:
Expand Down Expand Up @@ -899,6 +920,8 @@ impl TryFrom<u8> for RevocationReason {
#[cfg(feature = "alloc")]
#[cfg(test)]
mod tests {
use std::time::Duration;

use pki_types::CertificateDer;
use std::prelude::v1::*;
use std::println;
Expand Down Expand Up @@ -1229,6 +1252,29 @@ mod tests {
assert!(crl.authoritative(&path.node()));
}

#[test]
fn test_crl_expired() {
let crl = include_bytes!("../../tests/crls/crl.valid.der");
let crl: CertRevocationList = BorrowedCertRevocationList::from_der(&crl[..])
.unwrap()
.into();
let time = UnixTime::since_unix_epoch(Duration::from_secs(1706905579));

assert!(matches!(crl.check_expiration(time), Err(Error::CrlExpired)));
}

#[test]
fn test_crl_not_expired() {
let crl = include_bytes!("../../tests/crls/crl.valid.der");
let crl: CertRevocationList = BorrowedCertRevocationList::from_der(&crl[..])
.unwrap()
.into();
let expiration_time = 1666210326;
let time = UnixTime::since_unix_epoch(Duration::from_secs(expiration_time - 1000));

assert!(matches!(crl.check_expiration(time), Ok(())));
}

#[test]
fn test_construct_owned_crl() {
// It should be possible to construct an owned CRL directly from DER without needing
Expand Down
6 changes: 5 additions & 1 deletion src/error.rs
Original file line number Diff line number Diff line change
Expand Up @@ -42,6 +42,10 @@ pub enum Error {
/// The certificate, or one of its issuers, has been revoked.
CertRevoked,

/// The CRL is expired; i.e. the verification time is not before the time
/// in the CRL nextUpdate field.
CrlExpired,

/// An end-entity certificate is being used as a CA certificate.
EndEntityUsedAsCa,

Expand Down Expand Up @@ -213,7 +217,7 @@ impl Error {
// Errors related to certificate validity
Error::CertNotValidYet | Error::CertExpired => 290,
Error::CertNotValidForName => 280,
Error::CertRevoked | Error::UnknownRevocationStatus => 270,
Error::CertRevoked | Error::UnknownRevocationStatus | Error::CrlExpired => 270,
Error::InvalidCrlSignatureForPublicKey | Error::InvalidSignatureForPublicKey => 260,
Error::SignatureAlgorithmMismatch => 250,
Error::RequiredEkuNotFound => 240,
Expand Down
4 changes: 3 additions & 1 deletion src/verify_cert.rs
Original file line number Diff line number Diff line change
Expand Up @@ -73,7 +73,7 @@ impl<'a, 'p: 'a> ChainOptions<'a, 'p> {
// TODO: check_distrust(trust_anchor_subject, trust_anchor_spki)?;

let node = path.node();
self.check_signed_chain(&node, trust_anchor, budget)?;
self.check_signed_chain(&node, trust_anchor, budget, time)?;
check_signed_chain_name_constraints(&node, trust_anchor, budget)?;

let verify = match verify_path {
Expand Down Expand Up @@ -139,6 +139,7 @@ impl<'a, 'p: 'a> ChainOptions<'a, 'p> {
path: &PathNode<'_>,
trust_anchor: &TrustAnchor,
budget: &mut Budget,
time: UnixTime,
) -> Result<(), ControlFlow<Error, 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());
Expand All @@ -159,6 +160,7 @@ impl<'a, 'p: 'a> ChainOptions<'a, 'p> {
issuer_key_usage,
self.supported_sig_algs,
budget,
time,
)?;
}

Expand Down

0 comments on commit 3f6eb0d

Please sign in to comment.