A serious security bug... caused by no bounds checking.

[Kagamin]

On Friday, 11 April 2014 at 21:58:13 UTC, Marco Leise wrote:
>> byte[] packet = recieve();
>> int length = get_payload_length(packet);
>> dest[0..length] = packet[3..3+length];
>
> I'd argue that at this point you already knew you would mess
> up the heartbeat code and designed it directly around D's
> bounds checks instead of using structs and direct access of
> header fields. It's a clever solution that you propose here.
> Have you used it on real code before (i.e. does it scale) or
> did you come up with it just for this case?

Well, I didn't write openssl in D, so this code is clearly made 
up for this case. But I don't circumvent bounds checking in my 
code. I don't know, how well it scales, I never needed such 
microoptimizations, the speed is either acceptable or not, and I 
have never seen bounds checking to promote the code from the 
former class to the latter. Though, I don't think openssl 
developer didn't want bounds checking in their code, they used 
memcpy to copy the payload. There's memcpy_s function with bounds 
checking, but it's standardized only as an optional extension to 
C11, so they were just out of luck to realistically require it.

This code is not specialized for this particular buffer overrun 
bug, slices account for all buffer overrun bugs, wherever they 
are. This is the whole point in language integrated bounds 
checks: to make their use simple. I didn't invent anything new, 
it's *the* D way to handle buffers as slices, pointers are 
usually not used:
http://dlang.org/phobos/std_stdio.html#.File.rawRead
http://dlang.org/phobos/std_socket.html#.Socket.receive
In fact, openssl didn't use structs to parse the client packet, 
they used raw byte pointer:

hbtype = *p++; //read type
n2s(p, payload); //read network-order short and increment pointer 
by 2
pl = p;
memcpy(bp, pl, payload);

and equivalent D code would use slice.