<div dir="ltr">Thank you for pointing out. That is not what I expect, but very important point for fix.<div><br></div><div>Sincerely,</div><div>Yuchi Tian</div></div><div class="gmail_extra"><br><div class="gmail_quote">On Mon, Feb 6, 2017 at 4:59 PM, Lars Nordin <span dir="ltr"><<a href="mailto:Lars.Nordin@lndata.se" target="_blank">Lars.Nordin@lndata.se</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<div text="#000000" bgcolor="#FFFFFF"><div><div class="h5">
<div class="m_-3025051973565356947moz-cite-prefix">On 2017-02-05 07:54, yuchi tian wrote:<br>
</div>
<blockquote type="cite">
<div dir="ltr">
<div>Dear OpenSSL developers,</div>
<div><br>
</div>
<div>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</div>
<div>various cryptographic libraries and applications that use
them.</div>
<div><br>
</div>
<div>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.</div>
<div><br>
</div>
<div>Please let us know how you intend to address these issues.</div>
<div><br>
</div>
<div>1:</div>
<div><a href="https://github.com/openssl/openssl/blob/master/apps/ts.c" target="_blank">https://github.com/openssl/<wbr>openssl/blob/master/apps/ts.c</a></div>
<div>line 891, BIO_new_file(data, "rb") </div>
<div>bug info: memory leak on error path</div>
<div>patch:</div>
<div><br>
</div>
<div>--- a/apps/ts.c</div>
<div>+++ b/apps/ts.c</div>
<div>@@ -878,6 +878,7 @@ static TS_VERIFY_CTX
*create_verify_ctx(const char *data, co</div>
<div> {</div>
<div> TS_VERIFY_CTX *ctx = NULL;</div>
<div> BIO *input = NULL;</div>
<div>+ BIO *out = NULL;</div>
<div> TS_REQ *request = NULL;</div>
<div> int ret = 0;</div>
<div> int f = 0;</div>
<div>@@ -888,7 +889,8 @@ static TS_VERIFY_CTX
*create_verify_ctx(const char *data, co</div>
<div> f = TS_VFY_VERSION | TS_VFY_SIGNER;</div>
<div> if (data != NULL) {</div>
<div> f |= TS_VFY_DATA;</div>
<div>- if (TS_VERIFY_CTX_set_data(ctx,
BIO_new_file(data, "rb")) == NULL)</div>
<div>+ out = BIO_new_file(data, "rb")</div>
<div>+ if (TS_VERIFY_CTX_set_data(ctx, out) == NULL)</div>
<div> goto err;</div>
<div> } else if (digest != NULL) {</div>
<div> long imprint_len;</div>
<div>@@ -931,6 +933,7 @@ static TS_VERIFY_CTX
*create_verify_ctx(const char *data, co</div>
<div> }</div>
<div> BIO_free_all(input);</div>
<div> TS_REQ_free(request);</div>
<div>+ BIO_free_all(out)</div>
<div> return ctx;</div>
<div> }</div>
<div><br>
</div>
<div><br>
</div>
<div><br>
</div>
<div>2:</div>
<div><a href="https://github.com/openssl/openssl/blob/master/crypto/dh/dh_gen.c" target="_blank">https://github.com/openssl/<wbr>openssl/blob/master/crypto/dh/<wbr>dh_gen.c</a></div>
<div>line 75,77, ret->p = BN_new() </div>
<div>bug info: memory leak on error path</div>
<div>patch:</div>
<div>@@ -126,5 +126,7 @@ static int dh_builtin_genparams(DH
*ret, int prime_len, int </div>
<div> BN_CTX_end(ctx);</div>
<div> BN_CTX_free(ctx);</div>
<div> }</div>
<div>+ if(ret->p!=NULL)BN_free(ret-><wbr>p);</div>
<div>+ if(ret->g!=NULL)BN_free(ret-><wbr>g);</div>
<div> return ok;</div>
<div> }</div>
<div><br>
</div>
<div><br>
</div>
<div>3:</div>
<div><a href="https://github.com/openssl/openssl/blob/master/crypto/ec/ec_key.c" target="_blank">https://github.com/openssl/<wbr>openssl/blob/master/crypto/ec/<wbr>ec_key.c</a></div>
<div>line 117, dest->priv_key = BN_new(); </div>
<div>bug info: memory leak on error path</div>
<div>patch:</div>
<div><br>
</div>
<div>@@ -119,9 +119,11 @@ EC_KEY *EC_KEY_copy(EC_KEY *dest,
const EC_KEY *src)</div>
<div> return NULL;</div>
<div> }</div>
<div> if (!BN_copy(dest->priv_key,
src->priv_key))</div>
<div>+ BN_free(dest->priv_key)</div>
<div> return NULL;</div>
<div> if (src->group->meth->keycopy</div>
<div> &&
src->group->meth->keycopy(<wbr>dest, src) == 0)</div>
<div>+ BN_free(dest->priv_key)</div>
</div>
</blockquote></div></div>
The tool need can't just add an extra line for an if-statement
without {}<span class=""><br>
<br>
<blockquote type="cite">
<div dir="ltr">
<div> return NULL;</div>
<div> }</div>
<div> }</div>
<div>@@ -134,6 +136,7 @@ EC_KEY *EC_KEY_copy(EC_KEY *dest, const
EC_KEY *src)</div>
<div> dest->flags = src->flags;</div>
<div> if (!CRYPTO_dup_ex_data(CRYPTO_<wbr>EX_INDEX_EC_KEY,</div>
<div> &dest->ex_data,
&src->ex_data))</div>
<div>+ BN_free(dest->priv_key)</div>
</div>
</blockquote></span>
Same comment!<span class=""><br>
<blockquote type="cite">
<div dir="ltr">
<div> return NULL;</div>
<div> </div>
<div> if (src->meth != dest->meth) {</div>
<div>@@ -146,6 +149,7 @@ EC_KEY *EC_KEY_copy(EC_KEY *dest, const
EC_KEY *src)</div>
<div> }</div>
<div> </div>
<div> if (src->meth->copy != NULL &&
src->meth->copy(dest, src) == 0)</div>
<div>+ BN_free(dest->priv_key)</div>
<div> return NULL;</div>
</div>
</blockquote></span>
Another one<span class=""><br>
<blockquote type="cite">
<div dir="ltr">
<div> </div>
<div> return dest;</div>
<div><br>
</div>
<div><br>
</div>
<div>4:(solved in the recent commit)</div>
<div><a href="https://github.com/openssl/openssl/blob/master/crypto/asn1/a_digest.c" target="_blank">https://github.com/openssl/<wbr>openssl/blob/master/crypto/<wbr>asn1/a_digest.c</a></div>
<div>line 33, str = OPENSSL_malloc(i)); </div>
<div>bug info: memory leak on error path</div>
<div>patch: OPENSSL_free(str);</div>
<div>patch location: 41</div>
<div><br>
</div>
<div>5:</div>
<div><a href="https://github.com/openssl/openssl/blob/master/crypto/asn1/bio_ndef.c" target="_blank">https://github.com/openssl/<wbr>openssl/blob/master/crypto/<wbr>asn1/bio_ndef.c</a></div>
<div>line 116,185, p = OPENSSL_malloc(derlen);</div>
<div>bug info: memory leak on error path</div>
<div>patch:</div>
<div><br>
</div>
<div>@@ -122,6 +122,7 @@ static int ndef_prefix(BIO *b, unsigned
char **pbuf, int *pl</div>
<div> derlen = ASN1_item_ndef_i2d(ndef_aux-><wbr>val, &p,
ndef_aux->it);</div>
<div> </div>
<div> if (!*ndef_aux->boundary)</div>
<div>+ OPENSSL_free(p);</div>
<div> return 0;</div>
<div> </div>
</div>
</blockquote></span>
And again<span class=""><br>
<br>
<blockquote type="cite">
<div dir="ltr">
<div> *plen = *ndef_aux->boundary - *pbuf;</div>
<div>@@ -191,6 +192,7 @@ static int ndef_suffix(BIO *b, unsigned
char **pbuf, int *pl</div>
<div> derlen = ASN1_item_ndef_i2d(ndef_aux-><wbr>val, &p,
ndef_aux->it);</div>
<div> </div>
<div> if (!*ndef_aux->boundary)</div>
<div>+ OPENSSL_free(p);</div>
<div> return 0;</div>
</div>
</blockquote></span>
And again<span class=""><br>
<blockquote type="cite">
<div dir="ltr">
<div> *pbuf = *ndef_aux->boundary;</div>
<div> *plen = derlen - (*ndef_aux->boundary -
ndef_aux->derbuf);</div>
<div><br>
</div>
<div>6:</div>
<div><a href="https://github.com/openssl/openssl/blob/master/crypto/bio/bss_bio.c" target="_blank">https://github.com/openssl/<wbr>openssl/blob/master/crypto/<wbr>bio/bss_bio.c</a></div>
<div>line 625, b1->buf = OPENSSL_malloc(b1->size);</div>
<div>bug info: memory leak on error path</div>
<div>patch:</div>
<div><br>
</div>
<div>@@ -635,6 +635,7 @@ static int bio_make_pair(BIO *bio1, BIO
*bio2)</div>
<div> b2->buf = OPENSSL_malloc(b2->size);</div>
<div> if (b2->buf == NULL) {</div>
<div> BIOerr(BIO_F_BIO_MAKE_PAIR,
ERR_R_MALLOC_FAILURE);</div>
<div>+ OPENSSL_free(b1->buf);</div>
<div> return 0;</div>
<div> }</div>
<div> b2->len = 0;</div>
<div><br>
</div>
<div>7:</div>
<div><a href="https://github.com/openssl/openssl/blob/master/crypto/ec/ec_ameth.c" target="_blank">https://github.com/openssl/<wbr>openssl/blob/master/crypto/ec/<wbr>ec_ameth.c</a></div>
<div>line 244, ep = OPENSSL_malloc(eplen); </div>
<div>bug info: memory leak on error path</div>
<div>patch:</div>
<div>@@ -255,6 +255,7 @@ static int
eckey_priv_encode(PKCS8_PRIV_<wbr>KEY_INFO *p8, const </div>
<div> </div>
<div> if (!PKCS8_pkey_set0(p8,
OBJ_nid2obj(NID_X9_62_id_<wbr>ecPublicKey), 0,</div>
<div> ptype, pval, ep, eplen))</div>
<div>+ OPENSSL_free(ep);</div>
<div> return 0;</div>
<div> </div>
</div>
</blockquote></span>
Another<div><div class="h5"><br>
<blockquote type="cite">
<div dir="ltr">
<div> return 1;</div>
<div><br>
</div>
<div><br>
</div>
<div>8:</div>
<div><a href="https://github.com/openssl/openssl/blob/master/ssl/ssl_ciph.c" target="_blank">https://github.com/openssl/<wbr>openssl/blob/master/ssl/ssl_<wbr>ciph.c</a></div>
<div>line 1833, return 0;</div>
<div>bug info: Error propagation</div>
<div>patch:</div>
<div>@@ -1830,7 +1830,7 @@ int
SSL_COMP_add_compression_<wbr>method(int id, COMP_METHOD *c</div>
<div> if (id < 193 || id > 255) {</div>
<div> SSLerr(SSL_F_SSL_COMP_ADD_<wbr>COMPRESSION_METHOD,</div>
<div>
SSL_R_COMPRESSION_ID_NOT_<wbr>WITHIN_PRIVATE_RANGE);</div>
<div>- return 0;</div>
<div>+ return 1;</div>
<div> }</div>
<div> </div>
<div> CRYPTO_mem_ctrl(CRYPTO_MEM_<wbr>CHECK_DISABLE);</div>
<div><br>
</div>
<div><br>
</div>
<div>9:</div>
<div><a href="https://github.com/openssl/openssl/blob/master/ssl/t1_enc.c" target="_blank">https://github.com/openssl/<wbr>openssl/blob/master/ssl/t1_<wbr>enc.c</a></div>
<div>line 370, return (1);</div>
<div>bug info: Error propagation</div>
<div><br>
</div>
<div>line 388, p = OPENSSL_malloc(num)</div>
<div>bug info: memory leak on error path</div>
<div><br>
</div>
<div>patch:</div>
<div>@@ -367,7 +367,7 @@ int tls1_setup_key_block(SSL *s)</div>
<div> int ret = 0;</div>
<div> </div>
<div> if (s->s3->tmp.key_block_length != 0)</div>
<div>- return (1);</div>
<div>+ return (0);</div>
<div> </div>
<div> if (!ssl_cipher_get_evp</div>
<div> (s->session, &c, &hash, &mac_type,
&mac_secret_size, &comp,</div>
<div>@@ -448,6 +448,7 @@ int tls1_setup_key_block(SSL *s)</div>
<div> </div>
<div> ret = 1;</div>
<div> err:</div>
<div>+ OPENSSL_free(p);</div>
<div> return (ret);</div>
<div> }</div>
<div><br>
</div>
<div><br>
</div>
<div><br>
</div>
<div>10:</div>
<div><a href="https://github.com/openssl/openssl/blob/master/ssl/s3_enc.c" target="_blank">https://github.com/openssl/<wbr>openssl/blob/master/ssl/s3_<wbr>enc.c</a></div>
<div>line 301, </div>
<div>bug info: missing check</div>
<div>line 270,</div>
<div>bug info: error propagation</div>
<div>line 295, </div>
<div>bug info: memory leak on error path</div>
<div>patch:</div>
<div><br>
</div>
<div>@@ -268,7 +268,7 @@ int ssl3_setup_key_block(SSL *s)</div>
<div> SSL_COMP *comp;</div>
<div> </div>
<div> if (s->s3->tmp.key_block_length != 0)</div>
<div>- return (1);</div>
<div>+ return (0);</div>
<div> </div>
<div> if (!ssl_cipher_get_evp(s-><wbr>session, &c,
&hash, NULL, NULL, &comp, 0)) {</div>
<div> SSLerr(SSL_F_SSL3_SETUP_KEY_<wbr>BLOCK,
SSL_R_CIPHER_OR_HASH_<wbr>UNAVAILABLE);</div>
<div>@@ -299,6 +299,7 @@ int ssl3_setup_key_block(SSL *s)</div>
<div> s->s3->tmp.key_block = p;</div>
<div> </div>
<div> ret = ssl3_generate_key_block(s, p, num);</div>
<div>+ if (ret==0) goto err;</div>
<div> </div>
<div> if (!(s->options &
SSL_OP_DONT_INSERT_EMPTY_<wbr>FRAGMENTS)) {</div>
<div> /*</div>
<div>@@ -317,11 +318,12 @@ int ssl3_setup_key_block(SSL *s)</div>
<div> #endif</div>
<div> }</div>
<div> }</div>
<div>-</div>
<div>+ ret = 1</div>
<div> return ret;</div>
<div> </div>
<div> err:</div>
<div> SSLerr(SSL_F_SSL3_SETUP_KEY_<wbr>BLOCK,
ERR_R_MALLOC_FAILURE);</div>
<div>+ OPENSSL_free(p);</div>
<div> return (0);</div>
<div> }</div>
<div><br>
</div>
<div>Others: memory leak on error path</div>
<div>test/bntest.c</div>
<div>test/evp_test.c</div>
<div><br>
</div>
<div>Sincerely,</div>
<div>Baishakhi Ray, Yuchi Tian</div>
</div>
<br>
<fieldset class="m_-3025051973565356947mimeAttachmentHeader"></fieldset>
<br>
</blockquote>
</div></div><span class="HOEnZb"><font color="#888888"><p>/Lars<br>
</p>
</font></span></div>
<br>--<br>
openssl-dev mailing list<br>
To unsubscribe: <a href="https://mta.openssl.org/mailman/listinfo/openssl-dev" rel="noreferrer" target="_blank">https://mta.openssl.org/<wbr>mailman/listinfo/openssl-dev</a><br>
<br></blockquote></div><br></div>