Bug 82820

Comment on attachment 104901 [details] [review]
[PATCH] config: set DEFAULT_MESSAGE_UNIX_FDS to 16

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

Looks good, but I'd remove the QNX extra case altogether.

::: configure.ac
@@ +1241,3 @@
>  # Determine maximum number of Unix fds which may be passed
>  AS_CASE([$host_os],
>    [*qnx*],

You should remove the case altogether.

Created attachment 104969 [details] [review]
[PATCH v2] config: set DEFAULT_MESSAGE_UNIX_FDS to 16

Thanks for the review.

v2:
- remove the case, as per Thiago's review
- add a reference to bugzilla in the commit log

Comment on attachment 104969 [details] [review]
[PATCH v2] config: set DEFAULT_MESSAGE_UNIX_FDS to 16

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

Looks good, ship it.

Comment on attachment 104969 [details] [review]
[PATCH v2] config: set DEFAULT_MESSAGE_UNIX_FDS to 16

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

(I'm back from "nowhere near dbus" and hoping to have considerably more time to deal with Alban's recent security work now.)

dbus-daemon does attempt to raise its fd limit to at least

    max_completed_connections +
    max_incomplete_connections +
    32 /* arbitrary number for pipes, inotify, misc */

but indeed that does not take into account that fd-passing can also increase the number of fds it needs.

If we're going to be doing an embargoed security release for at least one of Alban's embargoed things, which seems likely, then I think it makes sense to queue this for that release.

(In reply to comment #0)
> This means that a single user could create 256 connections and transmit
> 256*4096 = 1048576 file descriptors.

I was about to say: I think it's actually more complicated than that. The byte-limit on incoming messages is allowed to be exceeded by up to 1 read cycle (2 KiB) or 1 entire message, whichever is greater, minus 1 byte - this means we can still make forward progress if the limit on the size of a message is greater than the limit on incoming messages.

However, while investigating this I found that the same logic is not applied particularly consistently to fds, and that if you send a lot of fds in quick succession, you can end up with less space to read fds into than max_message_unix_fds (possibly down to zero), because _dbus_message_loader_get_unix_fds does not allocate (currently queued number + max_message_unix_fds) but just (max_message_unix_fds) of space in total; so you're more likely to get MSG_CTRUNC and be disconnected. I don't think this is a security bug, but it is potentially an ordinary functionality bug, and dropping this limit seems likely to exacerbate it. I'm not sure whether the author of our fd-passing code (Lennart?) had realised that for performance, we "eagerly" fill the DBusMessageLoader with as many messages as we can, rather than stopping at exactly 1 message.

If we go for the same "may exceed by up to 1 maximal message" logic, Alban's figures for the number of fds a uid can consume need to be multiplied by 5/4.

Alternatively, we could drop max_incoming_unix_fds to be closer to max_message_unix_fds?

(In reply to comment #4)
> dbus-daemon does attempt to raise its fd limit to at least
> 
>     max_completed_connections +
>     max_incomplete_connections +
>     32 /* arbitrary number for pipes, inotify, misc */

I have not seen that. This is in bus/bus.c raise_file_descriptor_limit(). But on my system, dbus-daemon runs as user "messagebus" and raise_file_descriptor_limit() skips the attempt when getuid () != 0.

(In reply to comment #6)
> I have not seen that. This is in bus/bus.c raise_file_descriptor_limit().
> But on my system, dbus-daemon runs as user "messagebus" and
> raise_file_descriptor_limit() skips the attempt when getuid () != 0.

Hmm, yes. The systemd unit starts it as root and lets it do its own setuid() to messagebus, but the Debian sysvinit script starts it as messagebus.

I should maybe change the sysvinit script in Debian to start as root too.

(In reply to comment #5)
> If we go for the same "may exceed by up to 1 maximal message" logic, Alban's
> figures for the number of fds a uid can consume need to be multiplied by 5/4.

I am not sure I understand this correctly... With the patch applied, we have:

max_connections_per_user * max_incoming_unix_fds
 = 256 * DBUS_DEFAULT_MESSAGE_UNIX_FDS * 4
 = 256 * 16 * 4
 = 16384

Even multiplied by 5/4, we get 20480, which is less than 65536 ("ulimit -n"), so it's fine. Is it correct?

This patch might fix two security issues for the price of one :)

There is a limit I didn't know before: on Linux, it's not possible to send more than 253 fds in a single sendmsg() system call: sendmsg() would return -EINVAL.
#define SCM_MAX_FD      253
http://lxr.free-electrons.com/source/include/net/scm.h#L13

Imagine a malicious D-Bus client sending a single D-Bus message by calling sendmsg() two times to send the two halves of the message separately. If the D-Bus message has 256 fds attached in total but each halves of the message has 128 fds attached, then the two sendmsg() calls will succeed. Dbus-daemon will receive the message successfully and attempt to forward it to the recipient in a single sendmsg() call. It will fail with -EINVAL and dbus-daemon will disconnect the recipient instead of blaming the sender. So a random client could disconnect a system service by sending this crafted message in two parts...

Do we need to add -EINVAL along with ETOOMANYREFS as explained in Bug #80163 Comment #3? If we keep max_incoming_unix_fds lower than SCM_MAX_FD, then we don't have to change the error management.

Do we need a new bug for that, or can I just edit the commit log in this patch?

(In reply to comment #8)
> Even multiplied by 5/4, we get 20480, which is less than 65536 ("ulimit
> -n"), so it's fine. Is it correct?

Yes, I think so.

I think I need to do some testing on this stuff, it isn't sufficiently clear what's going on.

(In reply to comment #9)
> This patch might fix two security issues for the price of one :)
...
> Imagine a malicious D-Bus client sending a single D-Bus message by calling
> sendmsg() two times to send the two halves of the message separately.

I can also imagine a malicious D-Bus client calling sendmsg() once per byte, so we can be given one fd per byte of message (header + padding + payload). We certainly want to allow multi-KiB messages.

> Dbus-daemon
> will receive the message successfully and attempt to forward it to the
> recipient in a single sendmsg() call. It will fail with -EINVAL and
> dbus-daemon will disconnect the recipient instead of blaming the sender.

Yes, I think you're right. So we need the limit on fds per message to be 253 or less, if we want dbus-daemon to be able to forward it in a single sendmsg(), which we do.

> Do we need to add -EINVAL along with ETOOMANYREFS as explained in Bug #80163
> Comment #3? If we keep max_incoming_unix_fds lower than SCM_MAX_FD, then we
> don't have to change the error management.

More precisely, we want to keep max_message_unix_fds <= SCM_MAX_FD.

I don't think we need to add -EINVAL to the -ETOOMANYREFS handling for this reason, because your patch would already address this; and I don't really want to make -EINVAL non-fatal for the connection, because it's such a generic error code that it's anyone's guess what it means.

> Do we need a new bug for that, or can I just edit the commit log in this
> patch?

I think an edited commit message is fine.

(In reply to comment #5)
> However, while investigating this I found that the same logic is not applied
> particularly consistently to fds, and that if you send a lot of fds in quick
> succession, you can end up with less space to read fds into than
> max_message_unix_fds (possibly down to zero), because
> _dbus_message_loader_get_unix_fds does not allocate (currently queued number
> + max_message_unix_fds) but just (max_message_unix_fds) of space in total;
> so you're more likely to get MSG_CTRUNC and be disconnected.

In practice, as long as you start each new message with a new sendmsg(), I don't think you can hit this problem. So I think we're OK here.

(In reply to comment #10)
> More precisely, we want to keep max_message_unix_fds <= SCM_MAX_FD.

SCM_MAX_FD changed value during Linux history:
- it used to be (OPEN_MAX-1)
- commit c09edd6eb (Jul 2007) changed it to 255
- commit bba14de98 (Nov 2010) changed it to 253

And it is not exported to userspace.

But I think that with the new value max_message_unix_fds=16, we are safe: even if SCM_MAX_FD is further reduced, we have some margin.

Created attachment 105987 [details] [review]
[PATCH v3] config: reduce DEFAULT_MESSAGE_UNIX_FDS to 16

v3: edit commit log to explain the two denials of service

Comment on attachment 105987 [details] [review]
[PATCH v3] config: reduce DEFAULT_MESSAGE_UNIX_FDS to 16

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

Looks good to me.

Comment on attachment 105987 [details] [review]
[PATCH v3] config: reduce DEFAULT_MESSAGE_UNIX_FDS to 16

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

::: configure.ac
@@ +1238,5 @@
>    AC_DEFINE([WITH_VALGRIND], [1], [Define to add Valgrind instrumentation])
>  fi
>  
> +# Keep the default low to avoid DoS issues, see fd.o #82820
> +DEFAULT_MESSAGE_UNIX_FDS=16

While backporting this to 1.6 I noticed that you didn't change the corresponding value in the cmake build system.

Now that we're not distinguishing between QNX and other Unixes, we can just hard-code, which is a simplification.

Created attachment 106189 [details] [review]
[patch v4] config: change DEFAULT_MESSAGE_UNIX_FDS to 16

Based on a patch by Alban Crequy. Now that it's the same on all
platforms, there's little point in it being set by configure/cmake.

This change fixes two distinct denials of service:

fd.o#82820, part A
------------------

Before this patch, the system bus had the following default configuration:

[... what Alban said ...]

This is less than the usual "ulimit -n" (65536) with a good margin to
accomodate the other sources of file descriptors (stdin/stdout/stderr,
listening sockets, message loader, etc.).

Distributors on non-Linux may need to configure a smaller limit in
system.conf, if their limit on the number of fds is smaller than
Linux's.

fd.o#82820, part B
------------------

On Linux, it's not possible to send more than 253 fds in a single sendmsg()
call: [... what Alban said ...]

---

v4:
* define DBUS_DEFAULT_MESSAGE_UNIX_FDS in dbus-sysdeps.h instead of
  configure.ac
  - already included by bus/config-parser.c
  - explicitly include that header in dbus/dbus-message.c

* make bus/session.conf.in just not override the default,
  so it doesn't have to get @DEFAULT_MESSAGE_UNIX_FDS@ substituted

* mention in the commit message that non-Linux may need further reductions

Created attachment 106190 [details] [review]
[patch v4 backport to 1.6] config: change DEFAULT_MESSAGE_UNIX_FDS to 16

---

That constant is new in 1.8 so introduce it into 1.6.

Comment on attachment 106189 [details] [review]
[patch v4] config: change DEFAULT_MESSAGE_UNIX_FDS to 16

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

When applying the patch on the master branch, a trivial conflict on configure.ac will need to be fixed.

The patch looks good to me.

Comment on attachment 106190 [details] [review]
[patch v4 backport to 1.6] config: change DEFAULT_MESSAGE_UNIX_FDS to 16

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

> [patch v4 backport to 1.6] config: change DEFAULT_MESSAGE_UNIX_FDS to 16

It does not seem to be the correct attachment for a backport to 1.6.

(In reply to comment #18)
> When applying the patch on the master branch, a trivial conflict on
> configure.ac will need to be fixed.

I'm treating dbus-1.8 as the canonical version since that's what I'll have to use for the security release.

(In reply to comment #19)
> It does not seem to be the correct attachment for a backport to 1.6.

Oh, sorry, you're right. One moment.

Created attachment 106196 [details] [review]
[patch v4 backport to 1.6] config: change DEFAULT_MESSAGE_UNIX_FDS to 16

---

Actually the 1.6 version this time, not the 1.8 version.

Might assume that Bug #81053 has been fixed first.

CVE-2014-3636 has been allocated for this vulnerability (including both part A, described in Comment #0, and part B, covered in Comment #9).

Vulnerability fixed in 1.8.8, making public