Forgot your password?
typodupeerror
Security Bug Programming

Heartbleed Coder: Bug In OpenSSL Was an Honest Mistake 447

Posted by samzenpus
from the only-human dept.
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 Anonymous Coward on Thursday April 10, 2014 @08:38PM (#46720759)

    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 grub (11606) <slashdot@grub.net> on Thursday April 10, 2014 @08:41PM (#46720777) Homepage Journal

    and all running on top of TCP

    Not necessarily. OpenVPN is an SSL VPN which defaults to UDP/1194.
  • Re:Names! (Score:5, Informative)

    by bloodhawk (813939) on Thursday April 10, 2014 @08:43PM (#46720799)
    The reviewer was named too.Dr Stephen Henson
  • by viperidaenz (2515578) on Thursday April 10, 2014 @08:48PM (#46720833)

    Lawyers love EULA's and licenses. The OpenSSL license disclaims all liability

    * 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.

    https://www.openssl.org/source... [openssl.org]

    If you never agreed to that license, you're violating their copyright.

  • Re:Names! (Score:5, Informative)

    by grub (11606) <slashdot@grub.net> on Thursday April 10, 2014 @08:59PM (#46720933) Homepage Journal

    PR: 2658

    Submitted by: Robin Seggelmann <seggelmann@fh-muenster.de>
    Reviewed by: steve

    Support for TLS/DTLS heartbeats.

    Have a look for yourself. [github.com] The reviewer "steve" is Stephen Henson.

  • Sloppy code (Score:5, Informative)

    by billrp (1530055) on Thursday April 10, 2014 @09: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...
  • by grub (11606) <slashdot@grub.net> on Thursday April 10, 2014 @09:30PM (#46721219) Homepage Journal

    RSA has denied having knowledge of the backdoor, says NSA tricked them, and has never denied the $10M payout. Some of Snowden's leaks mention it.
    Reuters has a summary [reuters.com]

    proof-of-concept backdoor with a link to the github repo.

    None of that is a smoking gun, but there is enough smoke to tell me there is a fire.
  • by cold fjord (826450) on Thursday April 10, 2014 @10:45PM (#46721635)

    I'll go as far as it is reasonable to suspect there could be a fire. All of the background reading that I have done only shows that it may be possible that a backdoor exists, not that there actually is one. As for the RSA affair, it is entirely possible that they were simply trying to promote what was at the time a very hot and promising encryption technology.

    Of course keep in mind that it took people 20 years to figure out that NSA strengthened DES against cryptanalysis methods that were still secret when it changed the S-boxes before the DES standard was approved. NSA made DES stronger, not weaker.

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

    From the proof-of-concept page [0xbadc0de.be] I mentioned above.

    Conclusion

    It is quite obvious in light of the recent revelations from Snowden that this weakness was introduced by purpose by the NSA. It is very elegant and leaks its complete internal state in only 32 bytes of output, which is very impressive knowing it takes 32 bytes of input as a seed.

    Here is the Github repo for the PoC code. [github.com]

    This PRNG is not the NSA making a crypto system stronger ala DES, it's a backdoor.

  • by blueg3 (192743) on Thursday April 10, 2014 @11: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 on Friday April 11, 2014 @02:11AM (#46722481)

    A more fitting analogy is that you wrote an open source project, took the time to explain that the code is free and may not be perfect, and then someone ignored the warning that was available at the top of every source file, and ran into the current situation.

    It's not complicated enough to need analogies.

  • by Eunuchswear (210685) on Friday April 11, 2014 @04: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 squiggleslash (241428) on Friday April 11, 2014 @07:57AM (#46723839) Homepage Journal

    FWIW, that's a misreading of Theo (and other's) comments.

    The failure to use the system malloc() was not the underlying issue. In most operating systems, the Heartbleed bug would have been implemented even if OpenSSL used the system malloc().

    The issue was more that the system malloc() in OpenBSD (and some other operating systems) has been hardened so that, when passed various flags, it'll either zero out the block first before returning it (like calloc()) or in OpenBSD's case it'll actually mark the pages using the MMU in such a way that if the block is read before written to it'll cause the application reading the block to crash.

    As a result, IF OpenSSL had used the system malloc() then two things would have happened. First, the bug would have slightly more likely been discovered (if exploited, and if someone was religiously watching what was happening to the Apache child processes on their OpenBSD server.)

    Second, regardless of whether the bug would have been discovered, OpenBSD servers wouldn't have been compromised.

    That's it.

    The bug itself had to do with allowing a mismatch between the amount of data sent and the amount retransmitted in what's essentially an echo command that TLS implements. A hardened malloc() would make it impossible to exploit that, but OpenSSL would still have a bug even with one, just one that couldn't (probably, maybe, perhaps) be used to get confidential data.

From Sharp minds come... pointed heads. -- Bryan Sparrowhawk

Working...