Heap overflow in Xorg/XKB in optimized gcc 8.1.1 builds?

Wed Jun 20 17:09:11 UTC 2018

Hi,

My system (Arch Linux x86_64) started crashing at startup after I
updated xorg-server to 1.20.

When I investigated, I found that the cause was likely a heap overflow
which occurs when processing XKB requests (I load my layouts in my
xinitrc). Here is the output from ASAN (after the vndcmds fix I sent
earlier):

==15614==ERROR: AddressSanitizer: heap-buffer-overflow on address 0x60b0000ac7a0 at pc 0x7ffff6e48731 bp 0x7fffffffdad0 sp 0x7fffffffd278
WRITE of size 106 at 0x60b0000ac7a0 thread T0
    #0 0x7ffff6e48730 in __interceptor_memcpy /build/gcc/src/gcc/libsanitizer/sanitizer_common/sanitizer_common_interceptors.inc:737
    #1 0x555555a1c748 in _XkbSetGeometry (/home/vladimir/work/extern/xserver/build/hw/xfree86/Xorg+0x4c8748)
    #2 0x555555a1cc7c in ProcXkbSetGeometry (/home/vladimir/work/extern/xserver/build/hw/xfree86/Xorg+0x4c8c7c)
    #3 0x55555562ccbc in main (/home/vladimir/work/extern/xserver/build/hw/xfree86/Xorg+0xd8cbc)
    #4 0x7ffff6a7006a in __libc_start_main (/usr/lib/libc.so.6+0x2306a)
    #5 0x55555562ff09 in _start (/home/vladimir/work/extern/xserver/build/hw/xfree86/Xorg+0xdbf09)

0x60b0000ac7a0 is located 0 bytes to the right of 112-byte region [0x60b0000ac730,0x60b0000ac7a0)
allocated by thread T0 here:
    #0 0x7ffff6efa219 in __interceptor_realloc /build/gcc/src/gcc/libsanitizer/asan/asan_malloc_linux.cc:105
    #1 0x555555841a4d in _XkbGeomAlloc.lto_priv.621 (/home/vladimir/work/extern/xserver/build/hw/xfree86/Xorg+0x2eda4d)
    #2 0x5555558420dd in SrvXkbAddGeomOverlayRow (/home/vladimir/work/extern/xserver/build/hw/xfree86/Xorg+0x2ee0dd)
    #3 0x555555a1c4c4 in _XkbSetGeometry (/home/vladimir/work/extern/xserver/build/hw/xfree86/Xorg+0x4c84c4)
    #4 0x555555a1cc7c in ProcXkbSetGeometry (/home/vladimir/work/extern/xserver/build/hw/xfree86/Xorg+0x4c8c7c)
    #5 0x55555562ccbc in main (/home/vladimir/work/extern/xserver/build/hw/xfree86/Xorg+0xd8cbc)
    #6 0x7ffff6a7006a in __libc_start_main (/usr/lib/libc.so.6+0x2306a)

SUMMARY: AddressSanitizer: heap-buffer-overflow /build/gcc/src/gcc/libsanitizer/sanitizer_common/sanitizer_common_interceptors.inc:737 in __interceptor_memcpy
Shadow bytes around the buggy address:
  0x0c168000d8a0: fa fa fa fa 00 00 00 00 00 00 00 00 00 00 00 00
  0x0c168000d8b0: 00 fa fa fa fa fa fa fa fa fa 00 00 00 00 00 00
  0x0c168000d8c0: 00 00 00 00 00 00 00 fa fa fa fa fa fa fa fa fa
  0x0c168000d8d0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 fa fa
  0x0c168000d8e0: fa fa fa fa fa fa 00 00 00 00 00 00 00 00 00 00
=>0x0c168000d8f0: 00 00 00 00[fa]fa fa fa fa fa fa fa fa fa fa fa
  0x0c168000d900: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
  0x0c168000d910: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
  0x0c168000d920: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
  0x0c168000d930: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
  0x0c168000d940: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
Shadow byte legend (one shadow byte represents 8 application bytes):
  Addressable:           00
  Partially addressable: 01 02 03 04 05 06 07 
  Heap left redzone:       fa
  Freed heap region:       fd
  Stack left redzone:      f1
  Stack mid redzone:       f2
  Stack right redzone:     f3
  Stack after return:      f5
  Stack use after scope:   f8
  Global redzone:          f9
  Global init order:       f6
  Poisoned by user:        f7
  Container overflow:      fc
  Array cookie:            ac
  Intra object redzone:    bb
  ASan internal:           fe
  Left alloca redzone:     ca
  Right alloca redzone:    cb
==15614==ABORTING

The problem disappears with "--buildtype debug".

