Restrict setting some network headers through URLLoader interface.

The restricted headers potentially give Chrome and the server its
talking different views of what's going on for the rest of a
transaction, so it isn't a great idea to allow setting them.

Proxy headers are also restricted.

Bug: 973103
Change-Id: Id014181a332252d908c6255cc7c2593a4be781f4
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1659194Reviewed-by: Bill Budge <bbudge@chromium.org>
Reviewed-by: Asanka Herath <asanka@chromium.org>
Commit-Queue: Matt Menke <mmenke@chromium.org>
Cr-Commit-Position: refs/heads/master@{#669924}

ppapi/tests/test_url_loader.cc

@@ -692,7 +692,6 @@ std::string TestURLLoader::TestTrustedHttpRequests() {
     ASSERT_EQ(PP_OK, OpenTrusted("GET", "Accept-Charset:\n"));
     ASSERT_EQ(PP_OK, OpenTrusted("GET", "Accept-Encoding:\n"));
     ASSERT_EQ(PP_OK, OpenTrusted("GET", "Connection:\n"));
-    ASSERT_EQ(PP_OK, OpenTrusted("GET", "Content-Length:\n"));
     ASSERT_EQ(PP_OK, OpenTrusted("GET", "Cookie:\n"));
     ASSERT_EQ(PP_OK, OpenTrusted("GET", "Cookie2:\n"));
     ASSERT_EQ(PP_OK, OpenTrusted("GET", "Content-Transfer-Encoding:\n"));
@@ -704,14 +703,9 @@ std::string TestURLLoader::TestTrustedHttpRequests() {
     ASSERT_EQ(PP_OK, OpenTrusted("GET", "Keep-Alive:\n"));
     ASSERT_EQ(PP_OK, OpenTrusted("GET", "Referer:\n"));
     ASSERT_EQ(PP_OK, OpenTrusted("GET", "TE:\n"));
-    ASSERT_EQ(PP_OK, OpenTrusted("GET", "Trailer:\n"));
     ASSERT_EQ(PP_OK, OpenTrusted("GET", "Transfer-Encoding:\n"));
-    ASSERT_EQ(PP_OK, OpenTrusted("GET", "Upgrade:\n"));
     ASSERT_EQ(PP_OK, OpenTrusted("GET", "User-Agent:\n"));
     ASSERT_EQ(PP_OK, OpenTrusted("GET", "Via:\n"));
-    ASSERT_EQ(PP_OK,
-              OpenTrusted("GET",
-                  "Proxy-Authorization: Basic dXNlcjpwYXNzd29yZA==:\n"));
     ASSERT_EQ(PP_OK, OpenTrusted("GET", "Sec-foo:\n"));
   }
   // Trusted requests with custom referrer should succeed.

services/network/BUILD.gn

@@ -303,6 +303,7 @@ source_set("tests") {
     "ignore_errors_cert_verifier_unittest.cc",
     "initiator_lock_compatibility_unittest.cc",
     "keepalive_statistics_recorder_unittest.cc",
+    "loader_util_unittest.cc",
     "mojo_host_resolver_impl_unittest.cc",
     "network_change_manager_unittest.cc",
     "network_context_unittest.cc",

services/network/cors/cors_url_loader_unittest.cc

@@ -221,9 +221,11 @@ class CorsURLLoaderTest : public testing::Test {
     test_url_loader_factory_->NotifyClientOnComplete(error_code);
   }
 
-  void FollowRedirect(const std::vector<std::string>& removed_headers = {}) {
+  void FollowRedirect(const std::vector<std::string>& removed_headers = {},
+                      const net::HttpRequestHeaders& modified_headers =
+                          net::HttpRequestHeaders()) {
     DCHECK(url_loader_);
-    url_loader_->FollowRedirect(removed_headers, {} /*modified_headers*/,
+    url_loader_->FollowRedirect(removed_headers, modified_headers,
                                 base::nullopt /*new_url*/);
   }
 
@@ -1611,6 +1613,25 @@ TEST_F(CorsURLLoaderTest, RequestWithHostHeaderFails) {
   EXPECT_EQ(net::ERR_INVALID_ARGUMENT, client().completion_status().error_code);
 }
 
+TEST_F(CorsURLLoaderTest, RequestWithProxyAuthorizationHeaderFails) {
+  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::kProxyAuthorization,
+                            "Basic Zm9vOmJhcg==");
+  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);
@@ -1626,7 +1647,37 @@ TEST_F(CorsURLLoaderTest, SetHostHeaderOnRedirectFails) {
 
   ClearHasReceivedRedirect();
   // This should cause the request to fail.
-  AddHostHeaderAndFollowRedirect();
+  net::HttpRequestHeaders modified_headers;
+  modified_headers.SetHeader(net::HttpRequestHeaders::kHost, "bar.test");
+  FollowRedirect({} /* removed_headers */, modified_headers);
+
+  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);
+}
+
+TEST_F(CorsURLLoaderTest, SetProxyAuthorizationHeaderOnRedirectFails) {
+  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.
+  net::HttpRequestHeaders modified_headers;
+  modified_headers.SetHeader(net::HttpRequestHeaders::kProxyAuthorization,
+                             "Basic Zm9vOmJhcg==");
+  FollowRedirect({} /* removed_headers */, modified_headers);
 
   RunUntilComplete();
 

services/network/loader_util.cc

@@ -8,6 +8,7 @@
 
 #include "base/command_line.h"
 #include "base/logging.h"
+#include "base/strings/string_util.h"
 #include "base/strings/stringprintf.h"
 #include "net/base/load_flags.h"
 #include "net/base/mime_sniffer.h"
@@ -28,6 +29,44 @@ const char kFrameAcceptHeader[] =
     "image/apng,*/*;q=0.8";
 const char kDefaultAcceptHeader[] = "*/*";
 
+// Headers that consumers are not trusted to set. All "Proxy-" prefixed messages
+// are blocked inline. The"Authorization" auth header is deliberately not
+// included, since OAuth requires websites be able to set it directly.
+const char* kUnsafeHeaders[] = {
+    // This is determined by the upload body and set by net/. A consumer
+    // overriding that could allow for Bad Things.
+    net::HttpRequestHeaders::kContentLength,
+
+    // Disallow setting the Host header because it can conflict with specified
+    // URL and logic related to isolating sites.
+    net::HttpRequestHeaders::kHost,
+
+    // Trailers are not supported.
+    "Trailer",
+
+    // Websockets use a different API.
+    "Upgrade",
+
+    // TODO(mmenke): Gather stats on the following headers before adding them:
+    // Cookie, Cookie2, Referer, TE, Keep-Alive, Via.
+};
+
+// Headers that consumers are currently allowed to set, with the exception of
+// certain values could cause problems.
+// TODO(mmenke): Gather stats on these, and see if these headers can be banned
+// outright instead.
+const struct {
+  const char* name;
+  const char* value;
+} kUnsafeHeaderValues[] = {
+    // Websockets use a different API.
+    {net::HttpRequestHeaders::kConnection, "Upgrade"},
+
+    // Telling a server a non-chunked upload is chunked has similar implications
+    // as sending the wrong Content-Length.
+    {net::HttpRequestHeaders::kTransferEncoding, "Chunked"},
+};
+
 bool ShouldSniffContent(net::URLRequest* url_request,
                         ResourceResponse* response) {
   const std::string& mime_type = response->head.mime_type;
@@ -110,12 +149,26 @@ std::string ComputeReferrer(const GURL& referrer) {
 }
 
 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;
+  net::HttpRequestHeaders::Iterator it(request_headers);
+
+  while (it.GetNext()) {
+    for (const auto* header : kUnsafeHeaders) {
+      if (base::EqualsCaseInsensitiveASCII(header, it.name()))
+        return false;
+    }
+
+    for (const auto& header : kUnsafeHeaderValues) {
+      if (base::EqualsCaseInsensitiveASCII(header.name, it.name()) &&
+          base::EqualsCaseInsensitiveASCII(header.value, it.value())) {
+        return false;
+      }
+    }
+
+    // Proxy headers are destined for the proxy, so shouldn't be set by callers.
+    if (base::StartsWith(it.name(), "Proxy-",
+                         base::CompareCase::INSENSITIVE_ASCII)) {
+      return false;
+    }
   }
   return true;
 }

services/network/loader_util.h

@@ -55,8 +55,8 @@ scoped_refptr<HttpRawRequestResponseInfo> BuildRawRequestResponseInfo(
 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
+// Checks if any single header in a set of request headers is 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);

services/network/loader_util_unittest.cc

+// Copyright 2019 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 "services/network/loader_util.h"
+
+#include "base/strings/string_piece.h"
+#include "base/strings/string_util.h"
+#include "net/http/http_request_headers.h"
+#include "testing/gtest/include/gtest/gtest.h"
+
+namespace network {
+
+TEST(LoaderUtilTest, AreRequestHeadersSafe) {
+  const struct HeaderKeyValuePair {
+    const char* key;
+    const char* value;
+    bool is_safe;
+  } kHeaders[] = {
+      {"foo", "bar", true},
+
+      {net::HttpRequestHeaders::kContentLength, "42", false},
+      {net::HttpRequestHeaders::kHost, "foo.test", false},
+      {"Trailer", "header-names", false},
+      {"Upgrade", "websocket", false},
+      {"Upgrade", "webbedsocket", false},
+      {"hOsT", "foo.test", false},
+
+      {net::HttpRequestHeaders::kConnection, "Upgrade", false},
+      {net::HttpRequestHeaders::kConnection, "Close", true},
+      {net::HttpRequestHeaders::kTransferEncoding, "Chunked", false},
+      {net::HttpRequestHeaders::kTransferEncoding, "Chunky", true},
+      {"cOnNeCtIoN", "uPgRaDe", false},
+
+      {net::HttpRequestHeaders::kProxyAuthorization,
+       "Basic Zm9vOmJhcg==", false},
+      {"Proxy-Foo", "bar", false},
+      {"PrOxY-FoO", "bar", false},
+  };
+
+  // Check each header in isolation, and all combinations of two header
+  // key/value pairs that have different keys.
+  for (const auto& header1 : kHeaders) {
+    net::HttpRequestHeaders request_headers1;
+    request_headers1.SetHeader(header1.key, header1.value);
+    EXPECT_EQ(header1.is_safe, AreRequestHeadersSafe(request_headers1));
+
+    for (const auto& header2 : kHeaders) {
+      if (base::EqualsCaseInsensitiveASCII(header1.key, header2.key))
+        continue;
+
+      net::HttpRequestHeaders request_headers2;
+      request_headers2.SetHeader(header1.key, header1.value);
+      request_headers2.SetHeader(header2.key, header2.value);
+      EXPECT_EQ(header1.is_safe && header2.is_safe,
+                AreRequestHeadersSafe(request_headers2));
+    }
+  }
+}
+
+}  // namespace network

services/network/url_loader_unittest.cc

@@ -1896,38 +1896,47 @@ TEST_F(URLLoaderTest, RedirectModifiedHeaders) {
   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"));
+TEST_F(URLLoaderTest, RedirectFailsOnModifyUnsafeHeader) {
+  const char* kUnsafeHeaders[] = {
+      net::HttpRequestHeaders::kContentLength,
+      net::HttpRequestHeaders::kHost,
+      net::HttpRequestHeaders::kProxyConnection,
+      net::HttpRequestHeaders::kProxyAuthorization,
+      "Proxy-Foo",
+  };
 
-  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 */);
+  for (const auto* unsafe_header : kUnsafeHeaders) {
+    TestURLLoaderClient client;
 
-  client()->RunUntilRedirectReceived();
+    ResourceRequest request = CreateResourceRequest(
+        "GET", test_server()->GetURL("/redirect307-to-echo"));
 
-  net::HttpRequestHeaders redirect_headers;
-  redirect_headers.SetHeader(net::HttpRequestHeaders::kHost, "foo.test");
-  loader->FollowRedirect({}, redirect_headers, base::nullopt);
+    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()->RunUntilComplete();
-  delete_run_loop.Run();
+    client.RunUntilRedirectReceived();
 
-  EXPECT_TRUE(client()->has_received_completion());
-  EXPECT_EQ(net::ERR_INVALID_ARGUMENT,
-            client()->completion_status().error_code);
+    net::HttpRequestHeaders redirect_headers;
+    redirect_headers.SetHeader(unsafe_header, "foo");
+    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.