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

Improve key usage checks in internal_verify() and backport fix of issue 1418 to v1.1.1 #12357

Closed

Conversation

@DDvO
Copy link
Contributor

@DDvO DDvO commented Jul 2, 2020

@vdukhovni, this is the said backport of the core of #10587
for fixing #1418 also for OpenSSL v1.1.1.

  • documentation is added or updated
  • tests are added or updated
@DDvO DDvO changed the title Fix issue 1418 by moving check of KU_KEY_CERT_SIGN and weakening check_issued() Backport to v1.1.1: Fix issue 1418 by moving check of KU_KEY_CERT_SIGN and weakening check_issued() Jul 2, 2020
crypto/x509/x509_vfy.c Outdated
* and its depth (rather than the depth of the subject).
*/
if (xs != xi || (ctx->param->flags & X509_V_FLAG_CHECK_SS_SIGNATURE)) {
EVP_PKEY *pkey;
int issuer_depth = n + (xi == xs ? 0 : 1);
int ret = x509_signing_allowed(xi, xs);

This comment has been minimized.

@vdukhovni

vdukhovni Jul 2, 2020

Perhaps the signing-allowed check should happen only at issuer_depth > 0? Otherwise an EE-self-signed cert might fail when X509_V_FLAG_CHECK_SS_SIGNATURE is set, and the keyUsage does not include KU_KEY_CERT_SIGN. And I think the goal of this PR is allow such EE certs. I don't think that X509_V_FLAG_CHECK_SS_SIGNATURE should override that policy. Am I missing something?

This comment has been minimized.

@DDvO

DDvO Jul 3, 2020
Author Contributor

Good thought not to check key usage restrictions for root certs regardless of X509_V_FLAG_CHECK_SS_SIGNATURE.
I've done this tentatively, and all tests still pass.

Before suggesting to forward-port ;-) this to the master, I plan to check if this is RFC 5280 compliant...

This comment has been minimized.

@vdukhovni

vdukhovni Jul 3, 2020

Thanks! Indeed a forward-port to master may be appropriate. With this much more focused PR, it was easier to separate heat from chaff and notice this potential anomaly. I'll be happy to review your findings. Good luck.

This comment has been minimized.

@vdukhovni

vdukhovni Jul 3, 2020

Note that my comment wasn't broadly about self-signatures on root CA, but just about the depth 0 case. But presumably if the CA is not at depth 0 then the key usage would be checked anyway to allow it to sign the immediate subject certificate, right? If so, it may be appropriate to just skip the check for the top of chain self-signed TA regardless of the chain depth.

This comment has been minimized.

@DDvO

DDvO Jul 4, 2020
Author Contributor

Note that my comment wasn't broadly about self-signatures on root CA, but just about the depth 0 case.

Sure.

But presumably if the CA is not at depth 0 then the key usage would be checked anyway to allow it to sign the immediate subject certificate, right?

Right. I've just verified with RFC 5280 that checking the keyUsage keyCertSign is demanded during chain validation for a cert of depth n in preparation of checking cert n+1.
Quoting from https://tools.ietf.org/html/rfc5280#section-6.1.4:

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

Moreover, I understand this in a way that this step is not meant to be done if there is no such cert n+1, i.e., in our special case of a single self-signed (or at least self-issued) EE cert.
So to address my above concern of RFC 5280 compliance, I've slightly adapted the fixup to do the key usage check also for depth 0 certs in case there are further certs in the given chain.
The tests still pass on the adapted version.

This comment has been minimized.

@vdukhovni

vdukhovni Jul 4, 2020

On the other hand from the perspective of RFC5280, a trust anchor is just a public key, trust anchors in the form of self-signed certificates are somewhat outside the model. Verifying self-signatures is therefore in good measure also outside the model. I'll take a closer look at the updated code soon.

This comment has been minimized.

@DDvO

DDvO Jul 5, 2020
Author Contributor

I wrote yesterday:

Here is also an interesting further requirement of RFC 5280 regarding verifying cert signatures and key usage checks:

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

On this it looks like we have a problem that I'm currently investigating:
Adding such a tests makes a couple of tests fail.
Therefore in the tentative commit that I did pushed I've commented out the new check !x509_signing_allowed(xi, xs).

This comment has been minimized.

@DDvO

DDvO Jul 6, 2020
Author Contributor

It turns out that the mentioned problem with some tests failing was just due to a bug interpreting the result of x509_signing_allowed().

This comment has been minimized.

@DDvO

DDvO Jul 6, 2020
Author Contributor

Perhaps the signing-allowed check should happen only at issuer_depth > 0? Otherwise an EE-self-signed cert might fail when X509_V_FLAG_CHECK_SS_SIGNATURE is set, and the keyUsage does not include KU_KEY_CERT_SIGN. And I think the goal of this PR is allow such EE certs. I don't think that X509_V_FLAG_CHECK_SS_SIGNATURE should override that policy. Am I missing something?

It turns out that we must not skip the signing-allowed check because of the mentioned extra requirement in https://tools.ietf.org/html/rfc5280#section-4.2.1.3:

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

