Bug 187310 – Add the possibility to run unsandboxed plug-ins

• Status: RESOLVED
• Resolution: FIXED
• Component: Plug-ins

[youenn fablet]

Add the possibility to run unsandboxed plug-ins

[Radar WebKit Bug Importer]

<rdar://problem/41798808>

[youenn fablet]

Created attachment 344254 [details]
Patch

[youenn fablet]

Created attachment 344257 [details]
Patch

[EWS Watchlist]

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

[EWS Watchlist]

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

[Alexey Proskuryakov]

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.

[youenn fablet]

Created attachment 344629 [details]
Patch for landing

[youenn fablet]

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

[WebKit Commit Bot]

Comment on attachment 344629 [details]
Patch for landing

Clearing flags on attachment: 344629

Committed r233672: <https://trac.webkit.org/changeset/233672>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.