Skip to content

Commit

Permalink
Pass more specific errors to read_all()
Browse files Browse the repository at this point in the history
  • Loading branch information
@djc
djc committed Aug 1, 2023
1 parent 056c003 commit 838cf36
Show file tree
Hide file tree
Showing 11 changed files with 251 additions and 170 deletions.
79 changes: 46 additions & 33 deletions src/cert.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -56,14 +56,20 @@ impl<'a> Cert<'a> {
cert_der: untrusted::Input<'a>,
ee_or_ca: EndEntityOrCa<'a>,
) -> Result<Self, Error> {
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("Certificate"), |cert_der| {
der::nested(
cert_der,
der::Tag::Sequence,
Error::TrailingData("Certificate::signed_data"),
|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("Cert::tbs_certificate"), |tbs| {
version3(tbs)?;

let serial = lenient_certificate_serial_number(tbs)?;
Expand Down Expand Up @@ -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)?)
},
Expand Down Expand Up @@ -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)]
Expand Down
128 changes: 72 additions & 56 deletions src/crl.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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,
)?;
Expand Down Expand Up @@ -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
Expand All @@ -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> {
Expand Down Expand Up @@ -484,64 +486,76 @@ impl<'a> BorrowedRevokedCert<'a> {

impl<'a> FromDer<'a> for BorrowedRevokedCert<'a> {
fn from_der(reader: &mut untrusted::Reader<'a>) -> Result<Self, Error> {
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::crl_entry"),
|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::crl_entry_extensions"),
|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.
Expand Down Expand Up @@ -595,6 +609,8 @@ impl<'a> FromDer<'a> for RevocationReason {
reason.read_byte().map_err(|_| Error::BadDer)
})?)
}

const NAME: &'static str = "RevocationReason";
}

impl TryFrom<u8> for RevocationReason {
Expand Down Expand Up @@ -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)?;
Expand Down
46 changes: 30 additions & 16 deletions src/der.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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<Self, Error>;

const NAME: &'static str;
}

pub(crate) fn read_all<'a, T: FromDer<'a>>(input: untrusted::Input<'a>) -> Result<T, Error> {
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
Expand Down Expand Up @@ -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<untrusted::Input<'a>, 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> {
Expand Down Expand Up @@ -350,6 +357,8 @@ impl<'a> FromDer<'a> for u8 {
_ => Err(Error::BadDer),
}
}

const NAME: &'static str = "u8";
}

pub(crate) fn nonnegative_integer<'a>(
Expand Down Expand Up @@ -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 {
Expand Down Expand Up @@ -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(&[]))
);

Expand Down
Loading

0 comments on commit 838cf36

Please sign in to comment.