Fix some places where X509_up_ref is used

without error handling.

This takes up the ball from #11278
without trying to solve everything at once.

[extended tests]
Reviewed-by: NMatt Caswell <matt@openssl.org>
Reviewed-by: NKurt Roeckx <kurt@roeckx.be>
(Merged from https://github.com/openssl/openssl/pull/11850)

crypto/err/openssl.txt

@@ -1001,17 +1001,17 @@ PEM_F_D2I_PKCS8PRIVATEKEY_BIO:120:d2i_PKCS8PrivateKey_bio
 PEM_F_D2I_PKCS8PRIVATEKEY_FP:121:d2i_PKCS8PrivateKey_fp
 PEM_F_DO_B2I:132:do_b2i
 PEM_F_DO_B2I_BIO:133:do_b2i_bio
-PEM_F_OSSL_DO_BLOB_HEADER:134:ossl_do_blob_header
 PEM_F_DO_I2B:146:do_i2b
 PEM_F_DO_PK8PKEY:126:do_pk8pkey
 PEM_F_DO_PK8PKEY_FP:125:do_pk8pkey_fp
 PEM_F_DO_PVK_BODY:135:do_PVK_body
-PEM_F_OSSL_DO_PVK_HEADER:136:ossl_do_PVK_header
 PEM_F_GET_HEADER_AND_DATA:143:get_header_and_data
 PEM_F_GET_NAME:144:get_name
 PEM_F_I2B_PVK:137:i2b_PVK
 PEM_F_I2B_PVK_BIO:138:i2b_PVK_bio
 PEM_F_LOAD_IV:101:load_iv
+PEM_F_OSSL_DO_BLOB_HEADER:134:ossl_do_blob_header
+PEM_F_OSSL_DO_PVK_HEADER:136:ossl_do_PVK_header
 PEM_F_PEM_ASN1_READ:102:PEM_ASN1_read
 PEM_F_PEM_ASN1_READ_BIO:103:PEM_ASN1_read_bio
 PEM_F_PEM_ASN1_WRITE:104:PEM_ASN1_write
@@ -1874,6 +1874,7 @@ X509_F_X509_NAME_PRINT:117:X509_NAME_print
 X509_F_X509_OBJECT_NEW:150:X509_OBJECT_new
 X509_F_X509_PRINT_EX_FP:118:X509_print_ex_fp
 X509_F_X509_PUBKEY_DECODE:148:x509_pubkey_decode
+X509_F_X509_PUBKEY_GET:166:X509_PUBKEY_get
 X509_F_X509_PUBKEY_GET0:119:X509_PUBKEY_get0
 X509_F_X509_PUBKEY_SET:120:X509_PUBKEY_set
 X509_F_X509_REQ_CHECK_PRIVATE_KEY:144:X509_REQ_check_private_key

crypto/x509/x509_err.c

@@ -20,10 +20,10 @@ static const ERR_STRING_DATA X509_str_reasons[] = {
     {ERR_PACK(ERR_LIB_X509, 0, X509_R_BASE64_DECODE_ERROR),
     "base64 decode error"},
     {ERR_PACK(ERR_LIB_X509, 0, X509_R_CANT_CHECK_DH_KEY), "cant check dh key"},
-    {ERR_PACK(ERR_LIB_X509, 0, X509_R_CERT_ALREADY_IN_HASH_TABLE),
-    "cert already in hash table"},
     {ERR_PACK(ERR_LIB_X509, 0, X509_R_CERTIFICATE_VERIFICATION_FAILED),
     "certificate verification failed"},
+    {ERR_PACK(ERR_LIB_X509, 0, X509_R_CERT_ALREADY_IN_HASH_TABLE),
+    "cert already in hash table"},
     {ERR_PACK(ERR_LIB_X509, 0, X509_R_CRL_ALREADY_DELTA), "crl already delta"},
     {ERR_PACK(ERR_LIB_X509, 0, X509_R_CRL_VERIFY_FAILURE),
     "crl verify failure"},

crypto/x509/x509_vfy.c

@@ -139,10 +139,9 @@ static X509 *lookup_cert_match(X509_STORE_CTX *ctx, X509 *x)
         xtmp = sk_X509_value(certs, i);
         if (!X509_cmp(xtmp, x))
             break;
+        xtmp = NULL;
     }
-    if (i < sk_X509_num(certs))
-        X509_up_ref(xtmp);
-    else
+    if (xtmp != NULL && !X509_up_ref(xtmp))
         xtmp = NULL;
     sk_X509_pop_free(certs, X509_free);
     return xtmp;
@@ -275,17 +274,24 @@ int X509_verify_cert(X509_STORE_CTX *ctx)
         return -1;
     }
 
+    if (!X509_up_ref(ctx->cert)) {
+        X509err(X509_F_X509_VERIFY_CERT, ERR_R_INTERNAL_ERROR);
+        ctx->error = X509_V_ERR_UNSPECIFIED;
+        return -1;
+    }
+
     /*
      * first we make sure the chain we are going to build is present and that
      * the first entry is in place
      */
