[Webkit-unassigned] [Bug 188568] [GTK][WPE] Implement subprocess sandboxing
bugzilla-daemon at webkit.org
bugzilla-daemon at webkit.org
Sat Sep 1 18:37:00 PDT 2018
https://bugs.webkit.org/show_bug.cgi?id=188568
--- Comment #27 from Michael Catanzaro <mcatanzaro at igalia.com> ---
Comment on attachment 348659
--> https://bugs.webkit.org/attachment.cgi?id=348659
[GTK][WPE] Implement subprocess sandboxing
View in context: https://bugs.webkit.org/attachment.cgi?id=348659&action=review
Excellent! We are quite close to being able to land this. I'll probably give r+ after you address the next round of feedback.
I finally switched from Ephy Tech Preview to a local JHBuild build with this patch. So far, so good. The only problem I've noticed so far is that YouTube is broken like you mentioned, but like you said, that's not caused by this patch, so not your problem.
I tested this for about two days so far and was almost concerned that I haven't noticed anything broken yet. In particular, I was confused that downloads seem to be working fine. Eventually I realized it's because I need to change Epiphany to enable the sandbox. :P So I should really turn it on there and keep testing.
Make a note: once this lands, all the new environment variables need to be documented on https://trac.webkit.org/wiki/EnvironmentVariables. I'll have to handle that since only committers are able to edit the wiki.
Also important: we have an owners policy for Source/WebKit: changes to cross-platform files outside platform #ifdefs need to be approved by one of the owners from Apple. So we'll need to remember, as the very final step before landing, to get approval for your additions to WebsiteDataStore.[cpp,h]. (All the rest of the changes are platform-specific and can be approved by me.)
> Source/WebCore/ChangeLog:8
> + [GTK][WPE] Implement subprocess sandboxing
Remove this line (title belongs only at the top)
> Source/WebCore/ChangeLog:19
> + DBus, Pipewire, etc become more sandboxed and available.
Well you have a restrictive D-Bus proxy now, right? So I would update this commit message to mention the actual weaknesses that are known to exist:
* PulseAudio is allowed and that's very bad
* GPU access is allowed
* Web process still has network access
Anything else that should be mentioned?
Normally I wouldn't be so strict about commit messages, but this is an important commit and I suspect people will be looking back at it for a long time to come.
Actually, I would move these details to the Source/WebKit ChangeLog, and only mention your WebCore changes here.
> Source/WebKit/ChangeLog:8
> + [GTK][WPE] Implement subprocess sandboxing
Ditto regarding the duplicated title
> Source/WebKit/UIProcess/API/glib/WebKitWebContext.cpp:1155
> + * Set whether WebKit subprocesses will be sandboxed limiting access to the system.
comma after "sandboxed"
> Source/WebKit/UIProcess/API/glib/WebKitWebContext.cpp:1157
> + * This cannot be changed after processes are launched and is only implemented on Linux.
Let's copy the documentation from webkit_web_context_set_process_model():
* This method **must be called before any web process has been created**,
* as early as possible in your application. Calling it later will make
* your application crash.
And make sure the crash actually happens (with g_error() to leave a message, or CRASH() if you're feeling less-generous), because calling this function too late indicates a serious security mistake.
> Source/WebKit/UIProcess/API/glib/WebKitWebContext.cpp:1159
> + * Since: 2.22
2.24
> Source/WebKit/UIProcess/API/glib/WebKitWebContext.cpp:1176
> + * Since: 2.22
2.24
> Source/WebKit/UIProcess/Launcher/glib/ProcessLauncherGLib.cpp:65
> + DBusProxy() { };
Better:
DBusProxy() = default;
Best: just remove it. You don't need to declare it manually, right?
> Source/WebKit/UIProcess/Launcher/glib/ProcessLauncherGLib.cpp:73
> + bool isRunning() { return m_isRunning; };
bool isRunning() const { return m_isRunning; };
> Source/WebKit/UIProcess/Launcher/glib/ProcessLauncherGLib.cpp:75
> + CString proxyPath() { return m_proxyPath; };
> + CString path() { return m_path; };
These should both be const too:
CString proxyPath() const { return m_proxyPath; };
CString path() const { return m_path; };
Basically: every function that doesn't modify the object should be const so that it can be used from other const member functions, or on a const object, or via a const pointer to the object.
> Source/WebKit/UIProcess/Launcher/glib/ProcessLauncherGLib.cpp:77
> + void appendPermissions(std::initializer_list<CString> permissions)
Nice!
> Source/WebKit/UIProcess/Launcher/glib/ProcessLauncherGLib.cpp:79
> + ASSERT(!m_isRunning);
I would use ASSERT_WITH_SECURITY_IMPLICATION() here since any error in the use of this class is security-critical.
> 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?
> Source/WebKit/UIProcess/Launcher/glib/ProcessLauncherGLib.cpp:138
> + if (error.get()) {
> + g_warning("%s", error.get()->message);
> + return;
> + }
Ditto?
> 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.
> 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.)
> Source/WebKit/UIProcess/Launcher/glib/ProcessLauncherGLib.cpp:300
> +static void bindPulse(Vector<CString>& args)
Should add a second FIXME regarding just how big a hole Pulse is. I was quite surprised when you taught me about the bad things a malicious subprocess could do with access to the Pulse socket.
> Source/WebKit/UIProcess/Launcher/glib/ProcessLauncherGLib.cpp:374
> + static DBusProxy proxy;
Exit-time destructors are no bueno. Since DBusProxy has a trivial destructor where nothing important is happening, the solution is to use NeverDestroyed to prevent it from being freed:
static NeverDestroyed<DBusProxy> proxy;
and then proxy.get() to access it.
Standard C++ advice is to put only trivially-destructible objects (e.g. plain old data) in static variables to ensure complex destructors aren't running at exit time. I don't know if DBusProxy is trivially-destructible or not (it is if it does not have a custom destructor -- true -- and if none of its member variables have custom destructors), but it's good practice and more robust, so best do it anyway. Should cause people to think very carefully if adding a destructor to the class in the future.
> Source/WebKit/UIProcess/Launcher/glib/ProcessLauncherGLib.cpp:457
> + // FIXME: GST_PLUGIN_SCANNER
You need to check an env var? It sounds simple enough to fix now, right?
> 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.
> Source/WebKit/UIProcess/Launcher/glib/ProcessLauncherGLib.cpp:612
> +static void
> +ensureDirectoryExists(const String& path)
All on one line
> Source/WebKit/UIProcess/Launcher/glib/ProcessLauncherGLib.cpp:684
> + // FIXME: Find and add the permissions it needs for this (pacparser?)
pacrunner
> Source/WebKit/UIProcess/Launcher/glib/ProcessLauncherGLib.cpp:697
> + // "--talk=org.freedesktop.secrets" // FIXME: WebKitGTK uses for HTTP Auth
I almost forgot about this. Yeah, this will take some effort to fix. That will be a bit of a project to disentangle, since we don't want to give the web process this access. Plan to handle this in a future patch.
> 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.
> 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? fds are kinda a precious resources and it's a bit sad to use so many this way if they're not needed. They should only be needed before bwrap has execed the WebKit subprocess, right? If that's true, then a FIXME would be warranted, at least if ProcessLauncher doesn't have an easy way to tell when it's safe to close.
> 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
Good, IMO this is better than implementing SandboxExtension, even if that was quite tempting.
> Source/WebKit/UIProcess/WebProcessPool.h:469
> + bool sandboxEnabled() { return m_sandboxEnabled; };
bool sandboxEnabled() const
> Source/WebKit/UIProcess/WebProcessPool.h:716
> + HashMap<WebCore::SecurityOriginData, RefPtr<WebProcessProxy>> m_swappedProcesses;
Woah, what's this added here for? Merge conflict gone wrong?
> Source/cmake/OptionsGTK.cmake:209
> +# Since flatpak has its own sandbox we just ignore this option
> +if (EXISTS "/.flatpak-info")
> + set(ENABLE_BUBBLEWRAP_SANDBOX OFF)
> +endif ()
Problem is the options list will print that it is enabled, even though it's not, since you change it after the list is finalized in WEBKIT_OPTION_END(). So this needs to be checked earlier:
if (CMAKE_SYSTEM_NAME MATCHES "Linux" AND EXISTS "/.flatpak-info")
WEBKIT_OPTION_DEFAULT_PORT_VALUE(ENABLE_BUBBLEWRAP_SANDBOX PUBLIC ON)
else ()
WEBKIT_OPTION_DEFAULT_PORT_VALUE(ENABLE_BUBBLEWRAP_SANDBOX PRIVATE OFF)
endif ()
Then you can do a fatal error at the top of the if (ENABLE_BUBBLEWRAP_SANDBOX) condition below here if (EXISTS "/.flatpak-info").
> ChangeLog:8
> + [GTK][WPE] Implement subprocess sandboxing
Remove this duplicated title.
--
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/20180902/dde63c8d/attachment-0001.html>
More information about the webkit-unassigned
mailing list