Bug reports and patches for OpenSSL

Previous Topic Next Topic
 
classic Classic list List threaded Threaded
7 messages Options
Reply | Threaded
Open this post in threaded view
|

Bug reports and patches for OpenSSL

yuchi tian
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:
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:
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:
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)
                 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)
         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;
 
     return dest;


4:(solved in the recent commit)
line 33, str = OPENSSL_malloc(i)); 
bug info: memory leak on error path
patch: OPENSSL_free(str);
patch location: 41

5:
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;
 
     *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;
     *pbuf = *ndef_aux->boundary;
     *plen = derlen - (*ndef_aux->boundary - ndef_aux->derbuf);

6:
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:
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;
 
     return 1;


8:
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:
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:
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

--
openssl-dev mailing list
To unsubscribe: https://mta.openssl.org/mailman/listinfo/openssl-dev
Reply | Threaded
Open this post in threaded view
|

Re: Bug reports and patches for OpenSSL

Hanno Böck-4
On Sun, 5 Feb 2017 01:54:06 -0500
yuchi tian <[hidden email]> wrote:

> 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.

I can't answer on how to best report those bugs, but:
That sounds like interesting research.

Will you make the tool and the corresponding scientific publication
public?

--
Hanno Böck
https://hboeck.de/

mail/jabber: [hidden email]
GPG: FE73757FA60E4E21B937579FA5880072BBB51E42
--
openssl-dev mailing list
To unsubscribe: https://mta.openssl.org/mailman/listinfo/openssl-dev
Reply | Threaded
Open this post in threaded view
|

Re: Bug reports and patches for OpenSSL

Matt Caswell-2
In reply to this post by yuchi tian


On 05/02/17 06: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.

Guidance for how to correctly submit patches is given in the
CONTRIBUTING file here:

https://github.com/openssl/openssl/blob/master/CONTRIBUTING

Please could you submit your fixes as a github pull request? One pull
request for all of these issues should be fine.

We will also need a CLA from all authors:
https://www.openssl.org/policies/cla.html

Thanks!

Matt



>
> 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)
>                  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)
>          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;
>  
>      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;
>  
>      *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;
>      *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;
>  
>      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
>
>
--
openssl-dev mailing list
To unsubscribe: https://mta.openssl.org/mailman/listinfo/openssl-dev
Reply | Threaded
Open this post in threaded view
|

Re: Bug reports and patches for OpenSSL

yuchi tian
In reply to this post by Hanno Böck-4
> Will you make the tool and the corresponding scientific publication
> public?

Yes. We are currently in the step of evaluating our tools. We will submit our work and share our tools when the project is done.

Sincerely,
Yuchi Tian 

On Sun, Feb 5, 2017 at 6:16 AM, Hanno Böck <[hidden email]> wrote:
On Sun, 5 Feb 2017 01:54:06 -0500
yuchi tian <[hidden email]> wrote:

> 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.

I can't answer on how to best report those bugs, but:
That sounds like interesting research.

Will you make the tool and the corresponding scientific publication
public?

--
Hanno Böck
https://hboeck.de/

mail/jabber: [hidden email]
GPG: FE73757FA60E4E21B937579FA5880072BBB51E42
--
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
Reply | Threaded
Open this post in threaded view
|

Re: Bug reports and patches for OpenSSL

yuchi tian
In reply to this post by Matt Caswell-2
> Guidance for how to correctly submit patches is given in the
> CONTRIBUTING file here:


> Please could you submit your fixes as a github pull request? One pull
> request for all of these issues should be fine.

Thank you for the information. I will submit the fixes as a github pull request.

Sincerely,
Yuchi Tian


On Sun, Feb 5, 2017 at 8:49 AM, Matt Caswell <[hidden email]> wrote:


On 05/02/17 06: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.

Guidance for how to correctly submit patches is given in the
CONTRIBUTING file here:

https://github.com/openssl/openssl/blob/master/CONTRIBUTING

Please could you submit your fixes as a github pull request? One pull
request for all of these issues should be fine.

We will also need a CLA from all authors:
https://www.openssl.org/policies/cla.html

Thanks!

Matt



>
> 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)
>                  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)
>          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;
>
>      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;
>
>      *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;
>      *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;
>
>      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
>
>
--
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
Reply | Threaded
Open this post in threaded view
|

Re: Bug reports and patches for OpenSSL

Lars Nordin-2
In reply to this post by yuchi tian
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:
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:
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:
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)
line 33, str = OPENSSL_malloc(i)); 
bug info: memory leak on error path
patch: OPENSSL_free(str);
patch location: 41

5:
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:
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:
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:
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:
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:
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
Reply | Threaded
Open this post in threaded view
|

Re: Bug reports and patches for OpenSSL

yuchi tian
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 <[hidden email]> 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:
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:
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:
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)
line 33, str = OPENSSL_malloc(i)); 
bug info: memory leak on error path
patch: OPENSSL_free(str);
patch location: 41

5:
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:
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:
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:
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:
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:
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