From de13841bc763015ab992ad4c04c9134ac0c708cb Mon Sep 17 00:00:00 2001
From: Michael Catanzaro <mcatanzaro@redhat.com>
Date: Thu, 13 Oct 2022 16:02:45 -0500
Subject: [PATCH] [GTK] D-Bus proxy quietly fails if host bus address is not
 mounted in xdg-dbus-proxy's sandbox
 https://bugs.webkit.org/show_bug.cgi?id=246159

Reviewed by NOBODY (OOPS!).

D-Bus 1.15.2 has changed the default session bus address to a filesystem
socket that lives under /tmp. However, our xdg-dbus-proxy cannot access
this location because we assume the session bus socket will always be
mounted under /run, since that's where all major distros put it. It's OK
to be flexible and mount absolutely any directory, whatever it may be,
since we're not actually trying to create a sandbox that the
xdg-dbus-proxy cannot break out of. It's a trusted process, and the
sandbox exists solely so that portals can verify the app ID of the
process that is using the proxy, which is done by inspecting
/.flatpak-info in its mount namespace's filesystem root. So let's mount
whatever directory is in use and move on. Credit to oreo639 for
investigating the problem and proposing a fix in WebKit#5011.

The a11y bus has the same theoretical problem, although it's not an
issue today because currently it will always be under /run in
practice. Still, we should fix it. There is one complication:
PlatformDisplay currently uses just one variable for both the host a11y
bus address and the proxy bus address, relying on XDGDBusProxy to change
it from the host address to the proxy address. This is fragile and it's
easier to fix it than to work around it by caching the value before it
changes, so at Carlos's suggestion, I have removed the ability to
overwrite the value in PlatformDisplay, and added a separate variable to
track the proxy address in WTF's Sandbox helpers.

I have snuck in a drive-by cleanup to avoid duplicating BASE_DIRECTORY
between two files, a problem that I introduced in 255218@main.
Additionally, I remove a stale declaration for XDGDBusProxy::makePath,
which I forgot to delete after removing the function in the same commit.

Finally, always add the extra sandbox paths to the sandbox. These were
originally extra paths for the web process only, but changed to be extra
paths for both web process and D-Bus proxy. It's no longer needed except
for the web process, but there's no particular reason to limit it
either. I'm changing this here only because it's right next to the code
I'm editing anyway, and it's odd to be adding extra sandbox paths
specifically for the D-Bus proxy process.
---
 Source/WTF/wtf/glib/Sandbox.cpp               | 11 ++++
 Source/WTF/wtf/glib/Sandbox.h                 |  8 +++
 .../platform/graphics/PlatformDisplay.h       |  1 -
 .../Launcher/glib/BubblewrapLauncher.cpp      | 65 +++++++++++++++----
 .../UIProcess/Launcher/glib/XDGDBusProxy.cpp  | 21 +++---
 .../UIProcess/Launcher/glib/XDGDBusProxy.h    |  7 +-
 .../UIProcess/glib/WebProcessPoolGLib.cpp     | 13 +++-
 7 files changed, 95 insertions(+), 31 deletions(-)

diff --git a/Source/WTF/wtf/glib/Sandbox.cpp b/Source/WTF/wtf/glib/Sandbox.cpp
index 7421ee82bfef..905f80e16dce 100644
--- a/Source/WTF/wtf/glib/Sandbox.cpp
+++ b/Source/WTF/wtf/glib/Sandbox.cpp
@@ -60,4 +60,15 @@ bool shouldUsePortal()
     return returnValue;
 }
 
+String& sandboxedAccessibilityBusAddress()
+{
+    static String accessibilityBusAddress;
+    return accessibilityBusAddress;
+}
+
+void setSandboxedAccessibilityBusAddress(String&& address)
+{
+    sandboxedAccessibilityBusAddress() = address;
+}
+
 } // namespace WTF
