|Summary:||byteswapping a message doesn't change the byte-order mark|
|Product:||dbus||Reporter:||Simon McVittie <smcv>|
|Component:||core||Assignee:||Simon McVittie <smcv>|
|Status:||RESOLVED FIXED||QA Contact:||John (J5) Palmieri <johnp>|
|Priority:||high||CC:||brian.cameron, cosimo.alfarano, hp, will|
|i915 platform:||i915 features:|
|Bug Depends on:|
_dbus_header_byteswap: change the first byte of the message, not just the struct member
Add a test for marshalling and endian-swapping
dbus_message_demarshal_bytes_needed: correct a wrong assertion
marshal test: test dbus_message_demarshal_bytes_needed
Test that a message with the byte order mangled causes disconnection but no crash
Add a test for marshalling and endian-swapping (v2)
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.