From 10506cd763fb8721c9485f1d84573006be610ddd Mon Sep 17 00:00:00 2001 From: Dirkjan Ochtman Date: Wed, 26 Jul 2023 23:46:49 +0200 Subject: [PATCH] Move more from_der() methods to trait impls --- src/crl.rs | 243 +++++++++++++++++++++++---------------------- src/signed_data.rs | 22 ++-- 2 files changed, 138 insertions(+), 127 deletions(-) diff --git a/src/crl.rs b/src/crl.rs index 040bd266..0d46d0e9 100644 --- a/src/crl.rs +++ b/src/crl.rs @@ -14,9 +14,9 @@ use crate::cert::lenient_certificate_serial_number; use crate::der::Tag; +use crate::der::{self, DerIterator, FromDer}; use crate::signed_data::{self, SignedData}; use crate::x509::{remember_extension, set_extension_once, Extension}; -use crate::der::{self, FromDer, DerIterator}; use crate::{Error, SignatureAlgorithm, Time}; #[cfg(feature = "alloc")] @@ -122,112 +122,8 @@ impl<'a> BorrowedCertRevocationList<'a> { /// /// [^1]: pub fn from_der(crl_der: &'a [u8]) -> Result { - // Try to parse the CRL. - let reader = untrusted::Input::from(crl_der); - let (tbs_cert_list, signed_data) = reader.read_all(Error::BadDer, |crl_der| { - der::nested_limited( - crl_der, - Tag::Sequence, - Error::BadDer, - |signed_der| SignedData::from_der(signed_der, der::MAX_DER_SIZE), - der::MAX_DER_SIZE, - ) - })?; - - let crl = tbs_cert_list.read_all(Error::BadDer, |tbs_cert_list| { - // RFC 5280 §5.1.2.1: - // This optional field describes the version of the encoded CRL. When - // extensions are used, as required by this profile, this field MUST be - // present and MUST specify version 2 (the integer value is 1). - // RFC 5280 §5.2: - // Conforming CRL issuers are REQUIRED to include the authority key - // identifier (Section 5.2.1) and the CRL number (Section 5.2.3) - // extensions in all CRLs issued. - // As a result of the above we parse this as a required section, not OPTIONAL. - // NOTE: Encoded value of version 2 is 1. - if der::small_nonnegative_integer(tbs_cert_list)? != 1 { - return Err(Error::UnsupportedCrlVersion); - } - - // RFC 5280 §5.1.2.2: - // This field MUST contain the same algorithm identifier as the - // signatureAlgorithm field in the sequence CertificateList - let signature = der::expect_tag_and_get_value(tbs_cert_list, Tag::Sequence)?; - if signature != signed_data.algorithm { - return Err(Error::SignatureAlgorithmMismatch); - } - - // RFC 5280 §5.1.2.3: - // The issuer field MUST contain a non-empty X.500 distinguished name (DN). - let issuer = der::expect_tag_and_get_value(tbs_cert_list, Tag::Sequence)?; - - // RFC 5280 §5.1.2.4: - // This field indicates the issue date of this CRL. thisUpdate may be - // encoded as UTCTime or GeneralizedTime. - // We do not presently enforce the correct choice of UTCTime or GeneralizedTime based on - // whether the date is post 2050. - der::time_choice(tbs_cert_list)?; - - // While OPTIONAL in the ASN.1 module, RFC 5280 §5.1.2.5 says: - // 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. - der::time_choice(tbs_cert_list)?; - - // RFC 5280 §5.1.2.6: - // When there are no revoked certificates, the revoked certificates list - // MUST be absent - // TODO(@cpu): Do we care to support empty CRLs if we don't support delta CRLs? - let revoked_certs = if tbs_cert_list.peek(Tag::Sequence.into()) { - der::expect_tag_and_get_value_limited( - tbs_cert_list, - Tag::Sequence, - der::MAX_DER_SIZE, - )? - } else { - untrusted::Input::from(&[]) - }; - - let mut crl = BorrowedCertRevocationList { - signed_data, - issuer, - revoked_certs, - }; - - // RFC 5280 §5.1.2.7: - // This field may only appear if the version is 2 (Section 5.1.2.1). If - // present, this field is a sequence of one or more CRL extensions. - // RFC 5280 §5.2: - // Conforming CRL issuers are REQUIRED to include the authority key - // identifier (Section 5.2.1) and the CRL number (Section 5.2.3) - // extensions in all CRLs issued. - // As a result of the above we parse this as a required section, not OPTIONAL. - der::nested( - tbs_cert_list, - Tag::ContextSpecificConstructed0, - Error::MalformedExtensions, - |tagged| { - der::nested_of_mut( - tagged, - Tag::Sequence, - Tag::Sequence, - Error::BadDer, - |extension| { - // RFC 5280 §5.2: - // If a CRL contains a critical extension - // that the application cannot process, then the application MUST NOT - // use that CRL to determine the status of certificates. However, - // applications may ignore unrecognized non-critical extensions. - crl.remember_extension(&Extension::parse(extension)?) - }, - ) - }, - )?; - - Ok(crl) - })?; - - Ok(crl) + let input = untrusted::Input::from(crl_der); + input.read_all(Error::BadDer, ::from_der) } /// Convert the CRL to an [`OwnedCertRevocationList`]. This may error if any of the revoked @@ -338,6 +234,123 @@ impl CertRevocationList for BorrowedCertRevocationList<'_> { } } +impl<'a> FromDer<'a> for BorrowedCertRevocationList<'a> { + /// Try to parse the given bytes as a RFC 5280[^1] profile Certificate Revocation List (CRL). + /// + /// Webpki does not support: + /// * CRL versions other than version 2. + /// * CRLs missing the next update field. + /// * CRLs missing certificate revocation list extensions. + /// * Delta CRLs. + /// * CRLs larger than (2^32)-1 bytes in size. + /// + /// [^1]: + fn from_der(reader: &mut untrusted::Reader<'a>) -> Result { + let (tbs_cert_list, signed_data) = der::nested_limited( + reader, + Tag::Sequence, + Error::BadDer, + |signed_der| SignedData::from_der(signed_der, der::MAX_DER_SIZE), + der::MAX_DER_SIZE, + )?; + + let crl = tbs_cert_list.read_all(Error::BadDer, |tbs_cert_list| { + // RFC 5280 §5.1.2.1: + // This optional field describes the version of the encoded CRL. When + // extensions are used, as required by this profile, this field MUST be + // present and MUST specify version 2 (the integer value is 1). + // RFC 5280 §5.2: + // Conforming CRL issuers are REQUIRED to include the authority key + // identifier (Section 5.2.1) and the CRL number (Section 5.2.3) + // extensions in all CRLs issued. + // As a result of the above we parse this as a required section, not OPTIONAL. + // NOTE: Encoded value of version 2 is 1. + if der::small_nonnegative_integer(tbs_cert_list)? != 1 { + return Err(Error::UnsupportedCrlVersion); + } + + // RFC 5280 §5.1.2.2: + // This field MUST contain the same algorithm identifier as the + // signatureAlgorithm field in the sequence CertificateList + let signature = der::expect_tag_and_get_value(tbs_cert_list, Tag::Sequence)?; + if signature != signed_data.algorithm { + return Err(Error::SignatureAlgorithmMismatch); + } + + // RFC 5280 §5.1.2.3: + // The issuer field MUST contain a non-empty X.500 distinguished name (DN). + let issuer = der::expect_tag_and_get_value(tbs_cert_list, Tag::Sequence)?; + + // RFC 5280 §5.1.2.4: + // This field indicates the issue date of this CRL. thisUpdate may be + // encoded as UTCTime or GeneralizedTime. + // We do not presently enforce the correct choice of UTCTime or GeneralizedTime based on + // whether the date is post 2050. + der::time_choice(tbs_cert_list)?; + + // While OPTIONAL in the ASN.1 module, RFC 5280 §5.1.2.5 says: + // 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. + der::time_choice(tbs_cert_list)?; + + // RFC 5280 §5.1.2.6: + // When there are no revoked certificates, the revoked certificates list + // MUST be absent + // TODO(@cpu): Do we care to support empty CRLs if we don't support delta CRLs? + let revoked_certs = if tbs_cert_list.peek(Tag::Sequence.into()) { + der::expect_tag_and_get_value_limited( + tbs_cert_list, + Tag::Sequence, + der::MAX_DER_SIZE, + )? + } else { + untrusted::Input::from(&[]) + }; + + let mut crl = BorrowedCertRevocationList { + signed_data, + issuer, + revoked_certs, + }; + + // RFC 5280 §5.1.2.7: + // This field may only appear if the version is 2 (Section 5.1.2.1). If + // present, this field is a sequence of one or more CRL extensions. + // RFC 5280 §5.2: + // Conforming CRL issuers are REQUIRED to include the authority key + // identifier (Section 5.2.1) and the CRL number (Section 5.2.3) + // extensions in all CRLs issued. + // As a result of the above we parse this as a required section, not OPTIONAL. + der::nested( + tbs_cert_list, + Tag::ContextSpecificConstructed0, + Error::MalformedExtensions, + |tagged| { + der::nested_of_mut( + tagged, + Tag::Sequence, + Tag::Sequence, + Error::BadDer, + |extension| { + // RFC 5280 §5.2: + // If a CRL contains a critical extension + // that the application cannot process, then the application MUST NOT + // use that CRL to determine the status of certificates. However, + // applications may ignore unrecognized non-critical extensions. + crl.remember_extension(&Extension::parse(extension)?) + }, + ) + }, + )?; + + Ok(crl) + })?; + + Ok(crl) + } +} + impl<'a> IntoIterator for &'a BorrowedCertRevocationList<'a> { type Item = Result, Error>; type IntoIter = DerIterator<'a, BorrowedRevokedCert<'a>>; @@ -429,7 +442,7 @@ impl<'a> BorrowedRevokedCert<'a> { match id { // id-ce-cRLReasons 2.5.29.21 - RFC 5280 §5.3.1. 21 => set_extension_once(&mut self.reason_code, || { - RevocationReason::from_der(extension.value) + RevocationReason::from_der(&mut untrusted::Reader::new(extension.value)) }), // id-ce-invalidityDate 2.5.29.24 - RFC 5280 §5.3.2. @@ -538,15 +551,13 @@ pub enum RevocationReason { AaCompromise = 10, } -impl RevocationReason { +impl<'a> FromDer<'a> for RevocationReason { // RFC 5280 §5.3.1. - fn from_der(value: untrusted::Input<'_>) -> Result { - value.read_all(Error::BadDer, |enumerated_reason| { - let value = der::expect_tag(enumerated_reason, Tag::Enum)?; - Self::try_from(value.value().read_all(Error::BadDer, |reason| { - reason.read_byte().map_err(|_| Error::BadDer) - })?) - }) + fn from_der(reader: &mut untrusted::Reader<'a>) -> Result { + let value = der::expect_tag(reader, Tag::Enum)?; + Self::try_from(value.value().read_all(Error::BadDer, |reason| { + reason.read_byte().map_err(|_| Error::BadDer) + })?) } } diff --git a/src/signed_data.rs b/src/signed_data.rs index 0b9856eb..6893ca55 100644 --- a/src/signed_data.rs +++ b/src/signed_data.rs @@ -12,9 +12,11 @@ // ACTION OF CONTRACT, NEGLIGENCE OR OTHER TORTIOUS ACTION, ARISING OUT OF // OR IN CONNECTION WITH THE USE OR PERFORMANCE OF THIS SOFTWARE. -use crate::{der, Error}; use ring::signature; +use crate::der::{self, FromDer}; +use crate::error::Error; + /// X.509 certificates and related items that are signed are almost always /// encoded in the format "tbs||signatureAlgorithm||signature". This structure /// captures this pattern as an owned data type. @@ -206,7 +208,7 @@ pub(crate) fn verify_signature( msg: untrusted::Input, signature: untrusted::Input, ) -> Result<(), Error> { - let spki = SubjectPublicKeyInfo::from_der(spki_value)?; + let spki = spki_value.read_all(Error::BadDer, SubjectPublicKeyInfo::from_der)?; if !signature_alg .public_key_alg_id .matches_algorithm_id_value(spki.algorithm_id_value) @@ -226,19 +228,17 @@ struct SubjectPublicKeyInfo<'a> { key_value: untrusted::Input<'a>, } -impl<'a> SubjectPublicKeyInfo<'a> { +impl<'a> FromDer<'a> for SubjectPublicKeyInfo<'a> { // Parse the public key into an algorithm OID, an optional curve OID, and the // key value. The caller needs to check whether these match the // `PublicKeyAlgorithm` for the `SignatureAlgorithm` that is matched when // parsing the signature. - fn from_der(input: untrusted::Input<'a>) -> Result { - input.read_all(Error::BadDer, |input| { - let algorithm_id_value = der::expect_tag_and_get_value(input, der::Tag::Sequence)?; - let key_value = der::bit_string_with_no_unused_bits(input)?; - Ok(SubjectPublicKeyInfo { - algorithm_id_value, - key_value, - }) + fn from_der(reader: &mut untrusted::Reader<'a>) -> Result { + let algorithm_id_value = der::expect_tag_and_get_value(reader, der::Tag::Sequence)?; + let key_value = der::bit_string_with_no_unused_bits(reader)?; + Ok(SubjectPublicKeyInfo { + algorithm_id_value, + key_value, }) } }