Here is the surrounding code leading up to the bad memcpy:

   0x00005555556f4889 <+13497>:	movzwl 0x2(%r12),%eax
   0x00005555556f488f <+13503>:	mov    %r13,%rdi
   0x00005555556f4892 <+13506>:	lea    (%rcx,%rax,8),%r14
   0x00005555556f4896 <+13510>:	callq  0x5555555eab40 <strlen at plt>
   0x00005555556f489b <+13515>:	mov    $0x4,%edx
   0x00005555556f48a0 <+13520>:	mov    %r13,%rsi
   0x00005555556f48a3 <+13523>:	lea    0x4(%r14),%rdi
   0x00005555556f48a7 <+13527>:	cmp    $0x4,%rax
   0x00005555556f48ab <+13531>:	cmovbe %rax,%rdx
   0x00005555556f48af <+13535>:	callq  0x5555555ebaf0 <memcpy at plt>
   0x00005555556f48b4 <+13540>:	mov    %rbx,%rdi
   0x00005555556f48b7 <+13543>:	callq  0x5555555eab40 <strlen at plt>
   0x00005555556f48bc <+13548>:	mov    %rbx,%rsi
   0x00005555556f48bf <+13551>:	mov    %r14,%rdi
   0x00005555556f48c2 <+13554>:	mov    %rax,%rdx
   0x00005555556f48c5 <+13557>:	callq  0x5555555ebaf0 <memcpy at plt>
=> 0x00005555556f48ca <+13562>:	addw   $0x1,0x2(%r12)

The compiler seems to inline _XkbSetGeometry and its callees into one
giant function.

I'm not 100% sure (due to the bug vanishing with lower optimization
levels, making debugging difficult), but it looks like this code comes
from these source lines (xkb/XKBGAlloc.c, function
XkbAddGeomOverlayKey):

    memcpy(key->under.name, under, min(XkbKeyNameLength, strlen(under)));
    memcpy(key->over.name, over, min(XkbKeyNameLength, strlen(over)));
    row->num_keys++;

If I'm right, it looks like the second min() expression is gone from the
generated code. The gcc backtrace seems to confirm this (see memcpy's
size parameter):

#0  0x00007ffff6f04f10 in __asan::ReportGenericError(unsigned long, unsigned long, unsigned long, unsigned long, bool, unsigned long, unsigned int, bool) (pc=140737335559985, bp=bp at entry=140737488346416, sp=sp at entry=140737488344280, addr=addr at entry=106309031216704, is_write=is_write at entry=true, access_size=access_size at entry=106, exp=0, fatal=false)
    at /build/gcc/src/gcc/libsanitizer/asan/asan_report.cc:384
#1  0x00007ffff6e48754 in __interceptor_memcpy(void*, void const*, __sanitizer::uptr) (dst=0x60b0000ac5d8, src=0x629000183a60, size=106) at /build/gcc/src/gcc/libsanitizer/sanitizer_common/sanitizer_common_interceptors.inc:737
#2  0x00005555556f48ca in _XkbSetGeometry ()
#3  0x00005555556ffebd in ProcXkbSetGeometry ()
#4  0x00005555556221c2 in main ()

Disabling inlining on just XkbAddGeomOverlayKey also makes the bug vanish:

diff --git a/xkb/XKBGAlloc.c b/xkb/XKBGAlloc.c
index 8958b0c52..ebc8d5cdf 100644
--- a/xkb/XKBGAlloc.c
+++ b/xkb/XKBGAlloc.c
@@ -788,6 +788,7 @@ XkbAddGeomDoodad(XkbGeometryPtr geom, XkbSectionPtr section, Atom name)
 }
 
 XkbOverlayKeyPtr
+__attribute__ ((noinline))
 XkbAddGeomOverlayKey(XkbOverlayPtr overlay,
                      XkbOverlayRowPtr row, char *over, char *under)
 {

The contents of the bad memcpy's src parameter (when interpreted as a
char*) is:

"I180FK01I163FK02I225FK03I152FK04I148FK05I179FK06I173FK07I172FK08I171FK09I121FK10I122FK11I123FK12I169DELE,\001"

Does that look like something that could occur in the "over" parameter?

So far, everything seems to point at a compiler bug (probably related to
LTO). I can have a go at reducing a test case tomorrow, unless someone
beats me to it. Same with full steps to reproduce, incl. the XKB data.

Also, even though it doesn't look like the problem is with the X
server's source code, there are builds with this bug out there, and it
looks like it might be exploitable? I'm not sure what the procedure is
for such cases. I guess the affected distributions will want to rebuild
the affected packages, though the way they do it might depend on whether
the upstream project decides to patch the code or not.
