Move verification of OpenURL_Params and DownloadURL_Params to util func.
This CL deduplicates verification code that was present in
- RenderFrameProxyHost::OnOpenURL
- RenderFrameHostImpl::OnOpenURL
- RenderFrameMessageFilter::OnDownloadUrl
The duplicated code is moved into new util functions. The new functions
will be tweaked in the future to add more verification (e.g. to verify
|initiator_origin|).
Bug: 919144
Change-Id: I50515c42d7f5003dd1f782ba59349599dca0f88d
Reviewed-on: https://chromium-review.googlesource.com/c/1395886
Reviewed-by: Nasko Oskov <nasko@chromium.org>
Commit-Queue: Łukasz Anforowicz <lukasza@chromium.org>
Cr-Commit-Position: refs/heads/master@{#626207}diff --git a/content/browser/BUILD.gn b/content/browser/BUILD.gn
index 2ececf6..c4d780c 100644
--- a/content/browser/BUILD.gn
+++ b/content/browser/BUILD.gn
@@ -868,6 +868,8 @@
"frame_host/interstitial_page_impl.h",
"frame_host/interstitial_page_navigator_impl.cc",
"frame_host/interstitial_page_navigator_impl.h",
+ "frame_host/ipc_utils.cc",
+ "frame_host/ipc_utils.h",
"frame_host/keep_alive_handle_factory.cc",
"frame_host/keep_alive_handle_factory.h",
"frame_host/mixed_content_navigation_throttle.cc",
diff --git a/content/browser/bad_message.h b/content/browser/bad_message.h
index 5dcf68ec..03b16dde 100644
--- a/content/browser/bad_message.h
+++ b/content/browser/bad_message.h
@@ -195,9 +195,9 @@
RWH_INVALID_FRAME_TOKEN = 167,
RWH_BAD_FRAME_SINK_REQUEST = 168,
RWH_SURFACE_INVARIANTS_VIOLATION = 169,
- RFH_ILLEGAL_UPLOAD_PARAMS = 170,
+ ILLEGAL_UPLOAD_PARAMS = 170,
RFH_BASE_URL_FOR_DATA_URL_SPECIFIED = 171,
- RFPH_ILLEGAL_UPLOAD_PARAMS = 172,
+ OBSOLETE_RFPH_ILLEGAL_UPLOAD_PARAMS = 172,
OBSOLETE_SWDH_PROVIDER_CREATED_ILLEGAL_TYPE = 173,
OBSOLETE_SWDH_PROVIDER_CREATED_ILLEGAL_TYPE_NOT_WINDOW = 174,
OBSOLETE_SWDH_PROVIDER_CREATED_ILLEGAL_TYPE_SERVICE_WORKER = 175,
@@ -226,10 +226,10 @@
SYNC_COMPOSITOR_NO_FUTURE_FRAME = 198,
SYNC_COMPOSITOR_NO_BEGIN_FRAME = 199,
WEBUI_BAD_HOST_ACCESS = 200,
- RFMF_BLOB_URL_TOKEN_FOR_NON_BLOB_URL = 201,
+ OBSOLETE_RFMF_BLOB_URL_TOKEN_FOR_NON_BLOB_URL = 201,
PERMISSION_SERVICE_BAD_PERMISSION_DESCRIPTOR = 202,
- RFH_BLOB_URL_TOKEN_FOR_NON_BLOB_URL = 203,
- RFPH_BLOB_URL_TOKEN_FOR_NON_BLOB_URL = 204,
+ BLOB_URL_TOKEN_FOR_NON_BLOB_URL = 203,
+ OBSOLETE_RFPH_BLOB_URL_TOKEN_FOR_NON_BLOB_URL = 204,
RFH_ERROR_PROCESS_NON_ERROR_COMMIT = 205,
RFH_ERROR_PROCESS_NON_UNIQUE_ORIGIN_COMMIT = 206,
RFH_CANNOT_RENDER_FALLBACK_CONTENT = 207,
diff --git a/content/browser/cross_site_transfer_browsertest.cc b/content/browser/cross_site_transfer_browsertest.cc
index 2bb9bfd..62bcf49 100644
--- a/content/browser/cross_site_transfer_browsertest.cc
+++ b/content/browser/cross_site_transfer_browsertest.cc
@@ -418,7 +418,7 @@
"setTimeout(\n"
" function() { document.getElementById('file-form').submit(); },\n"
" 0);"));
- EXPECT_EQ(bad_message::RFPH_ILLEGAL_UPLOAD_PARAMS, kill_waiter.Wait());
+ EXPECT_EQ(bad_message::ILLEGAL_UPLOAD_PARAMS, kill_waiter.Wait());
// The target frame should still be at the original location - the malicious
// navigation should have been stopped.
diff --git a/content/browser/frame_host/ipc_utils.cc b/content/browser/frame_host/ipc_utils.cc
new file mode 100644
index 0000000..1530444
--- /dev/null
+++ b/content/browser/frame_host/ipc_utils.cc
@@ -0,0 +1,114 @@
+// Copyright 2019 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#include "content/browser/frame_host/ipc_utils.h"
+
+#include <utility>
+
+#include "content/browser/bad_message.h"
+#include "content/browser/blob_storage/chrome_blob_storage_context.h"
+#include "content/browser/child_process_security_policy_impl.h"
+#include "content/common/frame.mojom.h"
+#include "content/common/frame_messages.h"
+#include "content/public/browser/browser_thread.h"
+#include "content/public/browser/render_process_host.h"
+#include "content/public/browser/site_instance.h"
+#include "content/public/common/child_process_host.h"
+#include "mojo/public/cpp/system/message_pipe.h"
+
+namespace content {
+
+namespace {
+
+bool VerifyBlobToken(
+ int process_id,
+ mojo::MessagePipeHandle received_token,
+ const GURL& received_url,
+ blink::mojom::BlobURLTokenPtrInfo* out_blob_url_token_info) {
+ DCHECK_NE(ChildProcessHost::kInvalidUniqueID, process_id);
+ DCHECK(out_blob_url_token_info);
+
+ mojo::ScopedMessagePipeHandle blob_url_token_handle(
+ std::move(received_token));
+ blink::mojom::BlobURLTokenPtrInfo blob_url_token_info(
+ std::move(blob_url_token_handle), blink::mojom::BlobURLToken::Version_);
+ if (blob_url_token_info) {
+ if (!received_url.SchemeIsBlob()) {
+ bad_message::ReceivedBadMessage(
+ process_id, bad_message::BLOB_URL_TOKEN_FOR_NON_BLOB_URL);
+ return false;
+ }
+ }
+
+ *out_blob_url_token_info = std::move(blob_url_token_info);
+ return true;
+}
+
+} // namespace
+
+bool VerifyDownloadUrlParams(
+ int process_id,
+ const FrameHostMsg_DownloadUrl_Params& params,
+ blink::mojom::BlobURLTokenPtrInfo* out_blob_url_token_info) {
+ DCHECK(BrowserThread::CurrentlyOn(BrowserThread::IO) ||
+ BrowserThread::CurrentlyOn(BrowserThread::UI));
+ DCHECK_NE(ChildProcessHost::kInvalidUniqueID, process_id);
+ DCHECK(out_blob_url_token_info);
+
+ // Verify |params.blob_url_token| and populate |out_blob_url_token_info|.
+ if (!VerifyBlobToken(process_id, params.blob_url_token, params.url,
+ out_blob_url_token_info)) {
+ return false;
+ }
+
+ // TODO(lukasza): Verify |params.initiator_origin|.
+
+ return true;
+}
+
+bool VerifyOpenURLParams(SiteInstance* site_instance,
+ const FrameHostMsg_OpenURL_Params& params,
+ GURL* out_validated_url,
+ scoped_refptr<network::SharedURLLoaderFactory>*
+ out_blob_url_loader_factory) {
+ DCHECK_CURRENTLY_ON(BrowserThread::UI);
+ DCHECK(site_instance);
+ DCHECK(out_validated_url);
+ DCHECK(out_blob_url_loader_factory);
+ RenderProcessHost* process = site_instance->GetProcess();
+ int process_id = process->GetID();
+
+ // Verify |params.url| and populate |out_validated_url|.
+ *out_validated_url = params.url;
+ process->FilterURL(false, out_validated_url);
+
+ // Verify |params.blob_url_token| and populate |out_blob_url_loader_factory|.
+ blink::mojom::BlobURLTokenPtrInfo blob_url_token_info;
+ if (!VerifyBlobToken(process_id, params.blob_url_token, params.url,
+ &blob_url_token_info)) {
+ return false;
+ }
+ if (blob_url_token_info) {
+ blink::mojom::BlobURLTokenPtr blob_url_token(
+ std::move(blob_url_token_info));
+ *out_blob_url_loader_factory =
+ ChromeBlobStorageContext::URLLoaderFactoryForToken(
+ process->GetBrowserContext(), std::move(blob_url_token));
+ }
+
+ // Verify |params.resource_request_body|.
+ auto* policy = ChildProcessSecurityPolicyImpl::GetInstance();
+ if (!policy->CanReadRequestBody(site_instance,
+ params.resource_request_body)) {
+ bad_message::ReceivedBadMessage(process,
+ bad_message::ILLEGAL_UPLOAD_PARAMS);
+ return false;
+ }
+
+ // TODO(lukasza): Verify |params.initiator_origin|.
+
+ return true;
+}
+
+} // namespace content
diff --git a/content/browser/frame_host/ipc_utils.h b/content/browser/frame_host/ipc_utils.h
new file mode 100644
index 0000000..5263264
--- /dev/null
+++ b/content/browser/frame_host/ipc_utils.h
@@ -0,0 +1,53 @@
+// Copyright 2019 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#ifndef CONTENT_BROWSER_FRAME_HOST_IPC_UTILS_H_
+#define CONTENT_BROWSER_FRAME_HOST_IPC_UTILS_H_
+
+#include "base/memory/ref_counted.h"
+#include "content/common/frame.mojom.h"
+#include "services/network/public/cpp/shared_url_loader_factory.h"
+#include "url/gurl.h"
+
+struct FrameHostMsg_DownloadUrl_Params;
+struct FrameHostMsg_OpenURL_Params;
+
+namespace content {
+
+class SiteInstance;
+
+// Verifies that |params| are valid and can be accessed by |process_id|.
+//
+// Returns true if the |params| are valid. As a side-effect of the verification
+// |out_blob_url_token_info| will be populated.
+//
+// Terminates the renderer with the given |process_id| and returns false if the
+// |params| are invalid.
+//
+// This function may be called on either the IO thread or the UI thread
+// (but not on other threads).
+bool VerifyDownloadUrlParams(
+ int process_id,
+ const FrameHostMsg_DownloadUrl_Params& params,
+ blink::mojom::BlobURLTokenPtrInfo* out_blob_url_token_info);
+
+// Verifies that |params| are valid and can be accessed by the renderer process
+// associated with |site_instance|.
+//
+// Returns true if the |params| are valid. As a side-effect of the verification
+// |out_validated_url| and |out_blob_url_loader_factory| will be populated.
+//
+// Terminates the renderer the process associated with |site_instance| and
+// returns false if the |params| are invalid.
+//
+// This function has to be called on the UI thread.
+bool VerifyOpenURLParams(SiteInstance* site_instance,
+ const FrameHostMsg_OpenURL_Params& params,
+ GURL* out_validated_url,
+ scoped_refptr<network::SharedURLLoaderFactory>*
+ out_blob_url_loader_factory);
+
+} // namespace content
+
+#endif // CONTENT_BROWSER_FRAME_HOST_IPC_UTILS_H_
diff --git a/content/browser/frame_host/render_frame_host_impl.cc b/content/browser/frame_host/render_frame_host_impl.cc
index 7c27281..0a92476 100644
--- a/content/browser/frame_host/render_frame_host_impl.cc
+++ b/content/browser/frame_host/render_frame_host_impl.cc
@@ -47,6 +47,7 @@
#include "content/browser/frame_host/frame_tree.h"
#include "content/browser/frame_host/frame_tree_node.h"
#include "content/browser/frame_host/input/input_injector_impl.h"
+#include "content/browser/frame_host/ipc_utils.h"
#include "content/browser/frame_host/keep_alive_handle_factory.h"
#include "content/browser/frame_host/navigation_entry_impl.h"
#include "content/browser/frame_host/navigation_handle_impl.h"
@@ -2061,29 +2062,11 @@
}
void RenderFrameHostImpl::OnOpenURL(const FrameHostMsg_OpenURL_Params& params) {
- GURL validated_url(params.url);
- GetProcess()->FilterURL(false, &validated_url);
-
- mojo::ScopedMessagePipeHandle blob_url_token_handle(params.blob_url_token);
- blink::mojom::BlobURLTokenPtr blob_url_token(
- blink::mojom::BlobURLTokenPtrInfo(std::move(blob_url_token_handle),
- blink::mojom::BlobURLToken::Version_));
+ // Verify and unpack IPC payload.
+ GURL validated_url;
scoped_refptr<network::SharedURLLoaderFactory> blob_url_loader_factory;
- if (blob_url_token) {
- if (!params.url.SchemeIsBlob()) {
- bad_message::ReceivedBadMessage(
- GetProcess(), bad_message::RFH_BLOB_URL_TOKEN_FOR_NON_BLOB_URL);
- return;
- }
- blob_url_loader_factory =
- ChromeBlobStorageContext::URLLoaderFactoryForToken(
- GetSiteInstance()->GetBrowserContext(), std::move(blob_url_token));
- }
-
- if (!ChildProcessSecurityPolicyImpl::GetInstance()->CanReadRequestBody(
- GetSiteInstance(), params.resource_request_body)) {
- bad_message::ReceivedBadMessage(GetProcess(),
- bad_message::RFH_ILLEGAL_UPLOAD_PARAMS);
+ if (!VerifyOpenURLParams(GetSiteInstance(), params, &validated_url,
+ &blob_url_loader_factory)) {
return;
}
@@ -3910,7 +3893,7 @@
if (!ChildProcessSecurityPolicyImpl::GetInstance()->CanReadRequestBody(
GetSiteInstance(), validated_params.post_data)) {
bad_message::ReceivedBadMessage(GetProcess(),
- bad_message::RFH_ILLEGAL_UPLOAD_PARAMS);
+ bad_message::ILLEGAL_UPLOAD_PARAMS);
return;
}
diff --git a/content/browser/frame_host/render_frame_message_filter.cc b/content/browser/frame_host/render_frame_message_filter.cc
index 15cd7e5a..e17622d6 100644
--- a/content/browser/frame_host/render_frame_message_filter.cc
+++ b/content/browser/frame_host/render_frame_message_filter.cc
@@ -22,6 +22,7 @@
#include "content/browser/bad_message.h"
#include "content/browser/blob_storage/chrome_blob_storage_context.h"
#include "content/browser/child_process_security_policy_impl.h"
+#include "content/browser/frame_host/ipc_utils.h"
#include "content/browser/frame_host/render_frame_host_impl.h"
#include "content/browser/gpu/gpu_data_manager_impl.h"
#include "content/browser/renderer_host/render_widget_helper.h"
@@ -518,14 +519,9 @@
void RenderFrameMessageFilter::OnDownloadUrl(
const FrameHostMsg_DownloadUrl_Params& params) {
- mojo::ScopedMessagePipeHandle blob_url_token_handle(params.blob_url_token);
- blink::mojom::BlobURLTokenPtrInfo blob_url_token(
- std::move(blob_url_token_handle), blink::mojom::BlobURLToken::Version_);
- if (blob_url_token && !params.url.SchemeIsBlob()) {
- bad_message::ReceivedBadMessage(
- this, bad_message::RFMF_BLOB_URL_TOKEN_FOR_NON_BLOB_URL);
+ blink::mojom::BlobURLTokenPtrInfo blob_url_token;
+ if (!VerifyDownloadUrlParams(render_process_id_, params, &blob_url_token))
return;
- }
DownloadUrl(params.render_view_id, params.render_frame_id, params.url,
params.referrer, params.initiator_origin, params.suggested_name,
diff --git a/content/browser/frame_host/render_frame_proxy_host.cc b/content/browser/frame_host/render_frame_proxy_host.cc
index f075640d..e70b7e4 100644
--- a/content/browser/frame_host/render_frame_proxy_host.cc
+++ b/content/browser/frame_host/render_frame_proxy_host.cc
@@ -17,6 +17,7 @@
#include "content/browser/frame_host/cross_process_frame_connector.h"
#include "content/browser/frame_host/frame_tree.h"
#include "content/browser/frame_host/frame_tree_node.h"
+#include "content/browser/frame_host/ipc_utils.h"
#include "content/browser/frame_host/navigator.h"
#include "content/browser/frame_host/render_frame_host_delegate.h"
#include "content/browser/renderer_host/render_view_host_impl.h"
@@ -283,23 +284,12 @@
void RenderFrameProxyHost::OnOpenURL(
const FrameHostMsg_OpenURL_Params& params) {
- GURL validated_url(params.url);
- GetProcess()->FilterURL(false, &validated_url);
-
- mojo::ScopedMessagePipeHandle blob_url_token_handle(params.blob_url_token);
- blink::mojom::BlobURLTokenPtr blob_url_token(
- blink::mojom::BlobURLTokenPtrInfo(std::move(blob_url_token_handle),
- blink::mojom::BlobURLToken::Version_));
+ // Verify and unpack IPC payload.
+ GURL validated_url;
scoped_refptr<network::SharedURLLoaderFactory> blob_url_loader_factory;
- if (blob_url_token) {
- if (!params.url.SchemeIsBlob()) {
- bad_message::ReceivedBadMessage(
- GetProcess(), bad_message::RFPH_BLOB_URL_TOKEN_FOR_NON_BLOB_URL);
- return;
- }
- blob_url_loader_factory =
- ChromeBlobStorageContext::URLLoaderFactoryForToken(
- GetSiteInstance()->GetBrowserContext(), std::move(blob_url_token));
+ if (!VerifyOpenURLParams(GetSiteInstance(), params, &validated_url,
+ &blob_url_loader_factory)) {
+ return;
}
// Verify that we are in the same BrowsingInstance as the current
@@ -308,15 +298,6 @@
if (!site_instance_->IsRelatedSiteInstance(current_rfh->GetSiteInstance()))
return;
- // Verify if the request originator (*not* |current_rfh|) has access to the
- // contents of the POST body.
- if (!ChildProcessSecurityPolicyImpl::GetInstance()->CanReadRequestBody(
- GetSiteInstance(), params.resource_request_body)) {
- bad_message::ReceivedBadMessage(GetProcess(),
- bad_message::RFPH_ILLEGAL_UPLOAD_PARAMS);
- return;
- }
-
// Since this navigation targeted a specific RenderFrameProxy, it should stay
// in the current tab.
DCHECK_EQ(WindowOpenDisposition::CURRENT_TAB, params.disposition);
diff --git a/content/browser/navigation_browsertest.cc b/content/browser/navigation_browsertest.cc
index 513ab44..0dadf66 100644
--- a/content/browser/navigation_browsertest.cc
+++ b/content/browser/navigation_browsertest.cc
@@ -565,7 +565,7 @@
"document.getElementById('file-form').submit();",
&result));
EXPECT_TRUE(result);
- EXPECT_EQ(bad_message::RFH_ILLEGAL_UPLOAD_PARAMS, process_kill_waiter.Wait());
+ EXPECT_EQ(bad_message::ILLEGAL_UPLOAD_PARAMS, process_kill_waiter.Wait());
}
// Test case to verify that redirects to data: URLs are properly disallowed,
diff --git a/content/browser/security_exploit_browsertest.cc b/content/browser/security_exploit_browsertest.cc
index 71247da..c6a41c4 100644
--- a/content/browser/security_exploit_browsertest.cc
+++ b/content/browser/security_exploit_browsertest.cc
@@ -744,7 +744,7 @@
EXPECT_EQ(start_url, root->current_frame_host()->GetLastCommittedURL());
// Verify that the malicious renderer got killed.
- EXPECT_EQ(bad_message::RFH_ILLEGAL_UPLOAD_PARAMS, kill_waiter.Wait());
+ EXPECT_EQ(bad_message::ILLEGAL_UPLOAD_PARAMS, kill_waiter.Wait());
}
// Test that forging a frame's unique name and commit won't allow changing the
diff --git a/tools/metrics/histograms/enums.xml b/tools/metrics/histograms/enums.xml
index 93f10f9..4c17ccfd 100644
--- a/tools/metrics/histograms/enums.xml
+++ b/tools/metrics/histograms/enums.xml
@@ -3805,9 +3805,9 @@
<int value="167" label="RWH_INVALID_FRAME_TOKEN"/>
<int value="168" label="RWH_BAD_FRAME_SINK_REQUEST"/>
<int value="169" label="RWH_SURFACE_INVARIANTS_VIOLATION"/>
- <int value="170" label="RFH_ILLEGAL_UPLOAD_PARAMS"/>
+ <int value="170" label="ILLEGAL_UPLOAD_PARAMS"/>
<int value="171" label="RFH_BASE_URL_FOR_DATA_URL_SPECIFIED"/>
- <int value="172" label="RFPH_ILLEGAL_UPLOAD_PARAMS"/>
+ <int value="172" label="OBSOLETE_RFPH_ILLEGAL_UPLOAD_PARAMS"/>
<int value="173" label="OBSOLETE_SWDH_PROVIDER_CREATED_ILLEGAL_TYPE"/>
<int value="174"
label="OBSOLETE_SWDH_PROVIDER_CREATED_ILLEGAL_TYPE_NOT_WINDOW"/>
@@ -3840,10 +3840,10 @@
<int value="198" label="SYNC_COMPOSITOR_NO_FUTURE_FRAME"/>
<int value="199" label="SYNC_COMPOSITOR_NO_BEGIN_FRAME"/>
<int value="200" label="WEBUI_BAD_HOST_ACCESS"/>
- <int value="201" label="RFMF_BLOB_URL_TOKEN_FOR_NON_BLOB_URL"/>
+ <int value="201" label="OBSOLETE_RFMF_BLOB_URL_TOKEN_FOR_NON_BLOB_URL"/>
<int value="202" label="PERMISSION_SERVICE_BAD_PERMISSION_DESCRIPTOR"/>
- <int value="203" label="RFH_BLOB_URL_TOKEN_FOR_NON_BLOB_URL"/>
- <int value="204" label="RFPH_BLOB_URL_TOKEN_FOR_NON_BLOB_URL"/>
+ <int value="203" label="BLOB_URL_TOKEN_FOR_NON_BLOB_URL"/>
+ <int value="204" label="OBSOLETE_RFPH_BLOB_URL_TOKEN_FOR_NON_BLOB_URL"/>
<int value="205" label="RFH_ERROR_PROCESS_NON_ERROR_COMMIT"/>
<int value="206" label="RFH_ERROR_PROCESS_NON_UNIQUE_ORIGIN_COMMIT"/>
<int value="207" label="RFH_CANNOT_RENDER_FALLBACK_CONTENT"/>
