Fix issue 1418 by moving check of KU_KEY_CERT_SIGN and weakening check_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 #1418

Reviewed-by: Viktor Dukhovni <viktor@openssl.org>
(Merged from https://github.com/openssl/openssl/pull/10587)

8 files changed, 52 insertions, 34 deletions

diff --git a/crypto/x509/v3_purp.c b/crypto/x509/v3_purp.c
index 1c0fba2743..e9f8823ce2 100644
--- a/crypto/x509/v3_purp.c
+++ b/crypto/x509/v3_purp.c
@@ -521,7 +521,8 @@ int X509v3_cache_extensions(X509 *x, OPENSSL_CTX *libctx, const char *propq)
     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 */
-            && !ku_reject(x, KU_KEY_CERT_SIGN))
+                /* .. 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 */
     }
     x->altname = X509_get_ext_d2i(x, NID_subject_alt_name, &i, NULL);
diff --git a/crypto/x509/x509_vfy.c b/crypto/x509/x509_vfy.c
index ba36bafdfc..122d6f8a3b 100644
--- a/crypto/x509/x509_vfy.c
+++ b/crypto/x509/x509_vfy.c
@@ -116,7 +116,6 @@ static int null_callback(int ok, X509_STORE_CTX *e)
  * This does not verify self-signedness but relies on x509v3_cache_extensions()
  * matching issuer and subject names (i.e., the cert being self-issued) and any
  * present authority key identifier matching the subject key identifier, etc.
- * Moreover the key usage (if present) must allow certificate signing - TODO correct this wrong semantics of x509v3_cache_extensions()
  */
 static int cert_self_signed(X509_STORE_CTX *ctx, X509 *x)
 {
@@ -343,36 +342,26 @@ static X509 *find_issuer(X509_STORE_CTX *ctx, STACK_OF(X509) *sk, X509 *x)
     return rv;
 }
 
