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

Tue Sep 18 04:12:41 PDT 2018

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

--- Comment #47 from Carlos Garcia Campos <cgarcia at igalia.com> ---
(In reply to Patrick Griffis from comment #45)
> (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.

so writeArgumentsToMemory() is what the function does, right?

> > > 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.

So, DBusProxyLauncher? I still think of something like GDBusProxy instead. What about XDGDBusProxyLauncher?

> > > 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.

Can't you use { } if the function receives a Vector<>&& ?

> > > 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()`?

No, it would just return a const char* instead of CString, and then the caller decide what to do with it.

> > > 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*>?

Yes, or even char** allocated with g_newa, but we don't need to duplicate all the strings

> > > 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.

I still don't understand what xdg-dbus-proxy is for :-P

> > >> 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?

addBindArgumentIfPathExists()

> > > 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.

I don't know, I don't understand how it works to decide on the API or name.

> > > 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.)

But there can be more than one web process. What happens when a process finishes or crashes? Theses proxies are all static.

> > > 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.

Then, let's not add this for now either.

> > > 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.

So, we have different ways of launching the bwrap thing?

> > > 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.

Yes, it's always better than adding platform ifdefs in cross-platform files.

> > > 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.

Ah, it's public in linux then?

> ---
> 
> I addressed all other comments.

Thanks!

-- 
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/20180918/f8f55127/attachment.html>