Bug 81469 - add new limit: max_connections_per_systemd_unit
Summary: add new limit: max_connections_per_systemd_unit
Status: REOPENED
Alias: None
Product: dbus
Classification: Unclassified
Component: core (show other bugs)
Version: 1.5
Hardware: All All
: low enhancement
Assignee: D-Bus Maintainers
QA Contact: D-Bus Maintainers
URL:
Whiteboard: review-
Keywords: patch
Depends on:
Blocks:
 
Reported: 2014-07-17 18:37 UTC by Alban Crequy
Modified: 2017-03-24 15:57 UTC (History)
6 users (show)

See Also:
i915 platform:
i915 features:


Attachments
[PATCH 1/3] config: add new limit: max_connections_per_cgroup (6.07 KB, patch)
2014-07-17 18:40 UTC, Alban Crequy
Details | Splinter Review
[PATCH 2/3] _dbus_cgroup_for_pid: new function (6.46 KB, patch)
2014-07-17 18:41 UTC, Alban Crequy
Details | Splinter Review
[PATCH 3/3] enforce new limit max_connections_per_cgroup (10.54 KB, patch)
2014-07-17 18:41 UTC, Alban Crequy
Details | Splinter Review
[PATCH 1/4] config: add new limit: max_connections_per_cgroup (6.10 KB, text/plain)
2014-08-11 14:51 UTC, Alban Crequy
Details
[PATCH 2/4] _dbus_cgroup_for_pid: new function (6.51 KB, patch)
2014-08-11 14:52 UTC, Alban Crequy
Details | Splinter Review
[PATCH 3/4] enforce new limit max_connections_per_cgroup (11.95 KB, patch)
2014-08-11 14:53 UTC, Alban Crequy
Details | Splinter Review
[PATCH 4/4] new tests: max_connections_per_user, max_connections_per_cgroup (5.91 KB, patch)
2014-08-11 14:53 UTC, Alban Crequy
Details | Splinter Review
[PATCH 1/3] config: add new limit: max_connections_per_systemd_unit (6.49 KB, patch)
2014-08-15 09:20 UTC, Alban Crequy
Details | Splinter Review
[PATCH 2/3] enforce new limit max_connections_per_systemd_unit (13.04 KB, patch)
2014-08-15 09:21 UTC, Alban Crequy
Details | Splinter Review
[PATCH 3/3] new tests: max_connections_per_user, max_connections_per_systemd_unit (8.11 KB, patch)
2014-08-15 09:22 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-17 18:37:36 UTC
dbus-daemon already has the following limits:
- max_completed_connections: (default|system bus)=2048 (session bus)=100000
- max_connections_per_user: (default|system bus)=256 (session bus)=100000

So an user on the system bus cannot use all connections and prevent other users
from connecting to the bus.

But this per-user granularity does not allow to distinguish different services
running as the same user. For example, both Avahi and ConsoleKit are system
services, running as the same user root, and they connect to the system bus. If
one of them starts to use all the available connections due to a bug, the other
will not be able to connect.

To fix this issue, this patch introduces a new configurable limit:
- max_connections_per_cgroup: (default|system bus)=256 (session bus)=100000

The default values are large enough to avoid impacting current systems but an
administrator could restrict it more.
Comment 1 Alban Crequy 2014-07-17 18:40:40 UTC
Created attachment 102994 [details] [review]
[PATCH 1/3] config: add new limit: max_connections_per_cgroup
Comment 2 Alban Crequy 2014-07-17 18:41:21 UTC
Created attachment 102995 [details] [review]
[PATCH 2/3] _dbus_cgroup_for_pid: new function
Comment 3 Alban Crequy 2014-07-17 18:41:49 UTC
Created attachment 102996 [details] [review]
[PATCH 3/3] enforce new limit max_connections_per_cgroup
Comment 4 Alban Crequy 2014-07-17 18:45:31 UTC
Cc Lennart because this is related to systemd's cgroups.
Comment 5 Alban Crequy 2014-07-23 11:29:36 UTC
The patches rely on parsing /proc/<pid>/cgroup because SCM_CGROUP does not exist yet (there are some patches about SCM_CGROUP being discussed on kernel mailing lists though).

Parsing /proc/<pid>/cgroup could be unsafe if the cgroup path contains '\n'. I wrote a patch to prevent that:
http://thread.gmane.org/gmane.linux.kernel.cgroups/11676
Comment 6 Alban Crequy 2014-08-08 12:37:10 UTC
See also similar feature request on Bug #82346 for max_connections_per_process.
Comment 7 Alban Crequy 2014-08-11 14:51:39 UTC
Created attachment 104444 [details]
[PATCH 1/4] config: add new limit: max_connections_per_cgroup

I rebased this series on the patches from Bug #82346.
Comment 8 Alban Crequy 2014-08-11 14:52:18 UTC
Created attachment 104445 [details] [review]
[PATCH 2/4] _dbus_cgroup_for_pid: new function
Comment 9 Alban Crequy 2014-08-11 14:53:00 UTC
Created attachment 104446 [details] [review]
[PATCH 3/4] enforce new limit max_connections_per_cgroup

