From 416898324decb7b6d86083a6e2ddc71e8d835feb Mon Sep 17 00:00:00 2001 From: Simon McVittie Date: Wed, 30 Apr 2014 20:58:39 +0100 Subject: development version --- NEWS | 3 +++ configure.ac | 2 +- 2 files changed, 4 insertions(+), 1 deletion(-) diff --git a/NEWS b/NEWS index 1ed70b3f..9a2b8890 100644 --- a/NEWS +++ b/NEWS @@ -1,3 +1,6 @@ +D-Bus 1.8.3 (UNRELEASED) +== + D-Bus 1.8.2 (2014-04-30) == diff --git a/configure.ac b/configure.ac index eccdd30f..767662d2 100644 --- a/configure.ac +++ b/configure.ac @@ -3,7 +3,7 @@ AC_PREREQ([2.63]) m4_define([dbus_major_version], [1]) m4_define([dbus_minor_version], [8]) -m4_define([dbus_micro_version], [2]) +m4_define([dbus_micro_version], [3]) m4_define([dbus_version], [dbus_major_version.dbus_minor_version.dbus_micro_version]) AC_INIT([dbus],[dbus_version],[https://bugs.freedesktop.org/enter_bug.cgi?product=dbus],[dbus]) -- cgit v1.2.3 From 24c590703ca47eb71ddef453de43126b90954567 Mon Sep 17 00:00:00 2001 From: Alban Crequy Date: Tue, 20 May 2014 14:37:37 +0100 Subject: CVE-2014-3477: deliver activation errors correctly, fixing Denial of Service How it should work: When a D-Bus message activates a service, LSMs (SELinux or AppArmor) check whether the message can be delivered after the service has been activated. The service is considered activated when its well-known name is requested with org.freedesktop.DBus.RequestName. When the message delivery is denied, the service stays activated but should not receive the activating message (the message which triggered the activation). dbus-daemon is supposed to drop the activating message and reply to the sender with a D-Bus error message. However, it does not work as expected: 1. The error message is delivered to the service instead of being delivered to the sender. As an example, the error message could be something like: An SELinux policy prevents this sender from sending this message to this recipient, [...] member="MaliciousMethod" If the sender and the service are malicious confederates and agree on a protocol to insert information in the member name, the sender can leak information to the service, even though the LSM attempted to block the communication between the sender and the service. 2. The error message is delivered as a reply to the RequestName call from service. It means the activated service will believe it cannot request the name and might exit. The sender could activate the service frequently and systemd will give up activating it. Thus the denial of service. The following changes fix the bug: - bus_activation_send_pending_auto_activation_messages() only returns an error in case of OOM. The prototype is changed to return TRUE, or FALSE on OOM (and its only caller sets the OOM error). - When a client is not allowed to talk to the service, a D-Bus error message is pre-allocated to be delivered to the client as part of the transaction. The error is not propagated to the caller so RequestName will not fail (except on OOM). [fixed a misleading comment -smcv] Bug: https://bugs.freedesktop.org/show_bug.cgi?id=78979 Reviewed-by: Simon McVittie Reviewed-by: Colin Walters --- bus/activation.c | 27 ++++++++++++++++++++------- bus/activation.h | 3 +-- bus/services.c | 5 +++-- 3 files changed, 24 insertions(+), 11 deletions(-) diff --git a/bus/activation.c b/bus/activation.c index fa6c1568..149cca8a 100644 --- a/bus/activation.c +++ b/bus/activation.c @@ -1162,14 +1162,11 @@ bus_activation_service_created (BusActivation *activation, dbus_bool_t bus_activation_send_pending_auto_activation_messages (BusActivation *activation, BusService *service, - BusTransaction *transaction, - DBusError *error) + BusTransaction *transaction) { BusPendingActivation *pending_activation; DBusList *link; - _DBUS_ASSERT_ERROR_IS_CLEAR (error); - /* Check if it's a pending activation */ pending_activation = _dbus_hash_table_lookup_string (activation->pending_activations, bus_service_get_name (service)); @@ -1186,6 +1183,9 @@ bus_activation_send_pending_auto_activation_messages (BusActivation *activation if (entry->auto_activation && (entry->connection == NULL || dbus_connection_get_is_connected (entry->connection))) { DBusConnection *addressed_recipient; + DBusError error; + + dbus_error_init (&error); addressed_recipient = bus_service_get_primary_owners_connection (service); @@ -1193,8 +1193,22 @@ bus_activation_send_pending_auto_activation_messages (BusActivation *activation if (!bus_dispatch_matches (transaction, entry->connection, addressed_recipient, - entry->activation_message, error)) - goto error; + entry->activation_message, &error)) + { + /* If permission is denied, we just want to return the error + * to the original method invoker; in particular, we don't + * want to make the RequestName call fail with that error + * (see fd.o #78979, CVE-2014-3477). */ + if (!bus_transaction_send_error_reply (transaction, entry->connection, + &error, entry->activation_message)) + { + bus_connection_send_oom_error (entry->connection, + entry->activation_message); + } + + link = next; + continue; + } } link = next; @@ -1203,7 +1217,6 @@ bus_activation_send_pending_auto_activation_messages (BusActivation *activation if (!add_restore_pending_to_transaction (transaction, pending_activation)) { _dbus_verbose ("Could not add cancel hook to transaction to revert removing pending activation\n"); - BUS_SET_OOM (error); goto error; } diff --git a/bus/activation.h b/bus/activation.h index 97f25b1f..fc5d426f 100644 --- a/bus/activation.h +++ b/bus/activation.h @@ -62,8 +62,7 @@ dbus_bool_t dbus_activation_systemd_failure (BusActivation *activation, dbus_bool_t bus_activation_send_pending_auto_activation_messages (BusActivation *activation, BusService *service, - BusTransaction *transaction, - DBusError *error); + BusTransaction *transaction); #endif /* BUS_ACTIVATION_H */ diff --git a/bus/services.c b/bus/services.c index 01a720ed..584485b1 100644 --- a/bus/services.c +++ b/bus/services.c @@ -588,8 +588,9 @@ bus_registry_acquire_service (BusRegistry *registry, activation = bus_context_get_activation (registry->context); retval = bus_activation_send_pending_auto_activation_messages (activation, service, - transaction, - error); + transaction); + if (!retval) + BUS_SET_OOM (error); out: return retval; -- cgit v1.2.3 From c3650785feef855647f5424e6331a791f4f22f97 Mon Sep 17 00:00:00 2001 From: Simon McVittie Date: Thu, 5 Jun 2014 14:54:36 +0100 Subject: Prepare embargoed security release --- NEWS | 10 +++++++++- configure.ac | 4 ++-- 2 files changed, 11 insertions(+), 3 deletions(-) diff --git a/NEWS b/NEWS index 9a2b8890..8ad88829 100644 --- a/NEWS +++ b/NEWS @@ -1,6 +1,14 @@ -D-Bus 1.8.3 (UNRELEASED) +D-Bus 1.8.4 (2014-06-10) == +Security fix: + +• Alban Crequy at Collabora Ltd. discovered and fixed a denial-of-service + flaw in dbus-daemon, part of the reference implementation of D-Bus. + Additionally, in highly unusual environments the same flaw could lead to + a side channel between processes that should not be able to communicate. + (CVE-2014-3477, fd.o #78979) + D-Bus 1.8.2 (2014-04-30) == diff --git a/configure.ac b/configure.ac index 767662d2..034388ac 100644 --- a/configure.ac +++ b/configure.ac @@ -3,7 +3,7 @@ AC_PREREQ([2.63]) m4_define([dbus_major_version], [1]) m4_define([dbus_minor_version], [8]) -m4_define([dbus_micro_version], [3]) +m4_define([dbus_micro_version], [4]) m4_define([dbus_version], [dbus_major_version.dbus_minor_version.dbus_micro_version]) AC_INIT([dbus],[dbus_version],[https://bugs.freedesktop.org/enter_bug.cgi?product=dbus],[dbus]) @@ -37,7 +37,7 @@ LT_CURRENT=11 ## increment any time the source changes; set to ## 0 if you increment CURRENT -LT_REVISION=4 +LT_REVISION=5 ## increment if any interfaces have been added; set to 0 ## if any interfaces have been changed or removed. removal has -- cgit v1.2.3 From 3488de849d09de774e4df7f1a4463ce2ee39d861 Mon Sep 17 00:00:00 2001 From: Simon McVittie Date: Tue, 10 Jun 2014 18:43:40 +0100 Subject: reset version --- NEWS | 5 +++++ configure.ac | 2 +- 2 files changed, 6 insertions(+), 1 deletion(-) diff --git a/NEWS b/NEWS index 8ad88829..15ed316c 100644 --- a/NEWS +++ b/NEWS @@ -1,3 +1,8 @@ +D-Bus 1.8.6 (UNRELEASED) +== + +... + D-Bus 1.8.4 (2014-06-10) == diff --git a/configure.ac b/configure.ac index 034388ac..13d0aa94 100644 --- a/configure.ac +++ b/configure.ac @@ -3,7 +3,7 @@ AC_PREREQ([2.63]) m4_define([dbus_major_version], [1]) m4_define([dbus_minor_version], [8]) -m4_define([dbus_micro_version], [4]) +m4_define([dbus_micro_version], [5]) m4_define([dbus_version], [dbus_major_version.dbus_minor_version.dbus_micro_version]) AC_INIT([dbus],[dbus_version],[https://bugs.freedesktop.org/enter_bug.cgi?product=dbus],[dbus]) -- cgit v1.2.3 From 5c7ea35c3dfc564f5c5679a663d5b9c101c61910 Mon Sep 17 00:00:00 2001 From: Роман Донченко Date: Wed, 11 Jun 2014 11:38:24 +0100 Subject: dbus-launch: kill bus if we can't attach to a session when requested Bug: https://bugs.freedesktop.org/show_bug.cgi?id=74698 Reviewed-by: Simon McVittie --- tools/dbus-launch.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tools/dbus-launch.c b/tools/dbus-launch.c index 58a0322e..41a20e83 100644 --- a/tools/dbus-launch.c +++ b/tools/dbus-launch.c @@ -536,7 +536,7 @@ kill_bus_when_session_ends (void) if (tty_fd < 0 && x_fd < 0) { fprintf (stderr, "No terminal on standard input and no X display; cannot attach message bus to session lifetime\n"); - exit (1); + kill_bus_and_exit (1); } while (TRUE) -- cgit v1.2.3 From 019d19214326115bfb53401db3023fe31caa280f Mon Sep 17 00:00:00 2001 From: Simon McVittie Date: Wed, 11 Jun 2014 11:45:28 +0100 Subject: NEWS --- NEWS | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/NEWS b/NEWS index 15ed316c..48d3e9b0 100644 --- a/NEWS +++ b/NEWS @@ -1,7 +1,11 @@ D-Bus 1.8.6 (UNRELEASED) == -... +Fixes: + +• When dbus-launch --exit-with-session starts a dbus-daemon but then cannot + attach to a session, kill the dbus-daemon as intended + (fd.o #74698, Роман Донченко) D-Bus 1.8.4 (2014-06-10) == -- cgit v1.2.3 From 07f4c12efe3b9bd45d109bc5fbaf6d9dbf69d78e Mon Sep 17 00:00:00 2001 From: Simon McVittie Date: Wed, 11 Jun 2014 12:24:20 +0100 Subject: If loader contains two messages with fds, don't corrupt the second There were two bugs here: we would previously overwrite the unused fds with the already-used fds instead of the other way round, and we would copy n bytes where we should have copied n ints. Additionally, sending crafted messages in a chosen sequence to a victim system service could cause an invalid file descriptor to be present when dbus-daemon tries to forward one of those crafted messages to the victim, causing sendmsg() to fail with EBADF, which resulted in disconnecting the victim service, which would likely respond to that by exiting. This is a denial of service (fd.o #80469, CVE-2014-3533). Bug: https://bugs.freedesktop.org/show_bug.cgi?id=79694 Bug: https://bugs.freedesktop.org/show_bug.cgi?id=80469 Reviewed-by: Alban Crequy --- dbus/dbus-message.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/dbus/dbus-message.c b/dbus/dbus-message.c index c6953d02..78df7558 100644 --- a/dbus/dbus-message.c +++ b/dbus/dbus-message.c @@ -4204,7 +4204,7 @@ load_message (DBusMessageLoader *loader, message->n_unix_fds_allocated = message->n_unix_fds = n_unix_fds; loader->n_unix_fds -= n_unix_fds; - memmove(loader->unix_fds + n_unix_fds, loader->unix_fds, loader->n_unix_fds); + memmove (loader->unix_fds, loader->unix_fds + n_unix_fds, loader->n_unix_fds * sizeof (loader->unix_fds[0])); } else message->unix_fds = NULL; -- cgit v1.2.3 From 9ca90648fc870c24d852ce6d7ce9387a9fc9a94a Mon Sep 17 00:00:00 2001 From: Alban Crequy Date: Tue, 24 Jun 2014 17:57:14 +0100 Subject: Handle ETOOMANYREFS when sending recursive fds (SCM_RIGHTS) Since Linux commit 25888e (from 2.6.37-rc4, Nov 2010), sendmsg() on Unix sockets returns -1 errno=ETOOMANYREFS ("Too many references: cannot splice") when the passfd mechanism (SCM_RIGHTS) is "abusively" used recursively by applications. A malicious client could use this to force a victim system service to be disconnected from the system bus; the victim would likely respond by exiting. This is a denial of service (fd.o #80163, CVE-2014-3532). This patch silently drops the D-Bus message on ETOOMANYREFS and does not close the connection. Bug: https://bugs.freedesktop.org/show_bug.cgi?id=80163 Reviewed-by: Thiago Macieira [altered commit message to explain DoS significance -smcv] Reviewed-by: Simon McVittie --- dbus/dbus-sysdeps.c | 14 ++++++++++++++ dbus/dbus-sysdeps.h | 1 + dbus/dbus-transport-socket.c | 34 +++++++++++++++++++++++++++++++++- 3 files changed, 48 insertions(+), 1 deletion(-) diff --git a/dbus/dbus-sysdeps.c b/dbus/dbus-sysdeps.c index de3a18cb..f4ba0fac 100644 --- a/dbus/dbus-sysdeps.c +++ b/dbus/dbus-sysdeps.c @@ -761,6 +761,20 @@ _dbus_get_is_errno_epipe (void) return errno == EPIPE; } +/** + * See if errno is ETOOMANYREFS + * @returns #TRUE if errno == ETOOMANYREFS + */ +dbus_bool_t +_dbus_get_is_errno_etoomanyrefs (void) +{ +#ifdef ETOOMANYREFS + return errno == ETOOMANYREFS; +#else + return FALSE; +#endif +} + /** * Get error message from errno * @returns _dbus_strerror(errno) diff --git a/dbus/dbus-sysdeps.h b/dbus/dbus-sysdeps.h index e586946f..21033ebf 100644 --- a/dbus/dbus-sysdeps.h +++ b/dbus/dbus-sysdeps.h @@ -384,6 +384,7 @@ dbus_bool_t _dbus_get_is_errno_eagain_or_ewouldblock (void); dbus_bool_t _dbus_get_is_errno_enomem (void); dbus_bool_t _dbus_get_is_errno_eintr (void); dbus_bool_t _dbus_get_is_errno_epipe (void); +dbus_bool_t _dbus_get_is_errno_etoomanyrefs (void); const char* _dbus_strerror_from_errno (void); void _dbus_disable_sigpipe (void); diff --git a/dbus/dbus-transport-socket.c b/dbus/dbus-transport-socket.c index 774f4598..199d3b54 100644 --- a/dbus/dbus-transport-socket.c +++ b/dbus/dbus-transport-socket.c @@ -645,12 +645,44 @@ do_writing (DBusTransport *transport) { /* EINTR already handled for us */ - /* For some discussion of why we also ignore EPIPE here, see + /* If the other end closed the socket with close() or shutdown(), we + * receive EPIPE here but we must not close the socket yet: there + * might still be some data to read. See: * http://lists.freedesktop.org/archives/dbus/2008-March/009526.html */ if (_dbus_get_is_errno_eagain_or_ewouldblock () || _dbus_get_is_errno_epipe ()) goto out; + + /* Since Linux commit 25888e (from 2.6.37-rc4, Nov 2010), sendmsg() + * on Unix sockets returns -1 errno=ETOOMANYREFS when the passfd + * mechanism (SCM_RIGHTS) is used recursively with a recursion level + * of maximum 4. The kernel does not have an API to check whether + * the passed fds can be forwarded and it can change asynchronously. + * See: + * https://bugs.freedesktop.org/show_bug.cgi?id=80163 + */ + + else if (_dbus_get_is_errno_etoomanyrefs ()) + { + /* We only send fds in the first byte of the message. + * ETOOMANYREFS cannot happen after. + */ + _dbus_assert (socket_transport->message_bytes_written == 0); + + _dbus_verbose (" discard message of %d bytes due to ETOOMANYREFS\n", + total_bytes_to_write); + + socket_transport->message_bytes_written = 0; + _dbus_string_set_length (&socket_transport->encoded_outgoing, 0); + _dbus_string_compact (&socket_transport->encoded_outgoing, 2048); + + /* The message was not actually sent but it needs to be removed + * from the outgoing queue + */ + _dbus_connection_message_sent_unlocked (transport->connection, + message); + } else { _dbus_verbose ("Error writing to remote app: %s\n", -- cgit v1.2.3 From 194f6f758983aacad4ea32dc0038ef19d23c6e21 Mon Sep 17 00:00:00 2001 From: Simon McVittie Date: Mon, 30 Jun 2014 14:18:03 +0100 Subject: Prepare 1.8.6 in advance --- NEWS | 19 +++++++++++++++++-- configure.ac | 4 ++-- 2 files changed, 19 insertions(+), 4 deletions(-) diff --git a/NEWS b/NEWS index 48d3e9b0..0944bf42 100644 --- a/NEWS +++ b/NEWS @@ -1,7 +1,22 @@ -D-Bus 1.8.6 (UNRELEASED) +D-Bus 1.8.6 (2014-06-02) == -Fixes: +Security fixes: + +• On Linux ≥ 2.6.37-rc4, if sendmsg() fails with ETOOMANYREFS, silently drop + the message. This prevents an attack in which a malicious client can + make dbus-daemon disconnect a system service, which is a local + denial of service. + (fd.o #80163, CVE-2014-3532; Alban Crequy) + +• Track remaining Unix file descriptors correctly when more than one + message in quick succession contains fds. This prevents another attack + in which a malicious client can make dbus-daemon disconnect a system + service. + (fd.o #79694, fd.o #80469, CVE-2014-3533; Alejandro Martínez Suárez, + Simon McVittie, Alban Crequy) + +Other fixes: • When dbus-launch --exit-with-session starts a dbus-daemon but then cannot attach to a session, kill the dbus-daemon as intended diff --git a/configure.ac b/configure.ac index 13d0aa94..8ffbb5c3 100644 --- a/configure.ac +++ b/configure.ac @@ -3,7 +3,7 @@ AC_PREREQ([2.63]) m4_define([dbus_major_version], [1]) m4_define([dbus_minor_version], [8]) -m4_define([dbus_micro_version], [5]) +m4_define([dbus_micro_version], [6]) m4_define([dbus_version], [dbus_major_version.dbus_minor_version.dbus_micro_version]) AC_INIT([dbus],[dbus_version],[https://bugs.freedesktop.org/enter_bug.cgi?product=dbus],[dbus]) @@ -37,7 +37,7 @@ LT_CURRENT=11 ## increment any time the source changes; set to ## 0 if you increment CURRENT -LT_REVISION=5 +LT_REVISION=6 ## increment if any interfaces have been added; set to 0 ## if any interfaces have been changed or removed. removal has -- cgit v1.2.3 From 8f31484171dbffea56600daf6b6be407bd79d759 Mon Sep 17 00:00:00 2001 From: Simon McVittie Date: Wed, 2 Jul 2014 18:24:44 +0100 Subject: start 1.8.7 --- NEWS | 5 +++++ configure.ac | 2 +- 2 files changed, 6 insertions(+), 1 deletion(-) diff --git a/NEWS b/NEWS index 0944bf42..3d5373fc 100644 --- a/NEWS +++ b/NEWS @@ -1,3 +1,8 @@ +D-Bus 1.8.8 (UNRELEASED) +== + +... + D-Bus 1.8.6 (2014-06-02) == diff --git a/configure.ac b/configure.ac index 8ffbb5c3..5412db2e 100644 --- a/configure.ac +++ b/configure.ac @@ -3,7 +3,7 @@ AC_PREREQ([2.63]) m4_define([dbus_major_version], [1]) m4_define([dbus_minor_version], [8]) -m4_define([dbus_micro_version], [6]) +m4_define([dbus_micro_version], [7]) m4_define([dbus_version], [dbus_major_version.dbus_minor_version.dbus_micro_version]) AC_INIT([dbus],[dbus_version],[https://bugs.freedesktop.org/enter_bug.cgi?product=dbus],[dbus]) -- cgit v1.2.3 From e2a78c0951212a4f22f95cffe29aa96e11120050 Mon Sep 17 00:00:00 2001 From: Alban Crequy Date: Thu, 24 Jul 2014 08:04:48 +0100 Subject: Stats: fix compilation issue Bug-Gentoo: https://bugs.gentoo.org/show_bug.cgi?id=507232 Bug: https://bugs.freedesktop.org/show_bug.cgi?id=81043 Reviewed-by: Simon McVittie --- bus/stats.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/bus/stats.c b/bus/stats.c index 2bf86d66..24308eb5 100644 --- a/bus/stats.c +++ b/bus/stats.c @@ -40,6 +40,7 @@ bus_stats_handle_get_stats (DBusConnection *connection, DBusMessage *message, DBusError *error) { + BusContext *context; BusConnections *connections; DBusMessage *reply = NULL; DBusMessageIter iter, arr_iter; @@ -48,7 +49,8 @@ bus_stats_handle_get_stats (DBusConnection *connection, _DBUS_ASSERT_ERROR_IS_CLEAR (error); - connections = bus_context_get_connections (transaction->context); + context = bus_transaction_get_context (transaction); + connections = bus_context_get_connections (context); reply = _dbus_asv_new_method_return (message, &iter, &arr_iter); -- cgit v1.2.3 From 496ebc6c5ed2752b081d14baa6fed9defc434e68 Mon Sep 17 00:00:00 2001 From: Simon McVittie Date: Fri, 5 Sep 2014 16:48:00 +0100 Subject: NEWS for 1.8 --- NEWS | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/NEWS b/NEWS index 3d5373fc..139345fc 100644 --- a/NEWS +++ b/NEWS @@ -1,7 +1,10 @@ D-Bus 1.8.8 (UNRELEASED) == -... +Fixes: + +• Fix compilation with --enable-stats (fd.o #81043, Gentoo #507232; + Alban Crequy) D-Bus 1.8.6 (2014-06-02) == -- cgit v1.2.3 From 575256cd48d583f2d6409c43a10a572939a01b00 Mon Sep 17 00:00:00 2001 From: Ralf Habacker Date: Wed, 15 Jan 2014 00:45:41 +0100 Subject: Fix windows doc for running tests. Bug: https://bugs.freedesktop.org/show_bug.cgi?id=41252 Reviewed-by: Simon McVittie --- README.win | 13 +++++++++---- 1 file changed, 9 insertions(+), 4 deletions(-) diff --git a/README.win b/README.win index bd67c188..8a5b3c0a 100644 --- a/README.win +++ b/README.win @@ -13,7 +13,7 @@ test not running yet and there is help needed to get them running. Supported compilers ------------------- -On windows Microsoft Visual Studio 2010 (Express and professional variants) +On windows Microsoft Visual Studio 2010 (Express and professional variants) and mingw-w64|32 are known to work. Building @@ -30,14 +30,19 @@ updated with windows specific stuff. Tests ----- + - run complete test suite + make check + or + ctest [-V] + - dbus library check - bin\test-dbus.exe \test\data + ctest [-V] -R test-dbus - bus daemon check - bin\test-bus.exe \test\data + ctest [-V] -R test-bus - check available names - bin\test_names.exe + ctest [-V] -R test-names - check if dbus-daemon is accessable bin\dbus-send.exe --session --type=method_call --print-reply --dest=org.freedesktop.DBus / org.freedesktop.DBus.ListNames method return sender=org.freedesktop.DBus -> dest=:1.4 array [ string "org.freedesktop.DBus"string ":1.4"] -- cgit v1.2.3 From ae268a2b1aaa14e16ffc47b3cd5ad74a26e39838 Mon Sep 17 00:00:00 2001 From: Umut Tezduyar Lindskog Date: Tue, 2 Sep 2014 09:02:31 +0200 Subject: enable build support without systemd compatibility libraries systemd 209 merged all the libraries to libsystemd. Old libraries can still be enabled with --enable-compat-libs switch in systemd but this increases the binary size. Implement a fallback library check in case compat libraries dont exist. [Fixed underquoting; switched priority so we try libsystemd first -smcv] Signed-off-by: Simon McVittie --- configure.ac | 11 +++++++---- 1 file changed, 7 insertions(+), 4 deletions(-) diff --git a/configure.ac b/configure.ac index 5412db2e..b7b91bec 100644 --- a/configure.ac +++ b/configure.ac @@ -1143,10 +1143,13 @@ dnl systemd detection if test x$enable_systemd = xno ; then have_systemd=no; else - PKG_CHECK_MODULES(SYSTEMD, - [libsystemd-login >= 32, libsystemd-daemon >= 32, libsystemd-journal >= 32], - have_systemd=yes, - have_systemd=no) + PKG_CHECK_MODULES([SYSTEMD], + [libsystemd >= 209], + [have_systemd=yes], + [PKG_CHECK_MODULES([SYSTEMD], + [libsystemd-login >= 32, libsystemd-daemon >= 32, libsystemd-journal >= 32], + [have_systemd=yes], + [have_systemd=no])]) fi if test x$have_systemd = xyes; then -- cgit v1.2.3 From 467f7a6d426a37ddc0f9401f592facad3d003dfa Mon Sep 17 00:00:00 2001 From: Simon McVittie Date: Fri, 12 Sep 2014 12:35:31 +0100 Subject: NEWS for 1.8.x --- NEWS | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/NEWS b/NEWS index 139345fc..2cff8cc8 100644 --- a/NEWS +++ b/NEWS @@ -3,9 +3,16 @@ D-Bus 1.8.8 (UNRELEASED) Fixes: +• Check for libsystemd from systemd >= 209, falling back to + the older separate libraries if not found (Umut Tezduyar Lindskog, + Simon McVittie) + • Fix compilation with --enable-stats (fd.o #81043, Gentoo #507232; Alban Crequy) +• Improve documentation for running tests on Windows (fd.o #41252, + Ralf Habacker) + D-Bus 1.8.6 (2014-06-02) == -- cgit v1.2.3 From ae50d46ff26de73ec5f8390abcca879340cb0962 Mon Sep 17 00:00:00 2001 From: Simon McVittie Date: Mon, 15 Sep 2014 11:50:48 +0100 Subject: On Linux, call prctl to disable core dumps Whenever I forget to turn off corekeeper, the regression tests take ages to record all test-segfault's crashes. Bug: https://bugs.freedesktop.org/show_bug.cgi?id=83772 Reviewed-by: Alban Crequy --- configure.ac | 4 ++++ test/test-segfault.c | 18 +++++++++++++++++- 2 files changed, 21 insertions(+), 1 deletion(-) diff --git a/configure.ac b/configure.ac index b7b91bec..c41426f6 100644 --- a/configure.ac +++ b/configure.ac @@ -598,6 +598,10 @@ if test "x$ac_cv_header_syslog_h" = "xyes"; then AC_CHECK_DECLS([LOG_PERROR], [], [], [[#include ]]) fi +# For test-segfault.c +AC_CHECK_HEADERS_ONCE([sys/prctl.h]) +AC_CHECK_FUNCS_ONCE([prctl raise]) + #### Check for broken poll; taken from Glib's configure AC_MSG_CHECKING([for broken poll]) diff --git a/test/test-segfault.c b/test/test-segfault.c index 329a21fd..c062ce1c 100644 --- a/test/test-segfault.c +++ b/test/test-segfault.c @@ -9,18 +9,34 @@ #include #endif +#ifdef HAVE_SYS_PRCTL_H +#include +#endif + int main (int argc, char **argv) { char *p; #if HAVE_SETRLIMIT + /* No core dumps please, we know we crashed. */ struct rlimit r = { 0, }; getrlimit (RLIMIT_CORE, &r); r.rlim_cur = 0; setrlimit (RLIMIT_CORE, &r); - +#endif + +#if defined(HAVE_PRCTL) && defined(PR_SET_DUMPABLE) + /* Really, no core dumps please. On Linux, if core_pattern is + * set to a pipe (for abrt/apport/corekeeper/etc.), RLIMIT_CORE of 0 + * is ignored (deliberately, so people can debug init(8) and other + * early stuff); but Linux has PR_SET_DUMPABLE, so we can avoid core + * dumps anyway. */ + prctl (PR_SET_DUMPABLE, 0, 0, 0, 0); +#endif + +#ifdef HAVE_RAISE raise (SIGSEGV); #endif p = NULL; -- cgit v1.2.3 From 900011a26816c7cc84f0a8b502eea6c4301823d0 Mon Sep 17 00:00:00 2001 From: Simon McVittie Date: Mon, 15 Sep 2014 11:50:57 +0100 Subject: NEWS for 1.8 --- NEWS | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/NEWS b/NEWS index 2cff8cc8..bfab7cbc 100644 --- a/NEWS +++ b/NEWS @@ -7,6 +7,10 @@ Fixes: the older separate libraries if not found (Umut Tezduyar Lindskog, Simon McVittie) +• On Linux, use prctl() to disable core dumps from a test executable + that deliberately raises SIGSEGV to test dbus-daemon's handling + of that condition (fd.o #83772, Simon McVittie) + • Fix compilation with --enable-stats (fd.o #81043, Gentoo #507232; Alban Crequy) -- cgit v1.2.3 From 5bc7f9519ebc6117ba300c704794b36b87c2194b Mon Sep 17 00:00:00 2001 From: Alban Crequy Date: Thu, 10 Jul 2014 15:08:06 +0100 Subject: system bus limit: use max_replies_per_connection=128 by default This addresses CVE-2014-3638. Bug: https://bugs.freedesktop.org/show_bug.cgi?id=81053 Reviewed-by: Simon McVittie --- bus/config-parser.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/bus/config-parser.c b/bus/config-parser.c index a6a8e1cf..7217531c 100644 --- a/bus/config-parser.c +++ b/bus/config-parser.c @@ -467,7 +467,7 @@ bus_config_parser_new (const DBusString *basedir, /* this is effectively a limit on message queue size for messages * that require a reply */ - parser->limits.max_replies_per_connection = 1024*8; + parser->limits.max_replies_per_connection = 128; } parser->refcount = 1; -- cgit v1.2.3 From 6465e37c8ff70a714e302d0c9e6534fa6181fce6 Mon Sep 17 00:00:00 2001 From: Simon McVittie Date: Fri, 12 Sep 2014 15:51:39 +0100 Subject: config: change DEFAULT_MESSAGE_UNIX_FDS to 16 This addresses CVE-2014-3636. Based on a patch by Alban Crequy. Now that it's the same on all platforms, there's little point in it being set by configure/cmake. This change fixes two distinct denials of service: fd.o#82820, part A ------------------ Before this patch, the system bus had the following default configuration: - max_connections_per_user: 256 - DBUS_DEFAULT_MESSAGE_UNIX_FDS: usually 1024 (or 256 on QNX, see fd.o#61176) as defined by configure.ac - max_incoming_unix_fds: DBUS_DEFAULT_MESSAGE_UNIX_FDS*4 = usually 4096 - max_outgoing_unix_fds: DBUS_DEFAULT_MESSAGE_UNIX_FDS*4 = usually 4096 - max_message_unix_fds: DBUS_DEFAULT_MESSAGE_UNIX_FDS = usually 1024 This means that a single user could create 256 connections and transmit 256*4096 = 1048576 file descriptors. The file descriptors stay attached to the dbus-daemon process while they are in the message loader, in the outgoing queue or waiting to be dispatched before D-Bus activation. dbus-daemon is usually limited to 65536 file descriptors (ulimit -n). If the limit is reached and dbus-daemon needs to receive a message with a file descriptor attached, this is signalled by recvfrom with the flag MSG_CTRUNC. Dbus-daemon cannot recover from that error because the kernel does not have any API to retrieve a file descriptor which has been discarded with MSG_CTRUNC. Therefore, it closes the connection of the sender. This is not necessarily the connection which generated the most file descriptors so it can lead to denial-of-service attacks. In order to prevent DoS issues, this patch reduces DEFAULT_MESSAGE_UNIX_FDS to 16: max_connections_per_user * max_incoming_unix_fds = 256 * 64 = 16384 This is less than the usual "ulimit -n" (65536) with a good margin to accomodate the other sources of file descriptors (stdin/stdout/stderr, listening sockets, message loader, etc.). Distributors on non-Linux may need to configure a smaller limit in system.conf, if their limit on the number of fds is smaller than Linux's. fd.o#82820, part B ------------------ On Linux, it's not possible to send more than 253 fds in a single sendmsg() call: sendmsg() would return -EINVAL. #define SCM_MAX_FD 253 SCM_MAX_FD changed value during Linux history: - it used to be (OPEN_MAX-1) - commit c09edd6eb (Jul 2007) changed it to 255 - commit bba14de98 (Nov 2010) changed it to 253 Libdbus always sends all of a message's fds, and the beginning of the message itself, in a single sendmsg() call. Combining these two, a malicious sender could split a message across two or more sendmsg() calls to construct a composite message with 254 or more fds. When dbus-daemon attempted to relay that message to its recipient in a single sendmsg() call, it would receive EINVAL, interpret that as a fatal socket error and disconnect the recipient, resulting in denial of service. This is fixed by keeping max_message_unix_fds <= SCM_MAX_FD. Bug: https://bugs.freedesktop.org/show_bug.cgi?id=82820 Reviewed-by: Alban Crequy --- bus/session.conf.in | 3 ++- cmake/CMakeLists.txt | 4 ---- cmake/config.h.cmake | 2 -- configure.ac | 11 ----------- dbus/dbus-message.c | 1 + dbus/dbus-sysdeps.h | 8 ++++++++ 6 files changed, 11 insertions(+), 18 deletions(-) diff --git a/bus/session.conf.in b/bus/session.conf.in index 74d9d1fd..d4730363 100644 --- a/bus/session.conf.in +++ b/bus/session.conf.in @@ -49,7 +49,8 @@ 1000000000 250000000 1000000000 - @DEFAULT_MESSAGE_UNIX_FDS@ + 120000 240000 100000 diff --git a/cmake/CMakeLists.txt b/cmake/CMakeLists.txt index b7c25299..c767c171 100644 --- a/cmake/CMakeLists.txt +++ b/cmake/CMakeLists.txt @@ -417,10 +417,6 @@ endif (WIN32) set (DBUS_USER ) -# In Autotools this has a different default on QNX, but there seems little -# point in replicating that here; if you're on an unusual Unix, use Autotools. -set (DEFAULT_MESSAGE_UNIX_FDS 1024) - # This won't work on Windows. It's not meant to - the system bus is # meaningless on Windows anyway. # diff --git a/cmake/config.h.cmake b/cmake/config.h.cmake index bd4cd444..eaec1e98 100644 --- a/cmake/config.h.cmake +++ b/cmake/config.h.cmake @@ -82,8 +82,6 @@ # define DBUS_ENABLE_X11_AUTOLAUNCH 1 #endif -#define DBUS_DEFAULT_MESSAGE_UNIX_FDS @DEFAULT_MESSAGE_UNIX_FDS@ - #define _DBUS_VA_COPY_ASSIGN(a1,a2) { a1 = a2; } #cmakedefine DBUS_VA_COPY_FUNC diff --git a/configure.ac b/configure.ac index c41426f6..8a530b29 100644 --- a/configure.ac +++ b/configure.ac @@ -1242,17 +1242,6 @@ if test x$with_valgrind != xno; then AC_DEFINE([WITH_VALGRIND], [1], [Define to add Valgrind instrumentation]) fi -# Determine maximum number of Unix fds which may be passed -AS_CASE([$host_os], - [*qnx*], - [DEFAULT_MESSAGE_UNIX_FDS=256], - [*], - [DEFAULT_MESSAGE_UNIX_FDS=1024]) -AC_DEFINE_UNQUOTED([DBUS_DEFAULT_MESSAGE_UNIX_FDS], - [$DEFAULT_MESSAGE_UNIX_FDS], - [Default for dbus_connection_get_max_message_unix_fds()]) -AC_SUBST([DEFAULT_MESSAGE_UNIX_FDS]) - #### Set up final flags LIBDBUS_LIBS="$THREAD_LIBS $NETWORK_libs" AC_SUBST([LIBDBUS_LIBS]) diff --git a/dbus/dbus-message.c b/dbus/dbus-message.c index 78df7558..f4787b06 100644 --- a/dbus/dbus-message.c +++ b/dbus/dbus-message.c @@ -35,6 +35,7 @@ #include "dbus-list.h" #include "dbus-threads-internal.h" #ifdef HAVE_UNIX_FD_PASSING +#include "dbus-sysdeps.h" #include "dbus-sysdeps-unix.h" #endif diff --git a/dbus/dbus-sysdeps.h b/dbus/dbus-sysdeps.h index 21033ebf..47ba2f43 100644 --- a/dbus/dbus-sysdeps.h +++ b/dbus/dbus-sysdeps.h @@ -558,6 +558,14 @@ void _dbus_request_file_descriptor_limit (unsigned int limit); const char * _dbus_replace_install_prefix (const char *configure_time_path); +/* Do not set this too high: it is a denial-of-service risk. + * See + * + * (This needs to be in the non-Unix-specific header so that + * the config-parser can use it.) + */ +#define DBUS_DEFAULT_MESSAGE_UNIX_FDS 16 + /** @} */ DBUS_END_DECLS -- cgit v1.2.3 From 54d26df52b6a394bea175651d1d7ad2ab3f87dea Mon Sep 17 00:00:00 2001 From: Alban Crequy Date: Tue, 8 Jul 2014 12:00:58 +0100 Subject: config: change default auth_timeout to 5 seconds This partially addresses CVE-2014-3639. This will change the default on the system bus where the limit ... is not specified. Bug: https://bugs.freedesktop.org/show_bug.cgi?id=80919 Reviewed-by: Thiago Macieira Reviewed-by: Simon McVittie --- bus/config-parser.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/bus/config-parser.c b/bus/config-parser.c index 7217531c..814725a1 100644 --- a/bus/config-parser.c +++ b/bus/config-parser.c @@ -438,7 +438,7 @@ bus_config_parser_new (const DBusString *basedir, * and legitimate auth will fail. If interactive auth (ask user for * password) is allowed, then potentially it has to be quite long. */ - parser->limits.auth_timeout = 30000; /* 30 seconds */ + parser->limits.auth_timeout = 5000; /* 5 seconds */ parser->limits.max_incomplete_connections = 64; parser->limits.max_connections_per_user = 256; -- cgit v1.2.3 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 ++- 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 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); diff --git a/dbus/dbus-server-protected.h b/dbus/dbus-server-protected.h index dd5234b9..e6dbd1e1 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 19d8590c..c1d5f6e5 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 b82c57d4..76a5d641 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. * diff --git a/dbus/dbus-watch.h b/dbus/dbus-watch.h index c5832141..321740ed 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); -- cgit v1.2.3 From bbf11cd5f92064c7c8af61ad4d9ff41f3a039abc Mon Sep 17 00:00:00 2001 From: Alban Crequy Date: Mon, 21 Jul 2014 17:34:08 +0100 Subject: config: add new limit: pending_fd_timeout This is one of four commits needed to address CVE-2014-3637. When a file descriptor is passed to dbus-daemon, the associated D-Bus message might not be fully sent to dbus-daemon yet. Dbus-daemon keeps the file descriptor in the DBusMessageLoader of the connection, waiting for the rest of the message. If the client stops sending the remaining bytes, dbus-daemon will wait forever and keep that file descriptor. This patch adds pending_fd_timeout (milliseconds) in the configuration to disconnect a connection after a timeout when a file descriptor was sent but not the remaining message. Bug: https://bugs.freedesktop.org/show_bug.cgi?id=80559 Reviewed-by: Simon McVittie --- bus/bus.c | 6 ++++++ bus/bus.h | 2 ++ bus/config-parser.c | 12 ++++++++++++ bus/session.conf.in | 1 + doc/dbus-daemon.1.xml.in | 4 ++++ 5 files changed, 25 insertions(+) diff --git a/bus/bus.c b/bus/bus.c index a3dce244..35d40754 100644 --- a/bus/bus.c +++ b/bus/bus.c @@ -1240,6 +1240,12 @@ bus_context_get_auth_timeout (BusContext *context) return context->limits.auth_timeout; } +int +bus_context_get_pending_fd_timeout (BusContext *context) +{ + return context->limits.pending_fd_timeout; +} + int bus_context_get_max_completed_connections (BusContext *context) { diff --git a/bus/bus.h b/bus/bus.h index 400c9d01..7d0b3697 100644 --- a/bus/bus.h +++ b/bus/bus.h @@ -54,6 +54,7 @@ typedef struct long max_message_unix_fds; /**< Max number of unix fds of a single message*/ int activation_timeout; /**< How long to wait for an activation to time out */ int auth_timeout; /**< How long to wait for an authentication to time out */ + int pending_fd_timeout; /**< How long to wait for a D-Bus message with a fd to time out */ int max_completed_connections; /**< Max number of authorized connections */ int max_incomplete_connections; /**< Max number of incomplete connections */ int max_connections_per_user; /**< Max number of connections auth'd as same user */ @@ -106,6 +107,7 @@ BusClientPolicy* bus_context_create_client_policy (BusContext DBusError *error); int bus_context_get_activation_timeout (BusContext *context); int bus_context_get_auth_timeout (BusContext *context); +int bus_context_get_pending_fd_timeout (BusContext *context); int bus_context_get_max_completed_connections (BusContext *context); int bus_context_get_max_incomplete_connections (BusContext *context); int bus_context_get_max_connections_per_user (BusContext *context); diff --git a/bus/config-parser.c b/bus/config-parser.c index 814725a1..7bc9c019 100644 --- a/bus/config-parser.c +++ b/bus/config-parser.c @@ -439,6 +439,11 @@ bus_config_parser_new (const DBusString *basedir, * password) is allowed, then potentially it has to be quite long. */ parser->limits.auth_timeout = 5000; /* 5 seconds */ + + /* Do not allow a fd to stay forever in dbus-daemon + * https://bugs.freedesktop.org/show_bug.cgi?id=80559 + */ + parser->limits.pending_fd_timeout = 150000; /* 2.5 minutes */ parser->limits.max_incomplete_connections = 64; parser->limits.max_connections_per_user = 256; @@ -1902,6 +1907,12 @@ set_limit (BusConfigParser *parser, must_be_int = TRUE; parser->limits.auth_timeout = value; } + else if (strcmp (name, "pending_fd_timeout") == 0) + { + must_be_positive = TRUE; + must_be_int = TRUE; + parser->limits.pending_fd_timeout = value; + } else if (strcmp (name, "reply_timeout") == 0) { must_be_positive = TRUE; @@ -3108,6 +3119,7 @@ limits_equal (const BusLimits *a, || a->max_message_unix_fds == b->max_message_unix_fds || a->activation_timeout == b->activation_timeout || a->auth_timeout == b->auth_timeout + || a->pending_fd_timeout == b->pending_fd_timeout || a->max_completed_connections == b->max_completed_connections || a->max_incomplete_connections == b->max_incomplete_connections || a->max_connections_per_user == b->max_connections_per_user diff --git a/bus/session.conf.in b/bus/session.conf.in index d4730363..cfe9544f 100644 --- a/bus/session.conf.in +++ b/bus/session.conf.in @@ -53,6 +53,7 @@ limit is also relatively low --> 120000 240000 + 150000 100000 10000 100000 diff --git a/doc/dbus-daemon.1.xml.in b/doc/dbus-daemon.1.xml.in index 7b7f4a1b..cd7942c3 100644 --- a/doc/dbus-daemon.1.xml.in +++ b/doc/dbus-daemon.1.xml.in @@ -528,6 +528,10 @@ Available limit names are: "auth_timeout" : milliseconds (thousandths) a connection is given to authenticate + "pending_fd_timeout" : milliseconds (thousandths) a + fd is given to be transmitted to + dbus-daemon before disconnecting the + connection "max_completed_connections" : max number of authenticated connections "max_incomplete_connections" : max number of unauthenticated connections -- cgit v1.2.3 From 995734750cea65012537748ee56488c707d2f027 Mon Sep 17 00:00:00 2001 From: Alban Crequy Date: Fri, 18 Jul 2014 16:45:07 +0100 Subject: DBusConnection: implements _dbus_connection_get_pending_fds_count This is one of four commits needed to address CVE-2014-3637. This will allow the bus to know whether there are pending file descriptors in a DBusConnection's DBusMessageLoader. https://bugs.freedesktop.org/show_bug.cgi?id=80559 Reviewed-by: Simon McVittie [fix compilation on platforms that do not HAVE_UNIX_FD_PASSING -smcv] Signed-off-by: Simon McVittie --- dbus/dbus-connection-internal.h | 1 + dbus/dbus-connection.c | 11 +++++++++++ dbus/dbus-message-internal.h | 1 + dbus/dbus-message.c | 15 +++++++++++++++ dbus/dbus-transport.c | 11 +++++++++++ dbus/dbus-transport.h | 1 + 6 files changed, 40 insertions(+) diff --git a/dbus/dbus-connection-internal.h b/dbus/dbus-connection-internal.h index 2842f2f4..24e47725 100644 --- a/dbus/dbus-connection-internal.h +++ b/dbus/dbus-connection-internal.h @@ -100,6 +100,7 @@ void _dbus_connection_test_get_locks (DBusConnectio DBusMutex **io_path_mutex_loc, DBusCondVar **dispatch_cond_loc, DBusCondVar **io_path_cond_loc); +int _dbus_connection_get_pending_fds_count (DBusConnection *connection); /* if DBUS_ENABLE_STATS */ void _dbus_connection_get_stats (DBusConnection *connection, diff --git a/dbus/dbus-connection.c b/dbus/dbus-connection.c index f0b6871e..6aa24bc4 100644 --- a/dbus/dbus-connection.c +++ b/dbus/dbus-connection.c @@ -2549,6 +2549,17 @@ _dbus_connection_block_pending_call (DBusPendingCall *pending) dbus_pending_call_unref (pending); } +/** + * Return how many file descriptors are pending in the loader + * + * @param connection the connection + */ +int +_dbus_connection_get_pending_fds_count (DBusConnection *connection) +{ + return _dbus_transport_get_pending_fds_count (connection->transport); +} + /** @} */ /** diff --git a/dbus/dbus-message-internal.h b/dbus/dbus-message-internal.h index 5d6594e3..45cb026e 100644 --- a/dbus/dbus-message-internal.h +++ b/dbus/dbus-message-internal.h @@ -96,6 +96,7 @@ long _dbus_message_loader_get_max_message_size (DBusMessageLoader void _dbus_message_loader_set_max_message_unix_fds(DBusMessageLoader *loader, long n); long _dbus_message_loader_get_max_message_unix_fds(DBusMessageLoader *loader); +int _dbus_message_loader_get_pending_fds_count (DBusMessageLoader *loader); typedef struct DBusInitialFDs DBusInitialFDs; DBusInitialFDs *_dbus_check_fdleaks_enter (void); diff --git a/dbus/dbus-message.c b/dbus/dbus-message.c index f4787b06..59f3bdc8 100644 --- a/dbus/dbus-message.c +++ b/dbus/dbus-message.c @@ -4499,6 +4499,21 @@ _dbus_message_loader_get_max_message_unix_fds (DBusMessageLoader *loader) return loader->max_message_unix_fds; } +/** + * Return how many file descriptors are pending in the loader + * + * @param loader the loader + */ +int +_dbus_message_loader_get_pending_fds_count (DBusMessageLoader *loader) +{ +#ifdef HAVE_UNIX_FD_PASSING + return loader->n_unix_fds; +#else + return 0; +#endif +} + static DBusDataSlotAllocator slot_allocator = _DBUS_DATA_SLOT_ALLOCATOR_INIT (_DBUS_LOCK_NAME (message_slots)); diff --git a/dbus/dbus-transport.c b/dbus/dbus-transport.c index ecc31827..690e5ba8 100644 --- a/dbus/dbus-transport.c +++ b/dbus/dbus-transport.c @@ -1512,6 +1512,17 @@ _dbus_transport_set_allow_anonymous (DBusTransport *transport, transport->allow_anonymous = value != FALSE; } +/** + * Return how many file descriptors are pending in the loader + * + * @param transport the transport + */ +int +_dbus_transport_get_pending_fds_count (DBusTransport *transport) +{ + return _dbus_message_loader_get_pending_fds_count (transport->loader); +} + #ifdef DBUS_ENABLE_STATS void _dbus_transport_get_stats (DBusTransport *transport, diff --git a/dbus/dbus-transport.h b/dbus/dbus-transport.h index 80fa24ef..ff102f3a 100644 --- a/dbus/dbus-transport.h +++ b/dbus/dbus-transport.h @@ -97,6 +97,7 @@ dbus_bool_t _dbus_transport_set_auth_mechanisms (DBusTransport const char **mechanisms); void _dbus_transport_set_allow_anonymous (DBusTransport *transport, dbus_bool_t value); +int _dbus_transport_get_pending_fds_count (DBusTransport *transport); /* if DBUS_ENABLE_STATS */ void _dbus_transport_get_stats (DBusTransport *transport, -- cgit v1.2.3 From 8021fd84267ee1394d96f4a119adb57de3971a62 Mon Sep 17 00:00:00 2001 From: Alban Crequy Date: Fri, 18 Jul 2014 17:28:32 +0100 Subject: DBusConnection: implements _dbus_connection_set_pending_fds_function This is one of four commits needed to address CVE-2014-3637. This will allow the bus to be notified whenever a file descriptor is added or removed from a DBusConnection's DBusMessageLoader. Bug: https://bugs.freedesktop.org/show_bug.cgi?id=80559 Reviewed-by: Simon McVittie --- dbus/dbus-connection-internal.h | 5 +++++ dbus/dbus-connection.c | 16 ++++++++++++++++ dbus/dbus-message-internal.h | 3 +++ dbus/dbus-message-private.h | 2 ++ dbus/dbus-message.c | 25 +++++++++++++++++++++++++ dbus/dbus-transport.c | 16 ++++++++++++++++ dbus/dbus-transport.h | 3 +++ 7 files changed, 70 insertions(+) diff --git a/dbus/dbus-connection-internal.h b/dbus/dbus-connection-internal.h index 24e47725..28974040 100644 --- a/dbus/dbus-connection-internal.h +++ b/dbus/dbus-connection-internal.h @@ -44,6 +44,8 @@ typedef enum /** default timeout value when waiting for a message reply, 25 seconds */ #define _DBUS_DEFAULT_TIMEOUT_VALUE (25 * 1000) +typedef void (* DBusPendingFdsChangeFunction) (void *data); + void _dbus_connection_lock (DBusConnection *connection); void _dbus_connection_unlock (DBusConnection *connection); DBusConnection * _dbus_connection_ref_unlocked (DBusConnection *connection); @@ -101,6 +103,9 @@ void _dbus_connection_test_get_locks (DBusConnectio DBusCondVar **dispatch_cond_loc, DBusCondVar **io_path_cond_loc); int _dbus_connection_get_pending_fds_count (DBusConnection *connection); +void _dbus_connection_set_pending_fds_function (DBusConnection *connection, + DBusPendingFdsChangeFunction callback, + void *data); /* if DBUS_ENABLE_STATS */ void _dbus_connection_get_stats (DBusConnection *connection, diff --git a/dbus/dbus-connection.c b/dbus/dbus-connection.c index 6aa24bc4..b574207d 100644 --- a/dbus/dbus-connection.c +++ b/dbus/dbus-connection.c @@ -2560,6 +2560,22 @@ _dbus_connection_get_pending_fds_count (DBusConnection *connection) return _dbus_transport_get_pending_fds_count (connection->transport); } +/** + * Register a function to be called whenever the number of pending file + * descriptors in the loader change. + * + * @param connection the connection + * @param callback the callback + */ +void +_dbus_connection_set_pending_fds_function (DBusConnection *connection, + DBusPendingFdsChangeFunction callback, + void *data) +{ + _dbus_transport_set_pending_fds_function (connection->transport, + callback, data); +} + /** @} */ /** diff --git a/dbus/dbus-message-internal.h b/dbus/dbus-message-internal.h index 45cb026e..e9a9ec01 100644 --- a/dbus/dbus-message-internal.h +++ b/dbus/dbus-message-internal.h @@ -97,6 +97,9 @@ void _dbus_message_loader_set_max_message_unix_fds(DBusMessageLoad long n); long _dbus_message_loader_get_max_message_unix_fds(DBusMessageLoader *loader); int _dbus_message_loader_get_pending_fds_count (DBusMessageLoader *loader); +void _dbus_message_loader_set_pending_fds_function (DBusMessageLoader *loader, + void (* callback) (void *), + void *data); typedef struct DBusInitialFDs DBusInitialFDs; DBusInitialFDs *_dbus_check_fdleaks_enter (void); diff --git a/dbus/dbus-message-private.h b/dbus/dbus-message-private.h index e1578abd..a611b095 100644 --- a/dbus/dbus-message-private.h +++ b/dbus/dbus-message-private.h @@ -80,6 +80,8 @@ struct DBusMessageLoader int *unix_fds; /**< File descriptors that have been read from the transport but not yet been handed to any message. Array will be allocated at first use. */ unsigned n_unix_fds_allocated; /**< Number of file descriptors this array has space for */ unsigned n_unix_fds; /**< Number of valid file descriptors in array */ + void (* unix_fds_change) (void *); /**< Notify when the pending fds change */ + void *unix_fds_change_data; #endif }; diff --git a/dbus/dbus-message.c b/dbus/dbus-message.c index 59f3bdc8..3e74fc54 100644 --- a/dbus/dbus-message.c +++ b/dbus/dbus-message.c @@ -4059,6 +4059,9 @@ _dbus_message_loader_return_unix_fds(DBusMessageLoader *loader, loader->n_unix_fds += n_fds; loader->unix_fds_outstanding = FALSE; + + if (n_fds && loader->unix_fds_change) + loader->unix_fds_change (loader->unix_fds_change_data); #else _dbus_assert_not_reached("Platform doesn't support unix fd passing"); #endif @@ -4206,6 +4209,9 @@ load_message (DBusMessageLoader *loader, message->n_unix_fds_allocated = message->n_unix_fds = n_unix_fds; loader->n_unix_fds -= n_unix_fds; memmove (loader->unix_fds, loader->unix_fds + n_unix_fds, loader->n_unix_fds * sizeof (loader->unix_fds[0])); + + if (loader->unix_fds_change) + loader->unix_fds_change (loader->unix_fds_change_data); } else message->unix_fds = NULL; @@ -4514,6 +4520,25 @@ _dbus_message_loader_get_pending_fds_count (DBusMessageLoader *loader) #endif } +/** + * Register a function to be called whenever the number of pending file + * descriptors in the loader change. + * + * @param loader the loader + * @param callback the callback + * @param data the data for the callback + */ +void +_dbus_message_loader_set_pending_fds_function (DBusMessageLoader *loader, + void (* callback) (void *), + void *data) +{ +#ifdef HAVE_UNIX_FD_PASSING + loader->unix_fds_change = callback; + loader->unix_fds_change_data = data; +#endif +} + static DBusDataSlotAllocator slot_allocator = _DBUS_DATA_SLOT_ALLOCATOR_INIT (_DBUS_LOCK_NAME (message_slots)); diff --git a/dbus/dbus-transport.c b/dbus/dbus-transport.c index 690e5ba8..f63e0ced 100644 --- a/dbus/dbus-transport.c +++ b/dbus/dbus-transport.c @@ -1523,6 +1523,22 @@ _dbus_transport_get_pending_fds_count (DBusTransport *transport) return _dbus_message_loader_get_pending_fds_count (transport->loader); } +/** + * Register a function to be called whenever the number of pending file + * descriptors in the loader change. + * + * @param transport the transport + * @param callback the callback + */ +void +_dbus_transport_set_pending_fds_function (DBusTransport *transport, + void (* callback) (void *), + void *data) +{ + _dbus_message_loader_set_pending_fds_function (transport->loader, + callback, data); +} + #ifdef DBUS_ENABLE_STATS void _dbus_transport_get_stats (DBusTransport *transport, diff --git a/dbus/dbus-transport.h b/dbus/dbus-transport.h index ff102f3a..39c74c46 100644 --- a/dbus/dbus-transport.h +++ b/dbus/dbus-transport.h @@ -98,6 +98,9 @@ dbus_bool_t _dbus_transport_set_auth_mechanisms (DBusTransport void _dbus_transport_set_allow_anonymous (DBusTransport *transport, dbus_bool_t value); int _dbus_transport_get_pending_fds_count (DBusTransport *transport); +void _dbus_transport_set_pending_fds_function (DBusTransport *transport, + void (* callback) (void *), + void *data); /* if DBUS_ENABLE_STATS */ void _dbus_transport_get_stats (DBusTransport *transport, -- cgit v1.2.3 From e0c9d31be3b9eea9ee2a3a255bc2cf9aad713642 Mon Sep 17 00:00:00 2001 From: Alban Crequy Date: Mon, 21 Jul 2014 17:17:11 +0100 Subject: bus: enforce pending_fd_timeout This is one of four commits needed to address CVE-2014-3637. The bus uses _dbus_connection_set_pending_fds_function and _dbus_connection_get_pending_fds_count to be notified when there are pending file descriptors. A timeout per connection is armed and disarmed when the file descriptor list is used and emptied. Bug: https://bugs.freedesktop.org/show_bug.cgi?id=80559 Reviewed-by: Simon McVittie --- bus/connection.c | 71 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 71 insertions(+) diff --git a/bus/connection.c b/bus/connection.c index 54fa3ab7..519122c5 100644 --- a/bus/connection.c +++ b/bus/connection.c @@ -33,6 +33,7 @@ #include #include #include +#include /* Trim executed commands to this length; we want to keep logs readable */ #define MAX_LOG_COMMAND_LEN 50 @@ -102,6 +103,8 @@ typedef struct int peak_match_rules; int peak_bus_names; #endif + int n_pending_unix_fds; + DBusTimeout *pending_unix_fds_timeout; } BusConnectionData; static dbus_bool_t bus_pending_reply_expired (BusExpireList *list, @@ -268,6 +271,15 @@ bus_connection_disconnected (DBusConnection *connection) dbus_connection_set_dispatch_status_function (connection, NULL, NULL, NULL); + + if (d->pending_unix_fds_timeout) + { + _dbus_loop_remove_timeout (bus_context_get_loop (d->connections->context), + d->pending_unix_fds_timeout); + _dbus_timeout_unref (d->pending_unix_fds_timeout); + } + d->pending_unix_fds_timeout = NULL; + _dbus_connection_set_pending_fds_function (connection, NULL, NULL); bus_connection_remove_transactions (connection); @@ -592,6 +604,42 @@ oom: return FALSE; } +static void +check_pending_fds_cb (DBusConnection *connection) +{ + BusConnectionData *d = BUS_CONNECTION_DATA (connection); + int n_pending_unix_fds_old = d->n_pending_unix_fds; + int n_pending_unix_fds_new; + + n_pending_unix_fds_new = _dbus_connection_get_pending_fds_count (connection); + + _dbus_verbose ("Pending fds count changed on connection %p: %d -> %d\n", + connection, n_pending_unix_fds_old, n_pending_unix_fds_new); + + if (n_pending_unix_fds_old == 0 && n_pending_unix_fds_new > 0) + { + _dbus_timeout_set_interval (d->pending_unix_fds_timeout, + bus_context_get_pending_fd_timeout (d->connections->context)); + _dbus_timeout_set_enabled (d->pending_unix_fds_timeout, TRUE); + } + + if (n_pending_unix_fds_old > 0 && n_pending_unix_fds_new == 0) + { + _dbus_timeout_set_enabled (d->pending_unix_fds_timeout, FALSE); + } + + + d->n_pending_unix_fds = n_pending_unix_fds_new; +} + +static dbus_bool_t +pending_unix_fds_timeout_cb (void *data) +{ + DBusConnection *connection = data; + dbus_connection_close (connection); + return TRUE; +} + dbus_bool_t bus_connections_setup_connection (BusConnections *connections, DBusConnection *connection) @@ -687,6 +735,22 @@ bus_connections_setup_connection (BusConnections *connections, } } + /* Setup pending fds timeout (see #80559) */ + d->pending_unix_fds_timeout = _dbus_timeout_new (100, /* irrelevant */ + pending_unix_fds_timeout_cb, + connection, NULL); + if (d->pending_unix_fds_timeout == NULL) + goto out; + + _dbus_timeout_set_enabled (d->pending_unix_fds_timeout, FALSE); + if (!_dbus_loop_add_timeout (bus_context_get_loop (connections->context), + d->pending_unix_fds_timeout)) + goto out; + + _dbus_connection_set_pending_fds_function (connection, + (DBusPendingFdsChangeFunction) check_pending_fds_cb, + connection); + _dbus_list_append_link (&connections->incomplete, d->link_in_connection_list); connections->n_incomplete += 1; @@ -734,6 +798,13 @@ bus_connections_setup_connection (BusConnections *connections, dbus_connection_set_dispatch_status_function (connection, NULL, NULL, NULL); + if (d->pending_unix_fds_timeout) + _dbus_timeout_unref (d->pending_unix_fds_timeout); + + d->pending_unix_fds_timeout = NULL; + + _dbus_connection_set_pending_fds_function (connection, NULL, NULL); + if (d->link_in_connection_list != NULL) { _dbus_assert (d->link_in_connection_list->next == NULL); -- cgit v1.2.3 From f70c0e98c5cc6eaae4727d14c389e2504e79e694 Mon Sep 17 00:00:00 2001 From: Simon McVittie Date: Tue, 9 Sep 2014 12:49:44 +0100 Subject: Add _DBUS_GNUC_UNUSED, and use it in _DBUS_STATIC_ASSERT This means we can use _DBUS_STATIC_ASSERT at non-global scope without tripping -Wunused-local-typedefs. Bug: https://bugs.freedesktop.org/show_bug.cgi?id=83767 Reviewed-by: Alban Crequy (cherry picked from commit 0e3d08d45cb9a9ceb2c077875eeb38306dad37b8) --- dbus/dbus-internals.h | 2 +- dbus/dbus-macros.h | 3 +++ 2 files changed, 4 insertions(+), 1 deletion(-) diff --git a/dbus/dbus-internals.h b/dbus/dbus-internals.h index c64d7566..4658b67b 100644 --- a/dbus/dbus-internals.h +++ b/dbus/dbus-internals.h @@ -371,7 +371,7 @@ dbus_bool_t _dbus_get_local_machine_uuid_encoded (DBusString *uuid_str); #define _DBUS_PASTE(a, b) _DBUS_PASTE2 (a, b) #define _DBUS_STATIC_ASSERT(expr) \ typedef struct { char _assertion[(expr) ? 1 : -1]; } \ - _DBUS_PASTE (_DBUS_STATIC_ASSERT_, __LINE__) + _DBUS_PASTE (_DBUS_STATIC_ASSERT_, __LINE__) _DBUS_GNUC_UNUSED DBUS_END_DECLS diff --git a/dbus/dbus-macros.h b/dbus/dbus-macros.h index cae4100e..8d6c3000 100644 --- a/dbus/dbus-macros.h +++ b/dbus/dbus-macros.h @@ -69,9 +69,12 @@ __attribute__((__format__ (__printf__, format_idx, arg_idx))) #define _DBUS_GNUC_NORETURN \ __attribute__((__noreturn__)) +#define _DBUS_GNUC_UNUSED \ + __attribute__((__unused__)) #else /* !__GNUC__ */ #define _DBUS_GNUC_PRINTF( format_idx, arg_idx ) #define _DBUS_GNUC_NORETURN +#define _DBUS_GNUC_UNUSED #endif /* !__GNUC__ */ #if __GNUC__ > 2 || (__GNUC__ == 2 && __GNUC_MINOR__ >= 96) -- cgit v1.2.3 From ee11ec12566afda5dee8a3a834274421a20661de Mon Sep 17 00:00:00 2001 From: Simon McVittie Date: Tue, 9 Sep 2014 12:44:22 +0100 Subject: _dbus_read_socket_with_unix_fds: do not accept extra fds in cmsg padding This addresses CVE-2014-3635. If (*n_fds * sizeof (int) % sizeof (size_t)) is nonzero, then CMSG_SPACE (*n_fds * sizeof (int)) > CMSG_LEN (*n_fds * sizeof (int) because the SPACE includes padding to a size_t boundary, whereas the LEN does not. We have to allocate the SPACE. Previously, we told the kernel that the buffer size we wanted was the SPACE, not the LEN, which meant it was free to fill the padding with additional fds: on a 64-bit platform with 32-bit int, that's one extra fd, if *n_fds happens to be odd. This meant that a malicious sender could send exactly 1 fd too many, which would make us fail an assertion if enabled, or overrun a buffer by 1 fd otherwise. Bug: https://bugs.freedesktop.org/show_bug.cgi?id=83622 Reviewed-by: Alban Crequy --- dbus/dbus-sysdeps-unix.c | 49 ++++++++++++++++++++++++++++++++++++++++++------ 1 file changed, 43 insertions(+), 6 deletions(-) diff --git a/dbus/dbus-sysdeps-unix.c b/dbus/dbus-sysdeps-unix.c index e81e52c3..fe891ab7 100644 --- a/dbus/dbus-sysdeps-unix.c +++ b/dbus/dbus-sysdeps-unix.c @@ -320,6 +320,12 @@ _dbus_read_socket_with_unix_fds (int fd, m.msg_control = alloca(m.msg_controllen); memset(m.msg_control, 0, m.msg_controllen); + /* Do not include the padding at the end when we tell the kernel + * how much we're willing to receive. This avoids getting + * the padding filled with additional fds that we weren't expecting, + * if a (potentially malicious) sender included them. (fd.o #83622) */ + m.msg_controllen = CMSG_LEN (*n_fds * sizeof(int)); + again: bytes_read = recvmsg(fd, &m, 0 @@ -359,18 +365,49 @@ _dbus_read_socket_with_unix_fds (int fd, for (cm = CMSG_FIRSTHDR(&m); cm; cm = CMSG_NXTHDR(&m, cm)) if (cm->cmsg_level == SOL_SOCKET && cm->cmsg_type == SCM_RIGHTS) { - unsigned i; - - _dbus_assert(cm->cmsg_len <= CMSG_LEN(*n_fds * sizeof(int))); - *n_fds = (cm->cmsg_len - CMSG_LEN(0)) / sizeof(int); + size_t i; + int *payload = (int *) CMSG_DATA (cm); + size_t payload_len_bytes = (cm->cmsg_len - CMSG_LEN (0)); + size_t payload_len_fds = payload_len_bytes / sizeof (int); + size_t fds_to_use; + + /* Every non-negative int fits in a size_t without truncation, + * and we already know that *n_fds is non-negative, so + * casting (size_t) *n_fds is OK */ + _DBUS_STATIC_ASSERT (sizeof (size_t) >= sizeof (int)); + + if (_DBUS_LIKELY (payload_len_fds <= (size_t) *n_fds)) + { + /* The fds in the payload will fit in our buffer */ + fds_to_use = payload_len_fds; + } + else + { + /* Too many fds in the payload. This shouldn't happen + * any more because we're setting m.msg_controllen to + * the exact number we can accept, but be safe and + * truncate. */ + fds_to_use = (size_t) *n_fds; + + /* Close the excess fds to avoid DoS: if they stayed open, + * someone could send us an extra fd per message + * and we'd eventually run out. */ + for (i = fds_to_use; i < payload_len_fds; i++) + { + close (payload[i]); + } + } - memcpy(fds, CMSG_DATA(cm), *n_fds * sizeof(int)); + memcpy (fds, payload, fds_to_use * sizeof (int)); found = TRUE; + /* This cannot overflow because we have chosen fds_to_use + * to be <= *n_fds */ + *n_fds = (int) fds_to_use; /* Linux doesn't tell us whether MSG_CMSG_CLOEXEC actually worked, hence we need to go through this list and set CLOEXEC everywhere in any case */ - for (i = 0; i < *n_fds; i++) + for (i = 0; i < fds_to_use; i++) _dbus_fd_set_close_on_exec(fds[i]); break; -- cgit v1.2.3 From 28cba657857141962278eef619cbd1d629c18208 Mon Sep 17 00:00:00 2001 From: Simon McVittie Date: Mon, 15 Sep 2014 12:43:04 +0100 Subject: Prepare 1.8.8 (embargoed until tomorrow) --- NEWS | 42 ++++++++++++++++++++++++++++++++++++++++-- configure.ac | 4 ++-- 2 files changed, 42 insertions(+), 4 deletions(-) diff --git a/NEWS b/NEWS index bfab7cbc..b10ab4f7 100644 --- a/NEWS +++ b/NEWS @@ -1,7 +1,45 @@ -D-Bus 1.8.8 (UNRELEASED) +D-Bus 1.8.8 (2014-09-16) == -Fixes: +The "smashy smashy egg man" release. + +Security fixes: + +• Do not accept an extra fd in the padding of a cmsg message, which + could lead to a 4-byte heap buffer overrun. + (CVE-2014-3635, fd.o #83622; Simon McVittie) + +• Reduce default for maximum Unix file descriptors passed per message + from 1024 to 16, preventing a uid with the default maximum number of + connections from exhausting the system bus' file descriptors under + Linux's default rlimit. Distributors or system administrators with a + more restrictive fd limit may wish to reduce these limits further. + + Additionally, on Linux this prevents a second denial of service + in which the dbus-daemon can be made to exceed the maximum number + of fds per sendmsg() and disconnect the process that would have + received them. + (CVE-2014-3636, fd.o #82820; Alban Crequy) + +• Disconnect connections that still have a fd pending unmarshalling after + a new configurable limit, pending_fd_timeout (defaulting to 150 seconds), + removing the possibility of creating an abusive connection that cannot be + disconnected by setting up a circular reference to a connection's + file descriptor. + (CVE-2014-3637, fd.o #80559; Alban Crequy) + +• Reduce default for maximum pending replies per connection from 8192 to 128, + mitigating an algorithmic complexity denial-of-service attack + (CVE-2014-3638, fd.o #81053; Alban Crequy) + +• Reduce default for authentication timeout on the system bus from + 30 seconds to 5 seconds, avoiding denial of service by using up + all unauthenticated connection slots; and when all unauthenticated + connection slots are used up, make new connection attempts block + instead of disconnecting them. + (CVE-2014-3639, fd.o #80919; Alban Crequy) + +Other fixes: • Check for libsystemd from systemd >= 209, falling back to the older separate libraries if not found (Umut Tezduyar Lindskog, diff --git a/configure.ac b/configure.ac index 8a530b29..2d4624c7 100644 --- a/configure.ac +++ b/configure.ac @@ -3,7 +3,7 @@ AC_PREREQ([2.63]) m4_define([dbus_major_version], [1]) m4_define([dbus_minor_version], [8]) -m4_define([dbus_micro_version], [7]) +m4_define([dbus_micro_version], [8]) m4_define([dbus_version], [dbus_major_version.dbus_minor_version.dbus_micro_version]) AC_INIT([dbus],[dbus_version],[https://bugs.freedesktop.org/enter_bug.cgi?product=dbus],[dbus]) @@ -37,7 +37,7 @@ LT_CURRENT=11 ## increment any time the source changes; set to ## 0 if you increment CURRENT -LT_REVISION=6 +LT_REVISION=7 ## increment if any interfaces have been added; set to 0 ## if any interfaces have been changed or removed. removal has -- cgit v1.2.3 From 8874d3a0c57c0cae97cbe426e3686936da53f649 Mon Sep 17 00:00:00 2001 From: Simon McVittie Date: Tue, 16 Sep 2014 17:47:46 +0100 Subject: 1.8.9 --- NEWS | 5 +++++ configure.ac | 2 +- 2 files changed, 6 insertions(+), 1 deletion(-) diff --git a/NEWS b/NEWS index b10ab4f7..91f7e7fa 100644 --- a/NEWS +++ b/NEWS @@ -1,3 +1,8 @@ +D-Bus 1.8.10 (UNRELEASED) +== + +... + D-Bus 1.8.8 (2014-09-16) == diff --git a/configure.ac b/configure.ac index 2d4624c7..287dbbbe 100644 --- a/configure.ac +++ b/configure.ac @@ -3,7 +3,7 @@ AC_PREREQ([2.63]) m4_define([dbus_major_version], [1]) m4_define([dbus_minor_version], [8]) -m4_define([dbus_micro_version], [8]) +m4_define([dbus_micro_version], [9]) m4_define([dbus_version], [dbus_major_version.dbus_minor_version.dbus_micro_version]) AC_INIT([dbus],[dbus_version],[https://bugs.freedesktop.org/enter_bug.cgi?product=dbus],[dbus]) -- cgit v1.2.3 From 4e466446d27f1a3991c22307a47a81c9e93e530d Mon Sep 17 00:00:00 2001 From: Simon McVittie Date: Tue, 4 Nov 2014 14:41:54 +0000 Subject: CVE-2014-7824: set fd rlimit to 64k for the system dbus-daemon This ensures that our rlimit is actually high enough to avoid the denial of service described in CVE-2014-3636 part A. CVE-2014-7824 has been allocated for this incomplete fix. Restore the original rlimit for activated services, to avoid them getting undesired higher limits. (Thanks to Alban Crequy for various adjustments which have been included in this commit.) Bug: https://bugs.freedesktop.org/show_bug.cgi?id=85105 Reviewed-by: Alban Crequy --- bus/activation.c | 28 +++++++- bus/bus.c | 50 ++++++++++++--- bus/bus.h | 1 + dbus/dbus-sysdeps-util-unix.c | 145 +++++++++++++++++++++++++++++++++--------- dbus/dbus-sysdeps-util-win.c | 35 +++++++++- dbus/dbus-sysdeps.h | 11 +++- 6 files changed, 227 insertions(+), 43 deletions(-) diff --git a/bus/activation.c b/bus/activation.c index 149cca8a..ecd19bb4 100644 --- a/bus/activation.c +++ b/bus/activation.c @@ -1688,6 +1688,31 @@ out: return retval; } +static void +child_setup (void *user_data) +{ +#ifdef DBUS_UNIX + BusActivation *activation = user_data; + DBusRLimit *initial_fd_limit; + DBusError error; + + dbus_error_init (&error); + initial_fd_limit = bus_context_get_initial_fd_limit (activation->context); + + if (initial_fd_limit != NULL && + !_dbus_rlimit_restore_fd_limit (initial_fd_limit, &error)) + { + /* unfortunately we don't actually know the service name here */ + bus_context_log (activation->context, + DBUS_SYSTEM_LOG_INFO, + "Failed to reset fd limit before activating " + "service: %s: %s", + error.name, error.message); + } +#endif +} + + dbus_bool_t bus_activation_activate_service (BusActivation *activation, DBusConnection *connection, @@ -2121,7 +2146,8 @@ bus_activation_activate_service (BusActivation *activation, service_name, argv, envp, - NULL, activation, + child_setup, + activation, &tmp_error)) { _dbus_verbose ("Failed to spawn child\n"); diff --git a/bus/bus.c b/bus/bus.c index 35d40754..47cc3452 100644 --- a/bus/bus.c +++ b/bus/bus.c @@ -64,6 +64,7 @@ struct BusContext BusPolicy *policy; BusMatchmaker *matchmaker; BusLimits limits; + DBusRLimit *initial_fd_limit; unsigned int fork : 1; unsigned int syslog : 1; unsigned int keep_umask : 1; @@ -659,19 +660,38 @@ oom: static void raise_file_descriptor_limit (BusContext *context) { +#ifdef DBUS_UNIX + DBusError error = DBUS_ERROR_INIT; - /* I just picked this out of thin air; we need some extra - * descriptors for things like any internal pipes we create, - * inotify, connections to SELinux, etc. - */ - unsigned int arbitrary_extra_fds = 32; - unsigned int limit; + /* we only do this once */ + if (context->initial_fd_limit != NULL) + return; - limit = context->limits.max_completed_connections + - context->limits.max_incomplete_connections - + arbitrary_extra_fds; + context->initial_fd_limit = _dbus_rlimit_save_fd_limit (&error); + + if (context->initial_fd_limit == NULL) + { + bus_context_log (context, DBUS_SYSTEM_LOG_INFO, + "%s: %s", error.name, error.message); + dbus_error_free (&error); + return; + } - _dbus_request_file_descriptor_limit (limit); + /* We used to compute a suitable rlimit based on the configured number + * of connections, but that breaks down as soon as we allow fd-passing, + * because each connection is allowed to pass 64 fds to us, and if + * they all did, we'd hit kernel limits. We now hard-code 64k as a + * good limit, like systemd does: that's enough to avoid DoS from + * anything short of multiple uids conspiring against us. + */ + if (!_dbus_rlimit_raise_fd_limit_if_privileged (65536, &error)) + { + bus_context_log (context, DBUS_SYSTEM_LOG_INFO, + "%s: %s", error.name, error.message); + dbus_error_free (&error); + return; + } +#endif } static dbus_bool_t @@ -1130,6 +1150,10 @@ bus_context_unref (BusContext *context) dbus_free (context->pidfile); } + + if (context->initial_fd_limit) + _dbus_rlimit_free (context->initial_fd_limit); + dbus_free (context); dbus_server_free_data_slot (&server_data_slot); @@ -1294,6 +1318,12 @@ bus_context_get_reply_timeout (BusContext *context) return context->limits.reply_timeout; } +DBusRLimit * +bus_context_get_initial_fd_limit (BusContext *context) +{ + return context->initial_fd_limit; +} + void bus_context_log (BusContext *context, DBusSystemLogSeverity severity, const char *msg, ...) _DBUS_GNUC_PRINTF (3, 4); diff --git a/bus/bus.h b/bus/bus.h index 7d0b3697..dac6ea5e 100644 --- a/bus/bus.h +++ b/bus/bus.h @@ -116,6 +116,7 @@ int bus_context_get_max_services_per_connection (BusContext int bus_context_get_max_match_rules_per_connection (BusContext *context); int bus_context_get_max_replies_per_connection (BusContext *context); int bus_context_get_reply_timeout (BusContext *context); +DBusRLimit * bus_context_get_initial_fd_limit (BusContext *context); void bus_context_log (BusContext *context, DBusSystemLogSeverity severity, const char *msg, diff --git a/dbus/dbus-sysdeps-util-unix.c b/dbus/dbus-sysdeps-util-unix.c index 0d8a66c7..199eb3dd 100644 --- a/dbus/dbus-sysdeps-util-unix.c +++ b/dbus/dbus-sysdeps-util-unix.c @@ -378,53 +378,140 @@ _dbus_change_to_daemon_user (const char *user, } #endif /* !HAVE_LIBAUDIT */ +#ifdef HAVE_SETRLIMIT -/** - * Attempt to ensure that the current process can open - * at least @p limit file descriptors. - * - * If @p limit is lower than the current, it will not be - * lowered. No error is returned if the request can - * not be satisfied. - * - * @param limit number of file descriptors +/* We assume that if we have setrlimit, we also have getrlimit and + * struct rlimit. */ -void -_dbus_request_file_descriptor_limit (unsigned int limit) + +struct DBusRLimit { + struct rlimit lim; +}; + +DBusRLimit * +_dbus_rlimit_save_fd_limit (DBusError *error) +{ + DBusRLimit *self; + + self = dbus_new0 (DBusRLimit, 1); + + if (self == NULL) + { + _DBUS_SET_OOM (error); + return NULL; + } + + if (getrlimit (RLIMIT_NOFILE, &self->lim) < 0) + { + dbus_set_error (error, _dbus_error_from_errno (errno), + "Failed to get fd limit: %s", _dbus_strerror (errno)); + dbus_free (self); + return NULL; + } + + return self; +} + +dbus_bool_t +_dbus_rlimit_raise_fd_limit_if_privileged (unsigned int desired, + DBusError *error) { -#ifdef HAVE_SETRLIMIT struct rlimit lim; - struct rlimit target_lim; /* No point to doing this practically speaking * if we're not uid 0. We expect the system * bus to use this before we change UID, and - * the session bus takes the Linux default - * of 1024 for both cur and max. + * the session bus takes the Linux default, + * currently 1024 for cur and 4096 for max. */ if (getuid () != 0) - return; + { + /* not an error, we're probably the session bus */ + return TRUE; + } if (getrlimit (RLIMIT_NOFILE, &lim) < 0) - return; + { + dbus_set_error (error, _dbus_error_from_errno (errno), + "Failed to get fd limit: %s", _dbus_strerror (errno)); + return FALSE; + } - if (lim.rlim_cur >= limit) - return; + if (lim.rlim_cur == RLIM_INFINITY || lim.rlim_cur >= desired) + { + /* not an error, everything is fine */ + return TRUE; + } /* Ignore "maximum limit", assume we have the "superuser" * privileges. On Linux this is CAP_SYS_RESOURCE. */ - target_lim.rlim_cur = target_lim.rlim_max = limit; - /* Also ignore errors; if we fail, we will at least work - * up to whatever limit we had, which seems better than - * just outright aborting. - * - * However, in the future we should probably log this so OS builders - * have a chance to notice any misconfiguration like dbus-daemon - * being started without CAP_SYS_RESOURCE. - */ - setrlimit (RLIMIT_NOFILE, &target_lim); + lim.rlim_cur = lim.rlim_max = desired; + + if (setrlimit (RLIMIT_NOFILE, &lim) < 0) + { + dbus_set_error (error, _dbus_error_from_errno (errno), + "Failed to set fd limit to %u: %s", + desired, _dbus_strerror (errno)); + return FALSE; + } + + return TRUE; +} + +dbus_bool_t +_dbus_rlimit_restore_fd_limit (DBusRLimit *saved, + DBusError *error) +{ + if (setrlimit (RLIMIT_NOFILE, &saved->lim) < 0) + { + dbus_set_error (error, _dbus_error_from_errno (errno), + "Failed to restore old fd limit: %s", + _dbus_strerror (errno)); + return FALSE; + } + + return TRUE; +} + +#else /* !HAVE_SETRLIMIT */ + +static void +fd_limit_not_supported (DBusError *error) +{ + dbus_set_error (error, DBUS_ERROR_NOT_SUPPORTED, + "cannot change fd limit on this platform"); +} + +DBusRLimit * +_dbus_rlimit_save_fd_limit (DBusError *error) +{ + fd_limit_not_supported (error); + return NULL; +} + +dbus_bool_t +_dbus_rlimit_raise_fd_limit_if_privileged (unsigned int desired, + DBusError *error) +{ + fd_limit_not_supported (error); + return FALSE; +} + +dbus_bool_t +_dbus_rlimit_restore_fd_limit (DBusRLimit *saved, + DBusError *error) +{ + fd_limit_not_supported (error); + return FALSE; +} + #endif + +void +_dbus_rlimit_free (DBusRLimit *lim) +{ + dbus_free (lim); } void diff --git a/dbus/dbus-sysdeps-util-win.c b/dbus/dbus-sysdeps-util-win.c index 4678b11e..ec9afbb6 100644 --- a/dbus/dbus-sysdeps-util-win.c +++ b/dbus/dbus-sysdeps-util-win.c @@ -258,9 +258,42 @@ _dbus_change_to_daemon_user (const char *user, return TRUE; } +static void +fd_limit_not_supported (DBusError *error) +{ + dbus_set_error (error, DBUS_ERROR_NOT_SUPPORTED, + "cannot change fd limit on this platform"); +} + +DBusRLimit * +_dbus_rlimit_save_fd_limit (DBusError *error) +{ + fd_limit_not_supported (error); + return NULL; +} + +dbus_bool_t +_dbus_rlimit_raise_fd_limit_if_privileged (unsigned int desired, + DBusError *error) +{ + fd_limit_not_supported (error); + return FALSE; +} + +dbus_bool_t +_dbus_rlimit_restore_fd_limit (DBusRLimit *saved, + DBusError *error) +{ + fd_limit_not_supported (error); + return FALSE; +} + void -_dbus_request_file_descriptor_limit (unsigned int limit) +_dbus_rlimit_free (DBusRLimit *lim) { + /* _dbus_rlimit_save_fd_limit() cannot return non-NULL on Windows + * so there cannot be anything to free */ + _dbus_assert (lim == NULL); } void diff --git a/dbus/dbus-sysdeps.h b/dbus/dbus-sysdeps.h index 47ba2f43..5465128e 100644 --- a/dbus/dbus-sysdeps.h +++ b/dbus/dbus-sysdeps.h @@ -546,8 +546,6 @@ dbus_bool_t _dbus_change_to_daemon_user (const char *user, void _dbus_flush_caches (void); -void _dbus_request_file_descriptor_limit (unsigned int limit); - /* * replaces the term DBUS_PREFIX in configure_time_path by the * current dbus installation directory. On unix this function is a noop @@ -566,6 +564,15 @@ _dbus_replace_install_prefix (const char *configure_time_path); */ #define DBUS_DEFAULT_MESSAGE_UNIX_FDS 16 +typedef struct DBusRLimit DBusRLimit; + +DBusRLimit *_dbus_rlimit_save_fd_limit (DBusError *error); +dbus_bool_t _dbus_rlimit_raise_fd_limit_if_privileged (unsigned int desired, + DBusError *error); +dbus_bool_t _dbus_rlimit_restore_fd_limit (DBusRLimit *saved, + DBusError *error); +void _dbus_rlimit_free (DBusRLimit *lim); + /** @} */ DBUS_END_DECLS -- cgit v1.2.3 From fc50a44527cf083da913533360ce4644cd69b243 Mon Sep 17 00:00:00 2001 From: Simon McVittie Date: Thu, 6 Nov 2014 15:39:51 +0000 Subject: Embargoed security release for Monday --- NEWS | 11 +++++++++-- configure.ac | 4 ++-- 2 files changed, 11 insertions(+), 4 deletions(-) diff --git a/NEWS b/NEWS index 91f7e7fa..aa821fe1 100644 --- a/NEWS +++ b/NEWS @@ -1,7 +1,14 @@ -D-Bus 1.8.10 (UNRELEASED) +D-Bus 1.8.10 (2014-11-10) == -... +The “tenants with a leaking roof get priority” release. + +Security fixes: + +• Increase dbus-daemon's RLIMIT_NOFILE rlimit to 65536 + so that CVE-2014-3636 part A cannot exhaust the system bus' + file descriptors, completing the incomplete fix in 1.8.8. + (CVE-2014-7824, fd.o #85105; Simon McVittie, Alban Crequy) D-Bus 1.8.8 (2014-09-16) == diff --git a/configure.ac b/configure.ac index 287dbbbe..df32f238 100644 --- a/configure.ac +++ b/configure.ac @@ -3,7 +3,7 @@ AC_PREREQ([2.63]) m4_define([dbus_major_version], [1]) m4_define([dbus_minor_version], [8]) -m4_define([dbus_micro_version], [9]) +m4_define([dbus_micro_version], [10]) m4_define([dbus_version], [dbus_major_version.dbus_minor_version.dbus_micro_version]) AC_INIT([dbus],[dbus_version],[https://bugs.freedesktop.org/enter_bug.cgi?product=dbus],[dbus]) @@ -37,7 +37,7 @@ LT_CURRENT=11 ## increment any time the source changes; set to ## 0 if you increment CURRENT -LT_REVISION=7 +LT_REVISION=8 ## increment if any interfaces have been added; set to 0 ## if any interfaces have been changed or removed. removal has -- cgit v1.2.3