x509_vfy.c: Sort out return values 0 vs. -1 (failure/internal error)

Also simplify first part of verify_chain()

Reviewed-by: Paul Dale <pauli@openssl.org>
(Merged from https://github.com/openssl/openssl/pull/14095)

1 files changed, 104 insertions, 73 deletions

diff --git a/crypto/x509/x509_vfy.c b/crypto/x509/x509_vfy.c
index d723239cb0..c3b0ba934a 100644
--- a/crypto/x509/x509_vfy.c
+++ b/crypto/x509/x509_vfy.c
@@ -106,19 +106,23 @@ int X509_self_signed(X509 *cert, int verify_signature)
     return X509_verify(cert, pkey);
 }
 
-/* Given a certificate try and find an exact match in the store */
-static X509 *lookup_cert_match(X509_STORE_CTX *ctx, X509 *x)
+/*
+ * Given a certificate, try and find an exact match in the store.
+ * Returns 1 on success, 0 on not found, -1 on internal error.
+ */
+static int lookup_cert_match(X509 **result, X509_STORE_CTX *ctx, X509 *x)
 {
     STACK_OF(X509) *certs;
     X509 *xtmp = NULL;
-    int i;
+    int i, ret;
 
+    *result = NULL;
     /* Lookup all certs with matching subject name */
     ERR_set_mark();
     certs = ctx->lookup_certs(ctx, X509_get_subject_name(x));
     ERR_pop_to_mark();
     if (certs == NULL)
-        return NULL;
+        return -1;
     /* Look for exact match */
     for (i = 0; i < sk_X509_num(certs); i++) {
         xtmp = sk_X509_value(certs, i);
@@ -126,10 +130,15 @@ static X509 *lookup_cert_match(X509_STORE_CTX *ctx, X509 *x)
             break;
         xtmp = NULL;
     }
-    if (xtmp != NULL && !X509_up_ref(xtmp))
-        xtmp = NULL;
+    ret = xtmp != NULL;
+    if (ret) {
+        if (!X509_up_ref(xtmp))
+            ret = -1;
+        else
+            *result = xtmp;
+    }
     sk_X509_pop_free(certs, X509_free);
-    return xtmp;
+    return ret;
 }
 
 /*-
@@ -194,6 +203,7 @@ static int check_auth_level(X509_STORE_CTX *ctx)
     return 1;
 }
 
+/* Returns -1 on internal error */
 static int verify_chain(X509_STORE_CTX *ctx)
 {
     int err;
@@ -213,18 +223,18 @@ static int verify_chain(X509_STORE_CTX *ctx)
 
     /* Verify chain signatures and expiration times */
     ok = ctx->verify != NULL ? ctx->verify(ctx) : internal_verify(ctx);
-    if (!ok)
-        return 0;
+    if (ok <= 0)
+        return ok;
 
-    if ((ok = check_name_constraints(ctx)) == 0)
-        return 0;
+    if ((ok = check_name_constraints(ctx)) <= 0)
+        return ok;
 
 #ifndef OPENSSL_NO_RFC3779
     /* RFC 3779 path validation, now that CRL check has been done */
-    if ((ok = X509v3_asid_validate_path(ctx)) == 0)
-        return 0;
-    if ((ok = X509v3_addr_validate_path(ctx)) == 0)
-        return 0;
+    if ((ok = X509v3_asid_validate_path(ctx)) <= 0)
+        return ok;
+    if ((ok = X509v3_addr_validate_path(ctx)) <= 0)
+        return ok;
 #endif
 
     /* If we get this far evaluate policies */
@@ -332,28 +342,32 @@ static int check_issued(ossl_unused X509_STORE_CTX *ctx, X509 *x, X509 *issuer)
     return x509_likely_issued(issuer, x) == X509_V_OK;
 }
 
-/* Alternative lookup method: look from a STACK stored in other_ctx */
+/*
+ * Alternative lookup method: look from a STACK stored in other_ctx.
+ * Returns -1 on internal error.
+ */
 static int get_issuer_sk(X509 **issuer, X509_STORE_CTX *ctx, X509 *x)
 {
     *issuer = find_issuer(ctx, ctx->other_ctx, x);
-    if (*issuer != NULL && X509_up_ref(*issuer))
-        return 1;
-
-    *issuer = NULL;
+    if (*issuer != NULL)
+        return X509_up_ref(*issuer) ? 1 : -1;
     return 0;
 }
 
