Am Fri, 11 Apr 2014 11:38:54 +0000
schrieb "Kagamin" <spam@here.lot>:

> On Thursday, 10 April 2014 at 07:14:10 UTC, Marco Leise wrote:
> > Am Thu, 10 Apr 2014 06:51:40 +0000
> > schrieb "w0rp" <devw0rp@gmail.com>:
> >
> >> On Wednesday, 9 April 2014 at 12:36:49 UTC, Marco Leise wrote:
> >> > Sorry, but wasn't this security risk instead caused by uninitialized memory, and shouldn't you instead have said:
> >> >
> >> > "I'm glad to be using a language with default initialization?"
> >> 
> >> Nope, it was caused by missing bounds checking.
> >> 
> >> https://www.openssl.org/news/secadv_20140407.txt
> >> 
> >> > A missing bounds check [...]
> >
> > Haha, I tried to read that about an hour ago to inform myself, but it still doesn't load for me.
> 
> http://blog.existentialize.com/diagnosis-of-the-openssl-heartbleed-bug.html
> The server copies data received from the client and sends it
> back, the length is specified (or forged) by the client,
> everything is initialized just fine.

Ah, so this is a typical ping packet, where you copy all
payload bytes from the client's packet and send them back.
Just that in this case the client can write anything into
the length header and OpenSSL would try to copy as many bytes
from the client provided packet into the answer packet, even if
that means reading beyond the end of that packet.

This still doesn't touch D's array bounds checking at all, because the array pointer and length come from an unreliable source to begin with!

-- 
Marco

On Fri, 11 Apr 2014 12:21:06 -0400, Marco Leise <Marco.Leise@gmx.de> wrote:

> Am Fri, 11 Apr 2014 11:38:54 +0000
> schrieb "Kagamin" <spam@here.lot>:

>> http://blog.existentialize.com/diagnosis-of-the-openssl-heartbleed-bug.html
>> The server copies data received from the client and sends it
>> back, the length is specified (or forged) by the client,
>> everything is initialized just fine.
>
> Ah, so this is a typical ping packet, where you copy all
> payload bytes from the client's packet and send them back.
> Just that in this case the client can write anything into
> the length header and OpenSSL would try to copy as many bytes
> from the client provided packet into the answer packet, even if
> that means reading beyond the end of that packet.
>
> This still doesn't touch D's array bounds checking at all,
> because the array pointer and length come from an unreliable
> source to begin with!

But in a D-created struct, the data would be an array, instead of a ptr+length.

-Steve

On Friday, 11 April 2014 at 16:17:49 UTC, Marco Leise wrote:
> This still doesn't touch D's array bounds checking at all,
> because the array pointer and length come from an unreliable
> source to begin with!

In D implementation the client packet would be reliably confined by a slice, so the forged length will be checked against packet boundaries.

byte[] packet = recieve();
int length = get_payload_length(packet);
dest[0..length] = packet[3..3+length];

> But in a D-created struct, the data would be an array, instead of a ptr+length.
> 
> -Steve

If I understand you right, you mean a variation of this:

struct Packet { ubyte[] payload; }

But indirections don't fly with serialized network packets.


Am Fri, 11 Apr 2014 17:35:15 +0000
schrieb "Kagamin" <spam@here.lot>:

> In D implementation the client packet would be reliably confined by a slice, so the forged length will be checked against packet boundaries.
> 
> 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?

-- 
Marco

On Fri, 11 Apr 2014 18:01:29 -0400, Marco Leise <Marco.Leise@gmx.de> wrote:

>> But in a D-created struct, the data would be an array, instead of a
>> ptr+length.
>>
>> -Steve
>
> If I understand you right, you mean a variation of this:
>
> struct Packet { ubyte[] payload; }
>
> But indirections don't fly with serialized network packets.

Indirections were in the struct that was the subject of that article. Here it is:

typedef struct ssl3_record_st
    {
        int type;               /* type of record */
        unsigned int length;    /* How many bytes available */
        unsigned int off;       /* read/write offset into 'buf' */
        unsigned char *data;    /* pointer to the record data */
        unsigned char *input;   /* where the decode bytes are */
        unsigned char *comp;    /* only used with decompression - malloc()ed */
        unsigned long epoch;    /* epoch number, needed by DTLS1 */
        unsigned char seq_num[8]; /* sequence number, needed by DTLS1 */
    } SSL3_RECORD;

No way that's a directly serialized network packet.

-Steve

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.

Am Sat, 12 Apr 2014 12:21:55 +0000
schrieb "Kagamin" <spam@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

Am Fri, 11 Apr 2014 22:52:23 -0400
schrieb "Steven Schveighoffer" <schveiguy@yahoo.com>:

> On Fri, 11 Apr 2014 18:01:29 -0400, Marco Leise <Marco.Leise@gmx.de> wrote:
> 
> >> But in a D-created struct, the data would be an array, instead of a ptr+length.
> >>
> >> -Steve
> >
> > If I understand you right, you mean a variation of this:
> >
> > struct Packet { ubyte[] payload; }
> >
> > But indirections don't fly with serialized network packets.
> 
> Indirections were in the struct that was the subject of that article. Here it is:
> 
> typedef struct ssl3_record_st
>      {
>          int type;               /* type of record */
>          unsigned int length;    /* How many bytes available */
>          unsigned int off;       /* read/write offset into 'buf' */
>          unsigned char *data;    /* pointer to the record data */
>          unsigned char *input;   /* where the decode bytes are */
>          unsigned char *comp;    /* only used with decompression -
> malloc()ed */
>          unsigned long epoch;    /* epoch number, needed by DTLS1 */
>          unsigned char seq_num[8]; /* sequence number, needed by DTLS1 */
>      } SSL3_RECORD;
> 
> No way that's a directly serialized network packet.
> 
> -Steve

If "data" and "length" are the pointer and size of the raw network packet, yes. I.e. it will turn into:

ssl3_record_st.data[] = socket.receiveRawPacket();

... which is safe. But when you said "D created struct" I assumed you referred to the network packet itself as a struct, which means that the length field would come directly from the client and doesn't need to match the physical length of the packet. With Kagamin's reply I now understand it better.

-- 
Marco

On Saturday, 12 April 2014 at 19:03:58 UTC, Marco Leise wrote:
> 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?

You can process fixed-length message with structs, but processing variable-length message requires access to the buffer with packet, as we see in the example of ssl heartbeat.

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

How is it different from memcpy? It's conceptually copying too.
with slice: read(buffer[0..length]);
with pointer: read(buffer, length);

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

I think, memcpy is messier than slice copy. It will be clearly visible, that you circumvent bounds checking, and that will smell, and everyone looking at it will tell you what they think about it.

Marco Leise:

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

In most cases D allows you to find a way to write your code in a reasonably nice and reasonably safe way. You just have to be in the right mind frame.

Bye,
bearophile