Stop listening on DBusServer sockets when reaching max_incomplete_connections

This addresses the parts of CVE-2014-3639 not already addressed by
reducing the default authentication timeout.

Bug: https://bugs.freedesktop.org/show_bug.cgi?id=80851
Bug: https://bugs.freedesktop.org/show_bug.cgi?id=80919
Reviewed-by: Simon McVittie <simon.mcvittie@collabora.co.uk>

4 files changed, 58 insertions, 25 deletions

diff --git a/bus/bus.c b/bus/bus.c
index a514e31d..a3dce244 100644
--- a/bus/bus.c
+++ b/bus/bus.c
@@ -39,6 +39,7 @@
 #include <dbus/dbus-hash.h>
 #include <dbus/dbus-credentials.h>
 #include <dbus/dbus-internals.h>
+#include <dbus/dbus-server-protected.h>
 
 #ifdef DBUS_CYGWIN
 #include <signal.h>
@@ -68,6 +69,7 @@ struct BusContext
   unsigned int keep_umask : 1;
   unsigned int allow_anonymous : 1;
   unsigned int systemd_activation : 1;
+  dbus_bool_t watches_enabled;
 };
 
 static dbus_int32_t server_data_slot = -1;
@@ -758,6 +760,8 @@ bus_context_new (const DBusString *config_file,
       goto failed;
     }
 
+  context->watches_enabled = TRUE;
+
   context->registry = bus_registry_new (context);
   if (context->registry == NULL)
     {
@@ -1658,3 +1662,36 @@ bus_context_check_security_policy (BusContext     *context,
   _dbus_verbose ("security policy allowing message\n");
   return TRUE;
 }
+
+void
+bus_context_check_all_watches (BusContext *context)
+{
+  DBusList *link;
+  dbus_bool_t enabled = TRUE;
+
+  if (bus_connections_get_n_incomplete (context->connections) >=
+      bus_context_get_max_incomplete_connections (context))
+    {
+      enabled = FALSE;
+    }
+
+  if (context->watches_enabled == enabled)
+    return;
+
+  context->watches_enabled = enabled;
+
+  for (link = _dbus_list_get_first_link (&context->servers);
+       link != NULL;
+       link = _dbus_list_get_next_link (&context->servers, link))
+    {
+      /* A BusContext might contains several DBusServer (if there are
+       * several <listen> configuration items) and a DBusServer might
+       * contain several DBusWatch in its DBusWatchList (if getaddrinfo
+       * returns several addresses on a dual IPv4-IPv6 stack or if
+       * systemd passes several fds).
+       * We want to enable/disable them all.
+       */
+      DBusServer *server = link->data;
+      _dbus_server_toggle_all_watches (server, enabled);
+    }
+}
diff --git a/bus/bus.h b/bus/bus.h
index 35978841..400c9d01 100644
--- a/bus/bus.h
+++ b/bus/bus.h
@@ -125,5 +125,6 @@ dbus_bool_t       bus_context_check_security_policy              (BusContext
                                                                   DBusConnection   *proposed_recipient,
                                                                   DBusMessage      *message,
                                                                   DBusError        *error);
+void              bus_context_check_all_watches                  (BusContext       *context);
 
 #endif /* BUS_BUS_H */
diff --git a/bus/connection.c b/bus/connection.c
index ea2d155a..54fa3ab7 100644
--- a/bus/connection.c
+++ b/bus/connection.c
@@ -293,6 +293,10 @@ bus_connection_disconnected (DBusConnection *connection)
           _dbus_list_remove_link (&d->connections->incomplete, d->link_in_connection_list);
           d->link_in_connection_list = NULL;
           d->connections->n_incomplete -= 1;
+
+          /* If we have dropped below the max. number of incomplete
+           * connections, start accept()ing again */
+          bus_context_check_all_watches (d->connections->context);
         }
       
       _dbus_assert (d->connections->n_incomplete >= 0);
@@ -688,31 +692,17 @@ bus_connections_setup_connection (BusConnections *connections,
   
   dbus_connection_ref (connection);
 
-  /* Note that we might disconnect ourselves here, but it only takes
-   * effect on return to the main loop. We call this to free up
-   * expired connections if possible, and to queue the timeout for our
-   * own expiration.
-   */
   bus_connections_expire_incomplete (connections);
   
-  /* And we might also disconnect ourselves here, but again it
-   * only takes effect on return to main loop.
-   */
-  if (connections->n_incomplete >
-      bus_context_get_max_incomplete_connections (connections->context))
-    {
-      _dbus_verbose ("Number of incomplete connections exceeds max, dropping oldest one\n");
-      
-      _dbus_assert (connections->incomplete != NULL);
-      /* Disconnect the oldest unauthenticated connection.  FIXME
-       * would it be more secure to drop a *random* connection?  This
-       * algorithm seems to mean that if someone can create new
-       * connections quickly enough, they can keep anyone else from
-       * completing authentication. But random may or may not really
-       * help with that, a more elaborate solution might be required.
-       */
-      dbus_connection_close (connections->incomplete->data);
-    }
+  /* The listening socket is removed from the main loop,
+   * i.e. does not accept(), while n_incomplete is at its
+   * maximum value; so we shouldn't get here in that case */
+  _dbus_assert (connections->n_incomplete <=
+      bus_context_get_max_incomplete_connections (connections->context));
+
+  /* If we have the maximum number of incomplete connections,
+   * stop accept()ing any more, to avert a DoS. See fd.o #80919 */
+  bus_context_check_all_watches (d->connections->context);
   
   retval = TRUE;
 
@@ -1419,6 +1409,10 @@ bus_connection_complete (DBusConnection   *connection,
   _dbus_assert (d->connections->n_incomplete >= 0);
   _dbus_assert (d->connections->n_completed > 0);
 
+  /* If we have dropped below the max. number of incomplete
+   * connections, start accept()ing again */
+  bus_context_check_all_watches (d->connections->context);
+
   /* See if we can remove the timeout */
   bus_connections_expire_incomplete (d->connections);
 
@@ -2348,7 +2342,6 @@ bus_transaction_add_cancel_hook (BusTransaction               *transaction,
   return TRUE;
 }
 
-#ifdef DBUS_ENABLE_STATS
 int
 bus_connections_get_n_active (BusConnections *connections)
 {
@@ -2361,6 +2354,7 @@ bus_connections_get_n_incomplete (BusConnections *connections)
   return connections->n_incomplete;
 }
 
+#ifdef DBUS_ENABLE_STATS
 int
 bus_connections_get_total_match_rules (BusConnections *connections)
 {
diff --git a/bus/connection.h b/bus/connection.h
index 9f4f9aea..6fbcd38d 100644
--- a/bus/connection.h
+++ b/bus/connection.h
@@ -139,9 +139,10 @@ dbus_bool_t     bus_transaction_add_cancel_hook  (BusTransaction               *
                                                   void                         *data,
                                                   DBusFreeFunction              free_data_function);
 
-/* called by stats.c, only present if DBUS_ENABLE_STATS */
 int bus_connections_get_n_active                  (BusConnections *connections);
 int bus_connections_get_n_incomplete              (BusConnections *connections);
+
+/* called by stats.c, only present if DBUS_ENABLE_STATS */
 int bus_connections_get_total_match_rules         (BusConnections *connections);
 int bus_connections_get_peak_match_rules          (BusConnections *connections);
 int bus_connections_get_peak_match_rules_per_conn (BusConnections *connections);