Bug 431191 - xorg / XFree86: MIT-SHM part of CVE-2007-6429 fix incomplete
Summary: xorg / XFree86: MIT-SHM part of CVE-2007-6429 fix incomplete
Keywords:
Status: CLOSED WONTFIX
Alias: None
Product: Security Response
Classification: Other
Component: vulnerability
Version: unspecified
Hardware: All
OS: Linux
low
low
Target Milestone: ---
Assignee: Adam Jackson
QA Contact:
URL:
Whiteboard:
Depends On:
Blocks:
TreeView+ depends on / blocked
 
Reported: 2008-02-01 13:59 UTC by Tomas Hoger
Modified: 2019-09-29 12:23 UTC (History)
4 users (show)

Fixed In Version:
Clone Of:
Environment:
Last Closed: 2011-07-28 17:51:55 UTC
Embargoed:


Attachments (Terms of Use)


Links
System ID Private Priority Status Summary Last Updated
FreeDesktop.org 13520 0 None None None Never

Description Tomas Hoger 2008-02-01 13:59:47 UTC
Following Gentoo bug report points out that fix addressing integer overflow in
MIT-SHM extension (part of CVE-2007-6429, originally tracked via bug #413741) is
incomplete and may not properly protect against overflow for certain bit depths:

http://bugs.gentoo.org/show_bug.cgi?id=208343

Upstream git commit that should address this problem:

http://gitweb.freedesktop.org/?p=xorg/xserver.git;a=commit;h=be6c17fcf9efebc0bbcc3d9a25f8c5a2450c2161

Comment 2 Tomas Hoger 2008-02-01 17:03:32 UTC
Related discussion in the original upstream bug report for MIT-SHM overflow:

https://bugs.freedesktop.org/show_bug.cgi?id=13520#c9


Comment 3 Adam Jackson 2008-02-14 19:43:35 UTC
Yeah, should pull this in.

Comment 4 Adam Jackson 2008-02-18 23:00:10 UTC
On reflection, this subsequent change is unnecessary.

One bit of seemingly irrelevant trivia before we get started.  The maximum size
of a SysV shared memory segment is controlled by /proc/sys/kernel/shmmax.  In
rawhide this is 32M; I don't believe it was ever much larger than this.

The issue in the original CVE is about this macro:

#define VERIFY_SHMSIZE(shmdesc,offset,len,client) \
{ \
    if ((offset + len) > shmdesc->size) \
    { \
        return BadAccess; \
    } \
}

shmdesc is a descriptor of the SysV shared memory segment holding the image
data, and ->size is the size in bytes.  offset is the (byte) offset into the shm
segment where the image data starts, clamped to ->size by earlier validation
code.  len is a derived value based on the width, height, and depth of the image
data, which describes the byte length of the data, taking all relevant padding
into account.  offset is uint32 from the wire protocol, len is however big it
needs to be according to C promotion rules (though we'll cover that in a
second), and size is unsigned long.

Conveniently, len is always representable in a uint32.  The maximum pixel
dimensions of a pixmap in X are 32767 by 32767, and the maximum number of bytes
per pixel is 4.  4294705156 < 4294967295.  The fix, as initially committed,
correctly clamps the width and height values to 32767 each, and the maximum
depth (in bits) to 32, so we know len will fit in a uint32.

Therefore, on LP64 platforms, we're done.  size is uint64, and the sum of two
uint32s can not possibly overflow a uint64.  The remainder of this discussion is
relevant for ILP32 platforms only.

At this point, having validated width/height/depth, we compute size:

    size = PixmapBytePad(width, depth) * height;

PixmapBytePad(), in the worst case, pads out to the nearest multiple of 32.  So
size is now the size in bytes of the image; but this size could still be _huge_,
such that adding offset to it would wrap around.  So we check:

    if (sizeof(void *) == 4 && BitsPerPixel(depth) > 8) {
        if (size < width * height)
            return BadAlloc;
        /* thankfully, offset is unsigned */
        if (stuff->offset + size < size)
            return BadAlloc;
    }

The outer conditional checks that we're on ILP32 and that we are using at least
one byte per pixel for storage.  If so, then we check two things.  First we test
for the edge case where PixmapBytePad(width, depth) ends up larger than 131076,
which would overflow, and thus make size very small.  Second, we check for
overflow when adding offset to size.  So in either case here, we are correctly
protected; if the requested dimensions from the client, plus the offset into the
shm segment, would wrap around in process memory, we just refuse.

So far, there's no contention.  The modification in
https://bugs.freedesktop.org/show_bug.cgi?id=13520#c9 moves the (stuff->offset +
size < size) check outside the check for BitsPerPixel(depth) < 8.  While valid,
this is meaningless.  BitsPerPixel(depth) < 8 only for depth == 1.  This is the
case of a bitmap, literally only one bit per pixel.  In this case the maximum
allocation is 32767 * 32767 bits (plus right padding out to the next multiple of
32 bits), which is a mere 128M.  Since offset is clamped to size by earlier
validation code, and size is clamped to 32M by system policy, there is no
possible way that (stuff->offset + size) can be > 160M, which is a long long way
from overflowing.

There is one final possibility, which is that the shm segment for this case
could have been attached between 128 and 160M of the top of address space.  This
is also a non-issue, userspace processes occupy the low 3G of address space.

In short, the proposed modification does not protect against any new overflow
scenario.

Comment 5 Tomas Hoger 2008-02-19 17:48:50 UTC
Value of /proc/sys/kernel/shmmax may be set by system administrator to much
higher value than default of 32M.  To cause an overflow using depth == 1
pixmaps, which are not checked by currently used patch, attacker would have to
create request with SHM segment (stuff) of size (4G - 128M) and offset near the
end of that segment (stuff->offset) to cause an overflow.

It's quite unlikely that shmmax is set to a value that would allow such large
SHM segments.  Additionally, standard 32bit kernels only have 3G virtual address
space for applications, creating additional limit on maximum SHM segment size. 
So this may only be an issue on systems with shmmax set to ~4G and kernels with
4G-4G userspace-kernel address split.


Note You need to log in before you can comment on or make changes to this bug.