rustls-webpki returns an error when parsing DNS names from a certificate generated using cfssl

[hawkw]

Summary

rustls-webpki's EndEntityCert::dns_names iterator returns an error when called on a certificate generated using cloudflare/cfssl and converted from PEM to DER using the openssl command-line tool. Meanwhile, Go's crypto/x509 package successfully parses the DNS names.

Both crypto/x509 and rustls-webpki do successfully validate these certificates.

Details

• rustls-webpki versions: This issue occurs with both v0.101.4 and v0.102.0-alpha-1 of rustls-webpki.
• Presence or lack of a common name: This issue occurs when a certificate is generated with or without a common name field, although v0.102.0-alpha.1 emits a different error when there is no common name (TrailingData(CommonNameOuter) rather than BadDer).

Reproduction

I've written a reproduction for this issue in https://github.com/hawkw/rustls-webpki-repro. The reproduction includes:

• a Rust program that attempts to read the cert's DNS names using rustls-webpki v0.101.4 and v0.102.0-alpha.1
• a Go program that does the same thing using the Go crypto/x509 package
• the certificates themselves
• a Bash script for generating the certificates.

I'm not really a TLS or ASN.1 expert, so please let me know if there's any additional information I can provide. Thanks!

Go program output

The Go program included in the repro emits the following output:

$ go run repro
=== Path: testdata/no-cn-test-ca1/crt.der===

--- go crypto/x509 ---

Verified valid for no-cn.test.com
Subject:
NotBefore: 2023-09-06 17:21:00 +0000 UTC
NotAfter: 2033-09-03 17:21:00 +0000 UTC

printing DNS names...
DNSNames[0]: no-cn.test.com


=== Path: testdata/cn-test-ca1/crt.der===

--- go crypto/x509 ---

Verified valid for cn.test.com
Subject: CN=cn.test.com
NotBefore: 2023-09-06 17:21:00 +0000 UTC
NotAfter: 2033-09-03 17:21:00 +0000 UTC

printing DNS names...
DNSNames[0]: cn.test.com

Rust repro output

The Rust repro emits the following output:

$ cargo run
   Compiling rustls-webpki-repro v0.1.0 (/home/eliza/Code/rustls-webpki-repro)
    Finished dev [unoptimized + debuginfo] target(s) in 0.39s
     Running `target/debug/rustls-webpki-repro`
=== Path: testdata/no-cn-test-ca1/crt.der ===

--- rustls-webpki v0.101.4 ---

Verified valid for no-cn.test.com
printing DNS names...
Error: BadDer

--- rustls-webpki v0.102.0-alpha.1 ---

Verified valid for no-cn.test.com
printing DNS names...
Error: TrailingData(CommonNameOuter)

=== Path: testdata/cn-test-ca1/crt.der ===

--- rustls-webpki v0.101.4 ---

Verified valid for cn.test.com
printing DNS names...
Error: BadDer

--- rustls-webpki v0.102.0-alpha.1 ---

Verified valid for cn.test.com
printing DNS names...
Error: BadDer

Expected output

I would expect the Rust program to instead output something more similar to the Go program, like:

$ cargo run
   Compiling rustls-webpki-repro v0.1.0 (/home/eliza/Code/rustls-webpki-repro)
    Finished dev [unoptimized + debuginfo] target(s) in 0.39s
     Running `target/debug/rustls-webpki-repro`
=== Path: testdata/no-cn-test-ca1/crt.der ===

--- rustls-webpki v0.101.4 ---

Verified valid for no-cn.test.com
printing DNS names...
dns_names[0]: no-cn.test.com

--- rustls-webpki v0.102.0-alpha.1 ---

Verified valid for no-cn.test.com
printing DNS names...
dns_names[0]: no-cn.test.com

=== Path: testdata/cn-test-ca1/crt.der ===

--- rustls-webpki v0.101.4 ---

Verified valid for cn.test.com
printing DNS names...
dns_names[0]: cn.test.com

--- rustls-webpki v0.102.0-alpha.1 ---

Verified valid for cn.test.com
printing DNS names...
dns_names[0]: cn.test.com

[cpu]

Thanks for the super detailed bug report!!! Very helpful. I think our handling of common names is broken :'(

RFC 5280 describes the common name attribute's ASN.1 as:

-- Naming attributes of type X520CommonName

id-at-commonName        AttributeType ::= { id-at 3 }

