Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add support for enforcing CRL expiration using nextUpdate field #227

Merged
merged 9 commits into from
May 16, 2024
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 @@
depth: RevocationCheckDepth,

status_policy: UnknownStatusPolicy,

expiration_policy: ExpirationPolicy,
}

impl<'a> RevocationOptionsBuilder<'a> {
Expand All @@ -50,6 +52,10 @@
/// 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 @@
crls,
depth: RevocationCheckDepth::Chain,
status_policy: UnknownStatusPolicy::Deny,
expiration_policy: ExpirationPolicy::Ignore,
})
}

Expand All @@ -76,12 +83,19 @@
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(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 @@
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 @@
(None, _) => return Err(Error::UnknownRevocationStatus),
};

if self.expiration_policy == ExpirationPolicy::Enforce {
jasperpatterson marked this conversation as resolved.
Show resolved Hide resolved
crl.check_expiration(time)?;

Check warning on line 148 in src/crl/mod.rs

View check run for this annotation

Codecov / codecov/patch

src/crl/mod.rs#L148

Added line #L148 was not covered by tests
}

// 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 @@
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 @@
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 @@
.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 @@
.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 @@
.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 @@
.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 @@
)
.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,

Check warning on line 148 in src/crl/types.rs

View check run for this annotation

Codecov / codecov/patch

src/crl/types.rs#L148

Added line #L148 was not covered by tests
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 @@
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 @@

/// 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 @@
.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 @@
// 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 @@
issuer,
revoked_certs,
issuing_distribution_point: None,
next_update,
};

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

use pki_types::CertificateDer;

use super::*;
Expand Down Expand Up @@ -1227,6 +1250,29 @@
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));
jasperpatterson marked this conversation as resolved.
Show resolved Hide resolved

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));
jasperpatterson marked this conversation as resolved.
Show resolved Hide resolved

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 @@ -74,7 +74,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 @@ -140,6 +140,7 @@ impl<'a, 'p: 'a> ChainOptions<'a, 'p> {
path: &PathNode<'_>,
trust_anchor: &TrustAnchor,
budget: &mut Budget,
time: UnixTime,
jasperpatterson marked this conversation as resolved.
Show resolved Hide resolved
) -> 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 @@ -160,6 +161,7 @@ impl<'a, 'p: 'a> ChainOptions<'a, 'p> {
issuer_key_usage,
self.supported_sig_algs,
budget,
time,
)?;
}

Expand Down
Loading