Skip to content

Access control improvements

gldrk requested to merge gldrk/xserver:whitelist into master

Currently, there are two mechanisms responsible for allowing connections over local transports: 1) the dummy localhost address family and 2) the LocalHostEnabled flag. Their semantics are identical as far as I can tell. Moreover, allowing 1) always implies 2). Otherwise, there is exactly one case in which 2) is true: at startup when the only active transport is local.

$ xinit $(which xhost) -- $(which Xephyr) :2 2>/dev/null
access control enabled, only authorized clients can connect
$ xinit $(which xhost) -- $(which Xephyr) :2 -listen inet 2>/dev/null
access control enabled, only authorized clients can connect
INET:localhost
INET:192.168.0.197
INET6:::1
INET6:fe80::208:caff:fef7:3d23
LOCAL:

Of these only 1) is visible to clients, leading to the following confusing interaction:

$ xhost +SI:localuser:gldrk
localuser:gldrk being added to access control list
$ xhost
access control enabled, only authorized clients can connect
SI:localuser:gldrk
$ sudo xhost
access control enabled, only authorized clients can connect
SI:localuser:gldrk
$ xhost -local:
non-network local connections being removed from access control list
$ xhost
access control enabled, only authorized clients can connect
SI:localuser:gldrk
$ sudo xhost
Authorization required, but no authorization protocol specified

xhost:  unable to open display ":2"

That is, it is impossible to tell from xhost’s output whether all connections over local transports are allowed.

This patch removes 2). It also changes the initialization routine to whitelist only those addresses whose family is in use by the X server. Specifically, DefineSelf is currently defined in a misleading way that hides the fact that this function is idempotent and does not depend on its argument: it makes no sense to call it in a loop like CreateWellKnownSockets does. The solution is to make DefineSelf only allow the requested address family. Compare to the above:

$ xinit $(which xhost) -- $PWD/hw/kdrive/ephyr/Xephyr :2 2>/dev/null
access control enabled, only authorized clients can connect
LOCAL:
$ xinit $(which xhost) -- $PWD/hw/kdrive/ephyr/Xephyr :2 -listen inet 2>/dev/null
access control enabled, only authorized clients can connect
INET:localhost
INET:192.168.0.197
LOCAL:

struct _XtransConnInfo pretends to be an implementation detail, but it is both 1) part of the spec and 2) vastly more useful than libXtrans’s pathetic public interface. There is no shame in relying on it.

Note: the whole concept of a static list of local addresses is fundamentally broken to begin with. In 2024 it’s totally normal for local addresses to become non-local and vice versa at any moment. Obviously it’s way too late to fix this.

Edited by gldrk

Merge request reports