Numerous security issues in X Window System clients

Posted May 25, 2013 18:11 UTC (Sat) by viro (subscriber, #7872)
In reply to: Numerous security issues in X Window System clients by kevinm
Parent article: Numerous security issues in X Window System clients

... and there you go, injecting reality into a perfectly good outraged handwaving.

In the meanwhile, the "fixes" in question are nearly pointless and seriously broken. Nearly pointless because compromised _server_ doesn't need to bugger the memory of client with overflows; usually it can do it either with ptrace (when on the same box) or via ssh + ptrace (when the client runs on remote box). Seriously broken - hell knows why, presumably it didn't get reviewed (embargo?).

Thursday fun: after grabbing these packages on a debian box, xine started to coredump on startup. After digging through the stack trace, it turned out that something had been calling XextAddDisplay with NULL dpy, with obvious fun results. Trace went through libxext and libxvmc, both included into updated, so the next step had been to compare source before and after. Diff looked like a pile of moderately pointless hardening, which might've caused that (if some test had been too eager and recovery from the misdetected error sufficiently bogus). However, it contained some bits outside of error paths, and those were worth looking at first. Like that one:

strncpy(*name,tmpBuf,rep.nameLen);
+ name[rep.nameLen - 1] = '\0';

Cargo-cult programming? Innocent braino that simply hadn't been caught before it hit the tree? Hell knows, in either case it's obviously bogus (name is char ** according to the older line, which makes the '\0' in the added one an obfuscated NULL) and very likely to cause the symptoms observed - by the look of that code, name is going to be an address of local variable in caller's stack frame, so it's quite plausible that dpy sat in a stack word nearby and got zeroed out by that assignment. Or the one added next to it (s/name/busID/). While we are at it, error cleanup changes right next to that place in diff were obviously not reviewed either -

XFree(*name);
*name = NULL;
XFree(*busID);
*name = NULL;

spells "cut'n'paste lossage". OK, fixed both, rebuilt libxvmc, xine's working again. Next question - was that Debian-specific fuckup or was it in mainline? git clone, git log -p and it turns out to be the latter... Next step was to catch somebody connected to upstream (airlied hadn't ducked in time) and point to the idiocy in question. Total time spent: two hours...

For what it's worth, the interesting (and potentially very nasty) story here is that this demonstrates a damn good way to insert a hole into some tree - pile up a lot of obvious stuff ("hardening" of that sort, etc.) and bury the backdoor in pages upon pages of that. Then claim a bunch of CVEs and feed the fixes to projects in question, demanding an embargo to reduce the amount of people who'll be reviewing that pile.

I do not believe that this is what happened here (IOW, that this fuckup had been malicious), but it's only a matter of time before somebody pulls that off for real; it's a very tempting attack vector. Review is blunted by combination of sheer boredom (lot of trivial stuff to look through), presumed good intent of the patchset (security improvements, no less), possibility to distribute the hole between several libraries and reduction of the available reviewers by embargo. Deniability is also pretty high, even if attacker gets caught afterwards...

to post comments

Numerous security issues in X Window System clients