v2:
 - fix memory leaks in adjust_connections_for_cgroup in case of oom
 - fix memory release in bus_connections_setup_connection in case of oom
Comment 10 Alban Crequy 2014-08-11 14:53:54 UTC
Created attachment 104447 [details] [review]
[PATCH 4/4] new tests: max_connections_per_user, max_connections_per_cgroup

New tests
Comment 11 Alban Crequy 2014-08-15 09:20:19 UTC
Following discussions on the mailing list:
http://lists.freedesktop.org/archives/dbus/2014-August/thread.html#16251
I will add v3 patches reading the systemd unit with sd_pid_get_unit() instead of the full cgroup path.
Comment 12 Alban Crequy 2014-08-15 09:20:56 UTC
Created attachment 104664 [details] [review]
[PATCH 1/3] config: add new limit: max_connections_per_systemd_unit
Comment 13 Alban Crequy 2014-08-15 09:21:59 UTC
Created attachment 104665 [details] [review]
[PATCH 2/3] enforce new limit max_connections_per_systemd_unit
Comment 14 Alban Crequy 2014-08-15 09:22:33 UTC
Created attachment 104666 [details] [review]
[PATCH 3/3] new tests: max_connections_per_user, max_connections_per_systemd_unit
Comment 15 Simon McVittie 2014-09-03 18:08:40 UTC
As with Bug #82346, what is the purpose of enforcing this limit? Is it intended to be a security measure, or a guard against mistakes, or what?

(In reply to comment #11)
> Following discussions on the mailing list:
> http://lists.freedesktop.org/archives/dbus/2014-August/thread.html#16251
> I will add v3 patches reading the systemd unit with sd_pid_get_unit()
> instead of the full cgroup path.

sd_pid_get_unit() has a TOCTOU race condition similar to the one discussed on many other security-sensitive D-Bus bugs: if we're asking about (say) pid 12345, you can

* start a connection from pid 12345
* hand the fd over to a cooperating process, pid 23456, say
* induce some other part of the system to fork() enough times that
  pid 12345 is recycled

and the result is that we get the wrong answer.

This can't be solved by any API that involves rummaging in /proc (which is what systemd does to implement sd_pid_get_unit()).

Perhaps one day it will be possible to get a fd pointing to a process, and call APIs on that.
Comment 16 Colin Walters 2014-09-07 16:00:32 UTC
(In reply to comment #15)

> sd_pid_get_unit() has a TOCTOU race condition similar to the one discussed
> on many other security-sensitive D-Bus bugs: 

Yeah, generally anything involving PIDs is racy.

There has been discussion in kernel space about adding the cgroup to the socket creds, I'm not sure where that is.
Comment 17 Simon McVittie 2014-09-15 16:24:41 UTC
Comment on attachment 104665 [details] [review]
[PATCH 2/3] enforce new limit max_connections_per_systemd_unit

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

> It can happen on Windows or on the non-supported case where
> dbus-daemon is configured to listen on TCP rather than a Unix socket.

... or on OS X, or on NetBSD, or (etc.)
Comment 18 Simon McVittie 2014-09-15 16:33:37 UTC
Comment on attachment 104665 [details] [review]
[PATCH 2/3] enforce new limit max_connections_per_systemd_unit

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

::: bus/connection.c
@@ +142,5 @@
> +{
> +  void *val;
> +  int current_count;
> +
> +  /* val is NULL is 0 when it isn't in the hash yet */

Same comments as for per-pid limits, except that if we're running under systemd, we know we're on Linux.

@@ +207,5 @@
> +  _dbus_assert (current_count >= 0);
> +
> +  current_count += adjustment;
> +
> +  _dbus_assert (current_count >= 0);

Same comments about wraparound as for per-pid limits.

@@ +637,5 @@
>    connections->context = context;
>    
>    return connections;
>  
> + failed_7:

As for the per-pid version, this is far too many labels, and the function deserves refactoring before adding more.
Comment 19 Simon McVittie 2014-09-15 16:35:43 UTC
Comment on attachment 104666 [details] [review]
[PATCH 3/3] new tests: max_connections_per_user, max_connections_per_systemd_unit

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

::: test/dbus-daemon.c
@@ +567,5 @@
>        &max_connections_per_process_config, setup,
> +      test_max_connections, teardown);
> +  g_test_add ("/limit/max_connections_per_user", Fixture,
> +      &max_connections_per_user_config, setup,
> +      test_max_connections, teardown);

I would like the per-uid test (to be run on G_OS_UNIX only), even if we don't merge your new per-process or per-systemd-unit limits.
Comment 20 Simon McVittie 2014-09-15 16:36:52 UTC
(In reply to comment #15)
> sd_pid_get_unit() has a TOCTOU race condition similar to the one discussed
> on many other security-sensitive D-Bus bugs

Between "I'm not convinced this is very useful" and this race condition, I'm reluctant to add this feature.
Comment 21 Simon McVittie 2017-03-24 15:57:45 UTC
I think this would be obsoleted by Bug #100344, at which point we could say "if you don't trust some of the services that share your uid, give them private D-Bus endpoints using Bug #100344" and close this WONTFIX.


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