Bug 79694

(In reply to comment #0)
> Suppose we have two FD's in the loader:

I think this is a potentially dangerous oversimplification, so I'm going to work through the logic with larger numbers.

Suppose we have 5 fds: the first message is meant to contain 2 and the second 3:

n_unix_fds = 2
message has 0 fds {}
loader has 5 fds { 11, 12, 21, 22, 23 }

We copy the first 2 fds into the message:

n_unix_fds = 2
message has 2 fds { 11, 12 }
loader has 5 fds { 11, 12, 21, 22, 23 }

and what we want to do is to shift unix_fds[2]..unix_fds[4], inclusive, down to unix_fds[0]..unix_fds[2], inclusive.

There are actually *two* bugs here. One is that we're trying to shift [0]..[2] upwards, not the other way round; so you would expect we would get { 11, 12, 21, 11, 12, [heap buffer overflow] }.

The other bug, which is what prevents this from being a buffer overflow, is that we're incorrectly copying loader->n_unix_fds *bytes*, not loader->n_unix_fds *ints*.

(The buffer overflows if

    n_unix_fds * 4 + loader->n_unix_fds > max_message_unix_fds * 4

but since we know that n_unix_fds + loader->n_unix_fds <= max_message_unix_fds, that's impossible. Strictly speaking that "4" should be sizeof(int), but the same relationships remain true.)

> memmove(loader->unix_fds, loader->unix_fds + n_unix_fds, loader->n_unix_fds);
> 
> This patch has worked succesfully for me.

I think that's only because you happen to be on a little-endian platform, fds are small integers (< 256, so the first byte of little-endian representation is enough), and you're only moving one fd.

Created attachment 100875 [details] [review]
If the loader contains two messages with fds, don't corrupt  the second

There were two bugs here: we would previously overwrite the unused
fds with the already-used fds instead of the other way round, and
we would copy n bytes where we should have copied n ints.

Where's GArray when you need it?

(In reply to comment #2)
> Created attachment 100875 [details] [review] [review]
> If the loader contains two messages with fds, don't corrupt  the second
> 
> There were two bugs here: we would previously overwrite the unused
> fds with the already-used fds instead of the other way round, and
> we would copy n bytes where we should have copied n ints.
> 
> Where's GArray when you need it?

The patch looks good to me.

I reproduced the bad behaviour when testing your scenario described in comment #1:

> Suppose we have 5 fds: the first message is meant to contain 2 and the
> second 3:

And the issue is gone with your patch. I tested the patch applied on top of the Debian package dbus-1.8.4-1.

Fixed in dbus 1.6.22, 1.8.6.

This bug also caused a security vulnerability, although it's a denial of service, not the buffer overflow I was initially worried about. See Bug #80469 for that.