+/* Returns NULL on internal error (such as out of memory) */
 static STACK_OF(X509) *lookup_certs_sk(X509_STORE_CTX *ctx,
                                        const X509_NAME *nm)
 {
-    STACK_OF(X509) *sk = NULL;
+    STACK_OF(X509) *sk = sk_X509_new_null();
     X509 *x;
     int i;
 
+    if (sk == NULL)
+        return NULL;
     for (i = 0; i < sk_X509_num(ctx->other_ctx); i++) {
         x = sk_X509_value(ctx->other_ctx, i);
         if (X509_NAME_cmp(nm, X509_get_subject_name(x)) == 0) {
-            if (!X509_add_cert_new(&sk, x, X509_ADD_FLAG_UP_REF)) {
+            if (!X509_add_cert(sk, x, X509_ADD_FLAG_UP_REF)) {
                 sk_X509_pop_free(sk, X509_free);
                 ctx->error = X509_V_ERR_OUT_OF_MEM;
                 return NULL;
@@ -366,6 +380,7 @@ static STACK_OF(X509) *lookup_certs_sk(X509_STORE_CTX *ctx,
 /*
  * Check EE or CA certificate purpose.  For trusted certificates explicit local
  * auxiliary trust can be used to override EKU-restrictions.
+ * Sadly, returns 0 also on internal error.
  */
 static int check_purpose(X509_STORE_CTX *ctx, X509 *x, int purpose, int depth,
                          int must_be_ca)
@@ -414,7 +429,10 @@ static int check_purpose(X509_STORE_CTX *ctx, X509 *x, int purpose, int depth,
     return verify_cb_cert(ctx, x, depth, X509_V_ERR_INVALID_PURPOSE);
 }
 
-/* Check extensions of a cert chain for consistency with the supplied purpose */
+/*
+ * Check extensions of a cert chain for consistency with the supplied purpose.
+ * Sadly, returns 0 also on internal error.
+ */
 static int check_chain(X509_STORE_CTX *ctx)
 {
     int i, must_be_ca, plen = 0;
@@ -596,7 +614,7 @@ static int has_san_id(X509 *x, int gtype)
     GENERAL_NAMES *gs = X509_get_ext_d2i(x, NID_subject_alt_name, NULL, NULL);
 
     if (gs == NULL)
-        return 0;
+        return -1;
 
     for (i = 0; i < sk_GENERAL_NAME_num(gs); i++) {
         GENERAL_NAME *g = sk_GENERAL_NAME_value(gs, i);
@@ -610,6 +628,7 @@ static int has_san_id(X509 *x, int gtype)
     return ret;
 }
 
+/* Returns -1 on internal error */
 static int check_name_constraints(X509_STORE_CTX *ctx)
 {
     int i;
@@ -672,7 +691,7 @@ static int check_name_constraints(X509_STORE_CTX *ctx)
             if (tmpsubject == NULL) {
                 ERR_raise(ERR_LIB_X509, ERR_R_MALLOC_FAILURE);
                 ctx->error = X509_V_ERR_OUT_OF_MEM;
-                return 0;
+                return -1;
             }
 
             tmpentry = X509_NAME_delete_entry(tmpsubject, last_loc);
@@ -701,6 +720,7 @@ static int check_name_constraints(X509_STORE_CTX *ctx)
 
             if (nc) {
                 int rv = NAME_CONSTRAINTS_check(x, nc);
+                int ret = 1;
 
                 /* If EE certificate check commonName too */
                 if (rv == X509_V_OK && i == 0
@@ -708,14 +728,16 @@ static int check_name_constraints(X509_STORE_CTX *ctx)
                         & X509_CHECK_FLAG_NEVER_CHECK_SUBJECT) == 0
                     && ((ctx->param->hostflags
                          & X509_CHECK_FLAG_ALWAYS_CHECK_SUBJECT) != 0
-                        || !has_san_id(x, GEN_DNS)))
+                        || (ret = has_san_id(x, GEN_DNS)) == 0))
                     rv = NAME_CONSTRAINTS_check_CN(x, nc);
+                if (ret < 0)
+                    return ret;
 
                 switch (rv) {
                 case X509_V_OK:
                     break;
                 case X509_V_ERR_OUT_OF_MEM:
-                    return 0;
+                    return -1;
                 default:
                     CB_FAIL_IF(1, ctx, x, i, rv);
                     break;
@@ -770,9 +792,10 @@ static int check_id(X509_STORE_CTX *ctx)
     return 1;
 }
 
+/* Returns -1 on internal error */
 static int check_trust(X509_STORE_CTX *ctx, int num_untrusted)
 {
-    int i;
+    int i, res;
     X509 *x = NULL;
     X509 *mx;
     SSL_DANE *dane = ctx->dane;
@@ -784,11 +807,9 @@ static int check_trust(X509_STORE_CTX *ctx, int num_untrusted)
      * match, we're done, otherwise we'll merely record the match depth.
      */
     if (DANETLS_HAS_TA(dane) && num_untrusted > 0 && num_untrusted < num) {
-        switch (trust = check_dane_issuer(ctx, num_untrusted)) {
-        case X509_TRUST_TRUSTED:
-        case X509_TRUST_REJECTED:
+        trust = check_dane_issuer(ctx, num_untrusted);
+        if (trust != X509_TRUST_UNTRUSTED)
             return trust;
-        }
     }
 
     /*
@@ -825,7 +846,9 @@ static int check_trust(X509_STORE_CTX *ctx, int num_untrusted)
          */
         i = 0;
         x = sk_X509_value(ctx->chain, i);
-        mx = lookup_cert_match(ctx, x);
+        res = lookup_cert_match(&mx, ctx, x);
+        if (res < 0)
+            return res;
         if (mx == NULL)
             return X509_TRUST_UNTRUSTED;
 
@@ -867,6 +890,7 @@ static int check_trust(X509_STORE_CTX *ctx, int num_untrusted)
     return X509_TRUST_UNTRUSTED;
 }
 
+/* Sadly, returns 0 also on internal error. */
 static int check_revocation(X509_STORE_CTX *ctx)
 {
     int i = 0, last = 0, ok = 0;
@@ -890,6 +914,7 @@ static int check_revocation(X509_STORE_CTX *ctx)
     return 1;
 }
 
+/* Sadly, returns 0 also on internal error. */
 static int check_cert(X509_STORE_CTX *ctx)
 {
     X509_CRL *crl = NULL, *dcrl = NULL;
@@ -1604,21 +1629,15 @@ static int check_policy(X509_STORE_CTX *ctx)
      * was verified via a bare public key, and pop it off right after the
      * X509_policy_check() call.
      */
-    if (ctx->bare_ta_signed && !sk_X509_push(ctx->chain, NULL)) {
-        ERR_raise(ERR_LIB_X509, ERR_R_MALLOC_FAILURE);
-        ctx->error = X509_V_ERR_OUT_OF_MEM;
-        return 0;
-    }
+    if (ctx->bare_ta_signed && !sk_X509_push(ctx->chain, NULL))
+        goto memerr;
     ret = X509_policy_check(&ctx->tree, &ctx->explicit_policy, ctx->chain,
                             ctx->param->policies, ctx->param->flags);
     if (ctx->bare_ta_signed)
         (void)sk_X509_pop(ctx->chain);
 
-    if (ret == X509_PCY_TREE_INTERNAL) {
-        ERR_raise(ERR_LIB_X509, ERR_R_MALLOC_FAILURE);
-        ctx->error = X509_V_ERR_OUT_OF_MEM;
-        return 0;
-    }
+    if (ret == X509_PCY_TREE_INTERNAL)
+        goto memerr;
     /* Invalid or inconsistent extensions */
     if (ret == X509_PCY_TREE_INVALID) {
         int i;
@@ -1655,6 +1674,11 @@ static int check_policy(X509_STORE_CTX *ctx)
     }
 
     return 1;
+
+ memerr:
+    ERR_raise(ERR_LIB_X509, ERR_R_MALLOC_FAILURE);
+    ctx->error = X509_V_ERR_OUT_OF_MEM;
+    return -1;
 }
 
 /*-
@@ -1690,7 +1714,10 @@ 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 */
+/*
+ * Verify the issuer signatures and cert times of ctx->chain.
+ * Sadly, returns 0 also on internal error.
+ */
 static int internal_verify(X509_STORE_CTX *ctx)
 {
     int n = sk_X509_num(ctx->chain) - 1;
@@ -1964,7 +1991,10 @@ int X509_get_pubkey_parameters(EVP_PKEY *pkey, STACK_OF(X509) *chain)
     return 1;
 }
 
-/* Make a delta CRL as the difference between two full CRLs */
+/*
+ * Make a delta CRL as the difference between two full CRLs.
+ * Sadly, returns NULL also on internal error.
+ */
 X509_CRL *X509_CRL_diff(X509_CRL *base, X509_CRL *newer,
                         EVP_PKEY *skey, const EVP_MD *md, unsigned int flags)
 {
@@ -2628,6 +2658,7 @@ static unsigned char *dane_i2d(X509 *cert, uint8_t selector,
 
 #define DANETLS_NONE 256 /* impossible uint8_t */
 
+/* Returns -1 on internal error */
 static int dane_match(X509_STORE_CTX *ctx, X509 *cert, int depth)
 {
     SSL_DANE *dane = ctx->dane;
@@ -2770,6 +2801,7 @@ static int dane_match(X509_STORE_CTX *ctx, X509 *cert, int depth)
     return matched;
 }
 
+/* Returns -1 on internal error */
 static int check_dane_issuer(X509_STORE_CTX *ctx, int depth)
 {
     SSL_DANE *dane = ctx->dane;
@@ -2786,7 +2818,7 @@ static int check_dane_issuer(X509_STORE_CTX *ctx, int depth)
      */
     cert = sk_X509_value(ctx->chain, depth);
     if (cert != NULL && (matched = dane_match(ctx, cert, depth)) < 0)
-        return  X509_TRUST_REJECTED;
+        return matched;
     if (matched > 0) {
         ctx->num_untrusted = depth - 1;
         return X509_TRUST_TRUSTED;
@@ -2850,6 +2882,7 @@ static int check_leaf_suiteb(X509_STORE_CTX *ctx, X509 *cert)
     return 1;
 }
 
+/* Returns -1 on internal error */
 static int dane_verify(X509_STORE_CTX *ctx)
 {
     X509 *cert = ctx->cert;
@@ -2874,8 +2907,8 @@ static int dane_verify(X509_STORE_CTX *ctx)
     matched = dane_match(ctx, ctx->cert, 0);
     done = matched != 0 || (!DANETLS_HAS_TA(dane) && dane->mdpth < 0);
 
-    if (done)
-        X509_get_pubkey_parameters(NULL, ctx->chain);
+    if (done && !X509_get_pubkey_parameters(NULL, ctx->chain))
+        return -1;
 
     if (matched > 0) {
         /* Callback invoked as needed */
@@ -2912,7 +2945,10 @@ static int dane_verify(X509_STORE_CTX *ctx)
     return verify_chain(ctx);
 }
 
-/* Get issuer, without duplicate suppression */
+/*
+ * Get issuer, without duplicate suppression
+ * Returns -1 on internal error.
+ */
 static int get_issuer(X509 **issuer, X509_STORE_CTX *ctx, X509 *cert)
 {
     STACK_OF(X509) *saved_chain = ctx->chain;
@@ -2925,6 +2961,7 @@ static int get_issuer(X509 **issuer, X509_STORE_CTX *ctx, X509 *cert)
     return ok;
 }
 
+/* Returns -1 on internal error */
 static int build_chain(X509_STORE_CTX *ctx)
 {
     SSL_DANE *dane = ctx->dane;
@@ -2971,11 +3008,8 @@ static int build_chain(X509_STORE_CTX *ctx)
      * typically the content of the peer's certificate message) so can make
      * multiple passes over it, while free to remove elements as we go.
      */
-    if ((sk_untrusted = sk_X509_dup(ctx->untrusted)) == NULL) {
-        ERR_raise(ERR_LIB_X509, ERR_R_MALLOC_FAILURE);
-        ctx->error = X509_V_ERR_OUT_OF_MEM;
-        return 0;
-    }
+    if ((sk_untrusted = sk_X509_dup(ctx->untrusted)) == NULL)
+        goto memerr;
 
     /*
      * If we got any "DANE-TA(2) Cert(0) Full(0)" trust anchors from DNS, add
@@ -2988,15 +3022,11 @@ static int build_chain(X509_STORE_CTX *ctx)
      * this to change. ]
      */
     if (DANETLS_ENABLED(dane) && dane->certs != NULL) {
-        if (sk_untrusted == NULL && (sk_untrusted = sk_X509_new_null()) == NULL) {
-            ERR_raise(ERR_LIB_X509, ERR_R_MALLOC_FAILURE);
-            ctx->error = X509_V_ERR_OUT_OF_MEM;
-            return 0;
-        }
+        if (sk_untrusted == NULL && (sk_untrusted = sk_X509_new_null()) == NULL)
+            goto memerr;
         if (!X509_add_certs(sk_untrusted, dane->certs, X509_ADD_FLAG_DEFAULT)) {
             sk_X509_free(sk_untrusted);
-            ctx->error = X509_V_ERR_OUT_OF_MEM;
-            return 0;
+            goto memerr;
         }
     }
 
@@ -3056,7 +3086,7 @@ static int build_chain(X509_STORE_CTX *ctx)
             ok = num > depth ? 0 : get_issuer(&issuer, ctx, curr);
 
             if (ok < 0) {
-                trust = X509_TRUST_REJECTED;
+                trust = -1;
                 ctx->error = X509_V_ERR_STORE_LOOKUP;
                 break;
             }
@@ -3078,11 +3108,8 @@ static int build_chain(X509_STORE_CTX *ctx)
                  */
                 if ((search & S_DOALTERNATE) != 0) {
                     if (!ossl_assert(num > i && i > 0 && !self_signed)) {
-                        ERR_raise(ERR_LIB_X509, ERR_R_INTERNAL_ERROR);
                         X509_free(issuer);
-                        trust = X509_TRUST_REJECTED;
-                        ctx->error = X509_V_ERR_UNSPECIFIED;
-                        break;
+                        goto int_err;
                     }
                     search &= ~S_DOALTERNATE;
                     for (; num > i; --num)
@@ -3110,10 +3137,7 @@ static int build_chain(X509_STORE_CTX *ctx)
                         goto int_err;
                     if (!sk_X509_push(ctx->chain, curr)) {
                         X509_free(issuer);
-                        ERR_raise(ERR_LIB_X509, ERR_R_MALLOC_FAILURE);
-                        trust = X509_TRUST_REJECTED;
-                        ctx->error = X509_V_ERR_OUT_OF_MEM;
-                        break;
+                        goto memerr;
                     }
                 } else if (num == ctx->num_untrusted) {
                     /*
@@ -3153,8 +3177,7 @@ static int build_chain(X509_STORE_CTX *ctx)
                         goto int_err;
                     search &= ~S_DOUNTRUSTED;
                     trust = check_trust(ctx, num);
-                    if (trust == X509_TRUST_TRUSTED
-                            || trust == X509_TRUST_REJECTED)
+                    if (trust != X509_TRUST_UNTRUSTED)
                         break;
                     if (!self_signed)
                         continue;
@@ -3223,6 +3246,9 @@ static int build_chain(X509_STORE_CTX *ctx)
     }
     sk_X509_free(sk_untrusted);
 
+    if (trust < 0) /* internal error */
+        return trust;
+
     /*
      * Last chance to make a trusted chain, either bare DANE-TA public-key
      * signers, or else direct leaf PKIX trust.
@@ -3264,7 +3290,12 @@ static int build_chain(X509_STORE_CTX *ctx)
     sk_X509_free(sk_untrusted);
     ERR_raise(ERR_LIB_X509, ERR_R_INTERNAL_ERROR);
     ctx->error = X509_V_ERR_UNSPECIFIED;
-    return 0;
+    return -1;
+
+ memerr:
+    ERR_raise(ERR_LIB_X509, ERR_R_MALLOC_FAILURE);
+    ctx->error = X509_V_ERR_OUT_OF_MEM;
+    return -1;
 }
 
 static const int minbits_table[] = { 80, 112, 128, 192, 256 };