Don't apply the SandboxPlugins flag until we know a plugin will be used

third_party/WebKit/Source/core/html/HTMLPlugInElement.cpp

Index: third_party/WebKit/Source/core/html/HTMLPlugInElement.cpp
diff --git a/third_party/WebKit/Source/core/html/HTMLPlugInElement.cpp b/third_party/WebKit/Source/core/html/HTMLPlugInElement.cpp
index e6a69bc8e321bb99769291a911d6fb780830fb39..eb1ab9d6c268871864f876f122c47b0f6dc4be77 100644
--- a/third_party/WebKit/Source/core/html/HTMLPlugInElement.cpp
+++ b/third_party/WebKit/Source/core/html/HTMLPlugInElement.cpp
@@ -38,6 +38,7 @@
 #include "core/html/HTMLImageLoader.h"
 #include "core/html/PluginDocument.h"
 #include "core/input/EventHandler.h"
+#include "core/inspector/ConsoleMessage.h"
 #include "core/layout/LayoutBlockFlow.h"
 #include "core/layout/LayoutEmbeddedObject.h"
 #include "core/layout/LayoutImage.h"
@@ -474,12 +475,18 @@ bool HTMLPlugInElement::requestObject(const String& url, const String& mimeType,
         return false;
 
     KURL completedURL = url.isEmpty() ? KURL() : document().completeURL(url);
-    if (!pluginIsLoadable(completedURL, mimeType))
+    if (!objectIsLoadable(completedURL, mimeType))
         return false;
 
     bool useFallback;
-    if (shouldUsePlugin(completedURL, mimeType, hasFallbackContent(), useFallback))
+    if (shouldUsePlugin(completedURL, mimeType, hasFallbackContent(), useFallback)) {
+        if (document().isSandboxed(SandboxPlugins)) {
+            document().addConsoleMessage(ConsoleMessage::create(SecurityMessageSource, ErrorMessageLevel,
+                "Failed to load '" + completedURL.elidedString() + "' as a plugin, because the frame into which the plugin is loading is sandboxed."));
+            return false;
+        }
         return loadPlugin(completedURL, mimeType, paramNames, paramValues, useFallback, true);

[pdr., 2016/01/29 22:14:54]

There is a scary side-codepath
(requestPluginCreationWithoutLayoutObjectIfPossible). Can this code be
structured so loadPlugin can ASSERT_WITH_SECURITY_IMPLICATION(objectIsLoadable
&& shouldUsePlugin)?

[fs, 2016/01/30 00:03:09]

Yeah, I noticed that - and was equally frightened. Looking closer though it
appeared to be a (poorly marked) escape-hatch for tests (test_runner embedder
impl. is the only thing that doesn't block it/return false.) I guess that
doesn't necessarily stop us from pushing the sandbox-check down into loadPlugin
- hopefully no test actually relies on the escape-hatch. Alternatively just push
the requireLayoutObject bool through requestObject too and then call that in
requestPluginCreationWithoutLayoutObjectIfPossible.

+    }
 
     // If the plugin element already contains a subframe,
     // loadOrRedirectSubframe will re-use it. Otherwise, it will create a new
@@ -541,8 +548,10 @@ bool HTMLPlugInElement::shouldUsePlugin(const KURL& url, const String& mimeType,
     if (document().frame()->page() && (mimeType == "image/tiff" || mimeType == "image/tif" || mimeType == "image/x-tiff")) {
         const PluginData* pluginData = document().frame()->page()->pluginData();
         String pluginName = pluginData ? pluginData->pluginNameForMimeType(mimeType) : String();
-        if (!pluginName.isEmpty() && !pluginName.contains("QuickTime", TextCaseInsensitive))
+        if (!pluginName.isEmpty() && !pluginName.contains("QuickTime", TextCaseInsensitive)) {
+            useFallback = false;
             return true;
+        }
     }
 
     ObjectContentType objectType = document().frame()->loader().client()->objectContentType(url, mimeType, shouldPreferPlugInsForImages());
@@ -561,7 +570,7 @@ void HTMLPlugInElement::dispatchErrorEvent()
         dispatchEvent(Event::create(EventTypeNames::error));
 }
 
-bool HTMLPlugInElement::pluginIsLoadable(const KURL& url, const String& mimeType)
+bool HTMLPlugInElement::objectIsLoadable(const KURL& url, const String& mimeType)
 {
     if (url.isEmpty() && mimeType.isEmpty())
         return false;
@@ -574,9 +583,6 @@ bool HTMLPlugInElement::pluginIsLoadable(const KURL& url, const String& mimeType
     if (MIMETypeRegistry::isJavaAppletMIMEType(mimeType))
         return false;
 
-    if (document().isSandboxed(SandboxPlugins))

[pdr., 2016/01/29 22:14:54]

It seems odd to me that sandboxing and the plugin csp checks below result in
different behavior. Is that intended?

[fs, 2016/01/30 00:03:09]

It's intended (for now at least) - as in better to tread with caution. AIUI,
this means you can still block an image/svg+xml object via a CSP directive.
Arguably it should move to within shouldUsePlugin though.

[fs, 2016/02/01 09:21:06]

I shuffled the code a bit and the plugin-CSP check is now grouped with the
sandbox check.

[fs, 2016/02/01 11:52:34]

Doing that added a new failure:

http/tests/security/contentSecurityPolicy/1.1/plugintypes-notype-url.html

I guess this makes us take the mimeType.isEmpty() path in
FrameLoaderClientImpl::objectContentType and end up returning
ObjectContentFrame.

-        return false;
-
     if (!document().securityOrigin()->canDisplay(url)) {
         FrameLoader::reportLocalLoadFailed(frame, url.string());
         return false;