Comment 1 Pierre Ossman 2021-11-03 09:43:42 CET

$XAUTHORITY handling seems to be a bit broken in snap. I can find these threads about it:

https://forum.snapcraft.io/t/migrate-x11-authentication-cookies-into-snap-environment/116
https://forum.snapcraft.io/t/x11-forwarding-using-ssh/2381

However they have merged a supposed fix:

https://github.com/snapcore/snapd/pull/3177

It should be in snap 2.25 and newer. And Ubuntu 20.04 supposedly has snap 2.51.1. So something with that new fix isn't working.

Comment 2 Pierre Ossman 2022-06-30 10:39:31 CEST

This has now got worse as it seems like (at least) Firefox is installed as a snap by default on Ubuntu 22.04, and things still don't work:

> cendio@ubuntu2204:~$ firefox
> No protocol specified
> Error: cannot open display: :11

Comment 3 Pierre Ossman 2022-06-30 10:55:59 CEST

I've found the issue. They only apply their fix if the file is in /tmp:

>	// Only do the migration from /tmp since the real /tmp is not visible for snaps
>	if !strings.HasPrefix(fin.Name(), "/tmp/") {
>		return "", nil
>	}

https://github.com/snapcore/snapd/blob/923eec0c1a07cebd9418c4fe87b33901d66065b2/cmd/snap/cmd_run.go#L621

This is to try to avoid a security issue with this system:

https://github.com/snapcore/snapd/pull/3177#discussion_r111426475

Comment 4 Pierre Ossman 2022-06-30 12:40:36 CEST

Reported the issue upstream:

https://bugs.launchpad.net/snapd/+bug/1980344

Comment 5 Pierre Ossman 2023-01-11 09:26:04 CET

https://community.thinlinc.com/t/sound-not-working-under-ubuntu-22-04/529

Comment 6 Alexander Zeijlon 2023-06-01 09:21:10 CEST

After some investigation, we have found that there are two factors that affect being able to start snap applications in a tl-session.
1. The $XAUTHORITY file must be in a *valid* location, which currently seems to be directly in $XDG_RUNTIME_DIR or in some path that has the prefix "/tmp/".
2. The logged-in user must be authorized in the X server.
   - For example, by adding it with the command xhost +si:localuser:<username>.

It seems that only one of the two points above need to be fulfilled for snap applications to work.

Point two is likely the best option since we do not want to either overwrite the default value of $XDG_RUNTIME_DIR, or put the $XAUTHORITY file in /tmp/ since files in this directory are automatically deleted at regular intervals.

Comment 7 Alexander Zeijlon 2023-06-05 11:13:09 CEST

We tested starting a snap application on a local login. We tested on Gnome with X11 and Gnome with Wayland on both Fedora 38 and Ubuntu 22.04.

Independent of combination of distribution and X11/Wayland, there was no difference in behavior when testing the scenarios described below:
1. The Xauthority file default location, with xhost +SI:localuser:<username> set.
> No issues.
2. The Xauthority file default location, with xhost +SI:localuser:<username> NOT set.
> No issues.
3. The Xauthority file other location, with xhost +SI:localuser:<username> set.
> No issues, no matter where the file was placed.
> We moved the Xauthority file to:
> * /run/ (which is visible in snap),
> * /otherdir/ (which is not visible in snap).
4. The Xauthority file other location, with xhost +SI:localuser:<username> NOT set.
> Snaps start when Xauthority is in:
> * /run/ (which is visible in snap)
> Do NOT start when Xauthority is in:
> * /otherdir/ (which is not visible in snap).

Comment 8 Alexander Zeijlon 2023-06-05 11:22:14 CEST

The visibility of files in snap could possibly be explained by the mount points that are defined in sc_populate_mount_ns() in snapd/cmd/snap-confine/mount-support.c in the snap source code. https://github.com/snapcore/snapd/blob/20ebeee0abcf15cb9e22badc9ab8426a5bb975d9/cmd/snap-confine/mount-support.c#L943

We can see that /run/ is one of the mount points, which is not the case for the /otherdir/ folder we created ourselves. We have not confirmed that this is actually the case by testing all the defined mount points, but it seems likely that this is the case.

