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 This bug is to add a new limit: - max_connections_per_process: (default|system bus)=8 (session bus)=8 Most applications should have only one connection to the bus. But sometimes, a process could legitimately create a few connections to the bus: it happens in an applications with plugins written with different dbus bindings such as libdbus and Qt D-Bus linked in the same process. But there are no good reasons for a process to have plenty of connections. See also Bug #81469 (max_connections_per_cgroup) for another limit on cgroups.
Created attachment 104276 [details] [review] [PATCH 1/3] config: add new limit: max_connections_per_process
Created attachment 104277 [details] [review] [PATCH 2/3] enforce new limit max_connections_per_process
Created attachment 104278 [details] [review] [PATCH 3/3] new test: max_connections_per_process
Created attachment 104441 [details] [review] [PATCH 3/3] new test: max_connections_per_process v2: - fix bug number in test/dbus-daemon.c - rename test to max_connections_per_process
It isn't clear to me why this is useful. Do you intend this to be a security mechanism, or a guard against mistakes, or what? It seems to me as though this would only have a practical effect if: (pseudocode) max_connections_per_process * our RLIMIT_NPROC < max_connections_per_uid
(In reply to comment #5) > It isn't clear to me why this is useful. Do you intend this to be a security > mechanism, or a guard against mistakes, or what? A guard against mistakes, and also make it slightly more complicated to write exploits. But I agree it is not helping as much as other limits. > It seems to me as though this would only have a practical effect if: > (pseudocode) > > max_connections_per_process * our RLIMIT_NPROC < max_connections_per_uid In environments using a LSM such as AppArmor, some processes are prevented from connecting to D-Bus so the formula becomes more complex in that case.
Comment on attachment 104276 [details] [review] [PATCH 1/3] config: add new limit: max_connections_per_process Review of attachment 104276 [details] [review]: ----------------------------------------------------------------- > But there are no good reasons for a process to have plenty of connections. o rly? I've seen "one connection per thread" suggested as a way to avoid threading issues when using libraries whose multi-threading does not have a good top-down design (like, er, libdbus). ::: bus/session.conf.in @@ +55,4 @@ > <limit name="max_completed_connections">100000</limit> > <limit name="max_incomplete_connections">10000</limit> > <limit name="max_connections_per_user">100000</limit> > + <limit name="max_connections_per_process">8</limit> The default limits for the session bus are intended to be "essentially unlimited" (with one exception: fd-passing, for reasons described in the comments). Also, if you really want the limit for the session bus to be the same as the hard-coded limit in config-parser.c, there's little point in adding this <limit/> to the config file at all. ::: doc/dbus-daemon.1.xml.in @@ +533,5 @@ > connections > "max_connections_per_user" : max number of completed connections from > the same user > + "max_connections_per_process": max number of completed connections from > + the same process "... on platforms where this can be determined reliably" At the moment that's: Linux, OpenBSD, other platforms with SO_PEERCRED FreeBSD, DragonflyBSD, other platforms with SCM_CREDS Solaris Windows (but that might be reverted, see bug #83499) but not, for instance, Cygwin Minix NetBSD (but it might be added, see bug #69702) OS X
Comment on attachment 104277 [details] [review] [PATCH 2/3] enforce new limit max_connections_per_process Review of attachment 104277 [details] [review]: ----------------------------------------------------------------- ::: bus/connection.c @@ +151,5 @@ > +{ > + void *val; > + int current_count; > + > + /* val is NULL is 0 when it isn't in the hash yet */ ISO C does not actually guarantee that. I don't think we actually care about any platforms where it would be false, but I'd prefer something like /* val is NULL when it isn't in the hash yet; * we assume here that (int) NULL is 0, as it * is on any sensible platform */ @@ +219,5 @@ > + _dbus_assert (current_count >= 0); > + > + current_count += adjustment; > + > + _dbus_assert (current_count >= 0); Careful: signed wraparound is undefined behaviour. In practice it probably wraps around like unsigned numbers do, but ISO C says compilers may assume that it doesn't happen and optimize accordingly. The practical result is that this is dangerous to rely on. @@ +523,5 @@ > connections->context = context; > > return connections; > > + failed_6: A label called "failed_6" is a sign that something has gone badly wrong. Before hacking up this function any further, I would like someone (maybe you, if you have things you want to add here) to refactor it into the form failed: if (connections->pending_replies != NULL) bus_expire_list_free (connections->pending_replies); if (connections->expire_timeout != NULL) _dbus_timeout_unref (connections->expire_timeout); /* etc. */ with the appropriate initializations to make that work. @@ +1616,5 @@ > + > + if (d->pid > 0) > + { > + if (get_connections_for_pid (connections, d->pid) >= > + bus_context_get_max_connections_per_process (connections->context)) So... I can bypass this limit by connecting in some way that doesn't reveal my pid, like TCP? (Not that it could be any other way - if you put all unknown-pid connections into the same "bucket", 8 would be far too few.)
Comment on attachment 104441 [details] [review] [PATCH 3/3] new test: max_connections_per_process Review of attachment 104441 [details] [review]: ----------------------------------------------------------------- This test is going to fail on platforms where the pid cannot be determined. OS X and NetBSD seem like the most significant examples. (test/dbus-daemon.c currently also fails on such platforms, if they're Unix.)
I'm not really convinced by the usefulness of this feature. In the absence of LSMs etc., processes with the same uid can ptrace each other, or subvert each other in various other ways, whatever you do.
Comment on attachment 104277 [details] [review] [PATCH 2/3] enforce new limit max_connections_per_process Review of attachment 104277 [details] [review]: ----------------------------------------------------------------- ::: bus/connection.c @@ +219,5 @@ > + _dbus_assert (current_count >= 0); > + > + current_count += adjustment; > + > + _dbus_assert (current_count >= 0); A safer way to do this would be something like (pseudocode): _dbus_assert (current_count >= 0); if (adjustment < 0) { if ((-adjustment) > current_count) fail; } else { if (adjustment > (MAXINT - current_count)) fail; } where "fail" would probably involve returning FALSE.
(In reply to comment #9) > This test is going to fail on platforms where the pid cannot be determined. > OS X and NetBSD seem like the most significant examples. I think it would be OK for this test to be run on those platforms where we know that not knowning the pid would be a regression, currently: #if defined(__FreeBSD__) || defined(__FreeBSD_kernel__) || \ defined(__linux__) || \ defined(__OpenBSD__)
I think this would be obsoleted by Bug #100344, at which point we could close this WONTFIX.
-- GitLab Migration Automatic Message -- This bug has been migrated to freedesktop.org's GitLab instance and has been closed from further activity. You can subscribe and participate further through the new bug through this link to our GitLab instance: https://gitlab.freedesktop.org/dbus/dbus/issues/107.
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.