RDH: Block a compromised renderer from reusing request ids

content/browser/security_exploit_browsertest.cc

 // Copyright (c) 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 <stdint.h>
 
 #include "base/command_line.h"
 #include "base/containers/hash_tables.h"
 #include "base/macros.h"
 #include "base/strings/utf_string_conversions.h"
 #include "build/build_config.h"
 #include "content/browser/dom_storage/dom_storage_context_wrapper.h"
 #include "content/browser/dom_storage/session_storage_namespace_impl.h"
 #include "content/browser/frame_host/navigator.h"
 #include "content/browser/frame_host/render_frame_host_impl.h"
 #include "content/browser/renderer_host/render_view_host_factory.h"
 #include "content/browser/renderer_host/render_view_host_impl.h"
 #include "content/browser/web_contents/web_contents_impl.h"
 #include "content/common/frame_messages.h"
 #include "content/common/resource_messages.h"
 #include "content/common/view_messages.h"
 #include "content/public/browser/browser_context.h"
 #include "content/public/browser/content_browser_client.h"
 #include "content/public/browser/interstitial_page.h"
 #include "content/public/browser/interstitial_page_delegate.h"
+#include "content/public/browser/resource_dispatcher_host.h"
 #include "content/public/browser/storage_partition.h"
 #include "content/public/common/appcache_info.h"
 #include "content/public/common/browser_side_navigation_policy.h"
 #include "content/public/common/content_switches.h"
 #include "content/public/common/file_chooser_params.h"
 #include "content/public/test/browser_test_utils.h"
 #include "content/public/test/content_browser_test.h"
 #include "content/public/test/content_browser_test_utils.h"
 #include "content/public/test/test_utils.h"
 #include "content/shell/browser/shell.h"
 #include "content/test/test_content_browser_client.h"
 #include "ipc/ipc_security_test_util.h"
 #include "net/dns/mock_host_resolver.h"
 #include "net/test/embedded_test_server/embedded_test_server.h"
