Refactor (without semantic changes) crypto/x509/{v3_purp.c,x509_vfy.c}

This prepares some corrections and improves readability (coding style).
Among others, it adds the static function check_sig_alg_match() and
the internal functions x509_likely_issued() and x509_signing_allowed().

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

4 files changed, 77 insertions, 55 deletions

diff --git a/crypto/x509/v3_purp.c b/crypto/x509/v3_purp.c
index 5d9b947a39..1c0fba2743 100644
--- a/crypto/x509/v3_purp.c
+++ b/crypto/x509/v3_purp.c
@@ -14,6 +14,7 @@
 #include <openssl/x509_vfy.h>
 #include "crypto/x509.h"
 #include "internal/tsan_assist.h"
+#include "x509_local.h"
 
 DEFINE_STACK_OF(GENERAL_NAME)
 DEFINE_STACK_OF(DIST_POINT)
@@ -346,6 +347,21 @@ static int setup_crldp(X509 *x)
     return 1;
 }
 
+/* Check that issuer public key algorithm matches subject signature algorithm */
+static int check_sig_alg_match(const EVP_PKEY *pkey, const X509 *subject)
+{
+    int pkey_nid;
+
+    if (pkey == NULL)
+        return X509_V_ERR_NO_ISSUER_PUBLIC_KEY;
+    if (OBJ_find_sigid_algs(OBJ_obj2nid(subject->cert_info.signature.algorithm),
+                            NULL, &pkey_nid) == 0)
+        return X509_V_ERR_UNSUPPORTED_SIGNATURE_ALGORITHM;
+    if (EVP_PKEY_type(pkey_nid) != EVP_PKEY_base_id(pkey))
+        return X509_V_ERR_SIGNATURE_ALGORITHM_MISMATCH;
+    return X509_V_OK;
+}
+
 #define V1_ROOT (EXFLAG_V1|EXFLAG_SS)
 #define ku_reject(x, usage) \
         (((x)->ex_flags & EXFLAG_KUSAGE) && !((x)->ex_kusage & (usage)))
@@ -815,39 +831,47 @@ static int no_check(const X509_PURPOSE *xp, const X509 *x, int ca)
  * Returns 0 for OK, or positive for reason for mismatch
  * where reason codes match those for X509_verify_cert().
  */
+int x509_check_issued_int(X509 *issuer, X509 *subject,
+                          OPENSSL_CTX *libctx, const char *propq)
+{
+    int ret;
 
-int x509_check_issued_int(X509 *issuer, X509 *subject, OPENSSL_CTX *libctx,
-                          const char *propq)
+    if ((ret = x509_likely_issued(issuer, subject, libctx, propq)) != X509_V_OK)
+        return ret;
+    return x509_signing_allowed(issuer, subject);
+}
+
+/* do the checks 1., 2., and 3. as described above for X509_check_issued() */
+int x509_likely_issued(X509 *issuer, X509 *subject,
+                       OPENSSL_CTX *libctx, const char *propq)
 {
+    int ret;
+
     if (X509_NAME_cmp(X509_get_subject_name(issuer),
-                      X509_get_issuer_name(subject)))
+                      X509_get_issuer_name(subject)) != 0)
         return X509_V_ERR_SUBJECT_ISSUER_MISMATCH;
 
     if (!X509v3_cache_extensions(issuer, libctx, propq)
             || !X509v3_cache_extensions(subject, libctx, propq))
         return X509_V_ERR_UNSPECIFIED;
 
-    if (subject->akid) {
-        int ret = X509_check_akid(issuer, subject->akid);
-        if (ret != X509_V_OK)
-            return ret;
-    }
+    ret = X509_check_akid(issuer, subject->akid);
+    if (ret != X509_V_OK)
+        return ret;
 
     /* check if the subject signature alg matches the issuer's PUBKEY alg */
-    {
-        EVP_PKEY *i_pkey = X509_get0_pubkey(issuer);
-        X509_ALGOR *s_algor = &subject->cert_info.signature;
-        int s_pknid = NID_undef, s_mdnid = NID_undef;
-
-        if (i_pkey == NULL)
-            return X509_V_ERR_NO_ISSUER_PUBLIC_KEY;
-
-        if (!OBJ_find_sigid_algs(OBJ_obj2nid(s_algor->algorithm),
-                                 &s_mdnid, &s_pknid)
-            || EVP_PKEY_type(s_pknid) != EVP_PKEY_base_id(i_pkey))
-            return X509_V_ERR_SIGNATURE_ALGORITHM_MISMATCH;
-    }
+    return check_sig_alg_match(X509_get0_pubkey(issuer), subject);
+}
 
