Fixing renderer's access to a file from HTTP POST (after a xsite transfer).

content/browser/frame_host/render_frame_host_impl.cc

 // Copyright 2013 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/render_frame_host_impl.h"
 
 #include <utility>
 
 #include "base/bind.h"
 #include "base/command_line.h"
@@ skipping 2739 matching lines @@
   if (!permission_manager)
     return;
 
   permission_manager->RegisterPermissionUsage(
       PermissionType::GEOLOCATION,
       last_committed_url().GetOrigin(),
       frame_tree_node()->frame_tree()->GetMainFrame()
           ->last_committed_url().GetOrigin());
 }
 
+void RenderFrameHostImpl::GrantFileAccessFromResourceRequestBody(
+    const ResourceRequestBody& body) {
+  ChildProcessSecurityPolicyImpl* policy =
+      ChildProcessSecurityPolicyImpl::GetInstance();
+
+  std::vector<base::FilePath> file_paths = body.GetReferencedFiles();
+  for (const auto& file : file_paths) {
+    if (!policy->CanReadFile(GetProcess()->GetID(), file))
+      policy->GrantReadFile(GetProcess()->GetID(), file);
+  }
+}
+
 void RenderFrameHostImpl::UpdatePermissionsForNavigation(
     const CommonNavigationParams& common_params,
     const RequestNavigationParams& request_params) {
   // Browser plugin guests are not allowed to navigate outside web-safe schemes,
   // so do not grant them the ability to request additional URLs.
   if (!GetProcess()->IsForGuestsOnly()) {
     ChildProcessSecurityPolicyImpl::GetInstance()->GrantRequestURL(
         GetProcess()->GetID(), common_params.url);
     if (common_params.url.SchemeIs(url::kDataScheme) &&
         common_params.base_url_for_data_url.SchemeIs(url::kFileScheme)) {
       // If 'data:' is used, and we have a 'file:' base url, grant access to
       // local files.
       ChildProcessSecurityPolicyImpl::GetInstance()->GrantRequestURL(
           GetProcess()->GetID(), common_params.base_url_for_data_url);
     }
   }
 
   // We may be returning to an existing NavigationEntry that had been granted
   // file access.  If this is a different process, we will need to grant the
-  // access again.  The files listed in the page state are validated when they
-  // are received from the renderer to prevent abuse.
-  if (request_params.page_state.IsValid()) {
+  // access again.  Abuse is prevented, because the files listed in the page
+  // state are validated earlier, when they are received from the renderer (in
+  // RenderFrameHostImpl::CanAccessFilesOfPageState).
+  if (request_params.page_state.IsValid())
     render_view_host_->GrantFileAccessFromPageState(request_params.page_state);

[Charlie Reis, 2016/06/16 20:18:00]

Seems a bit unfortunate we have to do this twice, once here and once for the
post_data in the params, but I suppose that's necessary (for history navigations
vs transfers).

That said, can we use a shared RFHI::GrantFileAccess(file_paths) helper function
for both cases, where we pass in request_params.page_state.GetReferencedFiles()
and common_params.post_data.GetReferencedFiles(), respectively?  Then we can
delete the RVH method.

[Łukasz Anforowicz, 2016/06/16 22:05:04]

Yes.  These 2 scenarios are also transferring the access for slightly different
reasons (copying access granted via historical file dialogs VS copying access
from the old to the new renderer after a cross-site transfer):

scenario 0: file dialog grants the renderer access to the files (even before any
navigation, because the files might be accessed by a plugin...).

scenario 1: history navigations: let's apply historical file dialog decisions to
the new renderer (without PlzNavigate we have to give the renderer the PageState
and only *afterwards* it will reply with ResourceRequestBody).

scenario 2: transfers: let's transfer file access from the old to the new
renderer
(the old renderer sent ResourceRequestBody to the browser;  this would be
checked for access in content/browser/loader/resource_dispatcher_host_impl.cc:
335: ShouldServiceRequest (but only by safe fallback + NOTREACHED, not by a
renderer kill)).
Yes!  I am already on it :-)  Please see the second CL at
https://codereview.chromium.org/2067493003/

[Charlie Reis, 2016/06/16 22:20:00]

Ha!  Sorry I missed it, and thanks!  :)

-  }
+
+  // We may be here after transferring navigation to different renderer process.
+  // In this case, we need to ensure that the new renderer retains ability to
+  // access files that the old renderer could access.  Abuse is prevented,
+  // because the files listed in ResourceRequestBody are validated earlier, when
+  // they are recieved by the renderer (in
+  // ResourceDispatcherHostImpl::BeginRequest).
+  if (common_params.post_data)
+    GrantFileAccessFromResourceRequestBody(*common_params.post_data);
 }
 
 bool RenderFrameHostImpl::CanExecuteJavaScript() {
   return g_allow_injecting_javascript ||
          !frame_tree_node_->current_url().is_valid() ||
          frame_tree_node_->current_url().SchemeIs(kChromeDevToolsScheme) ||
          ChildProcessSecurityPolicyImpl::GetInstance()->HasWebUIBindings(
              GetProcess()->GetID()) ||
          // It's possible to load about:blank in a Web UI renderer.
          // See http://crbug.com/42547
@@ skipping 112 matching lines @@
   // handler after it's destroyed so it can't run after the RFHI is destroyed.
   web_bluetooth_service_->SetClientConnectionErrorHandler(base::Bind(
       &RenderFrameHostImpl::DeleteWebBluetoothService, base::Unretained(this)));
 }
 
 void RenderFrameHostImpl::DeleteWebBluetoothService() {
   web_bluetooth_service_.reset();
 }
 
 }  // namespace content