Comment 9 Alexander Zeijlon 2023-06-05 13:26:34 CEST

Since Gnome Mutter v. 3.33.91, there is a functionality that sets xhost +SI:localuser:<username>, commit [1]. This functionality was removed in v. 42.6, commit [2].

1. https://gitlab.gnome.org/GNOME/mutter/-/commit/eac227a203dba4d45398dfb85ec5b4610b5f3be7
2. https://gitlab.gnome.org/GNOME/mutter/-/commit/b61b0478f7538db27c35dff48f4581a811458116

The removal of the functionality in commit [2] is motivated by the fact that "applications can bypass the X11 permission setting and access the X server through abstract sockets because X11 authentication is not enforced for the current user ID".

However, if this is the case, it would be interesting to know why the distributions themselves still set SI:localuser:<username> through an init-script on login, which is done both by Fedora and Ubuntu when starting Gnome with X11.

This also means that there now is a difference in security level between X11 and Wayland in these distributions. SI:localuser:<username> gets set under X11, but not in Wayland.
This has been reported upstream to Fedora, https://bugzilla.redhat.com/show_bug.cgi?id=2212300.

Comment 10 Alexander Zeijlon 2023-06-07 09:29:49 CEST

We have been trying to look why the scripts to set SI:localuser:<username> were added, but have not found any commits with any substantial information.

We found the commits that added the scripts that are run for a local login with X11 for both Fedora and Ubuntu, but they have minimal documentation and are quite old (from 2006 and 2012 respectively):
https://src.fedoraproject.org/rpms/xorg-x11-xinit/c/6cf01551afb21a2de9a54fb009d910bb7595fa08
https://git.launchpad.net/ubuntu/+source/xorg/commit/debian/local/Xsession.d/35x11-common_xhost-local?id=cce86cade8c1b22b3955aa15e74fa1cedf948127

The Ubuntu commit refers to a Debian upstream bug report https://bugs.debian.org/cgi-bin/bugreport.cgi?bug=586685, we couldn't find any corresponding report in Fedoras bug tracker.

We also investigated if Xorg had any recommendations for when SI:localuser:<username> should be used or not, but we had no luck finding any information on the topic. However, we can still look into why functionality SI:localuser was added, and hopefully there might be some clues in the commit message.

Comment 11 Alexander Zeijlon 2023-06-07 09:36:39 CEST

The reason why snap applications does not work in ThinLinc is because we place the Xauthority file in a place not accessible to snap (/var/opt/thinlinc/sessions/<user>/). Furthermore, since ThinLinc does not count as a local X11 login, there is no script setting SI:localuser:<username> when logging in. A local X11 login uses the local xinit scripts that are specific to a distribution.

Both these factors means that snap applications won't start since they can't get permission to use the Xserver.

While it would be an easy workaround to add a script that runs xhost +SI:localuser:<username>, this does lower the security level, and it's hard to evalute if this is an acceptable change if we don't know why the distributions considered this lower level as good enough.

We could move our session folders from /var/opt/thinlinc/ to eg. /run/thinlinc/ which would be accessible by snap applications. But this is currently outside the scope of this bug for now, since we are close to release right now and do not have time to fully evaluate possible side effects of such a move.

Comment 12 Alexander Zeijlon 2023-06-07 15:02:14 CEST

When investigating the behaviour on Fedora and Ubuntu in comment 7, we found that snap seems to define a set of regular expressions that matches the default paths for Xauthority-file in various display managers [1]. The affected variable is called 'x11ConnectedPlugAppArmor', which suggest that the regexp only has an effect on Ubuntu which runs AppArmor. 

1: https://github.com/snapcore/snapd/blob/master/interfaces/builtin/x11.go#L130

We tested the following scenario on both Fedora 38 and Ubuntu 22.04 in a local login:
 
 1. Move Xauthority file to /run/
 2. Point $XAUTHORITY to the moved Xauthority file
 3. Run 'xhost -si:localuser:<user>' to disable access control for the user 
 4. Start a snap application from the terminal.
   - On Fedora, the application should open without any issues.
   - On Ubuntu, the application does not start and the following is error is shown: 
