[Cors]URLLoaderFactory: Don't allow changing Host header on redirects.

(cherry picked from commit d31383577e0517843c8059dec9b87469bf30900f)

Bug: 973103
Change-Id: I463adfa133267b879e81875ee3252cc5fea684fa
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1655732
Commit-Queue: Matt Menke <mmenke@chromium.org>
Reviewed-by: Yutaka Hirano <yhirano@chromium.org>
Cr-Original-Commit-Position: refs/heads/master@{#668849}
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1663354
Reviewed-by: Matt Menke <mmenke@chromium.org>
Cr-Commit-Position: refs/branch-heads/3809@{#393}
Cr-Branched-From: d82dec1a818f378c464ba307ddd9c92133eac355-refs/heads/master@{#665002}

diff --git a/services/network/cors/cors_url_loader.cc b/services/network/cors/cors_url_loader.cc
index 2b1dc36..b09c9116 100644
--- a/services/network/cors/cors_url_loader.cc
+++ b/services/network/cors/cors_url_loader.cc

@@ -9,6 +9,7 @@
 #include "base/stl_util.h"
 #include "net/base/load_flags.h"
 #include "services/network/cors/preflight_controller.h"
+#include "services/network/loader_util.h"
 #include "services/network/public/cpp/cors/cors.h"
 #include "services/network/public/cpp/cors/origin_access_list.h"
 #include "url/url_util.h"
@@ -161,6 +162,11 @@
   }
   request_.headers.MergeFrom(modified_headers);
 
+  if (!AreRequestHeadersSafe(request_.headers)) {
+    HandleComplete(URLLoaderCompletionStatus(net::ERR_INVALID_ARGUMENT));
+    return;
+  }
+
   const std::string original_method = std::move(request_.method);
   request_.url = redirect_info_.new_url;
   request_.method = redirect_info_.new_method;

diff --git a/services/network/cors/cors_url_loader_factory.cc b/services/network/cors/cors_url_loader_factory.cc
index 20daf7b..7c3f7c28 100644
--- a/services/network/cors/cors_url_loader_factory.cc
+++ b/services/network/cors/cors_url_loader_factory.cc

@@ -14,6 +14,7 @@
 #include "services/network/cors/cors_url_loader.h"
 #include "services/network/cors/preflight_controller.h"
 #include "services/network/initiator_lock_compatibility.h"
+#include "services/network/loader_util.h"
 #include "services/network/network_context.h"
 #include "services/network/public/cpp/cors/cors.h"
 #include "services/network/public/cpp/features.h"
@@ -215,10 +216,8 @@
     }
   }
 
-  // Disallow setting the Host header over mojo::URLLoaderFactory interface
-  // because it can conflict with specified URL and make servers confused.
-  if (request.headers.HasHeader(net::HttpRequestHeaders::kHost)) {
-    LOG(WARNING) << "Host header should be set inside the network service";
+  if (!AreRequestHeadersSafe(request.headers) ||
+      !AreRequestHeadersSafe(request.cors_exempt_headers)) {
     return false;
   }
 

diff --git a/services/network/cors/cors_url_loader_unittest.cc b/services/network/cors/cors_url_loader_unittest.cc
index cc35cd9..3e465cb 100644
--- a/services/network/cors/cors_url_loader_unittest.cc
+++ b/services/network/cors/cors_url_loader_unittest.cc

@@ -227,6 +227,15 @@
                                 base::nullopt /*new_url*/);
   }
 
+  void AddHostHeaderAndFollowRedirect() {
+    DCHECK(url_loader_);
+    net::HttpRequestHeaders modified_headers;
+    modified_headers.SetHeader(net::HttpRequestHeaders::kHost, "bar.test");
+    url_loader_->FollowRedirect({},  // removed_headers
+                                modified_headers,
+                                base::nullopt);  // new_url
+  }
+
   const ResourceRequest& GetRequest() const {
     DCHECK(test_url_loader_factory_);
     return test_url_loader_factory_->request();
@@ -1584,6 +1593,49 @@
                 true, &origin_access_list));
 }
 
+TEST_F(CorsURLLoaderTest, RequestWithHostHeaderFails) {
+  ResourceRequest request;
+  request.fetch_request_mode = mojom::FetchRequestMode::kCors;
+  request.fetch_credentials_mode = mojom::FetchCredentialsMode::kOmit;
+  request.allow_credentials = false;
+  request.method = net::HttpRequestHeaders::kGetMethod;
+  request.url = GURL("https://foo.test/path");
+  request.request_initiator = url::Origin::Create(GURL("https://foo.test"));
+  request.headers.SetHeader(net::HttpRequestHeaders::kHost, "bar.test");
+  CreateLoaderAndStart(request);
+
+  RunUntilComplete();
+
+  EXPECT_FALSE(client().has_received_response());
+  EXPECT_TRUE(client().has_received_completion());
+  EXPECT_EQ(net::ERR_INVALID_ARGUMENT, client().completion_status().error_code);
+}
+
+TEST_F(CorsURLLoaderTest, SetHostHeaderOnRedirectFails) {
+  CreateLoaderAndStart(GURL("http://foo.test/"), GURL("http://foo.test/path"),
+                       mojom::FetchRequestMode::kCors);
+
+  NotifyLoaderClientOnReceiveRedirect(
+      CreateRedirectInfo(301, "GET", GURL("https://redirect.test/")));
+  RunUntilRedirectReceived();
+
+  EXPECT_TRUE(IsNetworkLoaderStarted());
+  EXPECT_TRUE(client().has_received_redirect());
+  EXPECT_FALSE(client().has_received_response());
+  EXPECT_FALSE(client().has_received_completion());
+
+  ClearHasReceivedRedirect();
+  // This should cause the request to fail.
+  AddHostHeaderAndFollowRedirect();
+
+  RunUntilComplete();
+
+  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);
+}
+
 }  // namespace
 
 }  // namespace cors

