The certificate was misidentified as a self-signed certificate

bangcheng commented 2 years ago

I created a secondary certificate(server.crt) signed by a CA certificate. In the old version of openssl, it can be correctly verified using command openssl verify -CAfile ca.crt server.crt. But when I upgrade openssl to the latest version, the signature verification command ends with an error：

error 18 at 0 depth lookup: self signed certificate
error server.crt: verification failed

By looking at the verification process, I found that this problem was caused by the change in the judgment logic of the self-signed certificate. The code is modified by this commit.

    /* latest code */
    if (!X509_NAME_cmp(X509_get_subject_name(x), X509_get_issuer_name(x))) {
        x->ex_flags |= EXFLAG_SI; /* cert is self-issued */
        if (X509_check_akid(x, x->akid) == X509_V_OK /* SKID matches AKID */
                /* .. and the signature alg matches the PUBKEY alg: */
                && check_sig_alg_match(X509_get0_pubkey(x), x) == X509_V_OK)
            x->ex_flags |= EXFLAG_SS; /* indicate self-signed */
    }

    /*  code before modification*/
    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;
    }

The content of issuer and subject name in my certificate is the same. There are no akid and skid fields in the certificate, the function X509_check_akid(x, x->akid) return X509_V_OK when x->akid = NULL. And the certificate is not used for certificate signing, the KU_KEY_CERT_SIGN not set in certificate. In older openssl versions, this certificate is correctly recognized as a non-self-signed certificate. But now the certificate is misidentified as a self-signed certificate, causing the verification to fail.

For some reason I can't remake the certificate. Is there any way to modify the openssl code so that the certificate can be recognized correctly.

t8m commented 2 years ago

You could add the && !ku_reject(x, KU_KEY_CERT_SIGN) I suppose. There are some self-signed certificates out there which do not pass this check and that's why it was removed but for your own purposes you can do this modification of the code.

bangcheng commented 2 years ago

For the reasons you mentioned, I can't add && !ku_reject(x, KU_KEY_CERT_SIGN) directly.

The current code will cause some certificates lacking akid extensions fields to not be recognized correctly. I think this is a bug that should be fixed.

As described in issues 1418 , self-signed certificate is the 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. For this definition, why not use (pkey = X509_get0_pubkey(x)) != NULL) && (X509_verify(x, pkey) > 0) directly to determine whether the certificate is a self-signed certificate?

t8m commented 2 years ago

I do not think this is a bug. The chain building routine cannot be perfect on the false negative side because it has to be fast. You simply should NOT use the same subject DN as the issuer DN and not have proper akid. Doing the explicit X509_verify() call can be expensive so it is not a good idea to do it in the chain building routine.

@DDvO, @vdukhovni might add some comments.

DDvO commented 2 years ago

As @t8m, wrote, the recognition of self-signed certs is just approximative because the actual self-signature verification is considered too costly.

Your server.crt is broken for two reasons:

its issuer equals the subject although it is not meant to be a root CA cert nor a self-signed directly trusted cert
the AKID extension is missing, which makes it non-comforming to RFC 5280. Section 4.2.1.1 says:

The keyIdentifier field of the authorityKeyIdentifier extension MUST be included in all certificates generated by conforming CAs to facilitate certification path construction.

You just have been lucky formerly as the cert path construction went fine simply because the check for (apparent) self-signedness involved key usage restrictions, which turned out to be inadequate in general.

The simplest workaround should be to directly trust this cert (by putting it in the trust store). Likely a verification callback function that overrides X509_V_ERR_DEPTH_ZERO_SELF_SIGNED_CERT will not help here.

bangcheng commented 2 years ago

Thank you for your detailed explanation of this issue.

