[openssl.org #4386] [PATCH] Add sanity checks for BN_new() in OpenSSL-1.0.2g

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

[openssl.org #4386] [PATCH] Add sanity checks for BN_new() in OpenSSL-1.0.2g

Rich Salz via RT
Hello All,

In reviewing code in directory 'engines/ccgost', file 'gost2001.c',
there are two calls to BN_new() which are not checked for a return
value of NULL, indicating failure.

The patch file below should address/correct this issue:

--- gost2001.c.orig     2016-03-06 11:32:49.676178425 -0800
+++ gost2001.c  2016-03-06 11:38:04.604204158 -0800
@@ -434,6 +434,10 @@
 int gost2001_keygen(EC_KEY *ec)
 {
     BIGNUM *order = BN_new(), *d = BN_new();
+    if (!order || !d) {
+       GOSTerr(GOST_F_GOST2001_KEYGEN, ERR_R_MALLOC_FAILURE);
+       return 0;
+    }
     const EC_GROUP *group = EC_KEY_get0_group(ec);

     if(!group || !EC_GROUP_get_order(group, order, NULL)) {

--
Ticket here: http://rt.openssl.org/Ticket/Display.html?id=4386
Please log in as guest with password guest if prompted


--
openssl-dev mailing list
To unsubscribe: https://mta.openssl.org/mailman/listinfo/openssl-dev

gost2001.c.patch (588 bytes) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: [openssl.org #4386] [PATCH] Add sanity checks for BN_new() in OpenSSL-1.0.2g

Dr Paul Dale

If one of the allocation calls succeeds and the other fails, the patched code will leak memory.

It needs something along the lines of:

 

if (order != NULL) BN_clear_free(order);

if (d != NULL) BN_clear_free(d);

 

in the failure case code.

 

 

Pauli

 

--

Oracle

Dr Paul Dale | Cryptographer | Network Security & Encryption

Phone +61 7 3031 7217

Oracle Australia

 

On Mon, 7 Mar 2016 05:55:23 PM Bill Parker via RT wrote:

> Hello All,

>

> In reviewing code in directory 'engines/ccgost', file 'gost2001.c',

> there are two calls to BN_new() which are not checked for a return

> value of NULL, indicating failure.

>

> The patch file below should address/correct this issue:

>

> --- gost2001.c.orig 2016-03-06 11:32:49.676178425 -0800

> +++ gost2001.c 2016-03-06 11:38:04.604204158 -0800

> @@ -434,6 +434,10 @@

> int gost2001_keygen(EC_KEY *ec)

> {

> BIGNUM *order = BN_new(), *d = BN_new();

> + if (!order || !d) {

> + GOSTerr(GOST_F_GOST2001_KEYGEN, ERR_R_MALLOC_FAILURE);

> + return 0;

> + }

> const EC_GROUP *group = EC_KEY_get0_group(ec);

>

> if(!group || !EC_GROUP_get_order(group, order, NULL)) {

>

>

 


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

Re: [openssl.org #4386] [PATCH] Add sanity checks for BN_new() in OpenSSL-1.0.2g

Rich Salz via RT
If one of the allocation calls succeeds and the other fails, the patched code will leak memory.
It needs something along the lines of:

if (order != NULL) BN_clear_free(order);
if (d != NULL) BN_clear_free(d);

in the failure case code.


Pauli

--
Oracle
Dr Paul Dale | Cryptographer | Network Security & Encryption
Phone +61 7 3031 7217
Oracle Australia

On Mon, 7 Mar 2016 05:55:23 PM Bill Parker via RT wrote:

> Hello All,
>
> In reviewing code in directory 'engines/ccgost', file 'gost2001.c',
> there are two calls to BN_new() which are not checked for a return
> value of NULL, indicating failure.
>
> The patch file below should address/correct this issue:
>
> --- gost2001.c.orig     2016-03-06 11:32:49.676178425 -0800
> +++ gost2001.c  2016-03-06 11:38:04.604204158 -0800
> @@ -434,6 +434,10 @@
>  int gost2001_keygen(EC_KEY *ec)
>  {
>      BIGNUM *order = BN_new(), *d = BN_new();
> +    if (!order || !d) {
> +       GOSTerr(GOST_F_GOST2001_KEYGEN, ERR_R_MALLOC_FAILURE);
> +       return 0;
> +    }
>      const EC_GROUP *group = EC_KEY_get0_group(ec);
>
>      if(!group || !EC_GROUP_get_order(group, order, NULL)) {
>
>


--
Ticket here: http://rt.openssl.org/Ticket/Display.html?id=4386
Please log in as guest with password guest if prompted

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

Re: [openssl.org #4386] [PATCH] Add sanity checks for BN_new() in OpenSSL-1.0.2g

Rich Salz via RT
Dr. Dale,

    I actually saw that, but forgot to correct it before sending (my
bad)...:(

Bill

On Mon, Mar 7, 2016 at 1:44 PM, [hidden email] via RT <[hidden email]>
wrote:

> If one of the allocation calls succeeds and the other fails, the patched
> code will leak memory.
> It needs something along the lines of:
>
> if (order != NULL) BN_clear_free(order);
> if (d != NULL) BN_clear_free(d);
>
> in the failure case code.
>
>
> Pauli
>
> --
> Oracle
> Dr Paul Dale | Cryptographer | Network Security & Encryption
> Phone +61 7 3031 7217
> Oracle Australia
>
> On Mon, 7 Mar 2016 05:55:23 PM Bill Parker via RT wrote:
> > Hello All,
> >
> > In reviewing code in directory 'engines/ccgost', file 'gost2001.c',
> > there are two calls to BN_new() which are not checked for a return
> > value of NULL, indicating failure.
> >
> > The patch file below should address/correct this issue:
> >
> > --- gost2001.c.orig     2016-03-06 11:32:49.676178425 -0800
> > +++ gost2001.c  2016-03-06 11:38:04.604204158 -0800
> > @@ -434,6 +434,10 @@
> >  int gost2001_keygen(EC_KEY *ec)
> >  {
> >      BIGNUM *order = BN_new(), *d = BN_new();
> > +    if (!order || !d) {
> > +       GOSTerr(GOST_F_GOST2001_KEYGEN, ERR_R_MALLOC_FAILURE);
> > +       return 0;
> > +    }
> >      const EC_GROUP *group = EC_KEY_get0_group(ec);
> >
> >      if(!group || !EC_GROUP_get_order(group, order, NULL)) {
> >
> >
>
>
> --
> Ticket here: http://rt.openssl.org/Ticket/Display.html?id=4386
> Please log in as guest with password guest if prompted
>
>

--
Ticket here: http://rt.openssl.org/Ticket/Display.html?id=4386
Please log in as guest with password guest if prompted

--
openssl-dev mailing list
To unsubscribe: https://mta.openssl.org/mailman/listinfo/openssl-dev