News, tips, partners, and perspectives for the Oracle Solaris operating system

Security hole in Xorg 6.9/7.0

Alan Coopersmith
Senior Principal Software Engineer

[CVE-2006-0745] X.Org Security Advisory: privilege escalation and DoS in X11R6.9, X11R7.0

X.Org announced this morning a security hole in the Xorg 6.9 & 7.0 releases that allows a local user to create or overwrite files as root or to run code as root. More details can be found in the X.Org Security Advisory.

This bug affects Solaris users who have installed Xorg 6.9 or 7.0, either on their own or from Sun's releases. Xorg 6.9 is included in Solaris 10 patches 118966-14 and later and in Nevada builds 28 and later, which have been released via the Solaris Express programs.

The fix for Solaris 10 is available in a preliminary T-patch from the SunSolve web site - it's the same we plan to release as the permanent fix, it just hasn't finished the QA cycles required for official release yet. See SunAlert 102252 for details and the links to the patch. The fix for Solaris Express was integrated into Nevada build 36, which should be out via the SX: Community Edition in a couple of weeks.

There's also a simple workaround you can apply now to make it impossible to exploit the bug - remove the setuid bit from the /usr/X11/bin/Xorg binary. X servers on x86 need root access for accessing the video hardware directly - but it only has to be setuid root if you want a non-root user to be able to start the X server directly, such as via the xinit program. Most Solaris installs that use X don't do this, but have a display manager such as gdm or dtlogin start X with a login screen. Since those programs run as root, they can start the X server with the needed privileges without having the Xorg binary be setuid root.

Behind the Hole: The Untold Story of this Bug

A couple of weeks ago, the CTO of Coverity sent mail to the X.Org Developers offering access to the results of a code scan of the X.Org code base by their Coverity Prevent code scanner (which is based on the Stanford Checker project). Their scan of the entire X11R6.9 code base found 1681 potential issues, so about a dozen of us have been working our way through the list, triaging the real bugs from the false alarms, and determining which need to be fixed.

While I was working on this one day, I got tired of looking at yet another memory leak (there are tons in programs like xset, xauth, xhost, etc. - but since the programs only run for less than a second before exiting, how much do you care?), and went to the menu to search by report type to see what other bugs it had found. One of the bug types was "BAD_COMPARE" which I hadn't see yet so I went to look at what it found. Someone had already triaged 3 of these as false alarms and 2 as actual bugs, so I went to look at one of the bugs. It showed (and this is a very cut down version of what the actual report looks like in the browser, displayed in the context of the full source file):

1378 	  /* First the options that are only allowed for root */
Event func_conv: Suspicious implicit conversion to function pointer: "&geteuid != 0"; did you intend to call the function?
1379       if (getuid() == 0 || geteuid != 0)

While I remember looking at the code around here a couple of times during the Xorg 6.9 release cycle, I had never before noticed that the parentheses were missing from the geteuid call. I think my brain simply subconciously autocorrected and inserted the parentheses for me when I read it. Fortunately, the Coverity checker has no subconcious to fool it, and automated attention to detail, so it found what we hadn't seen. Since without the parentheses, the code is simply checking to see if the geteuid function in libc was loaded somewhere other than address 0 (which is pretty much guaranteed to be true), it was reporting it was safe to allow risky options for all users, and thus a security hole was born.

So far that's the only security hole we've found in the Coverity reports - but we're only a little over half way through triaging the reports so far. (Of the 1681 potential issues found, our developers have determined 688 are actual bugs compared to 191 false alarms. Memory leaks are the biggest category, with NULL pointer comparison issues probably second. 63 bugs are already marked as fixed in the coverity reports, and anyone watching the xorg-commit traffic the last couple of weeks has seen a number of those fixes going into CVS for inclusion in the upcoming Xorg 7.1 release.)

P.S. Congratulations to the team at Red Hat and the members of the Fedora community on the release of Fedora Core 5 today, with the Xorg 7.0 modular codebase included. I know having a release-day security advisory isn't how you wanted to celebrate the FC5 launch, but I hope you're finding the new Xorg modular release model is making it much easier to get the fix out for it.

Join the discussion

Comments ( 2 )
  • Dan Price Monday, March 20, 2006
    Ouch, I read the code snippet twice and missed the bug both times. Ouch. Does lint catch these, I wonder?
    if (getuid() == 0 || geteuid != 0)
    return (1);
    return (0);
    $ lint -Nlevel=4 a.c
    $ echo $?

  • ajax Tuesday, March 21, 2006
    Most tools _won't_ catch this, because ANSI C requires that null pointers compare numerically equal to zero, and most compilers silently promote undecorated 0 in comparisons to match the type of what they're being compared against, as otherwise you'd be lost in a sea of warnings.
    And to answer Alan's question, yes, modular X absolutely reduces the pain level of getting this fix out. What used to be 35M of download time per user drops to about 3.2M.
Please enter your name.Please provide a valid email address.Please enter a comment.CAPTCHA challenge response provided was incorrect. Please try again.