> update.go:85: cannot change mount namespace according to change mount (/var/lib/snapd/hostfs/usr/share/gimp/2.0/help /usr/share/gimp/2.0/help none bind,ro 0 0): cannot open directory "/var/lib": permission denied
> update.go:85: cannot change mount namespace according to change mount (/var/lib/snapd/hostfs/usr/share/xubuntu-docs /usr/share/xubuntu-docs none bind,ro 0 0): cannot open directory "/var/lib": permission denied
> Authorization required, but no authorization protocol specified
> Error: cannot open display: :0
This could be relevant when deciding on a future placement of our Xauthority files.

Comment 14 Linn 2023-06-08 13:19:04 CEST

Despite not finding any clear information about why SI:localuser:<username> was added from the start, we have decided that it has been in practice for long enough (~15 years) that it has become part of the established norm. Also, the motivation Mutter had for removing SI:localuser:<username> was to avoid Flatpaks getting access to the Xserver through abstract sockets [1], which seems quite much like a corner case.

1: https://gitlab.gnome.org/GNOME/mutter/-/commit/b61b0478f7538db27c35dff48f4581a811458116

Since the Flatpak scenario is an unusual case, and the overall standard is to allow localuser access to the Xserver, we have decided to add a startup script to do this. This means that security levels are adjusted to match the rest of the world, and it will also make snap applications able to open as default.


Part of this bug will also be to assure that the packages needed to set xhost correctly are installed. Package 'xhost' is part of this, and likely also the package that provides the 'id' command

Comment 19 Alexander Zeijlon 2023-06-08 16:21:45 CEST

The xhost-package has been added to the list of required packages for supported distributions, which means that users will be asked if they want to install it when running tl-setup if the binary is not present on the system.

Comment 20 Alexander Zeijlon 2023-06-08 16:27:59 CEST

The id-command, mentioned in comment 14, will not be added as a requirement since it is a part of the coreutils-package that most likely will always be installed on systems that can run Thinlinc.

Comment 22 Linn 2023-06-08 16:48:15 CEST

Checked that snap applications now work in a ThinLinc session by default. Tested on Ubuntu 22.04 with Firefox snap and on Fedora 35 with Chromium snap.

* When running 'xhost', the user has access to the Xserver through SI:localuser:<user>

* Can open snap applications in a ThinLinc sessions

* Can still open non-snap applications in a ThinLinc session. Tested with Libre writer on both Ubuntu and Fedora.

* tl-single-app with a snap application works, before the fix the session closed down with message "Your session lasted less than 10 seconds, ..."

* When setting a snap application as a startup application, it opens without issues when the session starts.

Comment 24 Samuel Mannehed 2023-06-08 17:42:41 CEST

Note that we currently have a platform-specific note for this bug:

https://www.cendio.com/thinlinc/docs/platforms/ubuntu

Comment 25 Alexander Zeijlon 2023-06-09 09:25:37 CEST

Checked that the xhost command gets installed as expected after changes in comment 18.

Test procedure:
1. Configure a system with no X server (and no xhost) installed.
2. Install Thinlinc, select "Yes" when asked to install dependencies.
3. Verify that xhost is installed.

This has been verified in Fedora 38, Ubuntu 22.04, SLES 15. RHEL 9, by installing Thinlinc when there is no graphical environment installed on the system, and hence no xhost since it is a part of Xorg.

Comment 26 Alexander Zeijlon 2023-06-09 09:27:23 CEST

Note, in comment 25, that the package names differ between Fedora and RHEL, since Fedora has moved the xhost command out from xorg-x11-server-utils to its own package.

Our installer is asking for the Fedora package name to be installed in RHEL, where dnf seems to be able to resolve this and install xorg-x11-server-utils. This future proofs our installer for when RHEL eventually catches up with package versions present in Fedora.

Comment 28 Linn 2023-06-09 11:06:45 CEST

The scope of the bug has been extended to not only apply to snap applications, but to all applications that for some reason can't find our Xauthority files. This is more in line with the solution we have made, since the xhost setting applies for all applications, not only snap ones.

Comment 29 Linn 2023-06-09 11:13:17 CEST

