Re: [openssl-dev] Bug reports and patches for OpenSSL

[Lars Nordin]

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