[Pkg-openssl-devel] Bug#363516: Bug#363516: The actual change

[Tim Hudson]

Richard Kettlewell wrote:
> A couple of people have suggested I mention the change that was actually 
> made.  These are the relevant URLs:
> 
> http://svn.debian.org/viewsvn/pkg-openssl?rev=141&view=rev
> http://svn.debian.org/viewsvn/pkg-openssl/openssl/trunk/rand/md_rand.c?rev=141&r1=140&r2=141

What is missing is the context of the change. There seems to be some confusion 
around this.

Although it is the same line of code that is being changed the context is 
entirely different (and it is easy to not be aware of that context when seeing a 
diff and that certainly contributed to this change not being noticed when 
discussed on the openssl lists before it was made in the debian repository).

Basically there are two identical lines of code in completely different 
contexts. One was safe to remove, the other most certainly was not.

You can follow the context at:

http://svn.debian.org/viewsvn/pkg-openssl/openssl/trunk/rand/md_rand.c?rev=141&view=markup

The context of the first change:

Inside ssleay_rand_add (which is called by RAND_add_bytes when using this PRNG). 
This is used when application code is adding entropy to the PRNG and it is 
entirely up to the application where that entropy is coming from - it may be 
from any source including uninitialised buffers if that is what gets passed in.

The code change commented out the *input* bytes coming from the application to 
mix into the PRNG which reduces the seeding of the PRNG to code paths which 
don't go via RAND_add_bytes of which there are few - which is reflected in the 
tool for checking for a weak key with its 250k entries in its internal table.

The context of the second change:

Inside ssleay_rand_bytes (which is called by RAND_bytes when using this PRNG).
This is used when the application code (or any logic in the rest of OpenSSL) 
needs some random data - typically in the context of key generation or random 
padding. What the original code in OpenSSL does is actually mix in the callers 
buffer into which the output is to be copied into the PRNG pool. OpenSSL made a 
change to make this code conditional on PURIFY not being defined as it is of 
course a source of a large number of reports of 'errors' in OpenSSL when using 
purify and when using valgrind where the call chain varies substantially back 
into the application code and so isn't obvious to the casual developer when 
looking at it as to what is going on.

The value of adding that output buffer into the entropy pool can be debated but 
every bit helps and a conservative approach of mixing in whatever is available 
is prudent. The annoyance of a pile of purify and valgrind errors being reported 
against OpenSSL from other packages and applications which use it without a 
clean way of disabling the tools by noting that the usage is safe is 
unfortunate. There are ways to reorganise the code to make it straight forward 
to include the known safe call chain so the other 'real' errors are not hidden 
in the stream of output from purify and valgrind about this issue.

If anyone wants some assistance at writing FAQ entries or responses for this 
then drop me a line - I used to handle vulnerability responses for all the RSA 
security related SDKs so I'm well aware of the process and the importance of 
clear notices to impacted users.

Tim Hudson
tjh at cryptsoft.com / tim.hudson at attglobal.net

---8<---

Attributing blame for this issue is a pretty pointless exercise IMHO. The code 
has been in existance for two years. It is installed on systems I myself use and 
I didn't see the context of the diff in my first reading of the patch when this 
issue was announced which makes me think it was easy to miss. It required closer 
looking at the code (and finding a URL to the actual patch and the whole file in 
context). It is an extremely serious security issue and systems should be 
patched as quickly as possible and keys regenerated.