+/*-
+ * Check if certificate I<issuer> is allowed to issue certificate I<subject>
+ * according to the B<keyUsage> field of I<issuer> if present
+ * depending on any proxyCertInfo extension of I<subject>.
+ * Returns 0 for OK, or positive for reason for rejection
+ * where reason codes match those for X509_verify_cert().
+ */
+int x509_signing_allowed(const X509 *issuer, const X509 *subject)
+{
     if (subject->ex_flags & EXFLAG_PROXY) {
         if (ku_reject(issuer, KU_DIGITAL_SIGNATURE))
             return X509_V_ERR_KEYUSAGE_NO_DIGITAL_SIGNATURE;
@@ -863,8 +887,7 @@ int X509_check_issued(X509 *issuer, X509 *subject)
 
 int X509_check_akid(X509 *issuer, AUTHORITY_KEYID *akid)
 {
-
-    if (!akid)
+    if (akid == NULL)
         return X509_V_OK;
 
     /* Check key ids (if present) */
@@ -894,7 +917,7 @@ int X509_check_akid(X509 *issuer, AUTHORITY_KEYID *akid)
                 break;
             }
         }
-        if (nm && X509_NAME_cmp(nm, X509_get_issuer_name(issuer)))
+        if (nm != NULL && X509_NAME_cmp(nm, X509_get_issuer_name(issuer)) != 0)
             return X509_V_ERR_AKID_ISSUER_SERIAL_MISMATCH;
     }
     return X509_V_OK;
diff --git a/crypto/x509/x509_local.h b/crypto/x509/x509_local.h
index e174ae7611..a1fe4203b9 100644
--- a/crypto/x509/x509_local.h
+++ b/crypto/x509/x509_local.h
@@ -149,3 +149,6 @@ DEFINE_STACK_OF(STACK_OF_X509_NAME_ENTRY)
 
 void x509_set_signature_info(X509_SIG_INFO *siginf, const X509_ALGOR *alg,
                              const ASN1_STRING *sig);
+int x509_likely_issued(X509 *issuer, X509 *subject,
+                       OPENSSL_CTX *libctx, const char *propq);
+int x509_signing_allowed(const X509 *issuer, const X509 *subject);
diff --git a/crypto/x509/x509_txt.c b/crypto/x509/x509_txt.c
index 6ce8a722cc..63d8d95f3f 100644
--- a/crypto/x509/x509_txt.c
+++ b/crypto/x509/x509_txt.c
@@ -178,6 +178,9 @@ const char *X509_verify_cert_error_string(long n)
         return "subject signature algorithm and issuer public key algorithm mismatch";
     case X509_V_ERR_NO_ISSUER_PUBLIC_KEY:
         return "issuer certificate doesn't have a public key";
+    case X509_V_ERR_UNSUPPORTED_SIGNATURE_ALGORITHM:
+        return "Cannot find certificate signature algorithm";
+
     default:
         /* Printing an error number into a static buffer is not thread-safe */
         return "unknown certificate verification error";
