From 89219baab0bf6ff05142518110f45c8159be8092 Mon Sep 17 00:00:00 2001
From: Alban Crequy <alban.crequy@collabora.co.uk>
Date: Fri, 4 Jul 2014 15:05:51 +0100
Subject: [PATCH 04/10] 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>
(cherry picked from commit 8ad179a8dad789fc6a5402780044bc0ec3d41115)
---
bus/bus.c | 37 +++++++++++++++++++++++++++++++++++++
bus/bus.h | 1 +
bus/connection.c | 42 ++++++++++++++++++------------------------
bus/connection.h | 3 ++-
dbus/dbus-server-protected.h | 5 ++---
dbus/dbus-server.c | 19 +++++--------------
dbus/dbus-watch.c | 21 +++++++++++++++++++++
dbus/dbus-watch.h | 2 ++
8 files changed, 88 insertions(+), 42 deletions(-)
diff --git a/bus/bus.c b/bus/bus.c
index e80e708..7ffe772 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;
@@ -746,6 +748,8 @@ bus_context_new (const DBusString *config_file,
goto failed;
}
+ context->watches_enabled = TRUE;
+
context->registry = bus_registry_new (context);
if (context->registry == NULL)
{
@@ -1646,3 +1650,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
--- 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 d69758c..b3d87a7 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);
@@ -2316,7 +2310,6 @@ bus_transaction_add_cancel_hook (BusTransaction *transaction,
return TRUE;
}
-#ifdef DBUS_ENABLE_STATS
int
bus_connections_get_n_active (BusConnections *connections)
{
@@ -2329,6 +2322,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 c936021..06846e7 100644
--- a/bus/connection.h
+++ b/bus/connection.h
@@ -138,9 +138,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
--- 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 b62c2b4..0b8ef00 100644
--- 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 b9f4ac2..ece5cb5 100644
--- a/dbus/dbus-watch.c
+++ b/dbus/dbus-watch.c
@@ -454,6 +454,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
--- 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);
--
2.1.0
