It’s probably worth noting at this point that there are a few lessons to the debian OpenSSL débâcle:
- There is now a corollary to “do not write your own cryptographic routines”: “do not fix bugs in someone else’s cryptographic routines.” If there is a annotated view of the OpenSSL tree (I don’t know/don’t care), the DD who patched OpenSSL would have been better off contacting the person who wrote the offending line in the original source than trying to find the correct channel.
- Developers must publish correct information on how to contact them. Incorrect information on the OpenSSL website maintenance is just as much to blame for this as the DD in question, who did ask the suggested channels about his patch.
- Distros should have peer review of patches in security-critical code — by experienced developers — if they do not already.
- Rather than all the bitching, remember that the central tenet of F/LOSS is Fix It Yourself. This does not cease to apply simply because the problem exists in something you depend on. If anything, it should emphasize how necessary it is.
It would also have helped a lot if the openssl code was properly commented. If it would have been obvious that entropy from a random number generator is being added here, then no one would have considered to remove that line. But it looks like not even the most important parts of the code are documented.
Another thing that is obviously missing from the openssl code base are unit tests. It’s not rocket science to generate a small amount of keys and to check their distribution in the key space. If such a test would have existed and if it would have been run as part of ‘make check’, then this disaster would not have happened.
No doubt, but given that the OpenSSL code is just whack to begin with (I mean seriously, have you ever tried using the OpenSSL command line without finding some recipe for what you want to do), that’s not really reasonable to expect them to follow good practices WRT code and testing.
AFAIK, you cannot reasonably depend on the non-standard, undocumented behavior of having uninitialized memory be something random. Any platform that zeros uninitialized memory — e.g. as part of some app-isolating “secure virtual memory” — would also exhibit the same behavior.
Please do not confuse things here. The patch in question was removing two lines. One was the use of uninitialised memory, which is indeed a very questionable and unreliable source of entropy. The other line in the patch looked almost identical, but it was the call where the result of the random number generator was added as entropy source. Removing that call is what caused the problem.