|
|
|
#521: [PATCH] Avoid uninitialized data in random buffer
|
|
|
|
X
Ticket metadata
|
|
|
X
The Basics
|
|
| Id: |
521 |
| Status: |
resolved |
| Left: |
0 min |
| Priority: |
0/0 |
| Queue: |
OpenSSL-Bugs |
|
|
X
People
|
|
| Owner: |
jaenicke
<lutz@lutz-jaenicke.de>
|
| Requestors: |
daniel.brahneborg@infoflex.se
|
| Cc: |
|
| AdminCc: |
|
|
|
|
X
Links
|
|
| Depends on: |
|
| Depended on by: |
|
| Parents: |
|
| Children: |
|
| Refers to: |
|
| Referred to by: |
|
|
|
X
More about Daniel Brahneborg
|
|
Comments about this user:
No comment entered about this user
This user's 10 highest priority tickets:
Groups this user belongs to:
|
|
|
|
X
History
|
Display mode:[Brief headers] [Full headers]
|
| # |
  |
Fri Feb 28 10:28:30 2003 |
daniel.brahneborg@infoflex.se - Ticket created
|
|
|
|
Hi,
I'm using Valgrind to debug a program that uses the OpenSSL libraries, and got warnings about uninitialized data in the function RSA_padding_add_PKCS1_type_2(), on the line with "} while (*p == '\0');" (line 171 in version 0.9.7a). The following patch ensures that the data is always modified, something that the bytes() method obviously fails to do.
--- openssl-0.9.7a/crypto/rand/rand_lib.c Thu Jan 30 18:37:45 2003 +++ openssl-0.9.7a-safe/crypto/rand/rand_lib.c Wed Feb 26 13:48:27 2003 @@ -154,6 +154,7 @@ int RAND_bytes(unsigned char *buf, int num) { const RAND_METHOD *meth = RAND_get_rand_method(); + memset(buf, 0, num); if (meth && meth->bytes) return meth->bytes(buf,num); return(-1);
/Basic
______________________________________________________________________
OpenSSL Project http://www.openssl.org Development Mailing List openssl-dev@openssl.org Automated List Manager majordomo@openssl.org
|
| # |
  |
Fri Feb 28 11:10:07 2003 |
larsch@trustcenter.de - Correspondence added
|
|
|
|
Daniel Brahneborg via RT wrote:
> Hi,
>
> I'm using Valgrind to debug a program that uses the OpenSSL > libraries, and got warnings about uninitialized data in the > function RSA_padding_add_PKCS1_type_2(), on the line with > "} while (*p == '\0');" (line 171 in version 0.9.7a). The > following patch ensures that the data is always modified, > something that the bytes() method obviously fails to do.
If it's a bug in bytes() why do you change RAND_bytes(), wouldn't it be more appropriate to patch the bytes() function of the correspondig RAND_METHOD ? RAND_bytes() is only a wrapper function to call the bytes() function from the RAND_METHOD object (if existing).
Regards, Nils
|
| # |
  |
Fri Feb 28 11:34:18 2003 |
daniel.brahneborg@infoflex.se - Correspondence added
|
|
|
|
Nils Larsch via RT wrote:
> Daniel Brahneborg via RT wrote:
>>I'm using Valgrind to debug a program that uses the OpenSSL >>libraries, and got warnings about uninitialized data in the >>function RSA_padding_add_PKCS1_type_2(), on the line with >>"} while (*p == '\0');" (line 171 in version 0.9.7a). The >>following patch ensures that the data is always modified, >>something that the bytes() method obviously fails to do.
>
> If it's a bug in bytes() why do you change RAND_bytes(), > wouldn't it be more appropriate to patch the bytes() function > of the correspondig RAND_METHOD ? RAND_bytes() is only a > wrapper function to call the bytes() function from the > RAND_METHOD object (if existing).
Obviously patching the real bytes() is the proper way to do it, but since I didn't have the time to further examine where the uninitialized variable came from, patching RAND_bytes() became the easy (and simple) solution for me.
At lines 467-469 in crypto/rand/md_rand.c is an interesting thing:
#ifndef PURIFY MD_Update(&m,buf,j); /* purify complains */ #endif
That is the code that causes the problem (I just verified it with Valgrind). Does it have any bad side affects to always skip that code? Since both Purify and Valgrind is unhappy with that function call, something must be wrong with it.
/Basic
|
| # |
  |
Thu Mar 13 17:56:30 2003 |
geoff@geoffthorpe.net - Correspondence added
|
|
|
|
I think there's we need to create a FAQ entry about this ...
* Daniel Brahneborg (daniel.brahneborg@infoflex.se) wrote:
[snip]
> At lines 467-469 in crypto/rand/md_rand.c is an interesting > thing:
>
> #ifndef PURIFY > MD_Update(&m,buf,j); /* purify complains */ > #endif
>
> That is the code that causes the problem (I just verified > it with Valgrind). Does it have any bad side affects to > always skip that code? Since both Purify and Valgrind is > unhappy with that function call, something must be wrong > with it.
No, it's fine - the problem is Purify and Valgrind assume all use of uninitialised data is inherently bad, whereas a PRNG implementation has nothing but positive (or more correctly, non-negative) things to say about the idea.
Why do you think the "#ifndef PURIFY" logic is there?
If you're going to run an openssl-based app under instrumentation and *look* for uses of uninitialised data, add "-DPURIFY" to your configure line. Please also search the archives for words like "Valgrind", "Purify", "uninitialised memory", etc. This has come up many times before.
Cheers, Geoff
--
Geoff Thorpe geoff@openssl.org http://www.openssl.org/
|
| # |
  |
Thu Sep 20 09:38:48 2007 |
jaenicke - Taken
|
|
|
|
|
| # |
  |
Thu Sep 20 09:38:52 2007 |
jaenicke - Status changed from 'new' to 'open'
|
|
|
|
|
| # |
  |
Thu Sep 20 09:40:54 2007 |
jaenicke - Correspondence added
|
|
|
|
A respective compile time macro "PEDANTIC" (to be added to the C flags as "-DPEDANTIC") has been added for OpenSSL 0.9.8f. The behavior has been clarified in the manual page and the FAQ
|
| # |
  |
Thu Sep 20 09:40:56 2007 |
jaenicke - Status changed from 'open' to 'resolved'
|
|
|
|
|
|
|
|
Time to display: 0.586305
|
|