Don't let an active network attacker play tricks with CONNECT tunnels throgh proxy servers.

R=darin,wtc,eroman
BUG=7338

Review URL: http://codereview.chromium.org/27198

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@10595 0039d316-1c4b-4281-b951-d872f2087c98

4 files changed, 273 insertions, 45 deletions

diff --git a/net/base/net_error_list.h b/net/base/net_error_list.h
index 14555c3..9503f534 100644
--- a/net/base/net_error_list.h
+++ b/net/base/net_error_list.h
@@ -96,6 +96,10 @@ NET_ERROR(SSL_VERSION_OR_CIPHER_MISMATCH, -113)
 // The server requested a renegotiation (rehandshake).
 NET_ERROR(SSL_RENEGOTIATION_REQUESTED, -114)
 
+// The proxy claimed to want authenication but didn't provide the proper
+// challenge headers.
+NET_ERROR(PROXY_AUTH_REQUESTED, -115)
+
 // Certificate error codes
 //
 // The values of certificate error codes must be consecutive.
@@ -233,10 +237,6 @@ NET_ERROR(PAC_STATUS_NOT_OK, -326)
 // The evaluation of the PAC script failed.
 NET_ERROR(PAC_SCRIPT_FAILED, -327)
 
-// The response was 401 (Unauthorized), yet the request was a CONNECT request
-// to a proxy.
-NET_ERROR(UNEXPECTED_SERVER_AUTH, -328)
-
 // The cache does not have the requested entry.
 NET_ERROR(CACHE_MISS, -400)
 
diff --git a/net/http/http_network_transaction.cc b/net/http/http_network_transaction.cc
index 0e4f5b7..7969e06 100644
--- a/net/http/http_network_transaction.cc
+++ b/net/http/http_network_transaction.cc
@@ -783,6 +783,8 @@ int HttpNetworkTransaction::DoReadBody() {
 
 int HttpNetworkTransaction::DoReadBodyComplete(int result) {
   // We are done with the Read call.
+  DCHECK(!establishing_tunnel_) <<
+      "We should never read a response body of a tunnel.";
 
   bool unfiltered_eof = (result == 0);
 
@@ -809,10 +811,9 @@ int HttpNetworkTransaction::DoReadBodyComplete(int result) {
       done = true;
       keep_alive = response_.headers->IsKeepAlive();
       // We can't reuse the connection if we read more than the advertised
-      // content length, or if the tunnel was not established successfully.
+      // content length.
       if (unfiltered_eof ||
-          (content_length_ != -1 && content_read_ > content_length_) ||
-          establishing_tunnel_)
+          (content_length_ != -1 && content_read_ > content_length_))
         keep_alive = false;
     }
   }
