From c656bb77ff0b1af60b9de0fc6a524b94792f6ec7 Mon Sep 17 00:00:00 2001 From: Joe Birr-Pixton Date: Thu, 11 Jun 2026 12:50:46 +0100 Subject: [PATCH 1/4] Tighten reading of CRL bitflags --- src/crl/mod.rs | 38 +++++++++++++++++++++----------------- src/error.rs | 1 + 2 files changed, 22 insertions(+), 17 deletions(-) diff --git a/src/crl/mod.rs b/src/crl/mod.rs index 93cff58f..caee60f0 100644 --- a/src/crl/mod.rs +++ b/src/crl/mod.rs @@ -18,7 +18,7 @@ use pki_types::{SignatureVerificationAlgorithm, UnixTime}; use crate::error::Error; use crate::verify_cert::{Budget, PathNode, Role}; -use crate::{der, public_values_eq}; +use crate::{DerTypeId, der, public_values_eq}; mod types; pub use types::{ @@ -204,22 +204,26 @@ enum KeyUsageMode { impl KeyUsageMode { // https://www.rfc-editor.org/rfc/rfc5280#section-4.2.1.3 fn check(self, input: Option>) -> Result<(), Error> { - let bit_string = match input { - Some(input) => { - der::expect_tag(&mut untrusted::Reader::new(input), der::Tag::BitString)? - } - // While RFC 5280 requires KeyUsage be present, historically the absence of a KeyUsage - // has been treated as "Any Usage". We follow that convention here and assume the absence - // of KeyUsage implies the required_ku_bit_if_present we're checking for. - None => return Ok(()), - }; - - let flags = der::bit_string_flags(bit_string)?; - #[expect(clippy::as_conversions)] // u8 always fits in usize. - match flags.bit_set(self as usize) { - true => Ok(()), - false => Err(Error::IssuerNotCrlSigner), - } + untrusted::read_all_optional( + input, + Error::TrailingData(DerTypeId::KeyUsageExtension), + |reader| { + let Some(reader) = reader else { + // While RFC 5280 requires KeyUsage be present, historically the absence of a KeyUsage + // has been treated as "Any Usage". We follow that convention here and assume the absence + // of KeyUsage implies the required_ku_bit_if_present we're checking for. + return Ok(()); + }; + + let bit_string = der::expect_tag(reader, der::Tag::BitString)?; + let flags = der::bit_string_flags(bit_string)?; + #[expect(clippy::as_conversions)] // u8 always fits in usize. + match flags.bit_set(self as usize) { + true => Ok(()), + false => Err(Error::IssuerNotCrlSigner), + } + }, + ) } } diff --git a/src/error.rs b/src/error.rs index ba722673..e228dd1c 100644 --- a/src/error.rs +++ b/src/error.rs @@ -428,4 +428,5 @@ pub enum DerTypeId { IssuingDistributionPoint, IssuerUniqueId, SubjectUniqueId, + KeyUsageExtension, } From 3bd35b0fd4c8cebbfd7ec243a1da6c612d2d4c04 Mon Sep 17 00:00:00 2001 From: Joe Birr-Pixton Date: Thu, 11 Jun 2026 12:54:13 +0100 Subject: [PATCH 2/4] check_key_usage_cert_sign: use read_all() --- src/verify_cert.rs | 28 +++++++++++++++++----------- 1 file changed, 17 insertions(+), 11 deletions(-) diff --git a/src/verify_cert.rs b/src/verify_cert.rs index d57362ae..adf722af 100644 --- a/src/verify_cert.rs +++ b/src/verify_cert.rs @@ -28,7 +28,7 @@ use crate::end_entity::EndEntityCert; use crate::error::Error; #[cfg(feature = "alloc")] use crate::spki_for_anchor; -use crate::{public_values_eq, subject_name}; +use crate::{DerTypeId, public_values_eq, subject_name}; /// Build a [`VerifiedPath`] for an end-entity certificate from the given trust anchors. // Use `'a` for lifetimes that we don't care about, `'p` for lifetimes that become a part of @@ -455,16 +455,22 @@ fn check_issuer_independent_properties( fn check_key_usage_cert_sign(key_usage: untrusted::Input<'_>, role: Role) -> Result<(), Error> { // The KeyUsage extension is a BIT STRING; keyCertSign is bit 5. const KEY_CERT_SIGN: usize = 5; - let bit_string = der::expect_tag(&mut untrusted::Reader::new(key_usage), der::Tag::BitString)?; - match ( - role, - der::bit_string_flags(bit_string)?.bit_set(KEY_CERT_SIGN), - ) { - (Role::Issuer, true) | (Role::EndEntity, false) => Ok(()), - (Role::Issuer, false) => Err(Error::IssuerNotCertSigner), - // Disallowed per https://datatracker.ietf.org/doc/html/rfc5280#section-4.2.1.9 - (Role::EndEntity, true) => Err(Error::EndEntityCertHasCertSignKeyUsage), - } + + key_usage.read_all( + Error::TrailingData(DerTypeId::KeyUsageExtension), + |reader| { + let bit_string = der::expect_tag(reader, der::Tag::BitString)?; + match ( + role, + der::bit_string_flags(bit_string)?.bit_set(KEY_CERT_SIGN), + ) { + (Role::Issuer, true) | (Role::EndEntity, false) => Ok(()), + (Role::Issuer, false) => Err(Error::IssuerNotCertSigner), + // Disallowed per https://datatracker.ietf.org/doc/html/rfc5280#section-4.2.1.9 + (Role::EndEntity, true) => Err(Error::EndEntityCertHasCertSignKeyUsage), + } + }, + ) } fn check_eku( From 01b2d8f4c932e472867ed99b1b112813974e17cc Mon Sep 17 00:00:00 2001 From: Joe Birr-Pixton Date: Thu, 11 Jun 2026 20:23:54 +0100 Subject: [PATCH 3/4] Improve error if extension parsing has trailing bytes --- src/cert.rs | 25 ++++++++++++++----------- 1 file changed, 14 insertions(+), 11 deletions(-) diff --git a/src/cert.rs b/src/cert.rs index 2dc19800..33af1e2e 100644 --- a/src/cert.rs +++ b/src/cert.rs @@ -344,17 +344,20 @@ fn remember_cert_extension<'a>( }; set_extension_once(out, || { - extension.value.read_all(Error::BadDer, |value| match id { - // Unlike the other extensions we remember KU is a BitString and not a Sequence. We - // read the raw bytes here and parse at the time of use. - Standard(15) => Ok(value.read_bytes_to_end()), - - // signedCertificateTimestampList is an OCTET STRING - SignedCertificateTimestampList => der::expect_tag(value, Tag::OctetString), - - // All other remembered certificate extensions are wrapped in a Sequence. - _ => der::expect_tag(value, Tag::Sequence), - }) + extension.value.read_all( + Error::TrailingData(DerTypeId::Extension), + |value| match id { + // Unlike the other extensions we remember KU is a BitString and not a Sequence. We + // read the raw bytes here and parse at the time of use. + Standard(15) => Ok(value.read_bytes_to_end()), + + // signedCertificateTimestampList is an OCTET STRING + SignedCertificateTimestampList => der::expect_tag(value, Tag::OctetString), + + // All other remembered certificate extensions are wrapped in a Sequence. + _ => der::expect_tag(value, Tag::Sequence), + }, + ) }) }) } From d4c6b990839d97952fdffa5761b8f00fe096d3d9 Mon Sep 17 00:00:00 2001 From: Joe Birr-Pixton Date: Thu, 11 Jun 2026 12:57:43 +0100 Subject: [PATCH 4/4] Tighten reading on CRL `DistributionPointName` --- src/cert.rs | 14 +++++++++++--- src/crl/types.rs | 14 +++++++++++--- 2 files changed, 22 insertions(+), 6 deletions(-) diff --git a/src/cert.rs b/src/cert.rs index 33af1e2e..f3a505f0 100644 --- a/src/cert.rs +++ b/src/cert.rs @@ -382,9 +382,17 @@ pub(crate) struct CrlDistributionPoint<'a> { impl<'a> CrlDistributionPoint<'a> { /// Return the distribution point names (if any). pub(crate) fn names(&self) -> Result>, Error> { - self.distribution_point - .map(|input| DistributionPointName::from_der(&mut untrusted::Reader::new(input))) - .transpose() + untrusted::read_all_optional( + self.distribution_point, + Error::TrailingData(DerTypeId::DistributionPointName), + |reader| { + let Some(reader) = reader else { + return Ok(None); + }; + + Ok(Some(DistributionPointName::from_der(reader)?)) + }, + ) } } diff --git a/src/crl/types.rs b/src/crl/types.rs index 846183f5..8e8d1c59 100644 --- a/src/crl/types.rs +++ b/src/crl/types.rs @@ -641,9 +641,17 @@ impl<'a> IssuingDistributionPoint<'a> { /// Return the distribution point names (if any). pub(crate) fn names(&self) -> Result>, Error> { - self.distribution_point - .map(|input| DistributionPointName::from_der(&mut untrusted::Reader::new(input))) - .transpose() + untrusted::read_all_optional( + self.distribution_point, + Error::TrailingData(DerTypeId::DistributionPointName), + |reader| { + let Some(reader) = reader else { + return Ok(None); + }; + + Ok(Some(DistributionPointName::from_der(reader)?)) + }, + ) } /// Returns true if the CRL can be considered authoritative for the given certificate. We make