Heartbleed Coder: Bug In OpenSSL Was an Honest Mistake

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

Comments

Re:It's not just the implementation

[grub]

and all running on top of TCP

Not necessarily. OpenVPN is an SSL VPN which defaults to UDP/1194.

Sloppy code

[billrp]

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:Whatever you may think ...

[grub]

From the proof-of-concept page 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.

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

Re:Not malicious but not honest?

[Eunuchswear]

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:

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

Re:Not malicious but not honest?

[squiggleslash]

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.