From b434238c346a356b7aee4e43a9b88c1df1d4aef5 Mon Sep 17 00:00:00 2001 From: Chengwei Yang Date: Wed, 29 May 2013 20:50:21 +0800 Subject: [PATCH 1/8] When "activating" systemd, handle its special case better When dbus-daemon receives a request to activate a systemd service before systemd has connected to it, it enqueues a fake request to "activate" systemd itself (as a way to get a BusPendingActivationEntry to track the process of waiting for systemd). When systemd later joins the bus, dbus-daemon sends the actual activation message; any future activation messages are sent directly to systemd. In the "pending" code path, the activation messages are currently dispatched as though they had been sent by the same process that sent the original activation request, which is wrong: the bus security policy probably doesn't allow that process to talk to systemd directly. They should be dispatched as though they had been sent by the dbus-daemon itself (connection == NULL), the same as in the non-pending code path. In the worst case, if the attempt to activate systemd timed out, the dbus-daemon would crash with a (fatal) warning, because in this special case, activation_message is a signal with no serial number, whereas the code to send an error reply is expecting a method call with a serial number. Bug: https://bugs.freedesktop.org/show_bug.cgi?id=50199 Signed-off-by: Chengwei Yang Tested-by: Ma Yu Reviewed-by: Simon McVittie --- bus/activation.c | 19 ++++++++++++++----- 1 file changed, 14 insertions(+), 5 deletions(-) diff --git a/bus/activation.c b/bus/activation.c index 3dfba787..fcb71337 100644 --- a/bus/activation.c +++ b/bus/activation.c @@ -80,7 +80,14 @@ typedef struct BusPendingActivationEntry BusPendingActivationEntry; struct BusPendingActivationEntry { + /* Normally a method call, but if connection is NULL, this is a signal + * instead. + */ DBusMessage *activation_message; + /* NULL if this activation entry is for the dbus-daemon itself, + * waiting for systemd to start. In this case, auto_activation is always + * TRUE. + */ DBusConnection *connection; dbus_bool_t auto_activation; @@ -1106,7 +1113,8 @@ bus_activation_service_created (BusActivation *activation, BusPendingActivationEntry *entry = link->data; DBusList *next = _dbus_list_get_next_link (&pending_activation->entries, link); - if (dbus_connection_get_is_connected (entry->connection)) + /* entry->connection is NULL for activating systemd */ + if (entry->connection && dbus_connection_get_is_connected (entry->connection)) { /* Only send activation replies to regular activation requests. */ if (!entry->auto_activation) @@ -1175,7 +1183,7 @@ bus_activation_send_pending_auto_activation_messages (BusActivation *activation BusPendingActivationEntry *entry = link->data; DBusList *next = _dbus_list_get_next_link (&pending_activation->entries, link); - if (entry->auto_activation && dbus_connection_get_is_connected (entry->connection)) + if (entry->auto_activation && (entry->connection == NULL || dbus_connection_get_is_connected (entry->connection))) { DBusConnection *addressed_recipient; @@ -1233,7 +1241,7 @@ try_send_activation_failure (BusPendingActivation *pending_activation, BusPendingActivationEntry *entry = link->data; DBusList *next = _dbus_list_get_next_link (&pending_activation->entries, link); - if (dbus_connection_get_is_connected (entry->connection)) + if (entry->connection && dbus_connection_get_is_connected (entry->connection)) { if (!bus_transaction_send_error_reply (transaction, entry->connection, @@ -1759,7 +1767,8 @@ bus_activation_activate_service (BusActivation *activation, pending_activation_entry->activation_message = activation_message; dbus_message_ref (activation_message); pending_activation_entry->connection = connection; - dbus_connection_ref (connection); + if (connection) + dbus_connection_ref (connection); /* Check if the service is being activated */ pending_activation = _dbus_hash_table_lookup_string (activation->pending_activations, service_name); @@ -1972,7 +1981,7 @@ bus_activation_activate_service (BusActivation *activation, service_name, entry->systemd_service); /* systemd is not around, let's "activate" it. */ - retval = bus_activation_activate_service (activation, connection, activation_transaction, TRUE, + retval = bus_activation_activate_service (activation, NULL, activation_transaction, TRUE, message, "org.freedesktop.systemd1", error); } From 634dc5d8a02c4de31c146e845e908f01d803d411 Mon Sep 17 00:00:00 2001 From: Chengwei Yang Date: Fri, 31 May 2013 15:02:45 +0800 Subject: [PATCH 2/8] Fix build error: unused-result Signed-off-by: Chengwei Yang Reviewed-by: Simon McVittie --- bus/main.c | 10 ++++++++-- 1 file changed, 8 insertions(+), 2 deletions(-) diff --git a/bus/main.c b/bus/main.c index ca0be3ce..970c1def 100644 --- a/bus/main.c +++ b/bus/main.c @@ -91,7 +91,10 @@ signal_handler (int sig) static const char message[] = "Unable to write to reload pipe - buffer full?\n"; - write (STDERR_FILENO, message, strlen (message)); + if (write (STDERR_FILENO, message, strlen (message)) != strlen (message)) + { + /* ignore failure to write out a warning */ + } } } break; @@ -113,7 +116,10 @@ signal_handler (int sig) "Unable to write termination signal to pipe - buffer full?\n" "Will exit instead.\n"; - write (STDERR_FILENO, message, strlen (message)); + if (write (STDERR_FILENO, message, strlen (message)) != strlen (message)) + { + /* ignore failure to write out a warning */ + } _exit (1); } } From 16f3b1246c134a4ed72779ce248902e9b03d38f6 Mon Sep 17 00:00:00 2001 From: Chengwei Yang Date: Thu, 6 Jun 2013 13:25:10 +0800 Subject: [PATCH 3/8] Fix dbus-daemon crash due to invalid service file dbus-daemon will crash due to invalid service file which key/value starts before section. In that situation, new_line() will try to access invalid address. Bug: https://bugs.freedesktop.org/show_bug.cgi?id=60853 Signed-off-by: Chengwei Yang Reviewed-by: Simon McVittie --- bus/desktop-file.c | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/bus/desktop-file.c b/bus/desktop-file.c index ae441c5e..bfeb72e2 100644 --- a/bus/desktop-file.c +++ b/bus/desktop-file.c @@ -688,6 +688,12 @@ bus_desktop_file_load (DBusString *filename, else if (is_blank_line (&parser) || _dbus_string_get_byte (&parser.data, parser.pos) == '#') parse_comment_or_blank (&parser); + else if (parser.current_section < 0) + { + dbus_set_error(error, DBUS_ERROR_FAILED, + "invalid service file: key=value before [Section]"); + return NULL; + } else { if (!parse_key_value (&parser, error)) From 355b470da78e25cb451eab0c49f30437b2c5ccb9 Mon Sep 17 00:00:00 2001 From: Simon McVittie Date: Wed, 12 Jun 2013 13:42:58 +0100 Subject: [PATCH 4/8] NEWS for 1.6.x --- NEWS | 11 ++++++++++- 1 file changed, 10 insertions(+), 1 deletion(-) diff --git a/NEWS b/NEWS index 8dd994a5..f084c8ab 100644 --- a/NEWS +++ b/NEWS @@ -1,7 +1,16 @@ D-Bus 1.6.12 (UNRELEASED) == -... +Fixes: + +• In dbus-daemon, don't crash if a .service file starts with key=value + (fd.o #60853, Chengwei Yang) + +• Unix-specific: + · Fix an assertion failure if we try to activate systemd services before + systemd connects to the bus (fd.o #50199, Chengwei Yang) + · Avoid compiler warnings for ignoring the return from write() + (Chengwei Yang) D-Bus 1.6.10 (2013-04-24) == From 954d75b2b64e4799f360d2a6bf9cff6d9fee37e7 Mon Sep 17 00:00:00 2001 From: Simon McVittie Date: Mon, 10 Jun 2013 18:06:47 +0100 Subject: [PATCH 5/8] CVE-2013-2168: _dbus_printf_string_upper_bound: copy the va_list for each use Using a va_list more than once is non-portable: it happens to work under the ABI of (for instance) x86 Linux, but not x86-64 Linux. This led to _dbus_printf_string_upper_bound() crashing if it should have returned exactly 1024 bytes. Many system services can be induced to process a caller-controlled string in ways that end up using _dbus_printf_string_upper_bound(), so this is a denial of service. Reviewed-by: Thiago Macieira --- dbus/dbus-sysdeps-unix.c | 16 +++++++++++++--- dbus/dbus-sysdeps-win.c | 9 +++++++-- 2 files changed, 20 insertions(+), 5 deletions(-) diff --git a/dbus/dbus-sysdeps-unix.c b/dbus/dbus-sysdeps-unix.c index fc677990..e31c7355 100644 --- a/dbus/dbus-sysdeps-unix.c +++ b/dbus/dbus-sysdeps-unix.c @@ -3121,8 +3121,11 @@ _dbus_printf_string_upper_bound (const char *format, char static_buf[1024]; int bufsize = sizeof (static_buf); int len; + va_list args_copy; - len = vsnprintf (static_buf, bufsize, format, args); + DBUS_VA_COPY (args_copy, args); + len = vsnprintf (static_buf, bufsize, format, args_copy); + va_end (args_copy); /* If vsnprintf() returned non-negative, then either the string fits in * static_buf, or this OS has the POSIX and C99 behaviour where vsnprintf @@ -3138,8 +3141,12 @@ _dbus_printf_string_upper_bound (const char *format, * or the real length could be coincidentally the same. Which is it? * If vsnprintf returns the truncated length, we'll go to the slow * path. */ - if (vsnprintf (static_buf, 1, format, args) == 1) + DBUS_VA_COPY (args_copy, args); + + if (vsnprintf (static_buf, 1, format, args_copy) == 1) len = -1; + + va_end (args_copy); } /* If vsnprintf() returned negative, we have to do more work. @@ -3155,7 +3162,10 @@ _dbus_printf_string_upper_bound (const char *format, if (buf == NULL) return -1; - len = vsnprintf (buf, bufsize, format, args); + DBUS_VA_COPY (args_copy, args); + len = vsnprintf (buf, bufsize, format, args_copy); + va_end (args_copy); + dbus_free (buf); /* If the reported length is exactly the buffer size, round up to the diff --git a/dbus/dbus-sysdeps-win.c b/dbus/dbus-sysdeps-win.c index bc4951b5..c42316f1 100644 --- a/dbus/dbus-sysdeps-win.c +++ b/dbus/dbus-sysdeps-win.c @@ -538,9 +538,12 @@ int _dbus_printf_string_upper_bound (const char *format, char buf[1024]; int bufsize; int len; + va_list args_copy; bufsize = sizeof (buf); - len = _vsnprintf (buf, bufsize - 1, format, args); + DBUS_VA_COPY (args_copy, args); + len = _vsnprintf (buf, bufsize - 1, format, args_copy); + va_end (args_copy); while (len == -1) /* try again */ { @@ -553,7 +556,9 @@ int _dbus_printf_string_upper_bound (const char *format, if (p == NULL) return -1; - len = _vsnprintf (p, bufsize - 1, format, args); + DBUS_VA_COPY (args_copy, args); + len = _vsnprintf (p, bufsize - 1, format, args_copy); + va_end (args_copy); free (p); } From 2420f7ae8b72405de1a41760b213e2e0849b2b8d Mon Sep 17 00:00:00 2001 From: Simon McVittie Date: Wed, 12 Jun 2013 13:56:39 +0100 Subject: [PATCH 6/8] Add a test-case for CVE-2013-2168 Reviewed-by: Thiago Macieira [build system adjusted to compile it even if we don't have GLib -smcv] --- test/Makefile.am | 5 +++ test/internals/printf.c | 89 +++++++++++++++++++++++++++++++++++++++++ 2 files changed, 94 insertions(+) create mode 100644 test/internals/printf.c diff --git a/test/Makefile.am b/test/Makefile.am index e9448998..6f0e6e10 100644 --- a/test/Makefile.am +++ b/test/Makefile.am @@ -81,6 +81,10 @@ shell_test_LDADD = libdbus-testutils.la spawn_test_CPPFLAGS = $(static_cppflags) spawn_test_LDADD = $(top_builddir)/dbus/libdbus-internal.la +test_printf_SOURCES = internals/printf.c +test_printf_CPPFLAGS = $(static_cppflags) +test_printf_LDADD = $(top_builddir)/dbus/libdbus-internal.la + test_refs_SOURCES = internals/refs.c test_refs_CPPFLAGS = $(static_cppflags) test_refs_LDADD = libdbus-testutils.la $(GLIB_LIBS) @@ -97,6 +101,7 @@ testexec_PROGRAMS = installable_tests = \ shell-test \ + test-printf \ $(NULL) if DBUS_WITH_GLIB diff --git a/test/internals/printf.c b/test/internals/printf.c new file mode 100644 index 00000000..2d2fff8d --- /dev/null +++ b/test/internals/printf.c @@ -0,0 +1,89 @@ +/* Regression test for _dbus_printf_string_upper_bound + * + * Author: Simon McVittie + * Copyright © 2013 Intel Corporation + * + * Permission is hereby granted, free of charge, to any person + * obtaining a copy of this software and associated documentation files + * (the "Software"), to deal in the Software without restriction, + * including without limitation the rights to use, copy, modify, merge, + * publish, distribute, sublicense, and/or sell copies of the Software, + * and to permit persons to whom the Software is furnished to do so, + * subject to the following conditions: + * + * The above copyright notice and this permission notice shall be + * included in all copies or substantial portions of the Software. + * + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, + * EXPRESS OR IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF + * MERCHANTABILITY, FITNESS FOR A PARTICULAR PURPOSE AND + * NONINFRINGEMENT. IN NO EVENT SHALL THE AUTHORS OR COPYRIGHT HOLDERS + * BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER LIABILITY, WHETHER IN AN + * ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM, OUT OF OR IN + * CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE + * SOFTWARE. + */ + +#include + +#define DBUS_COMPILATION /* this test uses libdbus-internal */ +#include +#include +#include +#include "test-utils.h" + +#include +#include + +static void +do_test (int minimum, + const char *format, + ...) +{ + va_list ap; + int result; + + va_start (ap, format); + result = _dbus_printf_string_upper_bound (format, ap); + va_end (ap); + + if (result < minimum) + { + fprintf (stderr, "expected at least %d, got %d\n", minimum, result); + abort (); + } +} + +#define X_TIMES_8 "XXXXXXXX" +#define X_TIMES_16 X_TIMES_8 X_TIMES_8 +#define X_TIMES_32 X_TIMES_16 X_TIMES_16 +#define X_TIMES_64 X_TIMES_32 X_TIMES_32 +#define X_TIMES_128 X_TIMES_64 X_TIMES_64 +#define X_TIMES_256 X_TIMES_128 X_TIMES_128 +#define X_TIMES_512 X_TIMES_256 X_TIMES_256 +#define X_TIMES_1024 X_TIMES_512 X_TIMES_512 + +int +main (int argc, + char **argv) +{ + char buf[] = X_TIMES_1024 X_TIMES_1024 X_TIMES_1024 X_TIMES_1024; + int i; + + do_test (1, "%d", 0); + do_test (7, "%d", 1234567); + do_test (3, "%f", 3.5); + + do_test (0, "%s", ""); + do_test (1024, "%s", X_TIMES_1024); + do_test (1025, "%s", X_TIMES_1024 "Y"); + + for (i = 4096; i > 0; i--) + { + buf[i] = '\0'; + do_test (i, "%s", buf); + do_test (i + 3, "%s:%d", buf, 42); + } + + return 0; +} From 159fdbf680d2dcdd5f80568c3305e93114caddfa Mon Sep 17 00:00:00 2001 From: Simon McVittie Date: Wed, 12 Jun 2013 14:02:31 +0100 Subject: [PATCH 7/8] Prepare embargoed release for tomorrow --- NEWS | 6 +++++- configure.ac | 4 ++-- 2 files changed, 7 insertions(+), 3 deletions(-) diff --git a/NEWS b/NEWS index f084c8ab..9a577c36 100644 --- a/NEWS +++ b/NEWS @@ -1,8 +1,12 @@ -D-Bus 1.6.12 (UNRELEASED) +D-Bus 1.6.12 (2013-06-13) == Fixes: +• CVE-2013-2168: Fix misuse of va_list that could be used as a denial + of service for system services. Vulnerability reported by Alexandru Cornea. + (Simon) + • In dbus-daemon, don't crash if a .service file starts with key=value (fd.o #60853, Chengwei Yang) diff --git a/configure.ac b/configure.ac index 1f0597bd..a963d4d5 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], [6]) -m4_define([dbus_micro_version], [11]) +m4_define([dbus_micro_version], [12]) 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=10 ## increment any time the source changes; set to ## 0 if you increment CURRENT -LT_REVISION=3 +LT_REVISION=4 ## increment if any interfaces have been added; set to 0 ## if any interfaces have been changed or removed. removal has From 22fd9df043aab7e1e344909c45cde472e93bd152 Mon Sep 17 00:00:00 2001 From: Simon McVittie Date: Wed, 12 Jun 2013 14:46:24 +0100 Subject: [PATCH 8/8] Start 1.6.13 --- NEWS | 5 +++++ configure.ac | 2 +- 2 files changed, 6 insertions(+), 1 deletion(-) diff --git a/NEWS b/NEWS index 9a577c36..46cf8b32 100644 --- a/NEWS +++ b/NEWS @@ -1,3 +1,8 @@ +D-Bus 1.6.14 (UNRELEASED) +== + +... + D-Bus 1.6.12 (2013-06-13) == diff --git a/configure.ac b/configure.ac index a963d4d5..dab29ee4 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], [6]) -m4_define([dbus_micro_version], [12]) +m4_define([dbus_micro_version], [13]) 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])