To solve the problem of misidentification, can I add X509_verify(x, pkey) for myself when akid extension is missing? In this way, the speed of chain building will not decrease in the scenario where the certificate is reasonable. And there will be no certificate exclusion problems caused by !ku_reject(x, KU_KEY_CERT_SIGN).

    if (!X509_NAME_cmp(X509_get_subject_name(x), X509_get_issuer_name(x))) {
        x->ex_flags |= EXFLAG_SI; /* cert is self-issued */
        if (x->akid == NULL && pkey = X509_get0_pubkey(x)) != NULL) && (X509_verify(x, pkey) > 0)
                x->ex_flags |= EXFLAG_SS;
        if (X509_check_akid(x, x->akid) == X509_V_OK /* SKID matches AKID */
                /* .. and the signature alg matches the PUBKEY alg: */
                && check_sig_alg_match(X509_get0_pubkey(x), x) == X509_V_OK)
            x->ex_flags |= EXFLAG_SS; /* indicate self-signed */
    }

DDvO commented 2 years ago

Thank you for your detailed explanation of this issue.

You are welcome.

To solve the problem of misidentification, can I add X509_verify(x, pkey) for myself when akid extension is missing? In this way, the speed of chain building will not decrease in the scenario where the certificate is reasonable. And there will be no certificate exclusion problems caused by !ku_reject(x, KU_KEY_CERT_SIGN).

Very good thought! I even propose to do this generally in OpenSSL. I just would do the call X509_verify() a little differently than suggested above:

    if (X509_NAME_cmp(X509_get_subject_name(x), X509_get_issuer_name(x)) == 0) {
        EVP_PKEY *pkey;

        x->ex_flags |= EXFLAG_SI; /* Cert is self-issued */
        if (x->akid == NULL && (pkey = X509_get0_pubkey(x)) != NULL) {
            if (X509_verify(x, pkey) > 0) /* real self-signature check */
                x->ex_flags |= EXFLAG_SS;
        } else if (X509_check_akid(x, x->akid) == X509_V_OK /* SKID matches AKID */
                   /* .. and the signature alg matches the PUBKEY alg: */
                   && check_sig_alg_match(X509_get0_pubkey(x), x) == X509_V_OK) {
            x->ex_flags |= EXFLAG_SS; /* indicate self-signed */ 
        }
    }

bangcheng commented 2 years ago

@DDvO I put the modifications you provided into the source code, but I get an error when I run the test case. This shows that a test case is not getting the expected value.

[  817s] ../test/recipes/80-test_dtlsv1listen.t ............. ok
[  821s] ../test/recipes/80-test_ocsp.t .....................
[  821s] Dubious, test returned 1 (wstat 256, 0x100)
[  821s] Failed 1/11 subtests
[  821s] ../test/recipes/80-test_pkcs12.t ................... ok
[  845s] ../test/recipes/80-test_ssl_new.t .................. ok

....

[  886s] Test Summary Report
[  886s] -------------------
[  886s] ../test/recipes/80-test_ocsp.t                   (Wstat: 256 Tests: 11 Failed: 1)
[  886s]   Failed test:  10
[  886s]   Non-zero exit status: 1

I have no idea why the test cases don't all pass. Do the above results suggest that this modification has an effect on other mechanisms?

vdukhovni commented 2 years ago

Every change can adversely affect someone. In the case of apparently self-issued certs that are not self-signed and have no AKID, we could take a garbage-in -> garbage-out perspective.

But since the key usage check is cheap, and has been there a long time, if we add back the key usage check (in addition to the existing ones), what known issues would that cause?

That is why not check the algorithms (key vs. signature) and also the key usage bit?

DDvO commented 2 years ago

But since the key usage check is cheap, and has been there a long time, if we add back the key usage check (in addition to the existing ones), what known issues would that cause?

We removed the key usage check at this point for good reasons. See #1418.

DDvO commented 2 years ago

I have no idea why the test cases don't all pass. Do the above results suggest that this modification has an effect on other mechanisms?

I'm currently investigating how best to fix this. For both test failures the reason is the same:

80E22CE2CE7F0000:error:0200008A:rsa routines:RSA_padding_check_PKCS1_type_1:invalid padding:crypto/rsa/rsa_pk1.c:75:
80E22CE2CE7F0000:error:02000072:rsa routines:rsa_ossl_public_decrypt:padding check failed:crypto/rsa/rsa_ossl.c:599:
80E22CE2CE7F0000:error:1C880004:Provider routines:rsa_verify:RSA lib:providers/implementations/signature/rsa_sig.c:774:
80E22CE2CE7F0000:error:06880006:asn1 encoding routines:ASN1_item_verify_ctx:EVP lib:crypto/asn1/a_verify.c:217:

