briansmith / webpki

WebPKI X.509 Certificate Validation in Rust
https://briansmith.org/rustdoc/webpki/
Other
464 stars 165 forks source link

Non-standard certificates that identify a DNS name (or IP address) using the CN field of the subject are rejected; provide some way to opt into supporting them. #90

Open Ralith opened 5 years ago

Ralith commented 5 years ago

f5-cert.der.gz

OpenSSL says:

Certificate:
    Data:
        Version: 3 (0x2)
        Serial Number: 1549 (0x60d)
    Signature Algorithm: sha1WithRSAEncryption
        Issuer: C=US, ST=WA, L=Seattle, O=MyCompany, OU=IT, CN=localhost.localdomain/emailAddress=root@localhost.localdomain
        Validity
            Not Before: Mar 30 14:35:40 2014 GMT
            Not After : Mar 27 14:35:40 2024 GMT
        Subject: C=US, ST=WA, L=Seattle, O=MyCompany, OU=IT, CN=localhost.localdomain/emailAddress=root@localhost.localdomain
        Subject Public Key Info:
            Public Key Algorithm: rsaEncryption
                Public-Key: (2048 bit)
                Modulus:
                    <snip>
                Exponent: 65537 (0x10001)
    Signature Algorithm: sha1WithRSAEncryption
         <snip>

certlint says: W: Name has deprecated attribute emailAddress f5-cert.der

Even stepping through webpki with a debugger it's unclear what exactly the problem is, but it's interfering slightly with QUIC interop testing, and seemingly all other impls (using BoringSSL, picoTLS, and patched OpenSSL, among others) have no trouble here. My best guess is that subjectAltName is required by webpki, but this doesn't seem to be standard behavior, or documented.

briansmith commented 5 years ago

It doesn't have the X.509v3 extensions field.

My best guess is that subjectAltName is required by webpki, but this doesn't seem to be standard behavior, or documented.

Good point that it isn't documented. webpki does require the subjectAltName, just like Chrome (https://support.google.com/chrome/a/answer/7391219?hl=en) and Firefox (https://bugzilla.mozilla.org/show_bug.cgi?id=1245280) other probably other modern browsers.

Please ask the people who generated the test certificates to fix the broken certificates.

briansmith commented 5 years ago

