BadMessage for invalid plugin frame ID
(Follow-up work to CL:1213369).
Currently API in guest_view.mojom send two routing IDs to the browser:
the embedder frame routing ID (the frame which adds an <embed>/<object>)
and the plugin frame ID (the actual frame inside <embed>/<object>). This
is to support both a frame-based MimeHandlerView (which only needs the
plugin frame ID), and the BrowserPlugin-based version which uses the
embedder frame ID.
This CL adds a security check on the browser side to verify the reported
routing IDs make sense, i.e., when they point to actual RFH we always
observer the child and parent relationship. This of course is not the
case in BP-based case where we expect the child routing ID to be invalid
all the time.
Bug: 659750
Change-Id: I92f1643303899054cdee36897e7a973a5a3b3d85
Reviewed-on: https://chromium-review.googlesource.com/1217704
Reviewed-by: Istiaque Ahmed <lazyboy@chromium.org>
Reviewed-by: Ehsan Karamad <ekaramad@chromium.org>
Reviewed-by: Alex Moshchuk <alexmos@chromium.org>
Commit-Queue: Ehsan Karamad <ekaramad@chromium.org>
Cr-Commit-Position: refs/heads/master@{#590668}diff --git a/extensions/browser/bad_message.h b/extensions/browser/bad_message.h
index bf7fe1f..d93599e 100644
--- a/extensions/browser/bad_message.h
+++ b/extensions/browser/bad_message.h
@@ -35,6 +35,7 @@
EFD_BAD_MESSAGE_WORKER = 9,
WVG_PARTITION_ID_NOT_UTF8 = 10,
ESWMF_BAD_EVENT_ACK = 11,
+ MHVG_INVALID_PLUGIN_FRAME_ID = 12,
// Please add new elements here. The naming convention is abbreviated class
// name (e.g. ExtensionHost becomes EH) plus a unique description of the
// reason. After making changes, you MUST update histograms.xml by running:
diff --git a/extensions/browser/guest_view/extensions_guest_view_message_filter.cc b/extensions/browser/guest_view/extensions_guest_view_message_filter.cc
index 21ecbf1..172e6de 100644
--- a/extensions/browser/guest_view/extensions_guest_view_message_filter.cc
+++ b/extensions/browser/guest_view/extensions_guest_view_message_filter.cc
@@ -16,6 +16,7 @@
#include "content/public/browser/web_contents.h"
#include "content/public/common/mime_handler_view_mode.h"
#include "extensions/browser/api/extensions_api_client.h"
+#include "extensions/browser/bad_message.h"
#include "extensions/browser/extension_registry.h"
#include "extensions/browser/guest_view/mime_handler_view/mime_handler_stream_manager.h"
#include "extensions/browser/guest_view/mime_handler_view/mime_handler_view_constants.h"
@@ -38,6 +39,40 @@
namespace extensions {
+namespace {
+
+// TODO(ekaramad): Remove this once MimeHandlerViewGuest has fully migrated to
+// using cross-process-frames.
+// Returns true if |child_routing_id| corresponds to a frame which is a direct
+// child of |parent_rfh|.
+bool AreRoutingIDsConsistent(content::RenderFrameHost* parent_rfh,
+ int32_t child_routing_id) {
+ const bool uses_cross_process_frame =
+ content::MimeHandlerViewMode::UsesCrossProcessFrame();
+ const bool is_child_routing_id_none = (child_routing_id == MSG_ROUTING_NONE);
+
+ // For cross-process-frame MimeHandlerView, |child_routing_id| cannot be none.
+ bool should_shutdown_process =
+ (is_child_routing_id_none == uses_cross_process_frame);
+
+ if (!should_shutdown_process && uses_cross_process_frame) {
+ // The |child_routing_id| is the routing ID of either a RenderFrame or a
+ // proxy in the |parent_rfh|. Therefore, to get the associated RFH we need
+ // to go through the FTN first.
+ int32_t child_ftn_id =
+ content::RenderFrameHost::GetFrameTreeNodeIdForRoutingId(
+ parent_rfh->GetProcess()->GetID(), child_routing_id);
+ // The |child_rfh| is not really used; it is retrieved to verify whether or
+ // not what the renderer process says makes any sense.
+ auto* child_rfh = content::WebContents::FromRenderFrameHost(parent_rfh)
+ ->UnsafeFindFrameByFrameTreeNodeId(child_ftn_id);
+ should_shutdown_process =
+ child_rfh && (child_rfh->GetParent() != parent_rfh);
+ }
+ return !should_shutdown_process;
+}
+
+} // namespace
const uint32_t ExtensionsGuestViewMessageFilter::kFilteredMessageClasses[] = {
GuestViewMsgStart, ExtensionsGuestViewMsgStart};
@@ -128,8 +163,7 @@
int32_t plugin_frame_routing_id,
bool is_full_page_plugin) {
DCHECK_CURRENTLY_ON(BrowserThread::UI);
- DCHECK(content::MimeHandlerViewMode::UsesCrossProcessFrame() ||
- plugin_frame_routing_id == MSG_ROUTING_NONE);
+
auto* manager = GetOrCreateGuestViewManager();
auto* rfh = RenderFrameHost::FromID(render_process_id_, render_frame_id);
@@ -137,6 +171,12 @@
if (!embedder_web_contents)
return;
+ if (!AreRoutingIDsConsistent(rfh, plugin_frame_routing_id)) {
+ bad_message::ReceivedBadMessage(rfh->GetProcess(),
+ bad_message::MHVG_INVALID_PLUGIN_FRAME_ID);
+ return;
+ }
+
GuestViewManager::WebContentsCreatedCallback callback = base::BindOnce(
&ExtensionsGuestViewMessageFilter::MimeHandlerViewGuestCreatedCallback,
this, element_instance_id, render_process_id_, render_frame_id,
diff --git a/tools/metrics/histograms/enums.xml b/tools/metrics/histograms/enums.xml
index 145e5b5f..294b7b0 100644
--- a/tools/metrics/histograms/enums.xml
+++ b/tools/metrics/histograms/enums.xml
@@ -3453,6 +3453,7 @@
<int value="9" label="EFD_BAD_MESSAGE_WORKER"/>
<int value="10" label="WVG_PARTITION_ID_NOT_UTF8"/>
<int value="11" label="ESWMF_BAD_EVENT_ACK"/>
+ <int value="12" label="MHVG_INVALID_PLUGIN_FRAME_ID"/>
</enum>
<enum name="BadMessageReasonGuestView">
