Bug 82346

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.