Add heartbeat extension bounds check

This line made me go between it and inspecting the ssl3_record_st structure carefully, since the way it was written looked like a flexible array member:

    unsigned char *p = &s->s3->rrec.data[0], *pl;

I think it would be better written as:

    unsigned char *p = s->s3->rrec.data, *pl;

to make it clearer that data is a pointer within the structure.

Thanks; fascinating. The "length" of the payload is useful for error correcting, but the implementer didn't take advantage of this property, so it was usable against them. A double edged sword.

I believe there's another one that occurs every time a heartbeat request is received, and was just as surprised that they had to allocate a new buffer, copy the payload over (which is where the bug was), basically construct a new message, when they could've just validated the incoming message, changed type + padding, and sent it back out using the same buffer. Heartbeat extension is only slightly more complex than an echo protocol.

It's probably much harder to remove the one on reception due to how the rest of OpenSSL is written, but at least from a glance at the code, a lot easier to rid the sending one. In terms of design, the simplest implementation would be malloc-on-receive + modify-and-send; a little better is an expanding buffer that's allocated once but reallocated if necessary, and to me, the way it's currently being done is the most complex, inefficient, and error-prone.

Having many unnecessary dynamic allocations tends to be a trend I've noticed most often in C/C++ code written by programmers with a Java background. Not saying that this necessarily applies to heartbleed's culprit, but the general trend of excessive complexity is there.

I wouldn't know, but in any case that would require allocating the whole 64KiB for each connection, when the actual heartbeats can be much smaller. Wouldn't the memory waste be worse than the allocations?

I don't get it either. I am an old programmer full of habits too I suppose, but I had no problem to adopt mandatory curly braces after reading the rationale for mandatory curly braces in Golang years ago. I would think the Apple bug was an even stronger rationale to adopt mandatory curly braces habit...

Except in this case, an extra 'return 0' would just mean ignoring the heartbeat message completely. It's a slightly different situation from Apple's bug.

Indeed, seeing that coding style should raise further questions -- if the fix to a huge security hole is somewhat sloppy, what are the chances that everything else is sloppy?

I wonder how "unvalidated user input passed to memcpy()" didn't trigger all of the static analysers...

I guess that the anaylser doesn't know that it's untrusted - perhaps it's worth having separate "trusted int" and "untrusted int" data types, so this would've been a compile-time error?

I'm curious as to why they are not using constants or preprocessor macros for these values. It seems weird for it to be repeated in multiple places.

Yeah, it just that you wanna make sure the padding variable is not being overridden by some malloc ;)

Did you submit a pull request?

It's the biggest security vulnerability in the modern crypto age. Sysadmins everywhere are scrambling to renew keys and certs. It warrants at least 8 stories on the frontpage.