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

content/browser/loader/resource_dispatcher_host_impl.cc

Index: content/browser/loader/resource_dispatcher_host_impl.cc
diff --git a/content/browser/loader/resource_dispatcher_host_impl.cc b/content/browser/loader/resource_dispatcher_host_impl.cc
index 50153c6706bfa0951f8506161aa063702a7069b1..c14e80893b79a3f44d4995af6aad04b17a36b897 100644
--- a/content/browser/loader/resource_dispatcher_host_impl.cc
+++ b/content/browser/loader/resource_dispatcher_host_impl.cc
@@ -518,6 +518,16 @@ void NotifyForEachFrameFromUI(
                                      base::Passed(std::move(routing_ids))));
 }
 
+bool CanAccessFilesOfResourceRequestBody(
+    int child_id,
+    const scoped_refptr<ResourceRequestBody>& body) {
+  if (!body)
+    return true;
+
+  return ChildProcessSecurityPolicyImpl::GetInstance()->CanReadAllFiles(
+      child_id, body->GetReferencedFiles());
+}
+
 }  // namespace
 
 LoaderIOThreadNotifier::LoaderIOThreadNotifier(WebContents* web_contents)
@@ -1380,6 +1390,16 @@ void ResourceDispatcherHostImpl::BeginRequest(
     return;
   }
 
+  // Reject requests attempting to refer to unauthorized files.  This is
+  // important, because after a cross-site transfer, the new renderer process is
+  // unconditionally granted access to files from ResourceRequestBody.
+  if (!CanAccessFilesOfResourceRequestBody(child_id,
+                                           request_data.request_body)) {
+    bad_message::ReceivedBadMessage(
+        filter_, bad_message::RDH_CAN_ACCESS_FILES_OF_REQUEST_BODY);
+    return;
+  }

[Łukasz Anforowicz, 2016/06/14 01:07:17]

AFAICT this check should cover all cases where ResourceRequestBody is sent from
a renderer to the browser:
- ResourceHostMsg_SyncLoad
- ResourceHostMsg_RequestResource

The things above were found by grepping //content/common for ResourceRequestBody
and then grepping for structs embedding ResourceRequestBody
(CommonNavigationParams - only browser->renderer and ResourceRequest - this one
is sent renderer->browser).

Note that even before the check added above things were okay from security
perspective before this CL.  In particular file access was getting checked in
content/browser/loader/resource_dispatcher_host_impl.cc: 335:
ShouldServiceRequest (but the response was safe/secure fallback + NOTREACHED,
not a renderer kill).

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

Oh, we did catch it there.  We get to ShouldServiceRequest before deciding to
transfer as well.  Is your new check only for the post-transfer case?

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

Hmmm... I didn't previously look very hard how ShouldServiceRequest is getting
called.  I now see that it is called right below, directly from BeginRequest. 
So - the new check does indeed look a bit redundant.  I've added the new check,
because I was trying to mimic how we validate PageState and didn't realize that
ShouldServiceRequest check is done right here (I was searching for it separately
to verify that there was a check and confused myself into thinking that this
already existing check happens some time after the IPC).

The only advantage of the new check is that it would allow us to 1) more
aggressively stop compromised renderers and 2) get some insight into these
renderer kills via UMA.  OTOH, the new check also 1) adds a bit of unnecessary
code complexity and 2) has some small risk of actually killing an innocent
renderer (but in this case we would find bugs via UMA - yay?).

So - after typing the above, I am considering removing the new check (and the
new UMA / kill enum + histogram).  WDYT?  What would you want to do if I haven't
already pushed out a CR that includes the new check? :-)
I don't understand the question :-(

[Charlie Reis, 2016/06/16 22:19:59]

Ok, I agree this whole thing is a bit confusing.  Let's look closer:

BeginRequest gets called twice: once before the transfer from the old RFH (in
which case we hit ShouldServiceRequest below), and once from the new RFH (in
which case we call CompleteTransfer() and return before getting to
ShouldServiceRequest).

We won't kill the new RFH if it doesn't have access to those files at the time
of BeginRequest, but we will kill it at commit time.  That said, it's not like
killing the new RFH in BeginRequest is that meaningful anyway, because the POST
data has *already* been sent to the server.  The only thing that happens from
here is that the page gets committed in the new renderer, at which point we'll
verify it had access to the files.

I think this means we can probably skip the extra kill.

(FYI: Renderer kills now show up both in UMA data and as DumpWithoutCrashing
browser crash reports, so that we can track them with stacks, magic signatures,
etc.)

[Łukasz Anforowicz, 2016/06/16 23:33:55]

Done.  I think this is the right call - the new kill I've added wasn't really
needed (as I learned during the review) and was only unnecessarily adding extra
complexity.

BTW: ShouldServiceRequest already performs some renderer kills - if at a later
point in time we think the new kill is desirable, then we can add it there (not
in BeginRequest).

Also - note that after removing the kill, some of the refactoring/deduplicating
work wasn't really needed in this CL (i.e. no need for
ChildProcessSecurityPolicyImpl::CanReadAllFiles if it only has one caller / if
it is no longer called from RDHI) so I moved this part of the refactoring into
the follow-up, separate CL at https://codereview.chromium.org/2067493003/.

+
   // PlzNavigate: reject invalid renderer main resource request.
   bool is_navigation_stream_request =
       IsBrowserSideNavigationEnabled() &&