diff --git a/services/network/loader_util.cc b/services/network/loader_util.cc
index 598cab4..e1e104e 100644
--- a/services/network/loader_util.cc
+++ b/services/network/loader_util.cc

@@ -7,10 +7,12 @@
 #include <string>
 
 #include "base/command_line.h"
+#include "base/logging.h"
 #include "base/strings/stringprintf.h"
 #include "net/base/load_flags.h"
 #include "net/base/mime_sniffer.h"
 #include "net/http/http_raw_request_headers.h"
+#include "net/http/http_request_headers.h"
 #include "net/http/http_util.h"
 #include "net/url_request/url_request.h"
 #include "services/network/public/cpp/http_raw_request_response_info.h"
@@ -107,4 +109,15 @@
   return referrer.spec();
 }
 
+bool AreRequestHeadersSafe(const net::HttpRequestHeaders& request_headers) {
+  // Disallow setting the Host header because it can conflict with specified URL
+  // and logic related to isolating sites.
+  if (request_headers.HasHeader(net::HttpRequestHeaders::kHost)) {
+    LOG(WARNING)
+        << "Host header should not be set from outside the network service";
+    return false;
+  }
+  return true;
+}
+
 }  // namespace network

diff --git a/services/network/loader_util.h b/services/network/loader_util.h
index 5f41716..d73f315 100644
--- a/services/network/loader_util.h
+++ b/services/network/loader_util.h

@@ -12,6 +12,7 @@
 
 namespace net {
 class HttpRawRequestHeaders;
+class HttpRequestHeaders;
 class HttpResponseHeaders;
 class URLRequest;
 }  // namespace net
@@ -54,6 +55,12 @@
 COMPONENT_EXPORT(NETWORK_SERVICE)
 std::string ComputeReferrer(const GURL& referrer);
 
+// Any single headers in a set of request headers are not safe to send. When
+// adding sets of headers together, it's safe to call this on each set
+// individually.
+COMPONENT_EXPORT(NETWORK_SERVICE)
+bool AreRequestHeadersSafe(const net::HttpRequestHeaders& request_headers);
+
 }  // namespace network
 
 #endif  // SERVICES_NETWORK_LOADER_UTIL_H_

diff --git a/services/network/url_loader.cc b/services/network/url_loader.cc
index d4eee42..3643edd 100644
--- a/services/network/url_loader.cc
+++ b/services/network/url_loader.cc

@@ -395,6 +395,9 @@
   // they should be ignored by CORS checks.
   net::HttpRequestHeaders merged_headers = request.headers;
   merged_headers.MergeFrom(request.cors_exempt_headers);
+  // This should be ensured by the CorsURLLoaderFactory(), which is called
+  // before URLLoaders are created.
+  DCHECK(AreRequestHeadersSafe(merged_headers));
   url_request_->SetExtraRequestHeaders(merged_headers);
 
   url_request_->SetUserData(kUserDataKey,
@@ -647,6 +650,14 @@
     return;
   }
 
+  // Removing headers can't make the set of pre-existing headers unsafe, but
+  // adding headers can.
+  if (!AreRequestHeadersSafe(modified_headers)) {
+    NotifyCompleted(net::ERR_INVALID_ARGUMENT);
+    // |this| may have been deleted.
+    return;
+  }
+
   deferred_redirect_url_.reset();
   new_redirect_url_ = new_url;
 

diff --git a/services/network/url_loader_unittest.cc b/services/network/url_loader_unittest.cc
index 558086d..46237e6 100644
--- a/services/network/url_loader_unittest.cc
+++ b/services/network/url_loader_unittest.cc

@@ -1841,6 +1841,40 @@
   EXPECT_EQ("Value3", request_headers2.find("Header3")->second);
 }
 
+// Make sure requests are failed when the consumer tries to modify the host
+// header.
+TEST_F(URLLoaderTest, RedirectFailsOnModifyHostHeader) {
+  ResourceRequest request = CreateResourceRequest(
+      "GET", test_server()->GetURL("/redirect307-to-echo"));
+
+  base::RunLoop delete_run_loop;
+  mojom::URLLoaderPtr loader;
+  std::unique_ptr<URLLoader> url_loader;
+  mojom::URLLoaderFactoryParams params;
+  params.process_id = mojom::kBrowserProcessId;
+  params.is_corb_enabled = false;
+  url_loader = std::make_unique<URLLoader>(
+      context(), nullptr /* network_service_client */,
+      DeleteLoaderCallback(&delete_run_loop, &url_loader),
+      mojo::MakeRequest(&loader), mojom::kURLLoadOptionNone, request,
+      client()->CreateInterfacePtr(), TRAFFIC_ANNOTATION_FOR_TESTS, &params,
+      0 /* request_id */, resource_scheduler_client(), nullptr,
+      nullptr /* network_usage_accumulator */, nullptr /* header_client */);
+
+  client()->RunUntilRedirectReceived();
+
+  net::HttpRequestHeaders redirect_headers;
+  redirect_headers.SetHeader(net::HttpRequestHeaders::kHost, "foo.test");
+  loader->FollowRedirect({}, redirect_headers, base::nullopt);
+
+  client()->RunUntilComplete();
+  delete_run_loop.Run();
+
+  EXPECT_TRUE(client()->has_received_completion());
+  EXPECT_EQ(net::ERR_INVALID_ARGUMENT,
+            client()->completion_status().error_code);
+}
+
 // Test the client can remove headers during a redirect.
 TEST_F(URLLoaderTest, RedirectRemoveHeader) {
   ResourceRequest request = CreateResourceRequest(