Hacker News new | ask | show | jobs
by SAI_Peregrinus 395 days ago
On the other hand it exposed that OpenSSL was depending on Undefined Behavior always working predictably. Something as simple as a GCC update could have had the same effect across far more systems than just Debian, with no patch to OpenSSL itself.
2 comments

> On the other hand it exposed that OpenSSL was depending on Undefined Behavior always working predictably. Something as simple as a GCC update could have had the same effect across far more systems than just Debian, with no patch to OpenSSL itself.

No it wasn't. It was reading (and xoring into the randomness that would become the key being generated) uninitialised char values from an array whose address was taken, that results in unspecified values not undefined behaviour.

I see you're correct, I misremembered. That isn't really much better, since there's no requirement that unspecified values ever actually change. Compiler developers are free to always return `0x00` when reading any unspecified `char` value, which wouldn't provide any entropy. XORing it in guaranteed that it couldn't subtract entropy, but if there were no other entropy sources they failed to return an error. OpenSSL being able to generate 0 entropy and not return an error in its RNG was still an important bug to fix.
> XORing it in guaranteed that it couldn't subtract entropy, but if there were no other entropy sources they failed to return an error.

No, they XORed data from a bunch of entropy sources into an intermediate buffer (that was never initialised, because the whole point of it was to be random) and then XORed that into a buffer from which the key was made. Debian's patch removed that final XOR. It wasn't a bug in the original code (other than being hard to understand).

Current kernels zero pages. The code was buggy to begin with.
No, there was no bug. The code used an uninitialised array as a place to XOR randomness from various sources into. It didn't initialise the array since there was no value in doing so, because the whole point is to be random anyway.
As I said, new kernels zero the pages, an uninitialized area is likely to be zeroed or be rather deterministic.
Which is fine. It's not a bug if it's zero. There's just no point going out of your way to set it to zero when you're only going to xor a bunch of random values into it anyway.
The whole issue was caused because it was always zero because the patch zeroed it (:

Can you please read about the issue before commenting more?

The crazy thing is that after this incident they restored the uninitialized usage and retained it there for the next half decade. It wasn't as mild as being a risk of future compilers destroying the universe: it made valgrind much less useful on essentially all users of OpenSSL, exactly what you want for security critical software.

(meanwhile, long before this incident fedora just compiled openssl with -DPURIFY which disabled the bad behavior in a safe and correct way).