Sanitize headers in Proxy Authentication Required responses

BUG=431504

Review URL: https://codereview.chromium.org/769043003

Cr-Commit-Position: refs/heads/master@{#310014}

5 files changed, 150 insertions, 44 deletions

diff --git a/net/http/http_network_transaction_unittest.cc b/net/http/http_network_transaction_unittest.cc
index 19dfeb7..21a8c00 100644
--- a/net/http/http_network_transaction_unittest.cc
+++ b/net/http/http_network_transaction_unittest.cc
@@ -2546,11 +2546,11 @@ TEST_P(HttpNetworkTransactionTest, BasicAuthProxyKeepAlive) {
       NetLog::PHASE_NONE);
 
   const HttpResponseInfo* response = trans->GetResponseInfo();
-  ASSERT_TRUE(response != NULL);
-  ASSERT_FALSE(response->headers.get() == NULL);
+  ASSERT_TRUE(response);
+  ASSERT_TRUE(response->headers);
   EXPECT_TRUE(response->headers->IsKeepAlive());
   EXPECT_EQ(407, response->headers->response_code());
-  EXPECT_EQ(10, response->headers->GetContentLength());
+  EXPECT_EQ(-1, response->headers->GetContentLength());
   EXPECT_TRUE(HttpVersion(1, 1) == response->headers->GetHttpVersion());
   EXPECT_TRUE(CheckBasicProxyAuth(response->auth_challenge.get()));
 
@@ -2565,11 +2565,11 @@ TEST_P(HttpNetworkTransactionTest, BasicAuthProxyKeepAlive) {
   EXPECT_EQ(OK, rv);
 
   response = trans->GetResponseInfo();
-  ASSERT_TRUE(response != NULL);
-  ASSERT_FALSE(response->headers.get() == NULL);
+  ASSERT_TRUE(response);
+  ASSERT_TRUE(response->headers);
   EXPECT_TRUE(response->headers->IsKeepAlive());
   EXPECT_EQ(407, response->headers->response_code());
-  EXPECT_EQ(10, response->headers->GetContentLength());
+  EXPECT_EQ(-1, response->headers->GetContentLength());
   EXPECT_TRUE(HttpVersion(1, 1) == response->headers->GetHttpVersion());
   EXPECT_TRUE(CheckBasicProxyAuth(response->auth_challenge.get()));
 
@@ -2603,10 +2603,11 @@ TEST_P(HttpNetworkTransactionTest, BasicAuthProxyCancelTunnel) {
 
   // The proxy responds to the connect with a 407.
   MockRead data_reads[] = {
-    MockRead("HTTP/1.1 407 Proxy Authentication Required\r\n"),
-    MockRead("Proxy-Authenticate: Basic realm=\"MyRealm1\"\r\n"),
-    MockRead("Content-Length: 10\r\n\r\n"),
-    MockRead(SYNCHRONOUS, ERR_UNEXPECTED),  // Should not be reached.
+      MockRead("HTTP/1.1 407 Proxy Authentication Required\r\n"),
+      MockRead("Proxy-Authenticate: Basic realm=\"MyRealm1\"\r\n"),
+      MockRead("Content-Length: 10\r\n\r\n"),
+      MockRead("0123456789"),  // Should not be reached.
+      MockRead(SYNCHRONOUS, ERR_UNEXPECTED),
   };
 
   StaticSocketDataProvider data(data_reads, arraysize(data_reads),
@@ -2622,12 +2623,74 @@ TEST_P(HttpNetworkTransactionTest, BasicAuthProxyCancelTunnel) {
   EXPECT_EQ(OK, rv);
 
   const HttpResponseInfo* response = trans->GetResponseInfo();
-  ASSERT_TRUE(response != NULL);
+  ASSERT_TRUE(response);
+  ASSERT_TRUE(response->headers);
+  EXPECT_TRUE(response->headers->IsKeepAlive());
+  EXPECT_EQ(407, response->headers->response_code());
+  EXPECT_TRUE(HttpVersion(1, 1) == response->headers->GetHttpVersion());
+
+  std::string response_data;
+  rv = ReadTransaction(trans.get(), &response_data);
+  EXPECT_EQ(ERR_TUNNEL_CONNECTION_FAILED, rv);
+
+  // Flush the idle socket before the HttpNetworkTransaction goes out of scope.
+  session->CloseAllConnections();
+}
+
+// Test that we don't pass extraneous headers from the proxy's response to the
+// caller when the proxy responds to CONNECT with 407.
+TEST_P(HttpNetworkTransactionTest, SanitizeProxyAuthHeaders) {
+  HttpRequestInfo request;
+  request.method = "GET";
+  request.url = GURL("https://www.google.com/");
+  request.load_flags = 0;
+
+  // Configure against proxy server "myproxy:70".
+  session_deps_.proxy_service.reset(ProxyService::CreateFixed("myproxy:70"));
+
+  scoped_refptr<HttpNetworkSession> session(CreateSession(&session_deps_));
+
+  scoped_ptr<HttpTransaction> trans(
+      new HttpNetworkTransaction(DEFAULT_PRIORITY, session.get()));
+
+  // Since we have proxy, should try to establish tunnel.
+  MockWrite data_writes[] = {
+      MockWrite(
+          "CONNECT www.google.com:443 HTTP/1.1\r\n"
+          "Host: www.google.com\r\n"
+          "Proxy-Connection: keep-alive\r\n\r\n"),
+  };
+
+  // The proxy responds to the connect with a 407.
+  MockRead data_reads[] = {
+      MockRead("HTTP/1.1 407 Proxy Authentication Required\r\n"),
+      MockRead("X-Foo: bar\r\n"),
+      MockRead("Set-Cookie: foo=bar\r\n"),
+      MockRead("Proxy-Authenticate: Basic realm=\"MyRealm1\"\r\n"),
+      MockRead("Content-Length: 10\r\n\r\n"),
+      MockRead(SYNCHRONOUS, ERR_UNEXPECTED),  // Should not be reached.
+  };
+
+  StaticSocketDataProvider data(data_reads, arraysize(data_reads), data_writes,
+                                arraysize(data_writes));
+  session_deps_.socket_factory->AddSocketDataProvider(&data);
+
+  TestCompletionCallback callback;
 
+  int rv = trans->Start(&request, callback.callback(), BoundNetLog());
+  EXPECT_EQ(ERR_IO_PENDING, rv);
+
+  rv = callback.WaitForResult();
+  EXPECT_EQ(OK, rv);
+
+  const HttpResponseInfo* response = trans->GetResponseInfo();
+  ASSERT_TRUE(response);
+  ASSERT_TRUE(response->headers);
   EXPECT_TRUE(response->headers->IsKeepAlive());
   EXPECT_EQ(407, response->headers->response_code());
-  EXPECT_EQ(10, response->headers->GetContentLength());
   EXPECT_TRUE(HttpVersion(1, 1) == response->headers->GetHttpVersion());
+  EXPECT_FALSE(response->headers->HasHeader("X-Foo"));
+  EXPECT_FALSE(response->headers->HasHeader("Set-Cookie"));
 
   std::string response_data;
   rv = ReadTransaction(trans.get(), &response_data);
diff --git a/net/http/http_proxy_client_socket.cc b/net/http/http_proxy_client_socket.cc
index 515301c..97945d9 100644
--- a/net/http/http_proxy_client_socket.cc
+++ b/net/http/http_proxy_client_socket.cc
@@ -483,25 +483,27 @@ int HttpProxyClientSocket::DoReadHeadersComplete(int result) {
       // sanitize the response.  This still allows a rogue HTTPS proxy to
       // redirect an HTTPS site load to a similar-looking site, but no longer
       // allows it to impersonate the site the user requested.
-      if (is_https_proxy_ && SanitizeProxyRedirect(&response_, request_.url)) {
-        bool is_connection_reused = http_stream_parser_->IsConnectionReused();
-        redirect_has_load_timing_info_ =
-            transport_->GetLoadTimingInfo(
-                is_connection_reused, &redirect_load_timing_info_);
-        transport_.reset();
-        http_stream_parser_.reset();
-        return ERR_HTTPS_PROXY_TUNNEL_RESPONSE;
+      if (!is_https_proxy_ || !SanitizeProxyRedirect(&response_)) {
+        LogBlockedTunnelResponse();
+        return ERR_TUNNEL_CONNECTION_FAILED;
       }
 
-      // We're not using an HTTPS proxy, or we couldn't sanitize the redirect.
-      LogBlockedTunnelResponse();
-      return ERR_TUNNEL_CONNECTION_FAILED;
+      redirect_has_load_timing_info_ = transport_->GetLoadTimingInfo(
+          http_stream_parser_->IsConnectionReused(),
+          &redirect_load_timing_info_);
+      transport_.reset();
+      http_stream_parser_.reset();
+      return ERR_HTTPS_PROXY_TUNNEL_RESPONSE;
 
     case 407:  // Proxy Authentication Required
       // We need this status code to allow proxy authentication.  Our
       // authentication code is smart enough to avoid being tricked by an
       // active network attacker.
       // The next state is intentionally not set as it should be STATE_NONE;
+      if (!SanitizeProxyAuth(&response_)) {
+        LogBlockedTunnelResponse();
+        return ERR_TUNNEL_CONNECTION_FAILED;
+      }
       return HandleProxyAuthChallenge(auth_.get(), &response_, net_log_);
 
     default:
diff --git a/net/http/proxy_client_socket.cc b/net/http/proxy_client_socket.cc
index dcfae03..3c539c6 100644
--- a/net/http/proxy_client_socket.cc
+++ b/net/http/proxy_client_socket.cc
@@ -17,6 +17,20 @@
 
 namespace net {
 
+namespace {
+
+void CopyHeaderValues(scoped_refptr<HttpResponseHeaders> source,
+                      scoped_refptr<HttpResponseHeaders> dest,
+                      const std::string& header_name) {
+  void* iter = NULL;
+  std::string header_value;
+
+  while (source->EnumerateHeader(&iter, header_name, &header_value))
+    dest->AddHeader(header_name + ": " + header_value);
+}
+
+}  // namespace
+
 // static
 void ProxyClientSocket::BuildTunnelRequest(
     const HttpRequestInfo& request_info,
@@ -72,22 +86,39 @@ void ProxyClientSocket::LogBlockedTunnelResponse(int http_status_code,
 }
 
 // static
-bool ProxyClientSocket::SanitizeProxyRedirect(HttpResponseInfo* response,
-                                              const GURL& url) {
+bool ProxyClientSocket::SanitizeProxyAuth(HttpResponseInfo* response) {
+  DCHECK(response && response->headers.get());
+
+  scoped_refptr<HttpResponseHeaders> old_headers = response->headers;
+
+  const char kHeaders[] = "HTTP/1.1 407 Proxy Authentication Required\n\n";
+  scoped_refptr<HttpResponseHeaders> new_headers = new HttpResponseHeaders(
+      HttpUtil::AssembleRawHeaders(kHeaders, arraysize(kHeaders)));
+
+  new_headers->ReplaceStatusLine(old_headers->GetStatusLine());
+  CopyHeaderValues(old_headers, new_headers, "Connection");
+  CopyHeaderValues(old_headers, new_headers, "Proxy-Authenticate");
+
+  response->headers = new_headers;
+  return true;
+}
+
+// static
+bool ProxyClientSocket::SanitizeProxyRedirect(HttpResponseInfo* response) {
   DCHECK(response && response->headers.get());
 
   std::string location;
   if (!response->headers->IsRedirect(&location))
     return false;
 
-  // Return minimal headers; set "Content-length: 0" to ignore response body.
-  std::string fake_response_headers =
-      base::StringPrintf("HTTP/1.0 302 Found\n"
-                         "Location: %s\n"
-                         "Content-length: 0\n"
-                         "Connection: close\n"
-                         "\n",
-                         location.c_str());
+  // Return minimal headers; set "Content-Length: 0" to ignore response body.
+  std::string fake_response_headers = base::StringPrintf(
+      "HTTP/1.0 302 Found\n"
+      "Location: %s\n"
+      "Content-Length: 0\n"
+      "Connection: close\n"
+      "\n",
+      location.c_str());
   std::string raw_headers =
       HttpUtil::AssembleRawHeaders(fake_response_headers.data(),
                                    fake_response_headers.length());
diff --git a/net/http/proxy_client_socket.h b/net/http/proxy_client_socket.h
index aa59038..f3347c8 100644
--- a/net/http/proxy_client_socket.h
+++ b/net/http/proxy_client_socket.h
@@ -74,13 +74,19 @@ class NET_EXPORT_PRIVATE ProxyClientSocket : public StreamSocket {
                                        const GURL& url,
                                        bool is_https_proxy);
 
+  // When a proxy authentication response is received during tunnel
+  // construction, this method should be called to strip everything
+  // but the auth header from the redirect response.  If it returns
+  // false, the response should be discarded and tunnel construction should
+  // fail.
+  static bool SanitizeProxyAuth(HttpResponseInfo* response);
+
   // When a redirect (e.g. 302 response) is received during tunnel
   // construction, this method should be called to strip everything
   // but the Location header from the redirect response.  If it returns
   // false, the response should be discarded and tunnel construction should
-  // fail.  |url| is for logging purposes.
-  static bool SanitizeProxyRedirect(HttpResponseInfo* response,
-                                    const GURL& url);
+  // fail.
+  static bool SanitizeProxyRedirect(HttpResponseInfo* response);
 
  private:
   DISALLOW_COPY_AND_ASSIGN(ProxyClientSocket);
diff --git a/net/spdy/spdy_proxy_client_socket.cc b/net/spdy/spdy_proxy_client_socket.cc
index afc1953..661cfd9 100644
--- a/net/spdy/spdy_proxy_client_socket.cc
+++ b/net/spdy/spdy_proxy_client_socket.cc
@@ -414,20 +414,24 @@ int SpdyProxyClientSocket::DoReadReplyComplete(int result) {
     case 302:  // Found / Moved Temporarily
       // Try to return a sanitized response so we can follow auth redirects.
       // If we can't, fail the tunnel connection.
-      if (SanitizeProxyRedirect(&response_, request_.url)) {
-        redirect_has_load_timing_info_ =
-            spdy_stream_->GetLoadTimingInfo(&redirect_load_timing_info_);
-        // Note that this triggers a RST_STREAM_CANCEL.
-        spdy_stream_->DetachDelegate();
-        next_state_ = STATE_DISCONNECTED;
-        return ERR_HTTPS_PROXY_TUNNEL_RESPONSE;
-      } else {
+      if (!SanitizeProxyRedirect(&response_)) {
         LogBlockedTunnelResponse();
         return ERR_TUNNEL_CONNECTION_FAILED;
       }
 
+      redirect_has_load_timing_info_ =
+          spdy_stream_->GetLoadTimingInfo(&redirect_load_timing_info_);
+      // Note that this triggers a RST_STREAM_CANCEL.
+      spdy_stream_->DetachDelegate();
+      next_state_ = STATE_DISCONNECTED;
+      return ERR_HTTPS_PROXY_TUNNEL_RESPONSE;
+
     case 407:  // Proxy Authentication Required
       next_state_ = STATE_OPEN;
+      if (!SanitizeProxyAuth(&response_)) {
+        LogBlockedTunnelResponse();
+        return ERR_TUNNEL_CONNECTION_FAILED;
+      }
       return HandleProxyAuthChallenge(auth_.get(), &response_, net_log_);
 
     default: