diff --git a/Cargo.toml b/Cargo.toml index db3e90ae..0bc5314f 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -21,7 +21,7 @@ license = "ISC" name = "rustls-webpki" readme = "README.md" repository = "https://github.com/rustls/webpki" -version = "0.101.4" +version = "0.101.5" include = [ "Cargo.toml", @@ -101,4 +101,4 @@ harness = false [patch.crates-io] # TODO(XXX): Remove this once rcgen has cut a release w/ CRL support included. -rcgen = { git = 'https://github.com/est31/rcgen.git' } +rcgen = { git = 'https://github.com/est31/rcgen.git', rev = '83e548a06848d923eada1ac66d1a912735b67e79' } diff --git a/src/crl.rs b/src/crl.rs index 0c47e4a3..25a4997c 100644 --- a/src/crl.rs +++ b/src/crl.rs @@ -15,6 +15,7 @@ use crate::cert::lenient_certificate_serial_number; use crate::der::Tag; use crate::signed_data::{self, SignedData}; +use crate::verify_cert::Budget; use crate::x509::{remember_extension, set_extension_once, Extension}; use crate::{der, Error, SignatureAlgorithm, Time}; @@ -42,6 +43,7 @@ pub trait CertRevocationList: Sealed { &self, supported_sig_algs: &[&SignatureAlgorithm], issuer_spki: &[u8], + budget: &mut Budget, ) -> Result<(), Error>; } @@ -85,11 +87,13 @@ impl CertRevocationList for OwnedCertRevocationList { &self, supported_sig_algs: &[&SignatureAlgorithm], issuer_spki: &[u8], + budget: &mut Budget, ) -> Result<(), Error> { signed_data::verify_signed_data( supported_sig_algs, untrusted::Input::from(issuer_spki), &self.signed_data.borrow(), + budget, ) } } @@ -329,11 +333,13 @@ impl CertRevocationList for BorrowedCertRevocationList<'_> { &self, supported_sig_algs: &[&SignatureAlgorithm], issuer_spki: &[u8], + budget: &mut Budget, ) -> Result<(), Error> { signed_data::verify_signed_data( supported_sig_algs, untrusted::Input::from(issuer_spki), &self.signed_data, + budget, ) } } @@ -631,6 +637,8 @@ mod tests { #[test] #[cfg(feature = "alloc")] + // redundant clone, clone_on_copy allowed to verify derived traits. + #[allow(clippy::redundant_clone, clippy::clone_on_copy)] fn test_derived_traits() { let crl = crate::crl::BorrowedCertRevocationList::from_der(include_bytes!( "../tests/crls/crl.valid.der" diff --git a/src/der.rs b/src/der.rs index 213d7b76..d3a53a70 100644 --- a/src/der.rs +++ b/src/der.rs @@ -26,9 +26,7 @@ pub(crate) enum Tag { OctetString = 0x04, OID = 0x06, Enum = 0x0A, - UTF8String = 0x0C, Sequence = CONSTRUCTED | 0x10, // 0x30 - Set = CONSTRUCTED | 0x11, // 0x31 UTCTime = 0x17, GeneralizedTime = 0x18, diff --git a/src/end_entity.rs b/src/end_entity.rs index a8619390..eeba34db 100644 --- a/src/end_entity.rs +++ b/src/end_entity.rs @@ -257,3 +257,59 @@ impl<'a> EndEntityCert<'a> { subject_name::list_cert_dns_names(self) } } + +#[cfg(feature = "alloc")] +#[cfg(test)] +mod tests { + use super::*; + use crate::test_utils; + + // This test reproduces https://github.com/rustls/webpki/issues/167 --- an + // end-entity cert where the common name is a `PrintableString` rather than + // a `UTF8String` cannot iterate over its subject alternative names. + #[test] + fn printable_string_common_name() { + const DNS_NAME: &str = "test.example.com"; + + let issuer = test_utils::make_issuer("Test", None); + + let ee_cert_der = { + let mut params = rcgen::CertificateParams::new(vec![DNS_NAME.to_string()]); + // construct a certificate that uses `PrintableString` as the + // common name value, rather than `UTF8String`. + params.distinguished_name.push( + rcgen::DnType::CommonName, + rcgen::DnValue::PrintableString("example.com".to_string()), + ); + params.is_ca = rcgen::IsCa::ExplicitNoCa; + params.alg = test_utils::RCGEN_SIGNATURE_ALG; + let cert = rcgen::Certificate::from_params(params) + .expect("failed to make ee cert (this is a test bug)"); + cert.serialize_der_with_signer(&issuer) + .expect("failed to serialize signed ee cert (this is a test bug)") + }; + + expect_dns_name(&ee_cert_der, DNS_NAME); + } + + // This test reproduces https://github.com/rustls/webpki/issues/167 --- an + // end-entity cert where the common name is an empty SEQUENCE. + #[test] + fn empty_sequence_common_name() { + // handcrafted cert DER produced using `ascii2der`, since `rcgen` is + // unwilling to generate this particular weird cert. + let ee_cert_der = include_bytes!("../tests/misc/empty_sequence_common_name.der").as_slice(); + expect_dns_name(ee_cert_der, "example.com"); + } + + fn expect_dns_name(der: &[u8], name: &str) { + let cert = + EndEntityCert::try_from(der).expect("should parse end entity certificate correctly"); + + let mut names = cert + .dns_names() + .expect("should get all DNS names correctly for end entity cert"); + assert_eq!(names.next().map(<&str>::from), Some(name)); + assert_eq!(names.next().map(<&str>::from), None); + } +} diff --git a/src/error.rs b/src/error.rs index b129c836..a40b562e 100644 --- a/src/error.rs +++ b/src/error.rs @@ -13,6 +13,7 @@ // OR IN CONNECTION WITH THE USE OR PERFORMANCE OF THIS SOFTWARE. use core::fmt; +use core::ops::ControlFlow; /// An error that occurs during certificate validation or name validation. #[derive(Clone, Copy, Debug, PartialEq, Eq)] @@ -93,6 +94,15 @@ pub enum Error { /// invalid labels. MalformedNameConstraint, + /// The maximum number of name constraint comparisons has been reached. + MaximumNameConstraintComparisonsExceeded, + + /// The maximum number of internal path building calls has been reached. Path complexity is too great. + MaximumPathBuildCallsExceeded, + + /// The path search was terminated because it became too deep. + MaximumPathDepthExceeded, + /// The maximum number of signature checks has been reached. Path complexity is too great. MaximumSignatureChecksExceeded, @@ -185,51 +195,80 @@ impl Error { pub(crate) fn rank(&self) -> u32 { match &self { // Errors related to certificate validity - Error::CertNotValidYet | Error::CertExpired => 27, - Error::CertNotValidForName => 26, - Error::CertRevoked => 25, - Error::InvalidCrlSignatureForPublicKey | Error::InvalidSignatureForPublicKey => 24, - Error::SignatureAlgorithmMismatch => 23, - Error::RequiredEkuNotFound => 22, - Error::NameConstraintViolation => 21, - Error::PathLenConstraintViolated => 20, - Error::CaUsedAsEndEntity | Error::EndEntityUsedAsCa => 19, - Error::IssuerNotCrlSigner => 18, + Error::CertNotValidYet | Error::CertExpired => 290, + Error::CertNotValidForName => 280, + Error::CertRevoked => 270, + Error::InvalidCrlSignatureForPublicKey | Error::InvalidSignatureForPublicKey => 260, + Error::SignatureAlgorithmMismatch => 250, + Error::RequiredEkuNotFound => 240, + Error::NameConstraintViolation => 230, + Error::PathLenConstraintViolated => 220, + Error::CaUsedAsEndEntity | Error::EndEntityUsedAsCa => 210, + Error::IssuerNotCrlSigner => 200, // Errors related to supported features used in an invalid way. - Error::InvalidCertValidity => 17, - Error::InvalidNetworkMaskConstraint => 16, - Error::InvalidSerialNumber => 15, - Error::InvalidCrlNumber => 14, + Error::InvalidCertValidity => 190, + Error::InvalidNetworkMaskConstraint => 180, + Error::InvalidSerialNumber => 170, + Error::InvalidCrlNumber => 160, // Errors related to unsupported features. Error::UnsupportedCrlSignatureAlgorithmForPublicKey - | Error::UnsupportedSignatureAlgorithmForPublicKey => 13, - Error::UnsupportedCrlSignatureAlgorithm | Error::UnsupportedSignatureAlgorithm => 12, - Error::UnsupportedCriticalExtension => 11, - Error::UnsupportedCertVersion => 11, - Error::UnsupportedCrlVersion => 10, - Error::UnsupportedDeltaCrl => 9, - Error::UnsupportedIndirectCrl => 8, - Error::UnsupportedRevocationReason => 7, + | Error::UnsupportedSignatureAlgorithmForPublicKey => 150, + Error::UnsupportedCrlSignatureAlgorithm | Error::UnsupportedSignatureAlgorithm => 140, + Error::UnsupportedCriticalExtension => 130, + Error::UnsupportedCertVersion => 130, + Error::UnsupportedCrlVersion => 120, + Error::UnsupportedDeltaCrl => 110, + Error::UnsupportedIndirectCrl => 100, + Error::UnsupportedRevocationReason => 90, + // Reserved for webpki 0.102.0+ usages: + // Error::UnsupportedRevocationReasonsPartitioning => 80, + // Error::UnsupportedCrlIssuingDistributionPoint => 70, + Error::MaximumPathDepthExceeded => 61, // Errors related to malformed data. - Error::MalformedDnsIdentifier => 6, - Error::MalformedNameConstraint => 5, - Error::MalformedExtensions => 4, - Error::ExtensionValueInvalid => 3, + Error::MalformedDnsIdentifier => 60, + Error::MalformedNameConstraint => 50, + Error::MalformedExtensions => 40, + Error::ExtensionValueInvalid => 30, // Generic DER errors. - Error::BadDerTime => 2, - Error::BadDer => 1, + Error::BadDerTime => 20, + Error::BadDer => 10, - // Special case error - not subject to ranking. + // Special case errors - not subject to ranking. Error::MaximumSignatureChecksExceeded => 0, + Error::MaximumPathBuildCallsExceeded => 0, + Error::MaximumNameConstraintComparisonsExceeded => 0, // Default catch all error - should be renamed in the future. Error::UnknownIssuer => 0, } } + + /// Returns true for errors that should be considered fatal during path building. Errors of + /// this class should halt any further path building and be returned immediately. + #[inline] + pub(crate) fn is_fatal(&self) -> bool { + matches!( + self, + Error::MaximumSignatureChecksExceeded + | Error::MaximumPathBuildCallsExceeded + | Error::MaximumNameConstraintComparisonsExceeded + ) + } +} + +impl From for ControlFlow { + fn from(value: Error) -> Self { + match value { + // If an error is fatal, we've exhausted the potential for continued search. + err if err.is_fatal() => Self::Break(err), + // Otherwise we've rejected one candidate chain, but may continue to search for others. + err => Self::Continue(err), + } + } } impl fmt::Display for Error { diff --git a/src/lib.rs b/src/lib.rs index 617b43bf..b6229db9 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -60,6 +60,10 @@ mod x509; #[allow(deprecated)] pub use trust_anchor::{TlsClientTrustAnchors, TlsServerTrustAnchors}; + +#[cfg(test)] +pub(crate) mod test_utils; + pub use { cert::{Cert, EndEntityOrCa}, crl::{BorrowedCertRevocationList, BorrowedRevokedCert, CertRevocationList, RevocationReason}, diff --git a/src/signed_data.rs b/src/signed_data.rs index 0b9856eb..d7d5da4d 100644 --- a/src/signed_data.rs +++ b/src/signed_data.rs @@ -12,9 +12,13 @@ // ACTION OF CONTRACT, NEGLIGENCE OR OTHER TORTIOUS ACTION, ARISING OUT OF // OR IN CONNECTION WITH THE USE OR PERFORMANCE OF THIS SOFTWARE. +use crate::verify_cert::Budget; use crate::{der, Error}; use ring::signature; +#[cfg(feature = "alloc")] +use alloc::vec::Vec; + /// 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. @@ -152,7 +156,10 @@ pub(crate) fn verify_signed_data( supported_algorithms: &[&SignatureAlgorithm], spki_value: untrusted::Input, signed_data: &SignedData, + budget: &mut Budget, ) -> Result<(), Error> { + budget.consume_signature()?; + // We need to verify the signature in `signed_data` using the public key // in `public_key`. In order to know which *ring* signature verification // algorithm to use, we need to know the public key algorithm (ECDSA, @@ -498,7 +505,8 @@ mod tests { signed_data::verify_signed_data( SUPPORTED_ALGORITHMS_IN_TESTS, spki_value, - &signed_data + &signed_data, + &mut Budget::default() ) ); } @@ -795,6 +803,7 @@ mod tests { } } + use crate::verify_cert::Budget; use alloc::str::Lines; fn read_pem_section(lines: &mut Lines, section_name: &str) -> Vec { diff --git a/src/subject_name/mod.rs b/src/subject_name/mod.rs index 33d64c66..5d2058be 100644 --- a/src/subject_name/mod.rs +++ b/src/subject_name/mod.rs @@ -33,6 +33,4 @@ pub use ip_address::IpAddr; mod verify; #[cfg(feature = "alloc")] pub(super) use verify::list_cert_dns_names; -pub(super) use verify::{ - check_name_constraints, verify_cert_subject_name, SubjectCommonNameContents, -}; +pub(super) use verify::{check_name_constraints, verify_cert_subject_name}; diff --git a/src/subject_name/verify.rs b/src/subject_name/verify.rs index ba6ff31d..9a882fd2 100644 --- a/src/subject_name/verify.rs +++ b/src/subject_name/verify.rs @@ -19,7 +19,9 @@ use super::{ }; use crate::{ cert::{Cert, EndEntityOrCa}, - der, Error, + der, + verify_cert::Budget, + Error, }; #[cfg(feature = "alloc")] use { @@ -36,7 +38,6 @@ pub(crate) fn verify_cert_dns_name( iterate_names( Some(cert.subject), cert.subject_alt_name, - SubjectCommonNameContents::Ignore, Err(Error::CertNotValidForName), &mut |name| { if let GeneralName::DnsName(presented_id) = name { @@ -70,7 +71,6 @@ pub(crate) fn verify_cert_subject_name( // only against Subject Alternative Names. None, cert.inner().subject_alt_name, - SubjectCommonNameContents::Ignore, Err(Error::CertNotValidForName), &mut |name| { if let GeneralName::IpAddress(presented_id) = name { @@ -88,7 +88,7 @@ pub(crate) fn verify_cert_subject_name( pub(crate) fn check_name_constraints( input: Option<&mut untrusted::Reader>, subordinate_certs: &Cert, - subject_common_name_contents: SubjectCommonNameContents, + budget: &mut Budget, ) -> Result<(), Error> { let input = match input { Some(input) => input, @@ -115,13 +115,13 @@ pub(crate) fn check_name_constraints( iterate_names( Some(child.subject), child.subject_alt_name, - subject_common_name_contents, Ok(()), &mut |name| { check_presented_id_conforms_to_constraints( name, permitted_subtrees, excluded_subtrees, + budget, ) }, )?; @@ -141,11 +141,13 @@ fn check_presented_id_conforms_to_constraints( name: GeneralName, permitted_subtrees: Option, excluded_subtrees: Option, + budget: &mut Budget, ) -> NameIteration { match check_presented_id_conforms_to_constraints_in_subtree( name, Subtrees::PermittedSubtrees, permitted_subtrees, + budget, ) { stop @ NameIteration::Stop(..) => { return stop; @@ -157,6 +159,7 @@ fn check_presented_id_conforms_to_constraints( name, Subtrees::ExcludedSubtrees, excluded_subtrees, + budget, ) } @@ -170,6 +173,7 @@ fn check_presented_id_conforms_to_constraints_in_subtree( name: GeneralName, subtrees: Subtrees, constraints: Option, + budget: &mut Budget, ) -> NameIteration { let mut constraints = match constraints { Some(constraints) => untrusted::Reader::new(constraints), @@ -182,6 +186,10 @@ fn check_presented_id_conforms_to_constraints_in_subtree( let mut has_permitted_subtrees_mismatch = false; while !constraints.at_end() { + if let Err(e) = budget.consume_name_constraint_comparison() { + return NameIteration::Stop(Err(e)); + } + // http://tools.ietf.org/html/rfc5280#section-4.2.1.10: "Within this // profile, the minimum and maximum fields are not used with any name // forms, thus, the minimum MUST be zero, and maximum MUST be absent." @@ -298,16 +306,9 @@ enum NameIteration { Stop(Result<(), Error>), } -#[derive(Clone, Copy)] -pub(crate) enum SubjectCommonNameContents { - DnsName, - Ignore, -} - fn iterate_names<'names>( subject: Option>, subject_alt_name: Option>, - subject_common_name_contents: SubjectCommonNameContents, result_if_never_stopped_early: Result<(), Error>, f: &mut impl FnMut(GeneralName<'names>) -> NameIteration, ) -> Result<(), Error> { @@ -337,20 +338,7 @@ fn iterate_names<'names>( }; } - if let (SubjectCommonNameContents::DnsName, Some(subject)) = - (subject_common_name_contents, subject) - { - match common_name(subject) { - Ok(Some(cn)) => match f(GeneralName::DnsName(cn)) { - NameIteration::Stop(result) => result, - NameIteration::KeepGoing => result_if_never_stopped_early, - }, - Ok(None) => result_if_never_stopped_early, - Err(err) => Err(err), - } - } else { - result_if_never_stopped_early - } + result_if_never_stopped_early } #[cfg(feature = "alloc")] @@ -363,7 +351,6 @@ pub(crate) fn list_cert_dns_names<'names>( iterate_names( Some(cert.subject), cert.subject_alt_name, - SubjectCommonNameContents::DnsName, Ok(()), &mut |name| { if let GeneralName::DnsName(presented_id) = name { @@ -436,23 +423,3 @@ impl<'a> GeneralName<'a> { }) } } - -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_and_get_value(tagged, der::Tag::OID)?; - if name_oid == COMMON_NAME { - return der::expect_tag_and_get_value(tagged, der::Tag::UTF8String).map(Some); - } else { - // discard unused name value - der::read_tag_and_get_value(tagged)?; - } - } - Ok(None) - }) - }) -} diff --git a/src/test_utils.rs b/src/test_utils.rs new file mode 100644 index 00000000..65dcfd7a --- /dev/null +++ b/src/test_utils.rs @@ -0,0 +1,36 @@ +/// Signature algorithm used by certificates generated using `make_issuer` and +/// `make_end_entity`. This is exported as a constant so that tests can use the +/// same algorithm when generating certificates using `rcgen`. +#[cfg(feature = "alloc")] +pub(crate) static RCGEN_SIGNATURE_ALG: &rcgen::SignatureAlgorithm = &rcgen::PKCS_ECDSA_P256_SHA256; + +#[cfg(feature = "alloc")] +pub(crate) fn make_issuer( + org_name: impl Into, + name_constraints: Option, +) -> rcgen::Certificate { + let mut ca_params = rcgen::CertificateParams::new(Vec::new()); + ca_params + .distinguished_name + .push(rcgen::DnType::OrganizationName, org_name); + ca_params.is_ca = rcgen::IsCa::Ca(rcgen::BasicConstraints::Unconstrained); + ca_params.key_usages = vec![ + rcgen::KeyUsagePurpose::KeyCertSign, + rcgen::KeyUsagePurpose::DigitalSignature, + rcgen::KeyUsagePurpose::CrlSign, + ]; + ca_params.alg = RCGEN_SIGNATURE_ALG; + ca_params.name_constraints = name_constraints; + rcgen::Certificate::from_params(ca_params).unwrap() +} + +#[cfg(feature = "alloc")] +pub(crate) fn make_end_entity(issuer: &rcgen::Certificate) -> Vec { + let mut ee_params = rcgen::CertificateParams::new(vec!["example.com".to_string()]); + ee_params.is_ca = rcgen::IsCa::ExplicitNoCa; + ee_params.alg = RCGEN_SIGNATURE_ALG; + rcgen::Certificate::from_params(ee_params) + .unwrap() + .serialize_der_with_signer(issuer) + .unwrap() +} diff --git a/src/verify_cert.rs b/src/verify_cert.rs index 1b428337..9fcd280e 100644 --- a/src/verify_cert.rs +++ b/src/verify_cert.rs @@ -12,6 +12,9 @@ // ACTION OF CONTRACT, NEGLIGENCE OR OTHER TORTIOUS ACTION, ARISING OUT OF // OR IN CONNECTION WITH THE USE OR PERFORMANCE OF THIS SOFTWARE. +use core::default::Default; +use core::ops::ControlFlow; + use crate::{ cert::{Cert, EndEntityOrCa}, der, signed_data, subject_name, time, CertRevocationList, Error, SignatureAlgorithm, @@ -27,7 +30,10 @@ pub(crate) struct ChainOptions<'a> { } pub(crate) fn build_chain(opts: &ChainOptions, cert: &Cert, time: time::Time) -> Result<(), Error> { - build_chain_inner(opts, cert, time, 0, &mut 0_usize) + build_chain_inner(opts, cert, time, 0, &mut Budget::default()).map_err(|e| match e { + ControlFlow::Break(err) => err, + ControlFlow::Continue(err) => err, + }) } fn build_chain_inner( @@ -35,8 +41,8 @@ fn build_chain_inner( cert: &Cert, time: time::Time, sub_ca_count: usize, - signatures: &mut usize, -) -> Result<(), Error> { + budget: &mut Budget, +) -> Result<(), ControlFlow> { let used_as_ca = used_as_ca(&cert.ee_or_ca); check_issuer_independent_properties(cert, time, used_as_ca, sub_ca_count, opts.eku.inner)?; @@ -48,8 +54,7 @@ fn build_chain_inner( const MAX_SUB_CA_COUNT: usize = 6; if sub_ca_count >= MAX_SUB_CA_COUNT { - // TODO(XXX): Candidate for a more specific error - Error::PathTooDeep? - return Err(Error::UnknownIssuer); + return Err(Error::MaximumPathDepthExceeded.into()); } } UsedAsCa::No => { @@ -57,52 +62,39 @@ fn build_chain_inner( } } - // for the purpose of name constraints checking, only end-entity server certificates - // could plausibly have a DNS name as a subject commonName that could contribute to - // path validity - let subject_common_name_contents = if opts - .eku - .inner - .key_purpose_id_equals(EKU_SERVER_AUTH.oid_value) - && used_as_ca == UsedAsCa::No - { - subject_name::SubjectCommonNameContents::DnsName - } else { - subject_name::SubjectCommonNameContents::Ignore - }; - let result = loop_while_non_fatal_error( Error::UnknownIssuer, opts.trust_anchors, |trust_anchor: &TrustAnchor| { let trust_anchor_subject = untrusted::Input::from(trust_anchor.subject); if cert.issuer != trust_anchor_subject { - return Err(Error::UnknownIssuer); + return Err(Error::UnknownIssuer.into()); } - let name_constraints = trust_anchor.name_constraints.map(untrusted::Input::from); - - untrusted::read_all_optional(name_constraints, Error::BadDer, |value| { - subject_name::check_name_constraints(value, cert, subject_common_name_contents) - })?; - // TODO: check_distrust(trust_anchor_subject, trust_anchor_spki)?; - check_signatures( + check_signed_chain( opts.supported_sig_algs, cert, trust_anchor, opts.crls, - signatures, + budget, )?; + check_signed_chain_name_constraints(cert, trust_anchor, budget)?; + Ok(()) }, ); let err = match result { Ok(()) => return Ok(()), - Err(err) => err, + // Fatal errors should halt further path building. + res @ Err(ControlFlow::Break(_)) => return res, + // Non-fatal errors should be carried forward as the default_error for subsequent + // loop_while_non_fatal_error processing and only returned once all other path-building + // options have been exhausted. + Err(ControlFlow::Continue(err)) => err, }; loop_while_non_fatal_error(err, opts.intermediate_certs, |cert_der| { @@ -110,7 +102,7 @@ fn build_chain_inner( Cert::from_der(untrusted::Input::from(cert_der), EndEntityOrCa::Ca(cert))?; if potential_issuer.subject != cert.issuer { - return Err(Error::UnknownIssuer); + return Err(Error::UnknownIssuer.into()); } // Prevent loops; see RFC 4158 section 5.2. @@ -119,7 +111,7 @@ fn build_chain_inner( if potential_issuer.spki.value() == prev.spki.value() && potential_issuer.subject == prev.subject { - return Err(Error::UnknownIssuer); + return Err(Error::UnknownIssuer.into()); } match &prev.ee_or_ca { EndEntityOrCa::EndEntity => { @@ -131,36 +123,29 @@ fn build_chain_inner( } } - untrusted::read_all_optional(potential_issuer.name_constraints, Error::BadDer, |value| { - subject_name::check_name_constraints(value, cert, subject_common_name_contents) - })?; - let next_sub_ca_count = match used_as_ca { UsedAsCa::No => sub_ca_count, UsedAsCa::Yes => sub_ca_count + 1, }; - build_chain_inner(opts, &potential_issuer, time, next_sub_ca_count, signatures) + budget.consume_build_chain_call()?; + build_chain_inner(opts, &potential_issuer, time, next_sub_ca_count, budget) }) } -fn check_signatures( +fn check_signed_chain( supported_sig_algs: &[&SignatureAlgorithm], cert_chain: &Cert, trust_anchor: &TrustAnchor, crls: &[&dyn CertRevocationList], - signatures: &mut usize, -) -> Result<(), Error> { + budget: &mut Budget, +) -> Result<(), ControlFlow> { let mut spki_value = untrusted::Input::from(trust_anchor.spki); let mut issuer_subject = untrusted::Input::from(trust_anchor.subject); let mut issuer_key_usage = None; // TODO(XXX): Consider whether to track TrustAnchor KU. let mut cert = cert_chain; loop { - *signatures += 1; - if *signatures > 100 { - return Err(Error::MaximumSignatureChecksExceeded); - } - signed_data::verify_signed_data(supported_sig_algs, spki_value, &cert.signed_data)?; + signed_data::verify_signed_data(supported_sig_algs, spki_value, &cert.signed_data, budget)?; if !crls.is_empty() { check_crls( @@ -170,6 +155,7 @@ fn check_signatures( spki_value, issuer_key_usage, crls, + budget, )?; } @@ -189,6 +175,91 @@ fn check_signatures( Ok(()) } +fn check_signed_chain_name_constraints( + cert_chain: &Cert, + trust_anchor: &TrustAnchor, + budget: &mut Budget, +) -> Result<(), ControlFlow> { + let mut cert = cert_chain; + let mut name_constraints = trust_anchor + .name_constraints + .as_ref() + .map(|der| untrusted::Input::from(der)); + + loop { + untrusted::read_all_optional(name_constraints, Error::BadDer, |value| { + subject_name::check_name_constraints(value, cert, budget) + })?; + + match &cert.ee_or_ca { + EndEntityOrCa::Ca(child_cert) => { + name_constraints = cert.name_constraints; + cert = child_cert; + } + EndEntityOrCa::EndEntity => { + break; + } + } + } + + Ok(()) +} + +pub struct Budget { + signatures: usize, + build_chain_calls: usize, + name_constraint_comparisons: usize, +} + +impl Budget { + #[inline] + pub(crate) fn consume_signature(&mut self) -> Result<(), Error> { + self.signatures = self + .signatures + .checked_sub(1) + .ok_or(Error::MaximumSignatureChecksExceeded)?; + Ok(()) + } + + #[inline] + fn consume_build_chain_call(&mut self) -> Result<(), Error> { + self.build_chain_calls = self + .build_chain_calls + .checked_sub(1) + .ok_or(Error::MaximumPathBuildCallsExceeded)?; + Ok(()) + } + + #[inline] + pub(crate) fn consume_name_constraint_comparison(&mut self) -> Result<(), Error> { + self.name_constraint_comparisons = self + .name_constraint_comparisons + .checked_sub(1) + .ok_or(Error::MaximumNameConstraintComparisonsExceeded)?; + Ok(()) + } +} + +impl Default for Budget { + fn default() -> Self { + Self { + // This limit is taken from the remediation for golang CVE-2018-16875. However, + // note that golang subsequently implemented AKID matching due to this limit + // being hit in real applications (see ). + // So this may actually be too aggressive. + signatures: 100, + + // This limit is taken from NSS libmozpkix, see: + // + build_chain_calls: 200_000, + + // This limit is taken from golang crypto/x509's default, see: + // + name_constraint_comparisons: 250_000, + } + } +} + // Zero-sized marker type representing positive assertion that revocation status was checked // for a certificate and the result was that the certificate is not revoked. struct CertNotRevoked(()); @@ -207,6 +278,7 @@ fn check_crls( issuer_spki: untrusted::Input, issuer_ku: Option, crls: &[&dyn CertRevocationList], + budget: &mut Budget, ) -> Result, Error> { assert_eq!(cert.issuer, issuer_subject); @@ -222,7 +294,7 @@ fn check_crls( // TODO(XXX): consider whether we can refactor so this happens once up-front, instead // of per-lookup. // https://github.com/rustls/webpki/issues/81 - crl.verify_signature(supported_sig_algs, issuer_spki.as_slice_less_safe()) + crl.verify_signature(supported_sig_algs, issuer_spki.as_slice_less_safe(), budget) .map_err(crl_signature_err)?; // Verify that if the issuer has a KeyUsage bitstring it asserts cRLSign. @@ -499,8 +571,8 @@ impl KeyUsageMode { fn loop_while_non_fatal_error( default_error: Error, values: V, - mut f: impl FnMut(V::Item) -> Result<(), Error>, -) -> Result<(), Error> + mut f: impl FnMut(V::Item) -> Result<(), ControlFlow>, +) -> Result<(), ControlFlow> where V: IntoIterator, { @@ -508,82 +580,244 @@ where for v in values { match f(v) { Ok(()) => return Ok(()), - err @ Err(Error::MaximumSignatureChecksExceeded) => return err, - Err(new_error) => error = error.most_specific(new_error), + // Fatal errors should halt further looping. + res @ Err(ControlFlow::Break(_)) => return res, + // Non-fatal errors should be ranked by specificity and only returned + // once all other path-building options have been exhausted. + Err(ControlFlow::Continue(new_error)) => error = error.most_specific(new_error), } } - Err(error) + Err(error.into()) } #[cfg(test)] mod tests { use super::*; + #[cfg(feature = "alloc")] + use crate::test_utils::{make_end_entity, make_issuer}; + #[test] fn eku_key_purpose_id() { assert!(ExtendedKeyUsage::RequiredIfPresent(EKU_SERVER_AUTH) .key_purpose_id_equals(EKU_SERVER_AUTH.oid_value)) } + #[cfg(feature = "alloc")] + enum TrustAnchorIsActualIssuer { + Yes, + No, + } + + #[cfg(feature = "alloc")] + fn build_degenerate_chain( + intermediate_count: usize, + trust_anchor_is_actual_issuer: TrustAnchorIsActualIssuer, + budget: Option, + ) -> ControlFlow { + let ca_cert = make_issuer("Bogus Subject", None); + let ca_cert_der = ca_cert.serialize_der().unwrap(); + + let mut intermediates = Vec::with_capacity(intermediate_count); + let mut issuer = ca_cert; + for _ in 0..intermediate_count { + let intermediate = make_issuer("Bogus Subject", None); + let intermediate_der = intermediate.serialize_der_with_signer(&issuer).unwrap(); + intermediates.push(intermediate_der); + issuer = intermediate; + } + + if let TrustAnchorIsActualIssuer::No = trust_anchor_is_actual_issuer { + intermediates.pop(); + } + + verify_chain( + &ca_cert_der, + &intermediates, + &make_end_entity(&issuer), + budget, + ) + .unwrap_err() + } + #[test] #[cfg(feature = "alloc")] fn test_too_many_signatures() { - use crate::ECDSA_P256_SHA256; - use crate::{EndEntityCert, Time}; + assert!(matches!( + build_degenerate_chain(5, TrustAnchorIsActualIssuer::Yes, None), + ControlFlow::Break(Error::MaximumSignatureChecksExceeded) + )); + } - let alg = &rcgen::PKCS_ECDSA_P256_SHA256; - - let make_issuer = || { - let mut ca_params = rcgen::CertificateParams::new(Vec::new()); - ca_params - .distinguished_name - .push(rcgen::DnType::OrganizationName, "Bogus Subject"); - ca_params.is_ca = rcgen::IsCa::Ca(rcgen::BasicConstraints::Unconstrained); - ca_params.key_usages = vec![ - rcgen::KeyUsagePurpose::KeyCertSign, - rcgen::KeyUsagePurpose::DigitalSignature, - rcgen::KeyUsagePurpose::CrlSign, - ]; - ca_params.alg = alg; - rcgen::Certificate::from_params(ca_params).unwrap() - }; + #[test] + #[cfg(feature = "alloc")] + fn test_too_many_path_calls() { + assert!(matches!( + build_degenerate_chain( + 10, + TrustAnchorIsActualIssuer::No, + Some(Budget { + // Crafting a chain that will expend the build chain calls budget without + // first expending the signature checks budget is tricky, so we artificially + // inflate the signature limit to make this test easier to write. + signatures: usize::MAX, + ..Budget::default() + }) + ), + ControlFlow::Break(Error::MaximumPathBuildCallsExceeded) + )); + } - let ca_cert = make_issuer(); + #[cfg(feature = "alloc")] + fn build_linear_chain(chain_length: usize) -> Result<(), ControlFlow> { + let ca_cert = make_issuer(format!("Bogus Subject {chain_length}"), None); let ca_cert_der = ca_cert.serialize_der().unwrap(); - let mut intermediates = Vec::with_capacity(101); + let mut intermediates = Vec::with_capacity(chain_length); let mut issuer = ca_cert; - for _ in 0..101 { - let intermediate = make_issuer(); + for i in 0..chain_length { + let intermediate = make_issuer(format!("Bogus Subject {i}"), None); let intermediate_der = intermediate.serialize_der_with_signer(&issuer).unwrap(); intermediates.push(intermediate_der); issuer = intermediate; } - let mut ee_params = rcgen::CertificateParams::new(vec!["example.com".to_string()]); - ee_params.is_ca = rcgen::IsCa::ExplicitNoCa; - ee_params.alg = alg; - let ee_cert = rcgen::Certificate::from_params(ee_params).unwrap(); - let ee_cert_der = ee_cert.serialize_der_with_signer(&issuer).unwrap(); + verify_chain( + &ca_cert_der, + &intermediates, + &make_end_entity(&issuer), + None, + ) + } + + #[test] + #[cfg(feature = "alloc")] + fn longest_allowed_path() { + assert!(build_linear_chain(1).is_ok()); + assert!(build_linear_chain(2).is_ok()); + assert!(build_linear_chain(3).is_ok()); + assert!(build_linear_chain(4).is_ok()); + assert!(build_linear_chain(5).is_ok()); + assert!(build_linear_chain(6).is_ok()); + } - let anchors = &[TrustAnchor::try_from_cert_der(&ca_cert_der).unwrap()]; + #[test] + #[cfg(feature = "alloc")] + fn path_too_long() { + assert!(matches!( + build_linear_chain(7), + Err(ControlFlow::Continue(Error::MaximumPathDepthExceeded)) + )); + } + + #[test] + #[cfg(feature = "alloc")] + fn name_constraint_budget() { + // Issue a trust anchor that imposes name constraints. The constraint should match + // the end entity certificate SAN. + let ca_cert = make_issuer( + "Constrained Root", + Some(rcgen::NameConstraints { + permitted_subtrees: vec![rcgen::GeneralSubtree::DnsName(".com".into())], + excluded_subtrees: vec![], + }), + ); + let ca_cert_der = ca_cert.serialize_der().unwrap(); + + // Create a series of intermediate issuers. We'll only use one in the actual built path, + // helping demonstrate that the name constraint budget is not expended checking certificates + // that are not part of the path we compute. + const NUM_INTERMEDIATES: usize = 5; + let mut intermediates = Vec::with_capacity(NUM_INTERMEDIATES); + for i in 0..NUM_INTERMEDIATES { + intermediates.push(make_issuer(format!("Intermediate {i}"), None)); + } + + // Each intermediate should be issued by the trust anchor. + let mut intermediates_der = Vec::with_capacity(NUM_INTERMEDIATES); + for intermediate in &intermediates { + intermediates_der.push(intermediate.serialize_der_with_signer(&ca_cert).unwrap()); + } + + // Create an end-entity cert that is issued by the last of the intermediates. + let ee_cert = make_end_entity(intermediates.last().unwrap()); + + // We use a custom budget to make it easier to write a test, otherwise it is tricky to + // stuff enough names/constraints into the potential chains while staying within the path + // depth limit and the build chain call limit. + let passing_budget = Budget { + // One comparison against the intermediate's distinguished name. + // One comparison against the EE's distinguished name. + // One comparison against the EE's SAN. + // = 3 total comparisons. + name_constraint_comparisons: 3, + ..Budget::default() + }; + + // Validation should succeed with the name constraint comparison budget allocated above. + // This shows that we're not consuming budget on unused intermediates: we didn't budget + // enough comparisons for that to pass the overall chain building. + assert!(verify_chain( + &ca_cert_der, + &intermediates_der, + &ee_cert, + Some(passing_budget), + ) + .is_ok()); + + let failing_budget = Budget { + // See passing_budget: 2 comparisons is not sufficient. + name_constraint_comparisons: 2, + ..Budget::default() + }; + // Validation should fail when the budget is smaller than the number of comparisons performed + // on the validated path. This demonstrates we properly fail path building when too many + // name constraint comparisons occur. + let result = verify_chain( + &ca_cert_der, + &intermediates_der, + &ee_cert, + Some(failing_budget), + ); + + assert!(matches!( + result, + Err(ControlFlow::Break( + Error::MaximumNameConstraintComparisonsExceeded + )) + )); + } + + #[cfg(feature = "alloc")] + fn verify_chain( + trust_anchor_der: &[u8], + intermediates_der: &[Vec], + ee_cert_der: &[u8], + budget: Option, + ) -> Result<(), ControlFlow> { + use crate::ECDSA_P256_SHA256; + use crate::{EndEntityCert, Time}; + + let anchors = &[TrustAnchor::try_from_cert_der(trust_anchor_der).unwrap()]; let time = Time::from_seconds_since_unix_epoch(0x1fed_f00d); - let cert = EndEntityCert::try_from(&ee_cert_der[..]).unwrap(); - let intermediates_der: Vec<&[u8]> = intermediates.iter().map(|x| x.as_ref()).collect(); - let intermediate_certs: &[&[u8]] = intermediates_der.as_ref(); + let cert = EndEntityCert::try_from(ee_cert_der).unwrap(); + let intermediates_der = intermediates_der + .iter() + .map(|x| x.as_ref()) + .collect::>(); - let result = build_chain( + build_chain_inner( &ChainOptions { eku: KeyUsage::server_auth(), supported_sig_algs: &[&ECDSA_P256_SHA256], trust_anchors: anchors, - intermediate_certs, + intermediate_certs: &intermediates_der, crls: &[], }, cert.inner(), time, - ); - - assert!(matches!(result, Err(Error::MaximumSignatureChecksExceeded))); + 0, + &mut budget.unwrap_or_default(), + ) } } diff --git a/tests/better_tls.rs b/tests/better_tls.rs index 306b283b..b1718b63 100644 --- a/tests/better_tls.rs +++ b/tests/better_tls.rs @@ -15,7 +15,7 @@ pub fn path_building() { let path_building_suite = better_tls .suites .get("pathbuilding") - .expect("missing pathbuilding suite"); + .unwrap_or_else(|| panic!("missing pathbuilding suite")); for testcase in &path_building_suite.test_cases { println!("Testing path building test case {:?}", testcase.id); diff --git a/tests/custom_ekus.rs b/tests/custom_ekus.rs index 0fc094df..4a9e082c 100644 --- a/tests/custom_ekus.rs +++ b/tests/custom_ekus.rs @@ -31,7 +31,7 @@ pub fn verify_custom_eku_mdoc() { let ee = include_bytes!("misc/mdoc_eku.ee.der"); let ca = include_bytes!("misc/mdoc_eku.ca.der"); - let eku_mdoc = KeyUsage::required(&[(40 * 1) + 0, 129, 140, 93, 5, 1, 2]); + let eku_mdoc = KeyUsage::required(&[40, 129, 140, 93, 5, 1, 2]); check_cert(ee, ca, eku_mdoc, time, Ok(())); check_cert(ee, ca, KeyUsage::server_auth(), time, err); check_cert(ee, ca, eku_mdoc, time, Ok(())); diff --git a/tests/generate.py b/tests/generate.py index 3cf55546..0ec9e30b 100755 --- a/tests/generate.py +++ b/tests/generate.py @@ -312,13 +312,6 @@ def tls_server_certs(force: bool) -> None: permitted_subtrees=[x509.DNSName(".example.com")], ) - generate_tls_server_cert_test( - output, - "disallow_subject_common_name", - expected_error="NameConstraintViolation", - subject_common_name="disallowed.example.com", - excluded_subtrees=[x509.DNSName("disallowed.example.com")], - ) generate_tls_server_cert_test( output, "disallow_dns_san", @@ -353,15 +346,6 @@ def tls_server_certs(force: bool) -> None: x509.DNSName("allowed-cn.example.com"), ], ) - generate_tls_server_cert_test( - output, - "allow_dns_san_and_disallow_subject_common_name", - expected_error="NameConstraintViolation", - sans=[x509.DNSName("allowed-san.example.com")], - subject_common_name="disallowed-cn.example.com", - permitted_subtrees=[x509.DNSName("allowed-san.example.com")], - excluded_subtrees=[x509.DNSName("disallowed-cn.example.com")], - ) generate_tls_server_cert_test( output, "disallow_dns_san_and_allow_subject_common_name", diff --git a/tests/misc/empty_sequence_common_name.der b/tests/misc/empty_sequence_common_name.der new file mode 100644 index 00000000..30e4f77b Binary files /dev/null and b/tests/misc/empty_sequence_common_name.der differ diff --git a/tests/misc/empty_sequence_common_name.der.txt b/tests/misc/empty_sequence_common_name.der.txt new file mode 100644 index 00000000..dbe59d44 --- /dev/null +++ b/tests/misc/empty_sequence_common_name.der.txt @@ -0,0 +1,104 @@ +SEQUENCE { + SEQUENCE { + [0] { + INTEGER { 2 } + } + INTEGER { `7214c2cb574538a4d63af28bb25140821cd3c528` } + SEQUENCE { + # ecdsa-with-SHA256 + OBJECT_IDENTIFIER { 1.2.840.10045.4.3.2 } + } + SEQUENCE { + SET { + SEQUENCE { + # organizationUnitName + OBJECT_IDENTIFIER { 2.5.4.11 } + PrintableString { "None" } + } + } + } + SEQUENCE { + UTCTime { "230906172100Z" } + UTCTime { "330903172100Z" } + } + SEQUENCE {} + SEQUENCE { + SEQUENCE { + # ecPublicKey + OBJECT_IDENTIFIER { 1.2.840.10045.2.1 } + # secp256r1 + OBJECT_IDENTIFIER { 1.2.840.10045.3.1.7 } + } + BIT_STRING { `00` `04681e0d5d4e66ff6f0cc3a9f0e1ba6114d647e9dfc2ee23418d56ad58edfb78cfe4ec8dadf856fde5a890799753bcd2a9380896701b7c13e49ec2e097f8ee3421` } + } + [3] { + SEQUENCE { + SEQUENCE { + # keyUsage + OBJECT_IDENTIFIER { 2.5.29.15 } + BOOLEAN { TRUE } + OCTET_STRING { + BIT_STRING { b`101` } + } + } + SEQUENCE { + # extKeyUsage + OBJECT_IDENTIFIER { 2.5.29.37 } + OCTET_STRING { + SEQUENCE { + # serverAuth + OBJECT_IDENTIFIER { 1.3.6.1.5.5.7.3.1 } + # clientAuth + OBJECT_IDENTIFIER { 1.3.6.1.5.5.7.3.2 } + } + } + } + SEQUENCE { + # basicConstraints + OBJECT_IDENTIFIER { 2.5.29.19 } + BOOLEAN { TRUE } + OCTET_STRING { + SEQUENCE {} + } + } + SEQUENCE { + # subjectKeyIdentifier + OBJECT_IDENTIFIER { 2.5.29.14 } + OCTET_STRING { + OCTET_STRING { `8f8c61be7ce4c34689e2618034581e34ef88b24c` } + } + } + SEQUENCE { + # authorityKeyIdentifier + OBJECT_IDENTIFIER { 2.5.29.35 } + OCTET_STRING { + SEQUENCE { + [0 PRIMITIVE] { `c9a3daf11d035f6eab28e2ea195c677568b0adea` } + } + } + } + SEQUENCE { + # subjectAltName + OBJECT_IDENTIFIER { 2.5.29.17 } + BOOLEAN { TRUE } + OCTET_STRING { + SEQUENCE { + [2 PRIMITIVE] { "example.com" } + } + } + } + } + } + } + SEQUENCE { + # ecdsa-with-SHA256 + OBJECT_IDENTIFIER { 1.2.840.10045.4.3.2 } + } + BIT_STRING { + `00` + SEQUENCE { + INTEGER { `00f1891fc58e16c37752bf64d58999f703c8d458b1d22d8291292ce2ad87042990` } + INTEGER { `00a6f16e39c83ba001f7f6aae73aaad0aea46d596800746887fe4567a4ffe88f9b` } + } + } +} diff --git a/tests/tls_server_certs.rs b/tests/tls_server_certs.rs index e0d8f2ef..a71f0e12 100644 --- a/tests/tls_server_certs.rs +++ b/tests/tls_server_certs.rs @@ -89,16 +89,6 @@ fn additional_dns_labels() { ); } -#[test] -fn disallow_subject_common_name() { - let ee = include_bytes!("tls_server_certs/disallow_subject_common_name.ee.der"); - let ca = include_bytes!("tls_server_certs/disallow_subject_common_name.ca.der"); - assert_eq!( - check_cert(ee, ca, &[], &[]), - Err(webpki::Error::NameConstraintViolation) - ); -} - #[test] fn disallow_dns_san() { let ee = include_bytes!("tls_server_certs/disallow_dns_san.ee.der"); @@ -138,18 +128,6 @@ fn allow_dns_san_and_subject_common_name() { ); } -#[test] -fn allow_dns_san_and_disallow_subject_common_name() { - let ee = - include_bytes!("tls_server_certs/allow_dns_san_and_disallow_subject_common_name.ee.der"); - let ca = - include_bytes!("tls_server_certs/allow_dns_san_and_disallow_subject_common_name.ca.der"); - assert_eq!( - check_cert(ee, ca, &[], &[]), - Err(webpki::Error::NameConstraintViolation) - ); -} - #[test] fn disallow_dns_san_and_allow_subject_common_name() { let ee =