@@ -925,24 +926,44 @@ int HttpNetworkTransaction::DidReadResponseHeaders() {
   }
 
   if (establishing_tunnel_) {
-    if (headers->response_code() == 200) {
-      if (header_buf_body_offset_ != header_buf_len_) {
-        // The proxy sent extraneous data after the headers.
+    switch (headers->response_code()) {
+      case 200:  // OK
+        if (header_buf_body_offset_ != header_buf_len_) {
+          // The proxy sent extraneous data after the headers.
+          return ERR_TUNNEL_CONNECTION_FAILED;
+        }
+        next_state_ = STATE_SSL_CONNECT_OVER_TUNNEL;
+        // Reset for the real request and response headers.
+        request_headers_.clear();
+        request_headers_bytes_sent_ = 0;
+        header_buf_len_ = 0;
+        header_buf_body_offset_ = 0;
+        establishing_tunnel_ = false;
+        return OK;
+
+      // We aren't able to CONNECT to the remote host through the proxy.  We
+      // need to be very suspicious about the response because an active network
+      // attacker can force us into this state by masquerading as the proxy.
+      // The only safe thing to do here is to fail the connection because our
+      // client is expecting an SSL protected response.
+      // See http://crbug.com/7338.
+      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.
+        break;
+      default:
+        // For all other status codes, we conservatively fail the CONNECT
+        // request.
+        // We lose something by doing this.  We have seen proxy 403, 404, and
+        // 501 response bodies that contain a useful error message.  For
+        // example, Squid uses a 404 response to report the DNS error: "The
+        // domain name does not exist."
+        LOG(WARNING) <<
+            "Blocked proxy response to CONNECT request with status " <<
+            headers->response_code() << ".";
         return ERR_TUNNEL_CONNECTION_FAILED;
-      }
-      next_state_ = STATE_SSL_CONNECT_OVER_TUNNEL;
-      // Reset for the real request and response headers.
-      request_headers_.clear();
-      request_headers_bytes_sent_ = 0;
-      header_buf_len_ = 0;
-      header_buf_body_offset_ = 0;
-      establishing_tunnel_ = false;
-      return OK;
     }
-    // Sanitize any illegal response code for CONNECT to prevent us from
-    // handling it by mistake.  See http://crbug.com/7338.
-    if (headers->response_code() < 400 || headers->response_code() > 599)
-      headers->set_response_code(500);  // Masquerade as a 500.
   }
 
   // Check for an intermediate 100 Continue response.  An origin server is
@@ -1317,9 +1338,6 @@ int HttpNetworkTransaction::HandleAuthChallenge() {
   if (target == HttpAuth::AUTH_PROXY && proxy_info_.is_direct())
     return ERR_UNEXPECTED_PROXY_AUTH;
 
-  if (target == HttpAuth::AUTH_SERVER && establishing_tunnel_)
-    return ERR_UNEXPECTED_SERVER_AUTH;
-
   // The auth we tried just failed, hence it can't be valid. Remove it from
   // the cache so it won't be used again.
   if (HaveAuth(target))
@@ -1332,10 +1350,17 @@ int HttpNetworkTransaction::HandleAuthChallenge() {
                                 target,
                                 &auth_handler_[target]);
 
-  // We found no supported challenge -- let the transaction continue
-  // so we end up displaying the error page.
-  if (!auth_handler_[target])
+  if (!auth_handler_[target]) {
+    if (establishing_tunnel_) {
+      // We are establishing a tunnel, we can't show the error page because an
+      // active network attacker could control its contents.  Instead, we just
+      // fail to establish the tunnel.
+      return ERR_PROXY_AUTH_REQUESTED;
+    }
+    // We found no supported challenge -- let the transaction continue
+    // so we end up displaying the error page.
     return OK;
+  }
 
   // Pick a new auth identity to try, by looking to the URL and auth cache.
   // If an identity to try is found, it is saved to auth_identity_[target].
diff --git a/net/http/http_network_transaction_unittest.cc b/net/http/http_network_transaction_unittest.cc
index 5ceeb39..87f2636 100644
--- a/net/http/http_network_transaction_unittest.cc
+++ b/net/http/http_network_transaction_unittest.cc
@@ -1060,6 +1060,220 @@ TEST_F(HttpNetworkTransactionTest, BasicAuthProxyKeepAlive) {
   EXPECT_EQ(L"basic", response->auth_challenge->scheme);
 }
 
+static void ConnectStatusHelperWithExpectedStatus(
+    const MockRead& status, int expected_status) {
+  // Configure against proxy server "myproxy:70".
+  scoped_ptr<net::ProxyService> proxy_service(
+      CreateFixedProxyService("myproxy:70"));
+
+  scoped_refptr<net::HttpNetworkSession> session(
+      CreateSession(proxy_service.get()));
+
+  scoped_ptr<net::HttpTransaction> trans(new net::HttpNetworkTransaction(
+      session.get(), &mock_socket_factory));
+
+  net::HttpRequestInfo request;
+  request.method = "GET";
+  request.url = GURL("https://www.google.com/");
+  request.load_flags = 0;
+
+  // 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\r\n"),
+  };
+
+  MockRead data_reads[] = {
+    status,
+    MockRead("Content-Length: 10\r\n\r\n"),
+    // No response body because the test stops reading here.
+    MockRead(false, net::ERR_UNEXPECTED),  // Should not be reached.
+  };
+
+  MockSocket data;
+  data.writes = data_writes;
+  data.reads = data_reads;
+  mock_sockets[0] = &data;
+  mock_sockets[1] = NULL;
+
+  TestCompletionCallback callback;
+
+  int rv = trans->Start(&request, &callback);
+  EXPECT_EQ(net::ERR_IO_PENDING, rv);
+
+  rv = callback.WaitForResult();
+  EXPECT_EQ(expected_status, rv);
+}
+
+static void ConnectStatusHelper(const MockRead& status) {
+  ConnectStatusHelperWithExpectedStatus(
+      status, net::ERR_TUNNEL_CONNECTION_FAILED);
+}
+
+TEST_F(HttpNetworkTransactionTest, ConnectStatus100) {
+  ConnectStatusHelper(MockRead("HTTP/1.1 100 Continue\r\n"));
+}
+
+TEST_F(HttpNetworkTransactionTest, ConnectStatus101) {
+  ConnectStatusHelper(MockRead("HTTP/1.1 101 Switching Protocols\r\n"));
+}
+
+TEST_F(HttpNetworkTransactionTest, ConnectStatus201) {
+  ConnectStatusHelper(MockRead("HTTP/1.1 201 Created\r\n"));
+}
+
+TEST_F(HttpNetworkTransactionTest, ConnectStatus202) {
+  ConnectStatusHelper(MockRead("HTTP/1.1 202 Accepted\r\n"));
+}
+
+TEST_F(HttpNetworkTransactionTest, ConnectStatus203) {
+  ConnectStatusHelper(
+      MockRead("HTTP/1.1 203 Non-Authoritative Information\r\n"));
+}
+
+TEST_F(HttpNetworkTransactionTest, ConnectStatus204) {
+  ConnectStatusHelper(MockRead("HTTP/1.1 204 No Content\r\n"));
+}
+
+TEST_F(HttpNetworkTransactionTest, ConnectStatus205) {
+  ConnectStatusHelper(MockRead("HTTP/1.1 205 Reset Content\r\n"));
+}
+
+TEST_F(HttpNetworkTransactionTest, ConnectStatus206) {
+  ConnectStatusHelper(MockRead("HTTP/1.1 206 Partial Content\r\n"));
+}
+
+TEST_F(HttpNetworkTransactionTest, ConnectStatus300) {
+  ConnectStatusHelper(MockRead("HTTP/1.1 300 Multiple Choices\r\n"));
+}
+
+TEST_F(HttpNetworkTransactionTest, ConnectStatus301) {
+  ConnectStatusHelper(MockRead("HTTP/1.1 301 Moved Permanently\r\n"));
+}
+
+TEST_F(HttpNetworkTransactionTest, ConnectStatus302) {
+  ConnectStatusHelper(MockRead("HTTP/1.1 302 Found\r\n"));
+}
+
+TEST_F(HttpNetworkTransactionTest, ConnectStatus303) {
+  ConnectStatusHelper(MockRead("HTTP/1.1 303 See Other\r\n"));
+}
+
+TEST_F(HttpNetworkTransactionTest, ConnectStatus304) {
+  ConnectStatusHelper(MockRead("HTTP/1.1 304 Not Modified\r\n"));
+}
+
+TEST_F(HttpNetworkTransactionTest, ConnectStatus305) {
+  ConnectStatusHelper(MockRead("HTTP/1.1 305 Use Proxy\r\n"));
+}
+
+TEST_F(HttpNetworkTransactionTest, ConnectStatus306) {
+  ConnectStatusHelper(MockRead("HTTP/1.1 306\r\n"));
+}
+
+TEST_F(HttpNetworkTransactionTest, ConnectStatus307) {
+  ConnectStatusHelper(MockRead("HTTP/1.1 307 Temporary Redirect\r\n"));
+}
+
+TEST_F(HttpNetworkTransactionTest, ConnectStatus400) {
+  ConnectStatusHelper(MockRead("HTTP/1.1 400 Bad Request\r\n"));
+}
+
+TEST_F(HttpNetworkTransactionTest, ConnectStatus401) {
+  ConnectStatusHelper(MockRead("HTTP/1.1 401 Unauthorized\r\n"));
+}
+
+TEST_F(HttpNetworkTransactionTest, ConnectStatus402) {
+  ConnectStatusHelper(MockRead("HTTP/1.1 402 Payment Required\r\n"));
+}
+
+TEST_F(HttpNetworkTransactionTest, ConnectStatus403) {
+  ConnectStatusHelper(MockRead("HTTP/1.1 403 Forbidden\r\n"));
+}
+
+TEST_F(HttpNetworkTransactionTest, ConnectStatus404) {
+  ConnectStatusHelper(MockRead("HTTP/1.1 404 Not Found\r\n"));
+}
+
+TEST_F(HttpNetworkTransactionTest, ConnectStatus405) {
+  ConnectStatusHelper(MockRead("HTTP/1.1 405 Method Not Allowed\r\n"));
+}
+
+TEST_F(HttpNetworkTransactionTest, ConnectStatus406) {
+  ConnectStatusHelper(MockRead("HTTP/1.1 406 Not Acceptable\r\n"));
+}
+
+TEST_F(HttpNetworkTransactionTest, ConnectStatus407) {
+  ConnectStatusHelperWithExpectedStatus(
+      MockRead("HTTP/1.1 407 Proxy Authentication Required\r\n"),
+      net::ERR_PROXY_AUTH_REQUESTED);
+}
+
+TEST_F(HttpNetworkTransactionTest, ConnectStatus408) {
+  ConnectStatusHelper(MockRead("HTTP/1.1 408 Request Timeout\r\n"));
+}
+
+TEST_F(HttpNetworkTransactionTest, ConnectStatus409) {
+  ConnectStatusHelper(MockRead("HTTP/1.1 409 Conflict\r\n"));
+}
+
+TEST_F(HttpNetworkTransactionTest, ConnectStatus410) {
+  ConnectStatusHelper(MockRead("HTTP/1.1 410 Gone\r\n"));
+}
+
+TEST_F(HttpNetworkTransactionTest, ConnectStatus411) {
+  ConnectStatusHelper(MockRead("HTTP/1.1 411 Length Required\r\n"));
+}
+
+TEST_F(HttpNetworkTransactionTest, ConnectStatus412) {
+  ConnectStatusHelper(MockRead("HTTP/1.1 412 Precondition Failed\r\n"));
+}
+
+TEST_F(HttpNetworkTransactionTest, ConnectStatus413) {
+  ConnectStatusHelper(MockRead("HTTP/1.1 413 Request Entity Too Large\r\n"));
+}
+
+TEST_F(HttpNetworkTransactionTest, ConnectStatus414) {
+  ConnectStatusHelper(MockRead("HTTP/1.1 414 Request-URI Too Long\r\n"));
+}
+
+TEST_F(HttpNetworkTransactionTest, ConnectStatus415) {
+  ConnectStatusHelper(MockRead("HTTP/1.1 415 Unsupported Media Type\r\n"));
+}
+
+TEST_F(HttpNetworkTransactionTest, ConnectStatus416) {
+  ConnectStatusHelper(
+      MockRead("HTTP/1.1 416 Requested Range Not Satisfiable\r\n"));
+}
+
+TEST_F(HttpNetworkTransactionTest, ConnectStatus417) {
+  ConnectStatusHelper(MockRead("HTTP/1.1 417 Expectation Failed\r\n"));
+}
+
+TEST_F(HttpNetworkTransactionTest, ConnectStatus500) {
+  ConnectStatusHelper(MockRead("HTTP/1.1 500 Internal Server Error\r\n"));
+}
+
+TEST_F(HttpNetworkTransactionTest, ConnectStatus501) {
+  ConnectStatusHelper(MockRead("HTTP/1.1 501 Not Implemented\r\n"));
+}
+
+TEST_F(HttpNetworkTransactionTest, ConnectStatus502) {
+  ConnectStatusHelper(MockRead("HTTP/1.1 502 Bad Gateway\r\n"));
+}
+
+TEST_F(HttpNetworkTransactionTest, ConnectStatus503) {
+  ConnectStatusHelper(MockRead("HTTP/1.1 503 Service Unavailable\r\n"));
+}
+
+TEST_F(HttpNetworkTransactionTest, ConnectStatus504) {
+  ConnectStatusHelper(MockRead("HTTP/1.1 504 Gateway Timeout\r\n"));
+}
+
+TEST_F(HttpNetworkTransactionTest, ConnectStatus505) {
+  ConnectStatusHelper(MockRead("HTTP/1.1 505 HTTP Version Not Supported\r\n"));
+}
+
 // Test the flow when both the proxy server AND origin server require
 // authentication. Again, this uses basic auth for both since that is
 // the simplest to mock.
@@ -1269,7 +1483,6 @@ TEST_F(HttpNetworkTransactionTest, DontRecycleTCPSocketForSSLTunnel) {
   MockRead data_reads1[] = {
     MockRead("HTTP/1.1 404 Not Found\r\n"),
     MockRead("Content-Length: 10\r\n\r\n"),
-    MockRead("0123456789"),
     MockRead(false, net::ERR_UNEXPECTED),  // Should not be reached.
   };
 
@@ -1285,19 +1498,10 @@ TEST_F(HttpNetworkTransactionTest, DontRecycleTCPSocketForSSLTunnel) {
   EXPECT_EQ(net::ERR_IO_PENDING, rv);
 
   rv = callback1.WaitForResult();
-  EXPECT_EQ(net::OK, rv);
+  EXPECT_EQ(net::ERR_TUNNEL_CONNECTION_FAILED, rv);
 
   const net::HttpResponseInfo* response = trans->GetResponseInfo();
-  EXPECT_FALSE(response == NULL);
-
-  EXPECT_TRUE(response->headers->IsKeepAlive());
-  EXPECT_EQ(404, response->headers->response_code());
-  EXPECT_EQ(10, response->headers->GetContentLength());
-  EXPECT_TRUE(net::HttpVersion(1, 1) == response->headers->GetHttpVersion());
-
-  std::string response_data;
-  rv = ReadTransaction(trans.get(), &response_data);
-  EXPECT_STREQ("0123456789", response_data.c_str());
+  EXPECT_TRUE(response == NULL);
 
   // We now check to make sure the TCPClientSocket was not added back to
   // the pool.
diff --git a/net/url_request/url_request_unittest.cc b/net/url_request/url_request_unittest.cc
index f2028f13..c8e2896 100644
--- a/net/url_request/url_request_unittest.cc
+++ b/net/url_request/url_request_unittest.cc
@@ -103,9 +103,8 @@ TEST_F(URLRequestTest, ProxyTunnelRedirectTest) {
 
     MessageLoop::current()->Run();
 
-    EXPECT_EQ(URLRequestStatus::SUCCESS, r.status().status());
-    // We should have rewritten the 302 response code as 500.
-    EXPECT_EQ(500, r.GetResponseCode());
+    EXPECT_EQ(URLRequestStatus::FAILED, r.status().status());
+    EXPECT_EQ(net::ERR_TUNNEL_CONNECTION_FAILED, r.status().os_error());
     EXPECT_EQ(1, d.response_started_count());
     // We should not have followed the redirect.
     EXPECT_EQ(0, d.received_redirect_count());
@@ -132,7 +131,7 @@ TEST_F(URLRequestTest, UnexpectedServerAuthTest) {
     MessageLoop::current()->Run();
 
     EXPECT_EQ(URLRequestStatus::FAILED, r.status().status());
-    EXPECT_EQ(net::ERR_UNEXPECTED_SERVER_AUTH, r.status().os_error());
+    EXPECT_EQ(net::ERR_TUNNEL_CONNECTION_FAILED, r.status().os_error());
   }
 }