From e9e7b5df865c0bcd0a99d8146ec05378892a36e1 Mon Sep 17 00:00:00 2001 From: Bernd Edlinger Date: Sun, 17 May 2020 14:45:28 +0200 Subject: [PATCH] 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: Matt Caswell Reviewed-by: Kurt Roeckx (Merged from https://github.com/openssl/openssl/pull/11850) --- crypto/err/openssl.txt | 5 ++-- crypto/x509/x509_err.c | 4 +-- crypto/x509/x509_vfy.c | 52 ++++++++++++++++++++++++++++----------- crypto/x509/x_pubkey.c | 7 ++++-- include/openssl/x509err.h | 4 ++- 5 files changed, 51 insertions(+), 21 deletions(-) diff --git a/crypto/err/openssl.txt b/crypto/err/openssl.txt index 9d5e960841..4451ba95a1 100644 --- a/crypto/err/openssl.txt +++ b/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 diff --git a/crypto/x509/x509_err.c b/crypto/x509/x509_err.c index e37024b59f..450f2a7930 100644 --- a/crypto/x509/x509_err.c +++ b/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"}, diff --git a/crypto/x509/x509_vfy.c b/crypto/x509/x509_vfy.c index fb0469183f..75c5c0e201 100644 --- a/crypto/x509/x509_vfy.c +++ b/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) { diff --git a/crypto/x509/x_pubkey.c b/crypto/x509/x_pubkey.c index 23e92f2b39..d3e6af2552 100644 --- a/crypto/x509/x_pubkey.c +++ b/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; } diff --git a/include/openssl/x509err.h b/include/openssl/x509err.h index 34cd706abb..19743b5987 100644 --- a/include/openssl/x509err.h +++ b/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 -- 2.25.1