Add missing return statement after ReceivedBadMessage call.

content/browser/browser_side_navigation_browsertest.cc

 // Copyright 2014 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/strings/stringprintf.h"
 #include "base/strings/utf_string_conversions.h"
+#include "base/test/scoped_feature_list.h"
 #include "content/browser/child_process_security_policy_impl.h"
 #include "content/browser/frame_host/navigation_handle_impl.h"
 #include "content/browser/frame_host/navigation_request.h"
 #include "content/browser/web_contents/web_contents_impl.h"
 #include "content/common/frame_messages.h"
 #include "content/common/site_isolation_policy.h"
+#include "content/public/browser/navigation_controller.h"
 #include "content/public/browser/notification_types.h"
 #include "content/public/browser/web_contents.h"
+#include "content/public/common/content_features.h"
 #include "content/public/common/content_switches.h"
 #include "content/public/common/url_constants.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/navigation_handle_observer.h"
 #include "content/public/test/test_navigation_observer.h"
 #include "content/shell/browser/shell.h"
 #include "content/shell/browser/shell_network_delegate.h"
 #include "content/test/content_browser_test_utils_internal.h"
 #include "ipc/ipc_security_test_util.h"
+#include "net/base/filename_util.h"
 #include "net/base/load_flags.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_failed_job.h"
 #include "url/gurl.h"
 
 namespace content {
 
 class BrowserSideNavigationBrowserTest : public ContentBrowserTest {
  public:
@@ skipping 379 matching lines @@
   // to the URL that was blocked.
   EXPECT_EQ(1, controller.GetLastCommittedEntryIndex());
   EXPECT_FALSE(
       controller.GetLastCommittedEntry()->GetURL().SchemeIs(url::kDataScheme));
   EXPECT_TRUE(controller.GetLastCommittedEntry()->GetVirtualURL().SchemeIs(
       url::kDataScheme));
   EXPECT_EQ(url::kAboutBlankURL,
             controller.GetLastCommittedEntry()->GetURL().spec());
 }
 
+class BrowserSideNavigationBrowserDisableWebSecurityTest
+    : public BrowserSideNavigationBrowserTest {
+ public:
+  BrowserSideNavigationBrowserDisableWebSecurityTest() {}
+
+ protected:
+  void SetUpCommandLine(base::CommandLine* command_line) override {
+    // Simulate a compromised renderer, otherwise the cross-origin request to
+    // file: is blocked.
+    command_line->AppendSwitch(switches::kDisableWebSecurity);
+    BrowserSideNavigationBrowserTest::SetUpCommandLine(command_line);
+  }
+};
+
 // Test to verify that an exploited renderer process trying to specify a
 // non-empty URL for base_url_for_data_url on navigation is correctly
 // terminated.
 // TODO(nasko): This test case belongs better in
 // security_exploit_browsertest.cc, so move it there once PlzNavigate is on
 // by default.
-IN_PROC_BROWSER_TEST_F(BrowserSideNavigationBrowserTest,
+IN_PROC_BROWSER_TEST_F(BrowserSideNavigationBrowserDisableWebSecurityTest,
                        ValidateBaseUrlForDataUrl) {
   GURL start_url(embedded_test_server()->GetURL("/title1.html"));
   EXPECT_TRUE(NavigateToURL(shell(), start_url));
 
   RenderFrameHostImpl* rfh = static_cast<RenderFrameHostImpl*>(
       shell()->web_contents()->GetMainFrame());
 
+  GURL data_url("data:text/html,foo");
+  base::FilePath file_path = GetTestFilePath("", "simple_page.html");
+  GURL file_url = net::FilePathToFileURL(file_path);
+
+  // To get around DataUrlNavigationThrottle. Other attempts at getting around
+  // it don't work, i.e.:
+  // -if the request is made in a child frame then the frame is torn down
+  // immediately on process killing so the navigation doesn't complete
+  // -if it's classified as same document, then a DCHECK in
+  // NavigationRequest::CreateRendererInitiated fires
+  base::test::ScopedFeatureList feature_list;
+  feature_list.InitAndEnableFeature(
+      features::kAllowContentInitiatedDataUrlNavigations);
   // Setup a BeginNavigate IPC with non-empty base_url_for_data_url.
-  GURL url(embedded_test_server()->GetURL("/title2.html"));
   CommonNavigationParams common_params(
-      url, Referrer(), ui::PAGE_TRANSITION_LINK,
+      data_url, Referrer(), ui::PAGE_TRANSITION_LINK,

[Charlie Reis, 2017/06/01 00:33:43]

Just curious, why did you need to use a data URL?  I would think that the
previous web URL would also not be allowed to access the file URL, and would
thus be an ok test case without needing
kAllowContentInitiatedDataUrlNavigations.  But maybe there's a problem I'm
overlooking?

[jam, 2017/06/01 00:51:19]

The call to ChildProcessSecurityPolicy that uses base_url_for_data_url in
UpdatePermissionsForNavigation() is only done if the request is for a data url
(see line 3643 in the other file)

[Charlie Reis, 2017/06/01 01:00:09]

Ah, thanks.  So the original test wouldn't have gotten as far as the exploit
without the fix.  Sounds good!

       FrameMsg_Navigate_Type::DIFFERENT_DOCUMENT, true, false,
       base::TimeTicks(), FrameMsg_UILoadMetricsReportType::NO_REPORT,
-      embedded_test_server()->GetURL("foo.com",
-                                     "/title3.html"),  // base_url_for_data_url
+      file_url,  // base_url_for_data_url
       GURL(), PREVIEWS_UNSPECIFIED, base::TimeTicks::Now(), "GET", nullptr,
       base::Optional<SourceLocation>(), CSPDisposition::CHECK);
   BeginNavigationParams begin_params(
       std::string(), net::LOAD_NORMAL, false, false,
       REQUEST_CONTEXT_TYPE_LOCATION,
-      blink::WebMixedContentContextType::kBlockable, false, url::Origin(url));
+      blink::WebMixedContentContextType::kBlockable, false,
+      url::Origin(data_url));
   FrameHostMsg_BeginNavigation msg(rfh->GetRoutingID(), common_params,
                                    begin_params);
 
   // Receiving the invalid IPC message should lead to renderer process
   // termination.
   RenderProcessHostWatcher process_exit_observer(
       rfh->GetProcess(), RenderProcessHostWatcher::WATCH_FOR_PROCESS_EXIT);
   IPC::IpcSecurityTestUtil::PwnMessageReceived(rfh->GetProcess()->GetChannel(),
                                                msg);
   process_exit_observer.Wait();

[Charlie Reis, 2017/06/01 00:33:43]

Does the CanReadFile check work?  We should add it if so.  Something like:

EXPECT_FALSE(ChildProcessSecurityPolicyImpl::GetInstance()->CanReadFile(rfh->GetProcess()->GetID(),
file_url));

[jam, 2017/06/01 00:51:19]

Done.

+
+  // Reload the page to create another renderer process.
+  TestNavigationObserver tab_observer(shell()->web_contents(), 1);
+  shell()->web_contents()->GetController().Reload(ReloadType::NORMAL, false);
+  tab_observer.Wait();
+
+  // Make an XHR request to check if the page has access.
+  std::string script = base::StringPrintf(
+      "var xhr = new XMLHttpRequest()\n"
+      "xhr.open('GET', '%s', false);\n"
+      "xhr.send();\n"
+      "window.domAutomationController.send(xhr.responseText);",
+      file_url.spec().c_str());
+  std::string result;
+  EXPECT_TRUE(
+      ExecuteScriptAndExtractString(shell()->web_contents(), script, &result));
+  EXPECT_TRUE(result.empty());
 }
 
 }  // namespace content