[Cryptography] Heartbleed and fundamental crypto programming practices

[Jerry Leichter]

On Apr 11, 2014, at 7:43 PM, Jan Pechanec <jan.pechanec at oracle.com> wrote:
> 	Jerry, what you described is not how the Heartbleed works.  The 
> problem is not with allocation of the buffer and that it was not 
> cleared, the problem is that memory past the input buffer is copied into 
> the buffer allocated for the reply.  And that source memory could be 
> still used and could contain what you call RedStrings, also in use.  
> The whole output buffer is overwritten here.  The problem is missing 
> bound checking, not the memory allocation.
> 
> 	see the fix for yourself:
> 
> http://git.openssl.org/gitweb/?p=openssl.git;a=commitdiff;h=731f431497f463f3a2a97236fe0187b11c44aead;hp=4e6c12f3088d3ee5747ec9e16d03fc671b8f40be
The fix doesn't reveal where the problem was.  Both old and new code use OPENSSL_MALLOC() to allocate N bytes, and 30 or so lines later, both use dtls1_write_bytes() to write N bytes.  The same N in both cases, based on the internal data length the requester specified.

It appears that the actual fix is in validating that the internal data length is no longer than the buffer in which it is supposed to reside.

This diff doesn't show how much data is *copied*, but at this point the damage is already done when N is computed as above:

1.  If what's copied is based on the bogus internal length, then the outgoing buffer will receive whatever is in memory after the received buffer.

2.  If what's copied is based on the actual received buffer length, then when an attacker specifies a bogus large internal, most of the buffer is never over-written after being malloc'ed, and the attack receives a slice of just-allocated memory, with happened to be in it.

Either way, the attacker gets stuff he shouldn't be able to see.  And, yes, if the failure happens to be of type 1, then clearing sensitive data from memory before freeing it doesn't help.  (More properly, doesn't help *as much* - the memory beyond the buffer will often contain a mix of in-use and free memory, and if the free memory doesn't contain valuable, well, sending it isn't leaking anything.)

I must say, looking just at this brief snippet of code, that neither version would have passed my code review *even if it weren't security-sensitive code*.  Some places use 1 + 2 + length; others comment on what the constants are; in the old code, some places use 3 + length, I guess because we don't want the compiler to be too stressed out doing constant folding.  padding is set to 16, but the validity check (which is counting the size of the padding) uses an explicit 1 + 2 + 16.  We have two lengths floating around - the length of the buffer and the length of the data inside the buffer - but nothing gives you a hint about which is which.  If you're extracting data with a length from a surrounding structure with a length (like a buffer - but TLV fields often nest recursively), you should establish a convention for naming the two lengths and use stylized code to *always* validate one against the other.  (Depending on the overall structure of the code, you might have a common function to handle that.  There are many good patterns, but leaving it up to each developer of each individual function to come up with his own approach each time guarantees that someone will eventually screw it up.)

The new code is an improvement in a number of ways, but as a basis for security of a large part of the Internet ... this thing scares me.

(And yes, I've looked at plenty of open source - and non-open-source - code over many years, and I know this is *significantly better* than much of what's out there.  But "no worse than the other guy's" is hardly a good explanation - except when it comes to the *economics* of why this kind of code succeeds in the marketplace.)
                                                        -- Jerry