Bug#396958: /usr/bin/X: several off-by-one errors in debian/local/xserver-wrapper.c

[Jochen Voss <voss@debian.org>]

Package: x11-common
Version: 1:7.1.0-5
Severity: normal
File: /usr/bin/X

Hello,

recently I discovered the following bits of code in the source file
debian/local/xserver-wrapper.c (function 'main', starting at line
172):

    int
    main(int argc, char **argv)
    {
      ...
      char line[1024];
      char var[64];
      char value[256];
      ...

        val = fgets(line, 1024, cf);

        while (val != NULL) {
          var[0] = '\0';
          value[0] = '\0';
          if (sscanf(line, " %64[A-Za-z0-9_] = %256[A-Za-z0-9_ -] ",
                     var, value) > 0) {

This use of 'sscanf' is unsafe and leads to a buffer overflow if
either the key is >=64 characters long or the value is >=256
characters long (the trailing '\0' spills over).  From the GNU libc
manual:

   * Provide a buffer to store it in.  This is the default.  You should
     provide an argument of type `char *' or `wchar_t *' (the latter of
     the `l' modifier is present).

     *Warning:* To make a robust program, you must make sure that the
     input (plus its terminating null) cannot possibly exceed the size
     of the buffer you provide.  In general, the only way to do this is
     to specify a maximum field width one less than the buffer size. ...

Thus the offending line should read

          if (sscanf(line, " %63[A-Za-z0-9_] = %255[A-Za-z0-9_ -] ",
                     var, value) > 0) {

instead.

Some lines later the following bit of code can be found:

  char xserver[1024];
  ...
  i = readlink(X_SERVER_SYMLINK, xserver, 1024);
  ...
  xserver[i] = '\0'; /* readlink() does not null-terminate the string */

Again this is an off-by-one error.  'readlink' will happily return
1024 for long link targets (assuming that the file system allows
this), and the assignment to 'xserver[i]' will overflow the buffer
'xserver' then.

Neither of these issues looks especially exploitable to me but it
might be good to fix them anyway.  (Note that this code is part of an
suid root binary.)

I hope this helps,
Jochen

-- System Information:
Debian Release: 4.0
  APT prefers unstable
  APT policy: (500, 'unstable')
Architecture: i386 (i686)
Shell:  /bin/sh linked to /bin/bash
Kernel: Linux 2.6.18.1
Locale: LANG=en_GB.utf8, LC_CTYPE=en_GB.utf8 (charmap=UTF-8)

Versions of packages x11-common depends on:
ii  debconf [debconf-2.0]         1.5.8      Debian configuration management sy
ii  debianutils                   2.17.3     Miscellaneous utilities specific t
ii  lsb-base                      3.1-18     Linux Standard Base 3.1 init scrip

x11-common recommends no packages.

-- debconf information excluded