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

[Marco Leise]

Am Sat, 12 Apr 2014 12:21:55 +0000
schrieb "Kagamin" <spam at here.lot>:

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

Ah! Big misunderstanding. I meant to ask if you used this
idiom before, that you use just a byte[] for data you
load/receive to be able to use bounds checks and not some
header struct with pointers where they become ineffective?

And with scale I was aiming for the software architecture. Can
you keep this code style with global functions working on
byte[] instead of structs or will you wish for the style used
in OpenSSH, which is unsafe but probably easier to maintain?

> 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

Because rawRead and receive are meant to read N bytes. Perfect
match for a slice. In a library like OpenSSH you know what
your headers will look like, though.

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

Ok, that half-way convinced me. :)

hbtype = buffer[0]; // read type, ok
// but this??? ugh!!!
payload = bigEndianToHost(*cast(short*)buffer[1 .. 3].ptr);
bp[0 .. payload] = buffer[3 .. 3+payload];

The code becomes pretty messy in my eyes. I would have just
used memcpy as well, if that was the alternative.
That's why I asked if it will scale or if you just made it up
for the sake of argument.

-- 
Marco