+#include "net/test/url_request/url_request_slow_download_job.h"
 
 using IPC::IpcSecurityTestUtil;
 
 namespace content {
 
 namespace {
 
 // This is a helper function for the tests which attempt to create a
 // duplicate RenderViewHost or RenderWidgetHost. It tries to create two objects
 // with the same process and routing ids, which causes a collision.
@@ skipping 41 matching lines @@
   else
     next_rfh = wc->GetRenderManagerForTesting()->pending_frame_host();
 
   EXPECT_TRUE(next_rfh);
   EXPECT_NE(shell->web_contents()->GetRenderProcessHost()->GetID(),
             next_rfh->GetProcess()->GetID());
 
   return next_rfh->render_view_host();
 }
 
-ResourceHostMsg_Request CreateXHRRequestWithOrigin(const char* origin) {
+ResourceHostMsg_Request CreateDefaultRequest() {

[mmenke, 2016/01/28 16:20:08]

Optional:  Suggest making this take in a url, and setting the request.url to it.

[gzobqq, 2016/01/28 20:21:17]

Done.

   ResourceHostMsg_Request request;
   request.method = "GET";
   request.url = GURL("http://bar.com/simple_page.html");
-  request.first_party_for_cookies = GURL(origin);
   request.referrer_policy = blink::WebReferrerPolicyDefault;
-  request.headers = base::StringPrintf("Origin: %s\r\n", origin);
   request.load_flags = 0;
   request.origin_pid = 0;
   request.resource_type = RESOURCE_TYPE_XHR;
   request.request_context = 0;
   request.appcache_host_id = kAppCacheNoHostId;
   request.download_to_file = false;
   request.should_reset_appcache = false;
   request.is_main_frame = true;
   request.parent_is_main_frame = false;
   request.parent_render_frame_id = -1;
   request.transition_type = ui::PAGE_TRANSITION_LINK;
   request.allow_download = true;
   return request;
 }
 
+ResourceHostMsg_Request CreateXHRRequestWithOrigin(const char* origin) {
+  ResourceHostMsg_Request request = CreateDefaultRequest();
+  request.first_party_for_cookies = GURL(origin);
+  request.headers = base::StringPrintf("Origin: %s\r\n", origin);
+  return request;
+}
+
+ResourceHostMsg_Request CreateXHRRequestForURL(const char *url) {
+  ResourceHostMsg_Request request = CreateDefaultRequest();
+  request.url = GURL(url);
+  return request;
+}
+
+void TryCreateDuplicateRequestIds(Shell *shell, bool block_loaders) {
+  NavigateToURL(shell, GURL("http://foo.com/simple_page.html"));
+  RenderFrameHost* web_rfh = shell->web_contents()->GetMainFrame();
+
+  if (block_loaders) {
+    // Test the case where loaders are placed into blocked_loaders_map_.
+    int child_id = web_rfh->GetProcess()->GetID();
+    ResourceDispatcherHost::Get()->BlockRequestsForRoute(
+        child_id, web_rfh->GetRoutingID());

[mmenke, 2016/01/28 16:20:08]

This needs to be run on the IO thread, and we're currently on the UI thread. 
You can just post a task to run this to the IO thread - no need to wait for it
to complete, since everything else this needs to run before will also happen on
the IO thread.

[gzobqq, 2016/01/28 20:21:16]

Good catch, done.

+  }
+
+  // URLRequestSlowDownloadJob waits for another request to kFinishDownloadUrl
+  // to finish all pending requests. We never send it, so the following URL
+  // blocks indefinitely, which is good because the request stays alive and we

[mmenke, 2016/01/28 16:20:08]

nit (x2):  Avoid "we" in comments, due to ambiguity (Does it mean "we" the
chromium authors, "we" the object, "we" the code, etc).  Prefer passive voice.

[gzobqq, 2016/01/28 20:21:17]

Done.

+  // can try to reuse the request id without a race.
+  const char* blocking_url = net::URLRequestSlowDownloadJob::kUnknownSizeUrl;
+  ResourceHostMsg_Request request(CreateXHRRequestForURL(blocking_url));
+
+  RenderProcessHostWatcher web_process_killed(
+      web_rfh->GetProcess(),
+      RenderProcessHostWatcher::WATCH_FOR_PROCESS_EXIT);
+  // Use request id 1 twice.
+  IPC::IpcSecurityTestUtil::PwnMessageReceived(
+      web_rfh->GetProcess()->GetChannel(),
+      ResourceHostMsg_RequestResource(web_rfh->GetRoutingID(), 1, request));
+  IPC::IpcSecurityTestUtil::PwnMessageReceived(
+      web_rfh->GetProcess()->GetChannel(),
+      ResourceHostMsg_RequestResource(web_rfh->GetRoutingID(), 1, request));
+  web_process_killed.Wait();
+}
+
 }  // namespace
 
 
 // The goal of these tests will be to "simulate" exploited renderer processes,
 // which can send arbitrary IPC messages and confuse browser process internal
 // state, leading to security bugs. We are trying to verify that the browser
 // doesn't perform any dangerous operations in such cases.
 class SecurityExploitBrowserTest : public ContentBrowserTest {
  public:
   SecurityExploitBrowserTest() {}
 
   void SetUpCommandLine(base::CommandLine* command_line) override {
     ASSERT_TRUE(embedded_test_server()->Start());
 
     // Add a host resolver rule to map all outgoing requests to the test server.
     // This allows us to use "real" hostnames in URLs, which we can use to
     // create arbitrary SiteInstances.
     command_line->AppendSwitchASCII(
         switches::kHostResolverRules,
         "MAP * " +
             net::HostPortPair::FromURL(embedded_test_server()->base_url())
                 .ToString() +
             ",EXCLUDE localhost");
   }
 
+  void SetUpOnMainThread() override {
+    BrowserThread::PostTask(
+        BrowserThread::IO, FROM_HERE,
+        base::Bind(&net::URLRequestSlowDownloadJob::AddUrlHandler));
+  }
+
  protected:
   // Tests that a given file path sent in a ViewHostMsg_RunFileChooser will
   // cause renderer to be killed.
   void TestFileChooserWithPath(const base::FilePath& path);
 };
 
 void SecurityExploitBrowserTest::TestFileChooserWithPath(
     const base::FilePath& path) {
   GURL foo("http://foo.com/simple_page.html");
   NavigateToURL(shell(), foo);
@@ skipping 219 matching lines @@
   NavigateToURL(shell(), web_url);
   RenderFrameHost* web_rfh = shell()->web_contents()->GetMainFrame();
 
   // Web processes cannot make XHRs with chrome:// Origin headers.
   {
     RenderProcessHostWatcher web_process_killed(
         web_rfh->GetProcess(),
         RenderProcessHostWatcher::WATCH_FOR_PROCESS_EXIT);
     IPC::IpcSecurityTestUtil::PwnMessageReceived(
         web_rfh->GetProcess()->GetChannel(),
         ResourceHostMsg_RequestResource(web_rfh->GetRoutingID(), 1,

[mmenke, 2016/01/28 16:20:08]

These tests make me nervous now - we're depending on the request for the
navigation either not to use ID 1, or to be completely cleaned up before this
message reaches the IO thread.  Otherwise, the new check will cause these tests
to pass, even if what they're testing breaks.

Could we just declare a constant "const int kRequestIdNotPreviouslyUsed = 10000"
and use that for all of these (It's not a problem using the same ID in each
test, because in each case the old RFH is destroyed, so the new one has a new
ID).

May want to do that for your new test as well, though it shouldn't really
matter, there, since even if we match the ID of the original request, and it's
not cleaned up yet, we'll still crash the process the right way.

[gzobqq, 2016/01/28 20:21:17]

Done.

                                         chrome_origin_msg));
     web_process_killed.Wait();
   }
 
   // Web processes cannot make XHRs with URLs that the content embedder expects
   // to have process isolation.  Ideally this would test chrome-extension://
   // URLs for Chrome Apps, but those can't be tested inside content/ and the
   // ResourceHostMsg_Request IPC can't be created in a test outside content/.
   NavigateToURL(shell(), web_url);
   {
@@ skipping 32 matching lines @@
         web_rfh->GetProcess(),
         RenderProcessHostWatcher::WATCH_FOR_PROCESS_EXIT);
     IPC::IpcSecurityTestUtil::PwnMessageReceived(
         web_rfh->GetProcess()->GetChannel(),
         ResourceHostMsg_RequestResource(web_rfh->GetRoutingID(), 1,
                                         invalid_scheme_origin_msg));
     web_process_killed.Wait();
   }
 }
 
+// Renderer process should not be able to create multiple requests with the same
+// id.
+IN_PROC_BROWSER_TEST_F(SecurityExploitBrowserTest, InvalidRequestId) {
+  // Existing loader in pending_loaders_.
+  TryCreateDuplicateRequestIds(shell(), false);
+  // Existing loader in blocked_loaders_map_.
+  TryCreateDuplicateRequestIds(shell(), true);
+}
+
 }  // namespace content