Thank you for pointing out. That is not what I expect, but very important point for fix.
Sincerely, Yuchi Tian On Mon, Feb 6, 2017 at 4:59 PM, Lars Nordin <lars.nor...@lndata.se> wrote: > On 2017-02-05 07:54, yuchi tian wrote: > > Dear OpenSSL developers, > > We are software engineering researchers at University of Virginia. As part > of a research project, we have built a tool for automatically finding and > fixing error handling bugs and are testing it on > various cryptographic libraries and applications that use them. > > In the most recent version of OpenSSL, we discovered various instances > where there may be memory leak on error path, potential error propagation > or missing check of function call. And we also give a patch for each > potential bug. > > Please let us know how you intend to address these issues. > > 1: > https://github.com/openssl/openssl/blob/master/apps/ts.c > line 891, BIO_new_file(data, "rb") > bug info: memory leak on error path > patch: > > --- a/apps/ts.c > +++ b/apps/ts.c > @@ -878,6 +878,7 @@ static TS_VERIFY_CTX *create_verify_ctx(const char > *data, co > { > TS_VERIFY_CTX *ctx = NULL; > BIO *input = NULL; > + BIO *out = NULL; > TS_REQ *request = NULL; > int ret = 0; > int f = 0; > @@ -888,7 +889,8 @@ static TS_VERIFY_CTX *create_verify_ctx(const char > *data, co > f = TS_VFY_VERSION | TS_VFY_SIGNER; > if (data != NULL) { > f |= TS_VFY_DATA; > - if (TS_VERIFY_CTX_set_data(ctx, BIO_new_file(data, "rb")) == > NULL) > + out = BIO_new_file(data, "rb") > + if (TS_VERIFY_CTX_set_data(ctx, out) == NULL) > goto err; > } else if (digest != NULL) { > long imprint_len; > @@ -931,6 +933,7 @@ static TS_VERIFY_CTX *create_verify_ctx(const char > *data, co > } > BIO_free_all(input); > TS_REQ_free(request); > + BIO_free_all(out) > return ctx; > } > > > > 2: > https://github.com/openssl/openssl/blob/master/crypto/dh/dh_gen.c > line 75,77, ret->p = BN_new() > bug info: memory leak on error path > patch: > @@ -126,5 +126,7 @@ static int dh_builtin_genparams(DH *ret, int > prime_len, int > BN_CTX_end(ctx); > BN_CTX_free(ctx); > } > + if(ret->p!=NULL)BN_free(ret->p); > + if(ret->g!=NULL)BN_free(ret->g); > return ok; > } > > > 3: > https://github.com/openssl/openssl/blob/master/crypto/ec/ec_key.c > line 117, dest->priv_key = BN_new(); > bug info: memory leak on error path > patch: > > @@ -119,9 +119,11 @@ EC_KEY *EC_KEY_copy(EC_KEY *dest, const EC_KEY *src) > return NULL; > } > if (!BN_copy(dest->priv_key, src->priv_key)) > + BN_free(dest->priv_key) > return NULL; > if (src->group->meth->keycopy > && src->group->meth->keycopy(dest, src) == 0) > + BN_free(dest->priv_key) > > The tool need can't just add an extra line for an if-statement without {} > > return NULL; > } > } > @@ -134,6 +136,7 @@ EC_KEY *EC_KEY_copy(EC_KEY *dest, const EC_KEY *src) > dest->flags = src->flags; > if (!CRYPTO_dup_ex_data(CRYPTO_EX_INDEX_EC_KEY, > &dest->ex_data, &src->ex_data)) > + BN_free(dest->priv_key) > > Same comment! > > return NULL; > > if (src->meth != dest->meth) { > @@ -146,6 +149,7 @@ EC_KEY *EC_KEY_copy(EC_KEY *dest, const EC_KEY *src) > } > > if (src->meth->copy != NULL && src->meth->copy(dest, src) == 0) > + BN_free(dest->priv_key) > return NULL; > > Another one > > > return dest; > > > 4:(solved in the recent commit) > https://github.com/openssl/openssl/blob/master/crypto/asn1/a_digest.c > line 33, str = OPENSSL_malloc(i)); > bug info: memory leak on error path > patch: OPENSSL_free(str); > patch location: 41 > > 5: > https://github.com/openssl/openssl/blob/master/crypto/asn1/bio_ndef.c > line 116,185, p = OPENSSL_malloc(derlen); > bug info: memory leak on error path > patch: > > @@ -122,6 +122,7 @@ static int ndef_prefix(BIO *b, unsigned char **pbuf, > int *pl > derlen = ASN1_item_ndef_i2d(ndef_aux->val, &p, ndef_aux->it); > > if (!*ndef_aux->boundary) > + OPENSSL_free(p); > return 0; > > > And again > > *plen = *ndef_aux->boundary - *pbuf; > @@ -191,6 +192,7 @@ static int ndef_suffix(BIO *b, unsigned char **pbuf, > int *pl > derlen = ASN1_item_ndef_i2d(ndef_aux->val, &p, ndef_aux->it); > > if (!*ndef_aux->boundary) > + OPENSSL_free(p); > return 0; > > And again > > *pbuf = *ndef_aux->boundary; > *plen = derlen - (*ndef_aux->boundary - ndef_aux->derbuf); > > 6: > https://github.com/openssl/openssl/blob/master/crypto/bio/bss_bio.c > line 625, b1->buf = OPENSSL_malloc(b1->size); > bug info: memory leak on error path > patch: > > @@ -635,6 +635,7 @@ static int bio_make_pair(BIO *bio1, BIO *bio2) > b2->buf = OPENSSL_malloc(b2->size); > if (b2->buf == NULL) { > BIOerr(BIO_F_BIO_MAKE_PAIR, ERR_R_MALLOC_FAILURE); > + OPENSSL_free(b1->buf); > return 0; > } > b2->len = 0; > > 7: > https://github.com/openssl/openssl/blob/master/crypto/ec/ec_ameth.c > line 244, ep = OPENSSL_malloc(eplen); > bug info: memory leak on error path > patch: > @@ -255,6 +255,7 @@ static int eckey_priv_encode(PKCS8_PRIV_KEY_INFO *p8, > const > > if (!PKCS8_pkey_set0(p8, OBJ_nid2obj(NID_X9_62_id_ecPublicKey), 0, > ptype, pval, ep, eplen)) > + OPENSSL_free(ep); > return 0; > > > Another > > return 1; > > > 8: > https://github.com/openssl/openssl/blob/master/ssl/ssl_ciph.c > line 1833, return 0; > bug info: Error propagation > patch: > @@ -1830,7 +1830,7 @@ int SSL_COMP_add_compression_method(int id, > COMP_METHOD *c > if (id < 193 || id > 255) { > SSLerr(SSL_F_SSL_COMP_ADD_COMPRESSION_METHOD, > SSL_R_COMPRESSION_ID_NOT_WITHIN_PRIVATE_RANGE); > - return 0; > + return 1; > } > > CRYPTO_mem_ctrl(CRYPTO_MEM_CHECK_DISABLE); > > > 9: > https://github.com/openssl/openssl/blob/master/ssl/t1_enc.c > line 370, return (1); > bug info: Error propagation > > line 388, p = OPENSSL_malloc(num) > bug info: memory leak on error path > > patch: > @@ -367,7 +367,7 @@ int tls1_setup_key_block(SSL *s) > int ret = 0; > > if (s->s3->tmp.key_block_length != 0) > - return (1); > + return (0); > > if (!ssl_cipher_get_evp > (s->session, &c, &hash, &mac_type, &mac_secret_size, &comp, > @@ -448,6 +448,7 @@ int tls1_setup_key_block(SSL *s) > > ret = 1; > err: > + OPENSSL_free(p); > return (ret); > } > > > > 10: > https://github.com/openssl/openssl/blob/master/ssl/s3_enc.c > line 301, > bug info: missing check > line 270, > bug info: error propagation > line 295, > bug info: memory leak on error path > patch: > > @@ -268,7 +268,7 @@ int ssl3_setup_key_block(SSL *s) > SSL_COMP *comp; > > if (s->s3->tmp.key_block_length != 0) > - return (1); > + return (0); > > if (!ssl_cipher_get_evp(s->session, &c, &hash, NULL, NULL, &comp, > 0)) { > SSLerr(SSL_F_SSL3_SETUP_KEY_BLOCK, SSL_R_CIPHER_OR_HASH_ > UNAVAILABLE); > @@ -299,6 +299,7 @@ int ssl3_setup_key_block(SSL *s) > s->s3->tmp.key_block = p; > > ret = ssl3_generate_key_block(s, p, num); > + if (ret==0) goto err; > > if (!(s->options & SSL_OP_DONT_INSERT_EMPTY_FRAGMENTS)) { > /* > @@ -317,11 +318,12 @@ int ssl3_setup_key_block(SSL *s) > #endif > } > } > - > + ret = 1 > return ret; > > err: > SSLerr(SSL_F_SSL3_SETUP_KEY_BLOCK, ERR_R_MALLOC_FAILURE); > + OPENSSL_free(p); > return (0); > } > > Others: memory leak on error path > test/bntest.c > test/evp_test.c > > Sincerely, > Baishakhi Ray, Yuchi Tian > > > /Lars > > -- > openssl-dev mailing list > To unsubscribe: https://mta.openssl.org/mailman/listinfo/openssl-dev > >
-- openssl-dev mailing list To unsubscribe: https://mta.openssl.org/mailman/listinfo/openssl-dev