-- Naming attributes of type X520CommonName:
--   X520CommonName ::= DirectoryName (SIZE (1..ub-common-name))
--
-- Expanded to avoid parameterized type:
X520CommonName ::= CHOICE {
      teletexString     TeletexString   (SIZE (1..ub-common-name)),
      printableString   PrintableString (SIZE (1..ub-common-name)),
      universalString   UniversalString (SIZE (1..ub-common-name)),
      utf8String        UTF8String      (SIZE (1..ub-common-name)),
      bmpString         BMPString       (SIZE (1..ub-common-name)) }

Notably it's a CHOICE of several string types, yet in the function that parses common names, when we encounter id-at-commonName we unconditionally expect a utf8String:

webpki/src/subject_name/verify.rs

Lines 465 to 467 in 7e38e97

	if name_oid == COMMON_NAME {
	return der::expect_tag(tagged, der::Tag::UTF8String).map(Some);
	} else {

The crt.der test certificate from your repo has its common name encoded as a printableString, not utf8String:

$ der2ascii -i ~/Downloads/crt.der | grep "commonName" -A2
          # commonName
          OBJECT_IDENTIFIER { 2.5.4.3 }
          PrintableString { "cn.test.com" }

That appears to not be an unusual choice, e.g. a real world Let's Encrypt issued certificate does the same:

openssl s_client -connect binaryparadox.net:443 </dev/null 2>/dev/null | der2ascii -pem | grep "commonName" -A2
          # commonName
          OBJECT_IDENTIFIER { 2.5.4.3 }
          PrintableString { "R3" }
--
          # commonName
          OBJECT_IDENTIFIER { 2.5.4.3 }
          PrintableString { "binaryparadox.net" }

I don't think it makes sense to relax this code path to allow all of the whacky ASN.1 string types, but I suspect we should allow PrintableString in addition to UTF8String .

That still leaves the question of why the test certificate without a CN failed. I think the issue there is that the same problematic common_name fn in verify.rs is invoked with the empty subject SEQUENCE and we unconditionally expect to find a SET in the SEQUENCE:

webpki/src/subject_name/verify.rs

Line 455 in 7e38e97

der::Tag::Set,

tl;dr: I think there are two bugs, both with respect to handling common names. Both yield the same Error::BadDer error, but for different reasons: one for an unexpected string type and one from an unexpected empty subject sequence.

[hawkw]

@cpu thanks for the quick response! I'm happy to potentially contribute a PR to fix this, although I don't know a whole lot about ASN.1.

If I understand your comment correctly, it seems like the necessary changes would be:

1. changing common_name in verify.rs to allow any of the permitted string type tags when encountering a COMMON_NAME tag, and
2. changing common_name to permit an empty SEQUENCE

Is that correct?

[cpu]

Another question worth asking: why didn't the unit tests we added with this feature catch the issue?

We use a few test certificates:

• tests/netflix/ee.der - this one happens to have a subject common name that's encoded as a UTF8String.
• tests/misc/invalid_subject_alternative_name.der - ditto.
• tests/misc/dns_names_and_wildcards.der - this one omits the subject SEQUENCE entirely.
• tests/misc/no_subject_alternative_name.der - ditto.

So unfortunately our test cases side-stepped both bugs :'(

[cpu]

@cpu thanks for the quick response! I'm happy to potentially contribute a PR to fix this, although I don't know a whole lot about ASN.1.

@hawkw That would be great. I have a few other things on the go and probably can't work on a fix for a few days.

1. changing common_name in verify.rs to allow any of the permitted string type tags when encountering a COMMON_NAME tag

I think we would want to limit it to just UTF8String or PrintableString, the other string types are (IMO) bogus and not likely to be used in real world certificates that are well formed.

For the PrintableString case I think we should also verify the value matches the rules for what can be included in a PrintableString, it's more restrictive than UTF8String (e.g. see the isPrintable function that Go uses for this).

2. changing common_name to permit an empty SEQUENCE

Yes I think this makes sense, though I'm less confident that this isn't a bug in the encoder. e.g. maybe the encoder should be omitting the empty SEQUENCE?

As you observed Go happily parses the empty sequence (I believe that behaviour corresponds to this check in parseName) so that might be one vote in favour of handling empty subject SEQUENCEs instead of erroring.

[cpu]

I think we would want to limit it to just UTF8String or PrintableString

The CA/Browser forum's baseline requirements already require this, so that's one more reason to avoid the other types:

commonName	2.5.4.3	RFC 5280	MUST use UTF8String or PrintableString	64

[cpu]

Yes I think this makes sense, though I'm less confident that this isn't a bug in the encoder. e.g. maybe the encoder should be omitting the empty SEQUENCE?

The baseline reqs when describing the SAN extension also say:

If the subject field of the certificate is an empty SEQUENCE, this extension MUST be marked critical, as specified in RFC 5280, Section 4.2.1.6. Otherwise, this extension MUST NOT be marked critical.

That makes it sound like an empty subject SEQUENCE is expected/permitted and probably not an encoder bug. So we should support that too. Ideally also enforcing the criticality of the SAN extension, but that might be more involved.

[hawkw]

Great, thank you!

I'd like to start by adding tests reproducing the issue. I notice that the existing tests use Python scripts to generate key material and certificates. Is it necessary for all test data to be generated by Python? I'm asking because I imagine it's possible to get the Python library to generate a certificate with no COMMON_NAME, but I'm not immediately sure whether or not there's an API to get the encoder to use PrintableString rather than UTF8String`

[ctz]

I think we should consider removing all parsing of the CN. At the moment we parse it for processing name constraints, but we never use it for determining if a certificate is valid for a certain name. It doesn't seem to make sense for name constraints to constrain names we don't use?

There's also inconsistency in what EndEntityCert::dns_names returns -- I think it should only return names would pass verify_is_valid_for_subject_name.

[cpu]

Edit: oops, comment clash 😆

@hawkw Maybe hold off a little bit. After chatting with @ctz in the sidebar we're starting to wonder whether there's any value in handling the CN at all.

For the purpose of certificate validation we only rely on SANs. The common name handling code is only used for verifying name constraints, and for the name iteration code that you found the bug with. For the former, if we assume the webpki profile the subj. CN must be present as a SAN, so validating the SANs vs the constraints seems sufficient (?). For the latter, it seems potentially confusing to return a name from the CN that doesn't get used for validation (though again, the webpki profile would require that it matches a SAN value).

So maybe the better fix is to remove subj CN processing? 🤔 If so, apologies for starting you on a goose chase.

I'd like to start by adding tests reproducing the issue. I notice that the existing tests use Python scripts to generate key material and certificates. Is it necessary for all test data to be generated by Python?

We're trying to migrate away from using Python in favour of rcgen but there's a decent amount of older tests to migrate. See the bottom of verify_cert.rs for some newer example Rust-based unit tests.

I think PyCA Cryptography supports specifying the string type for a RDN but I know rcgen does for sure:

let mut cert_params = rcgen::CertificateParams::new(Vec::new());
cert_params.distinguished_name.push(DnType::CommonName, DnValue::PrintableString("Foo".into()));
...

I think the more challenging requirement will be finding a way to generate a certificate with an empty subject SEQUENCE. I'm not sure that's possible with rcgen.

Since I think we can exercise this code path without needing to do the full certificate validation dance it feels reasonable to cheat and check-in a manually crafted example. It won't need to have a valid signature, and we don't have to worry about the tests eventually failing from cert expiry. I would probably use der2ascii for hacking up an example.

[hawkw]

@hawkw Maybe hold off a little bit. After chatting with @ctz in the sidebar we're starting to wonder whether there's any value in handling the CN at all.
For the purpose of certificate validation we only rely on SANs. The common name handling code is only used for verifying name constraints, and for the name iteration code that you found the bug with. For the former, if we assume the webpki profile the subj. CN must be present as a SAN, so validating the SANs vs the constraints seems sufficient (?). For the latter, it seems potentially confusing to return a name from the CN that doesn't get used for validation (though again, the webpki profile would require that it matches a SAN value).

So maybe the better fix is to remove subj CN processing? 🤔 If so, apologies for starting you on a goose chase.

Hmm, that makes sense, I think. Fortunately, I haven't actually started on a change for CN processing, just started on introducing a test. I'll see if I can get a repro working with rcgen, and then I'll start on a fix once you and @ctz have agreed on the correct approach?

[hawkw]

Okay, I've written a failing test for an EE cert with a PrintableString CN: a53c3ee

Looks like @cpu is right, and I can't easily get rcgen to produce an empty subject SEQUENCE. Just excluding the distinguished name field entirely results in a cert that rustls-webpki does accept --- I'm guessing it produces a SEQUENCE containing a SET containing an empty SEQUENCE, instead. I can try to hand-craft one though.

[hawkw]

I've also added a test reproducing the failure in the case where the CN is an empty SEQUENCE: c075316

@cpu and @ctz, would your preference for a fix be to remove the CN parsing entirely, or to make it handle these cases correctly? I'm happy to implement either solution :)

[hawkw]

Regarding removing the CN handling completely, it looks like two of the existing tests fail with all CN handling removed:

# ... a bunch of passing tests ...

     Running tests/tls_server_certs.rs (target/debug/deps/tls_server_certs-d99324689a6599e1)

running 25 tests
# ... passing tests elided ...
test allow_dns_san_and_disallow_subject_common_name ... FAILED
test disallow_subject_common_name ... FAILED

failures:

---- allow_dns_san_and_disallow_subject_common_name stdout ----
thread 'allow_dns_san_and_disallow_subject_common_name' panicked at 'assertion failed: `(left == right)`
  left: `Ok(())`,
 right: `Err(NameConstraintViolation)`', tests/tls_server_certs.rs:150:5
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace

---- disallow_subject_common_name stdout ----
thread 'disallow_subject_common_name' panicked at 'assertion failed: `(left == right)`
  left: `Ok(())`,
 right: `Err(NameConstraintViolation)`', tests/tls_server_certs.rs:99:5

This is after fully removing all common name handling, i.e. applying the following patch:

diff --git a/src/der.rs b/src/der.rs
index 4993275..3e285e8 100644
--- a/src/der.rs
+++ b/src/der.rs
@@ -62,9 +62,9 @@ pub(crate) enum Tag {
     OctetString = 0x04,
     OID = 0x06,
     Enum = 0x0A,
-    UTF8String = 0x0C,
+    // UTF8String = 0x0C,
     Sequence = CONSTRUCTED | 0x10, // 0x30
-    Set = CONSTRUCTED | 0x11,      // 0x31
+    // Set = CONSTRUCTED | 0x11,      // 0x31
     UTCTime = 0x17,
     GeneralizedTime = 0x18,
 
diff --git a/src/subject_name/verify.rs b/src/subject_name/verify.rs
index f303e9f..5c1e1c6 100644
--- a/src/subject_name/verify.rs
+++ b/src/subject_name/verify.rs
@@ -333,20 +333,9 @@ impl<'a> Iterator for NameIterator<'a> {
             }
         }
 
-        if let Some(subject_directory_name) = self.subject_directory_name.take() {
-            return Some(Ok(GeneralName::DirectoryName(subject_directory_name)));
-        }
-
-        if let Some(subject_common_name) = self.subject_common_name.take() {
-            return match common_name(subject_common_name) {
-                Ok(Some(cn)) => Some(Ok(GeneralName::DnsName(cn))),
-                Ok(None) => None,
-                // All the iterator fields should be `None` at this point
-                Err(err) => Some(Err(err)),
-            };
-        }
-
-        None
+        self.subject_directory_name
+            .take()
+            .map(|subject_directory_name| Ok(GeneralName::DirectoryName(subject_directory_name)))
     }
 }
 