Also, I would be happy to review and accept a PR that calls out in the API documentation that (1) subjectAltName is required, (2) X.509v3 extensions (https://tools.ietf.org/html/rfc5280#section-4.1.2.9) are required.

I would also review a PR that provides more specific error codes, e.g. one that indicates specifically that the X.509v3 extensions field is missing and/or one that indicates that the subjectAltName extension is missing. Whether or not I would merge such a PR depends on how much complexity is added.

briansmith commented 5 years ago

This is called out in the source code here: https://github.com/briansmith/webpki/blob/f12b33991860c8f64895d3e551db4fafdfced51d/src/cert.rs#L93-L97

The place where the error happens is (probably) here: https://github.com/briansmith/webpki/blob/f12b33991860c8f64895d3e551db4fafdfced51d/src/cert.rs#L99-L100.

It looks like it would be reasonable to create a variant of der::nested_mut that takes an extra argument indicating which error value to return if the tag doesn't match. That would require adding similar variants of the underlying ring DER parsing primitives first. It seems like it wouldn't be too bad and would also help us address #53 because the solution for #53 could be done the same way.

Ralith commented 5 years ago

// the subjectAltName extension is mandatory

I did find this, but it wasn't obvious to me where the requirement arose from.

The place where the error happens is (probably) here:

That's consistent with what I found in the debugger; in particular, the expect_tag_and_get_value call fails. The outlined approaches make sense, perhaps through introducing a TagMismatch error case to the ring primitives.

briansmith commented 5 years ago

perhaps through introducing a TagMismatch error case to the ring primitives.

Yeah. Another option would be to avoid adding any new functions. Instead one could change ring so that it doesn't use error::Unspecified in its DER parser; instead it would return more specific errors, one of which would be TagMismatch. Then the webpki code could map_err that into a more specific/useful error. That would be ideal but it's also probably quite a bit of tedious work.

Demi-Marie commented 4 years ago

It is worth noting that non-web uses of X.509 do not necessarily require subjectAltName.

GopherJ commented 4 years ago

same error here, just for mqtt

Good point that it isn't documented. webpki does require the subjectAltName, just like Chrome (https://support.google.com/chrome/a/answer/7391219?hl=en) and Firefox (https://bugzilla.mozilla.org/show_bug.cgi?id=1245280) other probably other modern browsers.

I believe this doesn't make sense to non-web usage.

Demi-Marie commented 4 years ago

For those who are running into this error in non-web uses of X.509, check out https://github.com/paritytech/x509-signature. It is a low-level library that should be able to parse these certs, but you will need to do path building yourself.

mleonhard commented 4 years ago

Most versions of the openssl command line tool generate V1 certificates if you don't specify extensions. The x509-signature library rejects such certificates with UnsupportedCertVersion: https://github.com/paritytech/x509-signature/issues/2

detly commented 3 years ago

I'd like to revisit this comment:

It is worth noting that non-web uses of X.509 do not necessarily require subjectAltName.

Can I ask if this library is intended to be a web-only library or a more general TLS/SSL certificate validation library? The context is that this is used by rustls, which itself is used by other libraries, which themselves are not necessarily web related (eg. MQTT). Having an unequivocal "yes we are open to non-web downstream needs" or "no, it says Web PKI right there on the box" would be useful for making decisions about where to make necessary changes in those downstream projects.

briansmith commented 3 years ago

If we were to add support for identifying an endpoint by a (X.500) directory name then we could accept certificates without subjectAltName because we could use the subject instead. But, there is no support for identifying an endpoint by a directory name in webpki now. Currently webpki only supports DNS names, and the standards require DNS names to be in subjectAltName. In the near future I expect we'll add support for IP addresses. Those IP addresses also must be in the subjectAltName extension.

Thus, there's no certificate that webpki would ever accept that doesn't have X.509 extensions. Thus they must be v3.

Most versions of the openssl command line tool generate V1 certificates if you don't specify extensions.

That's not unreasonable of them. But when you create a certificate you almost always want to add at least a subjectAltName and probably EKU and keyUsage extensions, at a minimum. If you are not creating a certificate with at least one extension and you're not identifying the subject by an X.500 directory name then you are making a mistake. In particular, if you are generating a X.509 certificate that identifies a subject by its DNS name and/or an IP address and you don't include a subjectAltName then you are doing the wrong thing.

To be frank, the OpenSSL command line tools are optimized to help people make mistakes and do the wrong thing. It is bad that they have existed in that state for so long. The OpenSSL project does seem eager to improve the situation though, and I suggest people help them. As an alternative--probably a much easier and better alternative--use something better than the OpenSSL command line tool to create your certificates.

Can I ask if this library is intended to be a web-only library or a more general TLS/SSL certificate validation library? The context is that this is used by rustls, which itself is used by other libraries, which themselves are not necessarily web related (eg. MQTT).

If you are interested in adding MQTT support to webpki then I'd need you to file an issue that clearly and completely explains, with references to the (draft) standards, what would need to change to add standard-conformant MQTT certificates. Then if the changes don't seem too bad then we could consider adding support for those certificates. The same goes for any other standards.

Having an unequivocal "yes we are open to non-web downstream needs"

I am open to supporting more standards more broadly as long as that support's value significantly outweighs the burden it imposes on the more mainstream uses. I am less open to supporting non-standard and non-conformant stuff, but I wouldn't rule out all that stuff completely.

or "no, it says Web PKI right there on the box" would be useful for making decisions about where to make necessary changes in those downstream projects.

If the changes to support non-WebPKI things are large, and we cannot fix those non-WebPKI (draft) standards to be easier to support, then I'd consider splitting this library up into parts so that the non-WebPKI parts don't interfere with the WebPKI parts so much.

briansmith commented 3 years ago

I re-read the initial comment of this issue and I have changed the summary of this issue to "Non-standard certificates that identify a DNS name (or IP address) using the CN field of the subject are rejected; provide some way to opt into supporting them" since that is the specific problem that the initial report is trying to address.

Issue #53 is about returning a specific error that would help people see that the EE certificate is not a V3 certificate, which would allow them to find the documentation.

Issue #57 is about adding U2F support. U2F support is definitely within the scope of webpki, IMO, and I am currently biased towards adding it.

Somebody could file an issue about adding MQTT support; that doesn't belong here unless MQTT certificates wrongly use the subject CN for DNS names and/or IP addresses.

Similarly, changes needed to support other (draft) standards should be requested in separate issues, one for each

Now, back to the original issue here: Should we add support for DNS names (and/or IP addresses, after we add IP address support in issue #122) that are in the subject CN? I'm not convinced we should. I and several other people spent a lot of effort to get to the point where we could rip out such support from (browsers') X.509 libraries. We'd need a much stronger and more specific argument than "some people use bad tools to create X.509 certificates."

detly commented 3 years ago

@briansmith Thanks for the detailed response, it's extremely useful to know where to direct my efforts. I'll try to get the appropriate information for you if webpki needs to support anything additional.

Could I please ask @Demi-Marie to clarify the comment:

It is worth noting that non-web uses of X.509 do not necessarily require subjectAltName.

Specifically, are you talking about MQTT using X.509 certificates for transport encryption, or for client authentication? (Or both/neither?)

detly commented 3 years ago

@briansmith Also (mostly for my own understanding), when you say this:

In particular, if you are generating a X.509 certificate that identifies a subject by its DNS name and/or an IP address and you don't include a subjectAltName then you are doing the wrong thing.

...is this the relevant part of the relevant standard ie RFC 5280 / 4.2.1.6?

Whenever such identities are to be bound into a certificate, the subject alternative name (or issuer alternative name) extension MUST be used; however, a DNS name MAY also be represented in the subject field using the domainComponent attribute as described in Section 4.1.2.4.

briansmith commented 3 years ago

...is this the relevant part of the relevant standard ie RFC 5280 / 4.2.1.6?

Yes. Also RFC 6125

The certificate SHOULD include a "DNS-ID" if possible as a baseline for interoperability.

and

Therefore, the certificate SHOULD NOT include a CN-ID unless the certification authority issues the certificate in accordance with a specification that reuses this one and that explicitly encourages continued support for the CN-ID identifier type in the context of a given application technology.

Note the part of RFC 5280 you quoted says:

a DNS name MAY also be represented in the subject field using the domainComponent attribute as described in Section 4.1.2.4.

The domainComponent attribute is NOT the commonName attribute that is being abused here. In practice nothing supports the domainComponent attribute.

briansmith commented 3 years ago

Regarding OpenSSL, OpenSSL has made it easier to do the right thing in OpenSSL 1.1.1 and later.

Generate a PKCS#8 format RSA 2048 keypair (PKCS#8 so Rustls can use it), as documented at https://gist.github.com/briansmith/2ee42439923d8e65a266994d0f70180b:

openssl genpkey \
    -algorithm RSA \
        -pkeyopt rsa_keygen_bits:2048 \
        -pkeyopt rsa_keygen_pubexp:65537 | \
  openssl pkcs8 -topk8 -nocrypt -outform der > rsa-2048-private-key.p8

Create a CSR using that keypair with an empty subject and the domain name in the subjectAltName, without PEM encoding:

openssl req -new \
    -subj "/" \
    -addext "subjectAltName = DNS:example.com" \
    -keyform der -key rsa-2048-private-key.p8 \
    -outform der -out req.der
ghost commented 3 years ago

By the way there are web uses of X.509v1 certificates that work with any other software. For example client certificates do not require any extensions and the latest versions of nginx, firefox and chromium play well together with X.509v1 client certificates. Because of this caveat I will have to use OpenSSL bindings for now.

briansmith commented 3 years ago

By the way there are web uses of X.509v1 certificates that work with any other software. For example client certificates do not require any extensions and the latest versions of nginx, firefox and chromium play well together with X.509v1 client certificates.

That's issue #209. This issue (#90) is specifically about certificates that identify a website.