Comment by masklinn | Hacker News Reader

Comment by masklinn | Hacker News Reader

Parent Full thread

> Wow. Was it really that simple?

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

I think you have misunderstood the bug, actually. It has nothing to do with read(2) returning less data than requested. There are two length fields at play here, the SSL record layer length field, and a second length field in the heartbeat field. Both are 16 bits.

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)

So when the two length fields disagree, SSL's network code correctly reads the short packet, and the heartbeat code incorrectly reads past the end of the incoming packet buffer and copies it into the outgoing packet buffer.

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://twitter.com/moyix/status/453760960671383552

Zeroing memory does not help in the general case unless you allocate an input buffer large enough that you can never overrun it; otherwise you can still hit whatever follows your input buffer.

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.

> If the input buffer was guaranteed to be large enough in this particular case

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.

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

> then determine the length of variable length part

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.

Allocate a fixed length buffer for the header and read the header, inspect the length field and allocate a second buffer with this length and finally read the variable length part into this second buffer. Maybe check that there is no trailing data. This is what you would probably do anyway if the variable length part could be way larger then 64 kiB and just blindly allocating for the maximum length is not an option.

Right. Now you've got significantly more code (and even more chances of getting it wrong), more allocations, and you can still fail to correctly handle read(2) not completely filling a buffer and leaking data. Although most likely less than in heartbleed.

How did we end up here? My initial point was that zeroing memory does not prevent leaks in the general case and we both agree on that. I never claimed OpenSSL should or should not have done something differently.

Haha. Your comment is so complex it needs another XKCD comic just to make sure all those [-1], (2), 1., [1]'s etc. don't get mixed up.

That's because it was the in-depth, technical explanation of the comic. Based on the GP's comment, the comic is exactly accurate.

If it helps, the numbers in parantheses after read, malloc, calloc etc. are referring to the page number in the man pages.

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

Here you go: http://xkcd.com/1208/

An excellent explanation. Thanks.

Reply on news.ycombinator.com