No longer preconnect to unsafe ports. As UrlRequests were

never allowed to use these ports anyways, this wasn't a
security issue, but just doesn't seem like a good idea.

BUG=93326

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

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

5 files changed, 72 insertions, 37 deletions

diff --git a/net/base/net_util.h b/net/base/net_util.h
index c417688..f72938f 100644
--- a/net/base/net_util.h
+++ b/net/base/net_util.h
@@ -314,11 +314,11 @@ NET_EXPORT bool IsPortAllowedByDefault(int port);
 
 // Checks |port| against a list of ports which are restricted by the FTP
 // protocol.  Returns true if |port| is allowed, false if it is restricted.
-bool IsPortAllowedByFtp(int port);
+NET_EXPORT_PRIVATE bool IsPortAllowedByFtp(int port);
 
 // Check if banned |port| has been overriden by an entry in
 // |explicitly_allowed_ports_|.
-bool IsPortAllowedByOverride(int port);
+NET_EXPORT_PRIVATE bool IsPortAllowedByOverride(int port);
 
 // Set socket to non-blocking mode
 NET_EXPORT int SetNonBlocking(int fd);
diff --git a/net/http/http_stream_factory_impl_job.cc b/net/http/http_stream_factory_impl_job.cc
index 02a93d2..e21556e 100644
--- a/net/http/http_stream_factory_impl_job.cc
+++ b/net/http/http_stream_factory_impl_job.cc
@@ -485,6 +485,10 @@ int HttpStreamFactoryImpl::Job::DoLoop(int result) {
     State state = next_state_;
     next_state_ = STATE_NONE;
     switch (state) {
+      case STATE_START:
+        DCHECK_EQ(OK, rv);
+        rv = DoStart();
+        break;
       case STATE_RESOLVE_PROXY:
         DCHECK_EQ(OK, rv);
         rv = DoResolveProxy();
@@ -534,21 +538,29 @@ int HttpStreamFactoryImpl::Job::DoLoop(int result) {
 
 int HttpStreamFactoryImpl::Job::StartInternal() {
   CHECK_EQ(STATE_NONE, next_state_);
-
-  origin_ = HostPortPair(request_info_.url.HostNoBrackets(),
-                         request_info_.url.EffectiveIntPort());
-  origin_url_ = HttpStreamFactory::ApplyHostMappingRules(
-      request_info_.url, &origin_);
-
+  next_state_ = STATE_START;
   net_log_.BeginEvent(NetLog::TYPE_HTTP_STREAM_JOB,
                       HttpStreamJobParameters::Create(request_info_.url,
                                                       origin_url_));
-  next_state_ = STATE_RESOLVE_PROXY;
   int rv = RunLoop(OK);
   DCHECK_EQ(ERR_IO_PENDING, rv);
   return rv;
 }
 
+int HttpStreamFactoryImpl::Job::DoStart() {
+  // Don't connect to restricted ports.
+  int port = request_info_.url.EffectiveIntPort();
+  if (!IsPortAllowedByDefault(port) && !IsPortAllowedByOverride(port))
+    return ERR_UNSAFE_PORT;
+
+  origin_ = HostPortPair(request_info_.url.HostNoBrackets(), port);
+  origin_url_ = HttpStreamFactory::ApplyHostMappingRules(
+      request_info_.url, &origin_);
+
+  next_state_ = STATE_RESOLVE_PROXY;
+  return OK;
+}
+
 int HttpStreamFactoryImpl::Job::DoResolveProxy() {
   DCHECK(!pac_request_);
 
diff --git a/net/http/http_stream_factory_impl_job.h b/net/http/http_stream_factory_impl_job.h
index 7c95135..f705ed9 100644
--- a/net/http/http_stream_factory_impl_job.h
+++ b/net/http/http_stream_factory_impl_job.h
@@ -80,6 +80,7 @@ class HttpStreamFactoryImpl::Job {
 
  private:
   enum State {
+    STATE_START,
     STATE_RESOLVE_PROXY,
     STATE_RESOLVE_PROXY_COMPLETE,
 
@@ -134,6 +135,7 @@ class HttpStreamFactoryImpl::Job {
   // argument receive the result from the previous state.  If a method returns
   // ERR_IO_PENDING, then the result from OnIOComplete will be passed to the
   // next state method as the result arg.
+  int DoStart();
   int DoResolveProxy();
   int DoResolveProxyComplete(int result);
   int DoWaitForJob();
diff --git a/net/http/http_stream_factory_impl_unittest.cc b/net/http/http_stream_factory_impl_unittest.cc
index 0c5a8e4..e753d28 100644
--- a/net/http/http_stream_factory_impl_unittest.cc
+++ b/net/http/http_stream_factory_impl_unittest.cc
@@ -7,7 +7,6 @@
 #include <string>
 
 #include "base/basictypes.h"
-#include "net/base/capturing_net_log.h"
 #include "net/base/cert_verifier.h"
 #include "net/base/mock_host_resolver.h"
 #include "net/base/net_log.h"
@@ -49,7 +48,7 @@ class MockHttpStreamFactoryImpl : public HttpStreamFactoryImpl {
 
  private:
   // HttpStreamFactoryImpl methods.
-  virtual void OnPreconnectsCompleteInternal() {
+  virtual void OnPreconnectsCompleteInternal() OVERRIDE {
     preconnect_done_ = true;
     if (waiting_for_preconnect_)
       MessageLoop::current()->Quit();
@@ -162,8 +161,9 @@ TestCase kTests[] = {
   { 2, true},
 };
 
-void PreconnectHelper(const TestCase& test,
-                      HttpNetworkSession* session) {
+void PreconnectHelperForURL(int num_streams,
+                            const GURL& url,
+                            HttpNetworkSession* session) {
   HttpNetworkSessionPeer peer(session);
   MockHttpStreamFactoryImpl* mock_factory =
       new MockHttpStreamFactoryImpl(session);
@@ -173,18 +173,21 @@ void PreconnectHelper(const TestCase& test,
 
   HttpRequestInfo request;
   request.method = "GET";
-  request.url = test.ssl ?  GURL("https://www.google.com") :
-      GURL("http://www.google.com");
+  request.url = url;
   request.load_flags = 0;
 
-  ProxyInfo proxy_info;
-  TestOldCompletionCallback callback;
-
   session->http_stream_factory()->PreconnectStreams(
-      test.num_streams, request, ssl_config, ssl_config, BoundNetLog());
+      num_streams, request, ssl_config, ssl_config, BoundNetLog());
   mock_factory->WaitForPreconnects();
 };
 
+void PreconnectHelper(const TestCase& test,
+                      HttpNetworkSession* session) {
+  GURL url = test.ssl ? GURL("https://www.google.com") :
+      GURL("http://www.google.com");
+  PreconnectHelperForURL(test.num_streams, url, session);
+};
+
 template<typename ParentPool>
 class CapturePreconnectsSocketPool : public ParentPool {
  public:
@@ -200,7 +203,7 @@ class CapturePreconnectsSocketPool : public ParentPool {
                             RequestPriority priority,
                             ClientSocketHandle* handle,
                             OldCompletionCallback* callback,
-                            const BoundNetLog& net_log) {
+                            const BoundNetLog& net_log) OVERRIDE {
     ADD_FAILURE();
     return ERR_UNEXPECTED;
   }
@@ -208,36 +211,38 @@ class CapturePreconnectsSocketPool : public ParentPool {
   virtual void RequestSockets(const std::string& group_name,
                               const void* socket_params,
                               int num_sockets,
-                              const BoundNetLog& net_log) {
+                              const BoundNetLog& net_log) OVERRIDE {
     last_num_streams_ = num_sockets;
   }
 
   virtual void CancelRequest(const std::string& group_name,
-                             ClientSocketHandle* handle) {
+                             ClientSocketHandle* handle) OVERRIDE {
     ADD_FAILURE();
   }
   virtual void ReleaseSocket(const std::string& group_name,
                              StreamSocket* socket,
-                             int id) {
+                             int id) OVERRIDE {
     ADD_FAILURE();
   }
-  virtual void CloseIdleSockets() {
+  virtual void CloseIdleSockets() OVERRIDE {
     ADD_FAILURE();
   }
-  virtual int IdleSocketCount() const {
+  virtual int IdleSocketCount() const OVERRIDE {
     ADD_FAILURE();
     return 0;
   }
-  virtual int IdleSocketCountInGroup(const std::string& group_name) const {
+  virtual int IdleSocketCountInGroup(
+      const std::string& group_name) const OVERRIDE {
     ADD_FAILURE();
     return 0;
   }
-  virtual LoadState GetLoadState(const std::string& group_name,
-                                 const ClientSocketHandle* handle) const {
+  virtual LoadState GetLoadState(
+      const std::string& group_name,
+      const ClientSocketHandle* handle) const OVERRIDE {
     ADD_FAILURE();
     return LOAD_STATE_IDLE;
   }
-  virtual base::TimeDelta ConnectionTimeout() const {
+  virtual base::TimeDelta ConnectionTimeout() const OVERRIDE {
     return base::TimeDelta();
   }
 
@@ -389,6 +394,29 @@ TEST(HttpStreamFactoryTest, PreconnectDirectWithExistingSpdySession) {
   }
 }
 
+// Verify that preconnects to unsafe ports are cancelled before they reach
+// the SocketPool.
+TEST(HttpStreamFactoryTest, PreconnectUnsafePort) {
+  ASSERT_FALSE(IsPortAllowedByDefault(7));
+  ASSERT_FALSE(IsPortAllowedByOverride(7));
+
+  SessionDependencies session_deps(ProxyService::CreateDirect());
+  scoped_refptr<HttpNetworkSession> session(CreateSession(&session_deps));
+  HttpNetworkSessionPeer peer(session);
+  CapturePreconnectsTransportSocketPool* transport_conn_pool =
+      new CapturePreconnectsTransportSocketPool(
+          session_deps.host_resolver.get(),
+          session_deps.cert_verifier.get());
+  MockClientSocketPoolManager* mock_pool_manager =
+      new MockClientSocketPoolManager;
+  mock_pool_manager->SetTransportSocketPool(transport_conn_pool);
+  peer.SetClientSocketPoolManager(mock_pool_manager);
+
+  PreconnectHelperForURL(1, GURL("http://www.google.com:7"), session);
+
+  EXPECT_EQ(-1, transport_conn_pool->last_num_streams());
+}
+
 TEST(HttpStreamFactoryTest, JobNotifiesProxy) {
   const char* kProxyString = "PROXY bad:99; PROXY maybe:80; DIRECT";
   SessionDependencies session_deps(
@@ -404,9 +432,6 @@ TEST(HttpStreamFactoryTest, JobNotifiesProxy) {
   socket_data2.set_connect_data(MockConnect(true, OK));
   session_deps.socket_factory.AddSocketDataProvider(&socket_data2);
 
-  CapturingBoundNetLog log(CapturingNetLog::kUnbounded);
-  EXPECT_TRUE(log.bound().IsLoggingAllEvents());
-
   scoped_refptr<HttpNetworkSession> session(CreateSession(&session_deps));
 
   // Now request a stream.  It should succeed using the second proxy in the
@@ -421,7 +446,7 @@ TEST(HttpStreamFactoryTest, JobNotifiesProxy) {
   scoped_ptr<HttpStreamRequest> request(
       session->http_stream_factory()->RequestStream(request_info, ssl_config,
                                                     ssl_config, &waiter,
-                                                    log.bound()));
+                                                    BoundNetLog()));
   waiter.WaitForStream();
 
   // The proxy that failed should now be known to the proxy_service as bad.
diff --git a/net/url_request/url_request_http_job.cc b/net/url_request/url_request_http_job.cc
index 73cf26d..bf3df81 100644
--- a/net/url_request/url_request_http_job.cc
+++ b/net/url_request/url_request_http_job.cc
@@ -165,10 +165,6 @@ URLRequestJob* URLRequestHttpJob::Factory(URLRequest* request,
                                           const std::string& scheme) {
   DCHECK(scheme == "http" || scheme == "https");
 
-  int port = request->url().IntPort();
-  if (!IsPortAllowedByDefault(port) && !IsPortAllowedByOverride(port))
-    return new URLRequestErrorJob(request, ERR_UNSAFE_PORT);
-
   if (!request->context() ||
       !request->context()->http_transaction_factory()) {
     NOTREACHED() << "requires a valid context";