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.

Reviewed-by: Tomas Mraz <tmraz@fedoraproject.org>
(Merged from https://github.com/openssl/openssl/pull/12357)

3 files changed, 48 insertions, 18 deletions

diff --git a/crypto/x509/x509_vfy.c b/crypto/x509/x509_vfy.c
index 87b51e990d..f30c0f8d38 100644
--- a/crypto/x509/x509_vfy.c
+++ b/crypto/x509/x509_vfy.c
@@ -1716,6 +1716,7 @@ int x509_check_cert_time(X509_STORE_CTX *ctx, X509 *x, int depth)
     return 1;
 }
 
+/* verify the issuer signatures and cert times of ctx->chain */
 static int internal_verify(X509_STORE_CTX *ctx)
 {
     int n = sk_X509_num(ctx->chain) - 1;
@@ -1734,7 +1735,7 @@ static int internal_verify(X509_STORE_CTX *ctx)
     }
 
     if (ctx->check_issued(ctx, xi, xi))
-        xs = xi;
+        xs = xi; /* the typical case: last cert in the chain is self-issued */
     else {
         if (ctx->param->flags & X509_V_FLAG_PARTIAL_CHAIN) {
             xs = xi;
@@ -1754,26 +1755,49 @@ static int internal_verify(X509_STORE_CTX *ctx)
      */
     while (n >= 0) {
         /*
-         * Skip signature check for self-issued certificates unless explicitly
+         * For each iteration of this loop:
+         * n is the subject depth
+         * xs is the subject cert, for which the signature is to be checked
+         * xi is the supposed issuer cert containing the public key to use
+         * Initially xs == xi if the last cert in the chain is self-issued.
+         *
+         * Skip signature check for self-signed certificates unless explicitly
          * asked for because it does not add any security and just wastes time.
-         * If the issuer's public key is not available or its key usage does
-         * not support issuing the subject cert, report the issuer certificate
-         * and its depth (rather than the depth of the subject).
          */
-        if (xs != xi || (ctx->param->flags & X509_V_FLAG_CHECK_SS_SIGNATURE)) {
+        if (xs != xi || ((ctx->param->flags & X509_V_FLAG_CHECK_SS_SIGNATURE)
+                         && (xi->ex_flags & EXFLAG_SS) != 0)) {
             EVP_PKEY *pkey;
+            /*
+             * If the issuer's public key is not available or its key usage
+             * does not support issuing the subject cert, report the issuer
+             * cert and its depth (rather than n, the depth of the subject).
+             */
             int issuer_depth = n + (xs == xi ? 0 : 1);
-            int ret = x509_signing_allowed(xi, xs);
+            /*
+             * 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;
             if ((pkey = X509_get0_pubkey(xi)) == NULL) {
-                if (!verify_cb_cert(ctx, xi, xi != xs ? n+1 : n,
-                                    X509_V_ERR_UNABLE_TO_DECODE_ISSUER_PUBLIC_KEY))
+                ret = X509_V_ERR_UNABLE_TO_DECODE_ISSUER_PUBLIC_KEY;
+                if (!verify_cb_cert(ctx, xi, issuer_depth, ret))
                     return 0;
             } else if (X509_verify(xs, pkey) <= 0) {
-                if (!verify_cb_cert(ctx, xs, n,
-                                    X509_V_ERR_CERT_SIGNATURE_FAILURE))
+                ret = X509_V_ERR_CERT_SIGNATURE_FAILURE;
+                if (!verify_cb_cert(ctx, xs, n, ret))
                     return 0;
             }
         }
diff --git a/doc/man1/verify.pod b/doc/man1/verify.pod
index 63ba850b91..18e803c8d6 100644
--- a/doc/man1/verify.pod
+++ b/doc/man1/verify.pod
@@ -98,8 +98,11 @@ current system time. B<timestamp> is the number of seconds since
 
 =item B<-check_ss_sig>
 
-Verify the signature on the self-signed root CA. This is disabled by default
-because it doesn't add any security.
+Verify 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.
+This verification is disabled by default because it doesn't add any security.
 
 =item B<-CRLfile file>
 
diff --git a/doc/man3/X509_VERIFY_PARAM_set_flags.pod b/doc/man3/X509_VERIFY_PARAM_set_flags.pod
index 7593dea7da..a87b71d92a 100644
--- a/doc/man3/X509_VERIFY_PARAM_set_flags.pod
+++ b/doc/man3/X509_VERIFY_PARAM_set_flags.pod
@@ -264,12 +264,15 @@ they are enabled.
 If B<X509_V_FLAG_USE_DELTAS> is set delta CRLs (if present) are used to
 determine certificate status. If not set deltas are ignored.
 
-B<X509_V_FLAG_CHECK_SS_SIGNATURE> enables checking of the root CA self signed
-certificate signature. By default this check is disabled because it doesn't
+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.
+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.
+By default this check is disabled because it doesn't
 add any additional security but in some cases applications might want to
-check the signature anyway. A side effect of not checking the root CA
-signature is that disabled or unsupported message digests on the root CA
-are not treated as fatal errors.
+check the signature anyway. A side effect of not checking the self-signature
+of such a certificate is that disabled or unsupported message digests used for
+the signature are not treated as fatal errors.
 
 When B<X509_V_FLAG_TRUSTED_FIRST> is set, construction of the certificate chain
 in L<X509_verify_cert(3)> will search the trust store for issuer certificates