What's an untrusted xauth key? Is this related
to the X security extension? On what operating system?

I think in any case, you need to give us exact details of
what XErrors are occuring (backtraces when the application
is run with the --sync option)

Some things like getting the default theme are pretty
much inevitable ... the security mechanism aren't letting
GTK+ look at the other clients properties. (This possibly
could be fixed by adding code to gnome-settings-daemon
to export the information to "untrusted clients" but
I don't really know if that's possible.)

Hmm, reading
http://davinci01.man.ac.uk/aix433/x11/specs/pdf/security.htm, there
are possible issues with keyboard grabs too.

I've tried it out for myself with ssh 3.8, XFree 4.3, on Linux.

Any window using GTK_WIN_POS_CENTER will cause a problem, since
XQueryPointer on the root window is disallowed (in the above url, it
says that an exception might be warranted for the root window since
emacs needs this too).

Mass changing gtk+ bugs with target milestone of 2.4.2 to target 2.4.4, as
Matthias said he was trying to do himself on IRC and was asking for help with. 
If you see this message, it means I was successful at fixing the borken-ness in
bugzilla :)  Sorry for the spam; just query on this message and delete all
emails you get with this message, since there will probably be a lot.

*** Bug 155053 has been marked as a duplicate of this bug. ***

If we can figure out a way to find out if we are trusted, it might be a good
idea to emit a warning if we are not.

As owen points out, it might be worth to first try and identify all places where
gdk issues requests on other clients resources, and add traps around those.

Well, perhaps the Security Extension spec should be revisited, it's rather dated
and doesn't seem like the most perfect thing (see comment about root window
exception above)

Turns out that things *mostly* actually don't work that badly. 

Problems I ran into with a quick try with gtk-demo:

 - XQueryPointer fails on the root window, causing menus, 
   gdk_window_at_pointer(), etc, etc, to die.

 - The keyboard can't be grabbed, causing problems for menus,
   drag-and-drop, etc. We typically grab pointer and keyboard
   and fail when we can't grab either.

 - GtkEntry seems to have a bug when selection retrieval from
   the clipboard fails and spews:
 
(lt-gtk-demo:28248): Gtk-CRITICAL **: IA__gtk_entry_set_text: assertion `text !=
NULL' failed

 - Starting a failed drag produces:

(lt-gtk-demo:28248): Gtk-CRITICAL **: IA__gtk_drag_set_icon_default: assertion
`GDK_IS_DRAG_CONTEXT (context)' failed

 - XSETTINGS doesn't work. (Not much we can do about that)

The XQueryPointer problem is maybe not that hard to work around - error
trapping the XQueryPointer on the root window is free and it's not
too hard to come up with a "only touching our windows" equivalent.

The keyboard grab is a pain-and-a-half, but one possibility is just to
accept keyboard grab failures ... people won't be able to cancel menus
with Esc.

The other two problems are just fixable bugs.

Created attachment 62433 [details] [review]
gdk-ssh-X.patch

This patch is intended to fix XQueryPointer calls so that gdk doesn't die.

_gdk_windowing_get_pointer(): XQueryPointer dies on the root window. To work around this I create a 1x1 InputOnly window, XQueryPointer on that, and then destroy it. Brain-dead but it works.

_gdk_windowing_window_get_pointer(): XQueryPointer dies if the window isn't ours. I'd be surprised if it was possible to call this with any foreign window other than the root window (since gdk_window_foreign_new() returns NULL when running untrusted), but it's as well to be able to cope. Workaround is to get the pointer coords from _gdk_windowing_get_pointer() and subtract the window origin.

_gdk_windowing_window_at_pointer(): loop through all GDK-aware toplevels on all screens in the hope we hit one containing the pointer; then use that as the basis of the current XQueryPointer child recursion. Special case: we might be over an area of a toplevel without a child (e.g. the background of a GtkDialog); to detect such instances I create a 1x1 InputOnly window under the pointer and test to see if XQueryPointer on the toplevel picks it up (which it will iff that toplevel is the topmost window under the pointer).

Note that none of the added code is hit unless XQueryPointer raises an X error, so (I hope) the normal GDK code paths will be unaffected.

I've tested this patch reasonably thoroughly (the gedit Python Console has been a godsend, thanks Raphael and Adam!); I'll try to answer any queries anyone has.

Also, of course, this doesn't fix any other of the issues Owen raised (notably, context menus don't work). My motivation is to be able to use gvim untrusted without crashing.

Looks good in principle. I wonder if we should create that 1x1 window 
and keep it instead of recreating it all the time. Maybe its not worth it;
what do you think ?

Sorry for the delay.

Keeping the 1x1 window around would reduce roundtripping; if we were going to do that it would make sense to check at GDK initialisation whether we're running untrusted and if so use the workarounds. 

This would also help with pointer grabs (simplest would be to just return TRUE, but we could also provide the information to the client).

Window creation is not a round trip. 

(Yes, really! The trick is that the client allocates the window IDs)

Ooh, sneaky.

Checking whether we're untrusted at startup would still be useful, though: leaving out the gdk_error_trap_{push,pop} will eliminate an XSync. 