and it is because now the signature of some self-issued cert is checked and this cert is not actually self-signed, while on the other hand it is considered an issuer of itself by ossl_x509_likely_issued().

vdukhovni commented 2 years ago

We removed the key usage check at this point for good reasons. See #1418.

Yes, if the certificate is not a CA certificate, then self-signed need not assert a certSign key usage. But if it is a CA, it should.

So we could check the basic Constraints "CA" bit, and split the test. If a CA then require certSign in the key usage, if not a CA then just go with the current code (signature algorithm check).

Which could still leave the OP unhappy. If the cert in question shares the CA's DN, but is an EE certificate, it would look self-signed, too bad... don't do that. If the cert in question has CA:true, but no certSign in keyUsage (why?), then the OP might be happier.

In neither case would we have to perform public key operations to determine whether the certificate is self-signed.

DDvO commented 2 years ago

Yes, if the certificate is not a CA certificate, then self-signed need not assert a certSign key usage. But if it is a CA, it should.

Well, correct that a CA cert should have the certSign key usage, but we know from practice that this is not always the case. IIRC, for instancte the cert discussed in #1418 was a CA cert with a key usage not allowing certSign.

And I'm pretty sure that the cert in question is not marked as a CA, right @bangcheng? BTW, please post the contents of your cert such that we don't have to speculate about its details.

DDvO commented 2 years ago

It turns out that the failing tests involving in particular

apps/openssl verify -x509_strict -trusted test-runs/test_req/self-issued_v3_CA_no_AKID.pem -partial_chain test-runs/test_req/self-issued_v3_CA_no_AKID.pem

reveal an interesting bug in ossl_x509_likely_issued() for the corner case that its issuer and subject arguments are (semantically) the same cert and this cert is self-issued and appears self-signed (according to any included AKID and SKID and signature/key algorithm) but in fact is not self-signed. In this case, in combination with the new - actually correct - judgement that the cert is not self-signed, the chain building is mislead to form the chain [cert, cert] which then cannot be verified, which leads to the test failures mentioned above.

I fixed this by inserting in

int ossl_x509_likely_issued(X509 *issuer, X509 *subject)

after

         return X509_V_ERR_UNSPECIFIED;

the following code:

    if (issuer == subject
        || (X509_NAME_cmp(X509_get_issuer_name(issuer),
                          X509_get_issuer_name(subject)) == 0
            && ASN1_INTEGER_cmp(X509_get0_serialNumber(issuer),
                                X509_get0_serialNumber(subject)) == 0))
        /* issuer and subject are semantically the same cert */
        return (issuer->ex_flags & EXFLAG_SS) != 0
            ? X509_V_OK : X509_V_ERR_SUBJECT_ISSUER_MISMATCH;

I also enhanced the above mentioned changes to

int ossl_x509v3_cache_extensions(X509 *x)

by enclosing the X509_verify() call backup and restore of the error queue to prevent spurious entries on failing signature checks:

            ERR_set_mark();
            if (X509_verify(x, pkey) > 0) /* actual self-signature check */
                x->ex_flags |= EXFLAG_SS;
            ERR_pop_to_mark();

Now all tests pass.

I'm going to provide a PR for this soon.

vdukhovni commented 2 years ago

Before we make additional ad hoc changes to the code, I'd like to see a proposed documentation PR that clearly defines what "self-issued" and "self-signed" mean in OpenSSL, for:

Any legacy V1 CAs.
V3 CAs (IIRC these must have CA:true in basicConstraints)
V3 non-CAs

I'd also like to see some comparison against other libraries (e.g. NSS, BoringSSL, ...). In this space as much possible the emphasis should be on consistent behaviour across libraries, that complies with all reasonably sensible aspects of the specifications (occasionally violating the spec for good reasons is acceptable).

Once we have a definition, and some evidence that is not too baroque (largely matches other libraries), ... then we evaluate code relative to the definition.

[ Perhaps some day we can retire support for V1 CAs? ]

bangcheng commented 2 years ago

