On Thu, 09 Jun 2011 at 19:20:27 +0100, Simon McVittie wrote: > Tags: security > Forwarded: https://bugs.freedesktop.org/show_bug.cgi?id=38120 > > lbdbus-1-3, used by dbus-daemon, swaps the byte-order of incoming messages > into native endianness but does not swap the byte-order mark in messages > when swapping their byte order. As a result, if a message in non-native byte > order is sent through dbus-daemon to a system service like Avahi or > NetworkManager, that system service is likely to interpret the message as > invalid and disconnect from the system bus, leading to a local DoS.
I've fixed this upstream and in sid and experimental. Still waiting for a
CVE ID - or should I ask for one elsewhere?
Here is a proposed stable update (either for security or stable updates),
and a test-case (marshal.c). The proposed stable update is also available
on the debian-squeeze branch in git.
The test case requires libdbus-1-dev, libdbus-glib-1-dev and libglib2.0-dev,
and can be run with:
gcc -otest-marshal marshal.c \
`pkg-config --cflags --libs dbus-1 dbus-glib-1 glib-2.0`
./test-marshal
For it to work, it must be run by a user whose home directory (according
to /etc/passwd, not $HOME) can be written.
Successful output looks like this:
/demarshal/le: OK
/demarshal/be: OK
/demarshal/needed/le: OK
/demarshal/needed/be: OK
Unsuccessful output on a little-endian architecture looks like this:
/demarshal/le: OK
/demarshal/be: **
ERROR:marshal.c:193:test_endian: assertion failed (get_uint32 (output,
OFFSET_BODY_LENGTH, output[0]) == 8): (134217728 == 8)
Aborted
Big-endian architectures should fail /demarshal/le in a similar way.
(You can also unpack /usr/lib/dbus-1.0/test/test-marshal from dbus-1-dbg of
an appropriate architecture in unstable - it's the same test-case, and
should hopefully work with an older libdbus.)
Regards,
S
/* Simple sanity-check for D-Bus message serialization.
*
* Author: Simon McVittie <simon.mcvit...@collabora.co.uk>
* Copyright © 2010-2011 Nokia 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 <glib.h>
#include <dbus/dbus.h>
#include <dbus/dbus-glib-lowlevel.h>
typedef struct {
DBusError e;
} Fixture;
static void
assert_no_error (const DBusError *e)
{
if (G_UNLIKELY (dbus_error_is_set (e)))
g_error ("expected success but got error: %s: %s", e->name, e->message);
}
static void
setup (Fixture *f,
gconstpointer arg G_GNUC_UNUSED)
{
dbus_error_init (&f->e);
}
/* this is meant to be obviously correct, not efficient! */
static guint32
get_uint32 (const gchar *blob,
gsize offset,
char endian)
{
if (endian == 'l')
{
return
blob[offset] |
(blob[offset + 1] << 8) |
(blob[offset + 2] << 16) |
(blob[offset + 3] << 24);
}
else if (endian == 'B')
{
return
(blob[offset] << 24) |
(blob[offset + 1] << 16) |
(blob[offset + 2] << 8) |
blob[offset + 3];
}
else
{
g_assert_not_reached ();
}
}
#define BLOB_LENGTH (sizeof (le_blob) - 1)
#define OFFSET_BODY_LENGTH (4)
#define OFFSET_SERIAL (8)
const gchar le_blob[] =
/* byte 0 */
/* yyyyuu fixed headers */
"l" /* little-endian */
"\2" /* reply (which is the simplest message) */
"\2" /* no auto-starting */
"\1" /* D-Bus version = 1 */
/* byte 4 */
"\4\0\0\0" /* bytes in body = 4 */
/* byte 8 */
"\x78\x56\x34\x12" /* serial number = 0x12345678 */
/* byte 12 */
/* a(uv) variable headers start here */
"\x0f\0\0\0" /* bytes in array of variable headers = 15 */
/* pad to 8-byte boundary = nothing */
/* byte 16 */
"\5" /* in reply to: */
"\1u\0" /* variant signature = u */
/* pad to 4-byte boundary = nothing */
"\x12\xef\xcd\xab" /* 0xabcdef12 */
/* pad to 8-byte boundary = nothing */
/* byte 24 */
"\x08" /* signature: */
"\1g\0" /* variant signature = g */
"\1u\0" /* 1 byte, u, NUL (no alignment needed) */
"\0" /* pad to 8-byte boundary for body */
/* body; byte 32 */
"\xef\xbe\xad\xde" /* 0xdeadbeef */
;
const gchar be_blob[] =
/* byte 0 */
/* yyyyuu fixed headers */
"B" /* big-endian */
"\2" /* reply (which is the simplest message) */
"\2" /* no auto-starting */
"\1" /* D-Bus version = 1 */
/* byte 4 */
"\0\0\0\4" /* bytes in body = 4 */
/* byte 8 */
"\x12\x34\x56\x78" /* serial number = 0x12345678 */
/* byte 12 */
/* a(uv) variable headers start here */
"\0\0\0\x0f" /* bytes in array of variable headers = 15 */
/* pad to 8-byte boundary = nothing */
/* byte 16 */
"\5" /* in reply to: */
"\1u\0" /* variant signature = u */
/* pad to 4-byte boundary = nothing */
"\xab\xcd\xef\x12" /* 0xabcdef12 */
/* pad to 8-byte boundary = nothing */
/* byte 24 */
"\x08" /* signature: */
"\1g\0" /* variant signature = g */
"\1u\0" /* 1 byte, u, NUL (no alignment needed) */
"\0" /* pad to 8-byte boundary for body */
/* body; byte 32 */
"\xde\xad\xbe\xef" /* 0xdeadbeef */
;
static void
test_endian (Fixture *f,
gconstpointer arg)
{
const gchar *blob = arg;
const gchar *native_blob;
char *output;
DBusMessage *m;
int len;
dbus_uint32_t u;
dbus_bool_t ok;
g_assert_cmpuint ((guint) sizeof (le_blob), ==, (guint) sizeof (be_blob));
g_assert_cmpuint (get_uint32 (blob, OFFSET_BODY_LENGTH, blob[0]), ==, 4);
g_assert_cmpuint (get_uint32 (blob, OFFSET_SERIAL, blob[0]), ==,
0x12345678u);
len = dbus_message_demarshal_bytes_needed (blob, sizeof (le_blob));
/* everything in the string except the implicit "\0" at the end is part of
* the message */
g_assert_cmpint (len, ==, BLOB_LENGTH);
m = dbus_message_demarshal (blob, sizeof (le_blob), &f->e);
assert_no_error (&f->e);
g_assert (m != NULL);
g_assert_cmpuint (dbus_message_get_serial (m), ==, 0x12345678u);
g_assert_cmpuint (dbus_message_get_reply_serial (m), ==, 0xabcdef12u);
g_assert_cmpstr (dbus_message_get_signature (m), ==, "u");
/* Implementation detail: appending to the message results in it being
* byteswapped into compiler byte order, which exposed a bug in libdbus,
* fd.o #38120. (If that changes, this test might not exercise that
* particular bug but will still be valid.) */
u = 0xdecafbadu;
ok = dbus_message_append_args (m,
DBUS_TYPE_UINT32, &u,
DBUS_TYPE_INVALID);
g_assert (ok);
dbus_message_marshal (m, &output, &len);
g_assert (output[0] == 'l' || output[0] == 'B');
/* the single-byte fields are unaffected, even if the endianness was
* swapped */
g_assert_cmpint (output[1], ==, blob[1]);
g_assert_cmpint (output[2], ==, blob[2]);
g_assert_cmpint (output[3], ==, blob[3]);
/* the length and serial are in the new endianness, the length has expanded
* to 8, and the serial is correct */
g_assert_cmpuint (get_uint32 (output, OFFSET_BODY_LENGTH, output[0]), ==, 8);
g_assert_cmpuint (get_uint32 (output, OFFSET_SERIAL, output[0]), ==,
0x12345678u);
/* the second "u" in the signature replaced a padding byte, so only
* the length of the body changed */
g_assert_cmpint (len, ==, BLOB_LENGTH + 4);
}
static void
test_needed (Fixture *f,
gconstpointer arg)
{
const gchar *blob = arg;
/* We need at least 16 bytes to know how long the message is - that's just
* a fact of the D-Bus protocol. */
g_assert_cmpint (
dbus_message_demarshal_bytes_needed (blob, 0), ==, 0);
g_assert_cmpint (
dbus_message_demarshal_bytes_needed (blob, 15), ==, 0);
/* This is enough that we should be able to tell how much we need. */
g_assert_cmpint (
dbus_message_demarshal_bytes_needed (blob, 16), ==, BLOB_LENGTH);
/* The header is 32 bytes long (here), so that's another interesting
* boundary. */
g_assert_cmpint (
dbus_message_demarshal_bytes_needed (blob, 31), ==, BLOB_LENGTH);
g_assert_cmpint (
dbus_message_demarshal_bytes_needed (blob, 32), ==, BLOB_LENGTH);
g_assert_cmpint (
dbus_message_demarshal_bytes_needed (blob, 33), ==, BLOB_LENGTH);
g_assert_cmpint (
dbus_message_demarshal_bytes_needed (blob, BLOB_LENGTH - 1), ==,
BLOB_LENGTH);
g_assert_cmpint (
dbus_message_demarshal_bytes_needed (blob, BLOB_LENGTH), ==,
BLOB_LENGTH);
g_assert_cmpint (
dbus_message_demarshal_bytes_needed (blob, sizeof (be_blob)), ==,
BLOB_LENGTH);
}
static void
teardown (Fixture *f,
gconstpointer arg G_GNUC_UNUSED)
{
dbus_error_free (&f->e);
}
int
main (int argc,
char **argv)
{
g_test_init (&argc, &argv, NULL);
g_test_add ("/demarshal/le", Fixture, le_blob, setup, test_endian, teardown);
g_test_add ("/demarshal/be", Fixture, be_blob, setup, test_endian, teardown);
g_test_add ("/demarshal/needed/le", Fixture, le_blob, setup, test_needed,
teardown);
g_test_add ("/demarshal/needed/be", Fixture, be_blob, setup, test_needed,
teardown);
return g_test_run ();
}
diffstat for dbus-1.2.24 dbus-1.2.24
changelog | 8 ++++
control | 4 +-
patches/13-629938-_dbus_header_byteswap.patch | 46 ++++++++++++++++++++++++++
patches/series | 1
4 files changed, 57 insertions(+), 2 deletions(-)
diff -Nru dbus-1.2.24/debian/changelog dbus-1.2.24/debian/changelog
--- dbus-1.2.24/debian/changelog 2010-12-21 17:46:17.000000000 +0000
+++ dbus-1.2.24/debian/changelog 2011-06-12 12:50:11.000000000 +0100
@@ -1,3 +1,11 @@
+dbus (1.2.24-4+squeeze1~unreleased) UNRELEASED; urgency=low
+
+ * Update Vcs-* control fields to reflect the move to git
+ * Apply patch to fix upstream bug fd.o #38120, which is a local DoS for
+ system services (Closes: #629938)
+
+ -- Simon McVittie <s...@debian.org> Sun, 12 Jun 2011 12:18:39 +0100
+
dbus (1.2.24-4) unstable; urgency=high
* debian/patches/12-CVE-2010-4352-reject-deeply-nested-variants.patch
diff -Nru dbus-1.2.24/debian/control dbus-1.2.24/debian/control
--- dbus-1.2.24/debian/control 2010-12-21 17:32:03.000000000 +0000
+++ dbus-1.2.24/debian/control 2011-06-12 12:31:16.000000000 +0100
@@ -17,8 +17,8 @@
libx11-dev,
libselinux1-dev [linux-any]
Standards-Version: 3.9.0
-Vcs-Svn: svn://svn.debian.org/pkg-utopia/packages/unstable/dbus/
-Vcs-Browser: http://svn.debian.org/wsvn/pkg-utopia/packages/unstable/dbus/
+Vcs-Git: git://anonscm.debian.org/pkg-utopia/dbus.git
+Vcs-Browser: http://anonscm.debian.org/gitweb/?p=pkg-utopia/dbus.git
Homepage: http://dbus.freedesktop.org/
Package: dbus
diff -Nru dbus-1.2.24/debian/patches/13-629938-_dbus_header_byteswap.patch dbus-1.2.24/debian/patches/13-629938-_dbus_header_byteswap.patch
--- dbus-1.2.24/debian/patches/13-629938-_dbus_header_byteswap.patch 1970-01-01 01:00:00.000000000 +0100
+++ dbus-1.2.24/debian/patches/13-629938-_dbus_header_byteswap.patch 2011-06-12 12:43:20.000000000 +0100
@@ -0,0 +1,46 @@
+From: Simon McVittie <simon.mcvit...@collabora.co.uk>
+Date: Thu, 9 Jun 2011 17:52:10 +0100
+Subject: [PATCH] _dbus_header_byteswap: change the first byte of the message,
+ not just the struct member
+
+This has been wrong approximately forever, for instance see:
+http://lists.freedesktop.org/archives/dbus/2007-March/007357.html
+
+This prevents a local DoS, in which users can disconnect a system service
+from the system bus by sending a non-native-endian message to it.
+
+Bug: https://bugs.freedesktop.org/show_bug.cgi?id=38120
+Bug-Debian: http://bugs.debian.org/cgi-bin/bugreport.cgi?bug=629938
+Reviewed-by: Will Thompson <will.thomp...@collabora.co.uk>
+Origin: upstream, http://cgit.freedesktop.org/dbus/dbus/commit/?id=c3223ba6
+Applied-upstream: 1.4.12, commit:c3223ba6c401ba81df1305851312a47c485e6cd7
+Applied-upstream: 1.2.28, commit:6519a1f77c61d753d4c97efd6e15630eb275336e
+---
+ dbus/dbus-marshal-header.c | 6 ++++++
+ 1 files changed, 6 insertions(+), 0 deletions(-)
+
+diff --git a/dbus/dbus-marshal-header.c b/dbus/dbus-marshal-header.c
+index 3f31d7a..a6c9b80 100644
+--- a/dbus/dbus-marshal-header.c
++++ b/dbus/dbus-marshal-header.c
+@@ -1468,14 +1468,20 @@ void
+ _dbus_header_byteswap (DBusHeader *header,
+ int new_order)
+ {
++ unsigned char byte_order;
++
+ if (header->byte_order == new_order)
+ return;
+
++ byte_order = _dbus_string_get_byte (&header->data, BYTE_ORDER_OFFSET);
++ _dbus_assert (header->byte_order == byte_order);
++
+ _dbus_marshal_byteswap (&_dbus_header_signature_str,
+ 0, header->byte_order,
+ new_order,
+ &header->data, 0);
+
++ _dbus_string_set_byte (&header->data, BYTE_ORDER_OFFSET, new_order);
+ header->byte_order = new_order;
+ }
+
diff -Nru dbus-1.2.24/debian/patches/series dbus-1.2.24/debian/patches/series
--- dbus-1.2.24/debian/patches/series 2010-12-21 17:34:52.000000000 +0000
+++ dbus-1.2.24/debian/patches/series 2011-06-12 12:51:34.000000000 +0100
@@ -3,3 +3,4 @@
10_dbus-1.0.1-generate-xml-docs.patch
11-589662-reload-kqueue.patch
12-CVE-2010-4352-reject-deeply-nested-variants.patch
+13-629938-_dbus_header_byteswap.patch
signature.asc
Description: Digital signature
