#521: [PATCH] Avoid uninitialized data in random buffer

# Fri Feb 28 10:28:30 2003 Daniel Brahneborg - Ticket created

Date:	Wed, 26 Feb 2003 14:25:50 +0100
From:	Daniel Brahneborg <daniel.brahneborg@infoflex.se>
To:	openssl-dev@openssl.org
Cc:	Daniel Brahneborg <danbra@infoflex.se>
Subject:	[PATCH] Avoid uninitialized data in random buffer

Download (untitled) / with headers
text/plain 1k

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
Show quoted text

______________________________________________________________________
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 Nils Larsch - Correspondence added

Date:	Fri, 28 Feb 2003 11:10:13 +0100
From:	Nils Larsch <larsch@trustcenter.de>
To:	rt@openssl.org
Subject:	Re: [openssl.org #521] [PATCH] Avoid uninitialized data in random buffer
RT-Send-Cc:

Download (untitled) / with headers
text/plain 687b

Daniel Brahneborg via RT wrote:
Show quoted text

> 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 - Correspondence added

Date:	Fri, 28 Feb 2003 11:34:06 +0100
From:	Daniel Brahneborg <daniel.brahneborg@infoflex.se>
To:	rt@openssl.org
Cc:	openssl-dev@openssl.org, Daniel Brahneborg <daniel.brahneborg@infoflex.se>
Subject:	Re: [openssl.org #521] [PATCH] Avoid uninitialized data in random buffer
RT-Send-Cc:

Download (untitled) / with headers
text/plain 1.2k

Nils Larsch via RT wrote:
Show quoted text

> 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

Date:	Thu, 13 Mar 2003 11:56:17 -0500
From:	Geoff Thorpe <geoff@geoffthorpe.net>
Subject:	Re: [openssl.org #521] [PATCH] Avoid uninitialized data in random buffer
To:	openssl-dev@openssl.org
Cc:	rt@openssl.org, Daniel Brahneborg <daniel.brahneborg@infoflex.se>
RT-Send-Cc:

Download (untitled) / with headers
text/plain 1.1k

I think there's we need to create a FAQ entry about this ...

* Daniel Brahneborg (daniel.brahneborg@infoflex.se) wrote:

[snip]

Show quoted text

> 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 Lutz Jaenicke - Taken

# Thu Sep 20 09:38:52 2007 Lutz Jaenicke - Status changed from 'new' to 'open'

# Thu Sep 20 09:40:54 2007 Lutz Jaenicke - Correspondence added

Download (untitled) / with headers
text/plain 185b

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 Lutz Jaenicke - Status changed from 'open' to 'resolved'