Bug 79694 - Misbehaviour reading a message with passing file descriptors (duplicate/missing fds)
Summary: Misbehaviour reading a message with passing file descriptors (duplicate/missi...
Status: RESOLVED FIXED
Alias: None
Product: dbus
Classification: Unclassified
Component: core (show other bugs)
Version: unspecified
Hardware: All Linux (All)
: medium normal
Assignee: Simon McVittie
QA Contact:
URL:
Whiteboard: review+
Keywords: patch
Depends on:
Blocks:
 
Reported: 2014-06-05 17:47 UTC by Alejandro Martínez Suárez
Modified: 2014-07-02 16:06 UTC (History)
5 users (show)

See Also:
i915 platform:
i915 features:


Attachments
If the loader contains two messages with fds, don't corrupt the second (1.16 KB, patch)
2014-06-11 11:26 UTC, Simon McVittie
Details | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Alejandro Martínez Suárez 2014-06-05 17:47:55 UTC
This problem is related to file descriptor passing in Unix systems. It affects all versions since this functionality was introduced.

In source file 'dbus/dbus-message.c', 'load_message' function, if we have more than one file descriptors inside the loader and they are from different messages, when we are extracting them to compose the actual message, a misbehaviour uprises.

The problem is with this line (dbus/dbus-message.c:4207) of 1.8.X branch:

memmove(loader->unix_fds + n_unix_fds, loader->unix_fds, loader->n_unix_fds);

It pretends to move the array of FD's after extracting the current message ones, but the command has the source and destination arguments swapped.

Suppose we have two FD's in the loader:

 (35,36) loader->n_unix_fds = 2

The message we are composing uses one FD (the 35). We assign it to the message, and after executing the previous command, we end with this info in the loader:

 (35,35) loader->n_unix_fds = 1

We should have ended with:

 (36, ) loader->n_unix_fds = 1

The problem is that we are duplicating the id of the first file descriptor in the position of the second one. Fix it is simple, we only need to interchange them:

memmove(loader->unix_fds, loader->unix_fds + n_unix_fds, loader->n_unix_fds);

This patch has worked succesfully for me. I have tested it on a Suse 11, and my problems with duplicated file descriptors (because two different messages where sharing the same file descriptor) are finally gone :).
Comment 1 Simon McVittie 2014-06-11 11:20:09 UTC
(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.
Comment 2 Simon McVittie 2014-06-11 11:26:38 UTC
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?
Comment 3 Alban Crequy 2014-06-23 15:04:01 UTC
(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.
Comment 4 Simon McVittie 2014-07-02 16:06:16 UTC
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.


Use of freedesktop.org services, including Bugzilla, is subject to our Code of Conduct.