Unify code to deal with restricted ports.

Replace IsPortAllowedByDefault(), IsPortAllowedByFtp(), and
IsPortAllowedByOverride() with IsPortAllowedForScheme().  Previously,
consumers had to wrangle their way through the methods.  One method
makes things simpler, and consumers now have to explicitly opt into or
out of the override option.

BUG=475766

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

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

6 files changed, 61 insertions, 46 deletions

diff --git a/net/base/net_util.cc b/net/base/net_util.cc
index 031f836..028b56b 100644
--- a/net/base/net_util.cc
+++ b/net/base/net_util.cc
@@ -55,6 +55,7 @@
 #include "url/third_party/mozilla/url_parse.h"
 #include "url/url_canon.h"
 #include "url/url_canon_ip.h"
+#include "url/url_constants.h"
 
 #if defined(OS_ANDROID)
 #include "net/android/network_library.h"
@@ -271,32 +272,39 @@ bool IsPortValid(int port) {
   return port >= 0 && port <= std::numeric_limits<uint16_t>::max();
 }
 
-bool IsPortAllowedByDefault(int port) {
-  int array_size = arraysize(kRestrictedPorts);
-  for (int i = 0; i < array_size; i++) {
-    if (kRestrictedPorts[i] == port) {
-      return false;
-    }
-  }
-  return IsPortValid(port);
+bool IsWellKnownPort(int port) {
+  return port >= 0 && port < 1024;
 }
 
-bool IsPortAllowedByFtp(int port) {
-  int array_size = arraysize(kAllowedFtpPorts);
-  for (int i = 0; i < array_size; i++) {
-    if (kAllowedFtpPorts[i] == port) {
+NET_EXPORT bool IsPortAllowedForScheme(int port,
+                                       const std::string& url_scheme,
+                                       PortOverrideMode port_override_mode) {
+  // Reject invalid ports.
+  if (!IsPortValid(port))
+    return false;
+
+  // Allow explitly allowed ports for any scheme.
+  if (port_override_mode == PORT_OVERRIDES_ALLOWED &&
+      g_explicitly_allowed_ports.Get().count(port) > 0) {
+    return true;
+  }
+
+  // FTP requests have an extra set of whitelisted schemes.
+  if (LowerCaseEqualsASCII(url_scheme, url::kFtpScheme)) {
+    for (int allowed_ftp_port : kAllowedFtpPorts) {
+      if (allowed_ftp_port == port)
         return true;
     }
   }
-  // Port not explicitly allowed by FTP, so return the default restrictions.
-  return IsPortAllowedByDefault(port);
-}
 
-bool IsPortAllowedByOverride(int port) {
-  if (g_explicitly_allowed_ports.Get().empty())
-    return false;
+  // Finally check against the generic list of restricted ports for all
+  // schemes.
+  for (int restricted_port : kRestrictedPorts) {
+    if (restricted_port == port)
+      return false;
+  }
 
-  return g_explicitly_allowed_ports.Get().count(port) > 0;
+  return true;
 }
 
 int SetNonBlocking(int fd) {
diff --git a/net/base/net_util.h b/net/base/net_util.h
index 1a32fbe..f2ba404 100644
--- a/net/base/net_util.h
+++ b/net/base/net_util.h
@@ -222,17 +222,19 @@ NET_EXPORT base::string16 StripWWWFromHost(const GURL& url);
 // reserved).  Should be used before casting a port to a uint16_t.
 NET_EXPORT bool IsPortValid(int port);
 
-// Checks |port| against a list of ports which are restricted by default.
-// Returns true if |port| is allowed, false if it is restricted.
-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.
-NET_EXPORT_PRIVATE bool IsPortAllowedByFtp(int port);
-
-// Check if banned |port| has been overriden by an entry in
-// |explicitly_allowed_ports_|.
-NET_EXPORT_PRIVATE bool IsPortAllowedByOverride(int port);
+// Returns true if the port is in the range [0, 1023]. These ports are
+// registered by IANA and typically need root access to listen on.
+bool IsWellKnownPort(int port);
+
+enum PortOverrideMode { PORT_OVERRIDES_IGNORED, PORT_OVERRIDES_ALLOWED };
+
+// Checks if the port is allowed for the specified scheme.  If PortOverrideMode
+// is PORT_OVERIDES_ALLOWED, then ports set as allowed with
+// SetExplicitlyAllowedPorts() or by using ScopedPortException() will be
+// considered allowed for any scheme.
+NET_EXPORT bool IsPortAllowedForScheme(int port,
+                                       const std::string& url_scheme,
+                                       PortOverrideMode port_override_mode);
 
 // Set socket to non-blocking mode
 NET_EXPORT int SetNonBlocking(int fd);
diff --git a/net/ftp/ftp_network_transaction.cc b/net/ftp/ftp_network_transaction.cc
index 3c67114..32ede86 100644
--- a/net/ftp/ftp_network_transaction.cc
+++ b/net/ftp/ftp_network_transaction.cc
@@ -24,6 +24,7 @@
 #include "net/log/net_log.h"
 #include "net/socket/client_socket_factory.h"
 #include "net/socket/stream_socket.h"
+#include "url/url_constants.h"
 
 namespace net {
 
@@ -956,8 +957,11 @@ int FtpNetworkTransaction::ProcessResponseEPSV(
       int port;
       if (!ExtractPortFromEPSVResponse(response, &port))
         return Stop(ERR_INVALID_RESPONSE);
-      if (port < 1024 || !IsPortAllowedByFtp(port))
+      if (IsWellKnownPort(port) ||
+          !IsPortAllowedForScheme(port, url::kFtpScheme,
+                                  PORT_OVERRIDES_IGNORED)) {
         return Stop(ERR_UNSAFE_PORT);
+      }
       data_connection_port_ = static_cast<uint16>(port);
       next_state_ = STATE_DATA_CONNECT;
       break;
@@ -992,8 +996,11 @@ int FtpNetworkTransaction::ProcessResponsePASV(
       int port;
       if (!ExtractPortFromPASVResponse(response, &port))
         return Stop(ERR_INVALID_RESPONSE);
-      if (port < 1024 || !IsPortAllowedByFtp(port))
+      if (IsWellKnownPort(port) ||
+          !IsPortAllowedForScheme(port, url::kFtpScheme,
+                                  PORT_OVERRIDES_IGNORED)) {
         return Stop(ERR_UNSAFE_PORT);
+      }
       data_connection_port_ = static_cast<uint16>(port);
       next_state_ = STATE_DATA_CONNECT;
       break;
diff --git a/net/http/http_stream_factory_impl_job.cc b/net/http/http_stream_factory_impl_job.cc
index e3c9107..2ec9401 100644
--- a/net/http/http_stream_factory_impl_job.cc
+++ b/net/http/http_stream_factory_impl_job.cc
@@ -199,6 +199,10 @@ void HttpStreamFactoryImpl::Job::WaitFor(Job* job) {
   DCHECK_EQ(STATE_NONE, job->next_state_);
   DCHECK(!blocking_job_);
   DCHECK(!job->waiting_job_);
+
+  // Never share connection with other jobs for FTP requests.
+  DCHECK(!request_info_.url.SchemeIs("ftp"));
+
   blocking_job_ = job;
   job->waiting_job_ = this;
 }
@@ -668,14 +672,8 @@ int HttpStreamFactoryImpl::Job::DoStart() {
                  &alternative_service_, priority_));
 
   // Don't connect to restricted ports.
-  bool is_port_allowed = IsPortAllowedByDefault(server_.port());
-  if (request_info_.url.SchemeIs("ftp")) {
-    // Never share connection with other jobs for FTP requests.
-    DCHECK(!waiting_job_);
-
-    is_port_allowed = IsPortAllowedByFtp(server_.port());
-  }
-  if (!is_port_allowed && !IsPortAllowedByOverride(server_.port())) {
+  if (!IsPortAllowedForScheme(server_.port(), request_info_.url.scheme(),
+                              PORT_OVERRIDES_ALLOWED)) {
     if (waiting_job_) {
       waiting_job_->Resume(this);
       waiting_job_ = NULL;
diff --git a/net/http/http_stream_factory_impl_unittest.cc b/net/http/http_stream_factory_impl_unittest.cc
index 1f301e3..df8db28 100644
--- a/net/http/http_stream_factory_impl_unittest.cc
+++ b/net/http/http_stream_factory_impl_unittest.cc
@@ -552,8 +552,7 @@ TEST_P(HttpStreamFactoryTest, PreconnectDirectWithExistingSpdySession) {
 // Verify that preconnects to unsafe ports are cancelled before they reach
 // the SocketPool.
 TEST_P(HttpStreamFactoryTest, PreconnectUnsafePort) {
-  ASSERT_FALSE(IsPortAllowedByDefault(7));
-  ASSERT_FALSE(IsPortAllowedByOverride(7));
+  ASSERT_FALSE(IsPortAllowedForScheme(7, "http", PORT_OVERRIDES_ALLOWED));
 
   SpdySessionDependencies session_deps(
       GetParam(), ProxyService::CreateDirect());
@@ -570,7 +569,6 @@ TEST_P(HttpStreamFactoryTest, PreconnectUnsafePort) {
   peer.SetClientSocketPoolManager(mock_pool_manager.Pass());
 
   PreconnectHelperForURL(1, GURL("http://www.google.com:7"), session.get());
-
   EXPECT_EQ(-1, transport_conn_pool->last_num_streams());
 }
 
diff --git a/net/url_request/ftp_protocol_handler.cc b/net/url_request/ftp_protocol_handler.cc
index 56d15e6..418b6f6 100644
--- a/net/url_request/ftp_protocol_handler.cc
+++ b/net/url_request/ftp_protocol_handler.cc
@@ -27,9 +27,11 @@ FtpProtocolHandler::~FtpProtocolHandler() {
 
 URLRequestJob* FtpProtocolHandler::MaybeCreateJob(
     URLRequest* request, NetworkDelegate* network_delegate) const {
-  int port = request->url().IntPort();
-  if (request->url().has_port() &&
-      !IsPortAllowedByFtp(port) && !IsPortAllowedByOverride(port)) {
+  DCHECK_EQ("ftp", request->url().scheme());
+
+  if (!IsPortAllowedForScheme(request->url().EffectiveIntPort(),
+                              request->url().scheme(),
+                              PORT_OVERRIDES_ALLOWED)) {
     return new URLRequestErrorJob(request, network_delegate, ERR_UNSAFE_PORT);
   }