[Webkit-unassigned] [Bug 188568] [GTK][WPE] Implement subprocess sandboxing
bugzilla-daemon at webkit.org
bugzilla-daemon at webkit.org
Mon Sep 17 06:36:12 PDT 2018
https://bugs.webkit.org/show_bug.cgi?id=188568
Carlos Garcia Campos <cgarcia at igalia.com> changed:
What |Removed |Added
----------------------------------------------------------------------------
Attachment #349776|review? |review-
Flags| |
--- Comment #40 from Carlos Garcia Campos <cgarcia 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
I have no idea about sandboxing, so I'm sorry if I make any stupid comment or question about it. r- mainly because of the memory leaks and coding style issues, my other comments are probably because I don't know who the sandbox works.
> Source/WebKit/UIProcess/API/glib/WebKitWebContext.cpp:1158
> + * This method **must be called before any web process has been created**,
> + * as early as possible in your application. Calling it later is a fatal error.
Maybe we could make it a construct only property then, because it doesn't make sense to change it either.
> 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?
> Source/WebKit/UIProcess/Launcher/glib/ProcessLauncherGLib.cpp:79
> + GRefPtr<GBytes> bytes(g_string_free_to_bytes(buffer));
This is leaked, the reference needs to be adopted. Use GRefPtr<GBytes> bytes = adoptGRef(g_string_free_to_bytes(buffer));
> Source/WebKit/UIProcess/Launcher/glib/ProcessLauncherGLib.cpp:81
> + int memfd = memfd_create(name, MFD_ALLOW_SEALING);
Should we also pass MFD_CLOEXEC?
> Source/WebKit/UIProcess/Launcher/glib/ProcessLauncherGLib.cpp:86
> + ssize_t ret;
Move this below, where it's first needed. Do not use abbreviations for the variable names.
> Source/WebKit/UIProcess/Launcher/glib/ProcessLauncherGLib.cpp:93
> + if ((size_t)ret != size)
Use C++ style casts.
> 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?
> Source/WebKit/UIProcess/Launcher/glib/ProcessLauncherGLib.cpp:120
> + CString proxyPath() const { return m_proxyPath; };
const CString&
> Source/WebKit/UIProcess/Launcher/glib/ProcessLauncherGLib.cpp:121
> + CString path() const { return m_path; };
Ditto.
> 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?
> Source/WebKit/UIProcess/Launcher/glib/ProcessLauncherGLib.cpp:137
> + const char* runDir = g_get_user_runtime_dir();
> + GUniquePtr<char> appRunDir(g_build_filename(runDir, g_get_prgname(), nullptr));
I would use g_get_user_runtime_dir() directly, since runDir isn't reused, right?
> 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.
> 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.
> Source/WebKit/UIProcess/Launcher/glib/ProcessLauncherGLib.cpp:157
> + "--bind-try", m_path, m_path,
I guess this should be m_path.data() in both cases.
> 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?
>> Source/WebKit/UIProcess/Launcher/glib/ProcessLauncherGLib.cpp:204
>> + static char* makeProxySocket(const char *appRunDir)
>
> const char* appRunDir
This should return a GUniquePtr<char>
>> 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?
> Source/WebKit/UIProcess/Launcher/glib/ProcessLauncherGLib.cpp:211
> + char* proxySocketTemplate = g_build_filename(appRunDir, "dbus-proxy-XXXXXX", nullptr);
GUniquePtr
> Source/WebKit/UIProcess/Launcher/glib/ProcessLauncherGLib.cpp:220
> + return proxySocketTemplate;
I'm confused about this method too, it sounds like it's creating a proxy socket, but it's just creating a temp file.
> Source/WebKit/UIProcess/Launcher/glib/ProcessLauncherGLib.cpp:226
> + bool m_isRunning = false;
bool m_isRunning { false };
> Source/WebKit/UIProcess/Launcher/glib/ProcessLauncherGLib.cpp:227
> + GPid m_pid;
I wonder if we could make this a std::optional and then we don't need m_isRunning
> Source/WebKit/UIProcess/Launcher/glib/ProcessLauncherGLib.cpp:228
> + Vector<CString> m_permissions = { };
Vector<CString> m_permissions;
> Source/WebKit/UIProcess/Launcher/glib/ProcessLauncherGLib.cpp:231
> +static char* dbusAddressToPath(const char* address, bool abstract = false)
This should return a GUniquePtr<char>. Use an enum instead of a boolean parameter.
> Source/WebKit/UIProcess/Launcher/glib/ProcessLauncherGLib.cpp:234
> + const char* path;
> + const char* path_end;
Move this where they are fist needed. path_end -> pathEnd
> Source/WebKit/UIProcess/Launcher/glib/ProcessLauncherGLib.cpp:242
> + path = strstr(address, abstract ? "abstract=" : "path=");
const char* path =
> Source/WebKit/UIProcess/Launcher/glib/ProcessLauncherGLib.cpp:247
> + path_end = path;
const char* pathEnd =
> 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
> Source/WebKit/UIProcess/Launcher/glib/ProcessLauncherGLib.cpp:265
> + const char* type;
bindType?
> Source/WebKit/UIProcess/Launcher/glib/ProcessLauncherGLib.cpp:271
> + type = "--bind-try";
ah, the if exists is because of the -try suffix?
> 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?
> Source/WebKit/UIProcess/Launcher/glib/ProcessLauncherGLib.cpp:356
> + if (pulseConfig)
> + bindIfExists(args, pulseConfig);
Are we ever actually passing a nullptr as path? What is the if exists for then?
> Source/WebKit/UIProcess/Launcher/glib/ProcessLauncherGLib.cpp:413
> + static DBusProxy proxy;
So, the proxy is only for a11y?
> Source/WebKit/UIProcess/Launcher/glib/ProcessLauncherGLib.cpp:417
> + GRefPtr<GDBusConnection> sessionBus(g_bus_get_sync(G_BUS_TYPE_SESSION, nullptr, nullptr));
This is leaked too, you need to adopt the returned ref.
> Source/WebKit/UIProcess/Launcher/glib/ProcessLauncherGLib.cpp:421
> + GRefPtr<GDBusMessage> msg(g_dbus_message_new_method_call("org.a11y.Bus", "/org/a11y/bus", "org.a11y.Bus", "GetAddress"));
This is leaked too, you need to adopt the returned ref.
> Source/WebKit/UIProcess/Launcher/glib/ProcessLauncherGLib.cpp:423
> + GRefPtr<GDBusMessage> reply(g_dbus_connection_send_message_with_reply_sync(
This is leaked too, you need to adopt the returned ref.
> Source/WebKit/UIProcess/Launcher/glib/ProcessLauncherGLib.cpp:436
> + g_warning("Can't find a11y bus: %s", error.get()->message);
error->message
> Source/WebKit/UIProcess/Launcher/glib/ProcessLauncherGLib.cpp:563
> + struct scmp_arg_cmp clone_arg = SCMP_A0(SCMP_CMP_MASKED_EQ, CLONE_NEWUSER, CLONE_NEWUSER);
clone_arg -> cloneArg
> Source/WebKit/UIProcess/Launcher/glib/ProcessLauncherGLib.cpp:564
> + struct scmp_arg_cmp tty_arg = SCMP_A1(SCMP_CMP_EQ, (int)TIOCSTI);
tty_arg -> ttyArg
> Source/WebKit/UIProcess/Launcher/glib/ProcessLauncherGLib.cpp:568
> + } syscall_blacklist[] = {
syscall_blankclist -> syscallBlacklist
> Source/WebKit/UIProcess/Launcher/glib/ProcessLauncherGLib.cpp:613
> + g_error("Failed to init seccomp");
> + return -1;
Return after g_error again here.
> Source/WebKit/UIProcess/Launcher/glib/ProcessLauncherGLib.cpp:626
> + g_error("Failed to add seccomp rule");
> + seccomp_release(seccomp);
> + return -1;
Return and release after g_error here.
> Source/WebKit/UIProcess/Launcher/glib/ProcessLauncherGLib.cpp:634
> + if (tmpfd == -1) {
> + g_error("Failed to create memfd: %s", g_strerror(errno));
> + seccomp_release(seccomp);
> + return -1;
This can't happen, memfd_create already handles all errors caling g_error
> Source/WebKit/UIProcess/Launcher/glib/ProcessLauncherGLib.cpp:641
> + g_error("Failed to export seccomp bpf");
> + seccomp_release(seccomp);
> + close(tmpfd);
> + return -1;
More g_error here
> Source/WebKit/UIProcess/Launcher/glib/ProcessLauncherGLib.cpp:659
> +static void ensureDirectoryExists(const String& path)
> +{
> + if (path.isEmpty())
> + return;
> +
> + // FIXME: Blocking
> + if (g_mkdir_with_parents(path.utf8().data(), 0700) == -1 && errno != ENOTDIR)
> + g_warning("Could not create directory \"%s\": %s", path.utf8().data(), g_strerror(errno));
> +}
You can use Filesystem::makeAllDirectories()
> 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?
> Source/WebKit/UIProcess/Launcher/glib/ProcessLauncherGLib.cpp:716
> + // We would have to parse ld config files for more info
Comments should finish with period.
> Source/WebKit/UIProcess/Launcher/glib/ProcessLauncherGLib.cpp:810
> + Vector<CString> bwrapArgs = {
I'm confused again, wasn't this done by the proxy launch method?
> Source/WebKit/UIProcess/Launcher/glib/ProcessLauncherGLib.cpp:858
> +#endif
I think all the sandbox code should be moved to its own file
> Source/WebKit/UIProcess/Launcher/glib/ProcessLauncherGLib.cpp:931
> + int seccompFd = -1;
> + int bwrapFd = -1;
std::optional
> Source/WebKit/UIProcess/Launcher/glib/ProcessLauncherGLib.cpp:936
> + Vector<CString> sandboxArgs = { };
{ } is not needed, Vector is already empty.
> 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?
> Source/WebKit/UIProcess/Storage/StorageProcessProxy.cpp:95
> + WebsiteDataStore& store = m_processPool.websiteDataStore()->websiteDataStore();
> + store.resolveDirectoriesIfNecessary();
> + launchOptions.extraSandboxPaths.append(store.resolvedIndexedDatabaseDirectory());
Should we do this even when sandbox is disabled?
> Source/cmake/WebKitFeatures.cmake:88
> + WEBKIT_OPTION_DEFINE(ENABLE_BUBBLEWRAP_SANDBOX "Toggle bubblewrap sandboxing support" PRIVATE OFF)
Why is this private?
--
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/d168c005/attachment-0001.html>
More information about the webkit-unassigned
mailing list