If you can’t dance, don’t blame the dance floor.

Heartbleed, as you’ve probably read by now, is a serious security issue resulting from a bug in OpenSSL which allows an attacker to freely read the memory of the affected servers. Serious 💩, indeed.

Some people were quick to point their fingers at the language used for implementation of the SSL standard in the OpenSSL project—that is to say, C—and are clamoring for a new implementation in what they deem a safer language, such as Rust and Go.

Now, C is an old language and in many aspects it’s little more than assembly language dressed up. It has sharp edges and people cut themselves on those very often — dealing explicitly with memory allocation and pointers is prone to error, even for experienced programmers. I agree that having to deal with all the minutiae is cumbersome and usually unnecessary in the modern day. However, I believe the problem isn’t so much that the language allows you to shoot yourself in the foot but that many programmers are careless.

Let’s have a look at the code before the fix:

int
dtls1_process_heartbeat(SSL *s)
    {
    unsigned char *p = &s->s3->rrec.data[0], *pl;
    unsigned short hbtype;
    unsigned int payload;
    unsigned int padding = 16; /* Use minimum padding */

Rather boring variable declaration, although the names could be better (what kind of mischievous monster names a length variable payload?).

That SSL type is rather opaque, but following the trail, one ends up with this structure:

typedef struct ssl3_record_st
    {
        int type;               /* type of record */
        unsigned int length;    /* How many bytes available */
        unsigned int off;       /* read/write offset into 'buf' */
        unsigned char *data;    /* pointer to the record data */
        unsigned char *input;   /* where the decode bytes are */
        unsigned char *comp;    /* only used with decompression - malloc()ed */
        unsigned long epoch;    /* epoch number, needed by DTLS1 */
        unsigned char seq_num[8]; /* sequence number, needed by DTLS1 */
    } SSL3_RECORD;

Anyway, the argument s is read as a SSL3 record and char p is left pointing to s’s data. So far, nothing unusual or interesting. Onwards!

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

Ok, let’s go through this slowly.

p is of type char, so they’re jumping over the first byte and copying the first two bytes to hbtype (which is a short). This is because the heartbeat extension sticks this stuff into the data portion of SSL3_RECORD, so some gymnastics is needed to access it.

n2s and s2n are defined in ssl/ssl_locl.h as follows:

#define n2s(c,s)	((s=(((unsigned int)(c[0]))<< 8)| \
			    (((unsigned int)(c[1]))    )),c+=2)
#define s2n(s,c)	((c[0]=(unsigned char)(((s)>> 8)&0xff), \
			  c[1]=(unsigned char)(((s)    )&0xff)),c+=2)

n2s is supposed to read two chars and return them inside an integer (or anything with at least 2 bytes). What it actually does is read two of whatever the c pointer says its size is and return the result of arithmetical OR between them. A pedantic detail but dangerous if misused. I suppose strong type checking could be useful here to prevent mistakes.

s2n is similar but does the opposite—dump two chars into an integer.

So, we read a couple of bytes, convert them to a short and assign that to payload, which is the payload length.

Then pl is changed to point to the payload data, including the two bytes indicating its supposed length.

What’s becomes apparent if you’re following the code carefully is that the heartbeat payload length hasn’t been checked for validity yet and, as we’ll later see, isn’t before it’s used. This is probably the most common kind of error in C, which makes it somewhat more appalling because it should be at the forefront of the programmer’s mental checks.

Later on, there’s this:

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

        /* Allocate memory for the response, size is 1 bytes
         * message type, plus 2 bytes payload length, plus
         * payload, plus padding
         */
        buffer = OPENSSL_malloc(1 + 2 + payload + padding);
        bp = buffer;

        /* Enter response type, length and copy payload */
        *bp++ = TLS1_HB_RESPONSE;
        s2n(payload, bp);
        memcpy(bp, pl, payload);
        bp += payload;

First a buffer for the response is allocated, then a region of memory is copied. Both their sizes depend on the payload length received earlier and it wasn’t bound-checked!

This allows an attacker to send a heartbeat with an arbitrary payload length (up to 64KB (2¹⁶ bytes) of memory) and a much smaller payload body, leading to reads beyond the SSL record. 64KB seems small but it’s enough to get SSL certificates, passwords, cookies, whatever is reachable.

I’m not entirely sure that this mistake is of the sort that a cleverer language would catch, since it’s likely that the memcpy will happen within the application’s memory. If it tried to reach outside, the kernel would take notice and kill it with a Segmentation Fault, turning this issue into a less threatening program termination bug.

I know I shouldn’t be surprised at how trusting most programmers are about user input, but these people are programming a cryptography library. They should know better. Scratch that, everyone trusts them to know better and to be paranoid. To say I’m disappointed is putting it lightly.

While I agree that it would benefit everyone if a safer language was used for this sort of software, I do not agree that the blame lies entirely with C. Careless mistakes can happen with any language. The language used may reduce the likelihood that a mistake comes up in places with serious consequences but vigilance is absolutely essential.