Bug 81053 - CVE-2014-3638: Flooding dbus-daemon with pending replies: denial of service
Summary: CVE-2014-3638: Flooding dbus-daemon with pending replies: denial of service
Status: RESOLVED FIXED
Alias: None
Product: dbus
Classification: Unclassified
Component: core (show other bugs)
Version: unspecified
Hardware: Other All
: medium normal
Assignee: Simon McVittie
QA Contact: Sjoerd Simons
URL:
Whiteboard: review+ for minimal fix, review? for ...
Keywords: patch, security
Depends on:
Blocks: 83938
  Show dependency treegraph
 
Reported: 2014-07-08 14:15 UTC by Alban Crequy
Modified: 2014-09-16 16:40 UTC (History)
3 users (show)

See Also:
i915 platform:
i915 features:


Attachments
[PATCH] pending replies: use better data structures than a linked list (38.72 KB, patch)
2014-07-10 14:03 UTC, Alban Crequy
Details | Splinter Review
[PATCH] system bus limit: use max_replies_per_connection=128 by default (868 bytes, patch)
2014-07-10 14:30 UTC, Alban Crequy
Details | Splinter Review
[PATCH v2] pending replies: use better data structures than a linked list (45.25 KB, patch)
2014-07-28 15:04 UTC, Alban Crequy
Details | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Alban Crequy 2014-07-08 14:15:10 UTC
I have an issue with pending replies. It is related to the following limits:

- max_replies_per_connection; default: 1024*8; session=50000
- reply_timeout; default: -1 (disabled)
- max_connections_per_user=256
- max_replies total = max_replies_per_connection
                      * max_connections_per_user
                    = 256*1024*8 per user

So a single user on the system bus could generate up to 2097152 pending replies that dbus-daemon has to track. All pending replies are tracked in the linked list "connections->pending_replies", see bus_connections_check_reply().

So every time a benign connection sends a D-Bus method, dbus-daemon has to iterate over this linked list. It is taking too much time and during that time, it uses the cpu and does not process other D-Bus connections.

While a malicious process creates as many pending requests as it can, I tested how long a benign method takes with the following command:

  time dbus-send --system  --print-reply \
    --dest=org.freedesktop.DBus /org/freedesktop/DBus \
    org.freedesktop.DBus.GetConnectionUnixProcessID \
    string::1.76

After enough pending replies being added in the linked list, the simple dbus-send takes more than _DBUS_DEFAULT_TIMEOUT_VALUE (25 seconds) and it fails.

The "time" command gave me once the following result:
real	1m58.026s

The authentication steps are not included in the 25 seconds timeout, that's why "time" displays more than 25 seconds.

So while dbus-daemon is busy looping taking the cpu, D-Bus methods fail (depending on reply_timeout and applications' timeout) and authentication fails (depending on auth_timeout).
Comment 1 Alban Crequy 2014-07-10 14:03:26 UTC
Created attachment 102539 [details] [review]
[PATCH] pending replies: use better data structures than a linked list

With this patch, a message round trip takes less than 5 seconds even with plenty of pending replies.

Given the size of the patch, I expect it still contain bugs and needs more testing:
 3 files changed, 532 insertions(+), 213 deletions(-)

As a security workaround, I would like to suggest max_replies_per_connection=128 by default on the system bus. So we would not need an invasive security patch. With this low limit, I don't notice any DoS and a message round-trip is fast. I am not aware of applications needing more than 128 method calls in parallel on the system bus.
Comment 2 Alban Crequy 2014-07-10 14:30:17 UTC
Created attachment 102545 [details] [review]
[PATCH] system bus limit: use max_replies_per_connection=128 by default

A one-line patch as a security workaround.
Comment 3 Alban Crequy 2014-07-28 15:04:47 UTC
Created attachment 103596 [details] [review]
[PATCH v2] pending replies: use better data structures than a linked list

v2:
 - add some documentation
 - use the new API in the unit test
 - fix counting of deleted & non-deleted items
 - fix double free in bus_connections_check_reply
 - fix caller/callee confusion in bus_expire_list_foreach
 - fix memleaks in case of OOM in _bus_expire_list_add_helper

"make check" passes fine (including with --enable-developer for the memleaks tests).
Comment 4 Simon McVittie 2014-09-03 17:14:58 UTC
Comment on attachment 102545 [details] [review]
[PATCH] system bus limit: use max_replies_per_connection=128 by default

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

::: bus/config-parser.c
@@ +467,4 @@
>        /* this is effectively a limit on message queue size for messages
>         * that require a reply
>         */
> +      parser->limits.max_replies_per_connection = 128;

Before 2007, this was 32, which is too small.

128 seems sane.
Comment 5 Simon McVittie 2014-09-03 17:17:27 UTC
Comment on attachment 103596 [details] [review]
[PATCH v2] pending replies: use better data structures than a linked list

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

This is pretty scary. Alban, am I right in thinking that this is not required for the security fix? I would rather do this in 1.9.
Comment 6 Alban Crequy 2014-09-04 17:34:42 UTC
(In reply to comment #5)
> Comment on attachment 103596 [details] [review] [review]
> [PATCH v2] pending replies: use better data structures than a linked list
> 
> Review of attachment 103596 [details] [review] [review]:
> -----------------------------------------------------------------
> 
> This is pretty scary. Alban, am I right in thinking that this is not
> required for the security fix? I would rather do this in 1.9.

That's right: in practice, I have not noticed any issue when max_replies_per_connection is reduced to 128. So I think either patch would prevent the DoS, so we should choose the one-liner for now and the scary patch becomes an optimization which can wait.
Comment 7 Simon McVittie 2014-09-09 18:14:48 UTC
(In reply to comment #4)
> 128 seems sane.

I noticed while rummaging through the kdbus implementation that their hard-coded limit happens to also be 128 at the moment, so, yay consistency.
Comment 8 Simon McVittie 2014-09-12 22:17:58 UTC
CVE-2014-3638 has been allocated for this vulnerability.
Comment 9 Simon McVittie 2014-09-16 16:36:38 UTC
Vulnerability fixed in 1.8.8, making public
Comment 10 Simon McVittie 2014-09-16 16:40:35 UTC
(In reply to comment #3)
> [PATCH v2] pending replies: use better data structures than a linked list

I have cloned Bug #83938 for this. Closing this bug.


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