

RFC 6520 and the OpenSSL Heartbleed bug share the same author - JoelJacobson

I find it scary the git commit bd6941cfaa31ee8a3f8661cb98227a5cbcc0f9f3 which added support for TLS&#x2F;DTLS heartbeats, was written by Robin Seggelmann &lt;seggelmann@fh-muenster.de&gt; who is also one of the three authors of the RFC 6520 specification.<p>The &quot;payload&quot; in RFC 6520 is extremely bad design from a security perspective, it fulfils no purpose, a bit suspicious someone with brains would deliberatly add unnecessary complexity to the SSL procol, which is already a monster.<p>Even more scary, the spec clearly points out &quot;If the payload_length of a received HeartbeatMessage is too large, the received HeartbeatMessage MUST be discarded silently.&quot;, but someone the author of the spec failed to remmeber this when writing the OpenSSL implementation of the very same spec. Highly unlikely.
======
tptacek
The payload in RFC 6520 serves a clear purpose, one discussed in the open on
the TLS WG mailing list, which you should probably read before casting stones.

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.

~~~
sagemode
Can you explain why it is neccesary or functional 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." ??

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

------
apawloski
RFC 6520 is the Heartbeat specification, right? Design notwithstanding, is it
really an unbelievable scenario that the person who wrote the RFC also
implemented it?

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.

------
ProblemFactory
The Heartbleed bug was a C buffer overflow. Entirely unrelated to the
heartbeat feature, its spec, or the functionality of SSL as a whole. It had
serious impact because was in a popular network-accessible library - something
similar could also happen in the Linux TCP stack, in a popular web or database
server, or any other network-accessible C program.

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.

------
sagemode
It is wholly inappropriate to start blaming these authors, or to even suggest
they are malicious (which is even worse).

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.

------
bjourne
It's better to yourself get involved in drafting the rfc:s, if that is your
thing, rather than throwing out insane accusations.

~~~
ebyrob
Microsoft taught us all to "Never attribute to malice...".

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

~~~
ebyrob
Ugh, formatting.

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.

------
danielweber
You should demand your money back.

