rustls-webpki returns an error when parsing DNS names from a certificate generated using cfssl #167
Comments
|
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: Notably it's a webpki/src/subject_name/verify.rs Lines 465 to 467 in 7e38e97 The That appears to not be an unusual choice, e.g. a real world Let's Encrypt issued certificate does the same: 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 That still leaves the question of why the test certificate without a CN failed. I think the issue there is that the same problematic webpki/src/subject_name/verify.rs Line 455 in 7e38e97 tl;dr: I think there are two bugs, both with respect to handling common names. Both yield the same |
|
@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:
Is that correct? |
|
Another question worth asking: why didn't the unit tests we added with this feature catch the issue? We use a few test certificates:
So unfortunately our test cases side-stepped both bugs :'( |
@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.
I think we would want to limit it to just For the
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 |
The CA/Browser forum's baseline requirements already require this, so that's one more reason to avoid the other types:
|
The baseline reqs when describing the SAN extension also say:
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. |
|
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 |
|
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 |
|
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.
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 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 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. |
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? |
This commit adds a test reproducing issue rustls#167, where the `EndEntityCert::dns_names()` method returns an error incorrectly if the certificate DER encodes the common name as a `PrintableString` rather than a `UTF8String`.
|
Okay, I've written a failing test for an EE cert with a Looks like @cpu is right, and I can't easily get |
This reproduces the other issue described in rustls#167.
|
Regarding removing the CN handling completely, it looks like two of the existing tests fail with all CN handling removed: 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 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 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. |
|
I think I added the |
+1
I agree, with the broader context paged in I think it makes the most sense to remove the CN handling outright. @hawkw Thanks for your help! In addition to your diff above, I think we can probably remove the |
Yeah, I think that removing the CN handling will provide the opportunity to simplify |
Makes sense 👍 P.S.: I saw your issue on the |
Thanks! I had to write a quick nix package to install By the way, I threw together a quick dev shell flake for working in this repo (including installing Python for |
As suggested by @ctz in this comment: rustls#167 (comment).
Nice! I've cobbled together my own Flakes for most of the Rustls repos at this point (admittedly they're rough around the edges 😵). So far I've hesitated to try and upstream them because it didn't seem like anyone other than myself would use the environment and I don't have a lot of experience trying to integrate Nix/Flakes into community projects. I've also been worried about committing to continued maintenance without another Nix user to help out/review changes. If you have the cycles maybe it would be best to open a general discussion issue about the idea (here, or on the main Rustls repo)? Knowing if you're interested in helping with continued maintenance or if you can only commit to the initial development would help too (No pressure either way). |
This commit adds a test reproducing issue rustls#167, where the `EndEntityCert::dns_names()` method returns an error incorrectly if the certificate DER encodes the common name as a `PrintableString` rather than a `UTF8String`.
This reproduces the other issue described in rustls#167.
As suggested by @ctz in this comment: rustls#167 (comment).
|
Okay, I've submitted #169, which should fix this issue. I'm happy to chat a bit more about flakes later, as well. :) |
This commit adds a pair of tests reproducing issue rustls#167, where the `EndEntityCert::dns_names()` method returns an error incorrectly on some certificate DER encodings. In particular, `dns_names` fails if the CN is a `PrintableString`, or if it's an empty `SEQUENCE`, rather than a `SEQUENCE` containing an empty `SET`. The test for the `PrintableString` common name uses an end-entity certificate generated using `rcgen`, while the test for empty `SEQUENCE` CN required a hand-crafted DER using `ascii2der`. The text file that generated the `ascii2der` cert is also included.
As suggested by @ctz in this comment: rustls#167 (comment).
This commit adds a pair of tests reproducing issue rustls#167, where the `EndEntityCert::dns_names()` method returns an error incorrectly on some certificate DER encodings. In particular, `dns_names` fails if the CN is a `PrintableString`, or if it's an empty `SEQUENCE`, rather than a `SEQUENCE` containing an empty `SET`. The test for the `PrintableString` common name uses an end-entity certificate generated using `rcgen`, while the test for empty `SEQUENCE` CN required a hand-crafted DER using `ascii2der`. The text file that generated the `ascii2der` cert is also included.
As suggested by @ctz in this comment: rustls#167 (comment).
This commit adds a pair of tests reproducing issue #167, where the `EndEntityCert::dns_names()` method returns an error incorrectly on some certificate DER encodings. In particular, `dns_names` fails if the CN is a `PrintableString`, or if it's an empty `SEQUENCE`, rather than a `SEQUENCE` containing an empty `SET`. The test for the `PrintableString` common name uses an end-entity certificate generated using `rcgen`, while the test for empty `SEQUENCE` CN required a hand-crafted DER using `ascii2der`. The text file that generated the `ascii2der` cert is also included.
As suggested by @ctz in this comment: #167 (comment).
This commit adds a pair of tests reproducing issue rustls#167, where the `EndEntityCert::dns_names()` method returns an error incorrectly on some certificate DER encodings. In particular, `dns_names` fails if the CN is a `PrintableString`, or if it's an empty `SEQUENCE`, rather than a `SEQUENCE` containing an empty `SET`. The test for the `PrintableString` common name uses an end-entity certificate generated using `rcgen`, while the test for empty `SEQUENCE` CN required a hand-crafted DER using `ascii2der`. The text file that generated the `ascii2der` cert is also included.
As suggested by @ctz in this comment: rustls#167 (comment).
This commit adds a pair of tests reproducing issue rustls#167, where the `EndEntityCert::dns_names()` method returns an error incorrectly on some certificate DER encodings. In particular, `dns_names` fails if the CN is a `PrintableString`, or if it's an empty `SEQUENCE`, rather than a `SEQUENCE` containing an empty `SET`. The test for the `PrintableString` common name uses an end-entity certificate generated using `rcgen`, while the test for empty `SEQUENCE` CN required a hand-crafted DER using `ascii2der`. The text file that generated the `ascii2der` cert is also included.
As suggested by @ctz in this comment: rustls#167 (comment).
This commit adds a pair of tests reproducing issue rustls#167, where the `EndEntityCert::dns_names()` method returns an error incorrectly on some certificate DER encodings. In particular, `dns_names` fails if the CN is a `PrintableString`, or if it's an empty `SEQUENCE`, rather than a `SEQUENCE` containing an empty `SET`. The test for the `PrintableString` common name uses an end-entity certificate generated using `rcgen`, while the test for empty `SEQUENCE` CN required a hand-crafted DER using `ascii2der`. The text file that generated the `ascii2der` cert is also included.
As suggested by @ctz in this comment: rustls#167 (comment).
This picks up the upstream fix for rustls/webpki#167
This commit changes the `linkerd-meshtls-rustls` crate to use the upstream `rustls-webpki` crate, maintained by Rustls, rather than our fork of `briansmith/webpki` from GitHub. Since `rustls-webpki` includes the change which was the initial motivation for the `linkerd/webpki` fork (rustls/webpki#42), we can now depend on upstream. Currently, we must take a Git dependency on `rustls-webpki`, since a release including a fix for an issue (rustls/webpki#167) which prevents `rustls-webpki` from parsing our test certificates has not yet been published. Once v0.101.5 of `rustls-webpki` is published (PR see rustls/webpki#170), we can remove the Git dep. For now, I've updated `cargo-deny` to allow the Git dependency.
This commit changes the `linkerd-meshtls-rustls` crate to use the upstream `rustls-webpki` crate, maintained by Rustls, rather than our fork of `briansmith/webpki` from GitHub. Since `rustls-webpki` includes the change which was the initial motivation for the `linkerd/webpki` fork (rustls/webpki#42), we can now depend on upstream. Currently, we must take a Git dependency on `rustls-webpki`, since a release including a fix for an issue (rustls/webpki#167) which prevents `rustls-webpki` from parsing our test certificates has not yet been published. Once v0.101.5 of `rustls-webpki` is published (PR see rustls/webpki#170), we can remove the Git dep. For now, I've updated `cargo-deny` to allow the Git dependency. --- * use `rustls-webpki` instead of `linkerd/webpki` (linkerd/linkerd2-proxy#2465) Signed-off-by: Eliza Weisman <[email protected]>
This commit changes the `linkerd-meshtls-rustls` crate to use the upstream `rustls-webpki` crate, maintained by Rustls, rather than our fork of `briansmith/webpki` from GitHub. Since `rustls-webpki` includes the change which was the initial motivation for the `linkerd/webpki` fork (rustls/webpki#42), we can now depend on upstream. Currently, we must take a Git dependency on `rustls-webpki`, since a release including a fix for an issue (rustls/webpki#167) which prevents `rustls-webpki` from parsing our test certificates has not yet been published. Once v0.101.5 of `rustls-webpki` is published (PR see rustls/webpki#170), we can remove the Git dep. For now, I've updated `cargo-deny` to allow the Git dependency. --- * use `rustls-webpki` instead of `linkerd/webpki` (linkerd/linkerd2-proxy#2465) Signed-off-by: Eliza Weisman <[email protected]>
This commit adds a pair of tests reproducing issue #167, where the `EndEntityCert::dns_names()` method returns an error incorrectly on some certificate DER encodings. In particular, `dns_names` fails if the CN is a `PrintableString`, or if it's an empty `SEQUENCE`, rather than a `SEQUENCE` containing an empty `SET`. The test for the `PrintableString` common name uses an end-entity certificate generated using `rcgen`, while the test for empty `SEQUENCE` CN required a hand-crafted DER using `ascii2der`. The text file that generated the `ascii2der` cert is also included.
As suggested by @ctz in this comment: #167 (comment).
This commit changes the `linkerd-meshtls-rustls` crate to use the upstream `rustls-webpki` crate, maintained by Rustls, rather than our fork of `briansmith/webpki` from GitHub. Since `rustls-webpki` includes the change which was the initial motivation for the `linkerd/webpki` fork (rustls/webpki#42), we can now depend on upstream. Currently, we must take a Git dependency on `rustls-webpki`, since a release including a fix for an issue (rustls/webpki#167) which prevents `rustls-webpki` from parsing our test certificates has not yet been published. Once v0.101.5 of `rustls-webpki` is published (PR see rustls/webpki#170), we can remove the Git dep. For now, I've updated `cargo-deny` to allow the Git dependency.
This commit changes the `linkerd-meshtls-rustls` crate to use the upstream `rustls-webpki` crate, maintained by Rustls, rather than our fork of `briansmith/webpki` from GitHub. Since `rustls-webpki` includes the change which was the initial motivation for the `linkerd/webpki` fork (rustls/webpki#42), we can now depend on upstream. Currently, we must take a Git dependency on `rustls-webpki`, since a release including a fix for an issue (rustls/webpki#167) which prevents `rustls-webpki` from parsing our test certificates has not yet been published. Once v0.101.5 of `rustls-webpki` is published (PR see rustls/webpki#170), we can remove the Git dep. For now, I've updated `cargo-deny` to allow the Git dependency.
This commit changes the `linkerd-meshtls-rustls` crate to use the upstream `rustls-webpki` crate, maintained by Rustls, rather than our fork of `briansmith/webpki` from GitHub. Since `rustls-webpki` includes the change which was the initial motivation for the `linkerd/webpki` fork (rustls/webpki#42), we can now depend on upstream. Currently, we must take a Git dependency on `rustls-webpki`, since a release including a fix for an issue (rustls/webpki#167) which prevents `rustls-webpki` from parsing our test certificates has not yet been published. Once v0.101.5 of `rustls-webpki` is published (PR see rustls/webpki#170), we can remove the Git dep. For now, I've updated `cargo-deny` to allow the Git dependency. --- * use `rustls-webpki` instead of `linkerd/webpki` (linkerd/linkerd2-proxy#2465) Signed-off-by: Eliza Weisman <[email protected]>
This commit changes the `linkerd-meshtls-rustls` crate to use the upstream `rustls-webpki` crate, maintained by Rustls, rather than our fork of `briansmith/webpki` from GitHub. Since `rustls-webpki` includes the change which was the initial motivation for the `linkerd/webpki` fork (rustls/webpki#42), we can now depend on upstream. Currently, we must take a Git dependency on `rustls-webpki`, since a release including a fix for an issue (rustls/webpki#167) which prevents `rustls-webpki` from parsing our test certificates has not yet been published. Once v0.101.5 of `rustls-webpki` is published (PR see rustls/webpki#170), we can remove the Git dep. For now, I've updated `cargo-deny` to allow the Git dependency. --- * use `rustls-webpki` instead of `linkerd/webpki` (linkerd/linkerd2-proxy#2465) Signed-off-by: Eliza Weisman <[email protected]> Signed-off-by: Adam Shaw <[email protected]>
Summary
rustls-webpki'sEndEntityCert::dns_namesiterator returns an error when called on a certificate generated usingcloudflare/cfssland converted from PEM to DER using theopensslcommand-line tool. Meanwhile, Go'scrypto/x509package successfully parses the DNS names.Both
crypto/x509andrustls-webpkido successfully validate these certificates.Details
rustls-webpkiversions: This issue occurs with both v0.101.4 and v0.102.0-alpha-1 ofrustls-webpki.TrailingData(CommonNameOuter)rather thanBadDer).Reproduction
I've written a reproduction for this issue in https:/hawkw/rustls-webpki-repro. The reproduction includes:
rustls-webpkiv0.101.4 and v0.102.0-alpha.1crypto/x509packageI'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:
Rust repro output
The Rust repro emits the following output:
Expected output
I would expect the Rust program to instead output something more similar to the Go program, like:
The text was updated successfully, but these errors were encountered: