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
Related discussion in the original upstream bug report for MIT-SHM overflow: https://bugs.freedesktop.org/show_bug.cgi?id=13520#c9
Yeah, should pull this in.
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.
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.