From 2ef99c4b81bfac90f25c379c0a03ddb6bb7557a7 Mon Sep 17 00:00:00 2001 From: thb-sb <108470890+thb-sb@users.noreply.github.com> Date: Thu, 7 Sep 2023 11:20:28 +0200 Subject: [PATCH] Fix various memory leaks. (#245) This commit fixes various memory leaks that have been found using ASan. --- oqsprov/oqs_decode_der2key.c | 1 + oqsprov/oqsprov_keys.c | 21 +++++++++++++++------ test/oqs_test_endecode.c | 35 ++++++++++++++++++++--------------- test/oqs_test_kems.c | 11 ++++++++--- test/oqs_test_signatures.c | 4 ++-- 5 files changed, 46 insertions(+), 26 deletions(-) diff --git a/oqsprov/oqs_decode_der2key.c b/oqsprov/oqs_decode_der2key.c index 35c79a5d..da4d666b 100644 --- a/oqsprov/oqs_decode_der2key.c +++ b/oqsprov/oqs_decode_der2key.c @@ -197,6 +197,7 @@ OQSX_KEY *oqsx_d2i_PUBKEY(OQSX_KEY **a, const unsigned char **pp, long length) xpk = oqsx_d2i_X509_PUBKEY_INTERNAL(pp, length, NULL); key = oqsx_key_from_x509pubkey(xpk, NULL, NULL); + X509_PUBKEY_free(xpk); if (key == NULL) return NULL; diff --git a/oqsprov/oqsprov_keys.c b/oqsprov/oqsprov_keys.c index 9eb15552..d240c300 100644 --- a/oqsprov/oqsprov_keys.c +++ b/oqsprov/oqsprov_keys.c @@ -580,16 +580,21 @@ static int oqsx_hybsig_init(int bit_security, OQSX_EVP_CTX *evp_ctx, if (idx < 3) { // EC ret = EVP_PKEY_paramgen_init(evp_ctx->ctx); - ON_ERR_GOTO(ret <= 0, err); + ON_ERR_GOTO(ret <= 0, free_evp_ctx); ret = EVP_PKEY_CTX_set_ec_paramgen_curve_nid(evp_ctx->ctx, evp_ctx->evp_info->nid); - ON_ERR_GOTO(ret <= 0, err); + ON_ERR_GOTO(ret <= 0, free_evp_ctx); ret = EVP_PKEY_paramgen(evp_ctx->ctx, &evp_ctx->keyParam); - ON_ERR_GOTO(ret <= 0 || !evp_ctx->keyParam, err); + ON_ERR_GOTO(ret <= 0 || !evp_ctx->keyParam, free_evp_ctx); } // RSA bit length set only during keygen + goto err; + +free_evp_ctx: + EVP_PKEY_CTX_free(evp_ctx->ctx); + evp_ctx->ctx = NULL; err: return ret; @@ -867,12 +872,14 @@ void oqsx_key_free(OQSX_KEY *key) else if (key->keytype == KEY_TYPE_ECP_HYB_KEM || key->keytype == KEY_TYPE_ECX_HYB_KEM) { OQS_KEM_free(key->oqsx_provider_ctx.oqsx_qs_ctx.kem); + } else + OQS_SIG_free(key->oqsx_provider_ctx.oqsx_qs_ctx.sig); + EVP_PKEY_free(key->classical_pkey); + if (key->oqsx_provider_ctx.oqsx_evp_ctx) { EVP_PKEY_CTX_free(key->oqsx_provider_ctx.oqsx_evp_ctx->ctx); EVP_PKEY_free(key->oqsx_provider_ctx.oqsx_evp_ctx->keyParam); OPENSSL_free(key->oqsx_provider_ctx.oqsx_evp_ctx); - } else - OQS_SIG_free(key->oqsx_provider_ctx.oqsx_qs_ctx.sig); - OPENSSL_free(key->classical_pkey); + } #ifdef OQS_PROVIDER_NOATOMIC CRYPTO_THREAD_lock_free(key->lock); #endif @@ -1036,6 +1043,7 @@ static EVP_PKEY *oqsx_key_gen_evp_key(OQSX_EVP_CTX *ctx, unsigned char *pubkey, EVP_PKEY *ck2 = d2i_PrivateKey(ctx->evp_info->keytype, NULL, &privkey_enc2, privkeylen); ON_ERR_SET_GOTO(!ck2, ret, -14, errhyb); + EVP_PKEY_free(ck2); } ENCODE_UINT32(pubkey, pubkeylen); ENCODE_UINT32(privkey, privkeylen); @@ -1087,6 +1095,7 @@ int oqsx_key_gen(OQSX_KEY *key) ret = oqsx_key_gen_oqs(key, 0); } else { EVP_PKEY_free(pkey); + pkey = NULL; ret = oqsx_key_gen_oqs(key, 1); } } else if (key->keytype == KEY_TYPE_SIG) { diff --git a/test/oqs_test_endecode.c b/test/oqs_test_endecode.c index 55fa0f84..220f7d0b 100644 --- a/test/oqs_test_endecode.c +++ b/test/oqs_test_endecode.c @@ -78,8 +78,7 @@ static EVP_PKEY *oqstest_make_key(const char *type, EVP_PKEY *template, static int encode_EVP_PKEY_prov(const EVP_PKEY *pkey, const char *format, const char *structure, const char *pass, - const int selection, void **encoded, - long *encoded_len) + const int selection, BUF_MEM **encoded) { OSSL_ENCODER_CTX *ectx; BIO *mem_ser = NULL; @@ -110,8 +109,9 @@ static int encode_EVP_PKEY_prov(const EVP_PKEY *pkey, const char *format, goto end; /* pkey was successfully encoded into the bio */ - *encoded = mem_buf->data; - *encoded_len = mem_buf->length; + *encoded = BUF_MEM_new(); + (*encoded)->data = mem_buf->data; + (*encoded)->length = mem_buf->length; /* Detach the encoded output */ mem_buf->data = NULL; @@ -159,9 +159,9 @@ static int decode_EVP_PKEY_prov(const char *input_type, const char *structure, pkey = NULL; end: - EVP_PKEY_free(pkey); BIO_free(encoded_bio); OSSL_DECODER_CTX_free(dctx); + EVP_PKEY_free(pkey); return ok; } @@ -169,40 +169,45 @@ static int test_oqs_encdec(const char *sigalg_name) { EVP_PKEY *pkey = NULL; EVP_PKEY *decoded_pkey = NULL; - void *encoded = NULL; - long encoded_len = 0; + BUF_MEM *encoded = NULL; size_t i; int ok = 0; for (i = 0; i < nelem(test_params_list); i++) { - pkey = oqstest_make_key(sigalg_name, NULL, NULL); if (pkey == NULL) goto end; - if (!encode_EVP_PKEY_prov( - pkey, test_params_list[i].format, test_params_list[i].structure, - test_params_list[i].pass, test_params_list[i].selection, - &encoded, &encoded_len)) { + if (!encode_EVP_PKEY_prov(pkey, test_params_list[i].format, + test_params_list[i].structure, + test_params_list[i].pass, + test_params_list[i].selection, &encoded)) { printf("Failed encoding %s", sigalg_name); goto end; } if (!decode_EVP_PKEY_prov( test_params_list[i].format, test_params_list[i].structure, test_params_list[i].pass, test_params_list[i].keytype, - test_params_list[i].selection, &decoded_pkey, encoded, - encoded_len)) { + test_params_list[i].selection, &decoded_pkey, encoded->data, + encoded->length)) { printf("Failed decoding %s", sigalg_name); goto end; } if (EVP_PKEY_eq(pkey, decoded_pkey) != 1) goto end; + EVP_PKEY_free(pkey); + pkey = NULL; + EVP_PKEY_free(decoded_pkey); + decoded_pkey = NULL; + BUF_MEM_free(encoded); + encoded = NULL; } ok = 1; end: EVP_PKEY_free(pkey); EVP_PKEY_free(decoded_pkey); + BUF_MEM_free(encoded); return ok; } @@ -252,11 +257,11 @@ int main(int argc, char *argv[]) errcnt++; } - OSSL_LIB_CTX_free(libctx); OSSL_PROVIDER_unload(dfltprov); OSSL_PROVIDER_unload(keyprov); if (OPENSSL_VERSION_PREREQ(3, 1)) OSSL_PROVIDER_unload(oqsprov); // avoid crash in 3.0.x + OSSL_LIB_CTX_free(libctx); OSSL_LIB_CTX_free(keyctx); TEST_ASSERT(errcnt == 0) diff --git a/test/oqs_test_kems.c b/test/oqs_test_kems.c index 28179e8c..4b734aa9 100644 --- a/test/oqs_test_kems.c +++ b/test/oqs_test_kems.c @@ -15,7 +15,9 @@ static int test_oqs_kems(const char *kemalg_name) EVP_MD_CTX *mdctx = NULL; EVP_PKEY_CTX *ctx = NULL; EVP_PKEY *key = NULL; - unsigned char *out, *secenc, *secdec; + unsigned char *out = NULL; + unsigned char *secenc = NULL; + unsigned char *secdec = NULL; size_t outlen, seclen; int testresult = 1; @@ -34,7 +36,7 @@ static int test_oqs_kems(const char *kemalg_name) if (!testresult) goto err; - OPENSSL_free(ctx); + EVP_PKEY_CTX_free(ctx); ctx = NULL; testresult @@ -64,7 +66,10 @@ static int test_oqs_kems(const char *kemalg_name) err: EVP_PKEY_free(key); - OPENSSL_free(ctx); + EVP_PKEY_CTX_free(ctx); + OPENSSL_free(out); + OPENSSL_free(secenc); + OPENSSL_free(secdec); return testresult; } diff --git a/test/oqs_test_signatures.c b/test/oqs_test_signatures.c index 5aaf4dba..b1137839 100644 --- a/test/oqs_test_signatures.c +++ b/test/oqs_test_signatures.c @@ -57,7 +57,7 @@ static int test_oqs_signatures(const char *sigalg_name) EVP_MD_CTX_free(mdctx); EVP_PKEY_free(key); - OPENSSL_free(ctx); + EVP_PKEY_CTX_free(ctx); OPENSSL_free(sig); mdctx = NULL; key = NULL; @@ -84,7 +84,7 @@ static int test_oqs_signatures(const char *sigalg_name) EVP_MD_CTX_free(mdctx); EVP_PKEY_free(key); - OPENSSL_free(ctx); + EVP_PKEY_CTX_free(ctx); OPENSSL_free(sig); return testresult; }