Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

self-signed certificate seems not to be detected if keyUsage extension is present and does not assert the keyCertSig bit #1418

Closed
drwetter opened this issue Aug 6, 2016 · 48 comments · May be fixed by #7918
Assignees

Comments

@drwetter
Copy link
Contributor

drwetter commented Aug 6, 2016

Deatils see drwetter/testssl.sh#431 (comment) (kudos to @dcooper16)

@richsalz
Copy link
Contributor

richsalz commented Aug 6, 2016

isn't that abug in the cert?

@drwetter
Copy link
Contributor Author

drwetter commented Aug 7, 2016

Am 6. August 2016 16:02:39 MESZ, schrieb Rich Salz notifications@github.com:

isn't that abug in the cert?

My expectation is self-signed certs should be detected as such, also if that extension is present.

It looks to me that this has changed between 1.0.2e and 1.0.2g.

Cheers, Dirk

Sent from my mobile. Excuse my brevity&typos+the phone's autocorrection

@richsalz
Copy link
Contributor

richsalz commented Aug 7, 2016

If you have the keyUsage extension you need the keyCertSig bit. Yes, we got more strict.

@dcooper16
Copy link
Contributor

dcooper16 commented Aug 8, 2016

I found the change that Dirk was referring to: f51e5ed.

However, I don't believe that is the crux of the issue. X.509 includes the following definitions:

  • self-issued certificate: A public-key certificate where the issuer and the subject are the same CA.

  • self-signed certificate: A special case of self-issued certificates where the private key used by the CA to sign the certificate corresponds to the public key that is certified within the certificate.

    Note - Use of self-issued certificates and self-signed certificates issued by other than CAs are outside the scope of this Recommendation | International Standard.

At the moment, the check for whether a certificate is self-signed is:

    if (!X509_NAME_cmp(X509_get_subject_name(x), X509_get_issuer_name(x))) {
        x->ex_flags |= EXFLAG_SI;
        /* If SKID matches AKID also indicate self signed */
        if (X509_check_akid(x, x->akid) == X509_V_OK &&
            !ku_reject(x, KU_KEY_CERT_SIGN))
            x->ex_flags |= EXFLAG_SS;
}

So, the code checks that the certificate appears to be self-issued, but then, rather than checking that "the private key used by the CA to sign the certificate corresponds to the public key that is certified within the certificate," the code uses indirect means to try to determine whether the certificate is self-signed.

Rather than the indirect approach, why not just check whether "the private key used by the CA to sign the certificate corresponds to the public key that is certified within the certificate"? This may not be the correct code, but it seems that something close to the following would do it:

    if (!X509_NAME_cmp(X509_get_subject_name(x), X509_get_issuer_name(x))) {
        x->ex_flags |= EXFLAG_SI;
        if (((pkey = X509_get0_pubkey(x)) != NULL) && (X509_verify(x, pkey) > 0))
            x->ex_flags |= EXFLAG_SS;
        else
            ERR_clear_error();
    }

It seems that if the above test passes, then the certificate is self-signed.

I understand that, even though not required by X.509 or RFC 5280, many people believe that if a self-signed certificate that is being used as a source of trust anchor information contains constraints that those constraints should be respected, and if one were to follow that, the I guess one would not use a self-signed certificate that contains a keyUsage extension that does not assert keyCertSign as a trust anchor for path validation, but that still wouldn't mean the certificate is not self-signed.

Also, not that back in 2011 there were extensive discussions in the IETF on both the PKIX and DANE mail lists about the ides of self-signed end-entity certificates. The certificate for the self-signed.badssl.com website would seem to be an example of what they were referring to as as self-signed end-entity certificate; a certificate is self-signed, but where the public key is not intended to be used to sign certificates (other than the self-signed certificate itself). I think most people would believe that such a certificate should not assert the keyCertSign bit in keyUsage since relying parties should not use the public key to verify signatures on certificates.

@algv
Copy link

algv commented Apr 18, 2018

For self-signed certificate chain length =1. In RFC 5280, 6.1.3. Basic Certificate Processing:

If i is not equal to n, continue by performing the preparatory steps
listed in Section 6.1.4. If i is equal to n, perform the wrap-up
steps listed in Section 6.1.5.

Ok, we have i equal to n. And no need goto 6.1.4:

Section 6.1.4:

(n) If a key usage extension is present, verify that the
keyCertSign bit is set.

So, no need check keyCertSign bit for self-sign. I think need revert f51e5ed

keyCertSign must check only in check_ca(const X509 *x)

