To wit: TLS Heartbeats were originally intended for DTLS (in fact, the first draft was apparently DTLS-specific). Datagram protocols have a problem: if datagrams are too large, the IP protocol will need to fragment them. Fragmentation is calamitous for performance and reliability. So sizing datagrams is tricky. One technique for solving that is "path MTU discovery". The original use case for TLS Heartbeats was, as the RFC spells out clearly, PMTU discovery.

The payload/padding split exists so that one side of the protocol can send large messages, and the counterparty can reply with smaller ones, relying on the fact that the message includes discardable padding.

It's not a good protocol. The functionality is implemented at the wrong layer. And the inclusion of the extension into TCP TLS was a mistake, one (also) discussed on the mailing list. Unfortunately, the way standards work, it's easier to say "yes" to new features than "no", and it was easier to add the extension to both TCP TLS and DTLS than it was to hash out the argument of whether TCP TLS needed the feature.

If the payload has variable size to facilitate MTU discovery, then there is some logic to sending messages of varying sizes. An MTU can never exceed the 1500 of ethernet though, so this is weird stuff. What good can a payload of 64k really do? And MTU discovery doesn't involve any echoing by the receiving party. Either packets get blocked (at the IP layer) and error messages get returned (by the router blocking them) OR the packages arrive fully (or get reassembled) so this Heartbeat code won't know about any issues anyway. And the "echo" proves nothing. I really don't get this, what is the point...?

I think you said it yourself. So why argue when it's pointed out?

Clearly path discovery has nothing to do with keeping a connection open. Further, can't path discovery be done in an un-encrypted TCP or UDP channel with separate tooling and the results passed into tuning parameters of the encrypted protocol? (This is even assuming the security layer on top of TCP even needs to know MTU when TCP is handling it already)

Since when is THE secure protocol supposed to be the kitchen sink dumping ground of the internet?

The bug was a typical C slip-up. You see it everywhere. It happens. Yes, the stakes are much higher here, but I don't think the fact that Robin also authored RFC 6520 makes it more suspicious.

But something that seems just wrong is the expectation that "someone else" (the small all-volunteer OpenSSL team) should have taken more time away from their day jobs and family to: have different people writing specs and implementation, writing more tests, running static analysis tools, have code more reviews, and refactored code to be more readable. But if OpenSSL is so critical to your privacy, security and business, where were your contributions to writing those tests and code reviews?

I don't mean that every developer must contribute code to be allowed to have an opinion - but most of the lamentation at "what the OpenSSL team should have done" never considers who exactly should be spending time or money on this extra effort.

They are the ones the technical computer community that operates at this level of network implementation have "chosen" to do this work (or they've chosen themselves, which is practically the same thing) which is an honour to them.

If anything goes wrong here, it is that culture at large, the way the RFCs get debated and approved, etcetera. There is no one to blame here in isolation.

Although I will agree with you that it is remarkable that this person forgot to write the check on payload_length after stating that so explicitly in the document.

However, after reading the related RFC I've rarely seen such over-designed garbage. I'll be amazed if OpenSSL is the only implementation with a heartbeat exploit.

As to the implementation. It just makes me queasy to see a callback before any validation code (bounds checking). It's still that way in the latest fix. I physically can't get past that to even ponder the rest.

At least the new comment has the right idea:

/* Read type and payload length first /

Unfortunately... It's definitely not first.

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

Wow, on so many levels just wow. Terminate the connection forcibly anyone? return ALL_HELL_BROKE_LOOSE & YOURE_UNDER_ATTACK? No, guess that's not helpful here...

/ Read type and payload length first (now w/accuracy / if (1 + 2 + 16 > s->s3->rrec.length) for( int borken = 0;;) return 1 / borken;

if (s->msg_callback) ...

There, fixed that for ya.

Yet, there's ANOTHER line that stops me cold:

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

I don't suppose s or s3 could EVER be NULL? Of course, USUALLY that would just cause some kind of app crash...

Note: It's very obvious the validation code is in the wrong spot. However, after looking at it? It's so unclear what is going on in the validation code that I'd have to spend some time to detangle it.

This has all the markings of a brilliantly executed backdoor. That being said, my inkling is it was an honest mistake.