And I'm pretty sure that the cert in question is not marked as a CA, right @bangcheng? BTW, please post the contents of your cert such that we don't have to speculate about its details.

You are right, the cert in question is not marked as a CA.

DDvO commented 2 years ago

Before we make additional ad hoc changes to the code, I'd like to see a proposed documentation PR that clearly defines what "self-issued" and "self-signed" mean in OpenSSL, for:

Any legacy V1 CAs.

V3 CAs (IIRC these must have CA:true in basicConstraints)

V3 non-CAs

I'd also like to see some comparison against other libraries (e.g. NSS, BoringSSL, ...). In this space as much possible the emphasis should be on consistent behaviour across libraries, that complies with all reasonably sensible aspects of the specifications (occasionally violating the spec for good reasons is acceptable).

Once we have a definition, and some evidence that is not too baroque (largely matches other libraries), ... then we evaluate code relative to the definition.

[ Perhaps some day we can retire support for V1 CAs? ]

There is much more to clarify, define, document, and clean up on cert chain building and verification - see also #13780. And looks like I'm almost the only one in recent years who cares/dares to do something about this.

bangcheng commented 2 years ago

Will you merge the above commits into the master and 1.1.1 branches?

DDvO commented 2 years ago

Will you merge the above commits into the master and 1.1.1 branches?

Good that you ask, though better place such questions in the PR that I prepared for this purpose: #19114. That one still needs code review and two approvals.

DDvO commented 2 years ago

@vdukhovni wrote:

Before we make additional ad hoc changes to the code, I'd like to see a proposed documentation PR that clearly defines what "self-issued" and "self-signed" mean in OpenSSL, for:

Any legacy V1 CAs.

V3 CAs (IIRC these must have CA:true in basicConstraints)

V3 non-CAs

Concerning self-issuedness, the situation should be clear, both in general and for OpenSSL: A cert is self-issued iff (== if and only if) its issuer equals its subject: X509_NAME_cmp(X509_get_subject_name(issuer), X509_get_issuer_name(subject)) == 0. Note that this does not depend on the version nor on the type of the cert. In the OpenSSL docs, I only found an indirect definition for this, in openssl-x509.pod.

Concerning self-signedness, we have the unfortunate situation that while the term in general is clear as well: a cert is self-signed iff its public key verifies its signature (which should imply self-issuedess), in OpenSSL we typically use an approximation. A cert is considered self-signed iff

it is self-issued,
if it has an AKID of the form keyid, the value matches the SKID if present,
if it has an AKID of the form serial and/or issuer, the value(s) match the serial number and/or issuer,
the algorithm identifiers of its public key and signature match.

Note that also this is independent of the cert version (except that the presence of AKID or SKID extensions implies v3), type, and key usage constraints (while before commits 21db0c1db1ebe3fed9d464df0982ed3ef943d290 the basic key usage did play a role).

Part of this definition is given in x509_verify.pod.

BTW, in recent years apparently I was the only one who cleaned up confusion between self-issued and self-signed both in the code and the documentation and who filled in many gaps in the X.509-related documentation.

I'd also like to see some comparison against other libraries (e.g. NSS, BoringSSL, ...). In this space as much possible the emphasis should be on consistent behaviour across libraries, that complies with all reasonably sensible aspects of the specifications (occasionally violating the spec for good reasons is acceptable).

Once we have a definition, and some evidence that is not too baroque (largely matches other libraries), ... then we evaluate code relative to the definition.

Is there anyone (besides you) interested in this and having time to do this? If you plan to do this yourself, by when is this realistic?

[ Perhaps some day we can retire support for V1 CAs? ]

I do not see v1 certs as a major problem here. Also the cert(s) in question here bear v3 markings. Even if we stopped supporting v1 (CA) certs, which we should not do, as long as RFC 5280 is in effect, the issue that some (non-conforming) certs do not have SKID/AKID extensions remains.

bangcheng commented 2 years ago

Thanks a lot for your answer.

DDvO commented 1 year ago

Re-opening as I see no evidence that the issue has been solved.

tmshort commented 1 year ago

Remove the (resolved:answered) label?

openssl / openssl

The certificate was misidentified as a self-signed certificate #19095