From cb22ae9544237dc9ff02ef4263f6d21dfd0516d3 Mon Sep 17 00:00:00 2001 From: Jani Monoses Date: Tue, 21 Sep 2021 15:11:06 +0300 Subject: [PATCH 1/3] Do not fail verification for V3 certs with no extensions. --- src/cert.rs | 8 +++++--- tests/cert_without_extensions.rs | 5 ++--- 2 files changed, 7 insertions(+), 6 deletions(-) diff --git a/src/cert.rs b/src/cert.rs index 75e022a1..ad84621e 100644 --- a/src/cert.rs +++ b/src/cert.rs @@ -95,12 +95,14 @@ pub(crate) fn parse_cert_internal<'a>( subject_alt_name: None, }; - // mozilla::pkix allows the extensions to be omitted. However, since - // the subjectAltName extension is mandatory, the extensions are - // mandatory too, and we enforce that. Also, mozilla::pkix includes + // mozilla::pkix allows the extensions to be omitted. It also includes // special logic for handling critical Netscape Cert Type extensions. // That has been intentionally omitted. + if tbs.at_end() { + return Ok(cert) + } + der::nested( tbs, der::Tag::ContextSpecificConstructed3, diff --git a/tests/cert_without_extensions.rs b/tests/cert_without_extensions.rs index 075f331f..d7da187f 100644 --- a/tests/cert_without_extensions.rs +++ b/tests/cert_without_extensions.rs @@ -20,8 +20,7 @@ fn cert_without_extensions_test() { // `openssl x509 -in cert_without_extensions.der -inform DER -text -noout` const CERT_WITHOUT_EXTENSIONS_DER: &[u8] = include_bytes!("cert_without_extensions.der"); - assert_eq!( - Some(webpki::Error::MissingOrMalformedExtensions), - webpki::EndEntityCert::try_from(CERT_WITHOUT_EXTENSIONS_DER).err() + assert!( + webpki::EndEntityCert::try_from(CERT_WITHOUT_EXTENSIONS_DER).is_ok() ); } From e837936b80af8edf2e7d398275c43705afcd9578 Mon Sep 17 00:00:00 2001 From: Jani Monoses Date: Sat, 25 Sep 2021 18:10:54 +0300 Subject: [PATCH 2/3] Rename error to MalformedExtensions. --- src/cert.rs | 2 +- src/error.rs | 4 ++-- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/src/cert.rs b/src/cert.rs index ad84621e..97aa0b92 100644 --- a/src/cert.rs +++ b/src/cert.rs @@ -106,7 +106,7 @@ pub(crate) fn parse_cert_internal<'a>( der::nested( tbs, der::Tag::ContextSpecificConstructed3, - Error::MissingOrMalformedExtensions, + Error::MalformedExtensions, |tagged| { der::nested_of_mut( tagged, diff --git a/src/error.rs b/src/error.rs index ae11bbc9..d3872534 100644 --- a/src/error.rs +++ b/src/error.rs @@ -75,12 +75,12 @@ pub enum Error { /// is malformed. UnsupportedCertVersion, - /// The certificate extensions are missing or malformed. + /// The certificate extensions are malformed. /// /// In particular, webpki requires the DNS name(s) be in the subjectAltName /// extension as required by the CA/Browser Forum Baseline Requirements /// and as recommended by RFC6125. - MissingOrMalformedExtensions, + MalformedExtensions, /// The certificate contains an unsupported critical extension. UnsupportedCriticalExtension, From d8f9918f6f1fb834b25f479bc6c25373e7e260cc Mon Sep 17 00:00:00 2001 From: Jani Monoses Date: Sat, 11 Dec 2021 20:18:18 +0200 Subject: [PATCH 3/3] Address PR comments. --- src/cert.rs | 56 +++++++++++++++----------------- tests/cert_without_extensions.rs | 4 +-- 2 files changed, 27 insertions(+), 33 deletions(-) diff --git a/src/cert.rs b/src/cert.rs index 97aa0b92..894a940b 100644 --- a/src/cert.rs +++ b/src/cert.rs @@ -95,38 +95,34 @@ pub(crate) fn parse_cert_internal<'a>( subject_alt_name: None, }; - // mozilla::pkix allows the extensions to be omitted. It also includes - // special logic for handling critical Netscape Cert Type extensions. - // That has been intentionally omitted. - - if tbs.at_end() { - return Ok(cert) + 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| { + let extn_id = der::expect_tag_and_get_value(extension, der::Tag::OID)?; + let critical = der::optional_boolean(extension)?; + let extn_value = + der::expect_tag_and_get_value(extension, der::Tag::OctetString)?; + match remember_extension(&mut cert, extn_id, extn_value)? { + Understood::No if critical => { + Err(Error::UnsupportedCriticalExtension) + } + _ => Ok(()), + } + }, + ) + }, + )?; } - der::nested( - tbs, - der::Tag::ContextSpecificConstructed3, - Error::MalformedExtensions, - |tagged| { - der::nested_of_mut( - tagged, - der::Tag::Sequence, - der::Tag::Sequence, - Error::BadDER, - |extension| { - let extn_id = der::expect_tag_and_get_value(extension, der::Tag::OID)?; - let critical = der::optional_boolean(extension)?; - let extn_value = - der::expect_tag_and_get_value(extension, der::Tag::OctetString)?; - match remember_extension(&mut cert, extn_id, extn_value)? { - Understood::No if critical => Err(Error::UnsupportedCriticalExtension), - _ => Ok(()), - } - }, - ) - }, - )?; - Ok(cert) }) } diff --git a/tests/cert_without_extensions.rs b/tests/cert_without_extensions.rs index d7da187f..cf31022f 100644 --- a/tests/cert_without_extensions.rs +++ b/tests/cert_without_extensions.rs @@ -20,7 +20,5 @@ fn cert_without_extensions_test() { // `openssl x509 -in cert_without_extensions.der -inform DER -text -noout` const CERT_WITHOUT_EXTENSIONS_DER: &[u8] = include_bytes!("cert_without_extensions.der"); - assert!( - webpki::EndEntityCert::try_from(CERT_WITHOUT_EXTENSIONS_DER).is_ok() - ); + assert!(webpki::EndEntityCert::try_from(CERT_WITHOUT_EXTENSIONS_DER).is_ok()); }