diff --git a/src/cert.rs b/src/cert.rs index db02b57f..89f56545 100644 --- a/src/cert.rs +++ b/src/cert.rs @@ -22,7 +22,8 @@ use crate::public_values_eq; use crate::signed_data::SignedData; use crate::subject_name::{GeneralName, NameIterator, WildcardDnsNameRef}; use crate::x509::{ - DistributionPointName, Extension, ExtensionOid, remember_extension, set_extension_once, + DistributionPointName, Extension, ExtensionOid, UnknownExtensionPolicy, remember_extension, + set_extension_once, }; /// A parsed X509 certificate. @@ -50,7 +51,18 @@ pub struct Cert<'a> { } impl<'a> Cert<'a> { + pub(crate) fn for_trust_anchor(cert_der: untrusted::Input<'a>) -> Result { + Self::from_input(cert_der, UnknownExtensionPolicy::IgnoreCritical) + } + pub(crate) fn from_der(cert_der: untrusted::Input<'a>) -> Result { + Self::from_input(cert_der, UnknownExtensionPolicy::default()) + } + + fn from_input( + cert_der: untrusted::Input<'a>, + ext_policy: UnknownExtensionPolicy, + ) -> Result { let (tbs, signed_data) = cert_der.read_all(Error::TrailingData(DerTypeId::Certificate), |cert_der| { der::nested( @@ -152,6 +164,7 @@ impl<'a> Cert<'a> { remember_cert_extension( &mut cert, &Extension::from_der(extension)?, + ext_policy, ) }, ) @@ -295,6 +308,7 @@ pub(crate) fn lenient_certificate_serial_number<'a>( fn remember_cert_extension<'a>( cert: &mut Cert<'a>, extension: &Extension<'a>, + ext_policy: UnknownExtensionPolicy, ) -> Result<(), Error> { // We don't do anything with certificate policies so we can safely ignore // all policy-related stuff. We assume that the policy-related extensions @@ -302,7 +316,7 @@ fn remember_cert_extension<'a>( use ExtensionOid::*; - remember_extension(extension, |id| { + remember_extension(extension, ext_policy, |id| { let out = match id { // id-ce-keyUsage 2.5.29.15. Standard(15) => &mut cert.key_usage, @@ -326,7 +340,7 @@ fn remember_cert_extension<'a>( SignedCertificateTimestampList => &mut cert.scts, // Unsupported extension - _ => return extension.unsupported(), + _ => return extension.unsupported(ext_policy), }; set_extension_once(out, || { diff --git a/src/crl/types.rs b/src/crl/types.rs index a18d3145..0e6a698d 100644 --- a/src/crl/types.rs +++ b/src/crl/types.rs @@ -16,7 +16,10 @@ use crate::signed_data::OwnedSignedData; use crate::signed_data::SignedData; use crate::subject_name::GeneralName; use crate::verify_cert::{Budget, PathNode, Role}; -use crate::x509::{DistributionPointName, Extension, remember_extension, set_extension_once}; +use crate::x509::{ + DistributionPointName, Extension, UnknownExtensionPolicy, remember_extension, + set_extension_once, +}; /// A RFC 5280[^1] profile Certificate Revocation List (CRL). /// @@ -266,7 +269,7 @@ impl<'a> BorrowedCertRevocationList<'a> { fn remember_extension(&mut self, extension: &Extension<'a>) -> Result<(), Error> { use crate::x509::ExtensionOid::*; - remember_extension(extension, |id| { + remember_extension(extension, UnknownExtensionPolicy::default(), |id| { match id { // id-ce-cRLNumber 2.5.29.20 - RFC 5280 §5.2.3 Standard(20) => { @@ -304,7 +307,7 @@ impl<'a> BorrowedCertRevocationList<'a> { Standard(35) => Ok(()), // Unsupported extension - _ => extension.unsupported(), + _ => extension.unsupported(UnknownExtensionPolicy::default()), } }) } @@ -749,7 +752,7 @@ impl<'a> BorrowedRevokedCert<'a> { fn remember_extension(&mut self, extension: &Extension<'a>) -> Result<(), Error> { use crate::x509::ExtensionOid::*; - remember_extension(extension, |id| { + remember_extension(extension, UnknownExtensionPolicy::default(), |id| { match id { // id-ce-cRLReasons 2.5.29.21 - RFC 5280 §5.3.1. Standard(21) => { @@ -771,7 +774,7 @@ impl<'a> BorrowedRevokedCert<'a> { Standard(29) => Err(Error::UnsupportedIndirectCrl), // Unsupported extension - _ => extension.unsupported(), + _ => extension.unsupported(UnknownExtensionPolicy::default()), } }) } diff --git a/src/trust_anchor.rs b/src/trust_anchor.rs index 9aaf466f..cb9213bb 100644 --- a/src/trust_anchor.rs +++ b/src/trust_anchor.rs @@ -36,7 +36,7 @@ pub fn anchor_from_trusted_cert<'a>( // certificate using a special parser for v1 certificates. Notably, that // parser doesn't allow extensions, so there's no need to worry about // embedded name constraints in a v1 certificate. - match Cert::from_der(cert_der) { + match Cert::for_trust_anchor(cert_der) { Ok(cert) => Ok(TrustAnchor::from(cert)), Err(Error::UnsupportedCertVersion) => { extract_trust_anchor_from_v1_cert_der(cert_der).or(Err(Error::BadDer)) @@ -109,3 +109,41 @@ impl<'a> From> for TrustAnchor<'a> { fn skip(input: &mut untrusted::Reader<'_>, tag: der::Tag) -> Result<(), Error> { der::expect_tag(input, tag).map(|_| ()) } + +#[cfg(test)] +mod tests { + use rcgen::{CertificateParams, CustomExtension, KeyPair}; + + use super::*; + + // OID 1.2.3.4 -- not under the id-ce arc, so `ExtensionOid::lookup` returns `None`. + // This exercises the unknown-OID path in `remember_extension`. + #[test] + fn anchor_ignores_critical_extension_with_unknown_oid() { + let der = cert_with_critical_extension(&[1, 2, 3, 4]); + anchor_from_trusted_cert(&der) + .expect("critical extension with unknown OID should be ignored for trust anchors"); + } + + // OID 2.5.29.99 -- under the id-ce arc, so `ExtensionOid::lookup` returns + // `Some(Standard(99))`, but 99 isn't handled in `remember_cert_extension`. + // This exercises the unsupported-extension path in `remember_cert_extension`. + #[test] + fn anchor_ignores_critical_extension_with_unknown_id_ce_oid() { + let der = cert_with_critical_extension(&[2, 5, 29, 99]); + anchor_from_trusted_cert(&der).expect( + "critical id-ce extension with unknown OID should be ignored for trust anchors", + ); + } + + fn cert_with_critical_extension(oid: &[u64]) -> CertificateDer<'static> { + let mut params = CertificateParams::new(vec!["example.com".into()]).unwrap(); + let mut ext = CustomExtension::from_oid_content(oid, vec![1, 2]); + ext.set_criticality(true); + params.custom_extensions.push(ext); + + let key = KeyPair::generate_for(&rcgen::PKCS_ECDSA_P256_SHA256).unwrap(); + let cert = params.self_signed(&key).unwrap(); + CertificateDer::from(cert.der().to_vec()) + } +} diff --git a/src/x509.rs b/src/x509.rs index a626bd67..26bdc5be 100644 --- a/src/x509.rs +++ b/src/x509.rs @@ -23,10 +23,10 @@ pub(crate) struct Extension<'a> { } impl Extension<'_> { - pub(crate) fn unsupported(&self) -> Result<(), Error> { - match self.critical { - true => Err(Error::UnsupportedCriticalExtension), - false => Ok(()), + pub(crate) fn unsupported(&self, policy: UnknownExtensionPolicy) -> Result<(), Error> { + match (policy, self.critical) { + (UnknownExtensionPolicy::Strict, true) => Err(Error::UnsupportedCriticalExtension), + _ => Ok(()), } } } @@ -63,14 +63,22 @@ pub(crate) fn set_extension_once( pub(crate) fn remember_extension( extension: &Extension<'_>, + ext_policy: UnknownExtensionPolicy, mut handler: impl FnMut(ExtensionOid) -> Result<(), Error>, ) -> Result<(), Error> { match ExtensionOid::lookup(extension.id) { Some(oid) => handler(oid), - None => extension.unsupported(), + None => extension.unsupported(ext_policy), } } +#[derive(Clone, Copy, Debug, Default)] +pub(crate) enum UnknownExtensionPolicy { + #[default] + Strict, + IgnoreCritical, +} + /// A certificate revocation list (CRL) distribution point name, describing a source of /// CRL information for a given certificate as described in RFC 5280 section 4.2.3.13[^1]. ///