From e077455e9e57ed4ee4676996b4a9aa11df6327a6 Mon Sep 17 00:00:00 2001 From: Richard Levitte Date: Thu, 29 Sep 2022 13:57:34 +0200 Subject: Stop raising ERR_R_MALLOC_FAILURE in most places Since OPENSSL_malloc() and friends report ERR_R_MALLOC_FAILURE, and at least handle the file name and line number they are called from, there's no need to report ERR_R_MALLOC_FAILURE where they are called directly, or when SSLfatal() and RLAYERfatal() is used, the reason `ERR_R_MALLOC_FAILURE` is changed to `ERR_R_CRYPTO_LIB`. There were a number of places where `ERR_R_MALLOC_FAILURE` was reported even though it was a function from a different sub-system that was called. Those places are changed to report ERR_R_{lib}_LIB, where {lib} is the name of that sub-system. Some of them are tricky to get right, as we have a lot of functions that belong in the ASN1 sub-system, and all the `sk_` calls or from the CRYPTO sub-system. Some extra adaptation was necessary where there were custom OPENSSL_malloc() wrappers, and some bugs are fixed alongside these changes. Reviewed-by: Tomas Mraz Reviewed-by: Hugo Landau (Merged from https://github.com/openssl/openssl/pull/19301) --- engines/e_capi.c | 126 ++++++++++++++++++++++++----------------------- engines/e_dasync.c | 5 +- engines/e_loader_attic.c | 37 +++++--------- 3 files changed, 79 insertions(+), 89 deletions(-) (limited to 'engines') diff --git a/engines/e_capi.c b/engines/e_capi.c index 3c747cbecd..ffc5bf7a2a 100644 --- a/engines/e_capi.c +++ b/engines/e_capi.c @@ -323,7 +323,6 @@ static int capi_ctrl(ENGINE *e, int cmd, long i, void *p, void (*f) (void)) ctx->storename = tmpstr; CAPI_trace(ctx, "Setting store name to %s\n", p); } else { - CAPIerr(CAPI_F_CAPI_CTRL, ERR_R_MALLOC_FAILURE); ret = 0; } break; @@ -350,7 +349,6 @@ static int capi_ctrl(ENGINE *e, int cmd, long i, void *p, void (*f) (void)) ctx->debug_file = tmpstr; CAPI_trace(ctx, "Setting debug file to %s\n", ctx->debug_file); } else { - CAPIerr(CAPI_F_CAPI_CTRL, ERR_R_MALLOC_FAILURE); ret = 0; } break; @@ -417,8 +415,10 @@ static int capi_init(ENGINE *e) if (capi_idx < 0) { capi_idx = ENGINE_get_ex_new_index(0, NULL, NULL, NULL, 0); - if (capi_idx < 0) - goto memerr; + if (capi_idx < 0) { + CAPIerr(CAPI_F_CAPI_INIT, ERR_R_ENGINE_LIB); + goto err; + } cert_capi_idx = X509_get_ex_new_index(0, NULL, NULL, NULL, 0); @@ -437,7 +437,8 @@ static int capi_init(ENGINE *e) RSA_meth_get_bn_mod_exp(ossl_rsa_meth)) || !RSA_meth_set_finish(capi_rsa_method, capi_rsa_free) || !RSA_meth_set_sign(capi_rsa_method, capi_rsa_sign)) { - goto memerr; + CAPIerr(CAPI_F_CAPI_INIT, ERR_R_RSA_LIB); + goto err; } # ifndef OPENSSL_NO_DSA @@ -452,14 +453,17 @@ static int capi_init(ENGINE *e) DSA_meth_get_mod_exp(ossl_dsa_meth)) || !DSA_meth_set_bn_mod_exp(capi_dsa_method, DSA_meth_get_bn_mod_exp(ossl_dsa_meth))) { - goto memerr; + CAPIerr(CAPI_F_CAPI_INIT, ERR_R_DSA_LIB); + goto err; } # endif } ctx = capi_ctx_new(); - if (ctx == NULL) - goto memerr; + if (ctx == NULL) { + CAPIerr(CAPI_F_CAPI_INIT, ERR_R_CAPI_LIB); + goto err; + } ENGINE_set_ex_data(e, capi_idx, ctx); @@ -488,11 +492,8 @@ static int capi_init(ENGINE *e) return 1; - memerr: - CAPIerr(CAPI_F_CAPI_INIT, ERR_R_MALLOC_FAILURE); + err: return 0; - - return 1; } static int capi_destroy(ENGINE *e) @@ -655,7 +656,7 @@ static EVP_PKEY *capi_get_pkey(ENGINE *eng, CAPI_KEY *key) pubkey = OPENSSL_malloc(len); if (pubkey == NULL) - goto memerr; + goto err; if (!CryptExportKey(key->key, 0, PUBLICKEYBLOB, 0, pubkey, &len)) { CAPIerr(CAPI_F_CAPI_GET_PKEY, CAPI_R_PUBKEY_EXPORT_ERROR); @@ -684,8 +685,10 @@ static EVP_PKEY *capi_get_pkey(ENGINE *eng, CAPI_KEY *key) } rsa_modulus = (unsigned char *)(rp + 1); rkey = RSA_new_method(eng); - if (!rkey) - goto memerr; + if (!rkey) { + CAPIerr(CAPI_F_CAPI_GET_PKEY, ERR_R_RSA_LIB); + goto err; + } e = BN_new(); n = BN_new(); @@ -693,22 +696,29 @@ static EVP_PKEY *capi_get_pkey(ENGINE *eng, CAPI_KEY *key) if (e == NULL || n == NULL) { BN_free(e); BN_free(n); - goto memerr; + CAPIerr(CAPI_F_CAPI_GET_PKEY, ERR_R_BN_LIB); + goto err; } RSA_set0_key(rkey, n, e, NULL); - if (!BN_set_word(e, rp->pubexp)) - goto memerr; + if (!BN_set_word(e, rp->pubexp)) { + CAPIerr(CAPI_F_CAPI_GET_PKEY, ERR_R_BN_LIB); + goto err; + } rsa_modlen = rp->bitlen / 8; - if (!lend_tobn(n, rsa_modulus, rsa_modlen)) - goto memerr; + if (!lend_tobn(n, rsa_modulus, rsa_modlen)) { + CAPIerr(CAPI_F_CAPI_GET_PKEY, ERR_R_BN_LIB); + goto err; + } RSA_set_ex_data(rkey, rsa_capi_idx, key); - if ((ret = EVP_PKEY_new()) == NULL) - goto memerr; + if ((ret = EVP_PKEY_new()) == NULL) { + CAPIerr(CAPI_F_CAPI_GET_PKEY, ERR_R_EVP_LIB); + goto err; + } EVP_PKEY_assign_RSA(ret, rkey); rkey = NULL; @@ -731,8 +741,10 @@ static EVP_PKEY *capi_get_pkey(ENGINE *eng, CAPI_KEY *key) dsa_plen = dp->bitlen / 8; btmp = (unsigned char *)(dp + 1); dkey = DSA_new_method(eng); - if (!dkey) - goto memerr; + if (!dkey) { + CAPIerr(CAPI_F_CAPI_GET_PKEY, ERR_R_DSA_LIB); + goto err; + } p = BN_new(); q = BN_new(); g = BN_new(); @@ -742,27 +754,38 @@ static EVP_PKEY *capi_get_pkey(ENGINE *eng, CAPI_KEY *key) BN_free(q); BN_free(g); BN_free(pub_key); - goto memerr; + CAPIerr(CAPI_F_CAPI_GET_PKEY, ERR_R_BN_LIB); + goto err; } DSA_set0_pqg(dkey, p, q, g); DSA_set0_key(dkey, pub_key, NULL); - if (!lend_tobn(p, btmp, dsa_plen)) - goto memerr; + if (!lend_tobn(p, btmp, dsa_plen)) { + CAPIerr(CAPI_F_CAPI_GET_PKEY, ERR_R_CAPI_LIB); + goto err; + } btmp += dsa_plen; - if (!lend_tobn(q, btmp, 20)) - goto memerr; + if (!lend_tobn(q, btmp, 20)) { + CAPIerr(CAPI_F_CAPI_GET_PKEY, ERR_R_CAPI_LIB); + goto err; + } btmp += 20; - if (!lend_tobn(g, btmp, dsa_plen)) - goto memerr; + if (!lend_tobn(g, btmp, dsa_plen)) { + CAPIerr(CAPI_F_CAPI_GET_PKEY, ERR_R_CAPI_LIB); + goto err; + } btmp += dsa_plen; - if (!lend_tobn(pub_key, btmp, dsa_plen)) - goto memerr; + if (!lend_tobn(pub_key, btmp, dsa_plen)) { + CAPIerr(CAPI_F_CAPI_GET_PKEY, ERR_R_CAPI_LIB); + goto err; + } btmp += dsa_plen; DSA_set_ex_data(dkey, dsa_capi_idx, key); - if ((ret = EVP_PKEY_new()) == NULL) - goto memerr; + if ((ret = EVP_PKEY_new()) == NULL) { + CAPIerr(CAPI_F_CAPI_GET_PKEY, ERR_R_EVP_LIB); + goto err; + } EVP_PKEY_assign_DSA(ret, dkey); dkey = NULL; @@ -786,11 +809,6 @@ static EVP_PKEY *capi_get_pkey(ENGINE *eng, CAPI_KEY *key) } return ret; - - memerr: - CAPIerr(CAPI_F_CAPI_GET_PKEY, ERR_R_MALLOC_FAILURE); - goto err; - } static EVP_PKEY *capi_load_privkey(ENGINE *eng, const char *key_id, @@ -966,10 +984,8 @@ int capi_rsa_priv_dec(int flen, const unsigned char *from, } /* Create temp reverse order version of input */ - if ((tmpbuf = OPENSSL_malloc(flen)) == NULL) { - CAPIerr(CAPI_F_CAPI_RSA_PRIV_DEC, ERR_R_MALLOC_FAILURE); + if ((tmpbuf = OPENSSL_malloc(flen)) == NULL) return -1; - } for (i = 0; i < flen; i++) tmpbuf[flen - i - 1] = from[i]; @@ -1139,10 +1155,8 @@ static char *wide_to_asc(LPCWSTR wstr) return NULL; } str = OPENSSL_malloc(sz); - if (str == NULL) { - CAPIerr(CAPI_F_WIDE_TO_ASC, ERR_R_MALLOC_FAILURE); + if (str == NULL) return NULL; - } if (!WideCharToMultiByte(CP_ACP, 0, wstr, len_0, str, sz, NULL, NULL)) { OPENSSL_free(str); CAPIerr(CAPI_F_WIDE_TO_ASC, CAPI_R_WIN32_ERROR); @@ -1166,10 +1180,8 @@ static int capi_get_provname(CAPI_CTX *ctx, LPSTR *pname, DWORD *ptype, return 0; } name = OPENSSL_malloc(len); - if (name == NULL) { - CAPIerr(CAPI_F_CAPI_GET_PROVNAME, ERR_R_MALLOC_FAILURE); + if (name == NULL) return 0; - } if (!CryptEnumProviders(idx, NULL, 0, ptype, name, &len)) { err = GetLastError(); OPENSSL_free(name); @@ -1253,10 +1265,8 @@ static int capi_list_containers(CAPI_CTX *ctx, BIO *out) if (buflen == 0) buflen = 1024; cname = OPENSSL_malloc(buflen); - if (cname == NULL) { - CAPIerr(CAPI_F_CAPI_LIST_CONTAINERS, ERR_R_MALLOC_FAILURE); + if (cname == NULL) goto err; - } for (idx = 0;; idx++) { clen = buflen; @@ -1304,10 +1314,8 @@ static CRYPT_KEY_PROV_INFO *capi_get_prov_info(CAPI_CTX *ctx, NULL, &len)) return NULL; pinfo = OPENSSL_malloc(len); - if (pinfo == NULL) { - CAPIerr(CAPI_F_CAPI_GET_PROV_INFO, ERR_R_MALLOC_FAILURE); + if (pinfo == NULL) return NULL; - } if (!CertGetCertificateContextProperty(cert, CERT_KEY_PROV_INFO_PROP_ID, pinfo, &len)) { CAPIerr(CAPI_F_CAPI_GET_PROV_INFO, @@ -1623,10 +1631,8 @@ static CAPI_CTX *capi_ctx_new(void) { CAPI_CTX *ctx = OPENSSL_zalloc(sizeof(*ctx)); - if (ctx == NULL) { - CAPIerr(CAPI_F_CAPI_CTX_NEW, ERR_R_MALLOC_FAILURE); + if (ctx == NULL) return NULL; - } ctx->csptype = PROV_RSA_FULL; ctx->dump_flags = CAPI_DMP_SUMMARY | CAPI_DMP_FNAME; ctx->keytype = AT_KEYEXCHANGE; @@ -1674,10 +1680,8 @@ static int capi_ctx_set_provname(CAPI_CTX *ctx, LPSTR pname, DWORD type, CryptReleaseContext(hprov, 0); } tmpcspname = OPENSSL_strdup(pname); - if (tmpcspname == NULL) { - CAPIerr(CAPI_F_CAPI_CTX_SET_PROVNAME, ERR_R_MALLOC_FAILURE); + if (tmpcspname == NULL) return 0; - } OPENSSL_free(ctx->cspname); ctx->cspname = tmpcspname; ctx->csptype = type; diff --git a/engines/e_dasync.c b/engines/e_dasync.c index 4b7a53661d..b0a08654cc 100644 --- a/engines/e_dasync.c +++ b/engines/e_dasync.c @@ -679,11 +679,8 @@ static int dasync_cipher_init_key_helper(EVP_CIPHER_CTX *ctx, && EVP_CIPHER_impl_ctx_size(cipher) != 0) { pipe_ctx->inner_cipher_data = OPENSSL_zalloc( EVP_CIPHER_impl_ctx_size(cipher)); - if (pipe_ctx->inner_cipher_data == NULL) { - DASYNCerr(DASYNC_F_DASYNC_CIPHER_INIT_KEY_HELPER, - ERR_R_MALLOC_FAILURE); + if (pipe_ctx->inner_cipher_data == NULL) return 0; - } } pipe_ctx->numpipes = 0; diff --git a/engines/e_loader_attic.c b/engines/e_loader_attic.c index 7b9dcd9fee..a7285e871c 100644 --- a/engines/e_loader_attic.c +++ b/engines/e_loader_attic.c @@ -59,7 +59,7 @@ static char *file_get_pass(const UI_METHOD *ui_method, char *pass, char *prompt = NULL; if (ui == NULL) { - ATTICerr(0, ERR_R_MALLOC_FAILURE); + ATTICerr(0, ERR_R_UI_LIB); return NULL; } @@ -68,7 +68,7 @@ static char *file_get_pass(const UI_METHOD *ui_method, char *pass, UI_add_user_data(ui, data); if ((prompt = UI_construct_prompt(ui, desc, info)) == NULL) { - ATTICerr(0, ERR_R_MALLOC_FAILURE); + ATTICerr(0, ERR_R_UI_LIB); pass = NULL; } else if (UI_add_input_string(ui, prompt, UI_INPUT_FLAG_DEFAULT_PWD, pass, 0, maxsize - 1) <= 0) { @@ -190,9 +190,10 @@ static OSSL_STORE_INFO *new_EMBEDDED(const char *new_pem_name, OSSL_STORE_INFO *info = NULL; struct embedded_st *data = NULL; - if ((data = OPENSSL_zalloc(sizeof(*data))) == NULL - || (info = OSSL_STORE_INFO_new(STORE_INFO_EMBEDDED, data)) == NULL) { - ATTICerr(0, ERR_R_MALLOC_FAILURE); + if ((data = OPENSSL_zalloc(sizeof(*data))) == NULL) + return NULL; + if ((info = OSSL_STORE_INFO_new(STORE_INFO_EMBEDDED, data)) == NULL) { + ATTICerr(0, ERR_R_OSSL_STORE_LIB); OPENSSL_free(data); return NULL; } @@ -202,7 +203,6 @@ static OSSL_STORE_INFO *new_EMBEDDED(const char *new_pem_name, new_pem_name == NULL ? NULL : OPENSSL_strdup(new_pem_name); if (new_pem_name != NULL && data->pem_name == NULL) { - ATTICerr(0, ERR_R_MALLOC_FAILURE); store_info_free(info); info = NULL; } @@ -458,7 +458,7 @@ static OSSL_STORE_INFO *try_decode_PKCS8Encrypted(const char *pem_name, *matchcount = 1; if ((mem = BUF_MEM_new()) == NULL) { - ATTICerr(0, ERR_R_MALLOC_FAILURE); + ATTICerr(0, ERR_R_BUF_LIB); goto nop8; } @@ -481,7 +481,7 @@ static OSSL_STORE_INFO *try_decode_PKCS8Encrypted(const char *pem_name, store_info = new_EMBEDDED(PEM_STRING_PKCS8INF, mem); if (store_info == NULL) { - ATTICerr(0, ERR_R_MALLOC_FAILURE); + ATTICerr(0, ERR_R_OSSL_STORE_LIB); goto nop8; } @@ -1022,15 +1022,11 @@ static OSSL_STORE_LOADER_CTX *file_open_ex /* Successfully found a working path */ ctx = OPENSSL_zalloc(sizeof(*ctx)); - if (ctx == NULL) { - ATTICerr(0, ERR_R_MALLOC_FAILURE); + if (ctx == NULL) return NULL; - } ctx->uri = OPENSSL_strdup(uri); - if (ctx->uri == NULL) { - ATTICerr(0, ERR_R_MALLOC_FAILURE); + if (ctx->uri == NULL) goto err; - } if (S_ISDIR(st.st_mode)) { ctx->type = is_dir; @@ -1050,10 +1046,8 @@ static OSSL_STORE_LOADER_CTX *file_open_ex } if (propq != NULL) { ctx->propq = OPENSSL_strdup(propq); - if (ctx->propq == NULL) { - ATTICerr(0, ERR_R_MALLOC_FAILURE); + if (ctx->propq == NULL) goto err; - } } ctx->libctx = libctx; @@ -1079,7 +1073,6 @@ static OSSL_STORE_LOADER_CTX *file_attach if ((ctx = OPENSSL_zalloc(sizeof(*ctx))) == NULL || (propq != NULL && (ctx->propq = OPENSSL_strdup(propq)) == NULL)) { - ATTICerr(0, ERR_R_MALLOC_FAILURE); OSSL_STORE_LOADER_CTX_free(ctx); return NULL; } @@ -1184,10 +1177,8 @@ static OSSL_STORE_INFO *file_load_try_decode(OSSL_STORE_LOADER_CTX *ctx, OPENSSL_zalloc(sizeof(*matching_handlers) * OSSL_NELEM(file_handlers)); - if (matching_handlers == NULL) { - ATTICerr(0, ERR_R_MALLOC_FAILURE); + if (matching_handlers == NULL) goto err; - } *matchcount = 0; for (i = 0; i < OSSL_NELEM(file_handlers); i++) { @@ -1429,10 +1420,8 @@ static int file_name_to_uri(OSSL_STORE_LOADER_CTX *ctx, const char *name, + strlen(name) + 1 /* \0 */; *data = OPENSSL_zalloc(calculated_length); - if (*data == NULL) { - ATTICerr(0, ERR_R_MALLOC_FAILURE); + if (*data == NULL) return 0; - } OPENSSL_strlcat(*data, ctx->uri, calculated_length); OPENSSL_strlcat(*data, pathsep, calculated_length); -- cgit v1.2.3