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>

diff --git a/bus/bus.c b/bus/bus.c
index a514e31..a3dce24 100644 (file)
--- 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 3597884..400c9d0 100644 (file)
--- 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 ea2d155..54fa3ab 100644 (file)
--- 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 9f4f9ae..6fbcd38 100644 (file)
--- 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);

diff --git a/dbus/dbus-server-protected.h b/dbus/dbus-server-protected.h
index dd5234b..e6dbd1e 100644 (file)
--- a/dbus/dbus-server-protected.h
+++ b/dbus/dbus-server-protected.h
@@ -99,9 +99,8 @@ dbus_bool_t _dbus_server_add_watch      (DBusServer             *server,
                                          DBusWatch              *watch);
 void        _dbus_server_remove_watch   (DBusServer             *server,
                                          DBusWatch              *watch);
-void        _dbus_server_toggle_watch   (DBusServer             *server,
-                                         DBusWatch              *watch,
-                                         dbus_bool_t             enabled);
+void        _dbus_server_toggle_all_watches (DBusServer         *server,
+                                             dbus_bool_t         enabled);
 dbus_bool_t _dbus_server_add_timeout    (DBusServer             *server,
                                          DBusTimeout            *timeout);
 void        _dbus_server_remove_timeout (DBusServer             *server,

diff --git a/dbus/dbus-server.c b/dbus/dbus-server.c
index 19d8590..c1d5f6e 100644 (file)
--- a/dbus/dbus-server.c
+++ b/dbus/dbus-server.c
@@ -312,26 +312,17 @@ _dbus_server_remove_watch  (DBusServer *server,
 }
 
 /**
- * Toggles a watch and notifies app via server's
- * DBusWatchToggledFunction if available. It's an error to call this
- * function on a watch that was not previously added.
+ * Toggles all watch and notifies app via server's
+ * DBusWatchToggledFunction if available.
  *
  * @param server the server.
- * @param watch the watch to toggle.
  * @param enabled whether to enable or disable
  */
 void
-_dbus_server_toggle_watch (DBusServer  *server,
-                           DBusWatch   *watch,
-                           dbus_bool_t  enabled)
+_dbus_server_toggle_all_watches (DBusServer  *server,
+                                dbus_bool_t  enabled)
 {
-  _dbus_assert (watch != NULL);
-
-  HAVE_LOCK_CHECK (server);
-  protected_change_watch (server, watch,
-                          NULL, NULL,
-                          _dbus_watch_list_toggle_watch,
-                          enabled);
+  _dbus_watch_list_toggle_all_watches (server->watches, enabled);
 }
 
 /** Function to be called in protected_change_timeout() with refcount held */

diff --git a/dbus/dbus-watch.c b/dbus/dbus-watch.c
index b82c57d..76a5d64 100644 (file)
--- a/dbus/dbus-watch.c
+++ b/dbus/dbus-watch.c
@@ -455,6 +455,27 @@ _dbus_watch_list_toggle_watch (DBusWatchList           *watch_list,
 }
 
 /**
+ * Sets all watches to the given enabled state, invoking the
+ * application's DBusWatchToggledFunction if appropriate.
+ *
+ * @param watch_list the watch list.
+ * @param enabled #TRUE to enable
+ */
+void
+_dbus_watch_list_toggle_all_watches (DBusWatchList           *watch_list,
+                                     dbus_bool_t              enabled)
+{
+  DBusList *link;
+
+  for (link = _dbus_list_get_first_link (&watch_list->watches);
+       link != NULL;
+       link = _dbus_list_get_next_link (&watch_list->watches, link))
+    {
+      _dbus_watch_list_toggle_watch (watch_list, link->data, enabled);
+    }
+}
+
+/**
  * Sets the handler for the watch.
  *
  * @todo this function only exists because of the weird

diff --git a/dbus/dbus-watch.h b/dbus/dbus-watch.h
index c583214..321740e 100644 (file)
--- a/dbus/dbus-watch.h
+++ b/dbus/dbus-watch.h
@@ -76,6 +76,8 @@ void           _dbus_watch_list_remove_watch  (DBusWatchList           *watch_li
 void           _dbus_watch_list_toggle_watch  (DBusWatchList           *watch_list,
                                                DBusWatch               *watch,
                                                dbus_bool_t              enabled);
+void           _dbus_watch_list_toggle_all_watches (DBusWatchList      *watch_list,
+                                               dbus_bool_t              enabled);
 dbus_bool_t    _dbus_watch_get_enabled        (DBusWatch              *watch);
 
 dbus_bool_t    _dbus_watch_get_oom_last_time  (DBusWatch               *watch);