snap applications warn about XAUTHORITY path set by LightDM

[Markus Kuhn]

My $HOME resides on an NFS server with "root squash", therefore processes with effective user ID root(0) (and thus any application that I run with sudo) cannot access my ~/.Xauthority file, and as a result sudo or policy-kit rights elevation (e.g. for the gdebi-gtk package installer) fail for X11 GUI applications. As a workaround LightDM offers the option

  user-authority-in-system-dir=true

which I have set with

# echo -e '[LightDM]\nuser-authority-in-system-dir=true' >/etc/lightdm/lightdm.conf.d/50-user-authority-in-system-dir.conf

as described in

  https://bugs.launchpad.net/ubuntu/+source/lightdm/+bug/1648107

As a result LightDM sets up my xauthority file instead at XAUTHORITY=/var/run/lightdm/$USER/xauthority, i.e. in a local file system where euid=0 processes can read it as well.

However, on Ubuntu {16,18,20}.04 /var/run -> /run is a symbolic link. As a result, each time I start a snap application (including non-X11 applications!), I get a warning of the form

# snap install pdftk
$ pdftk
2018/08/24 17:32:48.267771 cmd_run.go:442: WARNING: XAUTHORITY environment value is not a clean path: "/run/lightdm/mgk25/xauthority"

Why does snapd warn about the XAUTHORITY environment variable containing a symbolic link, even though a commonly used option (user-authority-in-system-dir) in a widely used display manager (LightDM) sets it up that way?

What's wrong with symbolic links such as /var/run -> /run from snapd's point of view?

[Markus Kuhn (markus-kuhn)]

See also https://github.com/canonical/lightdm/issues/151

[Paweł Stołowski (stolowski)]

Thank you for the detailed bug report!

This XAUTHORITY check is part of the logic that migrates xauthority file into the sandbox before
starting the snap, and when doing that it performs various checks to prevent typical attack vectors. In this case it falls into:

// Ensure the XAUTHORITY env is not abused by checking that
// it point to exactly the file we just opened (no symlinks,
// no funny "../.." etc)
if fin.Name() != xauthPathCan {
  logger.Noticef("WARNING: XAUTHORITY environment value is not a clean path: %q", xauthPathCan)
...

It apparently fails because /var/run is a symlink to /run while XAUTHORITY is set to /var/run/..., so your diagnosis is correct.

[Markus Kuhn (markus-kuhn)]

Thanks! Looking at func migrateXauthority() in https://github.com/snapcore/snapd/blob/master/cmd/snap/cmd_run.go this appears to be a quite complicated function to essentially do

$ cp $XAUTHORITY $XDG_RUNTIME_DIR/.Xauthority

Is this executed as root?

I see lots of ad-hoc attempts to prevent some kind of time-of-check-to-time-of-use (TOCTTOU) vulnerability. That's not how I would do it. Can't you instead simply

  LockOSThread()
  o = setfsuid(u.Uid)
  ... read $XAUTHORITY file ...
  setfsuid(o)
  UnlockOSThread()

That reads the file with the user's FSUID, such that the kernel checks properly if the user is allowed to read that file. That avoids any TOCTTOU races, would eliminate any need to worry about symlinks, and will probably solve other problems (such as accessing .Xauthority in a root squash $HOME on a Kerberized NFS server). Much simpler and more secure. Never try to do access control checks that the kernel can do for you (lots of people got their fingers burnt this way before).

Also: for LightDM users using user-authority-in-system-dir=true, $XAUTHORITY is already under /run. Is that not sufficient? Does it have to be under $XDG_RUNTIME_DIR?

In case the latter is already the case, shouldn't you check if

  strings.HasPrefix(xauthPathCan, baseTargetDir + "/")

is true, as the X authority file is already where you want it to be?

(I'm not actually a golang programmer, so please treat any of the above only as pseudocode.)

[Markus Kuhn (markus-kuhn)]

P.S.: In the above pseudo-code example, please replace "setfsuid" with "SetfsuidRetUid" from golang.org/x/sys/unix.

P.P.S.: I know that in the Linux C API, setfsuid() is deprecated in favour of seteuid(), as the former non-POSIX syscall has been obsolete since Linux 2.0 (see the notes in "man 2 setfsuid" and "man 2 kill" for full story). However, the Go standard library does not appear to offer an implementation of seteuid. So SetfsuidRetUid still seems the function to call here to temporarily access a file owned by the user with the privileges of the user. An alternative would be to use Setresuid, as in

  LockOSThread()
  o = Geteuid()
  Setresuid(-1, u.Uid, -1)
  ... read file securely with the euid of user, without having to worry about races & symlinks ...
  Setresuid(-1, o, -1)
  UnlockOSThread()

which does the same thing, is more POSIX portable (in case you care about non-Linux kernels), but requires one syscall more.

[Ian Johnson (anonymouse67)]

Hi Markus, please see my comment about your concerns about reading/moving the file as root in this comment: https://bugs.launchpad.net/snappy/+bug/1620771/comments/52.

TLDR for anyone else stumbling on this bug: the migrateXauthority function in cmd_run.go which copies this XAUTHORITY file is run as the user, not as root. The symlink checks may be unnecessarily over-restricting here (and indeed it seems so, as this bug is valid), but we do still follow proper security by trying to do the copy by the calling user and thus the kernel is mediating this copying.