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).
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.
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.
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 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 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.
(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.
(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.
CVE-2014-3638 has been allocated for this vulnerability.
Vulnerability fixed in 1.8.8, making public
(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.