New paths for Wayland sockets (Re: Enabling Android-style per application user ids)

[Pekka Paalanen]

On Fri, 3 Nov 2017 09:38:46 +0100 (CET)
Jan Engelhardt <jengelh at inai.de> wrote:

> On Friday 2017-11-03 08:33, Pekka Paalanen wrote:
> >> > >
> >> > > wl_display_connect() always attempts to contact a server socket living at
> >> > > ${XDG_RUNTIME_DIR}/${WAYLAND_DISPLAY}.  
> >  
> >> > Modifying the meaning of WAYLAND_DISPLAY environment variable to
> >> > support also absolute paths has been proposed before IIRC. Maybe
> >> > resurrecting that work could be a way forward? Can anyone see a problem
> >> > with that?  
> >> 
> >> This would definitely work, so I don't object if this is the preference of
> >> other reviewers. I would prefer (for the reasons coming below) the
> >> /run/wayland/$WAYLAND_DISPLAY suggestion though.
> >> 
> >> One note about this: this would contain a subtle change in behavior to
> >> existing users of $WAYLAND_DISPLAY. If somebody sets
> >> WAYLAND_DISPLAY="/wayland-0" currently, this works okay. The concatenation
> >> logic in wl_display_connect() results in a string
> >> "/run/user/<uid>//wayland-0", which is valid despite the duplicate '/'. If
> >> $WAYLAND_DISPLAY is now examined for absolute-pathiness, the logic would
> >> probably see "/wayland-0" as an absolute path reference, and the connection
> >> attempt would fail.  
> 
> So, for comparison, the X11 DISPLAY variable is an abstract identifier only.
> libX11 checks that $DISPLAY matches /^:[0-9]+$/ and rejects attempts to use
> e.g. "/../"-kind of injection attacks.

I'm with Carsten here, what attacks are you thinking of?

Wayland very much relies on file system access permissions to limit the
reachability of a socket. There are not and cannot ever be reliable
restrictions implemented in libwayland-client.

> And I would think that the intent of WAYLAND_DISPLAY was very similar in that
> its creators saw it as an identifier rather than a path component. (It
> certainly has that _ring_ to it, given its proximity to X11's "DISPLAY" name.)

I would agree with you, if it actually was an abstract socket, but it
is not. It is a socket file in the file system already.

> As such, WAYLAND_DISPLAY maybe should be hardened to reject any '/',
> and consequently, a new variable ("WAYLAND_SOCKET"?) be introduced that,
> if set, 1. overrides WAYLAND_DISPLAY, 2. decidedly allows path specs
> to the desired effect so as to use /run/wayland/wayland-N.

WAYLAND_SOCKET is already used. If set, it must be set to an open file
descriptor number for a pre-created Wayland connection, and then it
will override everything else once.

I also don't like introducing more environment variables and I haven't
seen a good reason to do that yet.

> Summary of (individual) proposals follows.
> 
> >- modify WAYLAND_DISPLAY to support absolute paths which overrides
> >  any search paths  
> 
>  - introduce new WAYLAND_SOCKET
>  - modify WAYLAND_DISPLAY to reject '/'

What would be the functional difference to WAYLAND_DISPLAY accepting
absolute paths? Why would a different environment variable make a
difference?

> 
> >- when the current socket search fails, add one more place to look
> >  in: /run/wayland/$WAYLAND_DISPLAY with the default "wayland-0" if
> >  WAYLAND_DISPLAY is not set.  
> 
>  - WAYLAND_SOCKET always overrides WAYLAND_DISPLAY
>  - at most, automatic fallback if only if both WAYLAND_SOCKET and
>    WAYLAND_DISPLAY are unset


Thanks,
pq
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 833 bytes
Desc: OpenPGP digital signature
URL: <https://lists.freedesktop.org/archives/wayland-devel/attachments/20171103/9900c6c8/attachment.sig>