Bug 82820

Summary: CVE-2014-3636: using all available file descriptors in dbus-daemon: limit DEFAULT_MESSAGE_UNIX_FDS
Product: dbus Reporter: Alban Crequy <alban.crequy>
Component: coreAssignee: Simon McVittie <smcv>
Status: RESOLVED FIXED QA Contact: Sjoerd Simons <sjoerd>
Severity: normal    
Priority: medium CC: thiago, walters
Version: unspecifiedKeywords: patch, security
Hardware: Other   
OS: All   
Whiteboard: review+
i915 platform: i915 features:
Attachments: [PATCH] config: set DEFAULT_MESSAGE_UNIX_FDS to 16
[PATCH v2] config: set DEFAULT_MESSAGE_UNIX_FDS to 16
[PATCH v3] config: reduce DEFAULT_MESSAGE_UNIX_FDS to 16
[patch v4] config: change DEFAULT_MESSAGE_UNIX_FDS to 16
[patch v4 backport to 1.6] config: change DEFAULT_MESSAGE_UNIX_FDS to 16
[patch v4 backport to 1.6] config: change DEFAULT_MESSAGE_UNIX_FDS to 16

Description Alban Crequy 2014-08-19 15:21:55 UTC
Created attachment 104901 [details] [review]
[PATCH] config: set DEFAULT_MESSAGE_UNIX_FDS to 16

Before this patch, the system bus had the following default configuration:
- max_connections_per_user: 256
- DBUS_DEFAULT_MESSAGE_UNIX_FDS: usually 1024 (or 256 on QNX, see fd.o#61176)
  as defined by configure.ac
- max_incoming_unix_fds: DBUS_DEFAULT_MESSAGE_UNIX_FDS*4 = usually 4096
- max_outgoing_unix_fds: DBUS_DEFAULT_MESSAGE_UNIX_FDS*4 = usually 4096
- max_message_unix_fds: DBUS_DEFAULT_MESSAGE_UNIX_FDS = usually 1024

This means that a single user could create 256 connections and transmit
256*4096 = 1048576 file descriptors.

The file descriptors stay attached to the dbus-daemon process while they are
in the message loader, in the outgoing queue or waiting to be dispatched before
D-Bus activation.

dbus-daemon is usually limited to 65536 file descriptors (ulimit -n). If the
limit is reached and dbus-daemon needs to receive a message with a file
descriptor attached, this is signalled by recvfrom with the flag MSG_CTRUNC.
Dbus-daemon cannot recover from that error because the kernel does not have any
API to retrieve a file descriptor which has been discarded with MSG_CTRUNC.
Therefore, it closes the connection of the sender. This is not necessarily the
connection which generated the most file descriptors so it can lead to
denial-of-service attacks.

In order to prevent DoS issues, this patch reduces DEFAULT_MESSAGE_UNIX_FDS to
16:

max_connections_per_user * max_incoming_unix_fds = 256 * 64 = 16384

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.)
Comment 1 Thiago Macieira 2014-08-19 15:54:27 UTC
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.
Comment 2 Alban Crequy 2014-08-20 10:13:56 UTC
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 3 Thiago Macieira 2014-08-20 17:31:06 UTC
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 4 Simon McVittie 2014-09-03 17:12:50 UTC
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.
Comment 5 Simon McVittie 2014-09-05 17:44:49 UTC
(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?
Comment 6 Alban Crequy 2014-09-08 10:27:38 UTC
(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.
Comment 7 Simon McVittie 2014-09-08 11:07:13 UTC
(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.
Comment 8 Alban Crequy 2014-09-08 14:07:59 UTC
(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?
Comment 9 Alban Crequy 2014-09-08 14:14:48 UTC
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?
Comment 10 Simon McVittie 2014-09-08 14:29:04 UTC
(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.
Comment 11 Simon McVittie 2014-09-08 19:08:30 UTC
(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.
Comment 12 Alban Crequy 2014-09-09 14:33:23 UTC
(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.
Comment 13 Alban Crequy 2014-09-09 14:53:37 UTC
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 14 Simon McVittie 2014-09-09 18:13:27 UTC
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 15 Simon McVittie 2014-09-12 14:54:14 UTC
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.
Comment 16 Simon McVittie 2014-09-12 15:02:22 UTC
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
Comment 17 Simon McVittie 2014-09-12 15:36:57 UTC
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 18 Alban Crequy 2014-09-12 15:51:10 UTC
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 19 Alban Crequy 2014-09-12 15:56:46 UTC
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.
Comment 20 Simon McVittie 2014-09-12 16:10:05 UTC
(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.
Comment 21 Simon McVittie 2014-09-12 16:10:56 UTC
(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.
Comment 22 Simon McVittie 2014-09-12 16:12:06 UTC
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.
Comment 23 Simon McVittie 2014-09-12 22:17:10 UTC
CVE-2014-3636 has been allocated for this vulnerability (including both part A, described in Comment #0, and part B, covered in Comment #9).
Comment 24 Simon McVittie 2014-09-16 16:37:02 UTC
Vulnerability fixed in 1.8.8, making public

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