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

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. How we collect and use information is described in our Privacy Policy.