run: Preserve X11 display number instead of redirecting it to :99

[smcv]

• run: Preserve X11 display number instead of redirecting it to :99

  Suppose the user's "real" X11 display on the host is Xorg or Xwayland
  listening on :42, but they also have an Xvfb server listening on :99.

  If we change the X11 display number to the arbitrary value :99, and
  the Flatpak sandbox shares its network namespace with the host, then
  clients inside the Flatpak sandbox will prefer to connect to the
  abstract socket @/tmp/.X11-unix/X99 (which is Xvfb), rather than the
  filesystem-backed socket /tmp/.X11-unix/X99 in the sandbox (which is
  really /tmp/.X11-unix/X42 on the host, i.e. Xorg or Xwayland).

  If they're relying on Xauthority (MIT-MAGIC-COOKIE-1) for access
  control (as many display managers do), then this will fail, because
  we gave the sandboxed app access to the cookies for Xorg/Xwayland
  (rewriting their display number from 42 to 99 as we did so), but
  Xvfb does not accept those cookies.

  If we're relying on xhost +"si:localuser:$(id -nu)" for access control
  (as gdm does), then the Flatpak app will successfully (!) connect to
  whatever is on :99, for example Xvfb or Xephyr, which is rarely what
  anyone wants either.

  Resolves: could not connect to display :99.0 #3357

• enter: Don't overwrite the DISPLAY

  Now that we're using the same display number in the sandbox as on the
  host, we can forget about overwriting it with :99.

[smcv]

cc @RyuzakiKK: we should consider applying this change to steam-runtime-tools, for ValveSoftware/steam-runtime#524 (which is basically the same issue as #3357).

[smcv]

In the first ever commit to xdg-app (Flatpak's old name), @alexlarsson wrote:

  /* Bind mount in X socket
   * This is a bit iffy, as Xlib typically uses abstract unix domain sockets
   * to connect to X, but that is not namespaced. We instead set DISPLAY=99
   * and point /tmp/.X11-unix/X99 to the right X socket. Any Xserver listening
   * to global abstract unix domain sockets are still accessible to the app
   * though...
   */

My reasoning is that this cannot be regarded as a meaningful security boundary, therefore it's no problem to make it even more obvious.

One long-term solution would be for X11 servers to stop listening on abstract sockets by default.

Another long-term solution is for everyone to stop using X11 and move towards Wayland, so that access to Xwayland is uninteresting.

[gasinvein]

Excuse me for interjecting with a somehow unrelated question, but can we allow --filesystem=/tmp/.X11-unix once this PR is merged?
Currently, flatpak unconditionally mounts tmpfs over /tmp/.X11-unix, presumably for the same security reasons as remapping the X11 socket. This makes it impossible for a flatpak app to access multiple X servers, what e.g. renders VitualGL unusable within flatpak sandbox.

[smcv]

One long-term solution would be for X11 servers to stop listening on abstract sockets by default.

Unfortunately GNOME tried that, and it broke Snap (https://gitlab.gnome.org/GNOME/mutter/-/issues/1454) and had other fallout (https://gitlab.gnome.org/GNOME/mutter/-/issues/1613).

See also https://gitlab.freedesktop.org/xorg/lib/libxtrans/-/merge_requests/7#note_771760.

[smcv]

Excuse me for interjecting with a somehow unrelated question, but can we allow --filesystem=/tmp/.X11-unix once this PR is merged?

Please send new feature requests to new issues - I'd prefer this to be evaluated on its own merits rather than getting tangled up in a new feature. (But this would be a prerequisite for that new feature.)

[gasinvein]

Excuse me for interjecting with a somehow unrelated question, but can we allow --filesystem=/tmp/.X11-unix once this PR is merged?

Please send new feature requests to new issues - I'd prefer this to be evaluated on its own merits rather than getting tangled up in a new feature. (But this would be a prerequisite for that new feature.)

Done, see the link above.

[Erick555]

One long-term solution would be for X11 servers to stop listening on abstract sockets by default.

Unfortunately GNOME tried that, and it broke Snap (https://gitlab.gnome.org/GNOME/mutter/-/issues/1454)

Snap learned to use non-abstract socket since then.

[RyuzakiKK]

I reviewed this change from the steam-runtime-tools side.

The explanation regarding the security implications makes sense, but it's better to have another look from somebody else to confirm that this is safe, or at least no worst than the current status quo.

[smcv]

If the libX11/libxcb in all Flatpak runtimes supported a syntax like DISPLAY=/tmp/.X11-unix/X99, then we could use that instead, to avoid abstract sockets getting in the way; but it seems they don't (unless compiled with launchd support), so we can't.

[smcv]

If the libX11/libxcb in all Flatpak runtimes supported a syntax like DISPLAY=/tmp/.X11-unix/X99, then we could use that instead, to avoid abstract sockets getting in the way; but it seems they don't (unless compiled with launchd support), so we can't.

If https://gitlab.freedesktop.org/xorg/lib/libxcb/-/merge_requests/30 was merged, we could do this after it got into all runtimes, but that's going to take years (if not decades, given that some unmaintained runtimes are likely still in use).