For the grab issue, returning TRUE would be evil; how about providing gdk_display_is_pointer_grab_possible() and gdk_display_is_keyboard_grab_possible() for GtkMenu etc. to use?

Uh - the X SECURITY specs don't seem to indicate a way for a client to find out whether its cookie is trusted; we'd need a call guaranteed to succeed if trusted and fail if untrusted, maybe XQueryPointer on root?

Created attachment 64946 [details] [review]
gdk-gtk-trusted.patch

Update to the above patch.

This patch adds a trusted_client boolean member to the GdkDisplayX11 struct, which is set in gdk_display_open() if a XQueryPointer on the root window succeeds without error. As stated above this is believed to be an adequate method to detect whether the client is running trusted.

For pointer detection etc. the same workarounds as in the previous patch are used, but the trusted_client member is used instead of trapping for error. This eliminates an XSync; trusted_client test is wrapped in G_LIKELY to make performance impact minimal.

Pointer grabs: in gdk_pointer_grab(), if the client is untrusted _gdk_input_grab_pointer is allowed to return AlreadyGrabbed and the client will try to grab the main pointer anyway. It appears untrusted clients are allowed to execute a pointer grab on their own windows (with some exceptions), but not XGrabDevice. 

Keyboard grabs: untrusted clients are not allowed keyboard grabs. However, much of the functionality of a keyboard grab is given by Gdk rewriting keyboard events to the client to the grabbing widget, so the keyboard grab is pretended to succeed. (Is this acceptable?)

