Too similar to be different

Posted by: aigarius 6 years, 3 months ago

(21 comments)

Eric, I cann't claim to 100% understand the situation but after glancing trough the logs of the discussions and of the patches the conclusion I came to was this - OpenSSL used supposed randomness of the uninitialized memory as an added source of entropy (interesting hack, but not an example of good coding as such). Valgring caught that problem and the Debian maintainer during a cleanup fixed it. Making such a fix can be considered a preventive step against possible attack vectors by poisoning the uninitialized memory. He took it up to upstream, they did not raise red flags, but did not quite merge the 'clean up' patch either. It fell through the cracks.

The problem is that in the same file, in another function all other sources of entropy were being merged into the pool of randomness using exactly the same code line as the one code line flagged by Valgrind. The maintainer assumed that the second code line has a similar function to the first and commented that one as well. AFAIK that also did not show up in the emails to the upstream list.

So we have:


  • Upstream using clever hacks that rely on uninitialized memory having some randomness to it

  • Upstream using same code and same variable names to describe different things

  • Upstream having no comments in the code explaining the two things above

  • Maintainer slightly over-generalizing a change

  • A bug slipping trough the cracks in the review processes

  • Another Debian Developer discovering the bug and recognizing its significance despite all of the above

  • Debian project coming out and admitting all of the above and scrambling to get fixes out to its users ASAP

I am impressed by the swift action of the people involved in fixing this. And while I think everyone can find some lesson be learned here, I think this is another good example of free software in action. And I hope that in the aftermath of this we will find ways to prevent this from happening in the future without stifling our progress.

Currently unrated