So we have to obey any given key usage restrictions.
In case of a self-issued root/EE cert at the end of the chain, what I think we can do in case cert self-signature checks being requested by X509_V_FLAG_CHECK_SS_SIGNATURE is to simply ignore this request in case the key usage prohibits verifying the (self-)signatures of such a certs. This is what I've now implemented.

Moreover I've re-phrased the existing checks and added some comments for better readability of internal_verify().

This comment has been minimized.

@DDvO

DDvO Jul 6, 2020
Author Contributor

As planned above in #12357 (comment) I've meanwhile prepared a 'forward-port' of this last commit in #12375.

@romen, note that I've followed your practice and use as the number of that related PR the value that results from swapping the last two digits 😉

@DDvO DDvO force-pushed the siemens:fix_x509_vfy_issue_1418_v111 branch Jul 4, 2020
crypto/x509/x509_vfy.c Outdated Show resolved Hide resolved
@DDvO DDvO force-pushed the siemens:fix_x509_vfy_issue_1418_v111 branch Jul 5, 2020
crypto/x509/x509_vfy.c Outdated Show resolved Hide resolved
crypto/x509/x509_vfy.c Outdated
* and its depth (rather than the depth of the subject).
* Skip signature check for self-issued certificates unless explicitly
* asked for because it does not add any security and just wastes time.
* We must not verify the signature if key usage prohibits signing.

This comment has been minimized.

@vdukhovni

vdukhovni Jul 5, 2020

The last of this comment is perhaps now out of place? And I think it meant "Signature verification must fail if...", rather than "We must not verify the signature if...".

This comment has been minimized.

@DDvO

DDvO Jul 5, 2020
Author Contributor

See my above quote form the RFC 5280, which so far had been neglected:

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

This comment has been minimized.

@DDvO

DDvO Jul 6, 2020
Author Contributor

It this clear now, together with the further extended code comments and my new comment in the above review discussion?

This comment has been minimized.

@DDvO

DDvO Jul 6, 2020
Author Contributor

This is my summary of the proposed updated commit:

If a self-issued root/EE cert is last in chain we verify its signature only if X509_V_FLAG_CHECK_SS_SIGNATURE is requested.
(This has been the semantics so far.)

What this commit changes is:
Even in this case skip the signature verification if the cert key usage prohibits signing.
Make clear that we must check key usage when verifying the signature of
the next cert and we must not verify a signature if key usage forbids it.
Add some comments for making internal_verify() easier to understand.
Update the documentation of X509_V_FLAG_CHECK_SS_SIGNATURE accordingly.

This comment has been minimized.

@vdukhovni

vdukhovni Jul 6, 2020

What this commit changes is:
Even in this case skip the signature verification if the cert key usage prohibits signing.

I am not sure that's what users of X509_V_FLAG_CHECK_SS_SIGNATURE would expect. For a self-signed EE certificate, that does not have kuCertSign (intended to be a leaf only, not a CA), a self signature check is still possible, and would likely be expected if X509_V_FLAG_CHECK_SS_SIGNATURE is set. The EE self-signed case is entirely outside the scope of RFC5280. It can only be trusted by being directly bound as fit for a particular service without any CA involvement. So we get to set the verification rules subject to reasonable user expectations and our own documentation. So I'd like to ask for this to be considered more carefully, and a brief rationale written down one way or the other...

Make clear that we must check key usage when verifying the signature of
the next cert and we must not verify a signature if key usage forbids it.
Add some comments for making internal_verify() easier to understand.
Update the documentation of X509_V_FLAG_CHECK_SS_SIGNATURE accordingly.

This comment has been minimized.

@DDvO

DDvO Jul 7, 2020
Author Contributor

The EE self-signed case is entirely outside the scope of RFC5280.

This would surprise me - can you provide a proof for this statement?
I cannot find any such scope restriction on/within RFC 5280.

This comment has been minimized.

@vdukhovni

vdukhovni Jul 7, 2020

5280 has no concept of self-signed EE certificates. It only discusses self-signed CA certificates:

   This specification covers two classes of certificates: CA
   certificates and end entity certificates.  CA certificates may be
   further divided into three classes: cross-certificates, self-issued
   certificates, and self-signed certificates.  Cross-certificates are
   CA certificates in which the issuer and subject are different
   entities.  Cross-certificates describe a trust relationship between
   the two CAs.  Self-issued certificates are CA certificates in which
   the issuer and subject are the same entity.  Self-issued certificates
   are generated to support changes in policy or operations.  Self-
   signed certificates are self-issued certificates where the digital
   signature may be verified by the public key bound into the
   certificate.  Self-signed certificates are used to convey a public
   key for use to begin certification paths.  End entity certificates
   are issued to subjects that are not authorized to issue certificates.

And yet, many users of OpenSSL deploy self-signed EE certificates
that are in no way intended to be used as CAs that issue certificates
for other subject names. Furthermore, in 5280 a self-signed CA
certificate is just a public key container. There is no process in
5280 describing verification of such certificates.

   When the trust anchor is provided in the form of a self-signed
   certificate, this self-signed certificate is not included as part of
   the prospective certification path.  Information about trust anchors
   is provided as inputs to the certification path validation algorithm
   (Section 6.1.1).