algv added a commit to Trusted-eSign/esign that referenced this issue Apr 19, 2018
@DDvO
Copy link
Contributor

DDvO commented Dec 17, 2018

I confirm @algv 's analysis that the current behavior requiring that keyCertSign is allowed (explicitly, or implicitly by absence of the keyUsage extension) for self-signed end entity certificates is not correct.

A further reason is that if it was correct, this would force self-signed EE certs to have the cA bit set (as demanded in https://tools.ietf.org/html/rfc5280#section-4.2.1.3). Since moreover for successful verification self-signed certs need to be put in the trust store, this would imply the very dangerous situation that these certs could be misinterpreted and misused as (root) CA certs.

@levitte
Copy link
Member

levitte commented Dec 17, 2018

Section 4.2.1.3 says this, among other things:

If the keyUsage extension is present, then the subject public key
MUST NOT be used to verify signatures on certificates or CRLs unless
the corresponding keyCertSign or cRLSign bit is set.

Isn't that what the code you talk about reflects?

@DDvO
Copy link
Contributor

DDvO commented Dec 17, 2018

Good point, so RFC 5280 is effectively inconsistent in its requirements on the interpretation of keyCertSign. As resolution I prefer the requirements given by the algorithm in https://tools.ietf.org/html/rfc5280#section-6.1 because this avoids the dangerous situation I mentioned above.

@DDvO
Copy link
Contributor

DDvO commented Dec 17, 2018

BTW, I recently came across this issue because some developer's self-signed TLS server test cert did not successfully verify using, e.g., openssl verify -check_ss_sig -CAfile cert.pem cert.pem. Since the error reported is error 20 at 0 depth lookup: unable to get local issuer certificate is pretty misleading it took us a while to find out that actually the problem was the (missing but expected) keyCertSign key usage.

Workarounds are

  • to add the keyCertSign key usage, yet this leads to other issues (see above),
  • to leave out the keyUsage extension (which disables all basic key usage restrictions), or
  • to set the -partial_chain CLI option or X509_V_FLAG_PARTIAL_CHAIN, which is not intuitive at least.

Also other folks have been having headache with this issue since more than two years - see, e.g.,

@levitte
Copy link
Member

levitte commented Dec 17, 2018

Oh, now I see what's going on... yeah, x509v3_cache_extensions is not the right place for this sort of check. That function is really just meant to cache information it finds in diverse extensions for easy access by other functions, and that includes flags.

I agree f51e5ed should be reverted. Thanks for the hint about check_ca.

@levitte levitte self-assigned this Dec 17, 2018
@levitte
Copy link
Member

levitte commented Dec 17, 2018

Hmmm, check_ca doesn't know if it's getting checked as part of a chain or not, so that doesn't quite seem to be the right spot either...

@DDvO
Copy link
Contributor

DDvO commented Dec 17, 2018

I see no need to adapt check_ca(). KU_KEY_CERT_SIGN is already (correctly) checked there and in X509_check_issued().

For a fix it should suffice to remove its use again in this context:

        if (X509_check_akid(x, x->akid) == X509_V_OK &&
            !ku_reject(x, KU_KEY_CERT_SIGN))
            x->ex_flags |= EXFLAG_SS;

Here is an invocation stack of the too strick checki:

x509v3_cache_extensions() at v3_purp.c:475 0x7ffffee6ef0e	
X509_check_purpose() at v3_purp.c:83 0x7ffffee6e019	
cert_self_signed() at x509_vfy.c:115 0x7ffffee4b513	
build_chain() at x509_vfy.c:2,864 0x7ffffee50ae4	
verify_chain() at x509_vfy.c:218 0x7ffffee4b7e3	
X509_verify_cert() at x509_vfy.c:295 0x7ffffee4bb4d	

@DDvO
Copy link
Contributor

DDvO commented Dec 17, 2018

Yet it would be nice if check_ca() somehow resulted in X509_V_ERR_KEYUSAGE_NO_CERTSIGN in case the cA flag is set while KU_KEY_CERT_SIGN is prohibited by the key usage.

Edit: I just realized that this already achieved in X509_check_issued().

@DDvO
Copy link
Contributor

DDvO commented Dec 18, 2018

On the other hand, there is no error message in case keyCertSign is explicitly asserted and the cA bit is not set. RFC 5280 demands:

If the keyCertSign bit is asserted, then the cA bit in the basic
constraints extension (Section 4.2.1.9) MUST also be asserted.

While this does not directly require throwing an error if a cert does not fulfill this, maybe it would be helpful if a respective error message was produced as a hint on non-conforming certs.

@levitte
Copy link
Member

levitte commented Dec 18, 2018

So does this modification of check_ca seem about right?

diff --git a/crypto/x509v3/v3_purp.c b/crypto/x509v3/v3_purp.c
index 2080adbe67..7bab28ffe2 100644
--- a/crypto/x509v3/v3_purp.c
+++ b/crypto/x509v3/v3_purp.c
@@ -524,8 +524,13 @@ static void x509v3_cache_extensions(X509 *x)
 static int check_ca(const X509 *x)
 {
     /* keyUsage if present should allow cert signing */
-    if (ku_reject(x, KU_KEY_CERT_SIGN))
-        return 0;
+    if (x->ex_flags & EXFLAG_KUSAGE) {
+        if (ku_reject(x, KU_KEY_CERT_SIGN))
+            return 0;
+        /* CA bit in basicConstraints must also be set */
+        if (!(x->ex_flags & EXFLAG_BCONS) || !(x->ex_flags & EXFLAG_CA))
+            return 0;
+    }
     if (x->ex_flags & EXFLAG_BCONS) {
         if (x->ex_flags & EXFLAG_CA)
             return 1;

@DDvO
Copy link
Contributor

DDvO commented Dec 18, 2018

I consider this change an improvement since it will result in X509_V_ERR_INVALID_CA being thrown by check_chain_extensions() in case a CA cert is required. Yet it in this case (and in cases where a CA is not required) it does not give a more precise hint, namely to state the inconsistency in the cert that the cA boolean is missing. Imprecise error diagnostics makes it needlessly hard for users to debug certs or their usage.

@DDvO
Copy link
Contributor

DDvO commented Dec 18, 2018

I hope that in addition you will remove && !ku_reject(x, KU_KEY_CERT_SIGN)) from

        if (X509_check_akid(x, x->akid) == X509_V_OK &&
            !ku_reject(x, KU_KEY_CERT_SIGN))
            x->ex_flags |= EXFLAG_SS;

which fixes the too strict behavior introduced in commit f51e5ed, as you indicated earlier?

@levitte
Copy link
Member

levitte commented Dec 18, 2018

I hope that in addition you will remove && !ku_reject(x, KU_KEY_CERT_SIGN))

Yes

@levitte
Copy link
Member

levitte commented Dec 18, 2018

Yet it in this case (and in cases where a CA is not required) it does not give a more precise hint, namely to state the inconsistency in the cert that the cA boolean is missing.

That's not check_ca's job, really. Its job is to answer the question "is this a suitable CA"...

@chrisvdb
Copy link

chrisvdb commented Jan 12, 2020

It would be good if this thread has some guidance for the average user on how to spot the issue and work around it. In particular, it looks like development certs on dotnet core 3.1 on Linux might be affected by this issue.

When I run dotnet dev-certs https to generate a local self-signed development certificate the file 5B747D173E76080F5B380FEF9B56400CE0FC7284.pfx is generated in ~/.dotnet/corefx/cryptography/x509stores/my. When I do openssl pkcs12 -in 5B747D173E76080F5B380FEF9B56400CE0FC7284.pfx -nokeys -out publiccert.pem -nodes to convert this to PEM and then openssl verify publiccert.pem I get:

$ openssl verify publiccert.pem
CN = localhost
error 20 at 0 depth lookup: unable to get local issuer certificate
error publiccert.pem: verification failed

This the cert information:

$ openssl x509 -in publiccert.pem -text
Certificate:
    Data:
        Version: 3 (0x2)
        Serial Number: 1664239978563708184 (0x17189171674b2518)
        Signature Algorithm: sha256WithRSAEncryption
        Issuer: CN = localhost
        Validity
            Not Before: Jan 12 06:44:21 2020 GMT
            Not After : Jan 11 06:44:21 2021 GMT
        Subject: CN = localhost
        Subject Public Key Info:
            Public Key Algorithm: rsaEncryption
                RSA Public-Key: (2048 bit)
                Modulus: <snip>
                Exponent: 65537 (0x10001)
        X509v3 extensions:
            X509v3 Basic Constraints: critical
                CA:FALSE
            X509v3 Key Usage: critical
                Digital Signature, Key Encipherment
            X509v3 Extended Key Usage: critical
                TLS Web Server Authentication
            X509v3 Subject Alternative Name: critical
                DNS:localhost
            1.3.6.1.4.1.311.84.1.1: 
                .
    Signature Algorithm: sha256WithRSAEncryption
        <snip>
-----BEGIN CERTIFICATE-----
<snip>
-----END CERTIFICATE-----

Is the way that dotnet generates self-signed dev certs incompatible with openssl?

Is this the correct config to generate a self-signed cert that avoids these issues?

[req]
default_bits       = 2048
default_keyfile    = localhost.key
distinguished_name = req_distinguished_name
req_extensions     = req_ext
x509_extensions    = v3_ca

[req_distinguished_name]
commonName                  = Common Name (e.g. server FQDN or YOUR name)
commonName_default          = localhost
commonName_max              = 64

[req_ext]
subjectAltName = @alt_names

[v3_ca]
subjectAltName = @alt_names
basicConstraints = critical, CA:false
keyUsage = keyCertSign, cRLSign, digitalSignature,keyEncipherment

[alt_names]
DNS.1   = localhost
DNS.2   = 127.0.0.1
$ openssl version
OpenSSL 1.1.1c  28 May 2019

I have summarized the dotnet on Ubuntu dev cert issue and workaround on https://stackoverflow.com/a/59702094/3167480, please comment in case the solution is incorrect or not optimal so I can update the answer.

@DDvO
Copy link
Contributor

DDvO commented Jan 16, 2020

@chrisvdb, thanks for your comments.
The use case and the workaround you describe in https://stackoverflow.com/questions/55485511/how-to-run-dotnet-dev-certs-https-trust/59702094#59702094 matches the issue discussed here.

You just should have noted above that before callling openssl verify publiccert.pem the same self-signed cert has been added to the trusted store - otherwise verification clearly must fail.

As observed earlier, the problem is that if the self-signed cert has an explicit list of keyUsages that does not include keyCertSign the verification (needlessly) fails.

This will be cured in #10587 but unfortunately that PR is currently stuck due to unfinished reviews.

DDvO added a commit to siemens/openssl that referenced this issue Jun 30, 2020
…k_issued()

Move check that cert signing is allowed from x509v3_cache_extensions() to
where it belongs: internal_verify(), generalize it for proxy cert signing.
Correct and simplify check_issued(), now checking self-issued (not: self-signed).
Add test case to 25-test_verify.t that demonstrates successful fix

Fixes openssl#1418
@DDvO
Copy link
Contributor

DDvO commented Jul 1, 2020

Reopening this because the solution still needs to back-ported to v1.1.1.

@DDvO DDvO reopened this Jul 1, 2020
DDvO added a commit to siemens/openssl that referenced this issue Jul 13, 2020
…k_issued()

Move check that cert signing is allowed from x509v3_cache_extensions() to
where it belongs: internal_verify(), generalize it for proxy cert signing.
Correct and simplify check_issued(), now checking self-issued (not: self-signed).
Add test case to 25-test_verify.t that demonstrates successful fix.

As prerequisites, this adds the static function check_sig_alg_match()
and the internal functions x509_likely_issued() and x509_signing_allowed().

This is a backport of the core of PR openssl#10587.
Fixes openssl#1418
openssl-machine pushed a commit that referenced this issue Jul 16, 2020
…k_issued()

Move check that cert signing is allowed from x509v3_cache_extensions() to
where it belongs: internal_verify(), generalize it for proxy cert signing.
Correct and simplify check_issued(), now checking self-issued (not: self-signed).
Add test case to 25-test_verify.t that demonstrates successful fix.

As prerequisites, this adds the static function check_sig_alg_match()
and the internal functions x509_likely_issued() and x509_signing_allowed().

This is a backport of the core of PR #10587.
Fixes #1418

Reviewed-by: Tomas Mraz <tmraz@fedoraproject.org>
(Merged from #12357)
@DDvO
Copy link
Contributor

DDvO commented Jul 18, 2020

@drwetter, @chrisvdb, et al.
Finally, the fix for this issue is merged in both the OpenSSL master branch (to be released as version 3.0 in Q4 of 2020)
and OpenSSL_1_1_1-stable (to be released with the next letter release, 1.1.1h).

@DDvO DDvO closed this as completed Jul 18, 2020
MikaelSmith pushed a commit to MikaelSmith/kots that referenced this issue Jul 24, 2020
Adds `TLSCACert`, `TLSCertFromCA`, and `TLSKeyFromCA` to generate a
named CA cert/key pair and access the cert, as well as generate cert/key
pairs from that CA as needed (based on the combination of CA name, cert
name, and common name).

Useful when you need a separate CA certificate, such as working around
openssl/openssl#1418.

Fixes replicatedhq#844.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
10 participants