The following acceptance criterium has been moved into the separate bug 8167, since we are not following best practices for Xauthority file placement.

> SHOULD:
> * The solution should be robust by following best practices in the area instead
> of just doing a workaround for snaps.
A larger change such as migrating parts of our session folder is outside the scope of this bug.

Comment 30 Linn 2023-06-09 11:18:54 CEST

With that, all acceptance criteria are fulfilled.

MUST:
* Applications without access to our XAuthority should be able to run in Thinlinc sessions, without any additional setup required.
> They do. Tested with snap applications since they had this issue. See comment 22
> for more specific cases tested.
* Applications with access to our XAuthority should still run as before.
> They do, tested opening Libre Writer and it opened both with and without the fix
> (see comment 22 for dists tested)
* If actions are taken that in some way lowers the security level in the X server, this must be thoroughly evaluated and clearly motivated.
> Even if we did not get a clear answer on why SI:localuser:<username> is used,
> it has been the de facto standard to use this setting on well established linux
> distribution for at least 15 years, we consider our background check thorough
> enough to be able to lean on the fact that we can have a security level matching
> the majority.
* If any new dependencies are added, these dependencies should be added when installing ThinLinc
> Dependency xhost is installed if it is not present, see comment 25 for details.

Comment 32 Linn 2023-06-14 13:44:12 CEST

There is one aspect of this change that we didn't cover in the acceptance criteria. Programs that previously were not allowed to use an Xserver instance should still be denied access.

Reopening.

Comment 33 Linn 2023-06-14 14:28:05 CEST

Tested the following acceptance critera:

> * Applications that previously were not allowed to use an X server instance
>   should still be denied access.

Applications that start as another user on the X server are not able to use the X server. We tested this through the following scenario:

1) Run 'xvinfo' inside a ThinLinc session. Note the output of it (no adaptors present).
2) From outside the session, run 'xvinfo -display :<display>', where <display> is the display number of the session. It should say something along the lines of "Unable to open display :<display>".

We can therefore see that only the user of the ThinLinc session has access to its X server. Other users are denied access. To see what it looks like when the user outside the ThinLinc session gets access to X server, add the other user with 'xhost' inside the ThinLinc session.

Changing to resolved.

Comment 34 Tobias 2023-06-15 13:26:15 CEST

General
===============
Tested server builds #3281 (pre-fix) and #3286 (post-fix) on Fedora36, RHEL8, SLES15, and Ubuntu22.04.

Verified that xhost is properly installed in the post-fix build, and missing in the pre-fix build. Note that in RHEL8 for instance, the package xorg-x11-server-utils which contains xhost is installed as a dependency for another package, meaning xhost may be present for some dists even in pre-fix builds.

Furthermore, verified that applications – e.g. Snap – expecting and not finding the Xauthority file in certain specific locations are still able to work with the Xserver in the post-fix build, and conversely confirmed not to be working in the pre-fix build. This was tested (1) from the command line, (2) as a tl-single-app session, and (3) as a startup application. Moreover, applications identifying the proper $XAUTHORITY path work independently of the xhost user status, i.e. their behavior is unchanged.

Finally, reapplied the test case suggested in comment 33, and could confirm in the post-fix build that users lacking specific display credentials are still inhibited as in the pre-fix build. 

Had a look at the corresponding code implementation and it looks good.

Acceptance criteria
===============
✅ Applications without access to our XAuthority should be able to run in Thinlinc sessions, without any additional setup required.

Confirmed – no additional work is required by users or admins.

✅ Applications with access to our XAuthority should still run as before.

Confirmed.

✅ Applications that previously were not allowed to use an X server instance should still be denied access.

Confirmed.

✅ If actions are taken that in some way lowers the security level in the X server, this must be thoroughly evaluated and clearly motivated.

The step to allow localuser Xserver access is sufficiently motivated, referring primarily to the tendency for prominent dists to do just that for more than a decade; see e.g. comment 14. 

✅ If any new dependencies are added, these dependencies should be added when installing ThinLinc

Confirmed – xhost is properly installed.

Conclusion
===============
The acceptance criteria are well-encompassing and are all of them fulfilled, and the code implementation is fine. 

Closing.