Skip to content

Commit

Permalink
der: stop passing in Error::BadDer explicitly
Browse files Browse the repository at this point in the history
  • Loading branch information
@djc
djc committed Jul 31, 2023
1 parent 008ea8c commit 0343ad9
Show file tree
Hide file tree
Showing 6 changed files with 83 additions and 104 deletions.
59 changes: 28 additions & 31 deletions src/cert.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -57,7 +57,7 @@ impl<'a> Cert<'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| {
der::nested(cert_der, der::Tag::Sequence, |der| {
// limited to SEQUENCEs of size 2^16 or less.
SignedData::from_der(der, der::TWO_BYTE_DER_SIZE)
})
Expand Down Expand Up @@ -105,22 +105,20 @@ impl<'a> Cert<'a> {
};

if !tbs.at_end() {
der::nested(
tbs,
der::Tag::ContextSpecificConstructed3,
Error::MalformedExtensions,
|tagged| {
der::nested_of_mut(
tagged,
der::Tag::Sequence,
der::Tag::Sequence,
Error::BadDer,
|extension| {
remember_cert_extension(&mut cert, &Extension::from_der(extension)?)
},
)
},
)?;
der::nested(tbs, der::Tag::ContextSpecificConstructed3, |tagged| {
der::nested_of_mut(
tagged,
der::Tag::Sequence,
der::Tag::Sequence,
|extension| {
remember_cert_extension(&mut cert, &Extension::from_der(extension)?)
},
)
})
.map_err(|err| match err {
Error::BadDer => Error::MalformedExtensions,

Check warning on line 119 in src/cert.rs

View check run for this annotation

Codecov / codecov/patch

src/cert.rs#L119 

Added line #L119 was not covered by tests
err => err,
})?;
}

Ok(cert)
Expand Down Expand Up @@ -160,19 +158,18 @@ impl<'a> Cert<'a> {
// mozilla::pkix supports v1, v2, v3, and v4, including both the implicit
// (correct) and explicit (incorrect) encoding of v1. We allow only v3.
fn version3(input: &mut untrusted::Reader) -> Result<(), Error> {
der::nested(
input,
der::Tag::ContextSpecificConstructed0,
Error::UnsupportedCertVersion,
|input| {
let version = u8::from_der(input)?;
if version != 2 {
// v3
return Err(Error::UnsupportedCertVersion);
}
Ok(())
},
)
der::nested(input, der::Tag::ContextSpecificConstructed0, |input| {
let version = u8::from_der(input)?;
if version != 2 {
// v3
return Err(Error::UnsupportedCertVersion);

Check warning on line 165 in src/cert.rs

View check run for this annotation

Codecov / codecov/patch

src/cert.rs#L165 

Added line #L165 was not covered by tests
}
Ok(())
})
.map_err(|err| match err {
Error::BadDer => Error::UnsupportedCertVersion,
err => err,

Check warning on line 171 in src/cert.rs

View check run for this annotation

Codecov / codecov/patch

src/cert.rs#L171 

Added line #L171 was not covered by tests
})
}

pub(crate) fn lenient_certificate_serial_number<'a>(
Expand Down Expand Up @@ -273,7 +270,7 @@ impl<'a> FromDer<'a> for CrlDistributionPoint<'a> {
crl_issuer: None,
};

der::nested(reader, Tag::Sequence, Error::BadDer, |der| {
der::nested(reader, Tag::Sequence, |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;
Expand Down
95 changes: 41 additions & 54 deletions src/crl.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -260,7 +260,6 @@ impl<'a> FromDer<'a> for BorrowedCertRevocationList<'a> {
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,
)?;
Expand Down Expand Up @@ -334,27 +333,20 @@ impl<'a> FromDer<'a> for BorrowedCertRevocationList<'a> {
// 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::from_der(extension)?)
},
)
},
)?;
der::nested(tbs_cert_list, Tag::ContextSpecificConstructed0, |tagged| {
der::nested_of_mut(tagged, Tag::Sequence, Tag::Sequence, |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::from_der(extension)?)
})
})
.map_err(|err| match err {
Error::BadDer => Error::MalformedExtensions,
_ => err,
})?;

Ok(crl)
})?;
Expand Down Expand Up @@ -484,7 +476,7 @@ 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| {
der::nested(reader, Tag::Sequence, |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.
Expand Down Expand Up @@ -526,7 +518,7 @@ impl<'a> FromDer<'a> for BorrowedRevokedCert<'a> {

let mut reader = untrusted::Reader::new(ext_seq);
loop {
der::nested(&mut reader, Tag::Sequence, Error::BadDer, |ext_der| {
der::nested(&mut reader, Tag::Sequence, |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
Expand Down Expand Up @@ -665,39 +657,34 @@ impl<'a> IssuingDistributionPoint<'a> {
}

// RFC 5280 section §4.2.1.13:
der::nested(
&mut untrusted::Reader::new(der),
Tag::Sequence,
Error::BadDer,
|der| {
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))?
}
ONLY_CONTAINS_USER_CERTS_TAG => {
result.only_contains_user_certs = decode_bool(value)?
}
ONLY_CONTAINS_CA_CERTS_TAG => {
result.only_contains_ca_certs = decode_bool(value)?
}
ONLY_CONTAINS_SOME_REASONS_TAG => {
set_extension_once(&mut result.only_some_reasons, || {
der::bit_string_flags(value)
})?
}
INDIRECT_CRL_TAG => result.indirect_crl = decode_bool(value)?,
ONLY_CONTAINS_ATTRIBUTE_CERTS_TAG => {
result.only_contains_attribute_certs = decode_bool(value)?
}
_ => return Err(Error::BadDer),
der::nested(&mut untrusted::Reader::new(der), Tag::Sequence, |der| {
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))?
}
ONLY_CONTAINS_USER_CERTS_TAG => {
result.only_contains_user_certs = decode_bool(value)?
}
ONLY_CONTAINS_CA_CERTS_TAG => {
result.only_contains_ca_certs = decode_bool(value)?
}
ONLY_CONTAINS_SOME_REASONS_TAG => {
set_extension_once(&mut result.only_some_reasons, || {
der::bit_string_flags(value)
})?
}
INDIRECT_CRL_TAG => result.indirect_crl = decode_bool(value)?,
ONLY_CONTAINS_ATTRIBUTE_CERTS_TAG => {
result.only_contains_attribute_certs = decode_bool(value)?
}
_ => return Err(Error::BadDer),
}
}

Ok(())
},
)?;
Ok(())
})?;

