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

[bugzilla-daemon at webkit.org]

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

--- Comment #33 from Patrick Griffis <pgriffis at igalia.com> ---
(In reply to Michael Catanzaro from comment #27)
> > Source/WebKit/UIProcess/Launcher/glib/ProcessLauncherGLib.cpp:92
> > +        GUniquePtr<char> proxySocket(makeProxySocket());
> > +        if (!proxySocket.get())
> > +            return;
> 
> Should we make this a fatal error?
> 
> I imagine WebKit's functionality would be quite degraded if the proxy is not
> working. We might want to just CRASH() here instead?

Kinda sucks but yeah.

> 
> > Source/WebKit/UIProcess/Launcher/glib/ProcessLauncherGLib.cpp:239
> > +#if 0
> > +static void bindDBusSystem(Vector<CString>& args)
> 
> I see you have it disabled... what problems is it causing? Looks like this
> needs to be fixed before the patch can land.

We just don't use any system services atm.

> > Source/WebKit/UIProcess/Launcher/glib/ProcessLauncherGLib.cpp:285
> > +        GUniquePtr<char> dconfConfigDir(g_build_filename(configDir, "dconf", nullptr));
> > +        bindIfExists(args, dconfConfigDir.get(), BindFlags::ReadWrite);
> 
> Hmm, I think you're right that this is too big a hole... please add a FIXME
> here. I don't remember exactly what we discussed previously, but we should
> try to find some way to put dconf into a read-only mode so that our
> subprocesses have access to the settings but can't write them. I assume you
> already tried binding read-only and it didn't work?
> 
> (dconf just gained a maintainer, btw, so we should be able to change it if
> need be.)

Upstream DConf already planned on working on this but I don't think its far
along (no idea if I could help). I didn't actually test making it read-only
but I just imagined that doesn't matter when you can talk to the DBus service.

I briefly discussed this with mclasen and it seems like a portal to access
Gtk Settings would be accepted. I'll let you decide if thats worth working
on. (That also means we would depend on xdg-desktop-portal.)

> > Source/WebKit/UIProcess/Launcher/glib/ProcessLauncherGLib.cpp:459
> > +    // FIXME: There is an env var for this too (and should we allow its dbus access?)
> 
> Yeah we should allow D-Bus access. I think codec installation is broken
> currently anyway, though, so I'm OK with leaving that as a FIXME.
> 

Added permissions. It works fine when tested manually (and back when I tried MiniBrowser) but
not within Epiphany so I think the bug is there.

> 
> > Source/WebKit/UIProcess/Launcher/glib/ProcessLauncherGLib.cpp:684
> > +        // FIXME: Find and add the permissions it needs for this (pacparser?)
> 
> pacrunner

I needed the bus names and possibly to test it.

> > Source/WebKit/UIProcess/Launcher/glib/ProcessLauncherGLib.cpp:767
> > +    if (!g_key_file_load_from_file(infoFile.get(), "/.flatpak-info", G_KEY_FILE_NONE, nullptr)) {
> 
> Oh no! Blocking I/O!
> 
> In some cases, it's just necessary in order for the code to be sane. I don't
> think it's worth changing here.

I believe `/.flatpak-info` is in tmpfs anyway.

> > Source/WebKit/UIProcess/Launcher/glib/ProcessLauncherGLib.cpp:777
> > +static Vector<CString> getFlatpakArgs(ProcessLauncher::ProcessType processType)
> 
> This function deserves a substantial comment to explain what it is doing,
> and that it is not related to any of the previous sandboxing code. Currently
> it only becomes obvious that it's one or the other sandbox if you scroll
> down to line 868 and read the condition there.
> 
> > Source/WebKit/UIProcess/Launcher/glib/ProcessLauncherGLib.cpp:929
> > +    if (m_seccompFd != -1) {
> > +        close(m_seccompFd);
> > +        m_seccompFd = -1;
> > +    }
> 
> Huh, does the fd really need to be kept open this long?

No, fixed. It might be cleaner to refactor all of this to use GSubprocessLauncher and
use g_subprocess_launcher_take_fd() later.


---

I have resolved all other comments in comment #27.

-- 
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/20180903/188c1844/attachment.html>