Bug 219010 - [WPE][GTK] SleepDisabler does not inhibit sleep with bubblewrap sandbox enabled: need to run xdg-dbus-proxy under bwrap or xdg-desktop-portal does not read our app ID
Summary: [WPE][GTK] SleepDisabler does not inhibit sleep with bubblewrap sandbox enabl...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKitGTK (show other bugs)
Version: WebKit Nightly Build
Hardware: PC Linux
: P2 Normal
Assignee: Michael Catanzaro
URL:
Keywords:
Depends on: 221918
Blocks: Bubblewrap
  Show dependency treegraph
 
Reported: 2020-11-16 16:16 PST by Michael Catanzaro
Modified: 2021-02-15 14:28 PST (History)
4 users (show)

See Also:


Attachments
Patch (9.85 KB, patch)
2021-02-08 15:08 PST, Michael Catanzaro
no flags Details | Formatted Diff | Diff
Patch (8.79 KB, patch)
2021-02-08 15:12 PST, Michael Catanzaro
no flags Details | Formatted Diff | Diff
Patch for landing (8.77 KB, patch)
2021-02-15 09:21 PST, Michael Catanzaro
no flags Details | Formatted Diff | Diff
Patch for landing (9.50 KB, patch)
2021-02-15 10:03 PST, Michael Catanzaro
no flags Details | Formatted Diff | Diff
Patch for landing (9.44 KB, patch)
2021-02-15 10:11 PST, Michael Catanzaro
no flags Details | Formatted Diff | Diff
Patch for landing (9.41 KB, patch)
2021-02-15 13:58 PST, Michael Catanzaro
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Michael Catanzaro 2020-11-16 16:16:54 PST
Splitting this out of bug #186971. Sleep inhibitors are broken since the bubblewrap sandbox is enabled. First problem is that the org.freedesktop.ScreenSaver API is not allowed through the sandbox, so calls to that fail. (We could fix this bug by whitelisting it. That's the easy way.) It falls back to the portal API. (Ideally, we would call the modern portal API first, and fall back to the older ScreenSaver API only if portals are not supported.) Now, the portal API fails for a strange reason. For the portal API, we aren't trusted to provide our own app ID, so xdg-desktop-portal looks it up by (a) getting our pid from its D-Bus connection, via UNIX credentials to ensure it is trusted, then (b) opening /proc/$PID/root/.flatpak-info to read the app ID from there, which the app cannot falsify because it does not have write access. BubblewrapLauncher.cpp creates this file by passing a sealed memfd to bwrap's --ro-bind-data. To be completely certain it is property created inside the sandbox, I added some debug code to SleepDisablerGLib.cpp:

 void SleepDisablerGLib::acquireInhibitorViaInhibitPortalProxy()
 {
+WTFLogAlways("%s", __FUNCTION__);
+GUniqueOutPtr<char> contents;
+GUniqueOutPtr<GError> error;
+g_file_get_contents("/.flatpak-info", &contents.outPtr(), nullptr, &error.outPtr());
+if (!error)
+g_warning("contents=%s", contents.get());
+else g_warning("error=%s", error->message);

which prints:

contents=[Application]
name=org.gnome.Epiphany

So that seems to be working properly. Problem is, for some reason, it doesn't exist at /proc/$PID/root/.flatpak-info on the host, so xdg-destkop-portal cannot see it! For real flatpaks, the file exists here (and contains a lot more than just the app ID). I'm bamboozled as to why the file is visible inside the sandbox, but is not there in the host's view of the sandbox.
Comment 1 Michael Catanzaro 2020-11-16 16:20:08 PST
Um, Patrick, if you don't have any ideas, I guess we can just allow access to org.freedesktop.ScreenSaver through the D-Bus proxy? But I'm worried because likely other portal features are going to be broken too if the portal implementation does not see our .flatpak-info. This is likely to be a problem for a lot more than just the inhibit portal....
Comment 2 Patrick Griffis 2020-11-17 09:32:37 PST
(In reply to Michael Catanzaro from comment #1)
> Um, Patrick, if you don't have any ideas, I guess we can just allow access
> to org.freedesktop.ScreenSaver through the D-Bus proxy? But I'm worried
> because likely other portal features are going to be broken too if the
> portal implementation does not see our .flatpak-info. This is likely to be a
> problem for a lot more than just the inhibit portal....

Well I don't think that is a huge risk, but yes we really need to fix the `.flatpak-info` problem instead. I swore it worked when I tested it before.
Comment 3 Michael Catanzaro 2020-11-17 12:42:21 PST
(In reply to Michael Catanzaro from comment #0)
> I'm bamboozled as to why the
> file is visible inside the sandbox, but is not there in the host's view of
> the sandbox.

I looked closer. It's there in the WebKitWebProcess sandbox, but xdg-desktop-portal doesn't look into the WebKitWebProcess's mount namespace: it looks into *xdg-dbus-proxy's* mount namespace. And that is running on the host.

The difference is that flatpak sandboxes xdg-dbus-proxy with bwrap, but we run xdg-dbus-proxy unsandboxed. That doesn't seem unreasonable, since xdg-dbus-proxy is trusted, but it seems xdg-desktop-portal relies on this to get the .flatpak-info.

That also resolves my confusion regarding how xdg-dbus-proxy can get UNIX credentials from the sandboxed process if it's only talking to its xdg-dbus-proxy rather than talking to it directly. It doesn't. ;)
Comment 4 Michael Catanzaro 2020-11-30 06:54:40 PST
(In reply to Michael Catanzaro from comment #0)
> (Ideally, we would call the modern
> portal API first, and fall back to the older ScreenSaver API only if portals
> are not supported.)

BTW, this won't work, because even if we fix the bubblewrap sandbox, the portal API will fail if bubblewrap sandbox is disabled, as described in comment #3.
Comment 5 Patrick Griffis 2020-12-04 09:23:47 PST
(In reply to Michael Catanzaro from comment #4)
> BTW, this won't work, because even if we fix the bubblewrap sandbox, the
> portal API will fail if bubblewrap sandbox is disabled, as described in
> comment #3.

This just sounds like a bug. Portals should work for non-sandboxed applications.
Comment 6 Michael Catanzaro 2020-12-04 17:33:10 PST
(In reply to Patrick Griffis from comment #5)
> This just sounds like a bug. Portals should work for non-sandboxed
> applications.

How would we change that?

Currently any portals that depend on knowing the application's app ID will not work.
Comment 7 Michael Catanzaro 2021-02-01 08:17:19 PST
IMO we need to run xdg-dbus-proxy under bwrap, to match what flatpak does. That should solve this.
Comment 8 Michael Catanzaro 2021-02-02 16:14:38 PST
I have a patch that fixes this locally by running xdg-dbus-proxy under bwrap. It needs a bunch of cleanup before I post it for review, but it's coming soon.

Note that there are several media player bugs not yet fixed (bug #219353, bug #219354, bug #219355). My patch will only fix the sandbox side of things.
Comment 9 Michael Catanzaro 2021-02-08 15:08:47 PST
Created attachment 419636 [details]
Patch
Comment 10 Michael Catanzaro 2021-02-08 15:12:04 PST
Created attachment 419638 [details]
Patch
Comment 11 Michael Catanzaro 2021-02-08 15:15:03 PST
Comment on attachment 419638 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=419638&action=review

> Source/WebKit/UIProcess/Launcher/glib/BubblewrapLauncher.cpp:783
> +            "--bind", runDir.get(), runDir.get(),

So I'm having some trouble here. In theory, I should mount the exact paths required using bindDBus() instead of mounting the entire user run dir. But I just can't get it to work otherwise.

Note that a11y doesn't actually work with this change, but it didn't work *before* this change either, so that's at least not a regression. It seems the sandbox has to be completely disabled for orca to work.
Comment 12 Michael Catanzaro 2021-02-08 15:27:11 PST
(Also a11y doesn't seem to work when run under flatpak either, which cannot be BubblewrapLauncher's fault.)
Comment 13 Patrick Griffis 2021-02-13 14:31:02 PST
Comment on attachment 419638 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=419638&action=review

> Source/WebKit/UIProcess/Launcher/glib/BubblewrapLauncher.cpp:736
> +    GUniquePtr<char> runDir(g_strdup_printf("/run/user/%d", getuid()));

Any reason not to use g_get_user_runtime_dir()?
Comment 14 Patrick Griffis 2021-02-13 14:48:08 PST
Comment on attachment 419638 [details]
Patch

Overall this looks good to me. I'm also not sure why the runtime dir permission is needed but I also agree its a trusted process.
Comment 15 Michael Catanzaro 2021-02-13 15:37:32 PST
(In reply to Patrick Griffis from comment #13)
> Comment on attachment 419638 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=419638&action=review
> 
> > Source/WebKit/UIProcess/Launcher/glib/BubblewrapLauncher.cpp:736
> > +    GUniquePtr<char> runDir(g_strdup_printf("/run/user/%d", getuid()));
> 
> Any reason not to use g_get_user_runtime_dir()?

Hmm, I probably should indeed.

I copied it from flatpak-run but it's probably just an oversight.
Comment 16 Michael Catanzaro 2021-02-15 09:21:37 PST
Created attachment 420323 [details]
Patch for landing
Comment 17 Michael Catanzaro 2021-02-15 09:48:22 PST
Comment on attachment 420323 [details]
Patch for landing

Oops, this introduces a warning that I need to fix before landing:

[1833/2372] Building CXX object Source/WebKit/CMakeFiles/...ces/WebKit/unified-sources/UnifiedSource-88d1702b-1.cpp.o
In file included from DerivedSources/WebKit/unified-sources/UnifiedSource-88d1702b-1.cpp:1:
../../Source/WebKit/UIProcess/AuxiliaryProcessProxy.cpp: In member function ‘virtual void WebKit::AuxiliaryProcessProxy::getLaunchOptions(WebKit::ProcessLauncher::LaunchOptions&)’:
../../Source/WebKit/UIProcess/AuxiliaryProcessProxy.cpp:70:12: warning: enumeration value ‘DBusProxy’ not handled in switch [-Wswitch]
   70 |     switch (launchOptions.processType) {
      |            ^
Comment 18 Michael Catanzaro 2021-02-15 10:03:19 PST
Created attachment 420325 [details]
Patch for landing
Comment 19 Michael Catanzaro 2021-02-15 10:05:10 PST
Comment on attachment 420325 [details]
Patch for landing

Now I forgot to respond to Patrick's last feedback....
Comment 20 Michael Catanzaro 2021-02-15 10:11:44 PST
Created attachment 420326 [details]
Patch for landing
Comment 21 EWS 2021-02-15 10:40:13 PST
Committed r272863: <https://commits.webkit.org/r272863>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 420326 [details].
Comment 22 Michael Catanzaro 2021-02-15 13:18:31 PST
The patch that landed is broken. Launching processes fails with:

Failed to start proxy for unix:abstract=002d1: Error binding to address (GUnixSocketAddress): No such file or directory

I am not sure what's wrong yet and am going to revert the whole thing until I know what happened. Maybe failed to test a last-minute change or something.
Comment 23 Michael Catanzaro 2021-02-15 13:42:47 PST
(In reply to Michael Catanzaro from comment #15)
> (In reply to Patrick Griffis from comment #13)
> > Comment on attachment 419638 [details]
> > Patch
> > 
> > View in context:
> > https://bugs.webkit.org/attachment.cgi?id=419638&action=review
> > 
> > > Source/WebKit/UIProcess/Launcher/glib/BubblewrapLauncher.cpp:736
> > > +    GUniquePtr<char> runDir(g_strdup_printf("/run/user/%d", getuid()));
> > 
> > Any reason not to use g_get_user_runtime_dir()?
> 
> Hmm, I probably should indeed.
> 
> I copied it from flatpak-run but it's probably just an oversight.

I was so confident that g_get_user_runtime_dir() would be equivalent that after making this change, I only tested whether it built, not whether it worked. But I messed up the change and replaced --dir /run with --dir g_get_user_runtime_dir(), which means the entire user runtime dir was replaced with an empty directory. Oops.
Comment 24 Michael Catanzaro 2021-02-15 13:56:01 PST
No that's wrong, sorry, I misread the code. The user run dir really is supposed to an empty directory, so sandboxed processes can't see anything created by the host.

The problem is I replaced this line of the code:

"--bind", runDir.get(), runDir.get(),

With this:

"--bind", runDir,

Whoops, now the rest of the bubblewrap command line is off by one....
Comment 25 Michael Catanzaro 2021-02-15 13:58:11 PST
Created attachment 420367 [details]
Patch for landing
Comment 26 EWS 2021-02-15 14:28:07 PST
Committed r272882: <https://commits.webkit.org/r272882>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 420367 [details].