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

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKitGTK

[Michael Catanzaro]

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.

[Michael Catanzaro]

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....

[Patrick Griffis]

(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.

[Michael Catanzaro]

(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. ;)

[Michael Catanzaro]

(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.

[Patrick Griffis]

(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.

[Michael Catanzaro]

(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.

[Michael Catanzaro]

IMO we need to run xdg-dbus-proxy under bwrap, to match what flatpak does. That should solve this.

[Michael Catanzaro]

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.

[Michael Catanzaro]

Created attachment 419636 [details]
Patch

[Michael Catanzaro]

Created attachment 419638 [details]
Patch

[Michael Catanzaro]

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.

[Michael Catanzaro]

(Also a11y doesn't seem to work when run under flatpak either, which cannot be BubblewrapLauncher's fault.)

[Patrick Griffis]

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()?

[Patrick Griffis]

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.

[Michael Catanzaro]

(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.

[Michael Catanzaro]

Created attachment 420323 [details]
Patch for landing

[Michael Catanzaro]

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) {
      |            ^

[Michael Catanzaro]

Created attachment 420325 [details]
Patch for landing

[Michael Catanzaro]

Comment on attachment 420325 [details]
Patch for landing

Now I forgot to respond to Patrick's last feedback....

[Michael Catanzaro]

Created attachment 420326 [details]
Patch for landing

[EWS]

Committed r272863: <https://commits.webkit.org/r272863>

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

[Michael Catanzaro]

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.

[Michael Catanzaro]

(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.

[Michael Catanzaro]

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....

[Michael Catanzaro]

Created attachment 420367 [details]
Patch for landing

[EWS]

Committed r272882: <https://commits.webkit.org/r272882>

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