diff --git a/src/cert.rs b/src/cert.rs index 27c0cbe0..dc8cabec 100644 --- a/src/cert.rs +++ b/src/cert.rs @@ -56,14 +56,20 @@ impl<'a> Cert<'a> { cert_der: untrusted::Input<'a>, ee_or_ca: EndEntityOrCa<'a>, ) -> Result { - let (tbs, signed_data) = cert_der.read_all(Error::BadDer, |cert_der| { - der::nested(cert_der, der::Tag::Sequence, Error::BadDer, |der| { - // limited to SEQUENCEs of size 2^16 or less. - SignedData::from_der(der, der::TWO_BYTE_DER_SIZE) - }) - })?; + let (tbs, signed_data) = + cert_der.read_all(Error::TrailingData("Cert::wrapper"), |cert_der| { + der::nested( + cert_der, + der::Tag::Sequence, + Error::TrailingData("Cert::wrapper"), + |der| { + // limited to SEQUENCEs of size 2^16 or less. + SignedData::from_der(der, der::TWO_BYTE_DER_SIZE) + }, + ) + })?; - tbs.read_all(Error::BadDer, |tbs| { + tbs.read_all(Error::TrailingData("TBS"), |tbs| { version3(tbs)?; let serial = lenient_certificate_serial_number(tbs)?; @@ -108,13 +114,13 @@ impl<'a> Cert<'a> { der::nested( tbs, der::Tag::ContextSpecificConstructed3, - Error::MalformedExtensions, + Error::TrailingData("Cert::extensions"), |tagged| { der::nested_of_mut( tagged, der::Tag::Sequence, der::Tag::Sequence, - Error::BadDer, + Error::TrailingData("Extension"), |extension| { remember_cert_extension(&mut cert, &Extension::from_der(extension)?) }, @@ -273,34 +279,41 @@ impl<'a> FromDer<'a> for CrlDistributionPoint<'a> { crl_issuer: None, }; - der::nested(reader, Tag::Sequence, Error::BadDer, |der| { - const DISTRIBUTION_POINT_TAG: u8 = CONTEXT_SPECIFIC | CONSTRUCTED; - const REASONS_TAG: u8 = CONTEXT_SPECIFIC | 1; - const CRL_ISSUER_TAG: u8 = CONTEXT_SPECIFIC | CONSTRUCTED | 2; - - while !der.at_end() { - let (tag, value) = der::read_tag_and_get_value(der)?; - match tag { - DISTRIBUTION_POINT_TAG => { - set_extension_once(&mut result.distribution_point, || Ok(value))? - } - REASONS_TAG => { - set_extension_once(&mut result.reasons, || der::bit_string_flags(value))? + der::nested( + reader, + Tag::Sequence, + Error::TrailingData(Self::NAME), + |der| { + const DISTRIBUTION_POINT_TAG: u8 = CONTEXT_SPECIFIC | CONSTRUCTED; + const REASONS_TAG: u8 = CONTEXT_SPECIFIC | 1; + const CRL_ISSUER_TAG: u8 = CONTEXT_SPECIFIC | CONSTRUCTED | 2; + + while !der.at_end() { + let (tag, value) = der::read_tag_and_get_value(der)?; + match tag { + DISTRIBUTION_POINT_TAG => { + set_extension_once(&mut result.distribution_point, || Ok(value))? + } + REASONS_TAG => set_extension_once(&mut result.reasons, || { + der::bit_string_flags(value) + })?, + CRL_ISSUER_TAG => set_extension_once(&mut result.crl_issuer, || Ok(value))?, + _ => return Err(Error::BadDer), } - CRL_ISSUER_TAG => set_extension_once(&mut result.crl_issuer, || Ok(value))?, - _ => return Err(Error::BadDer), } - } - // RFC 5280 section §4.2.1.13: - // a DistributionPoint MUST NOT consist of only the reasons field; either distributionPoint or - // cRLIssuer MUST be present. - match (result.distribution_point, result.crl_issuer) { - (None, None) => Err(Error::MalformedExtensions), - _ => Ok(result), - } - }) + // RFC 5280 section §4.2.1.13: + // a DistributionPoint MUST NOT consist of only the reasons field; either distributionPoint or + // cRLIssuer MUST be present. + match (result.distribution_point, result.crl_issuer) { + (None, None) => Err(Error::MalformedExtensions), + _ => Ok(result), + } + }, + ) } + + const NAME: &'static str = "CrlDistributionPoint"; } #[cfg(test)] diff --git a/src/crl.rs b/src/crl.rs index be6f0a66..028bef2e 100644 --- a/src/crl.rs +++ b/src/crl.rs @@ -260,7 +260,7 @@ impl<'a> FromDer<'a> for BorrowedCertRevocationList<'a> { let (tbs_cert_list, signed_data) = der::nested_limited( reader, Tag::Sequence, - Error::BadDer, + Error::TrailingData(Self::NAME), |signed_der| SignedData::from_der(signed_der, der::MAX_DER_SIZE), der::MAX_DER_SIZE, )?; @@ -343,7 +343,7 @@ impl<'a> FromDer<'a> for BorrowedCertRevocationList<'a> { tagged, Tag::Sequence, Tag::Sequence, - Error::BadDer, + Error::TrailingData("CrlExtension"), |extension| { // RFC 5280 §5.2: // If a CRL contains a critical extension @@ -367,6 +367,8 @@ impl<'a> FromDer<'a> for BorrowedCertRevocationList<'a> { Ok(crl) } + + const NAME: &'static str = "BorrowedCertRevocationList"; } impl<'a> IntoIterator for &'a BorrowedCertRevocationList<'a> { @@ -484,64 +486,76 @@ impl<'a> BorrowedRevokedCert<'a> { impl<'a> FromDer<'a> for BorrowedRevokedCert<'a> { fn from_der(reader: &mut untrusted::Reader<'a>) -> Result { - der::nested(reader, Tag::Sequence, Error::BadDer, |der| { - // RFC 5280 §4.1.2.2: - // Certificate users MUST be able to handle serialNumber values up to 20 octets. - // Conforming CAs MUST NOT use serialNumber values longer than 20 octets. - // - // Note: Non-conforming CAs may issue certificates with serial numbers - // that are negative or zero. Certificate users SHOULD be prepared to - // gracefully handle such certificates. - // Like the handling in cert.rs we choose to be lenient here, not enforcing the length - // of a CRL revoked certificate's serial number is less than 20 octets in encoded form. - let serial_number = lenient_certificate_serial_number(der) - .map_err(|_| Error::InvalidSerialNumber)? - .as_slice_less_safe(); - - let revocation_date = Time::from_der(der)?; - - let mut revoked_cert = BorrowedRevokedCert { - serial_number, - revocation_date, - reason_code: None, - invalidity_date: None, - }; - - // RFC 5280 §5.3: - // Support for the CRL entry extensions defined in this specification is - // optional for conforming CRL issuers and applications. However, CRL - // issuers SHOULD include reason codes (Section 5.3.1) and invalidity - // dates (Section 5.3.2) whenever this information is available. - if der.at_end() { - return Ok(revoked_cert); - } + der::nested( + reader, + Tag::Sequence, + Error::TrailingData("BorrowedRevokedCert::outer"), + |der| { + // RFC 5280 §4.1.2.2: + // Certificate users MUST be able to handle serialNumber values up to 20 octets. + // Conforming CAs MUST NOT use serialNumber values longer than 20 octets. + // + // Note: Non-conforming CAs may issue certificates with serial numbers + // that are negative or zero. Certificate users SHOULD be prepared to + // gracefully handle such certificates. + // Like the handling in cert.rs we choose to be lenient here, not enforcing the length + // of a CRL revoked certificate's serial number is less than 20 octets in encoded form. + let serial_number = lenient_certificate_serial_number(der) + .map_err(|_| Error::InvalidSerialNumber)? + .as_slice_less_safe(); + + let revocation_date = Time::from_der(der)?; + + let mut revoked_cert = BorrowedRevokedCert { + serial_number, + revocation_date, + reason_code: None, + invalidity_date: None, + }; + + // RFC 5280 §5.3: + // Support for the CRL entry extensions defined in this specification is + // optional for conforming CRL issuers and applications. However, CRL + // issuers SHOULD include reason codes (Section 5.3.1) and invalidity + // dates (Section 5.3.2) whenever this information is available. + if der.at_end() { + return Ok(revoked_cert); + } - // It would be convenient to use der::nested_of_mut here to unpack a SEQUENCE of one or - // more SEQUENCEs, however CAs have been mis-encoding the absence of extensions as an - // empty SEQUENCE so we must be tolerant of that. - let ext_seq = der::expect_tag(der, Tag::Sequence)?; - if ext_seq.is_empty() { - return Ok(revoked_cert); - } + // It would be convenient to use der::nested_of_mut here to unpack a SEQUENCE of one or + // more SEQUENCEs, however CAs have been mis-encoding the absence of extensions as an + // empty SEQUENCE so we must be tolerant of that. + let ext_seq = der::expect_tag(der, Tag::Sequence)?; + if ext_seq.is_empty() { + return Ok(revoked_cert); + } - let mut reader = untrusted::Reader::new(ext_seq); - loop { - der::nested(&mut reader, Tag::Sequence, Error::BadDer, |ext_der| { - // RFC 5280 §5.3: - // If a CRL contains a critical CRL entry extension that the application cannot - // process, then the application MUST NOT use that CRL to determine the - // status of any certificates. However, applications may ignore - // unrecognized non-critical CRL entry extensions. - revoked_cert.remember_extension(&Extension::from_der(ext_der)?) - })?; - if reader.at_end() { - break; + let mut reader = untrusted::Reader::new(ext_seq); + loop { + der::nested( + &mut reader, + Tag::Sequence, + Error::TrailingData("BorrowedRevokedCert::inner"), + |ext_der| { + // RFC 5280 §5.3: + // If a CRL contains a critical CRL entry extension that the application cannot + // process, then the application MUST NOT use that CRL to determine the + // status of any certificates. However, applications may ignore + // unrecognized non-critical CRL entry extensions. + revoked_cert.remember_extension(&Extension::from_der(ext_der)?) + }, + )?; + if reader.at_end() { + break; + } } - } - Ok(revoked_cert) - }) + Ok(revoked_cert) + }, + ) } + + const NAME: &'static str = "BorrowedRevokedCert"; } /// Identifies the reason a certificate was revoked. @@ -595,6 +609,8 @@ impl<'a> FromDer<'a> for RevocationReason { reason.read_byte().map_err(|_| Error::BadDer) })?) } + + const NAME: &'static str = "RevocationReason"; } impl TryFrom for RevocationReason { @@ -668,7 +684,7 @@ impl<'a> IssuingDistributionPoint<'a> { der::nested( &mut untrusted::Reader::new(der), Tag::Sequence, - Error::BadDer, + Error::TrailingData("IssuingDistributionPoint"), |der| { while !der.at_end() { let (tag, value) = der::read_tag_and_get_value(der)?; diff --git a/src/der.rs b/src/der.rs index ac4fb30b..f5bb3f49 100644 --- a/src/der.rs +++ b/src/der.rs @@ -43,10 +43,12 @@ impl<'a, T: FromDer<'a>> Iterator for DerIterator<'a, T> { pub(crate) trait FromDer<'a>: Sized + 'a { /// Parse a value of type `Self` from the given DER-encoded input. fn from_der(reader: &mut untrusted::Reader<'a>) -> Result; + + const NAME: &'static str; } pub(crate) fn read_all<'a, T: FromDer<'a>>(input: untrusted::Input<'a>) -> Result { - input.read_all(Error::BadDer, T::from_der) + input.read_all(Error::TrailingData(T::NAME), T::from_der) } // Copied (and extended) from ring's src/der.rs @@ -283,13 +285,18 @@ pub(crate) fn nested_of_mut<'a>( pub(crate) fn bit_string_with_no_unused_bits<'a>( input: &mut untrusted::Reader<'a>, ) -> Result, Error> { - nested(input, Tag::BitString, Error::BadDer, |value| { - let unused_bits_at_end = value.read_byte().map_err(|_| Error::BadDer)?; - if unused_bits_at_end != 0 { - return Err(Error::BadDer); - } - Ok(value.read_bytes_to_end()) - }) + nested( + input, + Tag::BitString, + Error::TrailingData("BitString"), + |value| { + let unused_bits_at_end = value.read_byte().map_err(|_| Error::BadDer)?; + if unused_bits_at_end != 0 { + return Err(Error::BadDer); + } + Ok(value.read_bytes_to_end()) + }, + ) } pub(crate) struct BitStringFlags<'a> { @@ -350,6 +357,8 @@ impl<'a> FromDer<'a> for u8 { _ => Err(Error::BadDer), } } + + const NAME: &'static str = "u8"; } pub(crate) fn nonnegative_integer<'a>( @@ -387,14 +396,19 @@ impl<'a> FromDer<'a> for bool { return Ok(false); } - nested(reader, Tag::Boolean, Error::BadDer, |input| { - match input.read_byte() { + nested( + reader, + Tag::Boolean, + Error::TrailingData(Self::NAME), + |input| match input.read_byte() { Ok(0xff) => Ok(true), Ok(0x00) => Ok(false), _ => Err(Error::BadDer), - } - }) + }, + ) } + + const NAME: &'static str = "bool"; } macro_rules! oid { @@ -435,19 +449,19 @@ mod tests { // Unexpected type assert_eq!( - Err(Error::BadDer), + Err(Error::TrailingData("BitString")), bit_string_with_no_unused_bits(&mut bytes_reader(&[0x01, 0x01, 0xff])) ); // Unexpected nonexistent type assert_eq!( - Err(Error::BadDer), - bit_string_with_no_unused_bits(&mut bytes_reader(&[0x42, 0xff, 0xff])) + Err(Error::TrailingData("BitString")), + bit_string_with_no_unused_bits(&mut bytes_reader(&[0x42, 0xff, 0xff])), ); // Unexpected empty input assert_eq!( - Err(Error::BadDer), + Err(Error::TrailingData("BitString")), bit_string_with_no_unused_bits(&mut bytes_reader(&[])) ); diff --git a/src/error.rs b/src/error.rs index bb6a3cfe..452b1818 100644 --- a/src/error.rs +++ b/src/error.rs @@ -107,6 +107,9 @@ pub enum Error { /// does not match the algorithm in the signature of the certificate. SignatureAlgorithmMismatch, + /// Trailing data was found while parsing DER-encoded input for the named type. + TrailingData(&'static str), + /// A valid issuer for the certificate could not be found. UnknownIssuer, @@ -222,7 +225,7 @@ impl Error { // Errors related to malformed data. Error::MalformedDnsIdentifier => 6, Error::MalformedNameConstraint => 5, - Error::MalformedExtensions => 4, + Error::MalformedExtensions | Error::TrailingData(_) => 4, Error::ExtensionValueInvalid => 3, // Generic DER errors. diff --git a/src/ring_algs.rs b/src/ring_algs.rs index c3698f80..e94b73cf 100644 --- a/src/ring_algs.rs +++ b/src/ring_algs.rs @@ -204,14 +204,14 @@ mod tests { let algorithm = untrusted::Input::from(&tsd.algorithm); let algorithm = algorithm - .read_all(Error::BadDer, |input| { + .read_all(Error::TrailingData("signature algorithm"), |input| { der::expect_tag(input, der::Tag::Sequence) }) .unwrap(); let signature = untrusted::Input::from(&tsd.signature); let signature = signature - .read_all(Error::BadDer, |input| { + .read_all(Error::TrailingData("signature"), |input| { der::bit_string_with_no_unused_bits(input) }) .unwrap(); @@ -250,7 +250,7 @@ mod tests { let signature = untrusted::Input::from(&tsd.signature); assert_eq!( Err(expected_error), - signature.read_all(Error::BadDer, |input| { + signature.read_all(Error::TrailingData("signature"), |input| { der::bit_string_with_no_unused_bits(input) }) ); diff --git a/src/signed_data.rs b/src/signed_data.rs index da7cde72..d9092e78 100644 --- a/src/signed_data.rs +++ b/src/signed_data.rs @@ -241,6 +241,8 @@ impl<'a> FromDer<'a> for SubjectPublicKeyInfo<'a> { key_value, }) } + + const NAME: &'static str = "SubjectPublicKeyInfo"; } /// An abstract signature verification algorithm. diff --git a/src/subject_name/verify.rs b/src/subject_name/verify.rs index e2892bc0..710f352e 100644 --- a/src/subject_name/verify.rs +++ b/src/subject_name/verify.rs @@ -442,24 +442,36 @@ impl<'a> FromDer<'a> for GeneralName<'a> { _ => return Err(Error::BadDer), }) } + + const NAME: &'static str = "GeneralName"; } static COMMON_NAME: untrusted::Input = untrusted::Input::from(&[85, 4, 3]); fn common_name(input: untrusted::Input) -> Result, Error> { let inner = &mut untrusted::Reader::new(input); - der::nested(inner, der::Tag::Set, Error::BadDer, |tagged| { - der::nested(tagged, der::Tag::Sequence, Error::BadDer, |tagged| { - while !tagged.at_end() { - let name_oid = der::expect_tag(tagged, der::Tag::OID)?; - if name_oid == COMMON_NAME { - return der::expect_tag(tagged, der::Tag::UTF8String).map(Some); - } else { - // discard unused name value - der::read_tag_and_get_value(tagged)?; - } - } - Ok(None) - }) - }) + der::nested( + inner, + der::Tag::Set, + Error::TrailingData("CommonName::outer"), + |tagged| { + der::nested( + tagged, + der::Tag::Sequence, + Error::TrailingData("CommonName::inner"), + |tagged| { + while !tagged.at_end() { + let name_oid = der::expect_tag(tagged, der::Tag::OID)?; + if name_oid == COMMON_NAME { + return der::expect_tag(tagged, der::Tag::UTF8String).map(Some); + } else { + // discard unused name value + der::read_tag_and_get_value(tagged)?; + } + } + Ok(None) + }, + ) + }, + ) } diff --git a/src/time.rs b/src/time.rs index e22a2ac9..7f59fbd2 100644 --- a/src/time.rs +++ b/src/time.rs @@ -70,33 +70,40 @@ impl<'a> FromDer<'a> for Time { Ok(value) } - der::nested(input, expected_tag, Error::BadDer, |value| { - let (year_hi, year_lo) = if is_utc_time { - let lo = read_two_digits(value, 0, 99)?; - let hi = if lo >= 50 { 19 } else { 20 }; - (hi, lo) - } else { - let hi = read_two_digits(value, 0, 99)?; - let lo = read_two_digits(value, 0, 99)?; - (hi, lo) - }; - - let year = (year_hi * 100) + year_lo; - let month = read_two_digits(value, 1, 12)?; - let days_in_month = days_in_month(year, month); - let day_of_month = read_two_digits(value, 1, days_in_month)?; - let hours = read_two_digits(value, 0, 23)?; - let minutes = read_two_digits(value, 0, 59)?; - let seconds = read_two_digits(value, 0, 59)?; - - let time_zone = value.read_byte().map_err(|_| Error::BadDerTime)?; - if time_zone != b'Z' { - return Err(Error::BadDerTime); - } - - time_from_ymdhms_utc(year, month, day_of_month, hours, minutes, seconds) - }) + der::nested( + input, + expected_tag, + Error::TrailingData(Self::NAME), + |value| { + let (year_hi, year_lo) = if is_utc_time { + let lo = read_two_digits(value, 0, 99)?; + let hi = if lo >= 50 { 19 } else { 20 }; + (hi, lo) + } else { + let hi = read_two_digits(value, 0, 99)?; + let lo = read_two_digits(value, 0, 99)?; + (hi, lo) + }; + + let year = (year_hi * 100) + year_lo; + let month = read_two_digits(value, 1, 12)?; + let days_in_month = days_in_month(year, month); + let day_of_month = read_two_digits(value, 1, days_in_month)?; + let hours = read_two_digits(value, 0, 23)?; + let minutes = read_two_digits(value, 0, 59)?; + let seconds = read_two_digits(value, 0, 59)?; + + let time_zone = value.read_byte().map_err(|_| Error::BadDerTime)?; + if time_zone != b'Z' { + return Err(Error::BadDerTime); + } + + time_from_ymdhms_utc(year, month, day_of_month, hours, minutes, seconds) + }, + ) } + + const NAME: &'static str = "Time"; } #[cfg(feature = "std")] diff --git a/src/trust_anchor.rs b/src/trust_anchor.rs index dde6102b..26b86e01 100644 --- a/src/trust_anchor.rs +++ b/src/trust_anchor.rs @@ -53,30 +53,40 @@ impl<'a> TrustAnchor<'a> { fn from_v1_der(cert_der: untrusted::Input<'a>) -> Result { // X.509 Certificate: https://tools.ietf.org/html/rfc5280#section-4.1. cert_der.read_all(Error::BadDer, |cert_der| { - der::nested(cert_der, der::Tag::Sequence, Error::BadDer, |cert_der| { - let anchor = der::nested(cert_der, der::Tag::Sequence, Error::BadDer, |tbs| { - // The version number field does not appear in v1 certificates. - lenient_certificate_serial_number(tbs)?; + der::nested( + cert_der, + der::Tag::Sequence, + Error::TrailingData("TrustAnchorV1::outer"), + |cert_der| { + let anchor = der::nested( + cert_der, + der::Tag::Sequence, + Error::TrailingData("TrustAnchorV1::inner"), + |tbs| { + // The version number field does not appear in v1 certificates. + lenient_certificate_serial_number(tbs)?; - skip(tbs, der::Tag::Sequence)?; // signature. - skip(tbs, der::Tag::Sequence)?; // issuer. - skip(tbs, der::Tag::Sequence)?; // validity. - let subject = der::expect_tag(tbs, der::Tag::Sequence)?; - let spki = der::expect_tag(tbs, der::Tag::Sequence)?; + skip(tbs, der::Tag::Sequence)?; // signature. + skip(tbs, der::Tag::Sequence)?; // issuer. + skip(tbs, der::Tag::Sequence)?; // validity. + let subject = der::expect_tag(tbs, der::Tag::Sequence)?; + let spki = der::expect_tag(tbs, der::Tag::Sequence)?; - Ok(TrustAnchor { - subject: subject.as_slice_less_safe(), - spki: spki.as_slice_less_safe(), - name_constraints: None, - }) - }); + Ok(TrustAnchor { + subject: subject.as_slice_less_safe(), + spki: spki.as_slice_less_safe(), + name_constraints: None, + }) + }, + ); - // read and discard signatureAlgorithm + signature - skip(cert_der, der::Tag::Sequence)?; - skip(cert_der, der::Tag::BitString)?; + // read and discard signatureAlgorithm + signature + skip(cert_der, der::Tag::Sequence)?; + skip(cert_der, der::Tag::BitString)?; - anchor - }) + anchor + }, + ) }) } } diff --git a/src/x509.rs b/src/x509.rs index 41edf29c..9a80cb54 100644 --- a/src/x509.rs +++ b/src/x509.rs @@ -42,6 +42,8 @@ impl<'a> FromDer<'a> for Extension<'a> { value, }) } + + const NAME: &'static str = "Extension"; } pub(crate) fn set_extension_once( @@ -107,4 +109,6 @@ impl<'a> FromDer<'a> for DistributionPointName<'a> { _ => Err(Error::BadDer), } } + + const NAME: &'static str = "DistributionPointName"; } diff --git a/tests/crl_tests.rs b/tests/crl_tests.rs index 7963fbc3..9da61ba3 100644 --- a/tests/crl_tests.rs +++ b/tests/crl_tests.rs @@ -59,7 +59,7 @@ fn parse_missing_next_update_crl() { // Parsing a CRL with a missing next update time should error. let crl = include_bytes!("crls/crl.missing.next.update.der"); let res = BorrowedCertRevocationList::from_der(&crl[..]); - assert!(matches!(res, Err(Error::BadDer))); + assert!(matches!(res, Err(Error::TrailingData("Time")))); } #[test]