Bug 187310 - Add the possibility to run unsandboxed plug-ins
Summary: Add the possibility to run unsandboxed plug-ins
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Plug-ins (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: youenn fablet
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2018-07-03 16:50 PDT by youenn fablet
Modified: 2018-07-09 17:26 PDT (History)
5 users (show)

See Also:


Attachments
Patch (8.34 KB, patch)
2018-07-03 19:02 PDT, youenn fablet
no flags Details | Formatted Diff | Diff
Patch (10.49 KB, patch)
2018-07-03 19:55 PDT, youenn fablet
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews123 for ios-simulator-wk2 (2.34 MB, application/zip)
2018-07-03 21:37 PDT, EWS Watchlist
no flags Details
Patch for landing (10.70 KB, patch)
2018-07-09 15:29 PDT, youenn fablet
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description youenn fablet 2018-07-03 16:50:29 PDT
Add the possibility to run unsandboxed plug-ins
Comment 1 Radar WebKit Bug Importer 2018-07-03 16:53:04 PDT
<rdar://problem/41798808>
Comment 2 youenn fablet 2018-07-03 19:02:28 PDT
Created attachment 344254 [details]
Patch
Comment 3 youenn fablet 2018-07-03 19:55:45 PDT
Created attachment 344257 [details]
Patch
Comment 4 EWS Watchlist 2018-07-03 21:37:48 PDT
Comment on attachment 344257 [details]
Patch

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

New failing tests:
animations/needs-layout.html
Comment 5 EWS Watchlist 2018-07-03 21:37:49 PDT
Created attachment 344261 [details]
Archive of layout-test-results from ews123 for ios-simulator-wk2

The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews123  Port: ios-simulator-wk2  Platform: Mac OS X 10.13.4
Comment 6 Alexey Proskuryakov 2018-07-09 13:20:06 PDT
Comment on attachment 344257 [details]
Patch

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

> Source/WebCore/page/RuntimeEnabledFeatures.h:269
> +    void setSandboxPlugInEnabled(bool isEnabled) { m_isSandboxPlugInEnabled = isEnabled; }
> +    bool sandboxPlugInEnabled() const { return m_isSandboxPlugInEnabled; }

This name seems confusing. Maybe something like "setExperimentalPlugInSandboxProfilesEnabled"?

> Source/WebKit/PluginProcess/mac/PluginProcessMac.mm:638
> +    if (!confstr(_CS_DARWIN_USER_CACHE_DIR, cacheDirectory, sizeof(cacheDirectory))) {

This look suspicious to me (but no more suspicious than existing code). The result of confstr depends on DIRHELPER_USER_DIR_SUFFIX, which is only set in ChildProcess::initializeSandbox that's called later. How is that OK? Which directory paths are we getting here?

Perhaps it's OK because we only use this with randomized paths like "WebKitPlugin-aQVpAV". But I don't see where that's being enforced.

> Source/WebKit/Shared/WebPreferences.yaml:1264
> +  humanReadableName: "Sandbox Plug-Ins"
> +  humanReadableDescription: "Enable Plug-In sandboxing"

These also need better names.

> Source/WebKit/UIProcess/Plugins/mac/PluginInfoStoreMac.mm:87
> +bool PluginInfoStore::shouldRunPluginUnsandboxed(const String& pluginBundleIdentifier)

Maybe allowPluginToRunUnsandboxed?

> Source/WebKit/UIProcess/Plugins/mac/PluginInfoStoreMac.mm:92
> +    return pluginBundleIdentifier == "com.cisco.webex.plugin.gpc64"

Let's make it _s at least, as this doesn't seem very performance sensitive to warrant a precomputed table of any kind.
Comment 7 youenn fablet 2018-07-09 15:29:54 PDT
Created attachment 344629 [details]
Patch for landing
Comment 8 youenn fablet 2018-07-09 15:40:37 PDT
> > Source/WebKit/PluginProcess/mac/PluginProcessMac.mm:638
> > +    if (!confstr(_CS_DARWIN_USER_CACHE_DIR, cacheDirectory, sizeof(cacheDirectory))) {
> 
> This look suspicious to me (but no more suspicious than existing code). The
> result of confstr depends on DIRHELPER_USER_DIR_SUFFIX, which is only set in
> ChildProcess::initializeSandbox that's called later. How is that OK? Which
> directory paths are we getting here?
> 
> Perhaps it's OK because we only use this with randomized paths like
> "WebKitPlugin-aQVpAV". But I don't see where that's being enforced.

Looking at it, m_nsurlCacheDirectory value is used only as a sandbox parameter and it will allow the plugin to allow reading/writing it.
It is probably not great in terms of sandboxing since all plugins will be allowed to read/write the same folder. But this is preexisting behavior, so it is best to split this issue from this bugzilla.
Comment 9 WebKit Commit Bot 2018-07-09 17:26:10 PDT
Comment on attachment 344629 [details]
Patch for landing

Clearing flags on attachment: 344629

Committed r233672: <https://trac.webkit.org/changeset/233672>
Comment 10 WebKit Commit Bot 2018-07-09 17:26:11 PDT
All reviewed patches have been landed.  Closing bug.