Bug 6988 - Server refuses to let untrusted client map InputOnly toplevel window
Summary: Server refuses to let untrusted client map InputOnly toplevel window
Status: RESOLVED FIXED
Alias: None
Product: xorg
Classification: Unclassified
Component: Server/General (show other bugs)
Version: 7.0.0
Hardware: x86 (IA32) Linux (All)
: high normal
Assignee: Xorg Project Team
QA Contact:
URL:
Whiteboard:
Keywords:
Depends on:
Blocks:
 
Reported: 2006-05-21 14:31 UTC by Ed Catmur
Modified: 2007-02-15 11:48 UTC (History)
0 users

See Also:
i915 platform:
i915 features:


Attachments
xorg-untrusted-toplevel.patch (390 bytes, patch)
2006-05-21 14:43 UTC, Ed Catmur
no flags Details | Splinter Review
xorg-untrusted-toplevel.patch (401 bytes, patch)
2007-01-27 07:37 UTC, Ed Catmur
no flags Details | Splinter Review
xorg-security-root-inputonly-docs.patch (596 bytes, patch)
2007-02-01 17:28 UTC, Ed Catmur
no flags Details | Splinter Review

Description Ed Catmur 2006-05-21 14:31:20 UTC
If a client is untrusted (in the X SECURITY sense) and tries to create an
InputOnly window with a root window as the parent, the server refuses to map it,
silently returning success:

xorg-server/dix/window.c ll 2718-2724 in CVS head:

#ifdef XCSECURITY
    /*  don't let an untrusted client map a child-of-trusted-window, InputOnly
     *  window; too easy to steal device input
     */
    if ( (client->trustLevel != XSecurityClientTrusted) &&
	 (pWin->drawable.class == InputOnly) &&
	 (wClient(pWin->parent)->trustLevel == XSecurityClientTrusted) )
	 return Success;
#endif	

This means that an untrusted client cannot use a toplevel InputOnly window for
pointer grabs (e.g. XDnd).

I suggest that the test be amended to check that the parent is not a root window:

#ifdef XCSECURITY
    /*  don't let an untrusted client map a child-of-trusted-window, InputOnly
     *  window; too easy to steal device input
     */
    if ( (client->trustLevel != XSecurityClientTrusted) &&
	 (pWin->drawable.class == InputOnly) &&
	 pWin->parent->parent
	 (wClient(pWin->parent)->trustLevel == XSecurityClientTrusted) )
	 return Success;
#endif	

Patch to follow.
Comment 1 Ed Catmur 2006-05-21 14:43:15 UTC
Created attachment 5700 [details] [review]
xorg-untrusted-toplevel.patch

Uh, as above but syntactically correct. Also check pWin->parent; not that it
could be null (root windows are trusted) but it makes the code more obviously
correct.

Against xorg-server-1.0.2 but applies against CVS HEAD.
Comment 2 Ed Catmur 2006-10-22 09:55:50 UTC
5-month ping.
Comment 3 Ed Catmur 2007-01-27 07:37:43 UTC
Created attachment 8516 [details] [review]
xorg-untrusted-toplevel.patch

Updated for 1.2.0.
Comment 4 Eamon Walsh 2007-01-29 08:49:12 UTC
With this patch, untrusted clients will be able to intercept input events going to the root window.  Could be a problem if the user starts typing while not realizing that the root window has the focus.  Since SetInputFocus does nothing when called by untrusted clients, this couldn't be actively exploited, but nonetheless keyboard security now relies on the user being aware of the input focus at all times...

Please provide more details on the usage scenario.  Is this a dnd application that supports dragging onto the root window?  Why is the application untrusted?
Comment 5 Ed Catmur 2007-01-30 10:48:46 UTC
Uh, see http://bugzilla.gnome.org/show_bug.cgi?id=136571#c22 - the pattern of creating a toplevel InputOnly window is used across GTK+ for pointer grabs and similar, in particular for context (popup) menus; it's used for DND as well in some situations (can't remember exactly what).

The application is (would be) untrusted because of being used through a ssh tunnel from an untrusted machine, in the general sense for which XSECURITY was created.

Currently GTK+ works round this with an offscreen InputOutput window, so I don't see that the current restriction offers any security benefit; any scenario where an attacker benefits from an InputOnly window silently gaining focus an offscreen InputOutput window would do just as well.

I'm uncertain how to read the specification on this; the Security Extension Specification[1] says:

> If an InputOnly window owned by an untrusted client has a parent owned by a 
> trusted client, all attempts to map the window will be ignored.

However, the root window is owned by the server, which isn't a client.  A conservative reading of the spec would seem to imply that mapping a child of the root window is OK.

1. http://wwweic.eri.u-tokyo.ac.jp/computer/manual/lx/SGI_Developer/books/X11_SecurExt/sgi_html/pr01.html#id5383493
Comment 6 Eamon Walsh 2007-02-01 15:12:36 UTC
The quoted document is at xorg-docs/specs/Xext/security.tex.  I would update the quoted sentence as part of the patchset.  Sounds reasonable otherwise.
Comment 7 Ed Catmur 2007-02-01 17:28:38 UTC
Created attachment 8566 [details] [review]
xorg-security-root-inputonly-docs.patch

OK, here.

Hmm - you wouldn't know how to get the xorg-docs tarball to actually /build/ the docs, would you?
Comment 8 Ed Catmur 2007-02-01 21:18:38 UTC
Right, current git builds, but only the stuff that's been converted to sgml.  I'll assume someone out there knows how to do the conversion and will get to it eventually.
Comment 9 David Nusinow 2007-02-03 07:28:12 UTC
(In reply to comment #8)
> Right, current git builds, but only the stuff that's been converted to sgml. 
> I'll assume someone out there knows how to do the conversion and will get to it
> eventually.
> 

I did the conversion to sgml. What docs aren't being built that need conversion?
Comment 10 Ed Catmur 2007-02-03 14:31:25 UTC
(In reply to comment #9)
> I did the conversion to sgml. What docs aren't being built that need
> conversion?

In this case, specs/Xext/security.tex - indeed, pretty much everything in specs/Xext.
Comment 11 David Nusinow 2007-02-03 15:24:28 UTC
(In reply to comment #10)
> (In reply to comment #9)
> > I did the conversion to sgml. What docs aren't being built that need
> > conversion?
> 
> In this case, specs/Xext/security.tex - indeed, pretty much everything in
> specs/Xext.

Ok, I've cloned this off as bug #9866. Let's track this specific issue there.
Comment 12 Eamon Walsh 2007-02-15 11:48:57 UTC
Patch applied to master.


Use of freedesktop.org services, including Bugzilla, is subject to our Code of Conduct. How we collect and use information is described in our Privacy Policy.