Wow. Was it really that simple? The heart beat request sends the text as well as the length it wants back?
Close enough.
> The heart beat request sends the text as well as the length it wants back?
The heartbeat sends a payload prefixed by its size. That's perfectly normal design (for variable-size payloads), that way the handler reads the size, allocates a buffer[-1] and uses read(2) to read the payload into the buffer. Otherwise the handler would have to "guess" the payload size, and that never ends well.
The problem here is twofold:
1. read(2) may read less than requested, if an attacker gave a bigger size than the actual one for instance. That's why read(2) returns the number of bytes actually read
2. malloc(3) hands out a bunch of memory, without clearing it[0]. Depending on the exact allocator and application runtime, chances are this bit of memory is at least in part freed memory, which is filled with the content of previous allocations such as SSH keys or passwords or whatever
(2.) is compounded by OpenSSL having its own freelists on top of malloc which it does not clear, making it certain to hit previously allocated data
You're supposed to check the result of read(2) and adjust your payload size and only copy that to the output buffer. Or just error out if the sizes differ.
And ideally unless you have very specific reasons not to you'd want to use calloc(3), so that if you forget to check read(2) you return zeroed memory anyway. The first part was forgotten and the second one not done (because "needs fasts!"), the whole input buffer was copied in the output buffer and an attacker gets 64kb[1] worth of previous allocations data.
[-1] possibly adding its own constraints on top of that, here the payload's 64KiB so it's not relevant, in other contexts the server could refuse overly large payloads
[0] except on BSD with a malloc.conf using the J or Z options
[1] because the user-provided length is a 16 bit uint
e.g typing `man 2 read` into the shell on a Unix system will give you information about the read system call.
2's are for system calls, 3's are for members of the C standard library (usually).
If the input buffer was guaranteed to be large enough in this particular case I don't know but I can imagine an implementation that does not allocate the buffer for the 64 kiB worst case but just large enough to contain the actual request.
It was, since it used the same size to allocate the buffer and call read(2) it would never put more data than expected in the buffer.
> otherwise you can still hit whatever follows your input buffer.
Yes, but that's not the issue in heartbleed. My comment was about heartbleed, not about covering all the ways in which you can fuck up memory access in C.
> I can imagine an implementation that does not allocate the buffer for the 64 kiB worst case but just large enough to contain the actual request.
"just large enough" is impossible, you'll always over-allocate by at least 1 byte, and then to get the actual best precision you have to read the input data a byte at a time, performing a read(2) per byte. That's both slow and less readable.
You can do this. First allocate for and read the fixed length part, then determine the length of variable length part and finally allocate for and read the variable length part. This may of course still return less data then expected and leave you with uninitialized memory. And you may of course receive a larger buffer then you asked for.
Yes, but that's not the issue in heartbleed. My comment was about heartbleed [...]
Of course, I just wanted to say that zeroing memory may not be sufficient in the general case without bound checking because sometimes people have or get the impression that this would be a good and easy fix.
That's the part you can't do, you're reading data from a socket, you can't skip around with fseek(3), you read(2) or you recv(2) and if you don't store your data somewhere you lose it.
OpenSSL correctly reads the whole packet using the SSL record length. It then passes the packet off to the `tls1_process_heartbeat` function, which uses the second length field to do (variable names changed because the originals were terrible):
outgoing_packet = malloc(heartbeat_len)
memcpy(outgoing_packet, incoming_packet, heartbeat_len)
Edit: As a shortcut to establish my bona fides, I wrote a honeypot for this issue so I ought to know what I'm talking about:
https://gist.github.com/takeshixx/10107280
On line 42 is the heartbeat request payload. 40 is the length of the message in hex = 64 bytes.
https://news.ycombinator.com/item?id=7555156
Such that "FF FF" will be 65535 (the 64kb everyone keeps talking about)
You have the option to specify the length implicitly by using a terminator like a zero byte in a zero delimited string but this is sometimes a bad idea. First you have to scan the entire field just to figure out its length while with an explicitly stored length before the actual data you can just read the length and for example skip to the next field without ever looking at the field. Forgetting to properly terminate the field will of course get you in serious trouble, too. And last but not least using a terminator only works if you have an used symbol (or are willing to perform escaping). In the case of the heartbeat extension there are no restrictions on the content and you can put in arbitrary binary data and therefore something simple like a zero terminator does not work.
Is there a trend to leave definitions of these protocols as loose as possible (eg not specifying the response field in the heartbeat as a double/long or whatever) so as to allow hacking it to serve some other use later?
Guess I'm going to have to look at the DTLS protocol definitions ...
It is probably a really bad idea to make your protocol depend on details of an underlying protocol layer. Just imagine sending something over Ethernet and receiving it at the other end over WLAN where both protocols may disagree on the maximum packet size. You usually want your standard as precise as possible without imposing unnecessary and artificial constraints. Being allowed to stuff undefined amounts of data into a heart beat packet just to ease abusing the protocol really seems to call for trouble.
Here's an analysis of the actual OpenSSL code:
http://blog.existentialize.com/diagnosis-of-the-openssl-hear...
Here's the patch itself:
https://github.com/openssl/openssl/commit/96db9023b881d7cd9f...