diff --git a/crypto/x509/x509_vfy.c b/crypto/x509/x509_vfy.c
index ef149a2e28..a7541d8572 100644
--- a/crypto/x509/x509_vfy.c
+++ b/crypto/x509/x509_vfy.c
@@ -344,15 +344,9 @@ static X509 *find_issuer(X509_STORE_CTX *ctx, STACK_OF(X509) *sk, X509 *x)
 static int check_issued(X509_STORE_CTX *ctx, X509 *x, X509 *issuer)
 {
     int ret;
-    int ss;
-
-    if (x == issuer) {
-        ss = cert_self_signed(ctx, x);
-        if (ss < 0)
-            return 0;
-        return ss;
-    }
 
+    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) {
         int i;
@@ -367,7 +361,7 @@ static int check_issued(X509_STORE_CTX *ctx, X509 *x, X509 *issuer)
             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)) {
+            if (ch == issuer || X509_cmp(ch, issuer) == 0) {
                 ret = X509_V_ERR_PATH_LOOP;
                 break;
             }
@@ -1779,8 +1773,6 @@ static int internal_verify(X509_STORE_CTX *ctx)
      * is allowed to reset errors (at its own peril).
      */
     while (n >= 0) {
-        EVP_PKEY *pkey;
-
         /*
          * Skip signature check for self-signed certificates unless explicitly
          * asked for because it does not add any security and just wastes time.
@@ -1788,9 +1780,12 @@ static int internal_verify(X509_STORE_CTX *ctx)
          * 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);
+
             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))
+                if (!verify_cb_cert(ctx, xi, issuer_depth,
+                                    X509_V_ERR_UNABLE_TO_DECODE_ISSUER_PUBLIC_KEY))
                     return 0;
             } else if (X509_verify_ex(xs, pkey, ctx->libctx, ctx->propq) <= 0) {
                 if (!verify_cb_cert(ctx, xs, n,
@@ -2962,7 +2957,7 @@ static int build_chain(X509_STORE_CTX *ctx)
     SSL_DANE *dane = ctx->dane;
     int num = sk_X509_num(ctx->chain);
     X509 *cert = sk_X509_value(ctx->chain, num - 1);
-    int ss;
+    int self_signed = cert_self_signed(ctx, cert);
     STACK_OF(X509) *sktmp = NULL;
     unsigned int search;
     int may_trusted = 0;
@@ -2980,9 +2975,8 @@ static int build_chain(X509_STORE_CTX *ctx)
         return 0;
     }
 
-    ss = cert_self_signed(ctx, cert);
-    if (ss < 0) {
-        X509err(X509_F_BUILD_CHAIN, ERR_R_INTERNAL_ERROR);
+    self_signed = cert_self_signed(ctx, cert);
+    if (self_signed < 0) {
         ctx->error = X509_V_ERR_UNSPECIFIED;
         return 0;
     }
@@ -3122,7 +3116,7 @@ static int build_chain(X509_STORE_CTX *ctx)
                  * certificate among the ones from the trust store.
                  */
                 if ((search & S_DOALTERNATE) != 0) {
-                    if (!ossl_assert(num > i && i > 0 && ss == 0)) {
+                    if (!ossl_assert(num > i && i > 0 && !self_signed)) {
                         X509err(X509_F_BUILD_CHAIN, ERR_R_INTERNAL_ERROR);
                         X509_free(xtmp);
                         trust = X509_TRUST_REJECTED;
@@ -3150,7 +3144,7 @@ static int build_chain(X509_STORE_CTX *ctx)
                  * Self-signed untrusted certificates get replaced by their
                  * trusted matching issuer.  Otherwise, grow the chain.
                  */
-                if (ss == 0) {
+                if (!self_signed) {
                     if (!sk_X509_push(ctx->chain, x = xtmp)) {
                         X509_free(xtmp);
                         X509err(X509_F_BUILD_CHAIN, ERR_R_MALLOC_FAILURE);
@@ -3159,9 +3153,8 @@ static int build_chain(X509_STORE_CTX *ctx)
                         search = 0;
                         continue;
                     }
-                    ss = cert_self_signed(ctx, x);
-                    if (ss < 0) {
-                        X509err(X509_F_BUILD_CHAIN, ERR_R_INTERNAL_ERROR);
+                    self_signed = cert_self_signed(ctx, x);
+                    if (self_signed < 0) {
                         ctx->error = X509_V_ERR_UNSPECIFIED;
                         return 0;
                     }
@@ -3211,7 +3204,7 @@ static int build_chain(X509_STORE_CTX *ctx)
                         search = 0;
                         continue;
                     }
-                    if (ss == 0)
+                    if (!self_signed)
                         continue;
                 }
             }
@@ -3233,7 +3226,7 @@ static int build_chain(X509_STORE_CTX *ctx)
                 /* Search for a trusted issuer of a shorter chain */
                 search |= S_DOALTERNATE;
                 alt_untrusted = ctx->num_untrusted - 1;
-                ss = 0;
+                self_signed = 0;
             }
         }
 
@@ -3255,7 +3248,8 @@ static int build_chain(X509_STORE_CTX *ctx)
              * Once we run out of untrusted issuers, we stop looking for more
              * and start looking only in the trust store if enabled.
              */
-            xtmp = (ss || depth < num) ? NULL : find_issuer(ctx, sktmp, x);
+            xtmp = (self_signed || depth < num) ? NULL
+                                                : find_issuer(ctx, sktmp, x);
             if (xtmp == NULL) {
                 search &= ~S_DOUNTRUSTED;
                 if (may_trusted)
@@ -3285,11 +3279,10 @@ static int build_chain(X509_STORE_CTX *ctx)
 
             x = xtmp;
             ++ctx->num_untrusted;
-            ss = cert_self_signed(ctx, xtmp);
-            if (ss < 0) {
-                X509err(X509_F_BUILD_CHAIN, ERR_R_INTERNAL_ERROR);
-                ctx->error = X509_V_ERR_UNSPECIFIED;
+            self_signed = cert_self_signed(ctx, xtmp);
+            if (self_signed < 0) {
                 sk_X509_free(sktmp);
+                ctx->error = X509_V_ERR_UNSPECIFIED;
                 return 0;
             }
 
@@ -3333,10 +3326,10 @@ static int build_chain(X509_STORE_CTX *ctx)
         if (DANETLS_ENABLED(dane) &&
             (!DANETLS_HAS_PKIX(dane) || dane->pdpth >= 0))
             return verify_cb_cert(ctx, NULL, num-1, X509_V_ERR_DANE_NO_MATCH);
-        if (ss && sk_X509_num(ctx->chain) == 1)
+        if (self_signed && sk_X509_num(ctx->chain) == 1)
             return verify_cb_cert(ctx, NULL, num-1,
                                   X509_V_ERR_DEPTH_ZERO_SELF_SIGNED_CERT);
-        if (ss)
+        if (self_signed)
             return verify_cb_cert(ctx, NULL, num-1,
                                   X509_V_ERR_SELF_SIGNED_CERT_IN_CHAIN);
         if (ctx->num_untrusted < num)