-/* Given a possible certificate and issuer check them */
-
+/*
+ * Check that the given certificate 'x' is issued by the certificate 'issuer'
+ * and the issuer is not yet in ctx->chain, where the exceptional case
+ * that 'x' is self-issued and ctx->chain has just one element is allowed.
+ */
 static int check_issued(X509_STORE_CTX *ctx, X509 *x, X509 *issuer)
 {
-    int ret;
-
-    if (x == issuer)
-        return cert_self_signed(ctx, x) == 1;
-    ret = x509_check_issued_int(issuer, x, ctx->libctx, ctx->propq);
-    if (ret == X509_V_OK) {
+    if (x509_likely_issued(issuer, x, ctx->libctx, ctx->propq) != X509_V_OK)
+        return 0;
+    if ((x->ex_flags & EXFLAG_SI) == 0 || sk_X509_num(ctx->chain) != 1) {
         int i;
         X509 *ch;
 
-        ss = cert_self_signed(ctx, x);
-        if (ss < 0)
-            return 0;
-
-        /* Special case: single (likely) self-signed certificate */
-        if (ss > 0 && sk_X509_num(ctx->chain) == 1)
-            return 1;
         for (i = 0; i < sk_X509_num(ctx->chain); i++) {
             ch = sk_X509_value(ctx->chain, i);
-            if (ch == issuer || X509_cmp(ch, issuer) == 0) {
-                ret = X509_V_ERR_PATH_LOOP;
-                break;
-            }
+            if (ch == issuer || X509_cmp(ch, issuer) == 0)
+                return 0;
         }
     }
-
-    return (ret == X509_V_OK);
+    return 1;
 }
 
 /* Alternative lookup method: look from a STACK stored in other_ctx */
@@ -1780,13 +1769,17 @@ static int internal_verify(X509_STORE_CTX *ctx)
         /*
          * 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 unusable, report the issuer certificate
+         * 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)) {
             EVP_PKEY *pkey;
             int issuer_depth = n + (xi == xs ? 0 : 1);
+            int ret = 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, issuer_depth,
                                     X509_V_ERR_UNABLE_TO_DECODE_ISSUER_PUBLIC_KEY))
diff --git a/doc/man1/openssl.pod b/doc/man1/openssl.pod
index 3c91757eaf..dee181d264 100644
--- a/doc/man1/openssl.pod
+++ b/doc/man1/openssl.pod
@@ -844,8 +844,7 @@ All available certificates with a subject name that matches the issuer
 name of the current certificate are subject to further tests.
 The relevant authority key identifier components of the current certificate
 (if present) must match the subject key identifier (if present)
-and issuer and serial number of the candidate issuer; in addition the keyUsage
-extension of the candidate issuer (if present) must permit certificate signing.
+and issuer and serial number of the candidate issuer certificate.
 
 The lookup first searches for issuer certificates in the trust store.
 If it does not find a match there it consults
@@ -876,9 +875,8 @@ The certificate signatures are also checked at this point
 which is verified only if the B<-check_ss_sig> option is given).
 When verifying a certificate signature
 the keyUsage extension (if present) of the candidate issuer certificate
-is checked to permit digitalSignature for signing proxy certificates or
-keyCertSign for signing other certificates, respectively.
-
+is checked to permit digitalSignature for signing proxy certificates
+or to permit keyCertSign for signing other certificates, respectively.
 If all operations complete successfully then certificate is considered
 valid. If any operation fails then the certificate is not valid.
 
diff --git a/doc/man3/X509_STORE_set_verify_cb_func.pod b/doc/man3/X509_STORE_set_verify_cb_func.pod
index e845906cc8..84b216ffbe 100644
--- a/doc/man3/X509_STORE_set_verify_cb_func.pod
+++ b/doc/man3/X509_STORE_set_verify_cb_func.pod
@@ -145,7 +145,9 @@ I<If no function to get the issuer is provided, the internal default
 function will be used instead.>
 
 X509_STORE_set_check_issued() sets the function to check that a given
-certificate B<x> is issued with the issuer certificate B<issuer>.
+certificate B<x> is issued by the issuer certificate B<issuer> and
+the issuer is not yet in the chain contained in <ctx>, where the exceptional
+case that B<x> is self-issued and ctx->chain has just one element is allowed.
 This function must return 0 on failure (among others if B<x> hasn't
 been issued with B<issuer>) and 1 on success.
 I<If no function to get the issuer is provided, the internal default
diff --git a/doc/man3/X509_check_issued.pod b/doc/man3/X509_check_issued.pod
index 0e8ab7deb3..cc98541bba 100644
--- a/doc/man3/X509_check_issued.pod
+++ b/doc/man3/X509_check_issued.pod
@@ -2,7 +2,7 @@
 
 =head1 NAME
 
-X509_check_issued - checks if certificate is likely issued by another
+X509_check_issued - checks if certificate is apparently issued by another
 certificate
 
 =head1 SYNOPSIS
@@ -14,8 +14,8 @@ certificate
 
 =head1 DESCRIPTION
 
-X509_check_issued() checks if certificate I<subject> was likely issued using CA
-certificate I<issuer>. This function takes into account not only
+X509_check_issued() checks if certificate I<subject> was apparently issued
+using (CA) certificate I<issuer>. This function takes into account not only
 matching of the issuer field of I<subject> with the subject field of I<issuer>,
 but also compares all sub-fields of the B<authorityKeyIdentifier> extension of
 I<subject>, as far as present, with the respective B<subjectKeyIdentifier>,
diff --git a/test/certs/ee-self-signed.pem b/test/certs/ee-self-signed.pem
new file mode 100644
index 0000000000..ad1e37ba0e
--- /dev/null
+++ b/test/certs/ee-self-signed.pem
@@ -0,0 +1,18 @@
+-----BEGIN CERTIFICATE-----
+MIICzzCCAbegAwIBAgIUBP7iEKPlKuinZGQNFxSY3IBIb0swDQYJKoZIhvcNAQEL
+BQAwGTEXMBUGA1UEAwwOZWUtc2VsZi1zaWduZWQwHhcNMjAwNjI4MTA1MTQ1WhcN
+MjAwNzI4MTA1MTQ1WjAZMRcwFQYDVQQDDA5lZS1zZWxmLXNpZ25lZDCCASIwDQYJ
+KoZIhvcNAQEBBQADggEPADCCAQoCggEBAKj/iVhhha7e2ywP1XP74reoG3p1YCvU
+fTxzdrWu3pMvfySQbckc9Io4zZ+igBZWy7Qsu5PlFx//DcZD/jE0+CjYdemju4iC
+76Ny4lNiBUVN4DGX76qdENJYDZ4GnjK7GwhWXWUPP2aOwjagEf/AWTX9SRzdHEIz
+BniuBDgj5ed1Z9OUrVqpQB+sWRD1DMFkrUrExjVTs5ZqghsVi9GZq+Seb5Sq0pbl
+V/uMkWSKPCQWxtIZvoJgEztisO0+HbPK+WvfMbl6nktHaKcpxz9K4iIntO+QY9fv
+0HJJPlutuRvUK2+GaN3VcxK4Q8ncQQ+io0ZPi2eIhA9h/nk0H0qJH7cCAwEAAaMP
+MA0wCwYDVR0PBAQDAgeAMA0GCSqGSIb3DQEBCwUAA4IBAQBiLmIUCGb+hmRGbmpO
+lDqEwiRVdxHBs4OSb3IA9QgU1QKUDRqn7q27RRelmzTXllubZZcX3K6o+dunRW5G
+d3f3FVr+3Z7wnmkQtC2y3NWtGuWNczss+6rMLzKvla5CjRiNPlSvluMNpcs7BJxI
+ppk1LxlaiYlQkDW32OPyxzXWDNv1ZkphcOcoCkHAagnq9x1SszvLTjAlo5XpYrm5
+CPgBOEnVwFCgne5Ab4QPTgkxPh/Ta508I/FKaPLJqci1EfGKipZkS7mMGTUJEeVK
+wZrn4z7RiTfJ4PdqO5iv8eOpt03fqdPEXQWe8DrKyfGM6/e369FaXMFhcd2ZxZy2
+WHoc
+-----END CERTIFICATE-----
diff --git a/test/certs/setup.sh b/test/certs/setup.sh
index f4f3e046f0..d1c56bb56d 100755
--- a/test/certs/setup.sh
+++ b/test/certs/setup.sh
@@ -185,6 +185,9 @@ OPENSSL_SIGALG=md5 \
 OPENSSL_KEYBITS=768 \
 ./mkcert.sh genee server.example ee-key-768 ee-cert-768 ca-key ca-cert
 
+# self-signed end-entity cert with explicit keyUsage not including KeyCertSign
+openssl req -new -x509 -key ee-key.pem -subj /CN=ee-self-signed -out ee-self-signed.pem -addext keyUsage=digitalSignature
+
 # Proxy certificates, off of ee-client
 # Start with some good ones
 ./mkcert.sh req pc1-key "0.CN = server.example" "1.CN = proxy 1" | \
diff --git a/test/recipes/25-test_verify.t b/test/recipes/25-test_verify.t
index 2997503355..42d44dcdce 100644
--- a/test/recipes/25-test_verify.t
+++ b/test/recipes/25-test_verify.t
@@ -27,7 +27,7 @@ sub verify {
     run(app([@args]));
 }
 
-plan tests => 143;
+plan tests => 144;
 
 # Canonical success
 ok(verify("ee-cert", "sslserver", ["root-cert"], ["ca-cert"]),
@@ -368,6 +368,9 @@ ok(verify("some-names2", "sslserver", ["many-constraints"], ["many-constraints"]
 ok(verify("root-cert-rsa2", "sslserver", ["root-cert-rsa2"], [], "-check_ss_sig"),
     "Public Key Algorithm rsa instead of rsaEncryption");
 
+    ok(verify("ee-self-signed", "sslserver", ["ee-self-signed"], []),
+       "accept trusted self-signed EE cert excluding key usage keyCertSign");
+
 SKIP: {
     skip "Ed25519 is not supported by this OpenSSL build", 5
 	      if disabled("ec");