Posted May 26, 2013 2:41 UTC (Sun) by drag (guest, #31333) [Link] (6 responses)

> ... and there you go, injecting reality into a perfectly good outraged handwaving.

> In the meanwhile, the "fixes" in question are nearly pointless and seriously broken. Nearly pointless because compromised _server_ doesn't need to bugger the memory of client with overflows; usually it can do it either with ptrace (when on the same box) or via ssh + ptrace (when the client runs on remote box). Seriously broken - hell knows why, presumably it didn't get reviewed (embargo?).

What I think you are missing is that it's common to run certain setuid root binaries or run programs via sudo.

I think a large part of that has been mitigated on full-flavored desktops like Gnome, but with the popularity of 'lite'er desktops then a lot of administrative duties is done via sudo and whatnot.

So any sort of flaw in x libraries means local and even remote root exploit.

Numerous security issues in X Window System clients

Posted May 26, 2013 3:36 UTC (Sun) by viro (subscriber, #7872) [Link] (5 responses)

It presupposes a compromised X server on the box you are sitting at. Which, at the absolute minimum, is as bad as attacker physically stumbling across your unattended terminal.

Sure, it might be yourself trying to escalate from limited sudo access to e.g. unrestricted root shell, so it's not absolutely pointless, but it's seriously oversold in the announcement. Note that the breakage introduced (and not caught on review) is 100% real and its impact is not a lot safer than "what if there's no NUL in that string?" crap it fixes.

The real problem here is abuse of embargo for PR purposes. The brainos in question are trivial - everyone had done something like that by accident. Many times. That's what code review is for. And in this case it was prevented by embargo ;-/

Numerous security issues in X Window System clients

Posted May 26, 2013 3:56 UTC (Sun) by viro (subscriber, #7872) [Link]

BTW, now that I've looked at that thing in more details... It's not foolproof - rep.nameLen and rep.busIDLen are u32, and on a 64bit box it's quite possible to have Xmalloc on both succeeding, with sum wrapping around.
At which point
strncpy(*name,tmpBuf,rep.nameLen);
(*name)[rep.nameLen - 1] = '\0';
strncpy(*busID,tmpBuf+rep.nameLen,rep.busIDLen);
(*busID)[rep.busIDLen - 1] = '\0';
will do wonders if rep.nameLen is large (exceeds the size of tmpBuf) *and* reply has NUL within the first few bytes. The first strncpy() succeeds nicely, the second one uses a pointer well beyond tmpBuf end as the source...

The damn thing really looks like it hadn't been reviewed at all.

Numerous security issues in X Window System clients

Posted May 26, 2013 18:41 UTC (Sun) by drag (guest, #31333) [Link] (3 responses)

> It presupposes a compromised X server on the box you are sitting at. Which, at the absolute minimum, is as bad as attacker physically stumbling across your unattended terminal.

There are X servers that do not require setuid root permissions to operate.

% which Xephyr
/usr/bin/Xephyr
% ls -altr `which Xephyr`
-rwxr-xr-x 1 root root 2101072 Apr 17 00:53 /usr/bin/Xephyr
% cp `which Xephyr` ~/
% ~/Xephyr

This works just fine. Besides Xephyr there is a wide variety of other X11 servers that can be copied around easily. One thing that I use quite often is TigerVNC's X11 server for doing remote desktop since for my purposes the VNC protocol is much more responsive and useful then X11 protocol. This sort of thing is quite common to use.

> Sure, it might be yourself trying to escalate from limited sudo access to e.g. unrestricted root shell, so it's not absolutely pointless, but it's seriously oversold in the announcement. Note that the breakage introduced (and not caught on review) is 100% real and its impact is not a lot safer than "what if there's no NUL in that string?" crap it fixes.

It goes to show how woefully people over estimate the security of the Linux desktop if somebody who programs like that is able to trivially find dozens of problems just by being the first one to actually read the code.

Numerous security issues in X Window System clients

Posted May 26, 2013 20:38 UTC (Sun) by viro (subscriber, #7872) [Link] (2 responses)

WTF does that have to do with suid-root? Compromised X server in front of you can bloody well feed arbitrary keystrokes to your shell/browsers/etc. and hide the resulting output from you. Or it can pretend that you've got screensaver kick in and wait for your password, etc. All keyboard/mouse/screen IO goes through it; it can do everything you can do.

Escalation from limited access via /etc/sudoers to full root shell is a different story, but it requires a setup that is probably less common than you think. Again, I certainly agree that one needs to validate the input via any kind of IPC. Something similar to these patches is needed; no arguments here. Severity of impact, OTOH, is quite overblown.

As for reading the source... Judging by the quality of patch, I strongly suspect that this guy hadn't actually done that - grepped for strncpy and friends and did cargo-cult kind of "fixing". Sure, that crap needed fixing, even though the impact is nowhere near as dramatic as you seem to imply (and fixes are obviously not good enough). No problem with that; what I *do* have a problem with is the amount of PR circus in the whole story. Shit happens; let the one who had never fucked up in a patch throw the first stone. Hyping the patch from "here's a bunch of proposed low-hanging fixes for unsafe strncpy() uses, please review and apply" to "28 CVEs!!!1!!! Security holes!!1!!!1!!!!! Embargo!!! (so that it would hit the websites all at once, promoting the hell out of the patch author)", OTOH, is a different story, especially since the embargo has prevented spotting the holes in the patch in question.

Numerous security issues in X Window System clients

Posted May 27, 2013 1:29 UTC (Mon) by drag (guest, #31333) [Link] (1 responses)

> WTF does that have to do with suid-root?

Because you seemed to think that a providing a compromised X Server would be some sort of barrier to the exploit unless the attacker already had root access. Xfree X server requires suid-root to run and I was guessing that is what you were thinking about. I was just pointing out that you don't need those types of permissions to run most X servers.

> As for reading the source... Judging by the quality of patch, I strongly suspect that this guy hadn't actually done that - grepped for strncpy and friends and did cargo-cult kind of "fixing".

Oh. So these vulnerabilities are even more trivial to find then I originally thought. It's all a bit depressing altogether. Another bit of evidence that can be used to show that he Android guys were right to drop all this X/traditional *nix nonsense right from the get-go.

Numerous security issues in X Window System clients

Posted May 27, 2013 2:04 UTC (Mon) by viro (subscriber, #7872) [Link]

> Because you seemed to think that a providing a compromised X Server would
> be some sort of barrier to the exploit unless the attacker already had
> root access. Xfree X server requires suid-root to run and I was guessing
> that is what you were thinking about. I was just pointing out that you
> don't need those types of permissions to run most X servers.

Huh? For one thing, the text above makes no sense whatsoever. For another, just what in the postings upthread might have possibly been interpreted in such a way? Granted, I'm not a native speaker, but...

Numerous security issues in X Window System clients

Posted May 28, 2013 18:17 UTC (Tue) by acoopersmith (subscriber, #72107) [Link] (1 responses)

> Nearly pointless because compromised _server_ doesn't need to bugger the memory of client with overflows;

The point of these is not to protect a user from a compromised server - it's to protect a system from a malicious local unprivileged user compromising a privileged client using their own customized server in order to gain the privileges of that client. For people running Linux on a box in which they're the only user, this is pretty much a non-problem (as long as you're sure no remote attacker could ever break in and then use a bug such as this to break in further), but for sites like schools & businesses using servers with many users, it could be worse.

While the common case of an X server is a privileged process talking directly to the hardware, it's simply a binary protocol - any program that can open a socket and respond in the fashion documented in the X protocol spec can act as an X server if you set your $DISPLAY to point to it. In the process of testing these I was able to corrupt memory in the clients with an “X server” consisting of less than 1000 lines of C code.

> presumably it didn't get reviewed (embargo?).

Yes, the embargo made it difficult to get review for the sheer number of patches involved and you'll note many of the commits missing a “Reviewed-by” tag. We got good coverage of the libX11 & libXi changes, not so much of the rest of the libraries.

Several of us also tested with the libraries on our desktops for a few weeks before release, but I don't think any of us use video players with libXvmc support. (I also missed an XKB bug because I use the default US/English keyboard layout and don't change layouts at runtime - our QA team in Oracle's Prague office found that quite quickly since failing to load a cz layout is much more noticeable to them, and Julien Cristau had developed a fix for Debian at the same time.) We also ran the automated test suites we have, but those mostly cover the libraries from the X11R5/6 timeframe in the mid-90's, not the later extensions, since no one has funded the tedious work of test case writing since then, and few have volunteered much time to work on that.

You can see from the git commit dates we'd been working on these for about 3 months, and finally figured we'd done as much as we could do on the private/embargoed list, so put them out to the world for wider review & testing. I specifically chose not to cut new stable releases of the libraries until this happened, and am glad to see we've gotten better testing and bugs reported via distros such as Debian who put them out to their users quickly.

> this demonstrates a damn good way to insert a hole into some tree - pile up a lot of obvious stuff ("hardening" of that sort, etc.) and bury the backdoor in pages upon pages of that

Yes, code review will never be perfect, and even small changes can introduce security holes - we've found that in X before when a bunch of code reviewers failed to notice a missing pair of parentheses that led to CVE-2006-0745, and only when Coverity pointed them out did we spot the bug and the implications.

Numerous security issues in X Window System clients

Posted May 28, 2013 21:03 UTC (Tue) by viro (subscriber, #7872) [Link]

Well, yes - that's why I said "nearly pointless" and not "entirely pointless". That's the escalation from suid/sudoers-mediated to root shell scenario, but to make it work you need an elevated-priv application attacker can run that would talk to X server. What would that be on common multi-user setups? IOW, the impact is rather limited. These bugs obviously needed fixing (still do, most likely - arithmetical overflow analysis was obviously incomplete), but IMO an embargo had been a mistake in the best case and PR games in the worst (realistic) one.

FWIW, any application that runs with elevated privs would be better served by isolating the part that needs those and trimming it down as far as possible. With protocol for interaction between the priveleged part and the rest being as simple as possible, precisely to make validation more robust... I'm obviously not saying that there's no userland programs that would violate that principle, but any such program is seriously asking for review and rewrite - it's very likely to contain other holes.

PS: "specifically chose not to cut new stable releases of the libraries until this happened" sounds like we are in agreement re the worth of embargo in this case - my impression from the whole thing is that embargo had nothing to do with protecting vulnerable systems and everything with PR theater wanted by the submitter of those patches...