[ MimeHandlerView ] Do not load in sandboxed frame

This CL ensure that a frame-based MimeHandlerView does not load inside a sandboxed frame.; to this end, the following changes are made to the loading mechanism for MimeHandlerView: 1- Blink embedder is not asked for a MimeHandlerView if the document is sandboxed. 2- For frame navigation to MimeHandlerView MIME type where the HTML page is injected by the browser, MimeHandlerViewEmbedder will clear itself at commit time, if the embedder frame turns out to be sandboxed. 3- MimeHandlerViewContainerManager will load an empty page. Bug: 963641 Change-Id: Ib68be60042556a39e38d92f954201d0297631890 Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1614282 Commit-Queue: Ehsan Karamad <ekaramad@chromium.org> Reviewed-by: James MacLean <wjmaclean@chromium.org> Reviewed-by: Kentaro Hara <haraken@chromium.org> Reviewed-by: Alex Moshchuk <alexmos@chromium.org> Reviewed-by: Ken Buchanan <kenrb@chromium.org> Cr-Commit-Position: refs/heads/master@{#664628}
marcosholgado · May 30, 2019 · 71e3227 · 71e3227
1 parent 7697836
commit 71e3227
Show file tree

Hide file tree

Showing 8 changed files with 82 additions and 8 deletions.
diff --git a/chrome/test/data/extensions/api_test/mime_handler_view/test_sandboxed_frame.html b/chrome/test/data/extensions/api_test/mime_handler_view/test_sandboxed_frame.html
@@ -0,0 +1,9 @@
+<!doctype html>
+<iframe id="sandbox1" sandbox src="testEmbedded.csv"></iframe>
+<iframe id="sandbox2" sandbox src="test_embedded.html"></iframe>
+<iframe id="notsandboxed" src="testEmbedded.csv"></iframe>
+<script>
+  function remove_frame(id) {
+    document.getElementById(id).outerHTML = "";
+  }
+</script>
diff --git a/extensions/browser/guest_view/mime_handler_view/mime_handler_view_browsertest.cc b/extensions/browser/guest_view/mime_handler_view/mime_handler_view_browsertest.cc
@@ -22,6 +22,7 @@
 #include "components/app_modal/native_app_modal_dialog.h"
 #include "components/guest_view/browser/test_guest_view_manager.h"
 #include "content/public/browser/browser_thread.h"
+#include "content/public/browser/navigation_handle.h"
 #include "content/public/browser/web_contents_observer.h"
 #include "content/public/common/content_features.h"
 #include "content/public/common/content_switches.h"
@@ -640,3 +641,33 @@ IN_PROC_BROWSER_TEST_P(MimeHandlerViewCrossProcessTest,
   // Run some JavaScript in embedder and make sure it is not crashed.
   ASSERT_TRUE(content::ExecJs(GetEmbedderWebContents(), "true"));
 }
+
+// Verifies that sandboxed frames do not create GuestViews (plugins are
+// blocked in sandboxed frames).
+IN_PROC_BROWSER_TEST_P(MimeHandlerViewCrossProcessTest,
+                       DoNotLoadInSandboxedFrame) {
+  // Use the testing subclass of MimeHandlerViewGuest.
+  GetGuestViewManager()->RegisterTestGuestViewType<MimeHandlerViewGuest>(
+      base::Bind(&TestMimeHandlerViewGuest::Create));
+
+  const extensions::Extension* extension = LoadTestExtension();
+  ASSERT_TRUE(extension);
+
+  ui_test_utils::NavigateToURL(
+      browser(), embedded_test_server()->GetURL("/test_sandboxed_frame.html"));
+
+  auto* guest_view_manager = GetGuestViewManager();
+  // The page contains three <iframes> where two are sandboxed. The expectation
+  // is that the sandboxed frames do not end up creating a MimeHandlerView.
+  // Therefore, it suffices to wait for one GuestView to be created, then remove
+  // the non-sandboxed frame, and ensue there are no GuestViews left.
+  if (guest_view_manager->num_guests_created() == 0)
+    ASSERT_TRUE(guest_view_manager->WaitForNextGuestCreated());
+  ASSERT_EQ(1U, guest_view_manager->num_guests_created());
+  // Remove the non-sandboxed frame.
+  ASSERT_TRUE(content::ExecJs(GetEmbedderWebContents(),
+                              "remove_frame('notsandboxed');"));
+  // The page is expected to embed only '1' GuestView. If there is GuestViews
+  // embedded inside other frames we should be timing out here.
+  guest_view_manager->WaitForAllGuestsDeleted();
+}
diff --git a/extensions/browser/guest_view/mime_handler_view/mime_handler_view_embedder.cc b/extensions/browser/guest_view/mime_handler_view/mime_handler_view_embedder.cc
@@ -20,6 +20,7 @@
 #include "extensions/common/mojo/guest_view.mojom.h"
 #include "third_party/blink/public/common/associated_interfaces/associated_interface_provider.h"
 #include "third_party/blink/public/common/frame/frame_owner_element_type.h"
+#include "third_party/blink/public/common/frame/sandbox_flags.h"
 
 namespace extensions {
 
@@ -89,17 +90,21 @@ void MimeHandlerViewEmbedder::DidStartNavigation(
 
 void MimeHandlerViewEmbedder::ReadyToCommitNavigation(
     content::NavigationHandle* handle) {
-  if (handle->GetFrameTreeNodeId() == frame_tree_node_id_ &&
-      !render_frame_host_) {
-    DCHECK_EQ(handle->GetURL(), resource_url_);
+  if (handle->GetFrameTreeNodeId() != frame_tree_node_id_)
+    return;
+  if (!render_frame_host_) {
     render_frame_host_ = handle->GetRenderFrameHost();
-      // In such cases the print helper might need to postMessage to the frame
-      // container and we should ensure one exists already. Note that in general
-      // <embed> and <object> navigations
     GetContainerManager()->SetInternalId(internal_id_);
   }
 }
 
+void MimeHandlerViewEmbedder::DidFinishNavigation(
+    content::NavigationHandle* handle) {
+  if (frame_tree_node_id_ != handle->GetFrameTreeNodeId())
+    return;
+  CheckSandboxFlags();
+}
+
 void MimeHandlerViewEmbedder::RenderFrameCreated(
     content::RenderFrameHost* render_frame_host) {
   if (!render_frame_host_ ||
@@ -212,4 +217,12 @@ void MimeHandlerViewEmbedder::ReadyToCreateMimeHandlerView(
     GetMimeHandlerViewEmbeddersMap()->erase(frame_tree_node_id_);
 }
 
+void MimeHandlerViewEmbedder::CheckSandboxFlags() {
+  if (!render_frame_host_->IsSandboxed(blink::WebSandboxFlags::kPlugins))
+    return;
+  // Notify the renderer to load an empty page instead.
+  GetContainerManager()->LoadEmptyPage(resource_url_);
+  GetMimeHandlerViewEmbeddersMap()->erase(frame_tree_node_id_);
+}
+
 }  // namespace extensions
diff --git a/extensions/browser/guest_view/mime_handler_view/mime_handler_view_embedder.h b/extensions/browser/guest_view/mime_handler_view/mime_handler_view_embedder.h
@@ -49,6 +49,7 @@ class MimeHandlerViewEmbedder : public content::WebContentsObserver {
   void FrameDeleted(content::RenderFrameHost* render_frame_host) override;
   void DidStartNavigation(content::NavigationHandle* handle) override;
   void ReadyToCommitNavigation(content::NavigationHandle* handle) override;
+  void DidFinishNavigation(content::NavigationHandle* handle) override;
 
   void ReadyToCreateMimeHandlerView(bool result);
 
@@ -64,6 +65,11 @@ class MimeHandlerViewEmbedder : public content::WebContentsObserver {
   // Returns null before |render_frame_host_| is known.
   mojom::MimeHandlerViewContainerManager* GetContainerManager();
 
+  // Checks the sandbox state of |render_frame_host_|. If the frame is sandboxed
+  // it will send an IPC to renderer to show an empty page and immediately
+  // deletes |this|.
+  void CheckSandboxFlags();
+
   // The ID for the embedder frame of MimeHandlerViewGuest.
   int32_t frame_tree_node_id_;
   const GURL resource_url_;

diff --git a/extensions/common/mojo/guest_view.mojom b/extensions/common/mojo/guest_view.mojom
@@ -45,10 +45,15 @@ interface GuestView {
 // creation process (by creating a MimeHandlerViewFrameContainer).
 interface MimeHandlerViewContainerManager {
   // Sets the expected |internal_id| of the plugin element that will be used
-  // to attach the MimeHandlerViewGuest. This is only used for wiring up
-  // postMessaging.
+  // to attach the MimeHandlerViewGuest.
   SetInternalId(string token_id);
 
+  // Requests the MimeHandlerViewContainerManager to load an empty page as an
+  // HTML string. This is used in cases where MimeHandlerViewEmbedder decides
+  // not to continue with loading the embedding page for the resource at
+  // |resource_url|, e.g., due to sandbox violation.
+  LoadEmptyPage(url.mojom.Url resource_url);
+
   // Called by the browser to request a BeforeUnloadControl interface pointer
   // which will later be connected to the request from the extension page to
   // provide the beforeunload API (to setup beforeunload in the embedder

diff --git a/extensions/renderer/guest_view/mime_handler_view/mime_handler_view_container_manager.cc b/extensions/renderer/guest_view/mime_handler_view/mime_handler_view_container_manager.cc
@@ -163,6 +163,14 @@ void MimeHandlerViewContainerManager::SetInternalId(
   internal_id_ = token_id;
 }
 
+void MimeHandlerViewContainerManager::LoadEmptyPage(const GURL& resource_url) {
+  // TODO(ekaramad): Add test coverage to ensure document ends up empty (see
+  // https://crbug.com/968350).
+  render_frame()->LoadHTMLString("<!doctype html>", resource_url, "UTF-8",
+                                 resource_url, true /* replace_current_item */);
+  GetRenderFrameMap()->erase(render_frame()->GetRoutingID());
+}
+
 void MimeHandlerViewContainerManager::CreateBeforeUnloadControl(
     CreateBeforeUnloadControlCallback callback) {
   if (!post_message_support_)

diff --git a/extensions/renderer/guest_view/mime_handler_view/mime_handler_view_container_manager.h b/extensions/renderer/guest_view/mime_handler_view/mime_handler_view_container_manager.h
@@ -94,6 +94,7 @@ class MimeHandlerViewContainerManager
 
   // mojom::MimeHandlerViewContainerManager overrides.
   void SetInternalId(const std::string& token_id) override;
+  void LoadEmptyPage(const GURL& resource_url) override;
   void CreateBeforeUnloadControl(
       CreateBeforeUnloadControlCallback callback) override;
   void DestroyFrameContainer(int32_t element_instance_id) override;

diff --git a/third_party/blink/renderer/core/html/html_plugin_element.cc b/third_party/blink/renderer/core/html/html_plugin_element.cc
@@ -180,6 +180,7 @@ bool HTMLPlugInElement::RequestObjectInternal(
   ObjectContentType object_type = GetObjectContentType();
   bool handled_externally =
       object_type == ObjectContentType::kExternalPlugin &&
+      AllowedToLoadPlugin(completed_url, service_type_) &&
       GetDocument().GetFrame()->Client()->IsPluginHandledExternally(
           *this, completed_url,
           service_type_.IsEmpty() ? GetMIMETypeFromURL(completed_url)