diff --git a/Source/WTF/wtf/glib/Sandbox.h b/Source/WTF/wtf/glib/Sandbox.h
index 817e12a7a041..9514036dbcc6 100644
--- a/Source/WTF/wtf/glib/Sandbox.h
+++ b/Source/WTF/wtf/glib/Sandbox.h
@@ -19,6 +19,8 @@
 
 #pragma once
 
+#include <wtf/text/WTFString.h>
+
 namespace WTF {
 
 WTF_EXPORT_PRIVATE bool isInsideFlatpak();
@@ -26,9 +28,15 @@ WTF_EXPORT_PRIVATE bool isInsideDocker();
 WTF_EXPORT_PRIVATE bool isInsideSnap();
 WTF_EXPORT_PRIVATE bool shouldUsePortal();
 
+WTF_EXPORT_PRIVATE String& sandboxedAccessibilityBusAddress();
+WTF_EXPORT_PRIVATE void setSandboxedAccessibilityBusAddress(String&&);
+
 } // namespace WTF
 
 using WTF::isInsideFlatpak;
 using WTF::isInsideDocker;
 using WTF::isInsideSnap;
 using WTF::shouldUsePortal;
+
+using WTF::sandboxedAccessibilityBusAddress;
+using WTF::setSandboxedAccessibilityBusAddress;
diff --git a/Source/WebCore/platform/graphics/PlatformDisplay.h b/Source/WebCore/platform/graphics/PlatformDisplay.h
index 5697812a6099..44b7b5ba5228 100644
--- a/Source/WebCore/platform/graphics/PlatformDisplay.h
+++ b/Source/WebCore/platform/graphics/PlatformDisplay.h
@@ -105,7 +105,6 @@ class PlatformDisplay {
 #endif
 
 #if USE(ATSPI)
-    void setAccessibilityBusAddress(String&& address) { m_accessibilityBusAddress = WTFMove(address); }
     const String& accessibilityBusAddress() const;
 #endif
 
diff --git a/Source/WebKit/UIProcess/Launcher/glib/BubblewrapLauncher.cpp b/Source/WebKit/UIProcess/Launcher/glib/BubblewrapLauncher.cpp
index 066305b3e912..0a201fe17609 100644
--- a/Source/WebKit/UIProcess/Launcher/glib/BubblewrapLauncher.cpp
+++ b/Source/WebKit/UIProcess/Launcher/glib/BubblewrapLauncher.cpp
@@ -199,13 +199,19 @@ static void bindIfExists(Vector<CString>& args, const char* path, BindFlags bind
         args.appendVector(Vector<CString>({ bindType, path, path }));
 }
 
+static const char* dbusProxyDirectory()
+{
+    static GUniquePtr<char> path(g_build_filename(g_get_user_runtime_dir(), BASE_DIRECTORY, nullptr));
+    return path.get();
+}
+
 static void bindDBusSession(Vector<CString>& args, XDGDBusProxy& dbusProxy, bool allowPortals)
 {
-    auto dbusSessionProxyPath = dbusProxy.dbusSessionProxy(allowPortals ? XDGDBusProxy::AllowPortals::Yes : XDGDBusProxy::AllowPortals::No);
+    auto dbusSessionProxyPath = dbusProxy.dbusSessionProxy(BASE_DIRECTORY, allowPortals ? XDGDBusProxy::AllowPortals::Yes : XDGDBusProxy::AllowPortals::No);
     if (!dbusSessionProxyPath)
         return;
 
-    GUniquePtr<char> sandboxedSessionBusPath(g_build_filename(g_get_user_runtime_dir(), BASE_DIRECTORY, "bus", nullptr));
+    GUniquePtr<char> sandboxedSessionBusPath(g_build_filename(dbusProxyDirectory(), "bus", nullptr));
     GUniquePtr<char> proxyAddress(g_strdup_printf("unix:path=%s", sandboxedSessionBusPath.get()));
     args.appendVector(Vector<CString> {
         "--ro-bind", *dbusSessionProxyPath, sandboxedSessionBusPath.get(),
@@ -347,8 +353,8 @@ static void bindGtkData(Vector<CString>& args)
 #if ENABLE(ACCESSIBILITY)
 static void bindA11y(Vector<CString>& args, XDGDBusProxy& dbusProxy)
 {
-    GUniquePtr<char> sandboxedAccessibilityBusPath(g_build_filename(g_get_user_runtime_dir(), BASE_DIRECTORY, "at-spi-bus", nullptr));
-    auto accessibilityProxyPath = dbusProxy.accessibilityProxy(sandboxedAccessibilityBusPath.get());
+    GUniquePtr<char> sandboxedAccessibilityBusPath(g_build_filename(dbusProxyDirectory(), "at-spi-bus", nullptr));
+    auto accessibilityProxyPath = dbusProxy.accessibilityProxy(BASE_DIRECTORY, sandboxedAccessibilityBusPath.get());
     if (!accessibilityProxyPath)
         return;
 
@@ -620,6 +626,30 @@ static bool shouldUnshareNetwork(ProcessLauncher::ProcessType processType)
     return true;
 }
 
+static std::optional<CString> directoryContainingDBusSocket(const char* dbusAddress)
+{
+    if (!dbusAddress || !g_str_has_prefix(dbusAddress, "unix:"))
+        return std::nullopt;
+
+    if (const char* pathStart = strstr(dbusAddress, "path=")) {
+        pathStart += strlen("path=");
+
+        const char* pathEnd = pathStart;
+        while (*pathEnd && *pathEnd != ',')
+            pathEnd++;
+
+        CString path(pathStart, pathEnd - pathStart);
+        GRefPtr<GFile> file = adoptGRef(g_file_new_for_path(path.data()));
+        GRefPtr<GFile> parent = adoptGRef(g_file_get_parent(file.get()));
+        if (!parent)
+            return std::nullopt;
+
+        return { g_file_peek_path(parent.get()) };
+    }
+
+    return std::nullopt;
+}
+
 static void addExtraPaths(const HashMap<CString, SandboxPermission>& paths, Vector<CString>& args)
 {
     for (const auto& pathAndPermission : paths) {
@@ -686,16 +716,31 @@ GRefPtr<GSubprocess> bubblewrapSpawn(GSubprocessLauncher* launcher, const Proces
         "--ro-bind-try", PKGLIBEXECDIR, PKGLIBEXECDIR,
     };
 
+    addExtraPaths(launchOptions.extraSandboxPaths, sandboxArgs);
+
     if (launchOptions.processType == ProcessLauncher::ProcessType::DBusProxy) {
         sandboxArgs.appendVector(Vector<CString>({
             "--ro-bind", DBUS_PROXY_EXECUTABLE, DBUS_PROXY_EXECUTABLE,
-            // This is a lot of access, but xdg-dbus-proxy is trusted so that's OK. It's sandboxed
-            // only because we have to mount .flatpak-info in its mount namespace. The user rundir
-            // is where we mount our proxy socket.
-            "--bind", runDir, runDir,
+            "--bind", dbusProxyDirectory(), dbusProxyDirectory(),
         }));
 
-        addExtraPaths(launchOptions.extraSandboxPaths, sandboxArgs);
+        // xdg-dbus-proxy is trusted, so it's OK to mount the directories that contain the session
+        // bus and a11y bus sockets wherever they may be. xdg-dbus-proxy is sandboxed only because
+        // we have to mount .flatpak-info in its mount namespace so that portals may use it as a
+        // trusted way to get the app ID of the process that is using it.
+        if (auto sessionBusDirectory = directoryContainingDBusSocket(g_getenv("DBUS_SESSION_BUS_ADDRESS"))) {
+            sandboxArgs.appendVector(Vector<CString>({
+                "--bind", *sessionBusDirectory, *sessionBusDirectory,
+            }));
+        }
+
+#if ENABLE(ACCESSIBILITY)
+        if (auto a11yBusDirectory = directoryContainingDBusSocket(PlatformDisplay::sharedDisplay().accessibilityBusAddress().utf8().data())) {
+            sandboxArgs.appendVector(Vector<CString>({
+                "--bind", *a11yBusDirectory, *a11yBusDirectory,
+            }));
+        }
+#endif
     }
 
     if (shouldUnshareNetwork(launchOptions.processType))
@@ -742,8 +787,6 @@ GRefPtr<GSubprocess> bubblewrapSpawn(GSubprocessLauncher* launcher, const Proces
             bindX11(sandboxArgs);
 #endif
 
-        addExtraPaths(launchOptions.extraSandboxPaths, sandboxArgs);
-
         Vector<String> extraPaths = { "applicationCacheDirectory"_s, "mediaKeysDirectory"_s, "waylandSocket"_s };
         for (const auto& path : extraPaths) {
             String extraPath = launchOptions.extraInitializationData.get(path);
diff --git a/Source/WebKit/UIProcess/Launcher/glib/XDGDBusProxy.cpp b/Source/WebKit/UIProcess/Launcher/glib/XDGDBusProxy.cpp
index bd9a9c486f52..1641df8cfecb 100644
--- a/Source/WebKit/UIProcess/Launcher/glib/XDGDBusProxy.cpp
+++ b/Source/WebKit/UIProcess/Launcher/glib/XDGDBusProxy.cpp
@@ -32,18 +32,13 @@
 #include <wtf/UniStdExtras.h>
 #include <wtf/glib/GRefPtr.h>
 #include <wtf/glib/GUniquePtr.h>
-
-#if PLATFORM(GTK)
-#define BASE_DIRECTORY "webkitgtk"
-#elif PLATFORM(WPE)
-#define BASE_DIRECTORY "wpe"
-#endif
+#include <wtf/glib/Sandbox.h>
 
 namespace WebKit {
 
-CString XDGDBusProxy::makeProxy(const char* proxyTemplate)
+CString XDGDBusProxy::makeProxy(const char* baseDirectory, const char* proxyTemplate)
 {
-    GUniquePtr<char> appRunDir(g_build_filename(g_get_user_runtime_dir(), BASE_DIRECTORY, nullptr));
+    GUniquePtr<char> appRunDir(g_build_filename(g_get_user_runtime_dir(), baseDirectory, nullptr));
     if (g_mkdir_with_parents(appRunDir.get(), 0700) == -1) {
         g_warning("Failed to mkdir for dbus proxy (%s): %s", appRunDir.get(), g_strerror(errno));
         return { };
@@ -59,7 +54,7 @@ CString XDGDBusProxy::makeProxy(const char* proxyTemplate)
     return proxySocketTemplate.get();
 }
 
-std::optional<CString> XDGDBusProxy::dbusSessionProxy(AllowPortals allowPortals)
+std::optional<CString> XDGDBusProxy::dbusSessionProxy(const char* baseDirectory, AllowPortals allowPortals)
 {
     if (!m_dbusSessionProxyPath.isNull())
         return m_dbusSessionProxyPath;
@@ -68,7 +63,7 @@ std::optional<CString> XDGDBusProxy::dbusSessionProxy(AllowPortals allowPortals)
     if (!dbusAddress)
         return std::nullopt;
 
-    m_dbusSessionProxyPath = makeProxy("bus-proxy-XXXXXX");
+    m_dbusSessionProxyPath = makeProxy(baseDirectory, "bus-proxy-XXXXXX");
     if (m_dbusSessionProxyPath.isNull())
         return std::nullopt;
 
@@ -97,7 +92,7 @@ std::optional<CString> XDGDBusProxy::dbusSessionProxy(AllowPortals allowPortals)
     return m_dbusSessionProxyPath;
 }
 
-std::optional<CString> XDGDBusProxy::accessibilityProxy(const char* sandboxedAccessibilityBusPath)
+std::optional<CString> XDGDBusProxy::accessibilityProxy(const char* baseDirectory, const char* sandboxedAccessibilityBusPath)
 {
 #if ENABLE(ACCESSIBILITY)
     if (!m_accessibilityProxyPath.isNull())
@@ -107,12 +102,12 @@ std::optional<CString> XDGDBusProxy::accessibilityProxy(const char* sandboxedAcc
     if (dbusAddress.isNull())
         return std::nullopt;
 
-    m_accessibilityProxyPath = makeProxy("a11y-proxy-XXXXXX");
+    m_accessibilityProxyPath = makeProxy(baseDirectory, "a11y-proxy-XXXXXX");
     if (m_accessibilityProxyPath.isNull())
         return std::nullopt;
 
 #if USE(ATSPI)
-    WebCore::PlatformDisplay::sharedDisplay().setAccessibilityBusAddress(makeString("unix:path=", sandboxedAccessibilityBusPath));
+    setSandboxedAccessibilityBusAddress(makeString("unix:path=", sandboxedAccessibilityBusPath));
 #endif
 
     m_args.appendVector(Vector<CString> {
diff --git a/Source/WebKit/UIProcess/Launcher/glib/XDGDBusProxy.h b/Source/WebKit/UIProcess/Launcher/glib/XDGDBusProxy.h
index 9098ffad0249..aaf5b498870c 100644
--- a/Source/WebKit/UIProcess/Launcher/glib/XDGDBusProxy.h
+++ b/Source/WebKit/UIProcess/Launcher/glib/XDGDBusProxy.h
@@ -41,14 +41,13 @@ class XDGDBusProxy {
     ~XDGDBusProxy() = default;
 
     enum class AllowPortals : bool { No, Yes };
-    std::optional<CString> dbusSessionProxy(AllowPortals);
-    std::optional<CString> accessibilityProxy(const char* sandboxedAccessibilityBusPath);
+    std::optional<CString> dbusSessionProxy(const char* baseDirectory, AllowPortals);
+    std::optional<CString> accessibilityProxy(const char* baseDirectory, const char* sandboxedAccessibilityBusPath);
 
     bool launch();
 
 private:
-    static CString makePath(const char* dbusAddress);
-    static CString makeProxy(const char* proxyTemplate);
+    static CString makeProxy(const char* baseDirectory, const char* proxyTemplate);
 
     Vector<CString> m_args;
     CString m_dbusSessionProxyPath;
diff --git a/Source/WebKit/UIProcess/glib/WebProcessPoolGLib.cpp b/Source/WebKit/UIProcess/glib/WebProcessPoolGLib.cpp
index 01e436171aa3..7ad5ae7aaf0a 100644
--- a/Source/WebKit/UIProcess/glib/WebProcessPoolGLib.cpp
+++ b/Source/WebKit/UIProcess/glib/WebProcessPoolGLib.cpp
@@ -34,6 +34,7 @@
 #include "WebProcessCreationParameters.h"
 #include <WebCore/PlatformDisplay.h>
 #include <wtf/FileSystem.h>
+#include <wtf/glib/Sandbox.h>
 
 #if ENABLE(REMOTE_INSPECTOR)
 #include <JavaScriptCore/RemoteInspector.h>
@@ -119,8 +120,16 @@ void WebProcessPool::platformInitializeWebProcess(const WebProcessProxy& process
 #endif
 
 #if USE(ATSPI)
-    static const char* accessibilityBusAddress = getenv("WEBKIT_A11Y_BUS_ADDRESS");
-    parameters.accessibilityBusAddress = accessibilityBusAddress ? String::fromUTF8(accessibilityBusAddress) : WebCore::PlatformDisplay::sharedDisplay().accessibilityBusAddress();
+    parameters.accessibilityBusAddress = [] {
+        if (auto* address = getenv("WEBKIT_A11Y_BUS_ADDRESS"))
+            return String::fromUTF8(address);
+
+        String& address = sandboxedAccessibilityBusAddress();
+        if (!address.isNull())
+            return address;
+
+        return WebCore::PlatformDisplay::sharedDisplay().accessibilityBusAddress();
+    }();
 #endif
 
 #if PLATFORM(GTK)
