Links

Ben Laurie blathering


Debian and OpenSSL: The Aftermath

There have been an astonishing number of comments on my post about the Debian OpenSSL debacle, clearly this is a subject people have strong feelings about. But there are some points raised that need addressing, so here we go.

Firstly, many, many people seem to think that I am opposed to removing the use of uninitialised memory. I am not. As has been pointed out, this leads to undefined behaviour – and whilst that’s probably not a real issue given the current state of compiler technology, I can certainly believe in a future where compilers are clever enough to work out that on some calls the memory is not initialised and take action that might be unfortunate. I would also note in passing that my copy of K&R (second edition) does not discuss this issue, and ISO/IEC 9899, which some have quoted in support, rather post-dates the code in OpenSSL. To be clear, I am now in favour of addressing this issue correctly.

And this leads me to the second point. Many people seem to be confused about what change was actually made. There were, in fact, two changes. The first concerned a function called ssleay_rand_add(). As a developer using OpenSSL you would never call this function directly, but it is usually (unless a custom PRNG has been substituted, as happens in FIPS mode, for example) called indirectly via RAND_add(). This call is the only way entropy can be added to the PRNG’s pool. OpenSSL calls RAND_add() on buffers that may not have been initialised in a couple of places, and this is the cause of the valgrind warnings. However, rather than fix the calls to RAND_add(), the Debian maintainer instead removed the code that added the buffer handed to ssleay_rand_add() to the pool. This meant that the pool ended up with essentially no entropy. Clearly this was a very bad idea.

The second change was in ssleay_rand_bytes(), a function that extracts randomness from the pool into a buffer. Again, applications would access this via RAND_bytes() rather than directly. In this function, the contents of the buffer before it is filled are added to the pool. Once more, this could be uninitialised. The Debian developer also removed this call, and that is fine.

The third point: several people have come to the conclusion that OpenSSL relies on uninitialised memory for entropy. This is not so. OpenSSL gets its entropy from a variety of platform-dependent sources. Uninitialised memory is merely a bonus source of potential entropy, and is not counted as “real” entropy.

Fourthly, I said in my original post that if the Debian maintainer had asked the developers, then we would have advised against such a change. About 50% of the comments on my post point to this conversation on the openssl-dev mailing list. In this thread, the Debian maintainer states his intention to remove for debugging purposes a couple of lines that are “adding an unintialiased buffer to the pool”. In fact, the first line he quotes is the first one I described above, i.e. the only route to adding anything to the pool. Two OpenSSL developers responded, the first saying “use -DPURIFY” and the second saying “if it helps with debugging, I’m in favor of removing them”. Had they been inspired to check carefully what these lines of code actually were, rather than believing the description, then they would, indeed, have noticed the problem and said something, I am sure. But their response can hardly be taken as unconditional endorsement of the change.

Fifthly, I said that openssl-dev was not the way to ensure you had the attention of the OpenSSL team. Many have pointed out that the website says it is the place to discuss the development of OpenSSL, and this is true, it is what it says. But it is wrong. The reality is that the list is used to discuss application development questions and is not reliably read by the development team.

Sixthly, my objection to the fix Debian put in place has been misunderstood. The issue is not that they did not fully reverse their previous patch – as I say above, the second removal is actually fine. My issue is that it was committed to a public repository five days before an advisory was issued. Only a single attacker has to notice that and realise its import in order to start exploiting vulnerable systems – and I will be surprised if that has not happened.

I think that’s about enough clarification. The question is: what should we do to avoid this happening again? Firstly, if package maintainers think they are fixing a bug, then they should try to get it fixed upstream, not fix it locally. Had that been done in this case, there is no doubt none of this would have happened. Secondly, it seems clear that we (the OpenSSL team) need to find a way that people can reliably communicate with us in these kinds of cases.

The problem with the second is that there are a lot of people who think we should assist them, and OpenSSL is spectacularly underfunded compared to most other open source projects of its importance. No-one that I am aware of is paid by their employer to work full-time on it. Despite the widespread use of OpenSSL, almost no-one funds development on it. And, indeed, many commercial companies who absolutely depend on it refuse to even acknowledge publicly that they use it, despite the requirements of the licence, let alone contribute towards it in any way.

I welcome any suggestions to improve this situation.

Incidentally, some of the comments are not exactly what I would consider appropriate, and there’s a lot of repetition. I moderate comments on my blog, but only to remove spam (and the occasional cockup, such as people posting twice, not realising they are being moderated). I do not censor the comments, so don’t blame me for their content!