So my question is what to do about EE self-signed certs. Should the self-signature
be checked when the X509_V_FLAG_CHECK_SS_SIGNATURE flag is set? I don't
claim to have the definitive answer, but I do claim that no answer is or can be given
in RFC5280. If I'm right, then we need to decide what the right (least-surprise)
answer is for users of OpenSSL. It may well be to ignore keyUsage when verifying EE
self-signatures, and check the signature anyway, when X509_V_FLAG_CHECK_SS_SIGNATURE
is set.

This comment has been minimized.

@DDvO

DDvO Jul 7, 2020
Author Contributor

As I wrote above, the topic is not necessarily self-signed EE certs, but more generally self-issued EE certs.

And the definition of EE certs in 5280 does not refer to self-issued not self-signed. So it does note exclude either of those types of EE certs.

This comment has been minimized.

@DDvO

DDvO Jul 8, 2020
Author Contributor

Self-issued certs do not carry any explicit indication whether they are self-signed or not.
The only indication that one has is whether their key usage permits self-signing.

This comment has been minimized.

@DDvO

DDvO Jul 8, 2020
Author Contributor

In my view the X509_V_FLAG_CHECK_SS_SIGNATURE user flag should be interpreted as:

Verify the signature of the last self-issued cert in the chain as self-signature where this is appropriate.
This typically is the case for (self-signed root) CA certs,
but also for certain self-issued EE certs, namely for those where the key usage allows certificate signing.

As mentioned, there are self-issued EE certs where the type of the public key is algorithmically unsuitable for signing (for instance, pure key agreement keys such as DH), so attempting to still verify "self-signature" would fail deeply.

crypto/x509/x509_vfy.c Outdated Show resolved Hide resolved
@DDvO DDvO force-pushed the siemens:fix_x509_vfy_issue_1418_v111 branch 2 times, most recently Jul 6, 2020
@DDvO DDvO changed the title Backport to v1.1.1: Fix issue 1418 by moving check of KU_KEY_CERT_SIGN and weakening check_issued() Improve key usage checks in internal_verify() and backport fix of issue 1418 to v1.1.1 Jul 6, 2020
@DDvO DDvO force-pushed the siemens:fix_x509_vfy_issue_1418_v111 branch 2 times, most recently Jul 6, 2020
@kroeckx
Copy link
Member

@kroeckx kroeckx commented Jul 6, 2020

@DDvO
Copy link
Contributor Author

@DDvO DDvO commented Jul 7, 2020

I think I'm failing to understand something.
I expect a root CA certificate to always have the keyCertSign bit set, that without that bit, the certificate can not be used to sign certificates. One of the certificates the root CA signs is the root CA certificate itself. So if the X509_V_FLAG_CHECK_SS_SIGNATURE is set, I see no reason why we can't verify that signature.

Sure, and this normal case is not in question.

The other trust anchor we can have is a self signed certificate that is not a CA. It should not have the keyCertSign bit set, otherwise it would also need to be a CA. In theory you could say that it's not allowed to sign it's own certificate, but at the same time it needs to do it. So here I see no reason that if someone insist to check the self signed signature that we check it, it just might need special code to be able to deal with it.

Note that not all self-issued certs are self-signed.
One such interesting examples is https://tools.ietf.org/html/draft-ietf-curdle-pkix-04#section-10.2
As I just wrote a little further up, for self-issued certs a self-signature and consequently a self-signature verification is not always technically possible.
And even if so, the creator of the cert might still want to prohibit the use of the public key for cert signature verification,
and we should and can respect this.

@kroeckx
Copy link
Member

@kroeckx kroeckx commented Jul 8, 2020

@vdukhovni
Copy link

@vdukhovni vdukhovni commented Jul 8, 2020

Note that not all self-issued certs are self-signed.

Sure, but unless you're also enable partial chains, a chain ending in a self-issued, but not self-signed certificate is incomplete! Self-issued certificates need to have an authority key id that makes it clear that the certificate is not self-signed:

   The keyIdentifier field of the authorityKeyIdentifier extension MUST
   be included in all certificates generated by conforming CAs to
   facilitate certification path construction.  There is one exception;
   where a CA distributes its public key in the form of a "self-signed"
   certificate, the authority key identifier MAY be omitted.  The
   signature on a self-signed certificate is generated with the private
   key associated with the certificate's subject public key.  (This
   proves that the issuer possesses both the public and private keys.)
   In this case, the subject and authority key identifiers would be
   identical, but only the subject key identifier is needed for
   certification path building.

And in any case no one compels users to use the X509_V_FLAG_CHECK_SS_SIGNATURE flag. It is off by default. But if they ask for it, then the signature should be checked. We should not confuse the keyUsage "certSign" bit (which is a policy constraint) with whether the algorithm is suitable for signing. Surely the certificate signature was signed by a key that does support signing, and if that's the same as the algorithm of the SPKI, then the signature can be checked...

