Bug 195169 - [WPE] Enable web process sandbox
Summary: [WPE] Enable web process sandbox
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WPE WebKit (show other bugs)
Version: WebKit Nightly Build
Hardware: PC Linux
: P2 Normal
Assignee: Michael Catanzaro
URL:
Keywords:
Depends on: 195402
Blocks:
  Show dependency treegraph
 
Reported: 2019-02-28 09:36 PST by Michael Catanzaro
Modified: 2019-03-11 09:47 PDT (History)
8 users (show)

See Also:


Attachments
Patch (8.96 KB, patch)
2019-02-28 09:37 PST, Michael Catanzaro
no flags Details | Formatted Diff | Diff
Patch (10.50 KB, patch)
2019-02-28 09:40 PST, Michael Catanzaro
no flags Details | Formatted Diff | Diff
Patch (12.03 KB, patch)
2019-02-28 09:41 PST, Michael Catanzaro
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews126 for ios-simulator-wk2 (2.57 MB, application/zip)
2019-02-28 11:31 PST, Build Bot
no flags Details
Patch (12.00 KB, patch)
2019-03-01 05:05 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 2019-02-28 09:36:52 PST
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.
Comment 1 Michael Catanzaro 2019-02-28 09:37:52 PST
Created attachment 363228 [details]
Patch
Comment 2 Michael Catanzaro 2019-02-28 09:40:31 PST
Created attachment 363229 [details]
Patch
Comment 3 Michael Catanzaro 2019-02-28 09:41:42 PST
Created attachment 363230 [details]
Patch
Comment 4 Patrick Griffis 2019-02-28 09:42:58 PST
> 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)
Comment 5 Michael Catanzaro 2019-02-28 09:48:44 PST
If WPE maintainers want me to remove the opt-out, I can do so. Otherwise I'll leave it in.
Comment 6 Michael Catanzaro 2019-02-28 11:01:04 PST
BTW the WPE bots need new deps: bubblewrap and libseccomp
Comment 7 Build Bot 2019-02-28 11:31:24 PST
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
Comment 8 Build Bot 2019-02-28 11:31:26 PST
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
Comment 9 Zan Dobersek 2019-03-01 00:18:22 PST
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?
Comment 10 Michael Catanzaro 2019-03-01 05:04:31 PST
(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).
Comment 11 Michael Catanzaro 2019-03-01 05:05:59 PST
Created attachment 363324 [details]
Patch
Comment 12 Daniel Bates 2019-03-03 14:14:25 PST
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
Comment 13 Michael Catanzaro 2019-03-04 06:59:51 PST
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.
Comment 14 WebKit Commit Bot 2019-03-04 07:25:49 PST
Comment on attachment 363324 [details]
Patch

Clearing flags on attachment: 363324

Committed r242354: <https://trac.webkit.org/changeset/242354>
Comment 15 WebKit Commit Bot 2019-03-04 07:25:50 PST
All reviewed patches have been landed.  Closing bug.
Comment 16 WebKit Commit Bot 2019-03-07 00:03:22 PST
Re-opened since this is blocked by bug 195402
Comment 17 Michael Catanzaro 2019-03-07 08:27:38 PST
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).
Comment 18 Michael Catanzaro 2019-03-11 06:06:40 PDT
(In reply to Michael Catanzaro from comment #17)
> Hm, I could either land it disabled by default

Let's do this.
Comment 19 Michael Catanzaro 2019-03-11 09:47:27 PDT
Committed r242709: <https://trac.webkit.org/changeset/242709>