Bug 28801 - Missing input sanitation in ProcRenderAddGlyphs triggers SEGV
Missing input sanitation in ProcRenderAddGlyphs triggers SEGV
Status: RESOLVED FIXED
Product: xorg
Classification: Unclassified
Component: Security
7.5 (2009.10)
Other All
: medium normal
Assigned To: X.Org Security
X.Org Security
:
Depends on:
Blocks: xserver-1.9
  Show dependency treegraph
 
Reported: 2010-06-28 13:20 UTC by halfdog
Modified: 2010-08-22 20:00 UTC (History)
0 users

See Also:
i915 platform:
i915 features:


Attachments
0001-render-Bounds-check-for-nglyphs-in-ProcRenderAddGlyp.patch (1005 bytes, patch)
2010-06-28 15:22 UTC, ajax at nwnk dot net
no flags Details | Splinter Review
28801.c (3.19 KB, text/plain)
2010-06-29 13:46 UTC, ajax at nwnk dot net
no flags Details

Note You need to log in before you can comment on or make changes to this bug.
Description halfdog 2010-06-28 13:20:43 UTC
During X server security testing, current lucid xserver was terminated on invalid RenderAddGlyphs request (1:7.5+5ubuntu1)

These 3 packets (auth, CreateGlyphs, RenderAddGlyphs) demonstrates probem on 32 bit system (do not forget to adjust resource base from 0xa00000 to valid value, marked with vvvv)


0000000: 6c00 0b00 0000 0000 0000 0000 9511 0300  l...............
              vvvv                          vvvv
0000010: 0102 a000 2300 0000 9514 1900 0102 a000  ....#...........
0000020: 0120 0000 4b00 0000 0600 0900 ffff 0900  . ..K...........
0000030: 0800 0000 ffff 0000 0000 0000 ffff 0000  ................
0000040: 0000 0000 ffff 0000 0000 0000 ffff ffff  ................
0000050: ec5f 0000 ffff 632f ffeb 0000 ffff 0902  ._....c/........
0000060: ffff 0000 ffff 0000 ffff 0000 ffff 0000  ................
0000070: ffff 0000 ffff 0000 ffff 0000            ............

Problem is in render/render.c ProcRenderAddGlyphs

>>>>> nglyphs is used without checking, gi might be outside of request data buffer
    gids = (CARD32 *) (stuff + 1);
    gi = (xGlyphInfo *) (gids + nglyphs);
    bits = (CARD8 *) (gi + nglyphs);
>>>>> remain is calculated, but not checked before first gi is used
    remain -= (sizeof (CARD32) + sizeof (xGlyphInfo)) * nglyphs;
    for (i = 0; i < nglyphs; i++)
    {
        size_t padded_width;
        glyph_new = &glyphs[i];

>>>>> Large nglyphs will terminate here
        padded_width = PixmapBytePad (gi[i].width,
                                      glyphSet->format->depth);

        if (gi[i].height && padded_width > (UINT32_MAX - sizeof(GlyphRec))/gi[i].height)
            break;

        size = gi[i].height * padded_width;
>>>>> This check can also be circumvented, crash will then occur in HashGlyph
        if (remain < size)
            break;


0: /usr/bin/X (xorg_backtrace+0x3b) [0x80e937b]
1: /usr/bin/X (0x8048000+0x61c7d) [0x80a9c7d]
2: (vdso) (__kernel_rt_sigreturn+0x0) [0x1d5410]
3: /lib/libgcrypt.so.11 (0xb21000+0x4b58f) [0xb6c58f]
4: /lib/libgcrypt.so.11 (0xb21000+0x1b946) [0xb3c946]
5: /lib/libgcrypt.so.11 (gcry_md_write+0x34) [0xb27364]
6: /usr/bin/X (HashGlyph+0xc3) [0x81b9e43]
7: /usr/bin/X (0x8048000+0xd5ac2) [0x811dac2]
8: /usr/bin/X (0x8048000+0xce9d3) [0x81169d3]
9: /usr/bin/X (0x8048000+0x2a477) [0x8072477]
10: /usr/bin/X (0x8048000+0x1ed7a) [0x8066d7a]
11: /lib/tls/i686/cmov/libc.so.6 (__libc_start_main+0xe6) [0x571bd6]
12: /usr/bin/X (0x8048000+0x1e961) [0x8066961]
Segmentation fault at address 0x9490000

Arbitrary code execution not yet evaluated, no POC for that (yet).


Credits to me@halfdog.net
Comment 1 halfdog 2010-06-28 13:26:59 UTC
Sorry, ubuntu versioning info was misleading:

ii  xserver-xorg                      1:7.5+5ubuntu1                                  the X.Org X server
ii  xserver-xorg-core                 2:1.7.6-2ubuntu7.2                              Xorg X server - core server
Comment 2 ajax at nwnk dot net 2010-06-28 15:22:04 UTC
Created attachment 36586 [details] [review]
0001-render-Bounds-check-for-nglyphs-in-ProcRenderAddGlyp.patch

Utterly untested patch that probably works.

I don't think this is an exploit path.  You could conceivably copy arbitrary data into a (freshly allocated) pixmap, but I can't see a way to make this write into arbitrary server memory.  Still, that's a potential information disclosure for XACE kinds of people.

But I could be wrong; a second set of eyes would be appreciated.
Comment 3 ajax at nwnk dot net 2010-06-29 13:46:25 UTC
Created attachment 36613 [details]
28801.c

Sample code to trigger the bug.  The patch in attachment #36586 [details] [review] does appear to prevent the crash (tested against a Xephyr server):

X Error of failed request:  BadLength (poly request too large or internal Xlib length error)
  Major opcode of failed request:  143 (RENDER)
  Minor opcode of failed request:  20 (RenderAddGlyphs)
  Serial number of failed request:  15
  Current serial number in output stream:  16
+++ exited (status 1) +++

But again, anyone who wants to double-check my math to make sure I'm computing the guards correctly, please do.
Comment 4 Julien Cristau 2010-07-13 04:37:54 UTC
Review of attachment 36586 [details] [review]:

Reviewed-by: Julien Cristau <jcristau@debian.org>
Comment 5 Julien Cristau 2010-08-20 09:10:58 UTC
commit 5725849a1b427cd4a72b84e57f211edb35838718
Author: Adam Jackson <ajax@redhat.com>
Date:   Mon Jun 28 18:08:50 2010 -0400

    render: Bounds check for nglyphs in ProcRenderAddGlyphs (#28801)
    
    Signed-off-by: Adam Jackson <ajax@redhat.com>
    Reviewed-by: Julien Cristau <jcristau@debian.org>
    Signed-off-by: Keith Packard <keithp@keithp.com>
Comment 6 Daniel Stone 2010-08-22 20:00:03 UTC
unmasking now that the commit is public.