Extra browser-side validation of transferred_request_child_id / request_id.

content/browser/loader/resource_dispatcher_host_impl.cc

 // Copyright (c) 2012 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.
 
 // See http://dev.chromium.org/developers/design-documents/multi-process-resource-loading
 
 #include "content/browser/loader/resource_dispatcher_host_impl.h"
 
 #include <stddef.h>
 
@@ skipping 1177 matching lines @@
     if (!handler->SanityCheckIsSameContext(filter_->service_worker_context())) {
       bad_message::ReceivedBadMessage(
           filter_, bad_message::RDHI_WRONG_STORAGE_PARTITION);
     } else {
       handler->CompleteCrossSiteTransfer(
           child_id, request_data.service_worker_provider_id);
     }
   }
 }
 
+void ResourceDispatcherHostImpl::CompleteTransfer(
+    int request_id,
+    const ResourceRequest& request_data,
+    int route_id) {
+  // Caller should ensure that |request_data| is associated with a transfer.
+  DCHECK(request_data.transferred_request_child_id != -1 ||
+         request_data.transferred_request_request_id != -1);
+
+  bool is_navigational_request =
+      request_data.resource_type == RESOURCE_TYPE_MAIN_FRAME ||
+      request_data.resource_type == RESOURCE_TYPE_SUB_FRAME;
+  if (!is_navigational_request) {
+    // Transfers apply only to navigational requests - the renderer seems to
+    // have sent bogus IPC data.
+    bad_message::ReceivedBadMessage(
+        filter_, bad_message::RDH_TRANSFERRING_NONNAVIGATIONAL_REQUEST);
+    return;
+  }
+
+  // Attempt to find a loader associated with the deferred transfer request.
+  LoaderMap::iterator it = pending_loaders_.find(
+      GlobalRequestID(request_data.transferred_request_child_id,
+                      request_data.transferred_request_request_id));
+  if (it == pending_loaders_.end()) {
+    // Renderer sent transferred_request_request_id and/or
+    // transferred_request_child_id that doesn't have a corresponding entry on
+    // the browser side.
+    bad_message::ReceivedBadMessage(
+        filter_, bad_message::RDH_TRANSFERRING_REQUEST_NOT_FOUND);
+    return;
+  }
+  ResourceLoader* pending_loader = it->second.get();
+
+  if (!pending_loader->is_transferring()) {
+    // Renderer sent transferred_request_request_id and/or
+    // transferred_request_child_id that doesn't correspond to an actually
+    // transferring loader on the browser side.
+    base::debug::Alias(pending_loader);
+    bad_message::ReceivedBadMessage(filter_,
+                                    bad_message::RDH_REQUEST_NOT_TRANSFERRING);
+    return;
+  }

[mmenke, 2016/10/25 20:23:47]

Random drive-by question:  How concerned are we about a compromised renderer
stealing requests this way?  If an attacker can guess a child_id and request_id
(Latter isn't hard, if opening a new tab) of a new navigation that's currently
transferring a request, can't it steal the request?

[Charlie Reis, 2016/10/25 20:30:07]

Yes, good point.  Łukasz was asking about this the other day.  I think we have
some logic in RenderFrameHostManager that will only accept a commit if it comes
from the expected pending RenderFrameHost, but that won't really defend against
this (which happens earlier).  Specifically, the response would be send to the
first renderer that asks for it, and that would matter from a cross-site
document blocking perspective for Site Isolation.

We should file a bug to track that.  Not sure if there's much impact today,
though-- probably mainly for --site-per-process.

+
+  // If the request is transferring to a new process, we can update our
+  // state and let it resume with its existing ResourceHandlers.
+  UpdateRequestForTransfer(filter_->child_id(), route_id, request_id,
+                           request_data, it);
+  pending_loader->CompleteTransfer();
+}
+
 void ResourceDispatcherHostImpl::BeginRequest(
     int request_id,
     const ResourceRequest& request_data,
     const SyncLoadResultCallback& sync_result_handler, // only valid for sync
     int route_id,
     mojo::InterfaceRequest<mojom::URLLoader> mojo_request,
     mojom::URLLoaderClientPtr url_loader_client) {
   int process_type = filter_->process_type();
   int child_id = filter_->child_id();
 
@@ skipping 24 matching lines @@
   }
 
   // If we crash here, figure out what URL the renderer was requesting.
   // http://crbug.com/91398
   char url_buf[128];
   base::strlcpy(url_buf, request_data.url.spec().c_str(), arraysize(url_buf));
   base::debug::Alias(url_buf);
 
   // If the request that's coming in is being transferred from another process,
   // we want to reuse and resume the old loader rather than start a new one.
-  LoaderMap::iterator it = pending_loaders_.find(
-      GlobalRequestID(request_data.transferred_request_child_id,
-                      request_data.transferred_request_request_id));
-  if (it != pending_loaders_.end()) {
+  if (request_data.transferred_request_child_id != -1 ||
+      request_data.transferred_request_request_id != -1) {
     // TODO(yhirano): Make mojo work for this case.
     DCHECK(!url_loader_client);
 
-    // If the request is transferring to a new process, we can update our
-    // state and let it resume with its existing ResourceHandlers.
-    if (it->second->is_transferring()) {
-      ResourceLoader* deferred_loader = it->second.get();
-      UpdateRequestForTransfer(child_id, route_id, request_id,
-                               request_data, it);
-      deferred_loader->CompleteTransfer();
-    } else {
-      bad_message::ReceivedBadMessage(
-          filter_, bad_message::RDH_REQUEST_NOT_TRANSFERRING);
-    }
+    CompleteTransfer(request_id, request_data, route_id);
     return;
   }
 
   ResourceContext* resource_context = NULL;
   net::URLRequestContext* request_context = NULL;
   filter_->GetContexts(request_data.resource_type, &resource_context,
                        &request_context);
 
   // Parse the headers before calling ShouldServiceRequest, so that they are
   // available to be validated.
@@ skipping 1469 matching lines @@
                                  &throttles);
     if (!throttles.empty()) {
       handler.reset(new ThrottlingResourceHandler(std::move(handler), request,
                                                   std::move(throttles)));
     }
   }
   return handler;
 }
 
 }  // namespace content