Bug 28801 - Missing input sanitation in ProcRenderAddGlyphs triggers SEGV
Summary: Missing input sanitation in ProcRenderAddGlyphs triggers SEGV
Status: RESOLVED FIXED
Alias: None
Product: xorg
Classification: Unclassified
Component: Security (show other bugs)
Version: 7.5 (2009.10)
Hardware: Other All
: medium normal
Assignee: X.Org Security
QA Contact: X.Org Security
URL:
Whiteboard:
Keywords:
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, Adam Jackson
no flags Details | Splinter Review
28801.c (3.19 KB, text/plain)
2010-06-29 13:46 UTC, Adam Jackson
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 Adam Jackson 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 Adam Jackson 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.


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.