Comments

  • Rudi Cilibrasi, PhD 6 years, 3 months ago

    It is not upstream's fault except perhaps that they didn't comment the code. Their FAQ seems technically correct to me and it's for the simple reason that adding a constant or any value to an entropy pool does not reduce the entropy in that pool. It is more a question of miscounting. The entropy going into the pool has to be counted conservatively in an accumulator (e.g. compressed size / 2) and entropy should not be pulled out of the pool faster than it goes in. Since the memory was unitialized, it could only help by adding more entropy but the 'worst case' of constant would not be a big deal except for the fact that (as I understand it) the other source of entropy was accidentally sabotaged by the debian maintainer's erroneous patch at the same time. Either strike against the keyspace size alone would have been minor but together it brought it way too low.

    Link / Reply
  • A simple matter of… » dsa-1571 6 years, 3 months ago

    [...] or rather commented out a similar line which was rather [...]

    Link / Reply
  • bd_ 6 years, 3 months ago

    Actually, uninitialized memory is far from random, and would result in /over/estimation of the available entropy. eg, memory handed out by the kernel is always zeroed. Memory previously used by the application may not be zeroed, but it's likely to be predictable, especially in such one-shot applications such as ssh-keygen or the analogous openssl call for SSL certificates.

    Link / Reply
  • mariuz 6 years, 3 months ago

    I think debian policy should be changed and to follow the slackware one

    I mean that packages should be generic and with minimal patches ( like paths and some small typos) and major changes should be done upstream and submit the patches upstream first
    (if possible)

    Link / Reply
  • Snazz 6 years, 3 months ago

    Also, wouldn't this (assuming uninitialised memory is non-zero) only apply to previous machines with smaller amounts of memory.

    ie With only 512mb it's likely that most memory addresses would have been read to/written from/swapped about. This wouldn't apply to modern machines with multiple gigabytes of memory that are never touched.

    Link / Reply
  • Miriam Ruiz 6 years, 3 months ago

    mariuz, you cannot be serious about that.

    If you consider a distribution as something more than a big chaotic bunch of packages, you'll have to make some distro-specific patches to make the whole distribution consistent. In the Games Team we have to really patch some games a lot to make them just usable. We're far ahead of Slackware in any case, so why try to go back to the past? Most of the patches provided by Debian improve, stabilize and integrate the programs. The fact that there might be an error once in a while doesn't contradict that, and in fact we're correcting much more errors than the ones we might be creating, as unfortunate as they might be, like in this case.

    Greetings,
    Miry

    Link / Reply
  • btmorex 6 years, 3 months ago

    @Miriam Ruiz

    Well, if there's any event that should cause a change of policy, it's this one. The Debian maintainer applied an unnecessary patch that has essentially compromised the security of any Debian machine that was installed in the past 2 years. (on a side note, everyone seems to be downplaying the seriousness of this vulnerability. It's not some theoretic decrease in key strength. Literally, the only source of randomness going into the key generation was the pid. This should make brute force attacks possible in minutes on high speed networks)

    If the maintainer had sent a full patch upstream to try to get them to apply it, they would have realized the mistake immediately and none of this would have ever happened.

    Honestly, as a mere debian user, I'd like to see a real response to this other than self congratulation on how fast the issue was dealt with. People should be asking "how could this possibly happen?" and "what can we do in the future to prevent this from happening?" One answer is to adopt a more slackware-esque approach to packaging. There are other possibilities too... I'd just like to see it addressed in some way because I just wasted a day fixing servers and even now I can't be sure that none of them were compromised.

    Link / Reply
  • aigarius 6 years, 3 months ago

    @Rudi - the FAQ entry was written well after the change.

    @slackers - I love Debian exactly because Debian maintainers have the opportunity and obligation to bring packages in line with Debian policy so that I and every system administrator knows what to expect. The benefits of the Debian Policy are many and varied and heavily outweigh the risk of a bug being introduced once in a blue moon.

    Link / Reply
  • OdyX 6 years, 3 months ago

    @btmorex :

    "If the maintainer had sent a full patch upstream to try to get them to apply it, they would have realized the mistake immediately and none of this would have ever happened."

    Actually, the maintainer did [0]. And upstream (at least, some with @openssl.org email addresses...) did not rise the red flag [1].

    Actually, the fact that it has been discovered, corrected, published and pushed to the users is a very positive thing to me. Another way of doing it would have been to not say anything and silently push an upload. Debian'd have avoided all these reactions... But this contradicts the Social Contract.

    Every DD is firstly a human and has therefor "excuses" for errors. If upstream did not see the issue, why would a Debian-specific patch-review team have seen the issue ?

    Regards, OdyX

    [0] http://marc.info/?l=openssl-dev&m=114651085826293&w=2
    [1] http://marc.info/?l=openssl-dev&m=114652287210110&w=2

    Link / Reply
  • Michael 6 years, 3 months ago

    OdyX: That technically doesn't count as a patch per the OpenSSL guidelines, so I could see it falling through the cracks. I could also see them thinking it's just a random guy wanting to run Valgrind on his application, isn't going to publish his changes to anyone, and isn't going to use the results of the run.

    Of course, it's possible that even if a real honest-to-goodness patch was submitted, at least one OpenSSL dev would still be hiding behind the "he sent it to the wrong list! It's supposed to go to a list whose only mention on our website and documentation is in a patch for IPv6 support! That's why we allowed it to go through!" argument.

    Link / Reply
  • Michael 6 years, 3 months ago

    Damn, forgot to mention. Both lines that were commented out in the original patch were mentioned in the original posting to the mailing list.

    Link / Reply
  • btmorex 6 years, 3 months ago

    @OdyX

    I saw the openssl-dev thread. It's not really what I would consider sending a patch upstream. In fact, the email doesn't even contain a patch just a couple line numbers and a few lines of code. More importantly, the context of the email is "is it ok to remove the use of this uninitialized buffer" rather than "openssl should be fixed so valgrind doesn't complain". Obviously, the openssl people who responded did not actually look at the file and specific change but rather responded to the general inquiry. I *do* think they would have noticed the problem if the issue was submitted as a patch.

    I guess the ultimate point is if debian developers think there's a bug in some piece of software, they should attempt to get it fixed upstream rather than applying patches locally.

    Link / Reply
  • Joe Buck 6 years, 3 months ago

    The premise of your post was incorrect. The patch in question made two changes. One got rid of a use of uninitialized memory. <i>This was not the problem</i>. The other hunk in the patch was the problem. OpenSSL was not relying on uninitialized memory for entropy.

    Link / Reply
  • The Changelog 6 years, 3 months ago

    <strong>Thoughtfulness on the OpenSSL bug...</strong>

    By now, I'm sure you all have read about the OpenSSL bug discovered in Debian.

    There's a lot being written about it. There's a lot of misinformation floating about, too. First thing to do is read this post, which should clear up some of that.
    ...

    Link / Reply
  • Rudi Cilibrasi, PhD 6 years, 3 months ago

    As I tried to explain, the logic in this entry was flawed. See correct logic at the following URL:

    http://www.links.org/?p=328

    Link / Reply
  • ciol 6 years, 3 months ago

    "It doesn't matter how often you do it: freezing thousands of packages into a stasis just doesn't work." tuomov, 03/03/2007. He was right.

    As a /. user says "who cares how "integrated" a program is, if it could have had arbitrary bugs silently introduced?"

    Debian's packages are /too/ well integrated.

    If -stable was smaller, maybe this would have not happened.

    Link / Reply
  • Erich 6 years, 3 months ago

    Hi,
    Just a quick followup. The main location where this warning of using "uninitialized memory" does indeed deliberately use uninitialized memory. The function is expected to fill a buffer with random data. It first uses the current memory contents to further randomize (maybe, if that data is random), then fills the buffer with randomness.
    This use of the existing contents is definitely safe and doesn't do any harm (except causing this warning, which sparked all this mess, actually...)

    The big mistake the Debian maintainer did was to also remove another *literate* copy of that line in the OpenSSL source, which happened to be used to feed randomness into the RNG, thus making all OpenSSL efforts to obtain more randomness moot.

    The reason why I believe OpenSSL upstream is also to blame is because they didn't care about other users wanting to use valgrind and not see their "deliberate" error reported again and again. This caused the Debian maintainer to fix the issue himself, introducing the mistake. If OpenSSL had cared about developers wanting to use valgrind, it wouldn't have happened in the first place.

    Link / Reply
  • Mac 6 years, 3 months ago

    Very well done article on a not so easy topic. It's ok if there are folks out there who disagree. That is their 'bug' to fix not mine. ;-) I use Debian today. No changes are planned.

    Link / Reply
  • Jon 6 years, 3 months ago

    I honestly cannot see the correlation between patching things for integration or the size of stable on this problem.

    The patches against ssh should have been reviewed by more than one person in the last few years, yes, there have definitely been errors here. But I don't see how this extrapolates to the entire archive.

    That sounds like slashdot sound-byte nonsense.

    Link / Reply
  • ciol 6 years, 3 months ago

    I honestly cannot see the correlation between patching things for integration or the /size of stable/ on this problem.

    The patches against ssh should have been reviewed by /more than one person/ in the last few year

    ...

    Link / Reply
  • Lanux &raquo; Vulnerabilidad de OpenSSL en Debian 6 years, 3 months ago

    [...] dejo mis links [...]

    Link / Reply

New Comment

required
required (not published)
optional

Recent Posts

Archive

2014
2013
2012
2011
2010
2009
2008
2007
2006
2005

Categories

Authors

Feeds

RSS / Atom