Prevent untrusted consumers from setting a param on ResourceRequest.

|update_network_isolation_key_on_redirect| should only be set on
navigation requests, and renderers are not trusted to set it.

This CL introduces ResourceRequest::TrustedParams, which contains fields
only the browser process is trusted to set, and an |is_trusted| bit that
can be set when creating URLLoaderFactories. If TrustedParams are sent
to a URLLoaderFactory without that bit set, the request will fail.

Bug: 991736
TBR: alexilin@chromium.org
Change-Id: Id3263e03e663b330ee9c21682a4d0de753c5331a
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1744926
Commit-Queue: Matt Menke <mmenke@chromium.org>
Reviewed-by: Shivani Sharma <shivanisha@chromium.org>
Reviewed-by: Kinuko Yasuda <kinuko@chromium.org>
Cr-Commit-Position: refs/heads/master@{#686472}

chrome/browser/predictors/loading_predictor_browsertest.cc

@@ -940,7 +940,8 @@ class LoadingPredictorNetworkIsolationKeyBrowserTest
     request->url = url;
     content::SimpleURLLoaderTestHelper simple_loader_helper;
     url::Origin origin = url::Origin::Create(url);
-    request->trusted_network_isolation_key =
+    request->trusted_params = network::ResourceRequest::TrustedParams();
+    request->trusted_params->network_isolation_key =
         net::NetworkIsolationKey(origin, origin);
     std::unique_ptr<network::SimpleURLLoader> simple_loader =
         network::SimpleURLLoader::Create(std::move(request),

content/browser/frame_host/render_frame_host_impl_browsertest.cc

@@ -36,6 +36,7 @@
 #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/simple_url_loader_test_helper.h"
 #include "content/public/test/test_frame_navigation_observer.h"
 #include "content/public/test/test_navigation_observer.h"
 #include "content/public/test/test_utils.h"
@@ -45,13 +46,20 @@
 #include "content/test/did_commit_navigation_interceptor.h"
 #include "content/test/frame_host_test_interface.mojom.h"
 #include "content/test/test_content_browser_client.h"
+#include "net/base/net_errors.h"
 #include "net/dns/mock_host_resolver.h"
 #include "net/test/embedded_test_server/controllable_http_response.h"
 #include "net/test/embedded_test_server/embedded_test_server.h"
 #include "net/test/embedded_test_server/http_request.h"
+#include "net/traffic_annotation/network_traffic_annotation_test_helper.h"
+#include "services/network/public/cpp/resource_request.h"
+#include "services/network/public/cpp/simple_url_loader.h"
+#include "services/network/public/mojom/url_loader_factory.mojom.h"
 #include "services/network/test/test_url_loader_factory.h"
 #include "services/service_manager/public/mojom/interface_provider.mojom-test-utils.h"
 #include "testing/gmock/include/gmock/gmock.h"
+#include "url/gurl.h"
+#include "url/origin.h"
 
 #if defined(OS_ANDROID)
 #include "base/android/build_info.h"
@@ -247,6 +255,33 @@ IN_PROC_BROWSER_TEST_F(RenderFrameHostImplBrowserTest,
   SetBrowserClientForTesting(old_client);
 }
 
+// Check that the URLLoaderFactories created by RenderFrameHosts for renderers
+// are not trusted.
+IN_PROC_BROWSER_TEST_F(RenderFrameHostImplBrowserTest,
+                       URLLoaderFactoryNotTrusted) {
+  EXPECT_TRUE(NavigateToURL(shell(), embedded_test_server()->GetURL("/echo")));
+  network::mojom::URLLoaderFactoryPtr url_loader_factory;
+  shell()->web_contents()->GetMainFrame()->CreateNetworkServiceDefaultFactory(
+      mojo::MakeRequest(&url_loader_factory));
+
+  std::unique_ptr<network::ResourceRequest> request =
+      std::make_unique<network::ResourceRequest>();
+  request->url = embedded_test_server()->GetURL("/echo");
+  request->request_initiator =
+      url::Origin::Create(embedded_test_server()->base_url());
+  request->trusted_params = network::ResourceRequest::TrustedParams();
+
+  content::SimpleURLLoaderTestHelper simple_loader_helper;
+  std::unique_ptr<network::SimpleURLLoader> simple_loader =
+      network::SimpleURLLoader::Create(std::move(request),
+                                       TRAFFIC_ANNOTATION_FOR_TESTS);
+  simple_loader->DownloadToStringOfUnboundedSizeUntilCrashAndDie(
+      url_loader_factory.get(), simple_loader_helper.GetCallback());
+  simple_loader_helper.WaitForCallback();
+  EXPECT_FALSE(simple_loader_helper.response_body());
+  EXPECT_EQ(net::ERR_INVALID_ARGUMENT, simple_loader->NetError());
+}
+
 namespace {
 
 class TestJavaScriptDialogManager : public JavaScriptDialogManager,

content/browser/loader/navigation_url_loader_impl.cc

@@ -174,14 +174,16 @@ std::unique_ptr<network::ResourceRequest> CreateResourceRequest(
   new_request->method = request_info->common_params->method;
   new_request->url = request_info->common_params->url;
   new_request->site_for_cookies = request_info->site_for_cookies;
-  new_request->trusted_network_isolation_key =
+  new_request->trusted_params = network::ResourceRequest::TrustedParams();
+  new_request->trusted_params->network_isolation_key =
       request_info->network_isolation_key;
 
   if (request_info->is_main_frame) {
-    new_request->update_network_isolation_key_on_redirect = network::mojom::
-        UpdateNetworkIsolationKeyOnRedirect::kUpdateTopFrameAndFrameOrigin;
+    new_request->trusted_params->update_network_isolation_key_on_redirect =
+        network::mojom::UpdateNetworkIsolationKeyOnRedirect::
+            kUpdateTopFrameAndFrameOrigin;
   } else {
-    new_request->update_network_isolation_key_on_redirect =
+    new_request->trusted_params->update_network_isolation_key_on_redirect =
         network::mojom::UpdateNetworkIsolationKeyOnRedirect::kUpdateFrameOrigin;
   }
 
@@ -330,7 +332,6 @@ class NavigationURLLoaderImpl::URLLoaderRequestController
     return options;
   }
 
-  // This can be called on the UI or IO thread.
   void Start(std::unique_ptr<network::SharedURLLoaderFactoryInfo>
                  network_loader_factory_info,
              ServiceWorkerNavigationHandle*
@@ -736,16 +737,17 @@ class NavigationURLLoaderImpl::URLLoaderRequestController
     if (resource_request_->resource_type ==
         static_cast<int>(ResourceType::kMainFrame)) {
       url::Origin origin = url::Origin::Create(resource_request_->url);
-      resource_request_->trusted_network_isolation_key =
+      resource_request_->trusted_params->network_isolation_key =
           net::NetworkIsolationKey(origin, origin);
     } else {
       DCHECK_EQ(static_cast<int>(ResourceType::kSubFrame),
                 resource_request_->resource_type);
       url::Origin subframe_origin = url::Origin::Create(resource_request_->url);
       base::Optional<url::Origin> top_frame_origin =
-          resource_request_->trusted_network_isolation_key.GetTopFrameOrigin();
+          resource_request_->trusted_params->network_isolation_key
+              .GetTopFrameOrigin();
       DCHECK(top_frame_origin);
-      resource_request_->trusted_network_isolation_key =
+      resource_request_->trusted_params->network_isolation_key =
           net::NetworkIsolationKey(top_frame_origin.value(), subframe_origin);
     }
 
@@ -1435,6 +1437,7 @@ void NavigationURLLoaderImpl::CreateURLLoaderFactoryWithHeaderClient(
       network::mojom::URLLoaderFactoryParams::New();
   params->header_client = std::move(header_client);
   params->process_id = network::mojom::kBrowserProcessId;
+  params->is_trusted = true;
   params->is_corb_enabled = false;
   params->disable_web_security =
       base::CommandLine::ForCurrentProcess()->HasSwitch(

content/browser/loader/navigation_url_loader_impl_unittest.cc

@@ -329,8 +329,10 @@ TEST_F(NavigationURLLoaderImplTest, NetworkIsolationKeyOfMainFrameNavigation) {
   delegate.WaitForRequestStarted();
 
   ASSERT_TRUE(most_recent_resource_request_);
-  EXPECT_EQ(net::NetworkIsolationKey(origin, origin),
-            most_recent_resource_request_->trusted_network_isolation_key);
+  ASSERT_TRUE(most_recent_resource_request_->trusted_params);
+  EXPECT_EQ(
+      net::NetworkIsolationKey(origin, origin),
+      most_recent_resource_request_->trusted_params->network_isolation_key);
 }
 
 TEST_F(NavigationURLLoaderImplTest,
@@ -343,8 +345,10 @@ TEST_F(NavigationURLLoaderImplTest,
 
   HTTPRedirectOriginHeaderTest(url, "GET", "GET", url.GetOrigin().spec());
 
-  EXPECT_EQ(net::NetworkIsolationKey(origin, origin),
-            most_recent_resource_request_->trusted_network_isolation_key);
+  ASSERT_TRUE(most_recent_resource_request_->trusted_params);
+  EXPECT_EQ(
+      net::NetworkIsolationKey(origin, origin),
+      most_recent_resource_request_->trusted_params->network_isolation_key);
 }
 
 TEST_F(NavigationURLLoaderImplTest, Redirect301Tests) {

content/browser/navigation_browsertest.cc

@@ -290,11 +290,14 @@ class NetworkIsolationNavigationBrowserTest
     URLLoaderInterceptor interceptor(base::BindLambdaForTesting(
         [&](URLLoaderInterceptor::RequestParams* params) -> bool {
           base::AutoLock top_frame_origins_lock(lock);
-          (*network_isolation_keys)[params->url_request.url] =
-              params->url_request.trusted_network_isolation_key;
-          (*update_network_isolation_key_on_redirects)[params->url_request
-                                                           .url] =
-              params->url_request.update_network_isolation_key_on_redirect;
+          if (params->url_request.trusted_params) {
+            (*network_isolation_keys)[params->url_request.url] =
+                params->url_request.trusted_params->network_isolation_key;
+            (*update_network_isolation_key_on_redirects)[params->url_request
+                                                             .url] =
+                params->url_request.trusted_params
+                    ->update_network_isolation_key_on_redirect;
+          }
 
           if (params->url_request.url == final_resource)
             run_loop.Quit();

content/browser/service_worker/service_worker_single_script_update_checker.cc

@@ -132,7 +132,8 @@ ServiceWorkerSingleScriptUpdateChecker::ServiceWorkerSingleScriptUpdateChecker(
 
   // This key is used to isolate requests from different contexts in accessing
   // shared network resources like the http cache.
-  resource_request.trusted_network_isolation_key =
+  resource_request.trusted_params = network::ResourceRequest::TrustedParams();
+  resource_request.trusted_params->network_isolation_key =
       net::NetworkIsolationKey(origin, origin);
 
   if (is_main_script_) {

content/browser/storage_partition_impl.cc

@@ -1801,6 +1801,9 @@ StoragePartitionImpl::GetURLLoaderFactoryForBrowserProcessInternal(
       network::mojom::URLLoaderFactoryParams::New();
   params->process_id = network::mojom::kBrowserProcessId;
   params->is_corb_enabled = corb_enabled;
+  // Corb requests are likely made on behalf of untrusted renderers.
+  if (!corb_enabled)
+    params->is_trusted = true;
   params->disable_web_security =
       base::CommandLine::ForCurrentProcess()->HasSwitch(
           switches::kDisableWebSecurity);

content/browser/url_loader_factory_getter.cc

@@ -285,6 +285,8 @@ void URLLoaderFactoryGetter::HandleNetworkFactoryRequestOnUIThread(
     return;
   network::mojom::URLLoaderFactoryParamsPtr params =
       network::mojom::URLLoaderFactoryParams::New();
+  // The browser process is considered trusted.
+  params->is_trusted = true;
   params->process_id = network::mojom::kBrowserProcessId;
   params->is_corb_enabled = is_corb_enabled;
   params->disable_web_security =

content/browser/worker_host/worker_script_fetch_initiator.cc

@@ -119,7 +119,8 @@ void WorkerScriptFetchInitiator::Start(
   resource_request = std::make_unique<network::ResourceRequest>();
   resource_request->url = script_url;
   resource_request->site_for_cookies = script_url;
-  resource_request->trusted_network_isolation_key =
+  resource_request->trusted_params = network::ResourceRequest::TrustedParams();
+  resource_request->trusted_params->network_isolation_key =
       trusted_network_isolation_key;
   resource_request->request_initiator = request_initiator;
   resource_request->referrer = sanitized_referrer.url,

services/network/cors/cors_url_loader_factory.cc

@@ -36,6 +36,7 @@ CorsURLLoaderFactory::CorsURLLoaderFactory(
     const OriginAccessList* origin_access_list,
     std::unique_ptr<mojom::URLLoaderFactory> network_loader_factory_for_testing)
     : context_(context),
+      is_trusted_(params->is_trusted),
       disable_web_security_(params->disable_web_security),
       process_id_(params->process_id),
       request_initiator_site_lock_(params->request_initiator_site_lock),
@@ -142,6 +143,14 @@ bool CorsURLLoaderFactory::IsSane(const NetworkContext* context,
     return false;
   }
 
+  // Reject request with trusted params if factory is not for a trusted
+  // consumer.
+  if (request.trusted_params && !is_trusted_) {
+    mojo::ReportBadMessage(
+        "CorsURLLoaderFactory: Untrusted caller making trusted request");
+    return false;
+  }
+
   // Ensure that renderer requests are covered either by CORS or CORB.
   if (process_id_ != mojom::kBrowserProcessId) {
     switch (request.mode) {

services/network/cors/cors_url_loader_factory.h

@@ -77,6 +77,9 @@ class COMPONENT_EXPORT(NETWORK_SERVICE) CorsURLLoaderFactory final
   NetworkContext* const context_ = nullptr;
   scoped_refptr<ResourceSchedulerClient> resource_scheduler_client_;
 
+  // If false, ResourceRequests cannot have their |trusted_params| fields set.
+  bool is_trusted_;
+
   // Retained from URLLoaderFactoryParams:
   const bool disable_web_security_;
   const uint32_t process_id_ = mojom::kInvalidProcessId;

services/network/cors/cors_url_loader_unittest.cc

@@ -183,10 +183,11 @@ class CorsURLLoaderTest : public testing::Test {
   }
 
   void CreateLoaderAndStart(const ResourceRequest& request) {
+    test_cors_loader_client_ = std::make_unique<TestURLLoaderClient>();
     cors_url_loader_factory_->CreateLoaderAndStart(
         mojo::MakeRequest(&url_loader_), 0 /* routing_id */, 0 /* request_id */,
         mojom::kURLLoadOptionNone, request,
-        test_cors_loader_client_.CreateInterfacePtr(),
+        test_cors_loader_client_->CreateInterfacePtr(),
         net::MutableNetworkTrafficAnnotationTag(TRAFFIC_ANNOTATION_FOR_TESTS));
   }
 
@@ -254,9 +255,11 @@ class CorsURLLoaderTest : public testing::Test {
     return test_url_loader_factory_->num_created_loaders();
   }
 
-  const TestURLLoaderClient& client() const { return test_cors_loader_client_; }
+  const TestURLLoaderClient& client() const {
+    return *test_cors_loader_client_;
+  }
   void ClearHasReceivedRedirect() {
-    test_cors_loader_client_.ClearHasReceivedRedirect();
+    test_cors_loader_client_->ClearHasReceivedRedirect();
   }
 
   void RunUntilCreateLoaderAndStartCalled() {
@@ -266,9 +269,9 @@ class CorsURLLoaderTest : public testing::Test {
     run_loop.Run();
     test_url_loader_factory_->SetOnCreateLoaderAndStart({});
   }
-  void RunUntilComplete() { test_cors_loader_client_.RunUntilComplete(); }
+  void RunUntilComplete() { test_cors_loader_client_->RunUntilComplete(); }
   void RunUntilRedirectReceived() {
-    test_cors_loader_client_.RunUntilRedirectReceived();
+    test_cors_loader_client_->RunUntilRedirectReceived();
   }
 
   void AddAllowListEntryForOrigin(const url::Origin& source_origin,
@@ -319,11 +322,11 @@ class CorsURLLoaderTest : public testing::Test {
   }
 
   void ResetFactory(base::Optional<url::Origin> initiator,
-                    uint32_t process_id) {
+                    uint32_t process_id,
+                    bool is_trusted = false) {
     std::unique_ptr<TestURLLoaderFactory> factory =
         std::make_unique<TestURLLoaderFactory>();
     test_url_loader_factory_ = factory->GetWeakPtr();
-
     auto factory_params = network::mojom::URLLoaderFactoryParams::New();
     if (initiator) {
       factory_params->request_initiator_site_lock = *initiator;
@@ -332,6 +335,7 @@ class CorsURLLoaderTest : public testing::Test {
         cloned_patterns.push_back(item.Clone());
       factory_params->factory_bound_allow_patterns = std::move(cloned_patterns);
     }
+    factory_params->is_trusted = is_trusted;
     factory_params->process_id = process_id;
     factory_params->is_corb_enabled = (process_id != mojom::kBrowserProcessId);
     constexpr int kRouteId = 765;
@@ -372,7 +376,7 @@ class CorsURLLoaderTest : public testing::Test {
   mojom::URLLoaderPtr url_loader_;
 
   // TestURLLoaderClient that records callback activities.
-  TestURLLoaderClient test_cors_loader_client_;
+  std::unique_ptr<TestURLLoaderClient> test_cors_loader_client_;
 
   // Holds for allowed origin access lists.
   OriginAccessList origin_access_list_;
@@ -1774,6 +1778,54 @@ TEST_F(CorsURLLoaderTest, OmitCredentialsModeOnNavigation) {
           "CorsURLLoaderFactory: unsupported credentials mode on navigation"));
 }
 
+// Make sure than when a request is failed due to having |trusted_params| set
+// and being sent to an untrusted URLLoaderFactory, no CORS request is made.
+TEST_F(CorsURLLoaderTest, TrustedParamsWithUntrustedFactoryFailsBeforeCORS) {
+  // Run the test with a trusted URLLoaderFactory as well, to make sure a CORS
+  // request is in fact made when using a trusted factory.
+  for (bool is_trusted : {false, true}) {
+    ResetFactory(base::nullopt, kRendererProcessId, is_trusted);
+
+    BadMessageTestHelper bad_message_helper;
+
+    ResourceRequest request;
+    request.mode = mojom::RequestMode::kCors;
+    request.credentials_mode = mojom::CredentialsMode::kOmit;
+    request.method = net::HttpRequestHeaders::kGetMethod;
+    request.url = GURL("http://other.com/foo.png");
+    request.request_initiator = url::Origin::Create(GURL("http://example.com"));
+    request.trusted_params = ResourceRequest::TrustedParams();
+    CreateLoaderAndStart(request);
+
+    if (!is_trusted) {
+      RunUntilComplete();
+      EXPECT_FALSE(IsNetworkLoaderStarted());
+      EXPECT_FALSE(client().has_received_redirect());
+      EXPECT_FALSE(client().has_received_response());
+      EXPECT_TRUE(client().has_received_completion());
+      EXPECT_EQ(net::ERR_INVALID_ARGUMENT,
+                client().completion_status().error_code);
+      EXPECT_THAT(
+          bad_message_helper.bad_message_reports(),
+          ::testing::ElementsAre(
+              "CorsURLLoaderFactory: Untrusted caller making trusted request"));
+    } else {
+      NotifyLoaderClientOnReceiveResponse(
+          {"Access-Control-Allow-Origin: http://example.com"});
+      NotifyLoaderClientOnComplete(net::OK);
+
+      RunUntilComplete();
+
+      EXPECT_TRUE(IsNetworkLoaderStarted());
+      EXPECT_TRUE(client().has_received_response());
+      EXPECT_TRUE(client().has_received_completion());
+      EXPECT_EQ(net::OK, client().completion_status().error_code);
+      EXPECT_TRUE(
+          GetRequest().headers.HasHeader(net::HttpRequestHeaders::kOrigin));
+    }
+  }
+}
+
 }  // namespace
 
 }  // namespace cors