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

[bugzilla-daemon at webkit.org]

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

--- Comment #45 from Patrick Griffis <pgriffis at igalia.com> ---
(In reply to Carlos Garcia Campos from comment #40)
> Comment on attachment 349776 [details]
> > Source/WebKit/UIProcess/Launcher/glib/ProcessLauncherGLib.cpp:72
> > +static int
> > +argsToFd(const Vector<CString>& args, const char *name)
> 
> This should be a single line. The function name is confusing, sounds to me
> like it's converting arguments to file descriptors. What is it doing exactly?

The list of permissions can get quite long and instead of passing 100s of args
to command lines you can pass args as an fd, so it shoves the args in a memfd.

> > Source/WebKit/UIProcess/Launcher/glib/ProcessLauncherGLib.cpp:81
> > +    int memfd = memfd_create(name, MFD_ALLOW_SEALING);
> 
> Should we also pass MFD_CLOEXEC?

We are passing the fd down two layers, bwrap -> dbus proxy, so it doesn't work
here.

> > Source/WebKit/UIProcess/Launcher/glib/ProcessLauncherGLib.cpp:105
> > +class DBusProxy {
> 
> This is too generic name for this class, I guess it doesn't represent a
> generic DBus proxy. What is this for?

It just contains the state to launch a DBus proxy.

> > Source/WebKit/UIProcess/Launcher/glib/ProcessLauncherGLib.cpp:127
> > +    void appendPermissions(std::initializer_list<CString> permissions)
> > +    {
> > +        RELEASE_ASSERT_WITH_SECURITY_IMPLICATION(!m_isRunning);
> > +
> > +        Vector<CString> newPermissions(permissions);
> 
> Why doesn't this receive a Vector<CString> directly?

I just found it cleaner to pass {"initializer", "lists"} around than create vectors.

> > Source/WebKit/UIProcess/Launcher/glib/ProcessLauncherGLib.cpp:141
> > +        m_proxyPath = proxySocket.get();
> 
> We could avoid another copy here if m_proxyPath was a GUniquePtr<char>
> instead of a CString.

Doesn't it then have to make a copy anyway when returning it in `proxyPath()`?

> > Source/WebKit/UIProcess/Launcher/glib/ProcessLauncherGLib.cpp:143
> > +        Vector<CString> bwrapArgs = {
> 
> Why are we using Vector<CString>? that means we are duplicating every string
> (most of them are static or already owned by the class), and then copied
> again in argsToFd.

No reason, do you mean it should be a Vector<char*>?

> > Source/WebKit/UIProcess/Launcher/glib/ProcessLauncherGLib.cpp:193
> > +        if (!g_spawn_async(NULL, argv, NULL, G_SPAWN_LEAVE_DESCRIPTORS_OPEN, NULL, NULL, &m_pid, &error.outPtr())) {
> 
> Use nullptr instead of NULL. Could we use GSubprocess API? since this patch
> is adding new deps, I think it's fine to also make the faue depend on a
> newer glib too. Why do we leave the descriptors open?

Sure we could. Same FD comment as a above, the `bwrap` command passes an fd to its child `xdg-dbus-proxy`.

We could just not sandbox `xdg-dbus-proxy` and clean this up a bit. The extra layer of sandboxing surely
can't hurt but its probably safe and `flatpak` doesn't do it.

> >> 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().
> 
> Why are all the errors fatal in this patch?

Michael asked for them to be. Really they can all be handled but the end process should fail to start.

> > Source/WebKit/UIProcess/Launcher/glib/ProcessLauncherGLib.cpp:260
> > +static void bindIfExists(Vector<CString>& args, const char* path, BindFlags bindFlags = BindFlags::ReadOnly)
> 
> I also find this name confusing, I guess appendBindArguments or something
> like that would be more accurate, no? What should exist? we only check if
> path is nullptr or not
> ah, the if exists is because of the -try suffix?

Yes. `bindIfPathExists()` sound better to you?

> > Source/WebKit/UIProcess/Launcher/glib/ProcessLauncherGLib.cpp:287
> > +        proxy.launch();
> 
> So, this is the one launching the bwrap executable. Should this be
> launchWhateverIfNeeded instead? or ensureWhatever?

`proxy.launchProxy()` ? I'm not sure the duplication is needed.

> > Source/WebKit/UIProcess/Launcher/glib/ProcessLauncherGLib.cpp:413
> > +    static DBusProxy proxy;
> 
> So, the proxy is only for a11y?

There are 3 proxies per UI process. 1 for a11y and 2 for session bus for web and network processes.
(At a later point another for system bus probably.)

> > Source/WebKit/UIProcess/Launcher/glib/ProcessLauncherGLib.cpp:673
> > +    // NOTE: This is not a great solution but we just assume that applications create this directory
> > +    // ahead of time if they require it
> > +    GUniquePtr<char> configDir(g_build_filename(g_get_user_config_dir(), g_get_prgname(), nullptr));
> 
> Shouldn't we add API to allow applications pass their config/data dirs
> instead?

Michael decided not to for now.

> > Source/WebKit/UIProcess/Launcher/glib/ProcessLauncherGLib.cpp:810
> > +    Vector<CString> bwrapArgs = {
> 
> I'm confused again, wasn't this done by the proxy launch method?

We are launching multiple processes.

> > Source/WebKit/UIProcess/Launcher/glib/ProcessLauncherGLib.cpp:858
> > +#endif
> 
> I think all the sandbox code should be moved to its own file

Maybe, what do you think Michael?

> > Source/WebKit/UIProcess/Network/NetworkProcessProxy.cpp:116
> > +#if PLATFORM(GTK) || PLATFORM(WPE)
> > +    WebsiteDataStore& store = processPool().websiteDataStore()->websiteDataStore();
> > +    store.resolveDirectoriesIfNecessary();
> > +    launchOptions.extraSandboxPaths.append(store.resolvedNetworkCacheDirectory());
> > +
> > +    launchOptions.sandboxEnabled = m_processPool.sandboxEnabled();
> > +#endif
> 
> I think we should add a platformGetLaunchOptions or something like that
> instead of adding platform ifdefs here. Should this depend on
> ENABLE(BUBBLEWRAP_SANDBOX) or OS(LINUX) instead of GTK|WPE?

I don't know if `platformGetLaunchOptions()` would be much better. Could change the ifdef though.

> > Source/cmake/WebKitFeatures.cmake:88
> > +    WEBKIT_OPTION_DEFINE(ENABLE_BUBBLEWRAP_SANDBOX "Toggle bubblewrap sandboxing support" PRIVATE OFF)
> 
> Why is this private?

Because it only supports Linux, why show it on non-Linux.

---

I addressed all other comments.

-- 
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/4f15655d/attachment-0001.html>