On reading about the Heartbleed bug bug, my first thoughts were:

  1. This sounds quite simple, is it really easy to spot?
  2. Why wasn’t this noticed during review?

The bug is in the Heartbeat Extension for TLS, as described in this draft. Interestingly, this document does actually highlight the importance of checking the payload length:

If payload_length is either shorter than expected and thus indicates padding in a HeartbeatResponse or exceeds the actual message length in any message type, an error Alert message using illegal_parameter as its AlertDescription MUST be sent in response

It also says ‘This document does not add any additional security considerations’, but I wouldn’t expect programming mistakes to be listed there anyway. So it looks like the initial draft is sound.

The actual commit that introduced the bug is online too; you can find it here. Now I don’t really know C, so I don’t think the bug is immediately obvious, even to competent programmers.

unsigned int payload;
unsigned int padding = 16; /* Use minimum padding */

/* Read type and payload length first /
hbtype = p++;
n2s(p, payload);
pl = p;


if (s->msg_callback)
s->msg_callback(, s->version, TLS1_RT_HEARTBEAT,
&s->s3->rrec.data[], s->s3->rrec.length,
s, s->msg_callback_arg);


if (hbtype == TLS1_HB_REQUEST)
{
unsigned char buffer, bp;
int r;


    <span class="cm">/* Allocate memory for the response, size is 1 bytes</span>



         * message type, plus 2 bytes payload length, plus
         * payload, plus padding
         */
buffer = OPENSSL_malloc(1 + 2 + payload + padding);
bp = buffer;


    <span class="cm">/* Enter response type, length and copy payload */</span>
    <span class="o">*</span><span class="n">bp</span><span class="o">++</span> <span class="o">=</span> <span class="n">TLS1_HB_RESPONSE</span><span class="p">;</span>
    <span class="n">s2n</span><span class="p">(</span><span class="n">payload</span><span class="p">,</span> <span class="n">bp</span><span class="p">);</span>
    <span class="n">memcpy</span><span class="p">(</span><span class="n">bp</span><span class="p">,</span> <span class="n">pl</span><span class="p">,</span> <span class="n">payload</span><span class="p">);</span>

    <span class="n">r</span> <span class="o">=</span> <span class="n">ssl3_write_bytes</span><span class="p">(</span><span class="n">s</span><span class="p">,</span> <span class="n">TLS1_RT_HEARTBEAT</span><span class="p">,</span> <span class="n">buffer</span><span class="p">,</span> <span class="mi">3</span> <span class="o">+</span> <span class="n">payload</span> <span class="o">+</span> <span class="n">padding</span><span class="p">);</span>

However, I’d still expect C programmers to find it, given a decent review. And even without knowing C, it has issues:

  1. The payload length is stored in a variable called payload. This is stupid naming. A variable called payload should contain the payload.
  2. The payload is actually stored in pl and bp.
  3. I can’t see any reference/comment to checking the payload length as in the above draft.

It’s not obvious to spot; I’ve certainly missed things like this in the past. But it’s still poor coding, doesn’t stick to the original draft, and was inadequately reviewed.