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 <[email protected]> > 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 [email protected] Automated List Manager [email protected]
