[Webkit-unassigned] [Bug 188568] [GTK][WPE] Implement subprocess sandboxing

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=188568

Adrian Perez <aperez at igalia.com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |aperez at igalia.com

--- Comment #11 from Adrian Perez <aperez at igalia.com> ---
Comment on attachment 347158
  --> https://bugs.webkit.org/attachment.cgi?id=347158
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=347158&action=review

This looks pretty good, thanks! I haven't had the time to apply the patch
locally and test, though. Other than the comment about the WPE API header,
this looks good to me and would like to see it landed if an actual reviewer
rubber-stamps the patch (I am not one yet ^_^).

> Source/WebKit/UIProcess/API/gtk/WebKitWebContext.h:256
> +

You forgot to also copy these two prototypes in “…/API/wpe/WebKitWebContext.h”

(We have the headers duplicated for both ports; one of the reasons was to avoid
the need to clutter them with “#ifdef”s inside them or having to generate code,
and I think there might be some “gtk-doc” related reason as well… Anyway, the
public APIs are not modified often, so it's no big deal for things to be this
way :P)

> Source/WebKit/UIProcess/API/gtk/docs/webkit2gtk-4.0-sections.txt:55
> +webkit_web_context_set_sandbox_enabled

…but we do not yet have the corresponding “…API/wpe/docs/wpewebkit-0.1-sections.txt”
because we still have pending to make possible to run gtk-doc for the WPE port so 
there is no need to worry about this for WPE O:-)

> Source/WebKit/UIProcess/Launcher/glib/ProcessLauncherGLib.cpp:175
> +{

Some systems may not use PulseAudio at all, and in that case the GStreamer
autoplugging machinery will choose ALSA (or even OSS!) if possible. I think
almost nobody is using OSS on GNU/Linux anymore in 2018, so I think at least
we should bind “/dev/snd” inside the sandbox. That would also cover for cases
like people configuring audio capture to use some particular ALSA device
instead of PulseAudio. One example of the latter that I have witnessed is
having a headset for confcalls that has an USB-audio interface but Pulse
would not pick it up somehow.

> Source/WebKit/UIProcess/Launcher/glib/ProcessLauncherGLib.cpp:327
> +        "--dev-bind-try", "/dev/nvidia", "/dev/nvidia",

While this list is a good initial approach that covers (all?) the Open
Source drivers (which are the sane ones, and have their device nodes
under “/dev/dri”), and a couple of extra cases (nice to see the entries
here already for Nvidia users), there are a few many proprietary drivers
which are not covered. For example we have WPE running on Adreno GPUs
that make “/dev/kgsl-3d0” and “/dev/ion”.

But the story does not end there: WPE can (and sometimes will) run without
a windowing system, which on some platforms means there is some WPE backend
implementation which may e.g. use old style framebuffer devices (“/dev/fbN”)
or even some custom device nodes.

I guess there's no better good way of doing this without adding entries
to the whitelist later on as we keep finding more cases of “bad” drivers
which have their device node paths outside of “/dev/dri” :-(

Anyway, I think this can go in like this for now because with the sandbox
being an opt-in it's not like we are going to break anything. I just want
to raise some awareness of this issue because we may want to think if
there's something we can do to improve on this in the future.

-- 
You are receiving this mail because:
You are the assignee for the bug.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.webkit.org/pipermail/webkit-unassigned/attachments/20180817/4547e1b4/attachment.html>