Bug 107332 - truncated messages ending with boolean can over-read up to 3 bytes
Summary: truncated messages ending with boolean can over-read up to 3 bytes
Status: RESOLVED FIXED
Alias: None
Product: dbus
Classification: Unclassified
Component: core (show other bugs)
Version: git master
Hardware: Other All
: medium normal
Assignee: Simon McVittie
QA Contact: D-Bus Maintainers
URL:
Whiteboard: review+
Keywords: patch
Depends on:
Blocks:
 
Reported: 2018-07-23 10:28 UTC by Simon McVittie
Modified: 2018-08-02 22:13 UTC (History)
2 users (show)

See Also:
i915 platform:
i915 features:


Attachments
validate_body_helper: Bounds-check before validating booleans (2.18 KB, patch)
2018-07-23 10:28 UTC, Simon McVittie
Details | Splinter Review

Description Simon McVittie 2018-07-23 10:28:58 UTC
Created attachment 140779 [details] [review]
validate_body_helper: Bounds-check before validating booleans

Running the "embedded tests" through valgrind revealed that without the
attached patch, we would have been willing to read up to 3 bytes off the
end of a message if the message is truncated part way through a boolean. Any
practical allocator will round up allocations to the next 32-bit (or
larger) boundary, so in practice this will not leave the memory buffer
(and in particular did not crash during unit testing), but it could read
uninitialized contents.

On little-endian CPUs, an attacker might be able to use this to learn
whether up to 3 bytes of uninitialized memory in the dbus-daemon
were all-zero (their crafted message would be relayed) or not (their
connection would be disconnected for sending an invalid message). On
big-endian CPUs, an attacker might be able to use this to learn whether
up to 3 bytes were all-zeroes (relayed to a cooperating peer), 0-2
bytes of all-zeroes followed by 0x01 (relayed to a cooperating peer),
or something else (disconnected).

---

I don't think this is practically exploitable, so I'll probably handle this as an ordinary bug rather than as an embargoed security vulnerability; but I wanted to give other D-Bus maintainers a chance to object and point out ways it could be exploited if they disagree with me, so for now it's private.
Comment 1 Simon McVittie 2018-07-23 10:30:11 UTC
This bug was introduced in early 2005 (62e46533 "hardcode dbus_bool_t to 32 bits") so all supported dbus versions are affected.
Comment 2 Simon McVittie 2018-07-23 18:08:46 UTC
I'll also need reviews on Bug #107349 and Bug #107350 before I can actually release this fix.
Comment 3 Thiago Macieira 2018-07-28 21:11:54 UTC
Comment on attachment 140779 [details] [review]
validate_body_helper: Bounds-check before validating booleans

Review of attachment 140779 [details] [review]:
-----------------------------------------------------------------

Looks good to me. Ship it.
Comment 4 Simon McVittie 2018-07-30 10:51:20 UTC
(In reply to Thiago Macieira from comment #3)
> Looks good to me. Ship it.

Do you agree with my assessment that this is probably not a practical security vulnerability, or do you think we need to do the embargo/CVE thing?
Comment 5 Philip Withnall 2018-07-30 15:02:00 UTC
Comment on attachment 140779 [details] [review]
validate_body_helper: Bounds-check before validating booleans

Review of attachment 140779 [details] [review]:
-----------------------------------------------------------------

r+

I can’t think of an easy way to exploit this, unless there’s a way for the attacker to guarantee that the heap allocation for their crafted message is over the top of some previously-freed memory which they care about getting some bytes from.

Maybe we should explicitly zero the allocation of an incoming message before doing anything about parsing that message, as paranoia? That would only guard against bugs of this class found in future releases though.
Comment 6 Simon McVittie 2018-08-02 18:39:47 UTC
Making this public since we have concluded that it isn't a security flaw.
Comment 7 Simon McVittie 2018-08-02 22:13:42 UTC
Fixed in git for 1.12.0, 1.13.6 and 1.10.28, thanks


Use of freedesktop.org services, including Bugzilla, is subject to our Code of Conduct. How we collect and use information is described in our Privacy Policy.