Cryptographic weakness on Debian systems

"How do we trust this announcement about weak keys is real?"

"We look at the keys they used in the annou... shit."

Is it just me, or did the "fix" only back out the change in one of two places?  See the patch
which caused the problem:

http://svn.debian.org/viewsvn/pkg-openssl/openssl/trunk/r...

The offending change is in two places.  Now see the "fix":

http://svn.debian.org/viewsvn/pkg-openssl/openssl/trunk/c...

The patch is only half undone.  Oversight?

> It does seem a little weird: if that was the only source of randomness,
> then it's not a very good source & needs fixing!

Only if that part is really to "add uninitialised data"!  Note that the comment is added by
the original Debian patch, not in the original SSL library.  The "buf" argument is actually
passed by a call to the "add" function of the rand_meth_st interface, the interface defining
how to collect random data for various pluggable methods.  My thinking is that it might
actually not be uninitialized data at all, but is instead the content handed over by a part of
the interface for SSL random number pool that actually accept random data and add them to the
pool, i.e., most of the "real" randomness available.

Somehow the URL in my message is chopped...  Here's the correct one.

http://www.dslreports.com/forum/r20474302-Heads-Up-Debian...

Fair enough. :-) I just needed a short-term fix while reading the advisories, and thought I'd
share it.

I'm relatively impressed by the Ubuntu patch, with its built-in blacklist and support for
regenerating host keys. (Time will tell if this code actually works.)

The Debian patch? Not so much. I'm still waiting to see how long it takes for anything other
than a placeholder to appear here:

http://www.debian.org/security/key-rollover/

And the rationale for the change can be found in the thread at
<http://marc.info/?t=114651088900003&r=1&w=2>.

It's interesting, looks like there's plenty of embarassment to go round.

This function does not just take arbitrary "uninitialized memory" and feed it into the entropy
pool. That would be stupid. It takes a parameter, which is supposed to be an array of entropy
bytes to put into the pool.

I haven't analysed the code enough to see whether the Valgrind reports are triggered by this
function going off the end of the provided buffer, or by the buffer providers (elsewhere in
OpenSSL code) not fully initialising their buffers, or even by a subtle false alarm (Valgrind
is good but it's not perfect, not yet).

In any case, with the patch removing this vital line of code, more or less none of the
intended entropy ends up being used. Entropy from /dev/random, from hardware RNG sources, and
so on, is discarded silently by Debian's patch AFAICT. Hence the list of millions of possible
"bogus" keys which can be generated from the little remaining entropy.

What's fascinating is that when Debian reported this terrible goof today some OpenSSL people
said if they'd come forward with this patch two years ago the OpenSSL developers would have
laughed at the obviously wrong fix... But it seems that Debian people did go to the OpenSSL
developers two years ago, and they got told more or less that if it shuts up Valgrind then
it's fine.

Now in parallel to proposing this goofy patch, someone proposed to Debian a fix which simply
writes zeroes to an uninitialised buffer elsewhere in OpenSSL. This patch seems to have been
rejected by Debian for unclear reasons, but an equivalent change has been included in newer
versions of OpenSSL and from there integrated back into Debian. It is probably one of several
such fixes which should have taken place instead of this silly one line patch that disables
the RNG more or less altogether.

The level of entropy would be essentially zero. Memory starts out as zeroed at process
creation time.  It may end up being recycled internally to the same process, but that would be
very predictable.

I suppose you could have some situation like:

 x = malloc(10);
 memcpy(x, randsrc, 10);
 free(x);
 y = malloc(10); /* we just happen to get the same memory */

could cause the situation as described, but that would be a horrible bug -- there is no
guarantee that y wouldn't be all zeros anyway.

I don't think that's what happened here though.

From other comments I think it may be the case that the zeroed buffer contained real entropy,
but possibly not filling the whole thing, thus reading it causes reads of uninitialized bytes.
It could also be that the entropy in the buffer was generated using an uninitialized variable
somewhere (any result of a calculation with uninitialized values must be treated as if it
results in uninitialized values).

I look forward to someone doing a more in-depth inspection of this code -- it should be made
clear exactly how the entropy is gathered and passed around, and there should be no
uninitialized values stored or read from the buffer.  Adding initializations at random points
in the code is not the way to write secure software.

I don't think it was completely uninitialized.
If it is, then it was a really bad idea, as there should be no expectation that it have any
useful entropy any more than you should expect it to be filled with zeros.

Plus, a C program which does that is buggy -- you can't read memory you haven't initialized.
Sure it mostly worked, but it is still a bug.

(Not that any of that excuses the "fix" applied.)

> If it indeed comes from the "uninitialized" (non-written) part of heap or
> stack, kernel has handily zeroed it for you...

I've learnt from a winning entry of the underhanded C contest that "uninitialized" stack can
contain interesting data, i.e., stack frames of previous calls.  The same can be said about
heap space, as long as someone used the free() function some time ago, and the heap space is
reused from there.  The data is not necessarily constant, since the data stored might reflect
the environment, e.g., it might contain the stat buffer so it contains data like the last
access time of files.  Or somebody called time() and put the result in the stack or heap, used
it and later free, and the space end up being reused.  Of course, all these does not seem like
a good source of untainted randomness.

> When compiled with a sufficiently nasty compiler...

I don't think this is true.  Does the C standard say anywhere that if you read an arbitrary
buffer before ever writing it, then the entire buffer can be ignored forevermore?  Even though
the OpenSSL code is idiotic, even the nastiest C compiler will be obligated to do the right
thing.

> openssl guys are not strong on admitting fault.

That is all too true.  :(  The OpenSSL guys' handling of this incident has been so strange
that I wonder if they're related to OpenBSD somehow...?

That's pretty funny that the development mailing list ( *-dev == development in every other
mailing list in existance)  is the incorrect place to try to communicate with developers.

Were on OpenSSL's website do they give the correct place to try to talk to developers then?

I updated openssl as soon as it was available and then ran ssh-keygen to replace
/root/.ssh/id_rsa and /root/.ssh/id_rsa.pub. But that was before an ssh update was available.
Was key creation fixed at this point, or are my new keys still potentially vulnerable? They
are not flagged by the now-available ssh-vulnkey, but that won't necessarily catch everything.
I also regenerated some SSL certificates, but since that is actually using the openssl
command-line interface it would obviously have been fixed by that point.

In any event, I need to figure out if I have to back and redo personal keys again.

Additionally, the new ssh packages don't give me an option to regenerate host keys, as the
email seemed allude to. I had to do that by hand.

What exactly is the price you allude to? Predictability of the random numbers? Some kind of
timing-related attack, because packet arrival times may affect the entropy pool input? A
kernel exploit that allows simultaneous compromise of all applications using the /dev/u?random
file?

How about a test that generates two keys in a row, within the same process, and makes sure
they are not identical to each other. If salt is involved, take that into account rather than
doing a bitwise comparison.

That seems like a pretty reasonable test at the library level, to ensure the key really is a
key and not a buffer full of zeros.