One such interesting examples is https://tools.ietf.org/html/draft-ietf-curdle-pkix-04#section-10.2
As I just wrote a little further up, for self-issued certs a self-signature and consequently a self-signature verification is not always technically possible.
And even if so, the creator of the cert might still want to prohibit the use of the public key for cert signature verification,
and we should and can respect this.

Not if they present a single-element chain with a self-signed EE key and ask for signature verification.

@DDvO
Copy link
Contributor Author

@DDvO DDvO commented Jul 11, 2020

Note that not all self-issued certs are self-signed.

Sure, but unless you're also enable partial chains, a chain ending in a self-issued, but not self-signed certificate is incomplete!

Yes. For such a cert directly trusting it would not be sufficient.

Self-issued certificates need to have an authority key id that makes it clear that the certificate is not self-signed:

   The keyIdentifier field of the authorityKeyIdentifier extensin MUST
   be included in all certificates generated by conforming CAs to
   facilitate certification path construction.  There is one exception;
   where a CA distributes its public key in the form of a "self-signed"
   certificate, the authority key identifier MAY be omitted.  The
   signature on a self-signed certificate is generated with the private
   key associated with the certificate's subject public key.  (This
   proves that the issuer possesses both the public and private keys.)
   In this case, the subject and authority key identifiers would be
   identical, but only the subject key identifier is needed for
   certification path building.