@@ -445,33 +434,3 @@ impl<'a> FromDer<'a> for GeneralName<'a> {
 
     const TYPE_ID: DerTypeId = DerTypeId::GeneralName;
 }
-
-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::TrailingData(DerTypeId::CommonNameOuter),
-        |tagged| {
-            der::nested(
-                tagged,
-                der::Tag::Sequence,
-                Error::TrailingData(DerTypeId::CommonNameInner),
-                |tagged| {
-                    while !tagged.at_end() {
-                        let name_oid = der::expect_tag(tagged, der::Tag::OID)?;
-                        if name_oid == COMMON_NAME {
-                            return der::expect_tag(tagged, der::Tag::UTF8String).map(Some);
-                        } else {
-                            // discard unused name value
-                            der::read_tag_and_get_value(tagged)?;
-                        }
-                    }
-                    Ok(None)
-                },
-            )
-        },
-    )
-}

On the other hand, if I simply change the list_cert_dns_names function to pass SubjectCommonNameContents::Ignore rather than SubjectCommonNameContents::DnsName, all tests in the repo pass. E.g., applying this patch:

diff --git a/src/subject_name/verify.rs b/src/subject_name/verify.rs
index f303e9f..141401b 100644
--- a/src/subject_name/verify.rs
+++ b/src/subject_name/verify.rs
@@ -360,7 +360,7 @@ pub(crate) fn list_cert_dns_names<'names>(
     let result = NameIterator::new(
         Some(cert.subject),
         cert.subject_alt_name,
-        SubjectCommonNameContents::DnsName,
+        SubjectCommonNameContents::Ignore,
     )
     .find_map(&mut |result| {
         let name = match result {

However, I'm not sure whether a test that tries to validate name constraints for a certificate that has a PrintableString CN would pass, since the earlier test failures seem to indicate that parsing the common name is needed to verify name constraints currently.

I'm not sure whether it's better to continue along the approach of removing the common name handling entirely, and changing the code for name constraints to also remove the use of the CN, or if we should continue with @cpu's original solution of actually fixing the CN parsing code.