Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[GTK] D-Bus proxy quietly fails if host bus address is not mounted in xdg-dbus-proxy's sandbox #5136

Conversation

mcatanzaro
Copy link
Contributor

@mcatanzaro mcatanzaro commented Oct 7, 2022

67cda4a

[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 Carlos Garcia Campos.

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.

Canonical link: https://commits.webkit.org/255530@main

14ccf3e

Misc iOS, tvOS & watchOS macOS Linux Windows
βœ… πŸ§ͺ style βœ… πŸ›  ios βœ… πŸ›  mac βœ… πŸ›  wpe   πŸ›  πŸ§ͺ win
βœ… πŸ§ͺ bindings βœ… πŸ›  ios-sim βœ… πŸ›  mac-debug βœ… πŸ›  gtk   πŸ›  wincairo
βœ… πŸ§ͺ webkitperl   πŸ§ͺ ios-wk2 βœ… πŸ›  mac-AS-debug   πŸ§ͺ gtk-wk2
  πŸ§ͺ api-ios   πŸ§ͺ api-mac   πŸ§ͺ api-gtk
βœ… πŸ›  πŸ§ͺ jsc βœ… πŸ›  tv   πŸ§ͺ mac-wk1 βœ… πŸ›  jsc-armv7
βœ… πŸ›  tv-sim   πŸ§ͺ mac-wk2 βœ… πŸ§ͺ jsc-armv7-tests
βœ… πŸ›  πŸ§ͺ merge βœ… πŸ›  watch   πŸ§ͺ mac-AS-debug-wk2 βœ… πŸ›  jsc-mips
βœ… πŸ›  watch-sim βœ… πŸ§ͺ mac-wk2-stress βœ… πŸ§ͺ jsc-mips-tests

@mcatanzaro mcatanzaro requested a review from a team as a code owner October 7, 2022 15:47
@mcatanzaro mcatanzaro self-assigned this Oct 7, 2022
@mcatanzaro mcatanzaro added Other WebKitGTK Bugs related to the Gtk API layer. labels Oct 7, 2022
@mcatanzaro mcatanzaro force-pushed the eng/GTK-D-Bus-proxy-quietly-fails-if-host-bus-address-is-not-mounted-in-xdg-dbus-proxys-sandbox branch from f575ab0 to e7e27d6 Compare October 7, 2022 15:56
@nater1983
Copy link

srry long day at work i ment to ask if this was applied on top of 5011 ( but u already answered me ) ;)

@nekopsykose
Copy link

hm, now it just hangs instead (including no ctrl-c response)

< epiphany 

(epiphany:24462): Handy-WARNING **: 23:18:52.771: Using GtkSettings:gtk-application-prefer-dark-theme together with HdyStyleManager is unsupported. Please use HdyStyleManager:color-scheme instead.
Failed to start proxy for unix:path=/tmp/dbus-cedV4wlGOs,guid=8ef707fe648236840582ef15634093c2: Error binding to address (GUnixSocketAddress): No such file or directory

@nater1983
Copy link

nater1983 commented Oct 7, 2022

Same as nekopsykose

bash-5.2$ epiphany

(epiphany:2687): Failed to start proxy for unix:path=/tmp/dbus-mtWqSI4TTT,guid=553a6cff8d17610f2c43237f6340f2a5: Error binding to address (GUnixSocketAddress): No such file or directory
Killed

@oreo639
Copy link

oreo639 commented Oct 8, 2022

That's because $XDG_RUNTIME_DIR/BASE_DIRECTORY never gets mounted when launching xdg-dbus-proxy, which is an issue since the proxy address is created there and it needs to be able to be mounted by the other containers.

@nater1983
Copy link

nater1983 commented Oct 8, 2022

What is the bind command you used?

@oreo639
Copy link

oreo639 commented Oct 8, 2022

