Bug 594 - [fb/afb/cfb/mfb] exploitable overflow in pixmap creation
Summary: [fb/afb/cfb/mfb] exploitable overflow in pixmap creation
Alias: None
Product: xorg
Classification: Unclassified
Component: Server/General (show other bugs)
Version: unspecified
Hardware: x86 (IA32) Linux (All)
: high critical
Assignee: Adam Jackson
QA Contact: X.Org Security
Keywords: security
Depends on:
Blocks: 1690
  Show dependency treegraph
Reported: 2004-05-05 04:12 UTC by Luke Hutchison
Modified: 2005-09-12 10:55 UTC (History)
7 users (show)

See Also:
i915 platform:
i915 features:

The postscript file (12.36 KB, application/postscript)
2004-05-05 04:12 UTC, Luke Hutchison
no flags Details
no core dump, but got a working backtrace at least... (8.30 KB, text/plain)
2004-07-22 22:33 UTC, Adam Jackson
no flags Details
A standalone reproducing app (483 bytes, text/x-csrc)
2005-08-23 10:50 UTC, Søren Sandmann Pedersen
no flags Details
patch against 4.3? (8.90 KB, patch)
2005-08-28 03:57 UTC, Daniel Stone
daniel: 6.8-branch?
Details | Splinter Review

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

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=, 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

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
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.

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.