Restrict support for redirecting CONNECT tunnels by HTTPS proxies.

* 302s during CONNECT for subresources now fails with ERR_HTTPS_PROXY_TUNNEL_RESPONSE_REDIRECT. * 302s during CONNECT are rejected for auto-detected proxies * Non auto-detected proxies are still able to redirect CONNECTs for main frames, as we evaluate the compatibility impact for removing it. This CL also records the frequency of redirects during CONNECT in the Net.Proxy.RedirectDuringConnect histogram. Bug: 928551 Change-Id: I438d9cf477486e5f7b5f76df2de5fe88028f14da TBR: atwilson@chromium.org Reviewed-on: https://chromium-review.googlesource.com/c/1474835 Commit-Queue: Eric Roman <eroman@chromium.org> Reviewed-by: Mark Pearson <mpearson@chromium.org> Reviewed-by: Matt Menke <mmenke@chromium.org> Cr-Commit-Position: refs/heads/master@{#633970}
nwjs · Feb 21, 2019 · 74103c7 · 74103c7
1 parent e55866b
commit 74103c7
Show file tree

Hide file tree

Showing 32 changed files with 417 additions and 128 deletions.
diff --git a/components/policy/core/common/cloud/device_management_service.cc b/components/policy/core/common/cloud/device_management_service.cc
@@ -64,7 +64,7 @@ bool IsProxyError(int net_error) {
     case net::ERR_PROXY_CONNECTION_FAILED:
     case net::ERR_TUNNEL_CONNECTION_FAILED:
     case net::ERR_PROXY_AUTH_UNSUPPORTED:
-    case net::ERR_HTTPS_PROXY_TUNNEL_RESPONSE:
+    case net::ERR_HTTPS_PROXY_TUNNEL_RESPONSE_REDIRECT:
     case net::ERR_MANDATORY_PROXY_CONFIGURATION_FAILED:
     case net::ERR_PROXY_CERTIFICATE_INVALID:
     case net::ERR_SOCKS_CONNECTION_FAILED:

diff --git a/net/base/net_error_list.h b/net/base/net_error_list.h
@@ -261,9 +261,12 @@ NET_ERROR(NETWORK_ACCESS_DENIED, -138)
 NET_ERROR(TEMPORARILY_THROTTLED, -139)
 
 // A request to create an SSL tunnel connection through the HTTPS proxy
-// received a non-200 (OK) and non-407 (Proxy Auth) response.  The response
-// body might include a description of why the request failed.
-NET_ERROR(HTTPS_PROXY_TUNNEL_RESPONSE, -140)
+// received a 302 (temporary redirect) response.  The response body might
+// include a description of why the request failed.
+//
+// TODO(https://crbug.com/928551): This is deprecated and should not be used by
+// new code.
+NET_ERROR(HTTPS_PROXY_TUNNEL_RESPONSE_REDIRECT, -140)
 
 // We were unable to sign the CertificateVerify data of an SSL client auth
 // handshake with the client certificate's private key.

diff --git a/net/http/bidirectional_stream.cc b/net/http/bidirectional_stream.cc
@@ -420,14 +420,14 @@ void BidirectionalStream::OnNeedsClientAuth(const SSLConfig& used_ssl_config,
   StartRequest(ssl_config);
 }
 
-void BidirectionalStream::OnHttpsProxyTunnelResponse(
+void BidirectionalStream::OnHttpsProxyTunnelResponseRedirect(
     const HttpResponseInfo& response_info,
     const SSLConfig& used_ssl_config,
     const ProxyInfo& used_proxy_info,
     std::unique_ptr<HttpStream> stream) {
   DCHECK(stream_request_);
 
-  NotifyFailed(ERR_HTTPS_PROXY_TUNNEL_RESPONSE);
+  NotifyFailed(ERR_HTTPS_PROXY_TUNNEL_RESPONSE_REDIRECT);
 }
 
 void BidirectionalStream::OnQuicBroken() {}

diff --git a/net/http/bidirectional_stream.h b/net/http/bidirectional_stream.h
@@ -218,10 +218,11 @@ class NET_EXPORT BidirectionalStream : public BidirectionalStreamImpl::Delegate,
                         HttpAuthController* auth_controller) override;
   void OnNeedsClientAuth(const SSLConfig& used_ssl_config,
                          SSLCertRequestInfo* cert_info) override;
-  void OnHttpsProxyTunnelResponse(const HttpResponseInfo& response_info,
-                                  const SSLConfig& used_ssl_config,
-                                  const ProxyInfo& used_proxy_info,
-                                  std::unique_ptr<HttpStream> stream) override;
+  void OnHttpsProxyTunnelResponseRedirect(
+      const HttpResponseInfo& response_info,
+      const SSLConfig& used_ssl_config,
+      const ProxyInfo& used_proxy_info,
+      std::unique_ptr<HttpStream> stream) override;
   void OnQuicBroken() override;
 
   // Helper method to notify delegate if there is an error.

diff --git a/net/http/http_network_transaction.cc b/net/http/http_network_transaction.cc
@@ -648,7 +648,7 @@ void HttpNetworkTransaction::OnNeedsClientAuth(
   OnIOComplete(ERR_SSL_CLIENT_AUTH_CERT_NEEDED);
 }
 
-void HttpNetworkTransaction::OnHttpsProxyTunnelResponse(
+void HttpNetworkTransaction::OnHttpsProxyTunnelResponseRedirect(
     const HttpResponseInfo& response_info,
     const SSLConfig& used_ssl_config,
     const ProxyInfo& used_proxy_info,
@@ -668,7 +668,7 @@ void HttpNetworkTransaction::OnHttpsProxyTunnelResponse(
   stream_ = std::move(stream);
   stream_->SetRequestHeadersCallback(request_headers_callback_);
   stream_request_.reset();  // we're done with the stream request
-  OnIOComplete(ERR_HTTPS_PROXY_TUNNEL_RESPONSE);
+  OnIOComplete(ERR_HTTPS_PROXY_TUNNEL_RESPONSE_REDIRECT);
 }
 
 void HttpNetworkTransaction::OnQuicBroken() {
@@ -854,22 +854,21 @@ int HttpNetworkTransaction::DoCreateStream() {
 }
 
 int HttpNetworkTransaction::DoCreateStreamComplete(int result) {
-  // If |result| is ERR_HTTPS_PROXY_TUNNEL_RESPONSE, then
-  // DoCreateStreamComplete is being called from OnHttpsProxyTunnelResponse,
-  // which resets the stream request first. Therefore, we have to grab the
-  // connection attempts in *that* function instead of here in that case.
-  if (result != ERR_HTTPS_PROXY_TUNNEL_RESPONSE)
+  // If |result| is ERR_HTTPS_PROXY_TUNNEL_RESPONSE_REDIRECT, then
+  // DoCreateStreamComplete is being called from
+  // OnHttpsProxyTunnelResponseRedirect, which resets the stream request first.
+  // Therefore, we have to grab the connection attempts in *that* function
+  // instead of here in that case.
+  if (result != ERR_HTTPS_PROXY_TUNNEL_RESPONSE_REDIRECT)
     CopyConnectionAttemptsFromStreamRequest();
 
   if (result == OK) {
     next_state_ = STATE_INIT_STREAM;
     DCHECK(stream_.get());
   } else if (result == ERR_SSL_CLIENT_AUTH_CERT_NEEDED) {
     result = HandleCertificateRequest(result);
-  } else if (result == ERR_HTTPS_PROXY_TUNNEL_RESPONSE) {
-    // Return OK and let the caller read the proxy's error page
-    next_state_ = STATE_NONE;
-    return OK;
+  } else if (result == ERR_HTTPS_PROXY_TUNNEL_RESPONSE_REDIRECT) {
+    return DoCreateStreamCompletedTunnelResponseRedirect();
   } else if (result == ERR_HTTP_1_1_REQUIRED ||
              result == ERR_PROXY_HTTP_1_1_REQUIRED) {
     return HandleHttp11Required(result);
@@ -1943,4 +1942,45 @@ bool HttpNetworkTransaction::ContentEncodingsValid() const {
   return result;
 }
 
+static HttpNetworkTransaction::TunnelRedirectHistogramValue
+GetTunnelRedirectHistogramValue(bool is_main_frame, bool was_auto_detected) {
+  if (!is_main_frame && !was_auto_detected)
+    return HttpNetworkTransaction::kSubresourceByExplicitProxy;
+  if (is_main_frame && !was_auto_detected)
+    return HttpNetworkTransaction::kMainFrameByExplicitProxy;
+  if (!is_main_frame && was_auto_detected)
+    return HttpNetworkTransaction::kSubresourceByAutoDetectedProxy;
+  return HttpNetworkTransaction::kMainFrameByAutoDetectedProxy;
+}
+
+// TODO(https://crbug.com/928551): Support for redirect on CONNECT is
+// deprecated, and support will be removed.
+//
+// The code in this method handles the temporary histogramming and
+// compatibility-mode policy during the phase-out.
+int HttpNetworkTransaction::DoCreateStreamCompletedTunnelResponseRedirect() {
+  bool is_main_frame = (request_->load_flags & LOAD_MAIN_FRAME_DEPRECATED) ==
+                       LOAD_MAIN_FRAME_DEPRECATED;
+  bool was_auto_detected = proxy_info_.did_use_auto_detected_pac_script();
+
+  UMA_HISTOGRAM_ENUMERATION(
+      "Net.Proxy.RedirectDuringConnect",
+      GetTunnelRedirectHistogramValue(is_main_frame, was_auto_detected));
+
+  // For legacy compatibility, the proxy is allowed to redirect CONNECT
+  // if:
+  //      (a) the request was for a top-level frame
+  //      (b) the proxy server was explicitly configured (i.e. not
+  //          auto-detected).
+  if (is_main_frame && !was_auto_detected) {
+    // Return OK and let the caller read the proxy's error page
+    next_state_ = STATE_NONE;
+    return OK;
+  }
+
+  // Otherwise let the request fail.
+  stream_.reset();
+  return ERR_HTTPS_PROXY_TUNNEL_RESPONSE_REDIRECT;
+}
+
 }  // namespace net
diff --git a/net/http/http_network_transaction.h b/net/http/http_network_transaction.h
@@ -50,6 +50,16 @@ class NET_EXPORT_PRIVATE HttpNetworkTransaction
     : public HttpTransaction,
       public HttpStreamRequest::Delegate {
  public:
+  // Enumeration used by Net.Proxy.RedirectDuringConnect. Exposed here for
+  // sharing by unit-tests.
+  enum TunnelRedirectHistogramValue {
+    kSubresourceByExplicitProxy = 0,
+    kMainFrameByExplicitProxy = 1,
+    kSubresourceByAutoDetectedProxy = 2,
+    kMainFrameByAutoDetectedProxy = 3,
+    kMaxValue = kMainFrameByAutoDetectedProxy
+  };
+
   HttpNetworkTransaction(RequestPriority priority,
                          HttpNetworkSession* session);
 
@@ -117,10 +127,11 @@ class NET_EXPORT_PRIVATE HttpNetworkTransaction
                         HttpAuthController* auth_controller) override;
   void OnNeedsClientAuth(const SSLConfig& used_ssl_config,
                          SSLCertRequestInfo* cert_info) override;
-  void OnHttpsProxyTunnelResponse(const HttpResponseInfo& response_info,
-                                  const SSLConfig& used_ssl_config,
-                                  const ProxyInfo& used_proxy_info,
-                                  std::unique_ptr<HttpStream> stream) override;
+  void OnHttpsProxyTunnelResponseRedirect(
+      const HttpResponseInfo& response_info,
+      const SSLConfig& used_ssl_config,
+      const ProxyInfo& used_proxy_info,
+      std::unique_ptr<HttpStream> stream) override;
 
   void OnQuicBroken() override;
   void GetConnectionAttempts(ConnectionAttempts* out) const override;
@@ -309,6 +320,10 @@ class NET_EXPORT_PRIVATE HttpNetworkTransaction
   // "Accept-Encoding".
   bool ContentEncodingsValid() const;
 
+  // Logic for handling ERR_HTTPS_PROXY_TUNNEL_RESPONSE_REDIRECT seen during
+  // DoCreateStreamCompletedTunnel().
+  int DoCreateStreamCompletedTunnelResponseRedirect();
+
   scoped_refptr<HttpAuthController>
       auth_controllers_[HttpAuth::AUTH_NUM_TARGETS];
 

diff --git a/net/http/http_network_transaction_unittest.cc b/net/http/http_network_transaction_unittest.cc
@@ -9498,7 +9498,7 @@ TEST_F(HttpNetworkTransactionTest, HTTPSViaHttpsProxy) {
                                  CONNECT_TIMING_HAS_SSL_TIMES);
 }
 
-// Test an HTTPS Proxy's ability to redirect a CONNECT request
+// Test that an HTTPS Proxy can redirect a CONNECT request for main frames.
 TEST_F(HttpNetworkTransactionTest, RedirectOfHttpsConnectViaHttpsProxy) {
   session_deps_.proxy_resolution_service =
       ProxyResolutionService::CreateFixedFromPacResult(
@@ -9511,6 +9511,7 @@ TEST_F(HttpNetworkTransactionTest, RedirectOfHttpsConnectViaHttpsProxy) {
   session_deps_.host_resolver->set_ondemand_mode(true);
 
   HttpRequestInfo request;
+  request.load_flags = LOAD_MAIN_FRAME_DEPRECATED;
   request.method = "GET";
   request.url = GURL("https://www.example.org/");
   request.traffic_annotation =
@@ -9582,7 +9583,7 @@ TEST_F(HttpNetworkTransactionTest, RedirectOfHttpsConnectViaHttpsProxy) {
   // name, and negotiate an SSL connection to it (Neither of which are done in
   // this case), which the DNS and SSL times for the proxy are all included in
   // connect_start / connect_end. See
-  // HttpNetworkTransaction::OnHttpsProxyTunnelResponse.
+  // HttpNetworkTransaction::OnHttpsProxyTunnelResponseRedirect
 
   EXPECT_TRUE(load_timing_info.connect_timing.dns_start.is_null());
   EXPECT_TRUE(load_timing_info.connect_timing.dns_end.is_null());
@@ -9600,8 +9601,114 @@ TEST_F(HttpNetworkTransactionTest, RedirectOfHttpsConnectViaHttpsProxy) {
   EXPECT_TRUE(load_timing_info.receive_headers_end.is_null());
 }
 
-// Test an HTTPS (SPDY) Proxy's ability to redirect a CONNECT request
+// Test that an HTTPS Proxy cannot redirect a CONNECT request for subresources.
+TEST_F(HttpNetworkTransactionTest,
+       RedirectOfHttpsConnectSubresourceViaHttpsProxy) {
+  base::HistogramTester histograms;
+  session_deps_.proxy_resolution_service =
+      ProxyResolutionService::CreateFixedFromPacResult(
+          "HTTPS proxy:70", TRAFFIC_ANNOTATION_FOR_TESTS);
+  TestNetLog net_log;
+  session_deps_.net_log = &net_log;
+
+  HttpRequestInfo request;
+  request.method = "GET";
+  request.url = GURL("https://www.example.org/");
+  request.traffic_annotation =
+      net::MutableNetworkTrafficAnnotationTag(TRAFFIC_ANNOTATION_FOR_TESTS);
+
+  MockWrite data_writes[] = {
+      MockWrite(ASYNC, 0,
+                "CONNECT www.example.org:443 HTTP/1.1\r\n"
+                "Host: www.example.org:443\r\n"
+                "Proxy-Connection: keep-alive\r\n\r\n"),
+  };
+
+  MockRead data_reads[] = {
+      MockRead(ASYNC, 1, "HTTP/1.1 302 Redirect\r\n"),
+      MockRead(ASYNC, 2, "Location: http://login.example.com/\r\n"),
+      MockRead(ASYNC, 3, "Content-Length: 0\r\n\r\n"),
+  };
+
+  SequencedSocketData data(MockConnect(ASYNC, OK), data_reads, data_writes);
+  SSLSocketDataProvider proxy_ssl(ASYNC, OK);  // SSL to the proxy
+
+  session_deps_.socket_factory->AddSocketDataProvider(&data);
+  session_deps_.socket_factory->AddSSLSocketDataProvider(&proxy_ssl);
+
+  TestCompletionCallback callback;
+
+  std::unique_ptr<HttpNetworkSession> session(CreateSession(&session_deps_));
+  HttpNetworkTransaction trans(DEFAULT_PRIORITY, session.get());
+
+  int rv = trans.Start(&request, callback.callback(), NetLogWithSource());
+  EXPECT_THAT(rv, IsError(ERR_IO_PENDING));
+
+  rv = callback.WaitForResult();
+  EXPECT_THAT(rv, IsError(ERR_HTTPS_PROXY_TUNNEL_RESPONSE_REDIRECT));
+
+  histograms.ExpectUniqueSample(
+      "Net.Proxy.RedirectDuringConnect",
+      HttpNetworkTransaction::kSubresourceByExplicitProxy, 1);
+}
+
+// Test that an HTTPS Proxy which was auto-detected cannot redirect a CONNECT
+// request for main frames.
+TEST_F(HttpNetworkTransactionTest,
+       RedirectOfHttpsConnectViaAutoDetectedHttpsProxy) {
+  base::HistogramTester histograms;
+  session_deps_.proxy_resolution_service =
+      ProxyResolutionService::CreateFixedFromAutoDetectedPacResult(
+          "HTTPS proxy:70", TRAFFIC_ANNOTATION_FOR_TESTS);
+  TestNetLog net_log;
+  session_deps_.net_log = &net_log;
+
+  HttpRequestInfo request;
+  request.load_flags = LOAD_MAIN_FRAME_DEPRECATED;
+  request.method = "GET";
+  request.url = GURL("https://www.example.org/");
+  request.traffic_annotation =
+      net::MutableNetworkTrafficAnnotationTag(TRAFFIC_ANNOTATION_FOR_TESTS);
+
+  MockWrite data_writes[] = {
+      MockWrite(ASYNC, 0,
+                "CONNECT www.example.org:443 HTTP/1.1\r\n"
+                "Host: www.example.org:443\r\n"
+                "Proxy-Connection: keep-alive\r\n\r\n"),
+  };
+
+  MockRead data_reads[] = {
+      MockRead(ASYNC, 1, "HTTP/1.1 302 Redirect\r\n"),
+      MockRead(ASYNC, 2, "Location: http://login.example.com/\r\n"),
+      MockRead(ASYNC, 3, "Content-Length: 0\r\n\r\n"),
+  };
+
+  SequencedSocketData data(MockConnect(ASYNC, OK), data_reads, data_writes);
+  SSLSocketDataProvider proxy_ssl(ASYNC, OK);  // SSL to the proxy
+
+  session_deps_.socket_factory->AddSocketDataProvider(&data);
+  session_deps_.socket_factory->AddSSLSocketDataProvider(&proxy_ssl);
+
+  TestCompletionCallback callback;
+
+  std::unique_ptr<HttpNetworkSession> session(CreateSession(&session_deps_));
+  HttpNetworkTransaction trans(DEFAULT_PRIORITY, session.get());
+
+  int rv = trans.Start(&request, callback.callback(), NetLogWithSource());
+  EXPECT_THAT(rv, IsError(ERR_IO_PENDING));
+
+  rv = callback.WaitForResult();
+  EXPECT_THAT(rv, IsError(ERR_HTTPS_PROXY_TUNNEL_RESPONSE_REDIRECT));
+
+  histograms.ExpectUniqueSample(
+      "Net.Proxy.RedirectDuringConnect",
+      HttpNetworkTransaction::kMainFrameByAutoDetectedProxy, 1);
+}
+
+// Test an HTTPS (SPDY) Proxy's ability to redirect a CONNECT request for main
+// frames.
 TEST_F(HttpNetworkTransactionTest, RedirectOfHttpsConnectViaSpdyProxy) {
+  base::HistogramTester histograms;
   session_deps_.proxy_resolution_service = ProxyResolutionService::CreateFixed(
       "https://proxy:70", TRAFFIC_ANNOTATION_FOR_TESTS);
   TestNetLog net_log;
@@ -9613,6 +9720,7 @@ TEST_F(HttpNetworkTransactionTest, RedirectOfHttpsConnectViaSpdyProxy) {
 
   HttpRequestInfo request;
   request.method = "GET";
+  request.load_flags = LOAD_MAIN_FRAME_DEPRECATED;
   request.url = GURL("https://www.example.org/");
   request.traffic_annotation =
       net::MutableNetworkTrafficAnnotationTag(TRAFFIC_ANNOTATION_FOR_TESTS);
@@ -9692,7 +9800,7 @@ TEST_F(HttpNetworkTransactionTest, RedirectOfHttpsConnectViaSpdyProxy) {
   // name, and negotiate an SSL connection to it (Neither of which are done in
   // this case), which the DNS and SSL times for the proxy are all included in
   // connect_start / connect_end. See
-  // HttpNetworkTransaction::OnHttpsProxyTunnelResponse.
+  // HttpNetworkTransaction::OnHttpsProxyTunnelResponseRedirect.
 
   EXPECT_TRUE(load_timing_info.connect_timing.dns_start.is_null());
   EXPECT_TRUE(load_timing_info.connect_timing.dns_end.is_null());
@@ -9706,6 +9814,10 @@ TEST_F(HttpNetworkTransactionTest, RedirectOfHttpsConnectViaSpdyProxy) {
   EXPECT_TRUE(load_timing_info.send_start.is_null());
   EXPECT_TRUE(load_timing_info.send_end.is_null());
   EXPECT_TRUE(load_timing_info.receive_headers_end.is_null());
+
+  histograms.ExpectUniqueSample(
+      "Net.Proxy.RedirectDuringConnect",
+      HttpNetworkTransaction::kMainFrameByExplicitProxy, 1);
 }
 
 // Test that an HTTPS proxy's response to a CONNECT request is filtered.

diff --git a/net/http/http_proxy_client_socket.cc b/net/http/http_proxy_client_socket.cc
@@ -507,7 +507,7 @@ int HttpProxyClientSocket::DoReadHeadersComplete(int result) {
       stream_socket_.reset();
       socket_ = nullptr;
       is_reused_ = false;
-      return ERR_HTTPS_PROXY_TUNNEL_RESPONSE;
+      return ERR_HTTPS_PROXY_TUNNEL_RESPONSE_REDIRECT;
 
     case 407:  // Proxy Authentication Required
       // We need this status code to allow proxy authentication.  Our

diff --git a/net/http/http_proxy_client_socket_pool_unittest.cc b/net/http/http_proxy_client_socket_pool_unittest.cc
@@ -436,7 +436,7 @@ TEST_P(HttpProxyClientSocketPoolTest, TunnelSetupRedirect) {
     EXPECT_FALSE(handle_.socket());
   } else {
     // Expect ProxyClientSocket to return the proxy's response, sanitized.
-    EXPECT_THAT(rv, IsError(ERR_HTTPS_PROXY_TUNNEL_RESPONSE));
+    EXPECT_THAT(rv, IsError(ERR_HTTPS_PROXY_TUNNEL_RESPONSE_REDIRECT));
     EXPECT_TRUE(handle_.is_initialized());
     ASSERT_TRUE(handle_.socket());
 

diff --git a/net/http/http_proxy_connect_job.cc b/net/http/http_proxy_connect_job.cc
@@ -279,7 +279,7 @@ int HttpProxyConnectJob::HandleConnectResult(int result) {
     error_response_info_ = client_socket_->GetAdditionalErrorState();
 
   if (result == OK || result == ERR_PROXY_AUTH_REQUESTED ||
-      result == ERR_HTTPS_PROXY_TUNNEL_RESPONSE) {
+      result == ERR_HTTPS_PROXY_TUNNEL_RESPONSE_REDIRECT) {
     SetSocket(std::move(client_socket_));
   }
   return result;