|Summary:||[fb/afb/cfb/mfb] exploitable overflow in pixmap creation|
|Product:||xorg||Reporter:||Luke Hutchison <luke.hutchison>|
|Component:||Server/General||Assignee:||Adam Jackson <ajax>|
|Status:||RESOLVED FIXED||QA Contact:||X.Org Security <xorg_security>|
|Priority:||high||CC:||ajax, alan.coopersmith, daniel, keithp, kem, mharris, pma|
|i915 platform:||i915 features:|
|Bug Depends on:|
Description Luke Hutchison 2004-05-05 04:12:27 UTC
Open the attached Postscript file in gv. It brings up several errors. gv shows a black content pane, rather than white. Clicking on the black region crashes the X server, with 100% reproducibility.
Comment 1 Luke Hutchison 2004-05-05 04:12:55 UTC
Created attachment 259 [details] The postscript file
Comment 2 Luke Hutchison 2004-05-05 04:13:46 UTC
gv-3.5.8-24 xorg-x11-0.0.6.6-0.0.2004_03_11.11 (FC2-Test2)
Comment 3 Adam Jackson 2004-05-30 12:35:18 UTC
Actually for me it nuked X instantly. Worse yet, it kills Xorg but not Xnest. I'll try to get either a core dump or a protocol trace.
Comment 4 Adam Jackson 2004-05-30 13:25:48 UTC
Verrry interesting. I can't get a protocol trace of the action, because if I run gv with DISPLAY=127.0.0.1:0, it doesn't crash, and there's no way to capture on a unix-domain socket. However, Xnest will not crash, regardless of whether gv connects to it over TCP or UDS. This, combined with the gv display when it doesn't crash (a ~gigapixel black rectangle) leads me to suspect an integer overflow somewhere in the MIT-SHM code. SHM isn't active over TCP, which would explain why I can't get a protocol dump of the crash. Xnest also does not do MIT-SHM (on my machine anyway; it should I would think). Finally, huge integers used as pointer offsets would easily cause a segfault and ensuing instant crash. A fairly wild guess, but that probably needs auditing anyway...
Comment 5 Adam Jackson 2004-05-30 13:29:20 UTC
assigning to me
Comment 6 Adam Jackson 2004-06-17 22:25:30 UTC
luke, what video card are you using? trying to determine if this is driver-dependent or not.
Comment 7 Luke Hutchison 2004-06-18 20:51:50 UTC
(In reply to comment #6) > luke, what video card are you using? trying to determine if this is > driver-dependent or not. I think the machine I reported this on was my work machine, which has an nVidia FX5200 chipset. It was using the nv driver, I think. The other machine it could have been is also an nVidia (TNT2), using the vesa driver (although I'm pretty sure it was the former machine). My laptop has an Intel 810/855 chipset, and it doesn't crash at all (Driver: "i810"). So you might be onto something. Although the laptop has the final FC2 with all the latest updates installed, if that makes a difference -- the machine with the crash was FC2test2.
Comment 8 Adam Jackson 2004-06-19 22:18:23 UTC
yeah, it seems to be build-dependent here too. i can't reproduce it in debrix at all, or on my mach64, but Xorg + tdfx blows right up. what's the package info for Xorg from FC2 final? maybe they've got a patch that addresses it...
Comment 9 Luke Hutchison 2004-06-19 23:02:42 UTC
# rpm -qa | grep xorg xorg-x11-Mesa-libGL-6.7.0-2 xorg-x11-devel-6.7.0-2 xorg-x11-tools-6.7.0-2 xorg-x11-libs-data-6.7.0-2 xorg-x11-libs-6.7.0-2 xorg-x11-Mesa-libGLU-6.7.0-2 xorg-x11-base-fonts-6.7.0-2 xorg-x11-xfs-6.7.0-2 xorg-x11-twm-6.7.0-2 xorg-x11-font-utils-6.7.0-2 xorg-x11-xauth-6.7.0-2 xorg-x11-6.7.0-2 xorg-x11-100dpi-fonts-6.7.0-2 xorg-x11-75dpi-fonts-6.7.0-2 Is this helpful?
Comment 10 Adam Jackson 2004-07-22 19:50:31 UTC
crasher, bumping severity. i've finally got the dlloader build mostly-working, let's see if i can't get a core dump...
Comment 11 Adam Jackson 2004-07-22 22:33:46 UTC
Created attachment 520 [details] no core dump, but got a working backtrace at least... in frame 0 at the point of the crash, src is out of bounds. the width parameter to fbBlt looks completely bogus...
Comment 12 Kevin E. Martin 2004-08-11 16:13:12 UTC
Keith what do you think?
Comment 13 Keith Packard 2004-08-11 16:40:30 UTC
Well, that was fun... Looks like fb needs to range check arguments to CreatePixmap to avoid pixmaps which can't be represented within a Region. I recall fixes like this in other areas of the server in the past; perhaps those never made it back into the sample implementation from various commercial copies? (And people wonder what's wrong wit h the MIT license...)
Comment 14 Kevin E. Martin 2004-08-14 14:12:48 UTC
I have tried to reproduce this with the current CVS head and cannot. It takes over 10 minutes to display, but it works fine. Keith suggested that the problem might be caused by running out of memory as gv allocates a tremendously large pixmap for this postscript file (RSS > 800M).
Comment 15 Kevin E. Martin 2004-08-16 12:02:21 UTC
This was discussed on today's release wranglers call, and we decided that this bug would not block the release unless a problem, other than the memory management or range checking issues mentioned above, can be confirmed. However, we agree that there are issues surrounding the pixmap size and what can be legally represented in a region. These issues are known issues and are being deferred until after the release. However, they do need to be documented for this release. Moving bug over to the bug 999 for documentation.
Comment 16 Søren Sandmann Pedersen 2005-08-23 10:50:50 UTC
Created attachment 3008 [details] A standalone reproducing app The attached program crashes my X server.
Comment 17 Daniel Stone 2005-08-28 03:54:20 UTC
CAN-2005-2495, embargoed until 2005-09-08, 1400 UTC.
Comment 18 Daniel Stone 2005-08-28 03:57:40 UTC
Created attachment 3072 [details] [review] patch against 4.3? I think this patch is against 4.3.x; it's to Søren's mail to xorg_security@.
Comment 19 Egbert Eich 2005-09-07 03:47:30 UTC
+ if (paddedWidth / 4 > 32767 || height > 32767) + return NullPixmap; datasize = height * paddedWidth; I'm trying to understand why we are dividing paddedWidth by 4. A paddedWidth of 32766*4 and a height of 32766 would be legal. datasize is int. 32766 * 32766*4 is 0xfff80010 - which is more than you can fit into an int. Where does the 4 come from? Also I don't see why we check for depth > 4 for ilbm and afb. Shouldn't this be caught elsewhere?
Comment 20 Daniel Stone 2005-09-07 18:33:26 UTC
embargo extended to 2005-08-12 1400 UTC per xorg_security and vendor-sec
Comment 21 Daniel Stone 2005-09-09 18:25:06 UTC
2005-09-12, of course
Comment 22 Daniel Stone 2005-09-12 07:01:43 UTC
embargo over, welcome to full disclosure city.
Comment 23 Daniel Stone 2005-09-12 18:06:15 UTC
Comment on attachment 3072 [details] [review] patch against 4.3? y'know, if we're serious about the whole 6.8.3 thing, this one sounds like a shoo-in
Comment 24 Daniel Stone 2005-09-12 18:13:31 UTC
committed søren's patch to head
Comment 25 Egbert Eich 2005-09-13 03:55:33 UTC
To round this up here is the explanation for the devision by 4 in: + if (paddedWidth / 4 > 32767 || height > 32767) + return NullPixmap; We must meet the condition: paddedWidth * height <= ((1 << (sizeof(size_t) << 3)) - 1) so that we don't cause an overflow in the argument of a subsequent malloc() and end up allocating less than the required amount. size_t should be at least unsigned int therefore the maximum value it can hold would be: 32767^2 * 4. height needs to be <= 32767 as it needs to fit into a short. This condition should have been tested in the calling function - we do it here again for safety.