Bug 38120 - (CVE-2011-2200) byteswapping a message doesn't change the byte-order mark
(CVE-2011-2200)
byteswapping a message doesn't change the byte-order mark
Status: RESOLVED FIXED
Product: dbus
Classification: Unclassified
Component: core
1.4.x
Other All
: high major
Assigned To: Simon McVittie
John (J5) Palmieri
http://cgit.freedesktop.org/~smcv/dbu...
: patch
Depends on:
Blocks: dbus-1.4
  Show dependency treegraph
 
Reported: 2011-06-09 09:51 UTC by Simon McVittie
Modified: 2011-06-13 14:26 UTC (History)
4 users (show)

See Also:


Attachments
_dbus_header_byteswap: change the first byte of the message, not just the struct member (1.34 KB, patch)
2011-06-09 10:38 UTC, Simon McVittie
Details | Splinter Review
Add a test for marshalling and endian-swapping (8.82 KB, patch)
2011-06-09 10:41 UTC, Simon McVittie
Details | Splinter Review
dbus_message_demarshal_bytes_needed: correct a wrong assertion (918 bytes, patch)
2011-06-09 10:42 UTC, Simon McVittie
Details | Splinter Review
marshal test: test dbus_message_demarshal_bytes_needed (2.30 KB, patch)
2011-06-09 10:42 UTC, Simon McVittie
Details | Splinter Review
Test that a message with the byte order mangled causes disconnection but no crash (3.47 KB, patch)
2011-06-09 10:43 UTC, Simon McVittie
Details | Splinter Review
Add a test for marshalling and endian-swapping (v2) (8.97 KB, patch)
2011-06-09 10:48 UTC, Simon McVittie
Details | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Simon McVittie 2011-06-09 09:51:42 UTC
As described in (at least) these mailing list messages:

http://lists.freedesktop.org/archives/dbus/2007-March/007357.html
http://lists.freedesktop.org/archives/dbus/2011-May/014408.html

when libdbus byteswaps a message into native order, it doesn't alter byte 0 of the fixed header (the byte order). Normally this is harmless, but if you're going to forward the message to another connection (e.g. you are dbus-daemon), badness occurs.

If someone had opened a bug back in 2007, this might have been fixed by now. Or maybe not.

I believe I've fixed this (the fix that Havoc suggested in 2007 looks obviously-correct), but I'm going to construct a test-case so this doesn't come back.
Comment 1 Simon McVittie 2011-06-09 10:38:37 UTC
Created attachment 47779 [details] [review]
_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
Comment 2 Simon McVittie 2011-06-09 10:41:12 UTC
Created attachment 47780 [details] [review]
Add a test for marshalling and endian-swapping

This requires the infrastructure from Bug #34570.
Comment 3 Simon McVittie 2011-06-09 10:42:06 UTC
Created attachment 47781 [details] [review]
dbus_message_demarshal_bytes_needed: correct a wrong  assertion

It's entirely possible for a message to indicate how many bytes we need,
without actually being complete.

(The other caller of _dbus_header_have_message_untrusted seems to be correct.)
Comment 4 Simon McVittie 2011-06-09 10:42:39 UTC
Created attachment 47782 [details] [review]
marshal test: test dbus_message_demarshal_bytes_needed

(Requires Attachment #47780 [details])
Comment 5 Simon McVittie 2011-06-09 10:43:30 UTC
Created attachment 47783 [details] [review]
Test that a message with the byte order mangled causes  disconnection but no crash

(Requires more commits from Bug #34570)
Comment 6 Simon McVittie 2011-06-09 10:48:52 UTC
Created attachment 47784 [details] [review]
Add a test for marshalling and endian-swapping (v2)

Replacement for Attachment #47780 [details], now with less reliance on implementation details.
Comment 7 Simon McVittie 2011-06-10 00:22:44 UTC
This is also <http://bugs.debian.org/cgi-bin/bugreport.cgi?bug=629938>. I've asked the Debian security team to allocate a CVE ID, since this could be used as a local DoS.
Comment 8 Will Thompson 2011-06-10 08:03:18 UTC
Review of attachment 47781 [details] [review]:

This looks fine.
Comment 9 Will Thompson 2011-06-10 08:06:19 UTC
Review of attachment 47779 [details] [review]:

Looks correct. (Next up, reviewing the tests…)
Comment 10 Simon McVittie 2011-06-10 10:11:22 UTC
Comment on attachment 47779 [details] [review]
_dbus_header_byteswap: change the first byte of the  message, not just the struct member

Actual bugs fixed in git for 1.4.12, will be merged to master before 1.5.4.

Tests awaiting review.
Comment 11 Simon McVittie 2011-06-10 10:58:49 UTC
Fixed in git for 1.2.28, 1.4.12 and 1.5.4. Still waiting for a CVE number from the Debian security team, but if I don't get one soon I'll just release anyway.
Comment 12 Simon McVittie 2011-06-13 14:26:07 UTC
This is CVE-2011-2200.

See the Debian bug for a standalone version of the test case from Attachment #47784 [details].