Migrate X11 authentication cookies into snap environment

morphis 2017-04-03 10:26:40 UTC #1

Hey everyone,

In Cannot run X applications on openSUSE Tumbleweed when using KDE we discussed a problem found in openSUSE where applications packaged as a snap doesn’t start in a KDE environment. Reason for this is that the necessary X11 authentication cookies are stored in a file inside /tmp on the host which isn’t shared with a snap.

Our X11 interface currently allows access to the following possible locations of the file specified in the XAUTHORITY environment variable:

  # .ICEauthority files required for X authentication, per user
  owner @{HOME}/.ICEauthority r,
  # .Xauthority files required for X connections, per user
  owner @{HOME}/.Xauthority r,
  owner /{,var/}run/gdm{,3}/*/database r,
  owner /{,var/}run/lightdm/authority/[0-9]* r,
  owner /{,var/}run/lightdm/*/xauthority r,
  owner /{,var/}run/user/*/gdm/Xauthority r,

If the XAUTHORITY environment variable points to one of these files we don’t have a problem and everything works as it should. However in some environments like KDE on openSUSE we can’t rely on this and don’t have the ability to fix the location of the file XAUTHORITY points to. Therefor we need to find another way to handle this problem more in general than only relying on a path we know and have in our AppArmor profiles.

I talked a bit with @zyga on IRC about possible solutions and what we came up with is the following:

When snap run gets executed, we parse the current XAUTHORITY environment variable and copy the file it points to into XDG_RUNTIME_DIR of the current user, more precisely at /run/user/$UID/snap.$SNAP_NAME/Xauthority. Afterwards we set XAUTHORITY to the new path. The snap application will still need to get access to that file by using the x11 interface, which will get an update to allow access the path too.

A preliminary implementation of this can be found here which I will submit as a PR once we agreed that this is the way forward.

zyga 2017-04-03 10:30:21 UTC #2

This part is somewhat tricky. The XDG_RUNTIME_DIR is actually different (check and see) but we have an open bug where snap-confine should create an snap-run should set that variable (I think it is set already, even if snap-confine does not create that particular directory).

morphis 2017-04-03 10:33:17 UTC #3

This part is somewhat tricky. The XDG_RUNTIME_DIR is actually different (check and see) but we have an open bug where snap-confine should create an snap-run should set that variable (I think it is set already, even if snap-confine does not create that particular directory).

Ok, to be precise: I meant XDG_RUNTIME_DIR outside of the snap environment. As long as we have what it points to available in the snap environment, it shouldn’t matter what the env variable XDG_RUNTIME_DIR is set to inside the snap env, as long as XAUTHORITY is set to an absolute path.

jdstrand 2017-04-03 17:29:22 UTC #4

This path is allowed in the default profile as seen in interfaces/appamor/template.go:
owner /run/user/[0-9]*/snap.@{SNAP_NAME}/** mrwklix,

you later state:

but this is not true due to the above template rule.

I think the general ideas that we only want to allow access to the Xauthority file to snaps that plugs the x11 (or unity7) interface is good, and I also think that copying the file somewhere and setting XAUTHORITY make a lot of sense.

The branch https://github.com/morphis/snapd/commit/a98b898f53d07ce7fc4116c44934482dbb932e29 is actually adds this rule:
owner /run/user/[0-9]*/Xauthority r,

which is what I think we should be using instead of /run/user/[0-9]*/snap.@{SNAP_NAME}/Xauthority.

There is potential problem with the overall approach though in parsing XAUTHORITY to begin with since a malicious user could call ‘XAUTHORITY=/path/to/anything snap run …’ and whatever is in XAUTHORITY gets copied to /run/user/<uid>/Xauthority. We have to be careful here to ensure that, for example, /etc/shadow can’t be copied to /run/user/<uid>/Xauthority (eg, we don’t want to allow an unprivileged user to be able to copy a privileged file into the user’s XDG_RUNTIME_DIR with read permissions of the user, thus invalidating the intended DAC restrictions).

Here is what should be happening, in this order

ensure all of the below is happening when snap run is running as the user and not snapd
look at XAUTHORITY file and when set, create a temporary dir with 0700 permissions, then copy XAUTHORITY, its permissions and its times to this temporary dir
if /run/user/<uid>/Xauthority exists (eg, it’s out of date), compare it with /path/to/temporary/Xauthority. If same, done. Otherwise, proceed to step 4
verify the file is an xauth file. One way to do this would be to run xauth -f /path/to/temporary/Xauthority info and look for Number of entries: as > 0 (but account for translations!)
if is an xauthority file, move (with overwrite) /path/to/temporary/Xauthority to /run/user/<uid>/Xauthority
set XAUTHORITY to /run/user/<uid>/Xauthority in snap env

With the above and adding owner /run/user/[0-9]*/Xauthority r, to the x11 and unity7 abstractions, this should be robust and safe. It doesn’t guard against the user replacing arbitrary xauthority files that were created by the user, but we aren’t trying to protect the user from herself anyway, only ensuring that the /run/user/<uid>/Xauthority is a valid xauth file that the user can read.

niemeyer 2017-04-03 17:58:45 UTC #5

Do we need the temporary file? These are generally tiny, and can be handled in memory. Let’s just enforce them to not be larger than a reasonable size with a read limiter.

jdstrand 2017-04-03 18:34:50 UTC #6

Not strictly, no. The point of this file is to avoid race conditions with the checks on the original file. It would be fine to stat and read the file into memory and atomically write out to disk with the permissions and times. Having the temporary file does mean you can use the existing xauth tool for verification rather than reimplementing the verification bits-- but implementing the verification checks in snap run and not calling out to an external tool has its own advantages.

niemeyer 2017-04-03 18:39:32 UTC #7

We probably don’t need a huge amount of verification at this point. The worst thing that can happen in this case in terms of security is the file being right.

morphis 2017-04-04 05:45:10 UTC #8

@jdstrand @niemeyer Thanks for the comments! Will go and implement things this way.

morphis 2017-04-04 11:31:59 UTC #9

Using xauth isn’t something we can easily do. It’s not part of the core snap. Making this a classic only thing also doesn’t work as we have and will have customers still using X11 also on Ubuntu Core. I could reimplement xauth parsing code in Go for this. What we want to avoid at all cost is having libxauth as a dependency. Any other ideas?

morphis 2017-04-05 10:45:44 UTC #10

@jdstrand I’ve implemented what you suggested now with this commit but also added validation of the Xauthority file with a simple parsing of data inside the Xauthority file to validate it. This misses unit tests etc. and is still a bit rough. Once we have an agreement on how we want to proceed I can finalize this and propose a PR.

morphis 2017-04-11 12:47:32 UTC #11

@jdstrand PR is now available at https://github.com/snapcore/snapd/pull/3177

X11 Forwarding using SSH

bazzet 2017-10-03 09:39:11 UTC #12

Is this the same reason why I cannot X11-forward a snap through ssh?

jdstrand 2017-10-26 20:16:11 UTC #13

No, this topic is about making sure that on distributions that store Xauthority files in /tmp that the Xauthority file is available somewhere. ssh forwarding is a different issue and discussed in X11 Forwarding using SSH (ie, .Xauthority doesn’t exist in /home/user/snap/foo/current/.Xauthority).