// RFC 5280 4.2.1.10:
// Conforming CRLs issuers MUST set the onlyContainsAttributeCerts boolean to FALSE.
Expand Down
23 changes: 9 additions & 14 deletions src/der.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -105,22 +105,20 @@ pub(crate) fn expect_tag_and_get_value_limited<'a>(
pub(crate) fn nested_limited<'a, R>(
input: &mut untrusted::Reader<'a>,
tag: Tag,
error: Error,
decoder: impl FnOnce(&mut untrusted::Reader<'a>) -> Result<R, Error>,
size_limit: usize,
) -> Result<R, Error> {
expect_tag_and_get_value_limited(input, tag, size_limit)?.read_all(error, decoder)
expect_tag_and_get_value_limited(input, tag, size_limit)?.read_all(Error::BadDer, decoder)
}

// TODO: investigate taking decoder as a reference to reduce generated code
// size.
pub(crate) fn nested<'a, R>(
input: &mut untrusted::Reader<'a>,
tag: Tag,
error: Error,
decoder: impl FnOnce(&mut untrusted::Reader<'a>) -> Result<R, Error>,
) -> Result<R, Error> {
nested_limited(input, tag, error, decoder, TWO_BYTE_DER_SIZE)
nested_limited(input, tag, decoder, TWO_BYTE_DER_SIZE)
}

pub(crate) fn expect_tag<'a>(
Expand Down Expand Up @@ -264,12 +262,11 @@ pub(crate) fn nested_of_mut<'a>(
input: &mut untrusted::Reader<'a>,
outer_tag: Tag,
inner_tag: Tag,
error: Error,
mut decoder: impl FnMut(&mut untrusted::Reader<'a>) -> Result<(), Error>,
) -> Result<(), Error> {
nested(input, outer_tag, error, |outer| {
nested(input, outer_tag, |outer| {
loop {
nested(outer, inner_tag, error, |inner| decoder(inner))?;
nested(outer, inner_tag, |inner| decoder(inner))?;
if outer.at_end() {
break;
}
Expand All @@ -281,7 +278,7 @@ 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| {
nested(input, Tag::BitString, |value| {
let unused_bits_at_end = value.read_byte().map_err(|_| Error::BadDer)?;
if unused_bits_at_end != 0 {
return Err(Error::BadDer);
Expand Down Expand Up @@ -385,12 +382,10 @@ impl<'a> FromDer<'a> for bool {
return Ok(false);
}

nested(reader, Tag::Boolean, Error::BadDer, |input| {
match input.read_byte() {
Ok(0xff) => Ok(true),
Ok(0x00) => Ok(false),
_ => Err(Error::BadDer),
}
nested(reader, Tag::Boolean, |input| match input.read_byte() {
Ok(0xff) => Ok(true),
Ok(0x00) => Ok(false),
_ => Err(Error::BadDer),
})
}
}
Expand Down
4 changes: 2 additions & 2 deletions src/subject_name/verify.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -448,8 +448,8 @@ static COMMON_NAME: untrusted::Input = untrusted::Input::from(&[85, 4, 3]);

fn common_name(input: untrusted::Input) -> Result<Option<untrusted::Input>, 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| {
der::nested(inner, der::Tag::Set, |tagged| {
der::nested(tagged, der::Tag::Sequence, |tagged| {
while !tagged.at_end() {
let name_oid = der::expect_tag(tagged, der::Tag::OID)?;
if name_oid == COMMON_NAME {
Expand Down
2 changes: 1 addition & 1 deletion src/time.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -70,7 +70,7 @@ impl<'a> FromDer<'a> for Time {
Ok(value)
}

der::nested(input, expected_tag, Error::BadDer, |value| {
der::nested(input, expected_tag, |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 };
Expand Down
4 changes: 2 additions & 2 deletions src/trust_anchor.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -53,8 +53,8 @@ impl<'a> TrustAnchor<'a> {
fn from_v1_der(cert_der: untrusted::Input<'a>) -> Result<Self, Error> {
// 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| {
der::nested(cert_der, der::Tag::Sequence, |cert_der| {
let anchor = der::nested(cert_der, der::Tag::Sequence, |tbs| {
// The version number field does not appear in v1 certificates.
lenient_certificate_serial_number(tbs)?;

Expand Down

0 comments on commit 0343ad9

Please sign in to comment.