Slashdot is powered by your submissions, so send in your scoop

 



Forgot your password?
typodupeerror
×
Security Bug Programming

Heartbleed Coder: Bug In OpenSSL Was an Honest Mistake 447

nk497 (1345219) writes "The Heartbleed bug in OpenSSL wasn't placed there deliberately, according to the coder responsible for the mistake — despite suspicions from many that security services may have been behind it. OpenSSL logs show that German developer Robin Seggelmann introduced the bug into OpenSSL when working on the open-source project two and a half years ago, according to an Australian newspaper. The change was logged on New Year's Eve 2011. 'I was working on improving OpenSSL and submitted numerous bug fixes and added new features,' Seggelmann told the Sydney Morning Herald. 'In one of the new features, unfortunately, I missed validating a variable containing a length.' His work was reviewed, but the reviewer also missed the error, and it was included in the released version of OpenSSL."
This discussion has been archived. No new comments can be posted.

Heartbleed Coder: Bug In OpenSSL Was an Honest Mistake

Comments Filter:
  • by marciot ( 598356 ) on Thursday April 10, 2014 @07:31PM (#46720699)

    Coding and campaign do not mix.

    • by Anonymous Coward on Thursday April 10, 2014 @11:03PM (#46722003)

      Hmm, considering that the real bug is OpenSSL's malloc, where it was reusing 'freed' memory I think that's the bug that is critical. The developer who put in the TLS support itself may have been under the assumption that since OpenSSL didn't die/crash once implemented that everything was honky-dory, and the reviewer likewise.

      • by 0xdeaddead ( 797696 ) on Friday April 11, 2014 @12:26AM (#46722309) Homepage Journal
        this x10000... as always everyone is looking at the wrong thing. and this goes for every project that tries to micromanage the heap.
      • by Eunuchswear ( 210685 ) on Friday April 11, 2014 @03:23AM (#46723081) Journal

        considering that the real bug is OpenSSL's malloc, where it was reusing 'freed' memory I think that's the bug that is critical.

        Well. no.

        The bad bit of code is, per http://www.tedunangst.com/flak/post/heartbleed-vs-mallocconf [tedunangst.com]:

        struct {
                unsigned short len;
                char payload[];
        } *packet;

        packet = malloc(amt);
        read(s, packet, amt);
        buffer = malloc(packet->len);
        memcpy(buffer, packet->payload, packet->len);
        write(s, buffer, packet->len);

        The bad bit is that "amt" is not checked against "packet->len", so the copy into "buffer" reads off the end of the allocated data structure "packet". The data read may be freed memory, or it may be allocated memory.

        The only way malloc could completely protect against the bug would be by putting an unmapped guard page after every malloced block - making every malloced block at least one page long and slowing malloc down by the time needed for all those munmaps. (Probably making malloc slow enough to incite OpenSSL devs to implement their own malloc layer...)

        I think the real bug is in the RFC.

        Look at the fix:

        /* Read type and payload length first */
        if (1 + 2 + 16 > s->s3->rrec.length)
                return 0; /* silently discard */
        hbtype = *p++;
        n2s(p, payload);
        if (1 + 2 + payload + 16 > s->s3->rrec.length)
                return 0; /* silently discard per RFC 6520 sec. 4 */
        pl = p;

        Why does the heartbeat request even contain the length of the heartbeat block? We know the length of the SSL record!

        (Not even bothering with the whole problem that the heartbeat thing is ridiculous - there are already ways of keeping connections alive at the TCP level - why does every layer of the protocol need it's own keepalive).

        • by Eunuchswear ( 210685 ) on Friday April 11, 2014 @03:44AM (#46723167) Journal

          I know this is redundant but this is funny:

          Seggelmann told the Sydney Morning Herald. 'In one of the new features, unfortunately, I missed validating a variable containing a length.'

          https://tools.ietf.org/html/rfc6520 [ietf.org]

          Internet Engineering Task Force (IETF)
          Request for Comments: 6520
          Category: Standards Track
          ISSN: 2070-1721

          R. Seggelmann, M. Tuexen: Muenster Univ. of Appl. Sciences
          M. Williams: GWhiz Arts & Sciences
          February 2012

          Transport Layer Security (TLS) and Datagram Transport Layer Security (DTLS) Heartbeat Extension ...

          If the payload_length of a received HeartbeatMessage is too large, the received HeartbeatMessage MUST be discarded silently.

          Can't even implement an RFC he wrote himself. Nice one.

          • by Jody Bruchon ( 3404363 ) on Friday April 11, 2014 @06:51AM (#46723805)
            Programmers make these kinds of mistakes all the time. It's not uncommon. The problem is that a mistake in a text editor "undo" function is nowhere near as big of a security issue as such a mistake in a network-connected cryptography library.

            Look at the recent Apple security hole: all they did was leave in a redundant "goto" statement that unfortunately had the effect of negating the purpose of the previous check and opening a hole. It could have been as simple as not deleting the line due to a distraction somewhere else in the code, and it likely passed all the unit tests (can't easily write a test for all of these types of mistakes.)

            Programmers are human. They'll make a ton of mistakes. I can't say I blame him for making one; anyone who has written enough C code is used to making plenty of mistakes before something usable comes out the other end.
            • by Jody Bruchon ( 3404363 ) on Friday April 11, 2014 @09:45AM (#46725149)
              In retrospect these bugs look so simple and stupid to casual observers, but programmers know that simplistic hindsight shoulda-woulda-coulda analysis such as "oh, it only needed a simple bounds check!" and "you should have removed that goto line while you were working on that code!" only shows a gross lack of understanding of what real-world programming is like. It's easy to forget a simple bounds check; hell, it's a miracle a programmer gets anything done at all with all of the variables and structs and malloc() calls and pointers that have to be memorized. I recently wrote the code for adding undo support for BusyBox vi, [busybox.net] and I can tell you firsthand that what seems like a simple thing (store changes, undo them) is a complicated and frustrating process once you start in on it. Even after I made simple undo actions work, the challenges compounded when I added the "intermediate queuing" enhancement to lower the otherwise insane malloc() overhead and improve the overall behavior of my "simplest thing that could possibly work" code.

              The process of dissolving a big problem into low-level steps as is required by C programming is mentally brutal. You can't just go "I want to save the text that was deleted and restore it when they hit the undo key." You have to translate that into variables, pointers, structs, mallocs, and glue logic. You have to take into account every corner case; how do you undo multiple lines of pasted text instead of single characters? How do you store undo information a paste-over operation, where you must keep a record of what was deleted but also record that it overwrites a (possibly different length) chunk of the text buffer? How do you store the undo data for find-and-replace operations? Now that you've managed to figure out ways to store all these different types of text editing operations for undoing, how do you actually undo each one? Where does all this stuff need to hook into the existing program, and how will you hook it in without breaking existing functionality?

              It's so easy to forget trivially simple things when you're trying to flow your mind through that complex glue logic that makes the magic happen, especially when you make a change, it appears to work, and you have no way to test for the bug you created by accidental error or omission.
            • Re: (Score:3, Interesting)

              by Bacon Bits ( 926911 )

              Programmers are human. They'll make a ton of mistakes.

              Doctors are human. We hold them accountable for their mistakes. Engineers are human. We hold them accountable for their mistakes. Indeed, we hold just about everybody accountable for their on-the-job mistakes and the consequences of their mistakes result in everything from terminations to criminal proceedings.

              So, when should programmers be held accountable for their mistakes, and how should be respond as a society?

              • by Jody Bruchon ( 3404363 ) on Friday April 11, 2014 @10:37AM (#46725747)
                Using your analogy, free software like OpenSSL would be under the "good samaritan" laws. If someone attempts to sew you up in the field to stop bleeding but the stitches fail and you bleed out and die anyway, that person wouldn't be held responsible for killing you since they were attempting to help you as best as they could and you'd have died otherwise. If you don't use a SSL stack, you have no encryption. OpenSSL programmers are good samaritans, but OpenSSL is not offered with any guarantee that it will be useful, you don't have to use it (there are many cipher suites), and since it's open, the user can audit it for themselves. One could argue, "why didn't the millions of users of the code audit the code before using it? When do we hold our hosting providers and banks responsible?"

                A doctor performing surgery or an engineer designing a manufacturing machine (both at significant expense to the customer) are quite different from using something you got completely for free with full design schematics an upfront "there is no guarantee that this will work but we hope you find it useful" warning and that free thing leaking its contents for some reason.
            • by MartinG ( 52587 )

              Programmers are human. They'll make a ton of mistakes

              Humans make certain classes of mistake. Things like array bounds checking are really easy to miss.

              If only machines were good at this stuff. How come we don't have any languages that do this for us yet?

        • by TheRaven64 ( 641858 ) on Friday April 11, 2014 @04:36AM (#46723311) Journal
          The point is not that a general malloc() would catch it, but that there are security-focussed malloc() implementations that will. Even valgrind will - it knows that malloc() has special properties and so will object if you derive a valid pointer to the wrong allocation by running off the end of another one. You don't need to use the security-focussed malloc() in deployment (unless you're really paranoid), you just need to support testing with it. Running this code with a malloc() that did aggressive bounds checking would have caught it immediately. That's something a continuous integration system and a test suite ought to have caught.
        • Re: (Score:3, Interesting)

          by Zidel ( 1687822 )

          Why does the heartbeat request even contain the length of the heartbeat block? We know the length of the SSL record!

          The record has two variable length fields, so you need a length field for either the payload or the padding. In this case the payload has the length field and the padding length is implicit.

        • by amn108 ( 1231606 ) on Friday April 11, 2014 @05:27AM (#46723529)

          If they had resorted to their area of expertise and simply used the malloc provided with the system, like all the regular chaps would do, even in their situation, the code would crash upon running (freed memory access) and the bug would surface already at New Years Eve 2012-2013 when Seggelmann was hopefully test-running it. So, even though indeed the code you quoted is the "bad bit", the real and broader issue probably is the teams questionable approach to development in general, in particular their false belief that someone writing a security library should consider themselves experts in rewriting heap management. Which ultimately cost them and their users. Sloppy sloppy.

          This kind of practice of overestimating ones area of expertise - should be frowned upon everytime, for a good reason. We (developers) need to put it in our heads - not all algorithms are equal, and even though you and me may be prime experts at say, writing a perfectly safe implementation of SSL/TLS, we probably should steer clear of the stuff others know much more about, like heap, strings and what not. Time and again, someone comes along with the "brilliant" idea of "optimizing" the system heap allocator through caching memory blocks. True genius. No offense Robin, but WHY?! Yes, maybe the system malloc is slower than you'd like - still it is NOT YOUR PROBLEM. Division of responsibility, man. Let Glibc folks optimize malloc, or submit a patch THEY can review, if you have wonderful malloc improvement ideas.

          • Re: (Score:3, Insightful)

            by Eunuchswear ( 210685 )

            If they had resorted to their area of expertise and simply used the malloc provided with the system, like all the regular chaps would do, even in their situation, the code would crash upon running (freed memory access) and the bug would surface already at New Years Eve 2012-2013 when Seggelmann was hopefully test-running it.

            Only if Seggelman thought to test it with a malformed packet.

            If the code was run with a malloc that splattered unmapped pages around like OpenBSD malloc apparently does then it would crash when it was exploited, and people would be complaining "OpenBSD breaks OpenSSL, fix your shit Theo" and the problem would have been found earlier.

      • Comment removed (Score:5, Informative)

        by account_deleted ( 4530225 ) on Friday April 11, 2014 @06:57AM (#46723839)
        Comment removed based on user account deletion
  • by Kittenman ( 971447 ) on Thursday April 10, 2014 @07:32PM (#46720709)
    hats off to the developer who admits a mistake.

    I suspect s/he'll get pilloried in the press and may end up with some lawsuits (?) but I, for one, recognize a person big enough to take responsibility.
    • Re: (Score:2, Informative)

      by Anonymous Coward

      The RFC and the Git commit both have his name firmly attached to the "feature". He hasn't admitted anything that wasn't proven already.

    • by Anonymous Coward on Thursday April 10, 2014 @07:46PM (#46720825)

      may end up with some lawsuits (?)

      very difficult;

      a) the license clearly states the lack of warantee

      b) the source code is available so you could have audited if you want

      c) I for one will contribute considerably if that happens unless decent evidence is uncovered that he works for the NSA. I'm sure there are several others who will do the same.

    • by Anonymous Coward on Thursday April 10, 2014 @07:55PM (#46720901)

      Devil's advocate here: I have to thank the developer for even working on OpenSSL. I fear that the bad press and consequences coming down on these developers will scare more people off from core OSS projects and we will end up with more app developers on smartphones instead of infrastructure improvements.

      It would be nice if they had some sort of code review in place for this sort of stuff. However, this isn't a paid project, so the developers writing this are doing arguably the best they can.

    • Well, he hasnt admitted to anything. He has two options 1) Admit it was malicious 2) Admit it was a mistake. It doesnt take a genius to figure out 2 is the best option.

    • by im_thatoneguy ( 819432 ) on Thursday April 10, 2014 @07:59PM (#46720935)

      Boy, if there's one thing that could ever kill Open Source it would be being held legally liable for a commit with a bug in it.

      Which is why all projects are released AS-IS without any liability.

    • by Krishnoid ( 984597 ) on Thursday April 10, 2014 @08:41PM (#46721293) Journal

      hats off to the developer who admits a mistake.

      It's laudable but insufficient; to genuinely move towards making the aggrieved parties whole, I think it demands nothing short of a full refund.

    • may end up with some lawsuits (?)

      If you have ever wondered why all the popular open source licenses, like GPL, BSD and Apache, include the "warranty" and "limitation of liability" clauses, this is exactly why. The clauses usually state something like "this software is provided 'as is' and without any warranty. The user of the software assumes all risks that may arise. In no event shall the project or its contributors be liable for any damages."

    • by gweihir ( 88907 )

      This is not the US. There will be no lawsuits. "No warranty" actually has meaning here.

    • by Albanach ( 527650 ) on Thursday April 10, 2014 @10:32PM (#46721859) Homepage

      Well pretty much anyone can start a lawsuit. But what damages are they suing for? Reimbursement of the purchase price?

      If you're using it, you're agreeing to the license:

        * THIS SOFTWARE IS PROVIDED BY THE OpenSSL PROJECT ``AS IS'' AND ANY
        * EXPRESSED OR IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED TO, THE
        * IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR A PARTICULAR
        * PURPOSE ARE DISCLAIMED. IN NO EVENT SHALL THE OpenSSL PROJECT OR
        * ITS CONTRIBUTORS BE LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL,
        * SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES (INCLUDING, BUT
        * NOT LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES;
        * LOSS OF USE, DATA, OR PROFITS; OR BUSINESS INTERRUPTION)
        * HOWEVER CAUSED AND ON ANY THEORY OF LIABILITY, WHETHER IN CONTRACT,
        * STRICT LIABILITY, OR TORT (INCLUDING NEGLIGENCE OR OTHERWISE)
        * ARISING IN ANY WAY OUT OF THE USE OF THIS SOFTWARE, EVEN IF ADVISED
        * OF THE POSSIBILITY OF SUCH DAMAGE.

      Now I am not a lawyer, and there are always folk looking for an opportunity to sue, but the license terms surely set them off on a bad start.

  • by Anonymous Coward on Thursday April 10, 2014 @07:36PM (#46720739)

    The design of the feature looks like a backdoor too. A heartbeat function with a variable length payload, and there is a superfluous field for the payload length, and all running on top of TCP, which already has a keep-alive function? And then the feature contains a "rookie mistake", but still passes review. Yes, we totally believe you. It was a mistake.

    • by grub ( 11606 ) <slashdot@grub.net> on Thursday April 10, 2014 @07:41PM (#46720777) Homepage Journal

      and all running on top of TCP

      Not necessarily. OpenVPN is an SSL VPN which defaults to UDP/1194.
      • OpenVPN does its own transport protocol [openvpn.net] (on top of UDP or whatever was configured) to wrap the SSL control connection in. And for that reason OpenVPN implements its own heartbeat protocol. Let me repeat that: there is no use for TLS heartbeats with OpenVPN.

        Side-note: as OpenVPN does not use vanilla SSL sockets, simple-minded Heartbleed exploits that work against HTTPS etc. won't be usable against it, but it is possible to hand-craft a Heartbleed attack [serverfault.com] against OpenVPN servers (or clients) running with u

    • by Anonymous Coward on Thursday April 10, 2014 @08:52PM (#46721357)

      The feature was TLS/DTLS heartbeats, with the "D" in DTLS standing for datagram.

      HTTPS isn't the only thing using OpenSSL. As grub pointed out, OpenVPN uses it as well, and will use DTLS by default.

      The advantage of tunneling traffic in datagrams is that if you try to tunnel TCP traffic within TCP, performance suffers due to dueling TCP backoff.

    • by Anonymous Coward on Thursday April 10, 2014 @09:03PM (#46721431)

      It's a possibility, but pretty paranoid reasoning.

      Assuming you have coded in your life (your language implies so), there were errors. I certainly made more than a few. Some the compilers caught, some the testing, some the users, some the accountants; some... probably nobody.

      Care to explain why any of yours were not intentional because it sure looks that way. Hindsight is cheap.

      My knee jerk was also that length would be superfluous - from zero context and understanding of the spec/RFC. But there could be a lot of reasons. If your job is to process a formally defined struct, you are not going to review the struct in an attempt to change the standard.

      Which brings us back to where we began. If this was a conspiracy, intentional coding would be required for exploit. Given that, I find it difficult to accept that an agency intentionally first complicated the spec by including a length field (which *could* be checked, in the name of security, for protocol robustness rather than local memory ops); then hen perverted a particular implementation in a manner that looks exceedingly garden variety. Easier to never have the length field in the first place.

      • by pjt33 ( 739471 ) on Friday April 11, 2014 @02:28AM (#46722799)

        If your job is to process a formally defined struct, you are not going to review the struct in an attempt to change the standard.

        The developer who wrote this code also wrote the spec [ietf.org], as part of his PhD research. To me the more worrying aspect of this whole affair is that OpenSSL accepted into the trunk an extension which at the time hadn't even reached "Proposed standard" status (and still doesn't seem to have progressed beyond it).

    • by blueg3 ( 192743 ) on Thursday April 10, 2014 @10:07PM (#46721753)

      As others have indicated, the primary stated use of heartbeat is for DTLS, which is not over TCP.

      The payload length is not actually superfluous. The packet has an arbitrary amount of both payload and padding, of which only the payload is echoed to the sender. Roughly: { uint16 payload_len; char payload[]; char padding[]; } The intent of payload_len is to tell you which of the bytes following it are payload rather than padding. Of course, you need to check that it's less than the remaining data in the packet. (Per the spec, at least 16 less -- at least 16 bytes of random padding are required.)

  • by Anonymous Coward

    All I know is the organization I work for has prohibited use of C or C++ for mission critical software for years now. The languages we use would not ALLOW code to execute which tries to copy 64K from a 2 byte sized container.

    Part of software engineering is to use the right tool for the right job. When a buffer overrun can destroy the security of the entire internet, you damn well better not be using C as your tool. Or assembly language for that matter.

    Do that where I work, and you'd be fired.

    • by gweihir ( 88907 )

      This is BS. You can screw up just as badly in any other language. It will just be less obvious. Of course, the combination of an incompetent code writer and an incompetent reviewer will quite often have spectacularly bad results.

  • Well, maybe this is a blessing. While it's open source, maybe multiple eye's need to look at it for final validation.
    While never have worked with open source, I do work with data very often. I re-validate entries a lot and just scan data look for something odd. I am amazed at the amount of errors I find, so I report them and go from that point onwards.
    sometimes I work with teams of people and have a kind of race looking to find the most errors, then we go out and the winner only buys 1 round, and does not p

    • by VortexCortex ( 1117377 ) <VortexCortex@pro ... m ['jec' in gap]> on Friday April 11, 2014 @01:01AM (#46722451)

      Well, maybe this is a blessing. While it's open source, maybe multiple eye's need to look at it for final validation.

      No it's a curse. I have input fuzzing, unit tests, code coverage profiling and Valgrind memory tests. Such a bug wouldn't have slipped past me with both eyes shut -- no seriously! If I fuck up accidentally like this THE COMPUTER TELLS ME SO without ever having to do anything but make the mistake and type make test all. I test every line of code on every side of my #ifdef options, in all my projects. If you're implementing ENCRYPTION AND/OR SECURITY SOFTWARE then I expect such practices as the absolute minimum effort -- I mean, that's what I do, even when it's just me coding on dinky indie games as a hobby. I don't want to be known as the guy who's game was used to compromise users' credentials or data, that would be game over for me.

      These ass-hats have just shown the world that they can't be trusted to use the fucking tools we wrote that would have prevented this shit if they'd have just ran them. It's really not acceptable. It's hard to comprehend the degree of unacceptable this is. It reeks of intentional disaster masquerading as coy "accidental" screw up, "silly me, I just didn't do anything you're supposed to do when you're developing industry standard security software". No. Just, no. An ancient optimization that was made default even though it only mattered on SOME slower platforms? Yeah, OK, that's fucking dumb, I can buy it as an accident. However, NOT TESTING BOTH BRANCHES for that option? What the actual fuck? I could see someone missing an edge case in their unit test, but not even using input fuzzing at all? It's not hard, shit, I have a script that generates the basic unit fuzzing code from the function signatures in .H files, you know, so you don't miss a stub...

      "Never attribute to malice what can be adequately explained by stupidity." -- The level of stupidity required is unexplainable. How the fuck are they this inept and in charge of THIS project? THAT'S the real issue. This isn't even the fist time OpenSSL shit the bed so bad. [taint.org] In <- this linked example, it was Debian maintainers and not the OpenSSL maintainers fault (directly): Instead of adding an exception to the Valgrind ignore list (which you most frequently must have in any moderately sized project, esp one that handles its own memory management) they instead commented out the source of entropy, making all the SSL connections and keys generated by OpenSSL easily exploitable since it gutted the entropy of the random number generator (which is a known prime target for breakage that's very hard to get right even if you're not evil, so any change thereto needs to be extremely well vetted). Last time the OpenSSL maintainers brazenly commented they "would have fallen about laughing, and once we had got our breath back, told them what a terrible idea this was." -- Except that they silently stopped paying attention to to the public bug tracker / questions and quietly moved to another dev area, making it nearly impossible to contact them to ask them about anything (a big no-no in Open Source dev), but it gives you a better idea about the sort of maintainers these fuck-tards are.

      We don't know absolutely for sure, but we're pretty damn close to absolutely certain that OpenSSL and other security products (see: RSA's BSafe) are being targeted for anti-sec by damn near all the powers that be. So, now we find out OpenSSL has an obsolete optimization -- a custom memory pool (red flag goes up right away if you see memory reuse in a security product, that shit MUST be even more throughly checked than entropy-pools, since it can cause remote code execution, memory leaks, and internal state exposure... you don't say?). We find that optimization would have been caught by basic fuzz test with Valgrind, which apparently folks have been using previously according to the comments in the prior S

      • Re:code review idea (Score:5, Interesting)

        by reikae ( 80981 ) on Friday April 11, 2014 @02:42AM (#46722855)

        Serious question: Why don't you become the new maintainer yourself, if you honestly believe you can do a significantly better job at it than the current person(s)?

        I don't do it myself because I can not guarantee that I wouldn't make even worse mistakes. I'm glad there are people out there who are willing to do the job, and I'm in no position to bite their heads off when they mess it up. And you're probably glad that I'm not a maintainer of anything even remotely security-related :-)

        • Bingo. There's a lot of people who are willing to declare the unpaid volunteers don't know what they're doing...but have absolutely no patience to try and contribute or agitate for change from within. Which really makes me wonder whether they'd have the wherewithall to run a new project, or you know, do anything at all.

          Code that gets written gets run.

      • Instead of adding an exception to the Valgrind ignore list (which you most frequently must have in any moderately sized project, esp one that handles its own memory management)

        If you need to add exceptions to get a tool to work... the tool is wrong for the job.

  • for a library... (Score:5, Insightful)

    by MarcoAtWork ( 28889 ) on Thursday April 10, 2014 @08:02PM (#46720955)

    ... so much of the internet depends on for security just one reviewer for a commit seems way way way too little, honestly checking anything into openssl (or gnutls) should be at least a 4-step approval process (submitter -> mantainer for that area -> overall library mantainer -> security officer), for any code that includes buffers/malloc especially if related to user supplied data the final security review should be a panel.

    Everybody makes mistakes, everybody can have a 'brown paper bag' coding moment (especially around Christmas/New Year's like it happened in this case), 2 people having a 'brown paper bag' moment at the same time around the holidays is definitely not that unlikely, for something as important as a crypto library on which so many things depend a single reviewer is just not enough.

    I do feel for the original developer, and hope that he won't suffer more about this than he already is (any developer worth their salt feels quite bad about bugs they introduce, let alone if they lead to this many problems), we've all made coding mistakes, no matter how experienced we are, so the focus should not be on "who" but more on "what kind of process can we introduce so this does not happen again".

    Moving away from C in my opinion would just be a band-aid, other languages don't expose you to this particular bug, that's fine, however for security software choosing a vetting process for what goes in the codebase is a lot more important than choosing what language it's written in, not to mention that it's not that "hard" to write "secure C" especially if one leans on all the various available tools/libraries and writes proper unit tests, in this case for example had the malloc decision not been influenced by performance reasons (on unspecified platforms) this would not have been as big of a deal as it was.

    • by MightyMartian ( 840721 ) on Thursday April 10, 2014 @08:15PM (#46721051) Journal

      Moving away from C just means you now have to have faith in some bytecode virtual machine's memory and buffer management. Is it a more secure approach? Maybe, but if the root complaint is putting faith in complex software, coding in Java or some .NET language means trusting the people coding those engines are equally capable of screwing up. All these higher level virtual machines and interpreters are ultimately written in C.

      • Moving away from C just means you now have to have faith in some bytecode virtual machine's memory and buffer management. Is it a more secure approach? Maybe, but if the root complaint is putting faith in complex software, coding in Java or some .NET language means trusting the people coding those engines are equally capable of screwing up. All these higher level virtual machines and interpreters are ultimately written in C.

        Or you could just use C++ complete with their bounds-checked containers.

    • by Lennie ( 16154 )

      Great, the OpenSSL project needs more people working on it (even if only reviewers).

      You are volunteering I see ?

  • by Bram Stolk ( 24781 ) on Thursday April 10, 2014 @08:11PM (#46721019) Homepage

    The bigger problem is coders that think they need to optimize for speed.
    Read the horror here: http://article.gmane.org/gmane... [gmane.org]

    Ugh... premature optimization, the root of all evil. And now also the root of the biggest security hole ever.

  • by musmax ( 1029830 ) on Thursday April 10, 2014 @08:19PM (#46721093)
    He has no reason to. He gave it a best effort shot given the resources. If you can do better, fucking do it.
    • by Anonymous Coward on Thursday April 10, 2014 @08:39PM (#46721281)

      Thank you for a bit of sanity. What you wrote should be the summary text for every article on the subject: "He gave it a best effort shot given the resources. If you can do better, fucking do it."

      As an open-source dev myself, I often wonder why the fuck I do anything useful for others when they'll just turn on me the moment their toys don't work exactly as desired because -- gorsh -- I'm not perfect, though I work very hard to be.

      • As an open-source dev myself, I often wonder why the fuck I do anything useful for others when they'll just turn on me the moment their toys don't work exactly as desired because -- gorsh -- I'm not perfect, though I work very hard to be.

        Well, I'm a developer too. Mostly open source. Thing is, I don't bite off more than I can chew. This is a security product. They're not using basic code coverage tools on every line, or input fuzzing. They missed a unit test that should have been automatically generated. This is like offering a free oil change service boasting A+ Certified Mechanics, then forgetting to put oil in the damn car. Yeah, it was a free oil change, but come the fuck-on man. You really can't fuck up this bad unless you're stoned! I mean, if you change the oil, you check the oil level after you're done to ensure it hasn't been over-filled... You check all the code-paths, and fuzz test to make sure both sides of the #ifdef validate the same, or else why even keep that code? "I can accept the responsibility of maintaining and contributing to an industry standard security product" "YIKES I Didn't Fully Test my Contribution! Don't blame me! I never said I could accept the responsibility of contributing to or maintaining an industry standard security product!"

        It's cancerous shit like you that give open source a bad name. Own up, or Fuck off.

      • by Solandri ( 704621 ) on Friday April 11, 2014 @02:01AM (#46722679)

        As an open-source dev myself, I often wonder why the fuck I do anything useful for others when they'll just turn on me the moment their toys don't work exactly as desired because -- gorsh -- I'm not perfect, though I work very hard to be.

        Welcome to Engineering. Scott Adams (of Dilbert fame) best summarized this disconnect between commendation and blame in the Engineers Explained [boulder.co.us] chapter of his book:

        Engineers hate risk. They try to eliminate it whenever they can. This is understandable, given that when an engineer makes one little mistake, the media will treat it like it's a big deal or something.

        Examples of Bad Press for Engineers

        • Hindenberg.
        • Space Shuttle Challenger.
        • SPANet(tm)
        • Hubble space telescope.
        • Apollo 13.
        • Titanic.
        • Ford Pinto.
        • Corvair.

        The risk/reward calculation for engineers looks something like this:

        RISK: Public humiliation and the death of thousands of innocent people. REWARD: A certificate of appreciation in a handsome plastic frame.

    • by gweihir ( 88907 )

      Indeed. While both this person and the reviewer messed up badly and their competence is rightfully in question, they were also set-up by omission of basically everything you need to do to produce secure code in OpenSSL and by sabotage (likely due to terminal stupidity) of safeguards that _were_ available.

      To produce a catastrophe, several people have to screw up spectacularly. This is what happened here. Quite often, when you dig down, you find a combination of big egos and small skills.

  • Sloppy code (Score:5, Informative)

    by billrp ( 1530055 ) on Thursday April 10, 2014 @08:26PM (#46721161)
    I glanced at some of the OpenSSL C code, in particular the new code that introduced this bug. No comments, no asserts, no cross-checks, stupid variable names (like "payload" for the size of the payload, "pl" for a pointer to the payload data), no suggestions for how to test this new feature (such as what if the request has the payload size field that's not the same as the actual payload). In an unrelated function I saw an array declared on the stack, getting filled up, and then a pointer to this array getting assigned to a field of an argument to this function, and then a return...
    • Re:Sloppy code (Score:4, Interesting)

      by itsdapead ( 734413 ) on Friday April 11, 2014 @05:06AM (#46723439)

      I glanced at some of the OpenSSL C code, in particular the new code that introduced this bug.

      I don't disagree about the 'coding style' issue, but that kinda misses the point. The points are:

      Theres a memcpy() - where is the bounds checking? Hello? Its not 1976. We all know memcpy is dangerous. Where there's a memcpy there should be a bounds check... even in a fart app. If the project has secure in the title there should be paranoid anal-retentive checking of both the source and destination buffers.

      The code uses data that has come from teh interwebs, - again, where's the obsessive-compulsive validity checking on everything that comes in?

      However, that's still not the point. Programmers make mistakes - and this bug was at least a bit more subtle than the usual one where the bad hat sends an over-length string.

      The problem is with the oft-made claim that Open Source security software is extra-safe because the code is public has been seen by many eyeballs. That claim is dead. Possibly crypto experts have been all over the actual encryption/decryption algorithms in OpenSSL like flies on shit - however, clearly none of them looked at the boring heartbeat stuff. That shouldn't be the death of open source, though - Windows is proprietary and look at the sheer terror caused by the prospect of running Windows XP for one day after the security patches stop...

  • by Rob Fielding ( 3524407 ) on Thursday April 10, 2014 @10:12PM (#46721789)
    C is a perfect language for back-dooring code. Without good tools - even for the sake of code reviewers that are trying to defend the code base against good malicious programmers - we won't be able to keep up. Most C code is going to be full of accidental back doors, and possibly a few intentional ones. Something has to be done about machine assisted proof of memory safety, and about ensuring that parse code either follows a spec or stops parsing the input. High level languages with memory safe semantics are essentially constructive proof of memory safety at the cost of performance. You can also go the route of fixing the defaults of what is going to be allowed in important C files, and force programmers to #pragma their way out of violating the constraints so that potential bugs are easy to spot. Or better yet, stop using read() in the raw to ingest input; and force the code to parse input with a well defined grammar (and fail to compile otherwise). You don't necessarily need an abstract garbage collected language that prevents you from cutting yourself. What you really need is by any means necessary to limit security critical code to constructs that the compiler can prove the consistency of. Code review should essentially amount to the reviewer making a list of proof demands, with the review process failing until there are machine checkable proofs of these facts. Every exception to these rules (Supress warnings, or accepting a hand proof, or proof by authority) need to be documented at the line where they happen. That way, when the next back door is discovered, we can easily search for the place that introduced it. This is not theoretical at all. I worked with a guy who was publicly accused of back dooring a Unix. No amount of code review will clear anyone of suspicion; because we use the crappiest tools for proving correctness of even small things.
    • As opposed to a language with polymorphism, where it's impossible at compile time to know which function will actually get called at runtime? Maybe every secure situation should require C.
  • by CodeBuster ( 516420 ) on Friday April 11, 2014 @02:07AM (#46722697)
    No production code without unit tests. Every possible type or class of input must be tested. All assumptions must be tested. All outputs must be verified for each possible combination of inputs. All failure modes must be exercised. No excuses, just do it.
    • No production code without unit tests. Every possible type or class of input must be tested. All assumptions must be tested. All outputs must be verified for each possible combination of inputs. All failure modes must be exercised. No excuses, just do it.

      Nope. It's a waste of time. Much of the time the people writing the unit tests are the same people writing the code, so their assumptions are also in the unit tests.

  • Robin Seggelmann, thank you and the entire OpenSSL Team for your contributions to free open source software. Glad we could find a serious security flaw, that you're helping to find out how it happend and that the OpenSSL crew is so fast in coming up with a fix.
    With just about any other development paradigm and folks like MS we'd've waited for weeks for that to happen.

    Carry on with the good work, you guys rock!

If A = B and B = C, then A = C, except where void or prohibited by law. -- Roy Santoro

Working...