Indeed for certs conforming to RFC 5280 one is able to deduce from the SKID and from the AKID (or its non-presence) whether the cert is (meant to be) self-signed.
Yet there are self-issued certs that do not have an SKID (e.g., X.509v1 certs) or with misleading SKID & AKID information (like the one in https://tools.ietf.org/html/draft-ietf-curdle-pkix-04#section-10.2, which is in test/certs/ee-ed25519.pem).
So the presence and the values of SKID and AKID are in general not sufficient for deciding whether it makes sense to check for self-signature.

And in any case no one compels users to use the X509_V_FLAG_CHECK_SS_SIGNATURE flag. It is off by default.

Right.

But if they ask for it, then the signature should be checked.

Up to this point it is not really clear whether we should be able and be allowed to check the self-signature.

We should not confuse the keyUsage "certSign" bit (which is a policy constraint) with whether the algorithm is suitable for signing.

I agree that we should not confuse these two.
Yet independently of this, as mentioned, https://tools.ietf.org/html/rfc5280#section-4.2.1.3 forbids verifying cert signatures if the key usage does not support keyCertSign. And this strict requirement is stated regardless of the cert type (be it CA, EE, self-issued, etc.) and of the context of the verification.

Surely the certificate signature was signed by a key that does support signing, and if that's the same as the algorithm of the SPKI, then the signature can be checked...

That's right (and I suppose we do not need to check for matching signatures beforehand because X509_verify() will give negative result on a non-match anyway).

One such interesting examples is https://tools.ietf.org/html/draft-ietf-curdle-pkix-04#section-10.2
As I just wrote a little further up, for self-issued certs a self-signature and consequently a self-signature verification is not always technically possible.
And even if so, the creator of the cert might still want to prohibit the use of the public key for cert signature verification,
and we should and can respect this.

Not if they present a single-element chain with a self-signed EE key and ask for signature verification.

Again, I see no good reason to disregard, even on user's request, to ignore the requirement

 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.
  • at least as long we do not make this explicit.

We could explicitly state in our documentation that the (self-)signature verification may in this case be in violation of that rule.
Is this an agreeable solution?

@kroeckx
Copy link
Member

@kroeckx kroeckx commented Jul 11, 2020

@DDvO
Copy link
Contributor Author

@DDvO DDvO commented Jul 12, 2020

I'm answering now on each of @kroeckx's most recent comments; here is the first section of this:

Note that not all self-issued certs are self-signed. One such interesting examples is https://tools.ietf.org/html/draft-ietf-curdle-pkix-04#section-10.2

We're talking about checking the self signatures, and that doesn't have a self signature.

Correct. Yet when the code in the body of internal_verify() comes across a self-issued cert last in the chain (where typically a self-signed root CA cert is placed) then it can have a hard time finding out whether the self-issued cert is meant to be self-signed because there is no direct indication in any X.509 (v1 or v3) cert saying "I'm self-signed".

The EXFLAG_SS flag should not be set in that certificate, so there is nothing to check when X509_V_FLAG_CHECK_SS_SIGNATURE is set.

Indeed the EXFLAG_SS indicates that x509v3_cache_extensions() believes that it is a self-signed cert, but as I wrote yesterday, (even in its improved version with the fixes for #1418) it can go wrong due to missing or misleading SKID and/or AKID entries. In particular, the weird cert in https://tools.ietf.org/html/draft-ietf-curdle-pkix-04#section-10.2 only has an SKID, which misleads x509v3_cache_extensions() to set EXFLAG_SS.
So relying on that flag can be slippery, and internal_verify does not rely on it.

@DDvO
Copy link
Contributor Author

@DDvO DDvO commented Jul 12, 2020

I guess we can also be in the case where an intermediate CA is in the trust store, but the root CA is not.

Yes.

We can't check the signature of the last certificate, because it's not self-signed. The X509_V_FLAG_CHECK_SS_SIGNATURE flag also doesn't do anything in that case.

Yes, because such a cert is not even self-issued (which would be required for a self-signed cert).

The manual currently says: B<X509_V_FLAG_CHECK_SS_SIGNATURE> requires verifying the signature of the last certificate in a chain even when it is a self-signed (root CA) certificate.

That sentence is really a bit strange.

This seems to assume that only self-signed certificates are in the trust store, so I guess that should be clarified.

The sentence in the manual was not meant to assume this.
How if I improve it to:
B<X509_V_FLAG_CHECK_SS_SIGNATURE> requests checking the signature of the last certificate in a chain if the certificate is supposedly self-signed ?

PS: That has been published as RFC 8410.

What did you mean here? Which information and in which section of it?

@kroeckx
Copy link
Member

@kroeckx kroeckx commented Jul 12, 2020

@kroeckx
Copy link
Member

@kroeckx kroeckx commented Jul 12, 2020

@DDvO
Copy link
Contributor Author

@DDvO DDvO commented Jul 12, 2020

I think we need to go over all the cases, because I'm not sure that we understand each other.

Maybe this is needed, but only as far as the last cert in a chain is concerned,
because for normal EE certs (at the beginning of a chain with lenght > 1) and for intermediate CA certs nothing is in question here
(and I presume this is the reason why the cases you listed do not include such certs).

  • Normal CA cert, signs an EE cert:

I guess you meant a root CA cert, which signs a intermediate CA cert or EE cert.

The CA cert has a self signature.

At least this is what typically can be assumed for root CA certs.
Yet maybe it is the cert of an intermediate CA that is directly trusted. In such a case the X509_V_FLAG_PARTIAL_CHAIN flag needs to be set for accepting this situation, and then there is no need to try verifying a self-signature (because the cert is not meant to be self-signed).

We don't check that signature by default, but it can be checked. The CA cert has the keyCertSign bit, because otherwise it can't sign the EE cert.

Right (assuming we are talking about a root CA cert, which could also sign an intermediate CA cert rather than an EE cert).

  • A non-CA self-signed certificate:

In other words, a self-signed EE cert.

It has no keyCertSign,

Well, it must not have keyCertSign, according to https://tools.ietf.org/html/rfc5280#section-4.2.1.3:

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

So in case the keyCertSign bit is still set for a non-CA cert the cert is non-conforming.

but it falls outside the scope of RFC5280. If requested, we can check the signature. Whatever RFC5280 says is irrelevant.

I still do not buy these statements until I am given a compelling argument for their validity.
RFC 5280 is full of requirements etc. for any sorts of X.509 (v3) certs, and naturally you yourself take various ideas about certs, including self-signed EE certs, from that RFC.
AFAICS it does not constrain its scope to certain (sub-)types of certs. Of course, the chain validation procedure in its section 6 makes certain assumptions on which types of certs can be in which parts of a chain, but section 4 pertains to any certs - it does not say 'we are excluding self-signed non-CA certs' or anything like this.

  • A not self-signed certificate that's placed in the trust store, like your self-issued X25519 example. It needs the partial chain option if it wasn't to be verified successful.

I guess you mean an /EE/ cert that is not self-signed.

It's not self-signed, so we can't check the signature.

Well, right, but, as explained earlier today (#12357 (comment)) there are various situations where the code cannot be /sure/ if a cert is meant to be self-signed (unless it does the actual verification).

  • Is there some case I'm missing?

Yes, even restricting ourselves to the last cert in a chain, from a logical (decision tree) perspective the following case is missing:

A not self-signed EE certificate that's not placed in the trust store.
Here (regardless whether the cert is self-issued or not) I think the verification should fail because the cert is not trusted.
In case the cert is self-issued we may in addition consider what to do in case the X509_V_FLAG_CHECK_SS_SIGNATURE flag is set.
As usual, the check will fail if the algorithms do not match or the key does not support signing.
If the algorithms match and support signing and the key usage does not allow cert signatures I'd say we should

  • not attempt verifying the signature and continue
  • or throw a key usage error
  • or explicitly point out that we will do the requested SS verification although RFC 5280 forbids it.
@DDvO
Copy link
Contributor Author

@DDvO DDvO commented Jul 12, 2020

You keep pointing to draft-ietf-curdle-pkix-04, which has been published as RFC 8410.

Ah, right, thanks for that hint!

@DDvO
Copy link
Contributor Author

@DDvO DDvO commented Jul 12, 2020

In particular, the weird cert in https://tools.ietf.org/html/draft-ietf-curdle-pkix-04#section-10.2 only has an SKID, which misleads x509v3_cache_extensions() to set EXFLAG_SS.

I would argue that that's a bug in the RFC 8410 and that you file an errata.

Good suggestion - done at https://www.rfc-editor.org/errata/eid6229

@kroeckx
Copy link
Member

@kroeckx kroeckx commented Jul 12, 2020

@DDvO
Copy link
Contributor Author

@DDvO DDvO commented Jul 13, 2020

So are we just talking about what should happen when we see such an invalid certificate? One that is detected as self-signed, but can't be self-signed because the the key type doesn't support signing? The X25519 certificate being an example.

The scope of our discussion is wider, namely how to handle the case that a certificate at the end of a chain appears to be self-signed (and typically is in fact self-signed) and X509_V_FLAG_CHECK_SS_SIGNATURE is requested but its key usage does not allow verifying certificate signatures.

So the problem arises also for valid/well-formed/conforming certificates that are actually self-signed and have a keyUsage extension that does not include keyCertSign. The typical - an perfectly legitimate - use case is a directly trusted self-signed EE cert (i.e., one that is not issued by a CA) for a key that supports signing. For technical reasons it must bear a signature, and thus usually a self-signature is used, while the cA flag and the keyCertSign key usage bit is not set (in order to prevent the cert from being interpreted as a CA cert). If no key usage extension is given the problem does not arise because then implicitly all key usages are allowed. Yet the cert (self-)issuer may want to make sure that the cert key use is restricted (e.g., to digitalSignature).

As I wrote yesterday (#12357 (comment)) in such a situation there are the following possibilities (where I added on case describing the status quo):

  • not attempt verifying the (self-)signature and continue, ignoring the X509_V_FLAG_CHECK_SS_SIGNATURE request. This was my original proposal in this PR, but this is not in line with principle of least surprise as pointed out by @vdukhovni.
  • throw a key usage error because X509_V_FLAG_CHECK_SS_SIGNATURE is requested but forbidden
  • attempt verifying the (self-)signature regardless, silently ignoring the key usage restriction without documenting this behavior. This is the status quo.
  • attempt verifying the (self-)signature regardless, ignoring the key usage restriction but explicitly pointing out in the documentation that we will do the requested SS verification although there is a rule in RFC 5280 not allowing it. This is my new proposal, given on Saturday (#12357 (comment)).
@beldmit
Copy link
Member

@beldmit beldmit commented Jul 13, 2020

  • attempt verifying the (self-)signature regardless, silently ignoring the key usage restriction without documenting this behavior. This is the status quo.

Maybe it males (some) sense to add one more flag - X509_IGNORE_SS_KU? Yes, it is over-engineering, I see...

@vdukhovni
Copy link

@vdukhovni vdukhovni commented Jul 13, 2020

My take is that a trusted chain consisting of exactly one self-issued certificate as its own trust-anchor that has keyUsage without certSign and yet is actually signed by the same key, is so far outside the scope of RFC5280 or any standard PKI model, that perhaps the right thing to do is what the user asked us to do, which is check the certificate signature as requested.

This supports the creation of self-signed certs in which keyUsage explicitly disavows certSign, and yet their self signature can be checked.

Ultimately however, this is rather a corner case, and we're in bikeshedding territory. If there's some support for David's position, I can back off...

@DDvO
Copy link
Contributor Author

@DDvO DDvO commented Jul 13, 2020

@kroeckx, on the side topic of invalid certificates, like that example cert in https://tools.ietf.org/html/rfc8410#section-10.2:

The X25519 certificat being an example.
For such certicates, I think there are 3 options:

  1. Always reject it, it's clearly an invalid certificate
  2. Reject it when they ask for the self signature to be checked
  3. Always accept it I think you're arguing for 3), while I think we should do 1)

Here I am also in favor of 1)

BTW, so far OpenSSL does not thoroughly check certificate conformance.
Two weeks back I've already started preparing a PR that adds various checks for not well-formed certs.

@DDvO
Copy link
Contributor Author

@DDvO DDvO commented Jul 13, 2020

@vdukhovni and @kroeckx,
are you fine if I retract the most recent change regarding key usage checking for self-signature verification
and write the following in the documentation?

B<X509_V_FLAG_CHECK_SS_SIGNATURE> demands checking the signature of
the last certificate in a chain if the certificate is supposedly self-signed
(ignoring any key usage restrictions in the certificate that may prohibit this).
@DDvO DDvO force-pushed the siemens:fix_x509_vfy_issue_1418_v111 branch Jul 13, 2020
@DDvO
Copy link
Contributor Author

@DDvO DDvO commented Jul 13, 2020

@vdukhovni and @kroeckx,
are you fine if I retract the most recent change regarding key usage checking for self-signature verification
and write the following in the documentation?

B<X509_V_FLAG_CHECK_SS_SIGNATURE> demands checking the signature of
the last certificate in a chain if the certificate is supposedly self-signed
(ignoring any key usage restrictions in the certificate that may prohibit this).

I've just done this tentatively.

…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
@DDvO DDvO force-pushed the siemens:fix_x509_vfy_issue_1418_v111 branch Jul 13, 2020
If a presumably self-signed cert is last in chain we verify its signature
only if X509_V_FLAG_CHECK_SS_SIGNATURE is set. Upon this request we do the
signature verification, but not in case it is a (non-conforming) self-issued
CA certificate with a key usage extension that does not include keyCertSign.

Make clear when we must verify the signature of a certificate
and when we must adhere to key usage restrictions of the 'issuing' cert.
Add some comments for making internal_verify() easier to understand.
Update the documentation of X509_V_FLAG_CHECK_SS_SIGNATURE accordingly.
@DDvO DDvO force-pushed the siemens:fix_x509_vfy_issue_1418_v111 branch Jul 14, 2020
@DDvO
Copy link
Contributor Author

@DDvO DDvO commented Jul 14, 2020

@vdukhovni and @kroeckx, during a very good discussion this morning with my colleague @HBrock
I found that there is an update to RFC 5280 that confirms what you stated (without giving clear evidence):

https://tools.ietf.org/html/rfc6818#section-2 says

Consistent with Section 3.4.61 of X.509 (11/2008) [X.509], we note
that use of self-issued certificates and self-signed certificates
issued by entities other than CAs are outside the scope of this
specification. Thus, for example, a web server or client might
generate a self-signed certificate to identify itself. These
certificates and how a relying party uses them to authenticate
asserted identities are both outside the scope of RFC 5280.

So you are right in the sense that for self-issued and self-signed certificates
that have not been issued by a CA we are not bound to RFC 5820 and can do whatever makes sense.
The typical use case are directly trusted 'home-grown' self-signed self-issued end-entity certs.

On the other hand, for CA certs that are self-issued and supposedly self-signed we are bound to RFC 5280
and must not attempt to verify the assumed self-signature in case its key usage does not support this.
This can happen only if that CA cert is non-conforming because in case the key usage extension is present
it is required to include the keyCertSign bit.

So for our implementation of internal_verify() we need to determine somehow if we have such a cert or not.
Unfortunately this distinction cannot really be done just by having a look at the cert in question.
Yet I suppose we can assume that a cert is such a 'home-grown' one if and only if
it appears at the end of the chain and is a non-CA cert with identical subject and issuer entries,
right?

@DDvO DDvO force-pushed the siemens:fix_x509_vfy_issue_1418_v111 branch Jul 14, 2020
@DDvO
Copy link
Contributor Author

@DDvO DDvO commented Jul 14, 2020

Due to this new insight I've updated the proposed solution as follows.

In the commit message I write:

x509_vfy.c: Improve key usage checks in internal_verify() of cert chains

If a presumably self-signed cert is last in chain we verify its signature
only if X509_V_FLAG_CHECK_SS_SIGNATURE is set. Upon this request we do the
signature verification, but not in case it is a (non-conforming) self-issued
CA certificate with a key usage extension that does not include keyCertSign.

Make clear when we must verify the signature of a certificate
and when we must adhere to key usage restrictions of the 'issuing' cert.
Add some comments for making internal_verify() easier to understand.
Update the documentation of X509_V_FLAG_CHECK_SS_SIGNATURE accordingly.

The respective section in internal_verify() now reads as:

            /*
             * According to https://tools.ietf.org/html/rfc5280#section-6.1.4
             * step (n) we must check any given key usage extension in a CA cert
             * when preparing the verification of a certificate issued by it.
             * According to https://tools.ietf.org/html/rfc5280#section-4.2.1.3
             * we must not verify a certifiate signature if the key usage of the
             * CA certificate that issued the certificate prohibits signing.
             * In case the 'issuing' certificate is the last in the chain and is
             * not a CA certificate but a 'self-issued' end-entity cert (i.e.,
             * xs == xi && !(xi->ex_flags & EXFLAG_CA)) RFC 5280 does not apply
             * (see https://tools.ietf.org/html/rfc6818#section-2) and thus
             * we are free to ignore any key usage restrictions on such certs.
             */
            int ret = xs == xi && (xi->ex_flags & EXFLAG_CA) == 0
                ? X509_V_OK : x509_signing_allowed(xi, xs);

            if (ret != X509_V_OK && !verify_cb_cert(ctx, xi, issuer_depth, ret))
                return 0;

and I've updated the documentation to:

B<X509_V_FLAG_CHECK_SS_SIGNATURE> demands checking the signature of
the last certificate in a chain if the certificate is supposedly self-signed.
This is prohibited and will result in an error if it is a non-conforming CA
certificate with key usage restrictions not including the keyCertSign bit.
@DDvO DDvO force-pushed the siemens:fix_x509_vfy_issue_1418_v111 branch to 46294ca Jul 14, 2020
@t8m
Copy link
Member

@t8m t8m commented Jul 14, 2020

Maybe I am confused but what is the non-conforming CA case really about - such an invalid CA certificate would not be able to verify any other certificate signature anyway so failing the self-signature check with X509_V_FLAG_CHECK_SS_SIGNATURE is expected and does not warrant any special handling.

@DDvO
Copy link
Contributor Author

@DDvO DDvO commented Jul 14, 2020

Maybe I am confused but what is the non-conforming CA case really about - such an invalid CA certificate would not be able to verify any other certificate signature anyway so failing the self-signature check with X509_V_FLAG_CHECK_SS_SIGNATURE is expected and does not warrant any special handling.

So far the implementation of internal_verify() did not pay attention to key usage restrictions (while it was done elsewhere, during chain building). This PR rectifies this including some special cases regarding directly trusted EE certs, fixing issue #1418, and we should make sure that we do it exactly where required.
Strongly connected is the topic when exactly we should attempt to verify any supposed cert self-signatures if requested.

The case that a CA cert is non-conforming because it key usage does not allow keyCertSign is of course just a corner case.
During chain validation, there are potentially two intended uses of the public key in such a cert:

  1. Verify the signature of the cert directly preceding it in the chain (in the view that the root is at the end).
  2. Optionally verify its own signature if X509_V_FLAG_CHECK_SS_SIGNATURE is set.
    Both cases must fail if the key usage does not allow keyCertSign.

There is also the general question what to do with certs that are not RFC 5280 compliant for various reasons,
but that's a topic to be addressed in a separate PR (which as mentioned I've started preparing).

@DDvO
Copy link
Contributor Author

@DDvO DDvO commented Jul 15, 2020

This PR has developed into two parts:

  1. the core of #10587 backported as fix of #1418 for v1.1.1 on which users have already been waiting for long time
  2. the further changes regarding X509_V_FLAG_CHECK_SS_SIGNATURE that we have been discussing since then

@vdukhovni, is it realistic that you can approve this PR by tomorrow, after which I'll be on vacation for two weeks?
If not, I'd suggest that we decouple part 1 such that the inclusion of the fix in v1.1.1 is not blocked longer.

@t8m
Copy link
Member

@t8m t8m commented Jul 15, 2020

I've approved the #12375 as I think that change in the end is pretty uncontroversial. For this one I'd leave the approval on Viktor.

@DDvO
Copy link
Contributor Author

@DDvO DDvO commented Jul 15, 2020

I've approved the #12375 as I think that change in the end is pretty uncontroversial. For this one I'd leave the approval on Viktor.

Thanks Tomáš!
Actually I've kept the contents of the (last) commit under discussion here consistent with the PR #12375 you've just approved.
So in my view, after the forth and back done during the discussion here, this PR it could be approved likewise
given that the rest of this PR is just a backport of the fix of #1418 already merged to our master in #10587.

@t8m
t8m approved these changes Jul 15, 2020
Copy link
Member

@t8m t8m left a comment

OK, you've convinced me. And I reviewed the backport and agree with it.

@openssl-machine
Copy link

@openssl-machine openssl-machine commented Jul 16, 2020

This pull request is ready to merge

openssl-machine pushed a commit that referenced this pull request 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)
openssl-machine pushed a commit that referenced this pull request Jul 16, 2020
If a presumably self-signed cert is last in chain we verify its signature
only if X509_V_FLAG_CHECK_SS_SIGNATURE is set. Upon this request we do the
signature verification, but not in case it is a (non-conforming) self-issued
CA certificate with a key usage extension that does not include keyCertSign.

Make clear when we must verify the signature of a certificate
and when we must adhere to key usage restrictions of the 'issuing' cert.
Add some comments for making internal_verify() easier to understand.
Update the documentation of X509_V_FLAG_CHECK_SS_SIGNATURE accordingly.

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

@DDvO DDvO commented Jul 16, 2020

Merged - glad that also this (extended) backport of the fix for #1418 is finally in.
Thanks @vdukhovni, @kroeckx, and @HBrock for the interesting discussion
and @t8m for approving this PR in the state reflecting the discussion outcome.

@@ -800,12 +833,23 @@ static int no_check(const X509_PURPOSE *xp, const X509 *x, int ca)
* These are:
* 1. Check issuer_name(subject) == subject_name(issuer)
* 2. If akid(subject) exists check it matches issuer
* 3. If key_usage(issuer) exists check it supports certificate signing
* 3. Check that issuer public key algorithm matches subject signature algorithm

This comment has been minimized.

@1185129439

1185129439 Feb 3, 2021

Hi ,I think your changes are great.
But what is the basis for this change? Is there a regulation in some FRC?
Thanks

This comment has been minimized.

@DDvO

DDvO Feb 3, 2021
Author Contributor

Thanks for your comment and question.
The basis for this additional check is the observation that in order to be able to sign the subject cert, the public key of the candidate issuer cert must have a matching type.
If it does not match, in particular it does not make sense to select the candidate issuer cert during chain building:
When later trying to verify the signature during chain verification, that verification will certainly fail, so the chain building reached a dead end. Since moreover the chain building implementation does not backtrack, the whole target cert verification would fail for an inadequate reason. This happened for the example referred to in #1418.

This comment has been minimized.

@t8m

t8m Feb 3, 2021
Member

This is simply a sanity check. There is no point in adding to the chain an issuer that could not possibly sign the subject certificate. Unfortunately it was incorrectly implemented in this PR as a RSA key can create RSA-PSS signatures. This is now fixed in the repository with commit c2fc111

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

7 participants