So just recently a vulnerability in OpenSSL’s heartbeat extension was found which was quickly dubbed –Heartbleed. In essence, this bug allows a malicious attacker to read more than an allowed chunk of memory that might contain sensitive information from other sessions i.e. the heart beat extension bleeds memory therefore heartbleed. Its a programming error which is surprisingly more common than we might like to believe. Anyway, so I decided to dig around a little and see if I can find the offending code. On the security advisory on openssl.org it mentions that this vulnerability is in versions until 1.0.1f, the later version 1.0.1g supposedly fixes it. So I downloaded the source code for both versions and did a diff check and found the following function in 1.0.1f (ssl/d1_both.c), you can also see my comments inlined in block quotes (assuming this is the actual function that has the bug):
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 */ /* Read type and payload length first */ hbtype = *p++; n2s(p, payload); pl = p; //do something with the payload if (s->msg_callback) s->msg_callback(0, s->version, TLS1_RT_HEARTBEAT, &s->s3->rrec.data[0], s->s3->rrec.length, s, s->msg_callback_arg); if (hbtype == TLS1_HB_REQUEST) { unsigned char *buffer, *bp; int r; /* Allocate memory for the response, size is 1 byte * message type, plus 2 bytes payload length, plus * payload, plus padding */ buffer = OPENSSL_malloc(1 + 2 + payload + padding); //allocate all that memory without any checks bp = buffer; /* Enter response type, length and copy payload */ *bp++ = TLS1_HB_RESPONSE; s2n(payload, bp); memcpy(bp, pl, payload); bp += payload; /* Random padding */ RAND_pseudo_bytes(bp, padding); r = dtls1_write_bytes(s, TLS1_RT_HEARTBEAT, buffer, 3 + payload + padding); //send the response back, even the stuff the attacker wasn't supposed to see if (r >= 0 && s->msg_callback) s->msg_callback(1, s->version, TLS1_RT_HEARTBEAT, buffer, 3 + payload + padding, s, s->msg_callback_arg); OPENSSL_free(buffer); if (r tlsext_hb_seq) { dtls1_stop_timer(s); s->tlsext_hb_seq++; s->tlsext_hb_pending = 0; } } return 0; }
Now, I am not an OpenSSL expert so I am not fully aware of what’s going on but I think the bold lines in the above method, show that there are no bounds check on the amount of memory that gets allocated for the heartbeat response that gets sent to the heartbeat requestor. In particular, the line:
buffer = OPENSSL_malloc(1 + 2 + payload + padding)
assumes that the payload is within bounds whereas that’s exactly whats wrong with it. The attacker sends a payload that tells the heartbeat extension (falsely) how big is it and the heartbeat extension just believes it and gives it a memory chunk the same size the attacker’s asked for in the payload. In a benign situation, the payload would have been 1 or 2 bytes long but a malicious attacker would trojanify the payload to look like as if its 64k long.
In the other boldened lines of code, it never does a bounds check either now I am not sure if that’s really needed or not because I am not familiar with the codebase or the architecture.
However, in version 1.0.1g, they seem to have added some checks:
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 */ if (s->msg_callback) s->msg_callback(0, s->version, TLS1_RT_HEARTBEAT, &s->s3->rrec.data[0], s->s3->rrec.length, s, s->msg_callback_arg); /* Read type and payload length first */ if (1 + 2 + 16 > s->s3->rrec.length) return 0; /* silently discard */ hbtype = *p++; n2s(p, payload); if (1 + 2 + payload + 16 > s->s3->rrec.length) return 0; /* silently discard per RFC 6520 sec. 4 */ pl = p; if (hbtype == TLS1_HB_REQUEST) { unsigned char *buffer, *bp; unsigned int write_length = 1 /* heartbeat type */ + 2 /* heartbeat length */ + payload + padding; int r; if (write_length > SSL3_RT_MAX_PLAIN_LENGTH) return 0; /* Allocate memory for the response, size is 1 byte * message type, plus 2 bytes payload length, plus * payload, plus padding */ buffer = OPENSSL_malloc(write_length); bp = buffer; /* Enter response type, length and copy payload */ *bp++ = TLS1_HB_RESPONSE; s2n(payload, bp); memcpy(bp, pl, payload); bp += payload; /* Random padding */ RAND_pseudo_bytes(bp, padding); r = dtls1_write_bytes(s, TLS1_RT_HEARTBEAT, buffer, write_length); if (r >= 0 && s->msg_callback) s->msg_callback(1, s->version, TLS1_RT_HEARTBEAT, buffer, write_length, s, s->msg_callback_arg); OPENSSL_free(buffer); if (r tlsext_hb_seq) { dtls1_stop_timer(s); s->tlsext_hb_seq++; s->tlsext_hb_pending = 0; } } return 0; }
Similar kind of function exists in /ssl/t1_lib.c as well with similar differences. I just thought to post this code because no one else has yet, I think.
Thanks for digging around! If anyone had posted the code elsewhere, it’s since been obscured in search results by the usual, superficial media frenzy.
Tangentially, similar attacks have existed at the MAC layer, as observed in the implementation of certain Ethernet device drivers: http://cve.mitre.org/cgi-bin/cvename.cgi?name=CVE-2003-0001.
As ever, it seems coding discipline, alone, is insufficient to catch such bugs, and this becomes no easier for large projects, including OpenSSL.
I thought that too but digging through to the 6th or 7th page in of Google search results and not finding anything convinced me enough that people are too busy panicking at the moment to think about code 🙂
I agree to some extent because these kinds of errors are more common than you or I would like to see. Heck, I will raise my hand and admit to making such mistakes albeit in a non security related system. I think it’s very easy to get lost into the implementation nitty gritty and completely forget about bounds check. All we as programmers can do is be doubly sure about edge cases and test a lot. I am sure if this had been tested thoroughly they would have found this error glaring in their faces. 🙂
Thanks so much for showing where the problem existed! I had wondered why there were no checks. At least now I can see it for myself with a very good breakdown.
You are welcome! I know once you look at the code the fix seems very obvious and too simple to have missed out. But like I said these kind of mistakes are easy to make an it takes a lot active attention to detail to catch them before they come out so publicly. I see these kinds of mistakes more often than I would like to and in the code of one of the largest software vendors in the world. I am glad my post helped you understand th problem more clearly. Thanks for dropping by.
Reblogged this on Zombie Code Kill and commented:
Thanks to code quirks n rants. This is a great example of code with poor readability. Unfortunately this sort of code is all too common in the world of C programming.
Poor readability is not exclusive to C, I have seen even worse code in C#, Java, PHP etc as well. But I agree to the fact that C’s syntax and library makes it really easy to write illegible code.
Yes, every language has many examples of illegible code. A few languages are designed with readability prioritised over performance, and while they can’t prevent developers from writing bad code they help in some areas. C and C++ are designed more for speed and power, so good coding guidelines and good unit tests are more important than ever.