56 Comments

  1. Fifthly, I said that openssl-dev was not the way to ensure you had the attention of the OpenSSL team. Many have pointed out that the website says it is the place to discuss the development of OpenSSL, and this is true, it is what it says. But it is wrong. The reality is that the list is used to discuss application development questions and is not reliably read by the development team.

    Clearly, it is Debian’s fault that they relied upon your documented channels of communication, and the onus is on them to fix it.

    Comment by Branden Robinson — 14 May 2008 @ 10:20

  2. If anyone is interested in my exercise in identifying openssl-team as the correct contact address using only the OpenSSL sources and website—as a Debian developer who has just taken responsibility for the package might do—they can peruse my findings at Advogato.

    Comment by Branden Robinson — 14 May 2008 @ 10:23

  3. First and most important suggestion:
    Fix your website and README to report correctly what users should do to contact the openssl developers. Both currently state that openssl-dev is the right way, but you say this isn’t true. If you are right, fix the docs.

    Also, I would like to remind that there was one important hint in Kurt’s mail that he was unsure wether the patch would be OK (between the ***):

    > What I currently see as best option is to actually comment out
    > those 2 lines of code. ***But I have no idea what effect this
    > really has on the RNG.*** The only effect I see is that the pool
    > might receive less entropy.

    regards,
    sven

    Comment by Sven Mueller — 14 May 2008 @ 10:25

  4. Hi, as far as the repetition goes,
    sorry for my part in that. I know
    when I wrote, the last two posts
    showing were by you.

    And now there’s over forty posts
    inbetween. I think that the numbering
    of the posts even changed.

    I’d not have added my two bits if I
    had known that a truckload of the same
    two bits were already en route.

    J.

    Comment by j — 14 May 2008 @ 10:35

  5. Seems to me the solution would be to have a test that somehow measures the randomness of the RNG, and fails if it is inadequate, if it repeats. This would not only catch mistakes made by downstream developers (assuming they run the tests), it would catch compiler-derived problems and even mistakes by the original team.

    Comment by Steve Loughran — 14 May 2008 @ 13:18

  6. You complain about lack of funding, but the project website (openssl.org) doesn’t appear to have a donation link or any details on how anyone can contribute financially.

    Comment by AllenJB — 14 May 2008 @ 15:05

  7. If there’s one way to get an army of screaming trolls to show up anywhere, it is to criticize Debian. The tsunami of near-identical posts defending the Debian maintainer in this entire episode is actually kind of scary. About 25% of the posts pointed out the same entry in the openssl-dev mailinglist archive and another 25% kept hammering on the ‘but your website said openssl-dev’ angle. Why did so many people feel the need to jump on these same two points without reading the dozens of earlier comments with exactly the same links making exactly the same argument? I haven’t seen that much zeal since the death of Guy Kawasaki’s ‘evange-list’ of foaming-at-the-mouth apple fanboy ninjas.

    You can keep hammering on the point about how utterly and completely wrong the openssl team has handled this and how noble and shiny the behavior of the debian maintainer has been, but I think this is hardly constructive. It is much more worthwhile to take a look at the context of this situation and the underlying problems. The chronic underfunding of the OpenSSL team is not unique in the open source world and there are about as many methods for keeping up communication channels between the developers and the community as there are open source projects. This sort of problem is bound to come up again and again if all you can do is insist that some open source projects are bad at community building so neener, neener. What the Debian community should rather do is find a process that makes these kinds of miscommunication-related mistakes less likely to happen.

    I can’t cast judgement on this specific situation, but I would note that it is odd that the Debian maintainer of openssl would rely only on information found on the project’s public website to figure out how to communicate. The Debian habit of keeping private patches is one that at least puts _some_ burden on their shoulders where it relates to getting on friendly ground with the developers of the products they are effectively forking and figuring out what their prevailing culture is to, you know, actually co-operate. From that perspective it should not have been hard to surmise that the openssl-dev list was probably poisoned by too much end user noise and forced the core developers to a more private list (that they probably didn’t mention on their homepage to prevent it from also being turned into a de facto developer helpdesk).

    My advice is to focus on this aspect, to think of a process for package maintainers that makes it more likely they will be able to effectively commuicate with all kinds open source projects, regardless of how much their culture looks like that of Debian.

    Please note that I’m not saying the openssl team was without faults here. But they’re actually grown up enough to see the problems and think about solutions.

    Comment by Pim — 14 May 2008 @ 15:51

  8. Some other widely redistributed free software projects have mailing lists for use specifically for communication among developers and distributors. So the list would be populated by members (some, anyway) of the OpenSSL development team, as well as the Debian, Ubuntu, Fedora, etc package maintainers. This may work well here. As this whole situation has shown, OpenSSL is critical and something that really needs to be done right…

    Comment by Noah — 14 May 2008 @ 15:56

  9. Regarding “there’s a lot of repetition” – I thought I was posting comment #45 (right after your comments #43 and #44), but because of the moderation delay, I ended up posting comment #97. Obviously if I’d seen any of the comments that really did appear after your #44 I wouldn’t have posted.

    Comment by Julius Davies — 14 May 2008 @ 16:44

  10. > Clearly, it is Debian’s fault that they relied upon your documented channels of
    > communication, and the onus is on them to fix it.

    It can hardly be said that there was an endorsement of any kind of the patch.

    But I guess the important part for Debian is to find a way to escape the blame for this fuck-up.

    Comment by niklas — 14 May 2008 @ 16:45

  11. Perhaps the reason why there were so many posts pointed that pointed out is because the post blamed Debian for something that was clearly at least partially the poster’s fault, and not any of Debian’s.

    Debian has accepted blame for having a bug in their patch that fixed a bug in OpenSSL. They did communicate with the developers, on the open mailing list designated for that purpose. If you want people to work with you more, then you have two options; use a license that forces it, or actually make it possible and useful for them to communicate with you. As it is, there’s no reason for them to jump through hoops to communicate with you.

    Comment by Prosfilaes — 14 May 2008 @ 18:22

  12. In my opinion some very important point regarding the mailing-list posting is missing: Kurt did not mentioned he writes the mail as debian maintainer. He appeared as some individual that has some debugging problems.

    So the OpenSSL devs didn’t even get the chance to realize how criticial the change was.

    Comment by Euphi — 14 May 2008 @ 18:39

  13. Perhaps joining ocert would help. Certainly helping fix communication problems between distros and downstream developers is part of their goals.

    Comment by mock — 14 May 2008 @ 19:12

  14. As a Debian user myself (since 2000), I was more than a bit annoyed by this flaw. This mess is a clear indication that package maintainers should not try to fix bugs in critical packages (such as OpenSSL) but get them fixed upstream.

    Comment by Christophe Devine — 14 May 2008 @ 19:23

  15. Neither OpenSSL nor Debian should escape blame.

    Comment by John — 14 May 2008 @ 19:47

  16. [...] paar bissige Kommentare von einem OpenSSL Entwickler [...]

    Pingback by Debian SSL Super-GAU at blog.gauner.org — 14 May 2008 @ 20:22

  17. @niklas: I don’t think too many people are trying to shift the blame here (not from Debian itself, anyway – if you look at the communication from the original advisory on it’s made very clear that it was a Debian-local change). If you look at Branden’s comments he’s specifically talking about the complaint about not sending stuff to -team. The main issue people are raising on that point is that it’s not entirely reasonable to criticize Kurt so strongly given how poorly advertised the appropriate contact information is. Fair cop on the patch itself but complaining so strongly that the communication wasn’t addressed to -team isn’t quite so reasonable – it looks like Kurt made an honest effort to try to talk to the developers.

    Comment by Mark Brown — 14 May 2008 @ 20:48

  18. I have to agree that the mailing list comments don’t constitute an endorsement of the patch. The Debian folks need to realize that it is their job to make sure their patches work and make sure they do what they think they do. It is not the job of the original developer to do that. If you want the original developer to endorse the patch, submit it and see what they say. If it’s correct, they will incorporate it into their software. If not, they won’t. Perhaps they will have another solution to the problem or perhaps they wont. Either way, any code that is not from the original author of the package is inherently not endorsed by the original developer. Which was the point of the previous post.

    But congrats to the commenters from steering this discussion away from the correctness of the patch and it’s originators responsibility and accountability, and towards a discussion of all the ways in which the OpenSSL folks do things wrong, be they documentation or community relations or something else. Shame on them for providing us all high quality cryptographic software, free of charge, with zero funding. Software which is quite complicated in many ways. And to think they didn’t also make sure the software is not 100% bug free. The nerve of some people. I want my $0 back.

    Comment by Steve — 14 May 2008 @ 20:55

  19. Blame? Everyone fucks up at security. Debian doesn’t even have any liability for it. Who cares about escaping blame, just get shit fixed!

    Comment by hatt — 14 May 2008 @ 21:41

  20. [...] in to the Debian packagers for fixing an apparent bug locally, and not sharing it with upstream. In a second post, Ben clarifies some confusing issues, like whether OpenSSL is relying on uninitialised memory for [...]

    Pingback by Lack of Entropy « niq’s soapbox — 14 May 2008 @ 21:47

  21. First, not being associated with Debian, it’s amazing the amount of anti-Debian sentiment out there.
    Second, repetition of details were due to moderation lag. I’d like to believe, most never saw comments above. Either that, or all Debian people are morons.
    Third, the reason a lot of people relate to Kurt, is because there are a lot of us little grubs at the bottom of the pile running around trying to fix crap that’s dumped on us. We read the READMEs and follow them. And we hope the upstreams get off their high horse once in a while to help us out. In this case the horse is too high apparently for Ben to see that everyone fucked up, _including_ his own team.
    Four, this whole shamble of devs supposedly not monitoring the -dev mailing list makes me _VERY_ uncomfortable about OpenSSL .. what else did you guys miss? Is this how you support other (read: smaller) distributions?
    And lastly, I don’t think I saw one post from Debian people denying their fuckup .. I have yet to see one from the OpenSSL team acknowledging their part.

    Sometime it’s interesting to be on the sidelines :)

    Comment by adrian — 14 May 2008 @ 22:18

  22. niklas: Now it appears that currently the important part for _OpenSSL_ is to find a way to escape the blame for _their_ fuck-up. They failed to publish the correct contact address for such important questions regarding OpenSSL. Branden has documented in his link mentioned by him above that the mail address mentioned by Ben is not documented _anywhere_. It is OpenSSLs responsibility that they allowed the misuse of openssl-dev for offtopic questions and then silently moving the dev stuff to a secret other list nobody outside OpenSSL knew about.

    I’m sure debian is willing to take their fair share of the blame if OpenSSL finally admits that their mistake played a major role here as well. After all the debian maintainer might have misrepresented the nature of his plans, but he gave warning signs and said he was unsure. But as it appears now all the people who might have noticed secretly left openssl-dev, the documented place for that kind of questions. This is hardly the fault of the maintainer.

    Comment by hans — 14 May 2008 @ 22:21

  23. > Two OpenSSL developers responded, the first saying “use -DPURIFY” and the second saying “if it helps
    > with debugging, I’m in favor of removing them”.
    Of course, the Debian maintainer asked the wrong question, for debugging purposes both replies are feasible

    > I said that openssl-dev was not the way to ensure you had the attention of the OpenSSL team.
    I think this is a second issue, for such an important library, there should be a mailing list where package maintainers could ask the upstream developers.

    Comment by gms — 14 May 2008 @ 22:33

  24. [...] ihm recht in diesen Fall. Danach schreibt er noch ein zweiten Artilkel, zu diesem Thema, ” Debian and Open SSL: The Aftermath“wo er eine Antwort, auf die Post im ersten Artikel, [...]

    Pingback by Razvan Munteanu`s Blog » Blog Archive » Kryptographische Schlüssel in Debian — 14 May 2008 @ 22:43

  25. Is acessing uninitialized memory undefined behavior, or just the contents are undefined? Also, I would hardly believe acessing uninitialized *buffer contents* (as opposed to a variable) as having undefined behavior. I think a compiler fscking that up would be in sin.

    Comment by pux — 14 May 2008 @ 23:01

  26. I think the most important issue now is to analyze what has happened, how it could have happened, what the impact is and more importantly what can be done to prevent such a thing from ever happening again.

    This bug will soon be exploited, not only technically, but socially: surely closed source vendors will use it. It’s better to stand up, accept the blame and offer reasonable strategies for the future. It does not help to resort to the usual flame wars or to go along “but closed source would have been even worse!!!”.

    Comment by Claus-Christoph Küthe — 14 May 2008 @ 23:51

  27. why this big regression does not found by something like ‘make test’ or ‘make check’ ?

    Comment by u — 15 May 2008 @ 1:26

  28. niklas @10: Oh, come on — it’s a bit absurdist to equate “people who are posting pro-Debian comments” with “Debian”. I would be quite certain that, even though there are plenty of comment-posters who seem to be finding it important to “demonstrate” that Debian isn’t to blame, the actual Debian maintainers are taking a hard and much more reasonable look at what of their processes they ought to fix.

    Comment by Brooks Moses — 15 May 2008 @ 2:16

  29. Valgrind provides a client API that lets applications or libraries provide it with additional information.

    If you want to continue using uninitialised buffers in OpenSSL, please consider adding some VALGRIND_MAKE_MEM_DEFINED() or VALGRIND_MAKE_MEM_DEFINED_IF_ADDRESSABLE() calls if Valgrind is available. If the call isn’t considered too expensive w.r.t. the other work being done it would be good to leave turned on in release builds.

    As well as removing the noise from Valgrind reports when run on OpenSSL or applications using OpenSSL, it would also help document your intent.

    Comment by James Henstridge — 15 May 2008 @ 3:28

  30. I have a worrying suspicion that for a lot of people this might all be too late.


    Sixthly, … My issue is that it was committed to a public repository five days before an advisory was issued. Only a single attacker has to notice that and realise its import in order to start exploiting vulnerable systems – and I will be surprised if that has not happened.

    And I just saw this today.

    http://www.informationweek.com/news/security/attacks/showArticle.jhtml?articleID=207603339
    http://stats.denyhosts.net/stats.html

    Notice the big jump in SSH attack attempts on May 11/12, before the announcement on May 13?

    Looks like the attackers are watching OpenSSL commits to me…

    Comment by Rob Mueller — 15 May 2008 @ 8:22

  31. hans (#22): It is the fault of the maintainer, because he never wrote that he es debian maintainer *AND* plans to remove the two lines from official distribution.

    Furthermore, he ignors concerns in debian’s bugtracker and checks in the change silently!!

    Comment by Euphi — 15 May 2008 @ 10:29

  32. [...] This comment puts it a little strongly, but is generally on the money in this regard: [...]

    Pingback by taint.org: Justin Mason’s Weblog » Serious Debian/Ubuntu openssl/openssh bug found — 15 May 2008 @ 15:26

  33. See 6.2.6.1 of C99 standard. Accessing uninitialized memory as a character type doesn’t cause undefined behaviour.

    There was never any patch posted to openssl-dev. Neither was it explained what the actual purpose of those described changes was.

    Instead he should have sent a patch generated with “diff -u” and some short descriptive comment like “I found a bug in OpenSSL with valgrind. Apparently it’s using uninitialized memory.”

    The lengthy prose blah blah and absence of the actual patch is a major communication mistake. An end user might not understand this but any maintainer and certainly any developer should.

    Don’t you think the “bug fix” was kind of bizarre anyway? The perceived “bug” clearly wasn’t a typo and it doesn’t look as if he even tried to follow the code path.

    Comment by Chris — 15 May 2008 @ 21:15

  34. [...] SSH Keys Generated on Debian/Ubuntu Compromised Debian OpenSSL Predictable PRNG Toys Debian and OpenSSL: The Aftermath [...]

    Pingback by Tiempo finito y logarítmico » /* purify complains */ — 16 May 2008 @ 2:03

  35. [...] to guess the next bit”, which has about zero implications for real world security), but as Ben Laurie explains, the Debian package maintainer apparently noticed that OpenSSL used uninitialized memory [...]

    Pingback by Thomas Themel's Wannabe Everything - The Debian SSH Fiasco — 16 May 2008 @ 7:10

  36. [...] of the OpenSSL developers has commented on the recent Debian OpenSSL [...]

    Pingback by sploit.dk » Details about Debian OpenSSL bug — 16 May 2008 @ 7:59

  37. Since you ask for suggestions…

    This problem made clear that lot of things could be improved on the vendor’s side. But that’s not what you’re asking for (I’ll send those suggestion where appropriate).

    Anyway, here are three things that openssl developers could be doing, and each would probably have been enough to avoid this nightmare :
    1- Please, subscribe to vendor’s bugtrackers openssl components, as much as you can. This Debian “patch” was discussed on Debian BTS ( http://bugs.debian.org/cgi-bin/bugreport.cgi?bug=363516 ).
    2- Please, as far as you can, subscribe to mails generated by vendor’s uploading patches on their openssl packages (or OpenBSD’s , NetBSD’s etc. cvs repositories).
    3- When a matter like “openssl and valgrind” come so often, and/or when you do something very special in the code (voluntary reading of uninitialized memory is not that usual), this may deserve one line of inline comment (and maybe sometimes an entry in the FAQ).

    Well, those suggestions are limited by what you said above (lack of developer time).

    Comment by benpi — 16 May 2008 @ 11:41

  38. benpi (#37), it is certainly a good idea to monitor lots of places for problem reports about your software. You shouldn’t have to though. A distributor who doesn’t inform upstream developers about modifications and who doesn’t forward problem reports and such to upstream, is simply doing an incredibly bad job. I know very well that many package maintainers of such software distributions are guilty of exactly this. Debian never forwarded any reports or requests to my official SourceForge hosted bug tracker. I only found them by incident. I’m not a Debian users, why should I know or have to know where and how Debian handles reports? There are dozens distributors and most of them behave the same, except for high-profile packages maybe.

    I can certainly sympathise with “vendors are bad [for security]” to a certain degree because more often than not, they act as blackholes and separate developers from users instead of acting as gateway between them as they should. Of course in case of a library it’s much harder to communicate with the users than it is for end user software with a user-interface which can easily tell users where they can get competent help and report problems.

    Oh and by the way, as much as I appreciate patches, I’ve rarely seen one which is directly acceptable without modifications. Someone who isn’t part of the developer team has to be either extremely competent (objectively not self-assumed) or lucky if he’s able to write better code than the team itself. More often than not outsiders can only identify problems but not the actual root cause and typically just treat the symptoms.

    This doesn’t mean there aren’t ignorant, arrogant upstream developers but that’s not the point.

    Comment by Chris — 16 May 2008 @ 18:04

  39. I guess we all should set the focus on how to avoid such an elementary failure of the whole quality assurance process in such a vital part of a major distribution in future. If you let a chimpansee juggle with handgrenades, you should not be surprised of the consequences. No offense, Kurt. Every third-class web magazine has a defined workflow so the content slaves hacking away on the stories are spared the embarrassment of publishing complete junk by some designated reviewer who is responsible for rejecting or approving changes. Obviously, here we have a typical 2-eye workflow, occasionally also called “black-eye-workflow” (by me, in case anybody wonders).
    Also: In my opinion many, many companies are happy to just leech on available open source resources. In all the years I am doing project work, only very few actually contributed anything to vital infrastructure projects. To them, open source just means “does not need a single penny of our precious budget and is brought by fairies at night”. I guess this problem needs a bit more publicity in the industry.

    Comment by Peter Koellner — 16 May 2008 @ 21:33

  40. OpenSSL — ein Zitat…

    Trackback by Qbi's Weblog — 19 May 2008 @ 14:19

  41. @Chriss (33)

    You are wrong here. You may be right that what OpenSSL does is valid in C99. I neither deny nor claim that. However, this is clearly undefined behavior in C89, which reads at paragraph 3.16:
    =====
    undefined behavior: Behavior, upon use of a nonportable or erroneous program construct, or erroneous data, or of indeterminately valued objects, for which this International Standard imposes no requirements.
    =====

    Uninitialised objects with allocated or automatic storage have an indeterminate value, in both standards. And since there are not many compilers out there which implement C99 (fully), there’s something fishy. When I compile OpenSSL on my box, for example, the macro __STDC_VERSION__ is NOT set to at least 199901L (which is mandatory for C99, section 6.10.8 of C99).

    So we have either a bug in OpenSSL, or in the build system (which doesn’t make sure this construct is restricted to C99 compatible compilers). Pick your poison.

    Comment by hb — 19 May 2008 @ 16:55

  42. One thing I haven’t seen mentioned in discussions about this bug are source code comments.

    I know the axiom “comments are misleading; debug only code.” However, if the author had taken the time to write an explanation of WHY he wrote code that used uninitialized memory, then people might not have tried to “fix” it.

    Comment by SKuck — 19 May 2008 @ 20:01

  43. Hb, if your interpretation is correct copying structures with memcpy is generally UB. This is a bug in the C89 standard, it was changed for a reason.

    Comment by jw — 19 May 2008 @ 22:15

  44. hb (#41), I was talking about this:
    http://groups.google.com/group/comp.lang.c/msg/5747675522c8f1e0

    The standard gives the impression that reading objects as “unsigned char” can never cause undefined behavior because “unsigned char” has no trap representations and standard also says
    “Values stored in objects of type unsigned char shall be represented using a pure binary notation.”

    Unfortunately, the C99 standard also contains this sentence:
    “The value of an object with automatic storage duration is used while it is indeterminate.”

    So whatever you might be able to infer from other rules, this one leaves no inch for automatic storage.

    I still think it is possible to access “uninitialized memory” without violating the standard, by exploiting the clause 6.2.6.1 #7:

    “When a value is stored in a member of an object of union type, the bytes of the object representation that do not correspond to that member but do correspond to other members take unspecified values, but the value of the union object shall not thereby become a trap representation.”

    The term “unspecified value” means the value is no longer “indeterminate”. Note this only applies to unions but not structs.

    Comment by Chris — 20 May 2008 @ 1:16

  45. @jw (43)

    That depends of how you actually understand the term “used”. In my understanding, trying to get the value of an object (while adding something to it, for example, or do any other operation _on that current value_) is “using” it.

    Clearly, just assigning a valid value to an object is not “using” it. Otherwise, there would be no way to “use” an uninitialized object at all. memcpy() does not try to get a value from the object, but just assigns a (hopefully) valid value to it.

    Comment by hb — 20 May 2008 @ 8:44

  46. @Chris (41)

    You are still arguing on the C99 standard. However, in 99.9% of the time, OpenSSL will NOT be compiled by a valid C99 compiler (of which not many exist). So in fact, you have to either make sure that a C99 compiler is used (for example by preprocessor-testing __STDC_VERSION__ >= 199901L in the code when relying on it, or by adjusting OpenSSL’s build system), or you must consider the older C89 standard.

    And C89 has a much simpler view on this issue. It doesn’t even know the notation of a trap representation. It just explicitely says: Use of indeterminate valued objects is undefined behavior.

    Comment by hb — 20 May 2008 @ 8:52

  47. Addition to myself:
    And even in C99, as you pointed out yourself, there is some confusion about what actually is valid, and what is undefined.

    So all-in-all, in the worst-case scenario, the use of uninitialized values in OpenSSL’s random number generator is harmful. In the best-case scenario (presuming that C99 is used AND it is valid in C99), it is just useless. If the use of uninitialized memory causes a significant entropy gain, there is something wrong with the RNG anyways.

    Comment by hb — 20 May 2008 @ 9:01

  48. Well.. I suppose memcpy could implement some black magic in which it doesn’t need to “get” the character values in order to copy them (which are possibly indeterminate due to padding).

    Comment by jw — 20 May 2008 @ 11:55

  49. jw (#43), the standard doesn’t say using an indeterminately valued object has undefined behaviour but only using its value. Assigning a value to such an object doesn’t have undefined behavior as it doesn’t use its value.

    Or are you talking about padding bytes in structs? C99 is very explicit on this and says that padding bytes take unspecified (not indeterminate!) when a value is assigned to the object or one if its members. As far as I understand C89 and C99 now, copying a struct that is not completely initialized whether with memcpy() or a struct copy has the same undefined behavior as using any other indeterminate valued object. There’s certainly a lot of code out there doing exactly this.

    Consider the following example. stat() isn’t defined by the C standard but the point is, there’s nothing in POSIX that guarantees stat() will initialize the whole struct on success, as far as I know. Only individual members might be initialized. You may think of analogous examples for other functions using structs. So if you want to be absolutely safe, you have to initialize the struct in advance yourself (or use a static variable).

    int stat_cwd(struct stat *sb_ptr)
    struct stat sb;
    int ret;

    memset(&sb, 0, sizeof sb); /* Avoid UB */
    ret = stat(“.”, &sb));
    if (0 == ret) {
    *sb_ptr = sb; /* Could cause UB if
    sb is only partially initialized */
    }
    return ret;
    }

    Comment by Chris — 20 May 2008 @ 12:55

  50. I think what most people lost sight of in the “uninitialized memory” debate (I first spelled that deBAIT… I think my subconscious is trying to tell me something :P) is that the (latest) debian patch actually implements the original OpenSSL devs’ suggestion: “go ahead and remove [the use of uninitialized memory]“.

    Should OpenSSL use uninitialized memory to increase its entropy pool? Probably not. But they DID include a flag to allow the software to be compiled without that extra bit of entropy…

    The fact that ONLY ONE of the two lines that were commented was preceded by an “#ifndef PURIFY” should’ve made the debian patcher think about WHY that line wasn’t generating any errors…

    That would’ve saved everyone a big headache.

    Comment by *Alessandro* — 20 May 2008 @ 16:28

  51. Somebody ought to tell developers about not trying to be the proverbial apprentice that messes with crypto software. It’s way too easy to break. And the rest of the world eels the consequences. Best solution is not to follow those (Linux) distributions that add their own patches to things like OpenSSL, PGP, … and/or pull crypto components directly from the source.

    As for debian taking the blame: they made the bad change, nobody else did, if they didn’t find a channel to get authoritative answers: that’s their problem. You cannot expect the very few crypto experts there are on this planet to be reachable for every wannabe.

    Crypto software is different!

    FD: Not affiliated with OpenSSL, just sane enough to know where I can mess and where it’s dangerous.

    Comment by anonymous coward — 20 May 2008 @ 16:59

  52. Many people have stated that the problem is that OpenSSL reads unitialized memory. But isn’t it (I haven’t read it — correct me if I’m wrong) that OpenSSL’s docs clearly state that the buffer will be read and mixed with entropy pool and so responsibility for doing something with the whole uninitialized-memory-access problem is on the caller?

    Comment by Robert Obryk — 26 May 2008 @ 21:22

  53. Firstly, a Valgrind warning can hardly be regarded as a bug that needs to be fixed, claims that it is so reveals a lack of understanding that the warning is harmless. Someone with that level of misunderstanding of the code (somehow managing to miss the big red #ifdef PURIFY hints), I don’t want near any crypto library I’ll be using in production code, lesson learned, Debian can’t be trusted.

    The SSL team can’t be blamed for this fuckup, the blame for making the change, and shipping it, lies squarely with the *person who made the change*.

    Stock upstream OpenSSL is secure for this particular issue, and Debian OpenSSL is (was) not.

    You can’t whine that upstream didn’t respond to you in time. Upstream isn’t a commercial organization, do you have an SLA or support contract with them? No? Well then, you introduced this fuckup, you carry the can for it. You can’t shift blame whatsoever.

    You fucked up, admit it, eat humble pie, and maybe consider changing the way you fork almost everyone’s software to “improve” it.

    Comment by James Wu — 27 May 2008 @ 23:08

  54. [...] security flaw was introduced when some poorly informed developer (see here for lots of info, also here) modified a grand total of two (2) lines of codes. (and if you’re curious how this poor [...]

    Pingback by Overheard In Providence » Blog Archive » By chance, flawed — 28 May 2008 @ 4:20

  55. For what it’s worth, I am paid to work — part-time — on OpenSSL in my company’s product. I’d be glad to have some of that work go back into OpenSSL; this is how most people are “paid to work on” most open-source projects.

    Generally speaking I have found attempts to propose any kind of nontrivial change to OpenSSL very frustrating. Either they’re ignored by the developers (e.g. one poor soul’s *two years* of attempts to get the developers’ acceptance of a change, with several iterations of a patch, to fix, compatibly, an API botch in shutdown for nonblocking sockets) or if one goes to quite a bit of effort to attract the developers’ attention, large, baroque alternative solutions are proposed but not, ultimately, checked in; so stasis is maintained in the situation where corporations who need to customize OpenSSL for use in their products keep their changes privately even if they _want_ to feed them back.

    I was interested — and then dismayed — to see a sudden burst of email activity from the OpenSSL developers over the past weeks bashing the Debian developers (justly, I should add) and various others who got caught in the crossfire (perhaps justly, perhaps not). I was interested because, frankly, this is the most activity from the OpenSSL developers *on their own mailing lists* I’ve seen in years of observation. Propose a patch to fix a problem that makes the non-blocking version of the API unusable without incurring a low-probability race condition? Expect to wait months or years with no developer feedback. Give up in frustration and don’t try to discuss your changes with the OpenSSL team? Well, if you *break* OpenSSL, expect to hear plenty from the same people whose attention you couldn’t get to save your life before.

    Given this I think it is really really unreasonable to complain that companies use OpenSSL but don’t contribute their work back. Contributing work back to OpenSSL is extremely difficult because of the poor communication around most if not all aspects of the project for outsiders. If that could be fixed, a lot of resulting problems might get much better.

    Comment by Tls — 30 May 2008 @ 20:12

  56. I welcome any suggestions to improve this situation.

    How about improving the code quality? Any time you do something unusual, like deliberately using uninitialized data, it ought to be commented to explain why you’re doing it.

    If the code had been commented adequately, perhaps the poor sap trying to maintain it wouldn’t have removed the critical lines. (I note that as it stands, the code doesn’t even bother to say what ssleay_rand_add is supposed to do or what its parameters represent.)

    Comment by mathew — 12 Jul 2008 @ 0:32

RSS feed for comments on this post.

Sorry, the comment form is closed at this time.

Powered by WordPress