[PATCH 1/3 v4] wayland-server: Add API to control globals visibility

[Pekka Paalanen]

On Tue, 16 Aug 2016 10:05:57 +0200
Olivier Fourdan <ofourdan at redhat.com> wrote:

> Add a new API to let compositor decide whether or not a wl_global
> should be advertised to the clients via wl_registry_bind() or
> display_get_registry()
> 
> By using its own filter, the compositor can decide which wl_global would
> be listed to clients.
> 
> Compositors can use this mechanism to hide their own private interfaces
> that regular clients should not use.
> 
> Signed-off-by: Olivier Fourdan <ofourdan at redhat.com>
> ---
>  v2: Follow-up on Jonas' comments on v1:
>      Add global's data as user data in callback filter function
>      Pass wl_global instead of wl_interface in callback filter function
>      Add wl_global_get_interface() to retrieve the wl_interface from the
>      given wl_global
>      Post an error if a client tries to bind a global which is filtered
>      out.
>  v3: Follow-up on Jonas' comments on v2:
>      Separate from the other wl_global and test additions
>      Add its own user data to the global filter
>      Rephrase the API doc
>      Other few small changes
>  v4: Follow-up on Yong's comments on v3:
>      Rename wl_display_filter_global_func_t as wl_display_global_filter_func_t
>      Document wl_display_global_filter_func_t
>      Rename wl_display_set_filter_global as wl_display_set_global_filter
>      Rename filter_global() as wl_global_is_visible() and add a short
>      explanation in the code
> 
>  src/wayland-server-core.h | 25 ++++++++++++++++++
>  src/wayland-server.c      | 67 +++++++++++++++++++++++++++++++++++++++++++----
>  2 files changed, 87 insertions(+), 5 deletions(-)

Hi,

sorry for taking so long to reply.

A recap from earlier emails as I understood them:

- This patch adds a callback in libwayland-server's wl_registry
  implementation, that will be called every time a) libwayland-server
  is about to send an advert of a global to a client, and b) when a
  client tries to correctly bind a global interface, to determine if
  that should actually happen. Case b) is needed in case a malicious
  client guesses the global name and interface correctly.

- This does not actually have any significant consequences and it does
  not close any "security holes" that would not be closable/closed
  already otherwise. It is purely about trimming down the list of
  globals per client. Essentially it's kind of like an ad hoc
  implementation of namespaces for globals.

- It was said that this is quite different to Giulio's plans for
  privileged/restricted interface negotiation [1].


I do like the simplicity and power of the design: a callback is simple,
yet the API user (compositor) can do whatever it could ever imagine
with it.

We could use this to replace all the existing private interface
permission checks in Weston.

Let's think more about this proposal vs. Giulio's privileged interface
negotiation. I believe the reasons why this cannot be used as a part
for implementing Giulio's proposal is that to do the negotation, you
have to create a wl_registry first to get access to the negotiation
interface. If the negotiation succeeds, then the compositor would
somehow have to send all the global ads that just became possible for
the client. That would be quite awkward to implement with wl_registry
in libwayland-server.

Revoking permissions for global interfaces that have already been bound
to is another case wl_registry cannot easily deal with, even though we
do have wl_registry.remove event that would be just perfect. The
problem is the interface between libwayland-server and the compositor.

As much as I'd like to see a common solution to both features, it
cannot happen with the globals filtering proposed in this patch, I
believe.

However, I think it would be possible to avoid all need for globals
filtering by developing Giulio's proposal. OTOH, it would be more code
and more work for the simple cases where we do not need negotiation
after creating the Wayland connection.

My main worry is that when someone develops Giulio's proposal into a
standard, this feature will become redundant. I have no other reason to
dislike this approach.

Have you looked at [1], what do you think of it?

I have made some negative comments in that thread, but afterwards I
have come around a bit, thinking it might be a good approach the
problems it aims to solve after all.


I'm very happy to see you wrote tests for the new API.

To get proper validation for the new libwayland-server API, I would
like to see it used in Weston to replace all the existing privileged
global checks. To make that fluent, I would like Weston to also use the
recently added "new client created" callback to set up per-wl_client
tracking data, a part of which would be flags telling which privileged
interfaces can be bound or the special role of the client.

As the only serious request for this patch series, I would like the
commit message to mention some more benefits we just figured out with
Jonas in IRC:

- Hiding interfaces that expose compositor implementation details makes
  it harder for clients to identify the compositor. Therefore clients
  are a little less likely to develop compositor-specific workarounds
  instead of reporting problems upstream.

- Hiding can be used to diminish the problems from missing namespacing:
  if two compositors happen to use the same named global with different
  interfaces for their special-purpose clients, the client expecting
  the different interface would probably never see it advertised.

Therefore I think this would be a beneficial addition:
Acked-by: Pekka Paalanen <pekka.paalanen at collabora.co.uk>


Thanks,
pq


[1] https://lists.freedesktop.org/archives/wayland-devel/2015-November/025734.html
continued in
https://lists.freedesktop.org/archives/wayland-devel/2015-December/025884.html
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 811 bytes
Desc: OpenPGP digital signature
URL: <https://lists.freedesktop.org/archives/wayland-devel/attachments/20160930/8c0bfbbc/attachment.sig>