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

[bugzilla-daemon at webkit.org]

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

--- Comment #38 from Michael Catanzaro <mcatanzaro at igalia.com> ---
Comment on attachment 349776
  --> https://bugs.webkit.org/attachment.cgi?id=349776
[GTK][WPE] Implement subprocess sandboxing

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

Excellent. r=me, meaning this has passed my review and is ready to commit without further review after addressing the pending comments. Ordinarily I would set r+ now, but Carlos Garcia actually wanted to review this too, so we're going to require two reviewers on this one (assuming he gets around to it soon).

Please read https://webkit.org/code-style-guidelines/#comments regarding all the comments.

> Source/WebKit/ChangeLog:4
> +        [GTK][WPE] Implement subprocess sandboxing
> +        https://bugs.webkit.org/show_bug.cgi?id=188568

The ChangeLog should also mention the current limitations and plans for resolving them:

 * file:// access broken (which we don't know how to handle)
 * credentials access broken (for immediate follow-up)

> Source/WebKit/UIProcess/API/glib/WebKitWebContext.cpp:1163
> + * If you use `$XDG_CONFIG_HOME/g_get_prgname()` in your #WebKitUserContentManager
> + * you must ensure it exists before subprocess are created.

#WebKitWebsiteDataManager

subprocesses

> Source/WebKit/UIProcess/Launcher/glib/ProcessLauncherGLib.cpp:204
> +    static char* makeProxySocket(const char *appRunDir)

const char* appRunDir

> Source/WebKit/UIProcess/Launcher/glib/ProcessLauncherGLib.cpp:208
> +            return nullptr;

Omit the return, since it's unreachable and surely not needed to avoid warnings. That will just confuse developers about g_error().

> Source/WebKit/UIProcess/Launcher/glib/ProcessLauncherGLib.cpp:216
> +            return nullptr;

Ditto.

> Source/WebKit/UIProcess/Launcher/glib/ProcessLauncherGLib.cpp:536
> +static int setupSeccomp()

TODO (after this lands): ideally these would be split from flatpak into some helper library so that WebKit doesn't have to duplicate them. It's too security-sensitive to duplicate here long-term.

> Source/WebKit/UIProcess/Launcher/glib/ProcessLauncherGLib.cpp:723
> +        // FIXME: The network process is used for `file://` URIs and
> +        // we would have to pass through most paths for this to work

Probably the biggest limitation and behavior change, but we don't currently have a plan to fix this, and don't want to block on it.

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

No blank lines here.

> Source/WebKit/UIProcess/Launcher/glib/ProcessLauncherGLib.cpp:737
> +                "--talk=org.freedesktop.ScreenSaver", // GStreamer inhibits, FIXME: Use xdg-desktop-portal

Actually I already wrote the code for this, it's in SleepDisablerGLib.cpp. Should fall back if org.freedesktop.ScreenSaver is unavailable. Is it not working?

> Source/WebKit/UIProcess/Launcher/glib/ProcessLauncherGLib.cpp:742
> +                // "--talk=org.freedesktop.secrets" // FIXME: WebKitGTK uses for HTTP Auth

Don't leave code commented out. File a new bug report for fixing the credentials access.

> Source/cmake/OptionsGTK.cmake:210
> +if (ENABLE_BUBBLEWRAP_SANDBOX)

I'm surprised that you clearly tested this for WPE (I see you added the /dev/fb devices) but didn't set up the build system for it to work? This all should be split out into another CMake file to be included by both OptionsGTK.cmake and OptionsWPE.cmake. Anyway, it can be done in a future patch, I suppose.

-- 
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/20180917/707b694d/attachment.html>