Bug 104123

Created attachment 135983 [details]
proposed fix

ComputeLocalClient in os/access.c is buggy, it incorrectly frees a pointer returned by strtok(cmd, ":") assuming it will still point to cmd. It does not if cmd starts with a ":" -- it's advanced to the first non-colon character, causing an invalid-free crash.

This issue can be reproduced by having a program whose name begins with a colon get to the client connection code. For example, by symlinking /usr/bin/vim to ~/bin/:e and executing :e with ~/bin in PATH. This crashes Xorg completely. Some programs cause the crash, some don't, probably depending on whether they try to use xorg. For me, vim, emacs and gedit all crash when called via a symlink beginning with ":".

I had some issues getting a backtrace on my Ubuntu 16.04, but reproduced the issue on a Debian 9 on a different machine:

#0  __GI_raise (sig=sig@entry=6) at ../sysdeps/unix/sysv/linux/raise.c:51
#1  0x00007fd24612719a in __GI_abort () at abort.c:89
#2  0x000055e7136ec042 in OsAbort () at ../../../../os/utils.c:1361
#3  0x000055e7136f6098 in AbortServer () at ../../../../os/log.c:877
#4  0x000055e7136f65b8 in FatalError (f=0x55e71371d5c0 "Caught signal %d (%s). Server aborting\n") at ../../../../os/log.c:1015
#5  0x000055e7136e8168 in OsSigHandler (signo=6, sip=0x7ffd018da570, unused=0x7ffd018da440) at ../../../../os/osinit.c:154
#6  <signal handler called>
#7  __GI_raise (sig=sig@entry=6) at ../sysdeps/unix/sysv/linux/raise.c:51
#8  0x00007fd24612719a in __GI_abort () at abort.c:89
#9  0x00007fd246164310 in __libc_message (do_abort=do_abort@entry=2, fmt=fmt@entry=0x7fd24625eb10 "*** Error in `%s': %s: 0x%s ***\n") at ../sysdeps/posix/libc_fatal.c:175
#10 0x00007fd24616a96e in malloc_printerr (action=3, str=0x7fd24625b667 "free(): invalid pointer", ptr=<optimized out>, ar_ptr=<optimized out>) at malloc.c:5079
#11 0x00007fd24616b1ce in _int_free (av=0x7fd24648fb00 <main_arena>, p=0x55e715b5c261, have_lock=0) at malloc.c:3875
#12 0x000055e7136e0f15 in ComputeLocalClient (client=0x55e715ae14b0) at ../../../../os/access.c:1148
#13 0x000055e7136e448e in AllocNewConnection (trans_conn=0x55e715b40be0, fd=14, conn_time=152416) at ../../../../os/connection.c:739
#14 0x000055e7136e46e1 in EstablishNewConnections (clientUnused=0x0, closure=0x4) at ../../../../os/connection.c:815
#15 0x000055e71369e871 in ProcessWorkQueue () at ../../../../dix/dixutils.c:523
#16 0x000055e7136df713 in WaitForSomething (are_ready=0) at ../../../../os/WaitFor.c:210
#17 0x000055e71368e274 in Dispatch () at ../../../../dix/dispatch.c:422
#18 0x000055e71369d649 in dix_main (argc=11, argv=0x7ffd018db218, envp=0x7ffd018db278) at ../../../../dix/main.c:287
#19 0x000055e71367e088 in main (argc=11, argv=0x7ffd018db218, envp=0x7ffd018db278) at ../../../../dix/stubmain.c:34

A look at os/access.c in git master shows it still has the buggy code. It can be tested even without knowing how to build xorg in the equivalent snippet:
#include <inttypes.h>
#include <stdlib.h>
#include <stdio.h>
#include <string.h>

int main() {
  char *cmd = strdup(":a");
  printf("%p\n", cmd);
  cmd = strtok(cmd, ":");
  printf("%p\n", cmd);
  free(cmd);  // Crash. *** Error in `./a.out': free(): invalid pointer: ...
  return 0;
}

It looks like this problem was introduced in https://lists.freedesktop.org/archives/xorg-devel/2015-December/048259.html aka https://cgit.freedesktop.org/xorg/xserver/commit/?id=e156c0ccb530897d3a428255bd5585f7ea7b9b41. The code should not reuse cmd for the strtok output, so it can free the original strdup'ed string -- see attached patch.