Bug 80163

I'm not sure I understand what the problem is. Is someone trying to pass the file descriptor of the D-Bus connection itself?

Created attachment 101289 [details]
dbus-dos-recursive-fdpass.jpg

(In reply to comment #1)
> I'm not sure I understand what the problem is. Is someone trying to pass the
> file descriptor of the D-Bus connection itself?

No, fds of D-Bus connections are not passed around. I am attaching a schema to show this.

The malicious program creates 5 socket pairs with socketpair(2):
(fd1, fd2), (fd3, fd4), (fd5, fd6), (fd7, fd8), (fd9, fd10)

The kernel initializes all unix_sk(sk)->recursion_level to 0.

And it passes fds around in this way:
- A packet containing fd2 is sent through fd3 to reach fd4's delivery queue
  (fd4's recursion level is 1)
- A packet containing fd4 is sent through fd5 to reach fd6's delivery queue
  (fd6's recursion level is 2)
- A packet containing fd6 is sent through fd7 to reach fd8's delivery queue
  (fd8's recursion level is 3)
- A packet containing fd8 is sent through fd9 to reach fd10's delivery queue
  (fd10's recursion level is MAX_RECURSION_LEVEL=4 so it cannot be passed)

So far, D-Bus socket fd11 was not involved. Then:
- A D-Bus message containing fd8 is sent through D-Bus socket fd11 to reach
  fd12's delivery queue in the dbus-daemon process.
- dbus-daemon receives the message and fd8. fd8's recursion level is still 3,
  so it could forward it to another D-Bus client. Unfortunately, fd8's
  recursion level can change at any time, because the malicious program still
  has a copy of fd8.
- The malicious program now sends a packet containing fd8 through fd7 to
  reach fd8's delivery queue. fd8's recursion level is now
  MAX_RECURSION_LEVEL=4 and fd8 cannot be passed anymore.
- dbus-daemon is scheduled and tries to forward the D-Bus message containing
  fd8 through fd13 for another client. It fails with ETOOMANYREFS.
- dbus-daemon close(fd13)
- another_client is notified on fd14 that the D-Bus connection is shutdown and
  it terminates. The malicious program managed to kill another_client in this
  way.

While we're looking at file descriptor passing, Bug #79694 is not a security vulnerability, but is fairly fundamental and only avoided being a buffer overflow by luck (one bug compensated for another) so it would be good to review + fix.

The obvious solution here is to catch ETOOMANYREFS. Unfortunately, by the time we start calling sendmsg(), it's too late to refuse to forward the message, so the only thing we can do is to drop it on the floor (possibly with a best-effort notification back to the sender, but if the sender is doing this maliciously, they don't deserve for dbus-daemon to go to extra effort to notify them that they have failed).

It's unfortunate that we can't ask the kernel "is this OK to forward?" but that would be a TOCTOU (time of check / time of use) vulnerability in any case - as Alban points out, this attack only works if the malicious sender alters the recursion depth of the fd after dbus-daemon has already received it.

Catching ETOOMANYREFS in the same way as EINTR would not be correct: we'd busy-loop forever. EINTR is the thing that's special here: you can get it in otherwise normal operation.

The least bad solution I can think of is for do_writing() in dbus-transport-socket.c to distinguish between *three* categories of error:

* Those that are ignored: currently EAGAIN, EWOULDBLOCK, EPIPE

* Those that are fatal to the connection: currently the default for
  unknown errno; ECONNRESET certainly needs to be in this category

* New category: those that cause the rest of this message to be skipped,
  but do not disconnect the connection entirely:
  ETOOMANYREFS

I suppose another possibility would be to re-send the message without the fd, and let the recipient deal with the missing fd; but that's only viable if major D-Bus implementations (libdbus, GDBus, systemd-bus) don't treat that as fatal.

I would welcome better ideas...

Adding Lennart to Cc since iirc he wrote this stuff in the first place, so he might have ideas for how to solve it.

(For completeness, another possibility would be to read things from the passed file descriptor to remove all packets from the delivery queue and to reduce the recursion depth to zero. dbus-daemon could check the content with recvfrom(MSG_PEEK) before removing them. But it would be intrusive for no good reason.)

I think dropping the message on the floor, with an error logged, and without notifying the sender is the best option.

I don't see any good reason to support more than 1 recursion depth in fd passing anyway.

(In reply to comment #5)
> I think dropping the message on the floor, with an error logged, and without
> notifying the sender is the best option.

I actually think we should report an error if we can, since this might not be malicious at all.

> I don't see any good reason to support more than 1 recursion depth in fd
> passing anyway.

That breaks abstractions. Sometimes, the level passing the fds doesn't know what the fds are. They may be other sockets that are doing work and happen to be passing file descriptors by themselves.

In any case, we can't detect 1 level any more than we can detect max-level. We can only try to pass and see what happens if we do.

For method calls, we should report the error back to the caller (if we can), if this isn't an eavasdropping delivery. For everything else, we drop the message and syslog an error.

(In reply to comment #6)
> For method calls, we should report the error back to the caller (if we can),
> if this isn't an eavasdropping delivery. For everything else, we drop the
> message and syslog an error.

Unfortunately this is in generic libdbus code to send messages, not in dbus-daemon; so as far as I can see, this would require adding something like

void _dbus_connection_set_unable_to_send_callback (DBusConnection *conn,
    void (*callback) (DBusConnection *, DBusMessage *, DBusError *,
        void *data),
    void *data)

or maybe

void _dbus_connection_set_unable_to_send_callback (DBusConnection *conn,
    void (*callback) (DBusConnection *, DBusMessage *, int errno_, void *),
    void *)

so that dbus-daemon could set that callback to an implementation of "syslog an error and maybe send an error back to the caller".

Alban, are you able to spend some time writing a patch for this?

(In reply to comment #8)
> Alban, are you able to spend some time writing a patch for this?

Yes, I will write a patch for this.

Created attachment 101748 [details] [review]
[PATCH] Handle ETOOMANYREFS

(In reply to comment #7)
> (In reply to comment #6)
> > For method calls, we should report the error back to the caller (if we can),
> > if this isn't an eavasdropping delivery. For everything else, we drop the
> > message and syslog an error.
> 
> Unfortunately this is in generic libdbus code to send messages, not in
> dbus-daemon; so as far as I can see, this would require adding something like
> 
> void _dbus_connection_set_unable_to_send_callback (DBusConnection *conn,
>     void (*callback) (DBusConnection *, DBusMessage *, DBusError *,
>         void *data),
>     void *data)
> 
> or maybe
> 
> void _dbus_connection_set_unable_to_send_callback (DBusConnection *conn,
>     void (*callback) (DBusConnection *, DBusMessage *, int errno_, void *),
>     void *)
> 
> so that dbus-daemon could set that callback to an implementation of "syslog
> an error and maybe send an error back to the caller".

I wrote the libdbus part of this, see attached patch. It is already big:
 7 files changed, 130 insertions(+), 7 deletions(-)

But the bus part seems more difficult and intrusive.

In bus/dispatch.c::bus_dispatch(), dbus-daemon uses bus_transaction_execute_and_free() to forward the message to the recipients.

The distinction between the main recipient (addressed recipient) and the additional recipients (match rules or eavesdropping) does not exist in the BusTransaction because bus_transaction_execute_and_free() is not supposed to fail and report errors to the caller.

Moreover, when this function returns, the messages might not have been sent on the sockets yet: if the sockets are not ready (no POLLOUT yet) it will merely add the D-Bus message in the DBusConnection outgoing queue. By the time the message is sent through the socket, the BusTransaction will have been freed. BusTransaction also does not have a reference to the DBusConnection of the sender, but only of its BusContext.

The attached patch + the changes for the bus part (not written yet) seem excessively big for a security fix.

So I suggest I write a simpler patch to just drop the unsendable message without telling the sender; this will turn this denial-of-service bug into a simple non-security bug. And after the security fix is released, I can open a new non-security bug for returning an error message to the sender. However I am still not convinced that a non-malicious application sending fds into fds with a recursion level greater than 4 could exist so my preference for that additional bug would be WONTFIX.

Created attachment 101754 [details] [review]
[PATCH] Handle ETOOMANYREFS when sending recursive fds (SCM_RIGHTS)

This patch fixes the security issue but does not send an error to the sender. It is also smaller:

 3 files changed, 46 insertions(+), 1 deletion(-)

(In reply to comment #10)
> The attached patch + the changes for the bus part (not written yet) seem
> excessively big for a security fix.

I agree.

> The distinction between the main recipient (addressed recipient) and the
> additional recipients (match rules or eavesdropping) does not exist in the
> BusTransaction because bus_transaction_execute_and_free() is not supposed to
> fail and report errors to the caller.

I expected this would cause difficulty. Indeed, bus_transaction_execute_and_free() exists *because* it isn't meant to fail at this point: it is meant to either all succeed (modulo disconnected connections), or not get this far (hence "transaction").

> By the
> time the message is sent through the socket, the BusTransaction will have
> been freed. BusTransaction also does not have a reference to the
> DBusConnection of the sender, but only of its BusContext.

I think the best we can possibly do here is best-effort. Perhaps for method calls, bus/ could attach user-data to the addressed recipient's DBusMessage with a reference to the sending DBusConnection, and the function called on ETOOMANYREFS could attempt to push an error message into that connection, but if there is an OOM or other error (including a full queue), just give up instead?

Additional can of worms: you can attach fds to a signal. I'm not sure why you'd want to, but you can. We can't do anything better than dropping the message on the floor in that case.

Additional additional can of worms: replying to a method reply (or an error, which could theoretically carry a fd, although in practice nobody sends a payload of signature != 's') is not a well-defined action, so I don't think we can/should synthesize an error reply for that either.

I'm starting to think Alban was right about WONTFIXing Thiago's request to send back an error reply: too much implementation, too little gain.

Lennart, Kay: how does kdbus deal with the equivalent of this?

Comment on attachment 101754 [details] [review]
[PATCH] Handle ETOOMANYREFS when sending recursive fds (SCM_RIGHTS)

Review of attachment 101754 [details] [review]:
-----------------------------------------------------------------

I am broadly in favour of this patch, although there are some minor issues.

I'll ask on distros@ for CVE IDs for this and Bug #80469 (either one or two CVEs, depending).

::: dbus/dbus-sysdeps.c
@@ +767,5 @@
> + */
> +dbus_bool_t
> +_dbus_get_is_errno_toomanyrefs (void)
> +{
> +  return errno == ETOOMANYREFS;

For portability, I think this has to be:

#ifdef ETOOMANYREFS
  return errno == ETOOMANYREFS;
#else
  return FALSE;
#endif

(Check that on GNU/Linux, ETOOMANYREFS is actually a #define - I think it is. I don't think we really consider non-GNU Linuxes like Android to be security-supported.)

::: dbus/dbus-sysdeps.h
@@ +384,4 @@
>  dbus_bool_t _dbus_get_is_errno_enomem                (void);
>  dbus_bool_t _dbus_get_is_errno_eintr                 (void);
>  dbus_bool_t _dbus_get_is_errno_epipe                 (void);
> +dbus_bool_t _dbus_get_is_errno_toomanyrefs           (void);

For consistency with the others this should ideally be ...is_errno_etoomanyrefs()

::: dbus/dbus-transport-socket.c
@@ +669,5 @@
> +               * ETOOMANYREFS cannot happen after.
> +               */
> +              _dbus_assert (socket_transport->message_bytes_written == 0);
> +
> +              _dbus_verbose (" discard message of %d bytes\n",

"...bytes due to ETOOMANYREFS\n"

@@ +672,5 @@
> +
> +              _dbus_verbose (" discard message of %d bytes\n",
> +                             total_bytes_to_write);
> +
> +              total += bytes_written;

bytes_written is -1 here. I don't think you want that?

(All this throws off is tracking of how many total bytes of messages we have sent without returning to the main loop, though.)

@@ +682,5 @@
> +              /* The message was not actually sent but it needs to be removed
> +               * from the outgoing queue
> +               */
> +              _dbus_connection_message_sent_unlocked (transport->connection,
> +                                                      message);

I agree with your reasoning that this is about the most that is feasible to do as a security fix, and that we can handle the rest as an ordinary bug later.

Other maintainers, what do you think? (I'm particularly interested in opinions/ideas from those who wrote this stuff.)

Comment on attachment 101754 [details] [review]
[PATCH] Handle ETOOMANYREFS when sending recursive fds (SCM_RIGHTS)

Review of attachment 101754 [details] [review]:
-----------------------------------------------------------------

::: dbus/dbus-sysdeps.c
@@ +767,5 @@
> + */
> +dbus_bool_t
> +_dbus_get_is_errno_toomanyrefs (void)
> +{
> +  return errno == ETOOMANYREFS;

It is a #define:

$ grep ETOOMANYREFS /usr/include/asm-generic/errno.h
#define	ETOOMANYREFS	109	/* Too many references: cannot splice */

Changed.

::: dbus/dbus-sysdeps.h
@@ +384,4 @@
>  dbus_bool_t _dbus_get_is_errno_enomem                (void);
>  dbus_bool_t _dbus_get_is_errno_eintr                 (void);
>  dbus_bool_t _dbus_get_is_errno_epipe                 (void);
> +dbus_bool_t _dbus_get_is_errno_toomanyrefs           (void);

Updated.

::: dbus/dbus-transport-socket.c
@@ +669,5 @@
> +               * ETOOMANYREFS cannot happen after.
> +               */
> +              _dbus_assert (socket_transport->message_bytes_written == 0);
> +
> +              _dbus_verbose (" discard message of %d bytes\n",

Updated.

@@ +672,5 @@
> +
> +              _dbus_verbose (" discard message of %d bytes\n",
> +                             total_bytes_to_write);
> +
> +              total += bytes_written;

Oops. I removed that line. So It will check the limit per write iteration counting only effectively written message and not discarded messages.

Created attachment 101811 [details] [review]
[PATCH] Handle ETOOMANYREFS when sending recursive fds (SCM_RIGHTS)

Patch updated following Simon's review. I added a small paragraph in the commit message too.

I tested the patch on top of 1.8.0 and it fixed the issue. I could see with strace that despite receiving ETOOMANYREFS, dbus-daemon does not close the target connection and the target application survives.

Comment on attachment 101811 [details] [review]
[PATCH] Handle ETOOMANYREFS when sending recursive fds (SCM_RIGHTS)

Review of attachment 101811 [details] [review]:
-----------------------------------------------------------------

Looks good.

Removing embargo. Fixed in dbus 1.8.6 and 1.6.22 which I just released.