Menus: the occasions when an untrusted client cannot make a pointer grab are when the window is InputOnly, or is not yet mapped, returning GrabNotViewable. (I can't see where this is specified in the X SECURITY specs; this is observed behaviour with the X.Org server, version 1.0.2). gtk_menu_popup responds to this by using an InputOutput, mapped, offscreen, override-redirect transfer window. As scaffolding, popup_grab_on_window takes a return location argument for the grab status of the failed grab, and menu_grab_transfer_window_get takes a gboolean argument for whether to create an InputOnly or InputOutput window.

Drag-and-drop: again, the IPC widget has to be InputOutput and mapped for a pointer grab to work. gtk_drag_ipc_widget_make_viewable() throws away the GdkWindow of the GtkInvisible IPC widget and replaces it with an InputOutput window. This is used if the pointer grab returns GrabNotViewable. After this, the creation of a window cache fails because of the use of XQueryTree; untrusted clients use gdk_screen_get_toplevel_windows() instead.

With the above patch, gtk-demo works completely (including drag-and-drop); menus work (including context menus called up by right-click and the Menu key); applications don't crash.

Any comments, questions, etc.

Created attachment 65748 [details] [review]
gdk-gtk-trusted.patch

Updated for 2.9.1.

> Keyboard grabs: untrusted clients are not allowed keyboard grabs. However, 
> much of the functionality of a keyboard grab is given by Gdk rewriting 
> keyboard events to the client to the grabbing widget, so the keyboard grab 
> is pretended to succeed. (Is this acceptable?)

This looks slightly odd to me. Can it not lead to problems if code assumes that
after grabbing the keyboard, it gets all key events ? Anyway, it may be the best
we can do. Maybe we should emit a warning when running untrusted that certain
things may not work ?

> gtk_menu_popup responds to
> this by using an InputOutput, mapped, offscreen, override-redirect transfer
> window.

Would it not be simpler to always use an InputOutput window then ?

> again, the IPC widget has to be InputOutput and mapped for a
> pointer grab to work.

Same question as for menus here. Maybe GtkInvisible should simply use an 
InputOutput window ?

> Can it not lead to problems if code assumes that after grabbing the keyboard, 
> it gets all key events ? 

Yes. But what clients perform a keyboard grab without input focus? Window managers? A11y clients? Keyloggers? Not the sort of thing you'd want to run as an untrusted client.

> Maybe we should emit a warning when running untrusted that certain things may
> not work ?

    if (G_UNLIKELY (gdk_error_trap_pop () == BadWindow)) {
      g_warning ("Connection to display %s appears to be untrusted. Pointer grabs and inter-client communication may not work as expected", display_name);
      display_x11->trusted_client = FALSE;
    }

Something like that?

> Would it not be simpler to always use an InputOutput window then ?

Although creating a window does not involve the server, mapping it involves a round trip and uses server resources, so is less efficient. My intention with this patch is to have minimal impact on efficiency in the usual (trusted client) case.

>> Can it not lead to problems if code assumes that after grabbing the keyboard, 
>> it gets all key events ? 
>
>Yes. But what clients perform a keyboard grab without input focus? Window
>managers? A11y clients? Keyloggers? Not the sort of thing you'd want to run as
>an untrusted client.

I guess I was thinking about PointerRoot focus, which is pretty irrelevant.

>> Maybe we should emit a warning when running untrusted that certain things may
>> not work ?
>
>    if (G_UNLIKELY (gdk_error_trap_pop () == BadWindow)) {
>      g_warning ("Connection to display %s appears to be untrusted. Pointer
>grabs and inter-client communication may not work as expected", display_name);
>.      display_x11->trusted_client = FALSE;
>    }
>
>Something like that?

Yes, but mention keyboard grabs as well.

> > Would it not be simpler to always use an InputOutput window then ?
>
>Although creating a window does not involve the server, mapping it involves a
>round trip and uses server resources, so is less efficient. My intention with
>this patch is to have minimal impact on efficiency in the usual (trusted
>client) case.

Fair enough.

Created attachment 65928 [details] [review]
gdk-gtk-trusted.patch

With warning message using gdk_display_get_name (display). Example:

(gtk-demo:28602): Gdk-WARNING **: Connection to display localhost:10.0 appears to be untrusted. Pointer and keyboard grabs and inter-client communication may not work as expected.

I'm not happy with the gtk_drag_ipc_widget_make_viewable() hack; I'm working on that right now.

Oh-kaaay. It seems there's an xorg bug (or at least a misfeature):

https://bugs.freedesktop.org/show_bug.cgi?id=6988

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:

#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	

The root window, natch, is trusted, so our ipc widget (and lots of other invisible widgets used similarly) doesn't get mapped, so pointer grabs fail with GrabNotViewable.

I've put a patch up at the Xorg bug, but even if it gets accepted it'll be years till the fix gets rolled out anywhere near complete coverage. So it would seem a workaround is in order. Idea: in gdk_window_new, if the class is InputOnly and the parent is a root window, change it to InputOutput and squeal. Working on it.

Created attachment 65949 [details] [review]
gdk-gtk-trusted.patch

Changes: when an untrusted client creates a toplevel InputOnly window, gdk_window_new changes it to InputOutput and squeals:

(gtk-demo:2825): Gdk-WARNING **: Coercing GDK_INPUT_ONLY toplevel window to GDK_INPUT_OUTPUT to work around bug in Xorg server

This means the hackish code in gtkmenu.c and gtkdnd.c can be removed entirely.

Unfortunately this introduces another bug (it's always the way...); GtkColorSelection reparents the GdkWindow of a GtkInvisible to one of its own windows. Because the GtkInvisible is mapped first, the GtkInvisible's GdkWindow is created as a child of the root window and so is coerced to an InputOutput window. As a result, the GtkColorSelection has a black square at its origin. (This only happens to untrusted clients, of course.)

The fix I use is to have GtkColorSelection use gtk_widget_set_parent_window before the GtkInvisible is mapped. This requires GtkInvisible to respect the GdkWindow set by gtk_widget_set_parent_window, which in turn requires gtk_widget_get_parent_window to return the window set by gtk_widget_set_parent_window even if the widget does not have a parent. I feel the change to gtk_widget_get_parent_window is an actual bugfix (one expects gtk_widget_get_parent_window to return the value set by gtk_widget_set_parent_window) and the change to GtkInvisible makes sense and is highly unlikely to impact on existing code (the change is that if gtk_widget_set_parent_window is set on the GtkInvisible before it is mapped, the GdkWindow will be created as a child of that window instead of the root window).

Also in GtkColorSelection, the eyedropper is fixed so that if getting the color under the pointer by screenshooting the root window fails, it tries to get the color from its own windows.

Created attachment 65950 [details] [review]
gdk-gtk-trusted.patch

(trivial fix)

would it be easier to just use negative coord in GtkColorSel when reparenting
the invisible ?

      gdk_window_reparent (priv->dropper_grab_widget->window, 
			   GTK_WIDGET (colorsel)->window, -100, -100);

Oh yeah, that works; I tried that first. It just didn't feel like the right solution.

I agree, and the gtk_widget_get_parent_window change indeed looks like a bug fix.

If you don't have any further improvements on the patch, I'd like to get this
committed soon.

Created attachment 66142 [details] [review]
gdk-gtk-trusted.patch

Some more changes, sorry ;)

Fix crashes in: gdk_x11_screen_get_window_manager_name, gdk_screen_get_active_window, gdk_notify_startup_complete, gdk_window_unstick

Details:
* gdk/x11/gdkdisplay-x11.c: bail in gdk_notify_startup_complete if untrusted
* gdk/x11/gdkwindow-x11.c: fold together common code from gdk_x11_window_move_to_current_desktop and gdk_window_unstick, using the former's guard to prevent crash
* gdk/x11/gdkevents-x11.c: place guards in fetch_net_wm_check_window, gdk_x11_screen_get_window_manager_name, gdk_x11_screen_supports_net_wm_hint to prevent errors caused by XGetWindowProperty on root

Also, in gdk/x11/gdkdnd-x11.c, Motif Dnd is impossible for untrusted clients, so don't even try.

I discovered the first of these problems in random testing, then tried to do an audit of the code for potentially fatal X calls. I think I've done an OK job; I don't have anything more to add to the patch.

Fixed in the development version. The fix will be available in the next major release. Thank you for your bug report.


2006-05-25  Matthias Clasen  <mclasen@redhat.com>

	Make GTK+ work as an untrusted X client. (#136571,
	Ed Catmur)