[openssl.org #3387] Bug Report with fixes: null pointer and uninitialised memory errors

classic Classic list List threaded Threaded
8 messages Options
Reply | Threaded
Open this post in threaded view
|

[openssl.org #3387] Bug Report with fixes: null pointer and uninitialised memory errors

Rich Salz via RT
Hello,

We ran parfait on OpenSSL and found the following errors in openssl-1.0.1g:

1. Error: Uninitialised memory (CWE 456)
    Possible access to uninitialised memory '&num'
         at line 267 of
components/openssl/openssl-1.0.1/build/sparcv9-wanboot/crypto/evp/bio_b64.c
in function 'b64_read'.
&num allocated at line 146.
&num uninitialised when ctx->start != 0 at line 221.
2. Error: Null pointer dereference (CWE 476)
    Read from null pointer rctx
         at line 114 of
components/openssl/openssl-1.0.1/build/sparcv9-wanboot/crypto/ocsp/ocsp_ht.c
in function 'OCSP_REQ_CTX_free'.
           Function OCSP_sendreq_new may return constant 'NULL' at line
171, called at line 491 in function 'OCSP_sendreq _bio'.
           Constant 'NULL' passed into function OCSP_REQ_CTX_free,
argument rctx, from call at line 498.
           Null pointer introduced at line 171 in function
'OCSP_sendreq_new'.
3. Error: Null pointer dereference (CWE 476)
    Read from null pointer rctx
         at line 268 of
components/openssl/openssl-1.0.1/build/sparcv9-wanboot/crypto/ocsp/ocsp_ht.c
in function 'OCSP_sendreq_nbio'.
           Function OCSP_sendreq_new may return constant 'NULL' at line
171, called at line 491 in function 'OCSP_sendreq_bio'.
           Constant 'NULL' passed into function OCSP_sendreq_nbio,
argument rctx, from call at line 495.
           Null pointer introduced at line 171 in function
'OCSP_sendreq_new'.
4. Error: Null pointer dereference (CWE 476)
    Read from null pointer frag
         at line 1175 of
components/openssl/openssl-1.0.1/build/sparcv9-wanboot/ssl/d1_both.c in
function 'dtls1_buffer_message'.
           Function dtls1_hm_fragment_new may return constant 'NULL' at
line 189, called at line 1173.
           Null pointer introduced at line 189 in function
'dtls1_hm_fragment_new'.

The following changes fixes the errors:

    2 --- openssl-1.0.1g/crypto/evp/bio_b64.c.~1~     Tue Jun  3
14:13:33 2014
    3 +++ openssl-1.0.1g/crypto/evp/bio_b64.c Tue Jun  3 14:14:23 2014
    4 @@ -143,7 +143,7 @@
    5
    6  static int b64_read(BIO *b, char *out, int outl)
    7         {
    8 -       int ret=0,i,ii,j,k,x,n,num,ret_code=0;
    9 +       int ret=0,i,ii,j,k,x,n,num=0,ret_code=0;
   10         BIO_B64_CTX *ctx;
   11         unsigned char *p,*q;
   12
   13 --- openssl-1.0.1g/crypto/ocsp/ocsp_ht.c.~1~    Tue Jun  3
14:15:18 2014
   14 +++ openssl-1.0.1g/crypto/ocsp/ocsp_ht.c        Tue Jun  3
14:15:46 2014
   15 @@ -490,6 +490,9 @@
   16
   17         ctx = OCSP_sendreq_new(b, path, req, -1);
   18
   19 +       if (!ctx)
   20 +               return NULL;
   21 +
   22         do
   23                 {
   24                 rv = OCSP_sendreq_nbio(&resp, ctx);
   25 --- openssl-1.0.1g/ssl/d1_both.c.~1~    Tue Jun  3 14:16:25 2014
   26 +++ openssl-1.0.1g/ssl/d1_both.c        Tue Jun  3 14:17:26 2014
   27 @@ -1172,6 +1172,8 @@
   28
   29         frag = dtls1_hm_fragment_new(s->init_num, 0);
   30
   31 +       if (!frag)
   32 +               return 0;
   33         memcpy(frag->fragment, s->init_buf->data, s->init_num);
   34
   35         if ( is_ccs)

Can you integrate this into the next release of OpenSSL?

Thanks,

Jenny Yung
Oracle Solaris Security

______________________________________________________________________
OpenSSL Project                                 http://www.openssl.org
Development Mailing List                       [hidden email]
Automated List Manager                           [hidden email]
Reply | Threaded
Open this post in threaded view
|

Re: [openssl.org #3387] Bug Report with fixes: null pointer and uninitialised memory errors

Rich Salz via RT
On 7/06/2014 7:10 PM, Jenny Yung via RT wrote:

> Hello,
>
> We ran parfait on OpenSSL and found the following errors in openssl-1.0.1g:
>
> 1. Error: Uninitialised memory (CWE 456)
>     Possible access to uninitialised memory '&num'
>          at line 267 of
> components/openssl/openssl-1.0.1/build/sparcv9-wanboot/crypto/evp/bio_b64.c
> in function 'b64_read'.
> &num allocated at line 146.
> &num uninitialised when ctx->start != 0 at line 221.

Already fixed in the 1.0.1 stable branch so it is already included in
1.0.1h onwards and 1.0.1m is the current recommended version.

commit a41d5174e27c99d1caefd76a8e927c814ede509e
Author: Dr. Stephen Henson <[hidden email]>
Date:   Tue May 6 14:07:37 2014 +0100

    Initialize num properly.
   
    PR#3289
    PR#3345
    (cherry picked from commit 3ba1e406c2309adb427ced9815ebf05f5b58d155)


> 2. Error: Null pointer dereference (CWE 476)
>     Read from null pointer rctx
>          at line 114 of
> components/openssl/openssl-1.0.1/build/sparcv9-wanboot/crypto/ocsp/ocsp_ht.c
> in function 'OCSP_REQ_CTX_free'.
>            Function OCSP_sendreq_new may return constant 'NULL' at line
> 171, called at line 491 in function 'OCSP_sendreq _bio'.
>            Constant 'NULL' passed into function OCSP_REQ_CTX_free,
> argument rctx, from call at line 498.
>            Null pointer introduced at line 171 in function
> 'OCSP_sendreq_new'.

This indicates a different issue is present - in that the error handling
path will leak memory.

        rctx->iobuf = OPENSSL_malloc(rctx->iobuflen);
        if (!rctx->iobuf)
                return 0;

So if malloc fails rctx itself isn't freed - so that will leak. That
will need to be looked at too.


> 3. Error: Null pointer dereference (CWE 476)
>     Read from null pointer rctx
>          at line 268 of
> components/openssl/openssl-1.0.1/build/sparcv9-wanboot/crypto/ocsp/ocsp_ht.c
> in function 'OCSP_sendreq_nbio'.
>            Function OCSP_sendreq_new may return constant 'NULL' at line
> 171, called at line 491 in function 'OCSP_sendreq_bio'.
>            Constant 'NULL' passed into function OCSP_sendreq_nbio,
> argument rctx, from call at line 495.
>            Null pointer introduced at line 171 in function
> 'OCSP_sendreq_new'.

Looks good - but missed other issue with memory leak on malloc failure.

> 4. Error: Null pointer dereference (CWE 476)
>     Read from null pointer frag
>          at line 1175 of
> components/openssl/openssl-1.0.1/build/sparcv9-wanboot/ssl/d1_both.c in
> function 'dtls1_buffer_message'.
>            Function dtls1_hm_fragment_new may return constant 'NULL' at
> line 189, called at line 1173.
>            Null pointer introduced at line 189 in function
> 'dtls1_hm_fragment_new'.

Looks good.

> The following changes fixes the errors:
>
>     2 --- openssl-1.0.1g/crypto/evp/bio_b64.c.~1~     Tue Jun  3
> 14:13:33 2014
>     3 +++ openssl-1.0.1g/crypto/evp/bio_b64.c Tue Jun  3 14:14:23 2014
>     4 @@ -143,7 +143,7 @@
>     5
>     6  static int b64_read(BIO *b, char *out, int outl)
>     7         {
>     8 -       int ret=0,i,ii,j,k,x,n,num,ret_code=0;
>     9 +       int ret=0,i,ii,j,k,x,n,num=0,ret_code=0;
>    10         BIO_B64_CTX *ctx;
>    11         unsigned char *p,*q;

Already covered in previous commits.

>    12
>    13 --- openssl-1.0.1g/crypto/ocsp/ocsp_ht.c.~1~    Tue Jun  3
> 14:15:18 2014
>    14 +++ openssl-1.0.1g/crypto/ocsp/ocsp_ht.c        Tue Jun  3
> 14:15:46 2014
>    15 @@ -490,6 +490,9 @@
>    16
>    17         ctx = OCSP_sendreq_new(b, path, req, -1);
>    18
>    19 +       if (!ctx)
>    20 +               return NULL;
>    21 +
>    22         do
>    23                 {
>    24                 rv = OCSP_sendreq_nbio(&resp, ctx);

Looks reasonable - although I don't think the spin loop there is
appropriate - basically with no delay, and no select, this will spin on
a non-blocking retry condition (which is meant to make it back to the
caller to enter their event loop. That is a broader issue to look at.

>    25 --- openssl-1.0.1g/ssl/d1_both.c.~1~    Tue Jun  3 14:16:25 2014
>    26 +++ openssl-1.0.1g/ssl/d1_both.c        Tue Jun  3 14:17:26 2014
>    27 @@ -1172,6 +1172,8 @@
>    28
>    29         frag = dtls1_hm_fragment_new(s->init_num, 0);
>    30
>    31 +       if (!frag)
>    32 +               return 0;
>    33         memcpy(frag->fragment, s->init_buf->data, s->init_num);
>    34
>    35         if ( is_ccs)

That looks good as a patch.

> Can you integrate this into the next release of OpenSSL?

Can you re-run parfait against the current release version of OpenSSL
for that branch - i.e. 1.0.1m

It would also be helpful to see suggested patch as a separate RT issue -
so we can discuss and track them individually.

Thanks,
Tim.


______________________________________________________________________
OpenSSL Project                                 http://www.openssl.org
Development Mailing List                       [hidden email]
Automated List Manager                           [hidden email]
Reply | Threaded
Open this post in threaded view
|

Re: [openssl.org #3387] Bug Report with fixes: null pointer and uninitialised memory errors

Rich Salz via RT
On Sun, Jun 08, 2014 at 12:01:28AM +0200, Tim Hudson via RT wrote:
> Already fixed in the 1.0.1 stable branch so it is already included in
> 1.0.1h onwards and 1.0.1m is the current recommended version.
[...]
> Can you re-run parfait against the current release version of OpenSSL
> for that branch - i.e. 1.0.1m

It seems you have your branches mixed up.  The latest version is
1.0.1h.  There is an also an 1.0.0m, but that's from an older
branch.


Kurt


______________________________________________________________________
OpenSSL Project                                 http://www.openssl.org
Development Mailing List                       [hidden email]
Automated List Manager                           [hidden email]
Reply | Threaded
Open this post in threaded view
|

Re: [openssl.org #3387] Bug Report with fixes: null pointer and uninitialised memory errors

Rich Salz via RT
On 8/06/2014 11:40 AM, Kurt Roeckx via RT wrote:
> On Sun, Jun 08, 2014 at 12:01:28AM +0200, Tim Hudson via RT wrote:
>> Already fixed in the 1.0.1 stable branch so it is already included in
>> 1.0.1h onwards and 1.0.1m is the current recommended version.
> [...]
>> Can you re-run parfait against the current release version of OpenSSL
>> for that branch - i.e. 1.0.1m
> It seems you have your branches mixed up.  The latest version is
> 1.0.1h.  There is an also an 1.0.0m, but that's from an older
> branch.

Opps - you are right - I did indeed mean *1.0.1h* ... 'm' is in the
1.0.0 branch - and I am requesting is for it to be run against the
current 1.0.1 version not an older version - which was especially
noticeable when it is pointing out an already resolved item.

It is always a good idea to run any tools against the current release
versions for a particular branch - and also handy to see the same
reports against "master" so that the forward development version also
gets items picked up - as it contains the latest not-yet-in-a-release code.

For coverity we use "master" and "OpenSSL_1_0_1-stable"

Tim.


______________________________________________________________________
OpenSSL Project                                 http://www.openssl.org
Development Mailing List                       [hidden email]
Automated List Manager                           [hidden email]
Reply | Threaded
Open this post in threaded view
|

Re: [openssl.org #3387] Bug Report with fixes: null pointer and uninitialised memory errors

Jenny Yung
In reply to this post by Rich Salz via RT
Hi Tim,

Thanks for your feedback!

On 06/ 7/14 03:01 PM, Tim Hudson via RT wrote:

> On 7/06/2014 7:10 PM, Jenny Yung via RT wrote:
>> Hello,
>>
>> We ran parfait on OpenSSL and found the following errors in openssl-1.0.1g:
>>
>> 1. Error: Uninitialised memory (CWE 456)
>>      Possible access to uninitialised memory '&num'
>>           at line 267 of
>> components/openssl/openssl-1.0.1/build/sparcv9-wanboot/crypto/evp/bio_b64.c
>> in function 'b64_read'.
>> &num allocated at line 146.
>> &num uninitialised when ctx->start != 0 at line 221.
> Already fixed in the 1.0.1 stable branch so it is already included in
> 1.0.1h onwards and 1.0.1m is the current recommended version.
>
> commit a41d5174e27c99d1caefd76a8e927c814ede509e
> Author: Dr. Stephen Henson <[hidden email]>
> Date:   Tue May 6 14:07:37 2014 +0100
>
>      Initialize num properly.
>    
>      PR#3289
>      PR#3345
>      (cherry picked from commit 3ba1e406c2309adb427ced9815ebf05f5b58d155)
I ran it on 1.0.1h and confirmed that it has been fixed. Thanks.

>
>> 2. Error: Null pointer dereference (CWE 476)
>>      Read from null pointer rctx
>>           at line 114 of
>> components/openssl/openssl-1.0.1/build/sparcv9-wanboot/crypto/ocsp/ocsp_ht.c
>> in function 'OCSP_REQ_CTX_free'.
>>             Function OCSP_sendreq_new may return constant 'NULL' at line
>> 171, called at line 491 in function 'OCSP_sendreq _bio'.
>>             Constant 'NULL' passed into function OCSP_REQ_CTX_free,
>> argument rctx, from call at line 498.
>>             Null pointer introduced at line 171 in function
>> 'OCSP_sendreq_new'.
> This indicates a different issue is present - in that the error handling
> path will leak memory.
>
>          rctx->iobuf = OPENSSL_malloc(rctx->iobuflen);
>          if (!rctx->iobuf)
>                  return 0;
>
> So if malloc fails rctx itself isn't freed - so that will leak. That
> will need to be looked at too.

see below

>
>
>> 3. Error: Null pointer dereference (CWE 476)
>>      Read from null pointer rctx
>>           at line 268 of
>> components/openssl/openssl-1.0.1/build/sparcv9-wanboot/crypto/ocsp/ocsp_ht.c
>> in function 'OCSP_sendreq_nbio'.
>>             Function OCSP_sendreq_new may return constant 'NULL' at line
>> 171, called at line 491 in function 'OCSP_sendreq_bio'.
>>             Constant 'NULL' passed into function OCSP_sendreq_nbio,
>> argument rctx, from call at line 495.
>>             Null pointer introduced at line 171 in function
>> 'OCSP_sendreq_new'.
> Looks good - but missed other issue with memory leak on malloc failure.
>
>> 4. Error: Null pointer dereference (CWE 476)
>>      Read from null pointer frag
>>           at line 1175 of
>> components/openssl/openssl-1.0.1/build/sparcv9-wanboot/ssl/d1_both.c in
>> function 'dtls1_buffer_message'.
>>             Function dtls1_hm_fragment_new may return constant 'NULL' at
>> line 189, called at line 1173.
>>             Null pointer introduced at line 189 in function
>> 'dtls1_hm_fragment_new'.
> Looks good.
>
>> The following changes fixes the errors:
>>
>>      2 --- openssl-1.0.1g/crypto/evp/bio_b64.c.~1~     Tue Jun  3
>> 14:13:33 2014
>>      3 +++ openssl-1.0.1g/crypto/evp/bio_b64.c Tue Jun  3 14:14:23 2014
>>      4 @@ -143,7 +143,7 @@
>>      5
>>      6  static int b64_read(BIO *b, char *out, int outl)
>>      7         {
>>      8 -       int ret=0,i,ii,j,k,x,n,num,ret_code=0;
>>      9 +       int ret=0,i,ii,j,k,x,n,num=0,ret_code=0;
>>     10         BIO_B64_CTX *ctx;
>>     11         unsigned char *p,*q;
> Already covered in previous commits.
>
>>     12
>>     13 --- openssl-1.0.1g/crypto/ocsp/ocsp_ht.c.~1~    Tue Jun  3
>> 14:15:18 2014
>>     14 +++ openssl-1.0.1g/crypto/ocsp/ocsp_ht.c        Tue Jun  3
>> 14:15:46 2014
>>     15 @@ -490,6 +490,9 @@
>>     16
>>     17         ctx = OCSP_sendreq_new(b, path, req, -1);
>>     18
>>     19 +       if (!ctx)
>>     20 +               return NULL;
>>     21 +
>>     22         do
>>     23                 {
>>     24                 rv = OCSP_sendreq_nbio(&resp, ctx);
> Looks reasonable - although I don't think the spin loop there is
> appropriate - basically with no delay, and no select, this will spin on
> a non-blocking retry condition (which is meant to make it back to the
> caller to enter their event loop. That is a broader issue to look at.
I see -- my change blocks the do-while loop. I'll check this some more.
Do you have any suggestions for now?

>
>>     25 --- openssl-1.0.1g/ssl/d1_both.c.~1~    Tue Jun  3 14:16:25 2014
>>     26 +++ openssl-1.0.1g/ssl/d1_both.c        Tue Jun  3 14:17:26 2014
>>     27 @@ -1172,6 +1172,8 @@
>>     28
>>     29         frag = dtls1_hm_fragment_new(s->init_num, 0);
>>     30
>>     31 +       if (!frag)
>>     32 +               return 0;
>>     33         memcpy(frag->fragment, s->init_buf->data, s->init_num);
>>     34
>>     35         if ( is_ccs)
> That looks good as a patch.
>
>> Can you integrate this into the next release of OpenSSL?
> Can you re-run parfait against the current release version of OpenSSL
> for that branch - i.e. 1.0.1m
>
> It would also be helpful to see suggested patch as a separate RT issue -
> so we can discuss and track them individually.

I have submitted another RT issue with the patch (after removing the
first part that was fixed in 1.0.1h)

Thanks!

Jenny
>
> Thanks,
> Tim.
>
>
______________________________________________________________________
OpenSSL Project                                 http://www.openssl.org
Development Mailing List                       [hidden email]
Automated List Manager                           [hidden email]
Reply | Threaded
Open this post in threaded view
|

Re: [openssl.org #3387] Bug Report with fixes: null pointer and uninitialised memory errors

Rich Salz via RT
Hi Tim,

Thanks for your feedback!

On 06/ 7/14 03:01 PM, Tim Hudson via RT wrote:

> On 7/06/2014 7:10 PM, Jenny Yung via RT wrote:
>> Hello,
>>
>> We ran parfait on OpenSSL and found the following errors in openssl-1.0.1g:
>>
>> 1. Error: Uninitialised memory (CWE 456)
>>      Possible access to uninitialised memory '&num'
>>           at line 267 of
>> components/openssl/openssl-1.0.1/build/sparcv9-wanboot/crypto/evp/bio_b64.c
>> in function 'b64_read'.
>> &num allocated at line 146.
>> &num uninitialised when ctx->start != 0 at line 221.
> Already fixed in the 1.0.1 stable branch so it is already included in
> 1.0.1h onwards and 1.0.1m is the current recommended version.
>
> commit a41d5174e27c99d1caefd76a8e927c814ede509e
> Author: Dr. Stephen Henson <[hidden email]>
> Date:   Tue May 6 14:07:37 2014 +0100
>
>      Initialize num properly.
>    
>      PR#3289
>      PR#3345
>      (cherry picked from commit 3ba1e406c2309adb427ced9815ebf05f5b58d155)
I ran it on 1.0.1h and confirmed that it has been fixed. Thanks.

>
>> 2. Error: Null pointer dereference (CWE 476)
>>      Read from null pointer rctx
>>           at line 114 of
>> components/openssl/openssl-1.0.1/build/sparcv9-wanboot/crypto/ocsp/ocsp_ht.c
>> in function 'OCSP_REQ_CTX_free'.
>>             Function OCSP_sendreq_new may return constant 'NULL' at line
>> 171, called at line 491 in function 'OCSP_sendreq _bio'.
>>             Constant 'NULL' passed into function OCSP_REQ_CTX_free,
>> argument rctx, from call at line 498.
>>             Null pointer introduced at line 171 in function
>> 'OCSP_sendreq_new'.
> This indicates a different issue is present - in that the error handling
> path will leak memory.
>
>          rctx->iobuf = OPENSSL_malloc(rctx->iobuflen);
>          if (!rctx->iobuf)
>                  return 0;
>
> So if malloc fails rctx itself isn't freed - so that will leak. That
> will need to be looked at too.

see below

>
>
>> 3. Error: Null pointer dereference (CWE 476)
>>      Read from null pointer rctx
>>           at line 268 of
>> components/openssl/openssl-1.0.1/build/sparcv9-wanboot/crypto/ocsp/ocsp_ht.c
>> in function 'OCSP_sendreq_nbio'.
>>             Function OCSP_sendreq_new may return constant 'NULL' at line
>> 171, called at line 491 in function 'OCSP_sendreq_bio'.
>>             Constant 'NULL' passed into function OCSP_sendreq_nbio,
>> argument rctx, from call at line 495.
>>             Null pointer introduced at line 171 in function
>> 'OCSP_sendreq_new'.
> Looks good - but missed other issue with memory leak on malloc failure.
>
>> 4. Error: Null pointer dereference (CWE 476)
>>      Read from null pointer frag
>>           at line 1175 of
>> components/openssl/openssl-1.0.1/build/sparcv9-wanboot/ssl/d1_both.c in
>> function 'dtls1_buffer_message'.
>>             Function dtls1_hm_fragment_new may return constant 'NULL' at
>> line 189, called at line 1173.
>>             Null pointer introduced at line 189 in function
>> 'dtls1_hm_fragment_new'.
> Looks good.
>
>> The following changes fixes the errors:
>>
>>      2 --- openssl-1.0.1g/crypto/evp/bio_b64.c.~1~     Tue Jun  3
>> 14:13:33 2014
>>      3 +++ openssl-1.0.1g/crypto/evp/bio_b64.c Tue Jun  3 14:14:23 2014
>>      4 @@ -143,7 +143,7 @@
>>      5
>>      6  static int b64_read(BIO *b, char *out, int outl)
>>      7         {
>>      8 -       int ret=0,i,ii,j,k,x,n,num,ret_code=0;
>>      9 +       int ret=0,i,ii,j,k,x,n,num=0,ret_code=0;
>>     10         BIO_B64_CTX *ctx;
>>     11         unsigned char *p,*q;
> Already covered in previous commits.
>
>>     12
>>     13 --- openssl-1.0.1g/crypto/ocsp/ocsp_ht.c.~1~    Tue Jun  3
>> 14:15:18 2014
>>     14 +++ openssl-1.0.1g/crypto/ocsp/ocsp_ht.c        Tue Jun  3
>> 14:15:46 2014
>>     15 @@ -490,6 +490,9 @@
>>     16
>>     17         ctx = OCSP_sendreq_new(b, path, req, -1);
>>     18
>>     19 +       if (!ctx)
>>     20 +               return NULL;
>>     21 +
>>     22         do
>>     23                 {
>>     24                 rv = OCSP_sendreq_nbio(&resp, ctx);
> Looks reasonable - although I don't think the spin loop there is
> appropriate - basically with no delay, and no select, this will spin on
> a non-blocking retry condition (which is meant to make it back to the
> caller to enter their event loop. That is a broader issue to look at.
I see -- my change blocks the do-while loop. I'll check this some more.
Do you have any suggestions for now?

>
>>     25 --- openssl-1.0.1g/ssl/d1_both.c.~1~    Tue Jun  3 14:16:25 2014
>>     26 +++ openssl-1.0.1g/ssl/d1_both.c        Tue Jun  3 14:17:26 2014
>>     27 @@ -1172,6 +1172,8 @@
>>     28
>>     29         frag = dtls1_hm_fragment_new(s->init_num, 0);
>>     30
>>     31 +       if (!frag)
>>     32 +               return 0;
>>     33         memcpy(frag->fragment, s->init_buf->data, s->init_num);
>>     34
>>     35         if ( is_ccs)
> That looks good as a patch.
>
>> Can you integrate this into the next release of OpenSSL?
> Can you re-run parfait against the current release version of OpenSSL
> for that branch - i.e. 1.0.1m
>
> It would also be helpful to see suggested patch as a separate RT issue -
> so we can discuss and track them individually.

I have submitted another RT issue with the patch (after removing the
first part that was fixed in 1.0.1h)

Thanks!

Jenny
>
> Thanks,
> Tim.
>
>


______________________________________________________________________
OpenSSL Project                                 http://www.openssl.org
Development Mailing List                       [hidden email]
Automated List Manager                           [hidden email]
Reply | Threaded
Open this post in threaded view
|

Re: [openssl.org #3387] Bug Report with fixes: null pointer and uninitialised memory errors

Misaki Miyashita
In reply to this post by Rich Salz via RT
Thank you, Tim.

>> 2. Error: Null pointer dereference (CWE 476)
>>      Read from null pointer rctx
>>           at line 114 of
>> components/openssl/openssl-1.0.1/build/sparcv9-wanboot/crypto/ocsp/ocsp_ht.c
>> in function 'OCSP_REQ_CTX_free'.
>>             Function OCSP_sendreq_new may return constant 'NULL' at line
>> 171, called at line 491 in function 'OCSP_sendreq _bio'.
>>             Constant 'NULL' passed into function OCSP_REQ_CTX_free,
>> argument rctx, from call at line 498.
>>             Null pointer introduced at line 171 in function
>> 'OCSP_sendreq_new'.
> This indicates a different issue is present - in that the error handling
> path will leak memory.
>
>          rctx->iobuf = OPENSSL_malloc(rctx->iobuflen);
>          if (!rctx->iobuf)
>                  return 0;
>
> So if malloc fails rctx itself isn't freed - so that will leak. That
> will need to be looked at too.

Good point!  We'll file a RT to check for the NULL pointer and free the
malloced resources on the error exit (multiple places in the function)

>>     12
>>     13 --- openssl-1.0.1g/crypto/ocsp/ocsp_ht.c.~1~    Tue Jun  3
>> 14:15:18 2014
>>     14 +++ openssl-1.0.1g/crypto/ocsp/ocsp_ht.c        Tue Jun  3
>> 14:15:46 2014
>>     15 @@ -490,6 +490,9 @@
>>     16
>>     17         ctx = OCSP_sendreq_new(b, path, req, -1);
>>     18
>>     19 +       if (!ctx)
>>     20 +               return NULL;
>>     21 +
>>     22         do
>>     23                 {
>>     24                 rv = OCSP_sendreq_nbio(&resp, ctx);
> Looks reasonable - although I don't think the spin loop there is
> appropriate - basically with no delay, and no select, this will spin on
> a non-blocking retry condition (which is meant to make it back to the
> caller to enter their event loop. That is a broader issue to look at.

Assuming you are referring to the do-while loop when you said 'spin
loop', that should be looked at separately.
Jenny's suggestion to check the return value of OCSP_sendreq_new()
should be a valid check.

Regards,

-- misaki
______________________________________________________________________
OpenSSL Project                                 http://www.openssl.org
Development Mailing List                       [hidden email]
Automated List Manager                           [hidden email]
Reply | Threaded
Open this post in threaded view
|

Re: [openssl.org #3387] Bug Report with fixes: null pointer and uninitialised memory errors

Rich Salz via RT
Thank you, Tim.

>> 2. Error: Null pointer dereference (CWE 476)
>>      Read from null pointer rctx
>>           at line 114 of
>> components/openssl/openssl-1.0.1/build/sparcv9-wanboot/crypto/ocsp/ocsp_ht.c
>> in function 'OCSP_REQ_CTX_free'.
>>             Function OCSP_sendreq_new may return constant 'NULL' at line
>> 171, called at line 491 in function 'OCSP_sendreq _bio'.
>>             Constant 'NULL' passed into function OCSP_REQ_CTX_free,
>> argument rctx, from call at line 498.
>>             Null pointer introduced at line 171 in function
>> 'OCSP_sendreq_new'.
> This indicates a different issue is present - in that the error handling
> path will leak memory.
>
>          rctx->iobuf = OPENSSL_malloc(rctx->iobuflen);
>          if (!rctx->iobuf)
>                  return 0;
>
> So if malloc fails rctx itself isn't freed - so that will leak. That
> will need to be looked at too.

Good point!  We'll file a RT to check for the NULL pointer and free the
malloced resources on the error exit (multiple places in the function)

>>     12
>>     13 --- openssl-1.0.1g/crypto/ocsp/ocsp_ht.c.~1~    Tue Jun  3
>> 14:15:18 2014
>>     14 +++ openssl-1.0.1g/crypto/ocsp/ocsp_ht.c        Tue Jun  3
>> 14:15:46 2014
>>     15 @@ -490,6 +490,9 @@
>>     16
>>     17         ctx = OCSP_sendreq_new(b, path, req, -1);
>>     18
>>     19 +       if (!ctx)
>>     20 +               return NULL;
>>     21 +
>>     22         do
>>     23                 {
>>     24                 rv = OCSP_sendreq_nbio(&resp, ctx);
> Looks reasonable - although I don't think the spin loop there is
> appropriate - basically with no delay, and no select, this will spin on
> a non-blocking retry condition (which is meant to make it back to the
> caller to enter their event loop. That is a broader issue to look at.

Assuming you are referring to the do-while loop when you said 'spin
loop', that should be looked at separately.
Jenny's suggestion to check the return value of OCSP_sendreq_new()
should be a valid check.

Regards,

-- misaki


______________________________________________________________________
OpenSSL Project                                 http://www.openssl.org
Development Mailing List                       [hidden email]
Automated List Manager                           [hidden email]