Debian needs some serious commit review

~ Nate Lawson

You’ve probably heard by now about the gaping hole in keys generated by Debian’s OpenSSL. If not, the summary is that your SSH keys and SSL certs were selected from a fixed pool of 215 (32,767) possibilities, and are thus easy to brute-force over the network. If you have any keys generated on a Debian system, you need to immediately replace them or disable the associated service. It’s that bad — remote login or root with only a few thousand tries.

Luckily, Debian recently fixed this 2-year-old hole in this commit. Great, right? Except, I made a quick comparison to the commit that introduced the bug, which shows they missed reverting both places the bug was added. So they still didn’t fix it completely.

As a past FreeBSD committer and crypto engineer, I knew any commits to the PRNG or other such critical code were subject to intense review. That’s before they could be committed. If a committer were found to have introduced such a fatal flaw, the patch to fix it would have been doubly-scrutinized before being allowed into the tree. Apparently, the same guy who introduced the bug was left to screw up the fix.

Once more, this time with prior review!

Edit: a commenter informed me that there was review of this fix, and Debian decided to leave their implementation silently incompatible with the OpenSSL API docs.

“If a Microsoft developer commented out seeding in Vista CryptGenRandom(), they would be fired 12 times. Then Microsoft would buy the next company that hired them in order to fire them again.”
— Thomas Ptacek

22 thoughts on “Debian needs some serious commit review”

  1. Not exactly.
    There was initially a well-discussed one-line fix zealously extended by another line fix, the real Big Bang bug.
    So Debian maintainers decided to keep the first intentional fix and just remove the big bug.

    FYI the intention was to remove the addition of uninitialized memory to the entropy pool because that doesn’t provide a tremendous amount of good randomness while providing a hell of troubles for the application developers using the openssl library and e.g. Valgrind.

    The problem is that the initial fix went a bit too far by removing all the other sources of entropy as well (except the PID) so we’re now back to square zero.

    Phil

  2. Philippe, thanks for commenting. However, I still think the code should be restored to its former state.

    The RAND_bytes() man page says that “The contents of buf is mixed into the entropy pool before retrieving the new pseudo-random bytes unless disabled at compile time (see FAQ).” So on Debian, applications that use the RAND_bytes function to (re-) seed the pool will be insecure. Have you audited all callers of this API?

    With regard to the Purify warning, the OpenSSL FAQ says that the proper way to get rid of the warning is to define the PURIFY compile flag. If you reverted the rest of the buggy commit, you’d restore that documented behavior.

    P.S. It’s good to hear this was at least discussed and reviewed, although I disagree with the outcome of the discussion.

  3. The fix was a bad one anyway. The code that made valgrind complained was already protected with #ifdef PURIFY, he could have just defined it at configure time if he really cared. No code changes needed.

  5. While that quote is great. Debian is free so deal with it. I for one reap lots of benefits form what debian brought to linux. AS ALMOST EVERYONE DOES. Deal with it or pay the developers.

  7. That quote is good, but your own analysis is sadly lacking. The fix Debian committed is solid, but slightly inelegant (they should’ve just defined PURIFY like the source code says.)

    Read what you quoted:

    “The contents of buf is mixed into the entropy pool before retrieving the new pseudo-random bytes unless disabled at compile time (see FAQ)”

    “Unless disabled at compile time” is the key phrase here, and that is just what Debian did. It’s explicitly covered by the documentation.

  8. When objective evaluation is done of the open source operating systems out there, linux users look like a cult.

  9. You want people to use your OS with that attitude? Yeah, I’m sure the companies which *rely* on security would be pleased with this bullshit excuse for a secure shell on Debian.

  10. This bug doesn’t matter. It is fixed or will be fixed and Debian is still far more secure than Windows.

  11. this bug clearly matters, and its handling puts debian security FAR BELOW any Microsoft product. Our company is moving to Redhat based on this incident. Very bad for debian, anyone would be well advised to disassociate their reputations from these losers.

  13. As an admin, I’ve seen arguably worse things on Windows Lobby inc.
    Remember what a “;” in the URL was doing to IIS ? Or how stability is always an issue on MS servers ? A stability problem is the worse ending of a security breach in most cases…

    Definitely not a fatal argument, sorry.

  14. “If a Microsoft developer commented out seeding in Vista CryptGenRandom(), they would be fired 12 times. Then Microsoft would buy the next company that hired them in order to fire them again.”
    – Thomas Ptacek

    Really? Well, then, Tom, I guess you won’t mind producing the code that proves that they haven’t.

    I’ll wait.

    *crickets*

  15. FlashLV,

    I guess you could always script something to attempt a brute force of a windows logon. If you do it in less than 32,767 attempts then you might have a valid point.

    *crickets*

  16. Um…no, I have a valid point without performing that exercise.

    Prove that the Vista code hasn’t been compromised by a comment.

    I’ll wait.

    *crickets*

  17. Saying that the fix isn’t complete is slightly misleading and shows he didn’t quite read enough on the issue. The line that wasn’t uncommented is the one between #ifdef PURIFY, as such, the code is already documented (by upstream) to work without it.

  18. We all know it’s easy for Microsoft to stand at the top of the walled fortress and throw off-hand remarks out due to their closed nature, and if this was Microsoft instead of Debian, well no-one would even know.

    However, if you are a security focussed company in the Free Software world, you choose Suse or Red Hat or another enterprise-y distribution. Hell, use QNX. People are very quick to say “Oh, you used Free software, it’s not as good as money you paid for.”, you pay for Linux. It’s a profitable operating system, you just don’t HAVE to.

    Just don’t run a high security network on a consumer operating system.

  19. This is nothing new.
    I Switched away from Debian years ago because of the crappy patches their “developers” were allowed to apply to packages.

    Actually Ubuntu has helped the situation a lot as they pay competent coders to clean up the Debian tree, at least the important packages are mostly fine now.

    @sheesh
    As for Vista: it still stores passwords as an unsalted hash. Which yes, makes it possible to reverse them in minutes. Look into ophcrack. This has little to do with cryptographic key quality though.

  20. I was surprised to see this post get so many comments months after it was published. Looks like Linux Haters brought in the hordes. I had to delete a few comments that were racist. Please try to add value with what you write here, don’t just insult others.

    With respect to the defense that “disabling seeding is documented behavior”, I think this is a terrible design. The only valid reason for this is some kind of test mode that should never be used on a live system. Thus, my recommendation was that enabling this test mode should result in a giant #warning at compile time and printf(“WARNING…”) at runtime.

Comments are closed.