After an evaluation, GNOME has moved from Bugzilla to GitLab. Learn more about GitLab.
No new issues can be reported in GNOME Bugzilla anymore.
To report an issue in a GNOME project, go to GNOME GitLab.
Do not go to GNOME Gitlab for: Bluefish, Doxygen, GnuCash, GStreamer, java-gnome, LDTP, NetworkManager, Tomboy.
Bug 605350 - GDM causes problems with exmh and tk-send
GDM causes problems with exmh and tk-send
Status: RESOLVED FIXED
Product: gdm
Classification: Core
Component: general
2.29.x
Other Solaris
: Normal normal
: ---
Assigned To: Brian Cameron
GDM maintainers
Depends on:
Blocks:
 
 
Reported: 2009-12-24 01:54 UTC by Brian Cameron
Modified: 2010-07-02 19:06 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
patch fixing problem (3.66 KB, patch)
2010-02-15 23:45 UTC, Brian Cameron
none Details | Review
updated patch (3.20 KB, patch)
2010-03-12 23:44 UTC, Brian Cameron
committed Details | Review

Description Brian Cameron 2009-12-24 01:54:19 UTC
A user reported to me that the exmh mail client does not work with the new GDM.

exmh (written in Tcl which relies on Tk) stopped working. The mailer uses a sub-process and the two processes talk to each other via tk-send which requires X authentication to be configured properly.  The exact error message is:

BgRegister X server insecure (must use xauth-style authorization); command ignored

Running "xhost" in the user session using the new GDM, I see:

access control enabled, only authorized clients can connect
SI:localuser:gdm
SI:localuser:root

Running:

$ xhost -SI:localuser:gdm
$ xhost -SI:localuser:root

Fixes the problem, so it appears that gdm does indeed do this (or something like it) under the covers:

xhost +si:localuser:gdm
xhost +si:localuser:root

GDM should probably remove the gdm/root entries when the user session starts
to avoid this problem.  Does this seem a reasonable way to fix this problem?
Or are these xauth permissions needed after the user session starts for some reason?  I wouldn't think so since GDM doesn't display and GUI after the user authenticates.  Unless there is some Fast-User-Switch need for this or something.
Comment 1 Ray Strode [halfline] 2010-01-13 22:11:53 UTC
relevant irc conversation:

[17:02:18] <yippi> halfline, do you have any thoughts about bug #605350?
[17:06:45] <halfline> first off, i think the BgRegister message is a little bogus
[17:06:56] <halfline> it's assumming all xhost authentication is insecure
[17:06:59] <halfline> which isn't true
[17:07:07] <yippi> yes, the message is a little bogus.
[17:07:23] <halfline> having said that we probably could drop that stuff
[17:07:30] <yippi> but still, it seems GDM should be nicer about leaving xauth in the same state that it used to.
[17:07:37] <halfline> what we probably shouldn't drop is one you didn't list
[17:07:52] <halfline> si:localuser:bcameron
[17:07:57] <yippi> right.  :)
[17:08:06] <halfline> and i don't understand why it's not bitching about that
[17:08:20] <halfline> unless it special cases that one specifically
[17:08:26] <yippi> yes, i believe it does
[17:08:33] <halfline> bizzaro
[17:08:35] <yippi> i think it just checks to make sure that only the user has xauth and nobody else
[17:09:03] <halfline> well not xauth, xhost auth
[17:09:20] <halfline> in some respects xauth is less secure than xhost
[17:09:25] <halfline> since anyone with the auth cookie gets access
[17:09:31] <yippi> rightright
[17:09:41] <halfline> but yea, we should probably fix gdm there
[17:09:48] <halfline> i can't think of a problem off hand
[17:09:55] <yippi> well, i'm a bit unsure how to fix this.  should I add calls to "xhost -SI:localuser:gdm" to the code somewhere
[17:10:09] <halfline> no
[17:10:09] <yippi> or is there a smarter way to do this programatically, for example.
[17:10:12] <halfline> yea
[17:10:17] <halfline> XRmHost or something
[17:10:27] <yippi> if you have any avice about how to o this, it would be cool if you could update the bug report.
[17:10:36] <halfline> XRemoveHos()
[17:10:38] <halfline> Host
[17:10:49] <halfline> sure
[17:10:53] <halfline> i'll just copy and paste our conversaton
[17:10:55] <halfline> one sec
Comment 2 Brian Cameron 2010-02-15 23:45:40 UTC
Created attachment 153881 [details] [review]
patch fixing problem


This patch calls XRemoveHosts in gdm_add_user_authorization so that when the user authorization is added the root and gdm user's xauth permissions are dropped.

Since the calls to XAddHosts and XRemoveHosts are almost identical, I moved the code into a common function which is passed in a boolean to control whether XAddHosts or XRemoveHosts is called.

I have tested and verified that after login, when I run "xhost" it now says:

access control enabled, only authorized clients can connect

So, this returns GDM to the behavior it had in GDM 2.20 and earlier.
Comment 3 Ray Strode [halfline] 2010-02-19 19:30:17 UTC
Remember this part of the conversation:

[17:07:37] <halfline> what we probably shouldn't drop is one you didn't list
[17:07:52] <halfline> si:localuser:bcameron
[17:07:57] <yippi> right.  :)

We probably still want to keep that, it's useful and makes sense authentication isn't tied to hostname (which is bogus since hostname is changed at runtime [which is also bogus, but that's an argument to be had with other parts of the OS])

I like the idea of consolidating ugly code into one function, but I don't quite like the breakdown you came up with.

the function name doesn't really describe what it does.  I think I'd rather a function get_host_entries() or similar which returns the populated host_entries array, and then have the caller call XAddHosts or XRemoveHosts on their own.
Comment 4 Brian Cameron 2010-02-20 20:21:10 UTC
Ray, I think we are seeing different behavior.  On OpenSolaris, I only see the following when I run xhost:

SI:localuser:gdm
SI:localuser:root

I am not sure what is adding the "si:localuser:(user)" to the xhost output on your machine.  Is it this block of code?  I do not see this code adding the xhost value for the user who is logging in.  In fact, I would not think that GDM would even know the user who is logging in at the time that the 
gdm_slave_connect_to_x11_display() function is called.

If you just want to to fix the patch so that there is a function get_host_entries() to build the host_entries value and have the caller call XAddHosts or XRemoveHosts, then I can make that change pretty easily.

However, if additional work is needed to ensure that "si:localuser:(user)" does not get lost, then I will need help to understand what needs to be done.
It may be a bug that "si:localuser:(user)" isn't get set properly on OpenSolaris.
Comment 5 Ray Strode [halfline] 2010-03-08 20:31:05 UTC
oh you're quite right, GDM doesn't add that entry, we do it via a distro-specific shell script at Xsession time.
Comment 6 Brian Cameron 2010-03-12 23:44:38 UTC
Created attachment 156024 [details] [review]
updated patch


In talking with Ray on IRC, he recommended the patch be changed to address the following:

- The function should have a name that better reflects the action.
  Now it is gdm_slave_setup_xauth
- The function should just setup the xauth structures and the calling 
  functions should call XAddHosts or XRemoveHosts instead of passing in a boolean
  to the function and having the function do it.

The updated patch now works this way.  Can this go upstream?
Comment 7 Brian Cameron 2010-04-21 22:13:35 UTC
Fixed in master with a little cleanup recommended by Ray Strode.  For example, he suggested that the function be renamed to gdm_slave_setup_xhost_auth and that the family value of the structure be setup in the function rather than the calling function.