From 8ad179a8dad789fc6a5402780044bc0ec3d41115 Mon Sep 17 00:00:00 2001 From: Alban Crequy Date: Fri, 4 Jul 2014 15:05:51 +0100 Subject: 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 --- bus/bus.c | 37 +++++++++++++++++++++++++++++++++++++ bus/bus.h | 1 + bus/connection.c | 42 ++++++++++++++++++------------------------ bus/connection.h | 3 ++- 4 files changed, 58 insertions(+), 25 deletions(-) (limited to 'bus') 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 #include #include +#include #ifdef DBUS_CYGWIN #include @@ -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 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); -- cgit v1.2.3