From 55500ea7c46c27a150a46832e1260891aaad8e52 Mon Sep 17 00:00:00 2001 From: Alessandro Ghedini Date: Thu, 27 Aug 2015 23:07:07 -0400 Subject: [PATCH] GH354: Memory leak fixes MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Fix more potential leaks in X509_verify_cert() Fix memory leak in ClientHello test Fix memory leak in gost2814789 test Fix potential memory leak in PKCS7_verify() Fix potential memory leaks in X509_add1_reject_object() Refactor to use "goto err" in cleanup. Signed-off-by: Rich Salz Reviewed-by: Emilia Käsper --- crypto/asn1/x_x509a.c | 7 +++++-- crypto/pkcs7/pk7_smime.c | 26 ++++++-------------------- crypto/x509/x509_vfy.c | 4 ++-- test/clienthellotest.c | 1 + test/gost2814789test.c | 3 +++ 5 files changed, 17 insertions(+), 24 deletions(-) diff --git a/crypto/asn1/x_x509a.c b/crypto/asn1/x_x509a.c index d81ccfb62f..e299b1fd50 100644 --- a/crypto/asn1/x_x509a.c +++ b/crypto/asn1/x_x509a.c @@ -172,11 +172,14 @@ int X509_add1_reject_object(X509 *x, ASN1_OBJECT *obj) if ((objtmp = OBJ_dup(obj)) == NULL) return 0; if ((aux = aux_get(x)) == NULL) - return 0; + goto err; if (aux->reject == NULL && (aux->reject = sk_ASN1_OBJECT_new_null()) == NULL) - return 0; + goto err; return sk_ASN1_OBJECT_push(aux->reject, objtmp); + err: + ASN1_OBJECT_free(objtmp); + return 0; } void X509_trust_clear(X509 *x) diff --git a/crypto/pkcs7/pk7_smime.c b/crypto/pkcs7/pk7_smime.c index e52e74679a..91557af5c8 100644 --- a/crypto/pkcs7/pk7_smime.c +++ b/crypto/pkcs7/pk7_smime.c @@ -255,8 +255,8 @@ int PKCS7_verify(PKCS7 *p7, STACK_OF(X509) *certs, X509_STORE *store, X509_STORE_CTX cert_ctx; char buf[4096]; int i, j = 0, k, ret = 0; - BIO *p7bio; - BIO *tmpin, *tmpout; + BIO *p7bio = NULL; + BIO *tmpin = NULL, *tmpout = NULL; if (!p7) { PKCS7err(PKCS7_F_PKCS7_VERIFY, PKCS7_R_INVALID_NULL_POINTER); @@ -274,18 +274,11 @@ int PKCS7_verify(PKCS7 *p7, STACK_OF(X509) *certs, X509_STORE *store, return 0; } - /* - * Very old Netscape illegally included empty content with - * a detached signature. To not support that, enable the - * following flag. - */ -#ifdef OPENSSL_DONT_SUPPORT_OLD_NETSCAPE /* Check for data and content: two sets of data */ if (!PKCS7_get_detached(p7) && indata) { PKCS7err(PKCS7_F_PKCS7_VERIFY, PKCS7_R_CONTENT_AND_DATA_PRESENT); return 0; } -#endif sinfos = PKCS7_get_signer_info(p7); @@ -295,7 +288,6 @@ int PKCS7_verify(PKCS7 *p7, STACK_OF(X509) *certs, X509_STORE *store, } signers = PKCS7_get0_signers(p7, certs, flags); - if (!signers) return 0; @@ -308,14 +300,12 @@ int PKCS7_verify(PKCS7 *p7, STACK_OF(X509) *certs, X509_STORE *store, if (!X509_STORE_CTX_init(&cert_ctx, store, signer, p7->d.sign->cert)) { PKCS7err(PKCS7_F_PKCS7_VERIFY, ERR_R_X509_LIB); - sk_X509_free(signers); - return 0; + goto err; } X509_STORE_CTX_set_default(&cert_ctx, "smime_sign"); } else if (!X509_STORE_CTX_init(&cert_ctx, store, signer, NULL)) { PKCS7err(PKCS7_F_PKCS7_VERIFY, ERR_R_X509_LIB); - sk_X509_free(signers); - return 0; + goto err; } if (!(flags & PKCS7_NOCRL)) X509_STORE_CTX_set0_crls(&cert_ctx, p7->d.sign->crl); @@ -328,8 +318,7 @@ int PKCS7_verify(PKCS7 *p7, STACK_OF(X509) *certs, X509_STORE *store, PKCS7_R_CERTIFICATE_VERIFY_ERROR); ERR_add_error_data(2, "Verify error:", X509_verify_cert_error_string(j)); - sk_X509_free(signers); - return 0; + goto err; } /* Check for revocation status here */ } @@ -348,7 +337,7 @@ int PKCS7_verify(PKCS7 *p7, STACK_OF(X509) *certs, X509_STORE *store, tmpin = BIO_new_mem_buf(ptr, len); if (tmpin == NULL) { PKCS7err(PKCS7_F_PKCS7_VERIFY, ERR_R_MALLOC_FAILURE); - return 0; + goto err; } } else tmpin = indata; @@ -398,15 +387,12 @@ int PKCS7_verify(PKCS7 *p7, STACK_OF(X509) *certs, X509_STORE *store, ret = 1; err: - if (tmpin == indata) { if (indata) BIO_pop(p7bio); } BIO_free_all(p7bio); - sk_X509_free(signers); - return ret; } diff --git a/crypto/x509/x509_vfy.c b/crypto/x509/x509_vfy.c index 6b1f7febff..a3077b5cbc 100644 --- a/crypto/x509/x509_vfy.c +++ b/crypto/x509/x509_vfy.c @@ -243,7 +243,7 @@ int X509_verify_cert(X509_STORE_CTX *ctx) if (ctx->param->flags & X509_V_FLAG_TRUSTED_FIRST) { ok = ctx->get_issuer(&xtmp, ctx, x); if (ok < 0) - return ok; + goto end; /* * If successful for now free up cert so it will be picked up * again later. @@ -341,7 +341,7 @@ int X509_verify_cert(X509_STORE_CTX *ctx) ok = ctx->get_issuer(&xtmp, ctx, x); if (ok < 0) - return ok; + goto end; if (ok == 0) break; x = xtmp; diff --git a/test/clienthellotest.c b/test/clienthellotest.c index acc56f84c6..318d6e84f8 100644 --- a/test/clienthellotest.c +++ b/test/clienthellotest.c @@ -213,6 +213,7 @@ int main(int argc, char *argv[]) EVP_cleanup(); CRYPTO_cleanup_all_ex_data(); CRYPTO_mem_leaks(err); + BIO_free(err); return testresult?0:1; } diff --git a/test/gost2814789test.c b/test/gost2814789test.c index b2cd41fadc..953e1e1540 100644 --- a/test/gost2814789test.c +++ b/test/gost2814789test.c @@ -1411,6 +1411,7 @@ int main(int argc, char *argv[]) } siglen = 4; OPENSSL_assert(EVP_DigestSignFinal(&mctx, bTest, &siglen)); + EVP_PKEY_free(mac_key); EVP_MD_CTX_cleanup(&mctx); enlu = (int)tcs[t].ullLen; enlf = 0; @@ -1434,6 +1435,8 @@ int main(int argc, char *argv[]) printf(" passed\n"); fflush(NULL); + NCONF_free(pConfig); + return EXIT_SUCCESS; } #endif