Bug 1083869 - Merge policy in xauth needs to be fixed (was: Starting vncserver changes existing xauthority)
Merge policy in xauth needs to be fixed (was: Starting vncserver changes exis...
Status: RESOLVED FIXED
Classification: openSUSE
Product: openSUSE Tumbleweed
Classification: openSUSE
Component: X.Org
Current
Other Other
: P3 - Medium : Normal (vote)
: ---
Assigned To: Gfx Bugs
Gfx Bugs
https://lists.x.org/archives/xorg/201...
:
Depends on:
Blocks: 1160249
  Show dependency treegraph
 
Reported: 2018-03-04 15:48 UTC by Andrei Borzenkov
Modified: 2022-01-19 11:09 UTC (History)
3 users (show)

See Also:
Found By: ---
Services Priority:
Business Priority:
Blocker: ---
Marketing QA Status: ---
IT Deployment: ---


Attachments

Note You need to log in before you can comment on or make changes to this bug.
Description Andrei Borzenkov 2018-03-04 15:48:02 UTC
After starting vncserver in shell all commands fail to connect to X server due to modified xauth.

bor@10:~> grep CPE_NAME /etc/os-release
CPE_NAME="cpe:/o:opensuse:tumbleweed:20180301"
bor@10:~> echo $DISPLAY
:1
bor@10:~> echo $XAUTHORITY 
/run/user/1000/gdm/Xauthority
bor@10:~> cp -p /run/user/1000/gdm/Xauthority .
bor@10:~> xdpyinfo | head -5
name of display:    :1
version number:    11.0
vendor string:    The X.Org Foundation
vendor release number:    11906000
X.Org version: 1.19.6
bor@10:~> vncserver

Warning: 10:1 is taken because of /tmp/.X11-unix/X1
Remove this file if there is no X server 10:1

New '10:2 (bor)' desktop is 10:2

Starting applications specified in /home/bor/.vnc/xstartup
Log file is /home/bor/.vnc/10:2.log

bor@10:~> xdpyinfo | head -5
No protocol specified
No protocol specified
xdpyinfo:  unable to open display ":1".
bor@10:~> cmp -b Xauthority /run/user/1000/gdm/Xauthority
Xauthority /run/user/1000/gdm/Xauthority differ: byte 8, line 1 is  12 ^J   1 ^A
bor@10:~> XAUTHORITY=Xauthority xdpyinfo | head -5
name of display:    :1
version number:    11.0
vendor string:    The X.Org Foundation
vendor release number:    11906000
X.Org version: 1.19.6
bor@10:~> rpm -q xorg-x11-Xvnc
xorg-x11-Xvnc-1.8.0-8.1.x86_64
bor@10:~> ps -ef | grep Xvnc
bor       2442  1883  0 18:44 pts/0    00:00:00 /usr/bin/Xvnc :2 -auth /run/user/1000/gdm/Xauthority -desktop 10:2 (bor) -fp /usr/share/fonts/misc,/usr/share/fonts/75dpi,/usr/share/fonts/100dpi,/usr/share/fonts/Speedo,/usr/share/fonts/Type1 -geometry 1024x768 -pn -rfbauth /home/bor/.vnc/passwd -rfbport 5902 -rfbwait 30000
bor@10:~>
Comment 1 Michal Srb 2018-03-05 08:46:05 UTC
The vncserver command does change the current Xauthority file, but it should only add records for the new display number, not modify any existing ones.

Can you please add the output of `xauth list` before and after running vncserver? You can delete part of the cookies if you want to keep them secret, but please leave enough to be recognizable which two are the same.
Comment 2 Andrei Borzenkov 2018-03-07 17:20:27 UTC
(In reply to Michal Srb from comment #1)

bor@10:~> xauth list
10/unix:  MIT-MAGIC-COOKIE-1  28df6f0a6055ef7c2328b6cfc12f43c2
#ffff#3130#:  MIT-MAGIC-COOKIE-1  28df6f0a6055ef7c2328b6cfc12f43c2
bor@10:~> vncserver

Warning: 10:1 is taken because of /tmp/.X11-unix/X1
Remove this file if there is no X server 10:1

New '10:2 (bor)' desktop is 10:2

Starting applications specified in /home/bor/.vnc/xstartup
Log file is /home/bor/.vnc/10:2.log

bor@10:~> xauth list
10/unix:2  MIT-MAGIC-COOKIE-1  9f84db0fd6e874540d92ca4fc8ff1194
0.0.0.10:2  MIT-MAGIC-COOKIE-1  9f84db0fd6e874540d92ca4fc8ff1194
bor@10:~>
Comment 3 Michal Srb 2018-03-08 12:36:09 UTC
Ok, I see the problem now. The vncserver command calls xauth to add the two extra entries ("10/unix:2" and "0.0.0.10:2"). The xauth command will merge these new entries with the old ones, removing doubles in the process. Normally it would not matter, because the vncserver entries have display number :2 and the real X server has display number :1 (in this case), so no doubles.

However, for some reason GDM does not include the display number in its xauth entries. So those xauth entries would be used for every display number. During the merge those old entries are considered as being intended for the same display as the new ones and are replaced with them.

I am not sure who is at fault yet, we could fix it by:
* Changing GDM to include the display number.
* Changing the merge policy in xauth.
* Changing vncserver to create its own Xauthority file.

Debugging more..
Comment 4 Fabian Vogt 2018-03-19 08:46:25 UTC
I had some fun with this while I implemented Xauthority support for kwin_wayland as well.

(In reply to Michal Srb from comment #3)
> Ok, I see the problem now. The vncserver command calls xauth to add the two
> extra entries ("10/unix:2" and "0.0.0.10:2"). The xauth command will merge
> these new entries with the old ones, removing doubles in the process.
> Normally it would not matter, because the vncserver entries have display
> number :2 and the real X server has display number :1 (in this case), so no
> doubles.
> 
> However, for some reason GDM does not include the display number in its
> xauth entries. So those xauth entries would be used for every display
> number.

This is valid.

> During the merge those old entries are considered as being intended
> for the same display as the new ones and are replaced with them.

This isn't. xauth must only merge identical entries - treating an empty display number as wildcard in both directions is wrong.

It also appears to replace the FamilyWild (#ffff#...) entry, which is totally wrong as xauth doesn't support it and must ignore it in all cases.

I guess using "xauth generate $DISPLAY" has the same issue.

> I am not sure who is at fault yet, we could fix it by:
> * Changing GDM to include the display number.
> * Changing the merge policy in xauth.

^ This

> * Changing vncserver to create its own Xauthority file.
> 
> Debugging more..
Comment 5 Michal Srb 2018-03-19 08:56:21 UTC
(In reply to Fabian Vogt from comment #4)
> > However, for some reason GDM does not include the display number in its
> > xauth entries. So those xauth entries would be used for every display
> > number.
> 
> This is valid.

Yeah, I got that confirmed from Keith on xorg's mailing list too.

> > * Changing the merge policy in xauth.

I'll work on a fix for xauth.
Additionally xauth will need to sort the numbered entries before the non-numbered to make this work reliably. I think it should be safe to do so, so I'll do that too.
Comment 10 Stefan Dirsch 2020-08-29 09:46:38 UTC
Discussion on xorg ML

https://lists.x.org/archives/xorg/2018-March/059202.html
Comment 12 Stefan Dirsch 2022-01-19 11:09:33 UTC
Wow! Thanks for catching this! This came in after xauth 1.0.10. So it is fixed since release 1.1, which I've updated to a long time ago

-------------------------------------------------------------------
Fri Jul 12 10:41:09 UTC 2019 - Stefan Dirsch <sndirsch@suse.com>

- Update to version 1.1
  * This release fixes a race condition where an existing
    authority file would be unlinked (possibly causing other
    clients to fail to connect), and fixes sorting and merging
    of authority file entries.

Obviously I missed to read the changelog thouroughly. :-( So let's close this one as fixed finally.