diff -rup BubblewrapLauncher.cpp.orig BubblewrapLauncher.cpp
--- BubblewrapLauncher.cpp.orig	2022-10-07 19:03:21.339834564 -0700
+++ BubblewrapLauncher.cpp	2022-10-07 19:04:31.467135669 -0700
@@ -713,8 +713,10 @@ GRefPtr<GSubprocess> bubblewrapSpawn(GSu
     addExtraPaths(launchOptions.extraSandboxPaths, sandboxArgs);
 
     if (launchOptions.processType == ProcessLauncher::ProcessType::DBusProxy) {
+        GUniquePtr<char> appRunDir(g_build_filename(g_get_user_runtime_dir(), BASE_DIRECTORY, nullptr));
         sandboxArgs.appendVector(Vector<CString>({
             "--ro-bind", DBUS_PROXY_EXECUTABLE, DBUS_PROXY_EXECUTABLE,
+            "--bind", appRunDir.get(), appRunDir.get(),
         }));
 
         // xdg-dbus-proxy is trusted, so it's OK to mount the directories that contain the session

@nater1983
Copy link

nater1983 commented Oct 8, 2022

I verified that oreo's additional patch for bubblewrap once again made Epiphany load with 5136.patch

@webkit-ews-buildbot webkit-ews-buildbot added the merging-blocked Applied to prevent a change from being merged label Oct 8, 2022
@mcatanzaro mcatanzaro marked this pull request as draft October 8, 2022 16:13
@mcatanzaro
Copy link
Contributor Author

Looks like the accessibility API tests actually caught that mistake, but I didn't notice because the tests are so flaky that I never bother to even look at the test failures anymore....

@mcatanzaro mcatanzaro removed the merging-blocked Applied to prevent a change from being merged label Oct 10, 2022
@mcatanzaro mcatanzaro force-pushed the eng/GTK-D-Bus-proxy-quietly-fails-if-host-bus-address-is-not-mounted-in-xdg-dbus-proxys-sandbox branch from e7e27d6 to 344d794 Compare October 10, 2022 17:15
@mcatanzaro
Copy link
Contributor Author

OK, hopefully fixed... please test this updated version. Let's also wait and see if the CI likes it.

@nater1983
Copy link

nater1983 commented Oct 10, 2022

Ok i just got done testing your updated 5136 patch
image

@nekopsykose
Copy link

everything works correctly for me. thanks @mcatanzaro!

@oreo639
Copy link

oreo639 commented Oct 11, 2022

I can confirm that this works for me as well, tysm.

Comment on lines 108 to 109
void setAccessibilityBusAddress(String&& address) { m_accessibilityBusAddress = WTFMove(address); }
void setAccessibilityBusProxyAddress(String&& address) { m_accessibilityBusProxyAddress = WTFMove(address); }
const String accessibilityBusProxyAddress() const;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think this is right, the platform display only cares about the actual address to use, the same way we override the env var, whether it's the actual one or the socket one is a detail the display doesn't need to know.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hm, then I think I need to move all the code that computes the accessibility bus address to someplace else. Will consider this.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think PlatformDisplay really has anything to do with the accessibility bus at all. We've just put the code to compute the accessibility bus address here because it's a convenient global location, and it seems as nice a place for it to go as anywhere else does, so I'm not inclined to try moving it.

The problem is that XDGDBusProxy overrides the address here with an address pointing to the proxy socket instead, but we still need the original unmodified address later on when launching the xdg-dbus-proxy process. So the problem to solve is: do not overwrite the original address. I thought separating the original address from the proxy address was a nice way to do it, but any other way would work just fine. My next proposal will be a new function PlatformDisplay::originalAccessibilityBusAddress that works the same as PlatformDisplay::accessibilityBusAddress except it ignores the value set with PlatformDisplay::setAccessibilityBusAddress. That should work. I wanted to call it PlatformDisplay::hostAccessibilityBusAddress, but that would be confusing when running in the web process, because they'll return the same result in the web process, which never sees the real host bus address, only the proxy address.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updated, does this look OK to you now? And if not, do you have an alternative suggestion?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The reason why it was added to PlatformDisplay is because in X11, the bus address is set as an atom in the x11 display (AT_SPI_BUS), see PlatformDisplayX11::platformAccessibilityBusAddress().

@mcatanzaro
Copy link
Contributor Author

Seems there are still a11y test failures:

dbind-FATAL-WARNING: AT-SPI: Unable to open bus connection: Failed to connect to socket /run/user/1000/at-spi2-2OG9T1/socket: No such file or directory
(TestWebKitAccessibility:7521): dbind-WARNING **: 18:15:16.737: AT-SPI: Unable to open bus connection: Failed to connect to socket /run/user/1000/at-spi2-2OG9T1/socket: No such file or directory

@nater1983
Copy link

nater1983 commented Oct 11, 2022

(process:3013): GLib-CRITICAL **: 21:49:32.257: Failed to set scheduler settings: Operation not permitted
free(): invalid size
This is error im recieving just trying to go to a particular website

@mcatanzaro
Copy link
Contributor Author

(process:3013): GLib-CRITICAL **: 21:49:32.257: Failed to set scheduler settings: Operation not permitted

That's https://gitlab.gnome.org/GNOME/glib/-/issues/2769. It's not related to this pull request.

free(): invalid size

Feel free to report a bug for this, but you'll need a backtrace taken with gdb. I doubt it's related to this pull request.

@nater1983
Copy link

I doubt it is too. I thought I would let you know.

@mcatanzaro mcatanzaro marked this pull request as ready for review October 12, 2022 16:56
@mcatanzaro mcatanzaro force-pushed the eng/GTK-D-Bus-proxy-quietly-fails-if-host-bus-address-is-not-mounted-in-xdg-dbus-proxys-sandbox branch from 344d794 to 4151de8 Compare October 12, 2022 17:01
@webkit-ews-buildbot webkit-ews-buildbot added the merging-blocked Applied to prevent a change from being merged label Oct 13, 2022
@mcatanzaro mcatanzaro removed the merging-blocked Applied to prevent a change from being merged label Oct 13, 2022
@mcatanzaro mcatanzaro force-pushed the eng/GTK-D-Bus-proxy-quietly-fails-if-host-bus-address-is-not-mounted-in-xdg-dbus-proxys-sandbox branch from 4151de8 to de13841 Compare October 13, 2022 21:04
@mcatanzaro
Copy link
Contributor Author

@oreo639 it's probably worth testing this one more time if you have a chance to do so. It should still be fine, but worth checking just in case.

@nekopsykose
Copy link

still works for me

Copy link
Contributor

@carlosgcampos carlosgcampos left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please add m_sandboxEnabled check before landing.

Source/WebKit/UIProcess/glib/WebProcessPoolGLib.cpp Outdated Show resolved Hide resolved
@oreo639
Copy link

oreo639 commented Oct 14, 2022

Yeah, it works for me. I did get a compilation error on gcc 10.2 due to a commit that has since been reverted and it builds after applying the revert.

@mcatanzaro mcatanzaro force-pushed the eng/GTK-D-Bus-proxy-quietly-fails-if-host-bus-address-is-not-mounted-in-xdg-dbus-proxys-sandbox branch from de13841 to bc4e4cd Compare October 14, 2022 11:12
@mcatanzaro mcatanzaro added the merge-queue Applied to send a pull request to merge-queue label Oct 14, 2022
@webkit-ews-buildbot webkit-ews-buildbot added merging-blocked Applied to prevent a change from being merged and removed merge-queue Applied to send a pull request to merge-queue labels Oct 14, 2022
@mcatanzaro mcatanzaro removed the merging-blocked Applied to prevent a change from being merged label Oct 14, 2022
@mcatanzaro mcatanzaro force-pushed the eng/GTK-D-Bus-proxy-quietly-fails-if-host-bus-address-is-not-mounted-in-xdg-dbus-proxys-sandbox branch from bc4e4cd to 14ccf3e Compare October 14, 2022 12:33
@mcatanzaro mcatanzaro added the merge-queue Applied to send a pull request to merge-queue label Oct 14, 2022
… xdg-dbus-proxy's sandbox

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

Reviewed by Carlos Garcia Campos.

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.

Canonical link: https://commits.webkit.org/255530@main
@webkit-commit-queue webkit-commit-queue force-pushed the eng/GTK-D-Bus-proxy-quietly-fails-if-host-bus-address-is-not-mounted-in-xdg-dbus-proxys-sandbox branch from 14ccf3e to 67cda4a Compare October 14, 2022 13:09
@webkit-commit-queue
Copy link
Collaborator

Committed 255530@main (67cda4a): https://commits.webkit.org/255530@main

Reviewed commits have been landed. Closing PR #5136 and removing active labels.

@webkit-commit-queue webkit-commit-queue merged commit 67cda4a into WebKit:main Oct 14, 2022
@webkit-commit-queue webkit-commit-queue removed the merge-queue Applied to send a pull request to merge-queue label Oct 14, 2022
@mcatanzaro mcatanzaro deleted the eng/GTK-D-Bus-proxy-quietly-fails-if-host-bus-address-is-not-mounted-in-xdg-dbus-proxys-sandbox branch October 14, 2022 13:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
WebKitGTK Bugs related to the Gtk API layer.
Projects
None yet
8 participants