-    if (((ctx->chain = sk_X509_new_null()) == NULL) ||
-        (!sk_X509_push(ctx->chain, ctx->cert))) {
+    if ((ctx->chain = sk_X509_new_null()) == NULL
+            || !sk_X509_push(ctx->chain, ctx->cert)) {
+        X509_free(ctx->cert);
         X509err(X509_F_X509_VERIFY_CERT, ERR_R_MALLOC_FAILURE);
         ctx->error = X509_V_ERR_OUT_OF_MEM;
         return -1;
     }
-    X509_up_ref(ctx->cert);
+
     ctx->num_untrusted = 1;
 
     /* If the peer's public key is too weak, we can stop early. */
@@ -370,11 +376,15 @@ static int check_issued(X509_STORE_CTX *ctx, X509 *x, X509 *issuer)
 static int get_issuer_sk(X509 **issuer, X509_STORE_CTX *ctx, X509 *x)
 {
     *issuer = find_issuer(ctx, ctx->other_ctx, x);
-    if (*issuer) {
-        X509_up_ref(*issuer);
-        return 1;
-    } else
-        return 0;
+
+    if (*issuer == NULL || !X509_up_ref(*issuer))
+        goto err;
+
+    return 1;
+
+ err:
+    *issuer = NULL;
+    return 0;
 }
 
 static STACK_OF(X509) *lookup_certs_sk(X509_STORE_CTX *ctx,
@@ -387,15 +397,20 @@ static STACK_OF(X509) *lookup_certs_sk(X509_STORE_CTX *ctx,
     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_up_ref(x)) {
+                X509err(X509_F_LOOKUP_CERTS_SK, ERR_R_INTERNAL_ERROR);
+                ctx->error = X509_V_ERR_UNSPECIFIED;
+                return NULL;
+            }
             if (sk == NULL)
                 sk = sk_X509_new_null();
-            if (sk == NULL || sk_X509_push(sk, x) == 0) {
+            if (sk == NULL || !sk_X509_push(sk, x)) {
+                X509_free(x);
                 sk_X509_pop_free(sk, X509_free);
                 X509err(X509_F_LOOKUP_CERTS_SK, ERR_R_MALLOC_FAILURE);
                 ctx->error = X509_V_ERR_OUT_OF_MEM;
                 return NULL;
             }
-            X509_up_ref(x);
         }
     }
     return sk;
@@ -3244,7 +3259,16 @@ static int build_chain(X509_STORE_CTX *ctx)
             /* Drop this issuer from future consideration */
             (void) sk_X509_delete_ptr(sktmp, xtmp);
 
+            if (!X509_up_ref(xtmp)) {
+                X509err(X509_F_BUILD_CHAIN, ERR_R_INTERNAL_ERROR);
+                trust = X509_TRUST_REJECTED;
+                ctx->error = X509_V_ERR_UNSPECIFIED;
+                search = 0;
+                continue;
+            }
+
             if (!sk_X509_push(ctx->chain, xtmp)) {
+                X509_free(xtmp);
                 X509err(X509_F_BUILD_CHAIN, ERR_R_MALLOC_FAILURE);
                 trust = X509_TRUST_REJECTED;
                 ctx->error = X509_V_ERR_OUT_OF_MEM;
@@ -3252,7 +3276,7 @@ static int build_chain(X509_STORE_CTX *ctx)
                 continue;
             }
 
-            X509_up_ref(x = xtmp);
+            x = xtmp;
             ++ctx->num_untrusted;
             ss = cert_self_signed(ctx, xtmp);
             if (ss < 0) {

crypto/x509/x_pubkey.c

@@ -219,8 +219,11 @@ EVP_PKEY *X509_PUBKEY_get0(X509_PUBKEY *key)
 EVP_PKEY *X509_PUBKEY_get(X509_PUBKEY *key)
 {
     EVP_PKEY *ret = X509_PUBKEY_get0(key);
-    if (ret != NULL)
-        EVP_PKEY_up_ref(ret);
+
+    if (ret != NULL && !EVP_PKEY_up_ref(ret)) {
+        X509err(X509_F_X509_PUBKEY_GET, ERR_R_INTERNAL_ERROR);
+        ret = NULL;
+    }
     return ret;
 }
 

include/openssl/x509err.h

@@ -28,6 +28,7 @@ int ERR_load_X509_strings(void);
 #  define X509_F_ADD_CERT_DIR                              0
 #  define X509_F_BUILD_CHAIN                               0
 #  define X509_F_BY_FILE_CTRL                              0
+#  define X509_F_CACHE_OBJECTS                             0
 #  define X509_F_CHECK_NAME_CONSTRAINTS                    0
 #  define X509_F_CHECK_POLICY                              0
 #  define X509_F_COMMON_VERIFY_SM2                         0
@@ -68,6 +69,7 @@ int ERR_load_X509_strings(void);
 #  define X509_F_X509_OBJECT_NEW                           0
 #  define X509_F_X509_PRINT_EX_FP                          0
 #  define X509_F_X509_PUBKEY_DECODE                        0
+#  define X509_F_X509_PUBKEY_GET                           0
 #  define X509_F_X509_PUBKEY_GET0                          0
 #  define X509_F_X509_PUBKEY_SET                           0
 #  define X509_F_X509_REQ_CHECK_PRIVATE_KEY                0
@@ -101,8 +103,8 @@ int ERR_load_X509_strings(void);
 # define X509_R_BAD_X509_FILETYPE                         100
 # define X509_R_BASE64_DECODE_ERROR                       118
 # define X509_R_CANT_CHECK_DH_KEY                         114
-# define X509_R_CERT_ALREADY_IN_HASH_TABLE                101
 # define X509_R_CERTIFICATE_VERIFICATION_FAILED           139
+# define X509_R_CERT_ALREADY_IN_HASH_TABLE                101
 # define X509_R_CRL_ALREADY_DELTA                         127
 # define X509_R_CRL_VERIFY_FAILURE                        131
 # define X509_R_IDP_MISMATCH                              128