Bug 81469

Summary: add new limit: max_connections_per_systemd_unit
Product: dbus Reporter: Alban Crequy <alban.crequy>
Component: coreAssignee: D-Bus Maintainers <dbus>
Status: REOPENED --- QA Contact: D-Bus Maintainers <dbus>
Severity: enhancement    
Priority: low CC: alban.crequy, lennart, msniko14, smcv, thiago, walters
Version: 1.5Keywords: patch
Hardware: All   
OS: All   
Whiteboard: review-
i915 platform: i915 features:
Attachments: [PATCH 1/3] config: add new limit: max_connections_per_cgroup
[PATCH 2/3] _dbus_cgroup_for_pid: new function
[PATCH 3/3] enforce new limit max_connections_per_cgroup
[PATCH 1/4] config: add new limit: max_connections_per_cgroup
[PATCH 2/4] _dbus_cgroup_for_pid: new function
[PATCH 3/4] enforce new limit max_connections_per_cgroup
[PATCH 4/4] new tests: max_connections_per_user, max_connections_per_cgroup
[PATCH 1/3] config: add new limit: max_connections_per_systemd_unit
[PATCH 2/3] enforce new limit max_connections_per_systemd_unit
[PATCH 3/3] new tests: max_connections_per_user, max_connections_per_systemd_unit

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.