Bug 195169 – [WPE] Enable web process sandbox

• Status: RESOLVED
• Resolution: FIXED
• Component: WPE WebKit

[Michael Catanzaro]

Enable web process sandbox for WPE. This is expected to break unexpected things, so the start of a new release cycle is the perfect time for it.

It's on by default, but applications can opt-out. Originally we had expected not to provide an opt-out, but now you can effectively opt-out of the bind mounting by whitelisting / so there's no real benefit. Can change any of this if desired.

[Michael Catanzaro]

Created attachment 363228 [details]
Patch

[Michael Catanzaro]

Created attachment 363229 [details]
Patch

[Michael Catanzaro]

Created attachment 363230 [details]
Patch

[Patrick Griffis]

> but now you can effectively opt-out of the bind mounting by whitelisting / so there's no real benefit.

I'm not sure that would actually work. Since user provided mounts are added after the webkit ones I think it will just error with conflicts? (untested)

[Michael Catanzaro]

If WPE maintainers want me to remove the opt-out, I can do so. Otherwise I'll leave it in.

[Michael Catanzaro]

BTW the WPE bots need new deps: bubblewrap and libseccomp

[EWS Watchlist]

Comment on attachment 363230 [details]
Patch

Attachment 363230 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: https://webkit-queues.webkit.org/results/11319346

New failing tests:
imported/w3c/web-platform-tests/webrtc/simplecall.https.html

[EWS Watchlist]

Created attachment 363242 [details]
Archive of layout-test-results from ews126 for ios-simulator-wk2

The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews126  Port: ios-simulator-wk2  Platform: Mac OS X 10.13.6

[Zan Dobersek]

Comment on attachment 363230 [details]
Patch

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

> Source/cmake/OptionsWPE.cmake:93
> +if (CMAKE_SYSTEM_NAME MATCHES "Linux" AND NOT EXISTS "/.flatpak-info")
> +    WEBKIT_OPTION_DEFAULT_PORT_VALUE(ENABLE_BUBBLEWRAP_SANDBOX PRIVATE ON)
> +else ()
> +    WEBKIT_OPTION_DEFAULT_PORT_VALUE(ENABLE_BUBBLEWRAP_SANDBOX PRIVATE OFF)
> +endif ()

Is the option private by default with a purpose? IMO it should be public, allowing deployers to disable it in case they are providing their own sandboxing solution.

Also, what's the story with .flatpak-info? Is this only meant for flatpak builds?

[Michael Catanzaro]

(In reply to Zan Dobersek from comment #9)
> Is the option private by default with a purpose? IMO it should be public,
> allowing deployers to disable it in case they are providing their own
> sandboxing solution.

I picked private to make it mandatory, but I'll change it to public.

> Also, what's the story with .flatpak-info? Is this only meant for flatpak
> builds?

It's to disable the setting for flatpak builds (since it will be broken: bubblewrap sandboxes do not nest).

[Michael Catanzaro]

Created attachment 363324 [details]
Patch

[Daniel Bates]

Comment on attachment 363324 [details]
Patch

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

I don’t know a lot about GTK or WPE, but felt this patch was sane. I do know about bubble wrap (the packaging stuff not the bwrap executable).

> Source/WebKit/PlatformWPE.cmake:19
> +add_definitions(-DLIBDIR="${LIB_INSTALL_DIR}")

A “why”? for the ignorant in the change log would be nice :)

> Source/WebKit/UIProcess/Launcher/glib/BubblewrapLauncher.cpp:671
> +#if ENABLE(NETSCAPE_PLUGIN_API)

There’s something about this launcher’s name I really like... could it be BUBBLEWRAP! :D Yes, that’s what it is.

> Source/cmake/BubblewrapSandboxChecks.cmake:4
> +        message(FATAL_ERROR "bwrap executable is needed for ENABLE_BUBBLEWRAP_SANDBOX")

Bubblewrap sandbox <— I like it

[Michael Catanzaro]

Comment on attachment 363324 [details]
Patch

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

>> Source/WebKit/PlatformWPE.cmake:19
>> +add_definitions(-DLIBDIR="${LIB_INSTALL_DIR}")
> 
> A “why”? for the ignorant in the change log would be nice :)

It's used in BubblewrapLauncher.cpp.

>> Source/WebKit/UIProcess/Launcher/glib/BubblewrapLauncher.cpp:671
>> +#if ENABLE(NETSCAPE_PLUGIN_API)
> 
> There’s something about this launcher’s name I really like... could it be BUBBLEWRAP! :D Yes, that’s what it is.

Imagine a bubblewrap cannon. Hire an artist.

[WebKit Commit Bot]

Comment on attachment 363324 [details]
Patch

Clearing flags on attachment: 363324

Committed r242354: <https://trac.webkit.org/changeset/242354>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[WebKit Commit Bot]

Re-opened since this is blocked by bug 195402

[Michael Catanzaro]

Hm, I could either land it disabled by default, or leave it for someone else to try to get layout tests working (probably by adding C API to whitelist directories for the TestRunner).

[Michael Catanzaro]

(In reply to Michael Catanzaro from comment #17)
> Hm, I could either land it disabled by default

Let's do this.

[Michael Catanzaro]

Committed r242709: <https://trac.webkit.org/changeset/242709>