RenderFrameProxyHost::OnOpenURL needs to validate resource request body.

content/browser/frame_host/render_frame_proxy_host.cc

Index: content/browser/frame_host/render_frame_proxy_host.cc
diff --git a/content/browser/frame_host/render_frame_proxy_host.cc b/content/browser/frame_host/render_frame_proxy_host.cc
index 1107e77fe4508b99b5a2f3446bf766bf59c7ad7d..ddc32830d23d00e3c6dc934d42698d95a77012dc 100644
--- a/content/browser/frame_host/render_frame_proxy_host.cc
+++ b/content/browser/frame_host/render_frame_proxy_host.cc
@@ -8,6 +8,7 @@
 
 #include "base/lazy_instance.h"
 #include "content/browser/bad_message.h"
+#include "content/browser/child_process_security_policy_impl.h"
 #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"
@@ -19,8 +20,12 @@
 #include "content/browser/site_instance_impl.h"
 #include "content/common/frame_messages.h"
 #include "content/common/frame_owner_properties.h"
+#include "content/common/resource_request_body_impl.h"
+#include "content/public/browser/browser_context.h"
 #include "content/public/browser/browser_thread.h"
+#include "content/public/browser/storage_partition.h"
 #include "ipc/ipc_message.h"
+#include "storage/browser/fileapi/file_system_context.h"
 
 namespace content {
 
@@ -32,8 +37,66 @@ typedef base::hash_map<RenderFrameProxyHostID, RenderFrameProxyHost*>
     RoutingIDFrameProxyMap;
 base::LazyInstance<RoutingIDFrameProxyMap>::DestructorAtExit
     g_routing_id_frame_proxy_map = LAZY_INSTANCE_INITIALIZER;
+
+// TODO(lukasza): https://crbug.com/726067: Remove code duplication - the
+// function below should be reused by RenderFrameHostImpl::OnBeginNavigation and
+// ResourceDispatcherHostImpl::ShouldServiceRequest.

[Łukasz Anforowicz, 2017/05/25 15:46:37]

I've duplicated the code, so that this CL is (hopefully) easier to merge
upstream.  I am trying to deduplicate in a follow-up CL @
https://codereview.chromium.org/2905763002, but this will need some discussion
about content/browser/loader/DEPS

+bool CanReadRequestBody(SiteInstance* site_instance,
+                        const scoped_refptr<ResourceRequestBodyImpl>& body) {
+  if (!body)
+    return true;
+
+  ChildProcessSecurityPolicyImpl* security_policy =
+      ChildProcessSecurityPolicyImpl::GetInstance();
+  int child_id = site_instance->GetProcess()->GetID();
+
+  StoragePartition* storage_partition = BrowserContext::GetStoragePartition(
+      site_instance->GetBrowserContext(), site_instance);
+  const storage::FileSystemContext* file_system_context =
+      storage_partition->GetFileSystemContext();
+
+  for (const ResourceRequestBodyImpl::Element& element : *body->elements()) {
+    switch (element.type()) {

[alexmos, 2017/05/25 18:17:34]

Thanks for being thorough and investigating all the element types!

+      case ResourceRequestBodyImpl::Element::TYPE_FILE:
+        if (!security_policy->CanReadFile(child_id, element.path()))
+          return false;
+        break;
+
+      case ResourceRequestBodyImpl::Element::TYPE_FILE_FILESYSTEM:
+        if (!security_policy->CanReadFileSystemFile(
+                child_id,
+                file_system_context->CrackURL(element.filesystem_url())))
+          return false;
+        break;
+
+      case ResourceRequestBodyImpl::Element::TYPE_DISK_CACHE_ENTRY:
+        // TYPE_DISK_CACHE_ENTRY can't be sent via IPC according to
+        // content/common/resource_messages.cc
+        NOTREACHED();
+        return false;
+
+      case ResourceRequestBodyImpl::Element::TYPE_BYTES:
+      case ResourceRequestBodyImpl::Element::TYPE_BYTES_DESCRIPTION:
+        // Data is self-contained within |body| - no need to check access.
+        break;
+
+      case ResourceRequestBodyImpl::Element::TYPE_BLOB:
+        // No need to validate - the unguessability of the uuid of the blob is a
+        // sufficient defense against access from an unrelated renderer.
+        break;
+
+      case ResourceRequestBodyImpl::Element::TYPE_UNKNOWN:
+      default:
+        // Fail safe - deny access.
+        NOTREACHED();
+        return false;
+    }
+  }
+  return true;
 }
 
+}  // namespace
+
 // static
 RenderFrameProxyHost* RenderFrameProxyHost::FromID(int process_id,
                                                    int routing_id) {
@@ -250,6 +313,14 @@ void RenderFrameProxyHost::OnOpenURL(
   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 (!CanReadRequestBody(GetSiteInstance(), params.resource_request_body)) {

[alexmos, 2017/05/25 18:17:34]

Sanity check: is the same check needed in RenderFrameHostImpl::OnOpenURL?  I
think that path also eventually hits NavigateToEntry -> Navigate ->
UpdatePermissionsForNavigation and grants file access.  Or is that validated
somewhere else on that path?  The validation in
RenderFrameHostImpl::ValidateUploadParams() is only called by
RenderFrameHostImpl::OnBeginNavigation, which is only for PlzNavigate.

[Łukasz Anforowicz, 2017/05/25 19:56:01]

Thanks for catching this.  Done.

+    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);