Comment 3 for bug 1902250

Revision history for this message
Markus Kuhn (markus-kuhn) wrote :

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.)