Don't trust server's PASV response that much in FtpNetworkTransaction.

TEST=Covered by net_unittests.
http://crbug.com/20334

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

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

5 files changed, 137 insertions, 49 deletions

diff --git a/net/ftp/ftp_network_transaction.cc b/net/ftp/ftp_network_transaction.cc
index 09d8d90..3c3e56f 100644
--- a/net/ftp/ftp_network_transaction.cc
+++ b/net/ftp/ftp_network_transaction.cc
@@ -9,6 +9,7 @@
 #include "net/base/connection_type_histograms.h"
 #include "net/base/load_log.h"
 #include "net/base/net_errors.h"
+#include "net/base/net_util.h"
 #include "net/ftp/ftp_network_session.h"
 #include "net/ftp/ftp_request_info.h"
 #include "net/socket/client_socket.h"
@@ -122,8 +123,7 @@ const FtpResponseInfo* FtpNetworkTransaction::GetResponseInfo() const {
 }
 
 LoadState FtpNetworkTransaction::GetLoadState() const {
-  if (next_state_ == STATE_CTRL_RESOLVE_HOST_COMPLETE ||
-      next_state_ == STATE_DATA_RESOLVE_HOST_COMPLETE)
+  if (next_state_ == STATE_CTRL_RESOLVE_HOST_COMPLETE)
     return LOAD_STATE_RESOLVING_HOST;
 
   if (next_state_ == STATE_CTRL_CONNECT_COMPLETE ||
@@ -397,13 +397,6 @@ int FtpNetworkTransaction::DoLoop(int result) {
         rv = DoCtrlWriteQUIT();
         break;
 
-      case STATE_DATA_RESOLVE_HOST:
-        DCHECK(rv == OK);
-        rv = DoDataResolveHost();
-        break;
-      case STATE_DATA_RESOLVE_HOST_COMPLETE:
-        rv = DoDataResolveHostComplete(rv);
-        break;
       case STATE_DATA_CONNECT:
         DCHECK(rv == OK);
         rv = DoDataConnect();
@@ -710,7 +703,8 @@ int FtpNetworkTransaction::DoCtrlWritePASV() {
 }
 
 // There are two way we can receive IP address and port.
-// (127,0,0,1,23,21) IP address and port encapsulate in ().
+// TODO(phajdan.jr): Figure out how this should work for IPv6.
+// (127,0,0,1,23,21) IP address and port encapsulated in ().
 // 127,0,0,1,23,21  IP address and port without ().
 int FtpNetworkTransaction::ProcessResponsePASV(
     const FtpCtrlResponse& response) {
@@ -736,9 +730,16 @@ int FtpNetworkTransaction::ProcessResponsePASV(
       }
       if (sscanf_s(ptr, "%d,%d,%d,%d,%d,%d",
                    &i0, &i1, &i2, &i3, &p0, &p1) == 6) {
-        data_connection_ip_ = StringPrintf("%d.%d.%d.%d", i0, i1, i2, i3);
+        // Ignore the IP address supplied in the response. We are always going
+        // to connect back to the same server to prevent FTP PASV port scanning.
+
         data_connection_port_ = (p0 << 8) + p1;
-        next_state_ = STATE_DATA_RESOLVE_HOST;
+
+        if (data_connection_port_ < 1024 ||
+            !IsPortAllowedByFtp(data_connection_port_))
+          return Stop(ERR_UNSAFE_PORT);
+
+        next_state_ = STATE_DATA_CONNECT;
       } else {
         return Stop(ERR_INVALID_RESPONSE);
       }
@@ -954,29 +955,16 @@ int FtpNetworkTransaction::ProcessResponseQUIT(
 
 // Data Connection
 
-int FtpNetworkTransaction::DoDataResolveHost() {
-  if (data_socket_ != NULL && data_socket_->IsConnected())
-    data_socket_->Disconnect();
-
-  next_state_ = STATE_DATA_RESOLVE_HOST_COMPLETE;
-
-  HostResolver::RequestInfo info(data_connection_ip_,
-                                 data_connection_port_);
-  // No known referrer.
-  return resolver_.Resolve(info, &addresses_, &io_callback_, load_log_);
-}
-
-int FtpNetworkTransaction::DoDataResolveHostComplete(int result) {
-  if (result == OK) {
-    next_state_ = STATE_DATA_CONNECT;
-    return result;
-  }
-  return Stop(result);
-}
-
 int FtpNetworkTransaction::DoDataConnect() {
   next_state_ = STATE_DATA_CONNECT_COMPLETE;
-  data_socket_.reset(socket_factory_->CreateTCPClientSocket(addresses_));
+  AddressList data_addresses;
+  // TODO(phajdan.jr): Use exactly same IP address as the control socket.
+  // If the DNS name resolves to several different IPs, and they are different
+  // physical servers, this will break. However, that configuration is very rare
+  // in practice.
+  data_addresses.Copy(addresses_.head());
+  data_addresses.SetPort(data_connection_port_);
+  data_socket_.reset(socket_factory_->CreateTCPClientSocket(data_addresses));
   return data_socket_->Connect(&io_callback_);
 }
 
diff --git a/net/ftp/ftp_network_transaction.h b/net/ftp/ftp_network_transaction.h
index 4dbf6f4..ee9c141 100644
--- a/net/ftp/ftp_network_transaction.h
+++ b/net/ftp/ftp_network_transaction.h
@@ -145,8 +145,6 @@ class FtpNetworkTransaction : public FtpTransaction {
   int DoCtrlWriteQUIT();
   int ProcessResponseQUIT(const FtpCtrlResponse& response);
 
-  int DoDataResolveHost();
-  int DoDataResolveHostComplete(int result);
   int DoDataConnect();
   int DoDataConnectComplete(int result);
   int DoDataRead();
@@ -196,7 +194,6 @@ class FtpNetworkTransaction : public FtpTransaction {
 
   bool retr_failed_;
 
-  std::string data_connection_ip_;
   int data_connection_port_;
 
   ClientSocketFactory* socket_factory_;
@@ -230,8 +227,6 @@ class FtpNetworkTransaction : public FtpTransaction {
     STATE_CTRL_WRITE_MDTM,
     STATE_CTRL_WRITE_QUIT,
     // Data connection states:
-    STATE_DATA_RESOLVE_HOST,
-    STATE_DATA_RESOLVE_HOST_COMPLETE,
     STATE_DATA_CONNECT,
     STATE_DATA_CONNECT_COMPLETE,
     STATE_DATA_READ,
diff --git a/net/ftp/ftp_network_transaction_unittest.cc b/net/ftp/ftp_network_transaction_unittest.cc
index aa143c0..e2b0b87 100644
--- a/net/ftp/ftp_network_transaction_unittest.cc
+++ b/net/ftp/ftp_network_transaction_unittest.cc
@@ -4,9 +4,19 @@
 
 #include "net/ftp/ftp_network_transaction.h"
 
+#include "build/build_config.h"
+
+#if defined(OS_WIN)
+#include <ws2tcpip.h>
+#elif defined(OS_POSIX)
+#include <netdb.h>
+#include <sys/socket.h>
+#endif
+
 #include "base/ref_counted.h"
 #include "net/base/io_buffer.h"
 #include "net/base/mock_host_resolver.h"
+#include "net/base/net_util.h"
 #include "net/base/test_completion_callback.h"
 #include "net/ftp/ftp_network_session.h"
 #include "net/ftp/ftp_request_info.h"
@@ -303,6 +313,32 @@ class FtpMockControlSocketFileDownloadRetrFail
   DISALLOW_COPY_AND_ASSIGN(FtpMockControlSocketFileDownloadRetrFail);
 };
 
+class FtpMockControlSocketEvilPasv : public FtpMockControlSocketFileDownload {
+ public:
+  explicit FtpMockControlSocketEvilPasv(const char* pasv_response,
+                                        State expected_state)
+      : pasv_response_(pasv_response),
+        expected_state_(expected_state) {
+  }
+
+  virtual MockWriteResult OnWrite(const std::string& data) {
+    if (InjectFault())
+      return MockWriteResult(true, data.length());
+    switch (state()) {
+      case PRE_PASV:
+        return Verify("PASV\r\n", data, expected_state_, pasv_response_);
+      default:
+        return FtpMockControlSocketFileDownload::OnWrite(data);
+    }
+  }
+
+ private:
+  const char* pasv_response_;
+  const State expected_state_;
+
+  DISALLOW_COPY_AND_ASSIGN(FtpMockControlSocketEvilPasv);
+};
+
 class FtpNetworkTransactionTest : public PlatformTest {
  public:
   FtpNetworkTransactionTest()
@@ -489,6 +525,65 @@ TEST_F(FtpNetworkTransactionTest, DownloadTransactionInvalidResponse) {
   ExecuteTransaction(&ctrl_socket, "ftp://host/file", ERR_INVALID_RESPONSE);
 }
 
+TEST_F(FtpNetworkTransactionTest, DownloadTransactionEvilPasvUnsafePort1) {
+  FtpMockControlSocketEvilPasv ctrl_socket("227 Portscan (127,0,0,1,0,22)\r\n",
+                                           FtpMockControlSocket::PRE_QUIT);
+  ExecuteTransaction(&ctrl_socket, "ftp://host/file", ERR_UNSAFE_PORT);
+}
+
+TEST_F(FtpNetworkTransactionTest, DownloadTransactionEvilPasvUnsafePort2) {
+  // Still unsafe. 1 * 256 + 2 = 258, which is < 1024.
+  FtpMockControlSocketEvilPasv ctrl_socket("227 Portscan (127,0,0,1,1,2)\r\n",
+                                           FtpMockControlSocket::PRE_QUIT);
+  ExecuteTransaction(&ctrl_socket, "ftp://host/file", ERR_UNSAFE_PORT);
+}
+
+TEST_F(FtpNetworkTransactionTest, DownloadTransactionEvilPasvUnsafePort3) {
+  // Still unsafe. 3 * 256 + 4 = 772, which is < 1024.
+  FtpMockControlSocketEvilPasv ctrl_socket("227 Portscan (127,0,0,1,3,4)\r\n",
+                                           FtpMockControlSocket::PRE_QUIT);
+  ExecuteTransaction(&ctrl_socket, "ftp://host/file", ERR_UNSAFE_PORT);
+}
+
+TEST_F(FtpNetworkTransactionTest, DownloadTransactionEvilPasvUnsafePort4) {
+  // Unsafe. 8 * 256 + 1 = 2049, which is used by nfs.
+  FtpMockControlSocketEvilPasv ctrl_socket("227 Portscan (127,0,0,1,8,1)\r\n",
+                                           FtpMockControlSocket::PRE_QUIT);
+  ExecuteTransaction(&ctrl_socket, "ftp://host/file", ERR_UNSAFE_PORT);
+}
+
+TEST_F(FtpNetworkTransactionTest, DownloadTransactionEvilPasvUnsafeHost) {
+  FtpMockControlSocketEvilPasv ctrl_socket(
+      "227 Portscan (10,1,2,3,4,123,456)\r\n", FtpMockControlSocket::PRE_SIZE);
+  std::string mock_data("mock-data");
+  MockRead data_reads[] = {
+    MockRead(mock_data.c_str()),
+  };
+  StaticMockSocket data_socket1(data_reads, NULL);
+  mock_socket_factory_.AddMockSocket(&ctrl_socket);
+  mock_socket_factory_.AddMockSocket(&data_socket1);
+  FtpRequestInfo request_info = GetRequestInfo("ftp://host/file");
+
+  // Start the transaction.
+  ASSERT_EQ(ERR_IO_PENDING,
+            transaction_.Start(&request_info, &callback_, NULL));
+  EXPECT_EQ(OK, callback_.WaitForResult());
+
+  // The transaction fires the callback when we can start reading data. That
+  // means that the data socket should be open.
+  MockTCPClientSocket* data_socket =
+      mock_socket_factory_.GetMockTCPClientSocket(1);
+  ASSERT_TRUE(data_socket);
+  ASSERT_TRUE(data_socket->IsConnected());
+
+  // Even if the PASV response specified some other address, we connect
+  // to the address we used for control connection.
+  EXPECT_EQ("127.0.0.1", NetAddressToString(data_socket->addresses().head()));
+
+  // Make sure we have only one host entry in the AddressList.
+  EXPECT_FALSE(data_socket->addresses().head()->ai_next);
+}
+
 TEST_F(FtpNetworkTransactionTest, DirectoryTransactionFailUser) {
   FtpMockControlSocketDirectoryListing ctrl_socket;
   TransactionFailHelper(&ctrl_socket,
diff --git a/net/socket/socket_test_util.cc b/net/socket/socket_test_util.cc
index 4b6a3ad..cd4423b 100644
--- a/net/socket/socket_test_util.cc
+++ b/net/socket/socket_test_util.cc
@@ -65,7 +65,8 @@ void MockClientSocket::RunCallback(int result) {
 
 MockTCPClientSocket::MockTCPClientSocket(const net::AddressList& addresses,
                                          net::MockSocket* socket)
-    : data_(socket),
+    : addresses_(addresses),
+      data_(socket),
       read_offset_(0),
       read_data_(true, net::ERR_UNEXPECTED),
       need_read_data_(true) {
@@ -290,18 +291,19 @@ void MockClientSocketFactory::ResetNextMockIndexes() {
   mock_ssl_sockets_.ResetNextIndex();
 }
 
-ClientSocket* MockClientSocketFactory::GetMockTCPClientSocket(int index) const {
+MockTCPClientSocket* MockClientSocketFactory::GetMockTCPClientSocket(
+    int index) const {
   return tcp_client_sockets_[index];
 }
 
-SSLClientSocket* MockClientSocketFactory::GetMockSSLClientSocket(
+MockSSLClientSocket* MockClientSocketFactory::GetMockSSLClientSocket(
     int index) const {
   return ssl_client_sockets_[index];
 }
 
 ClientSocket* MockClientSocketFactory::CreateTCPClientSocket(
     const AddressList& addresses) {
-  ClientSocket* socket =
+  MockTCPClientSocket* socket =
       new MockTCPClientSocket(addresses, mock_sockets_.GetNext());
   tcp_client_sockets_.push_back(socket);
   return socket;
@@ -311,7 +313,7 @@ SSLClientSocket* MockClientSocketFactory::CreateSSLClientSocket(
     ClientSocket* transport_socket,
     const std::string& hostname,
     const SSLConfig& ssl_config) {
-  SSLClientSocket* socket =
+  MockSSLClientSocket* socket =
       new MockSSLClientSocket(transport_socket, hostname, ssl_config,
                               mock_ssl_sockets_.GetNext());
   ssl_client_sockets_.push_back(socket);
diff --git a/net/socket/socket_test_util.h b/net/socket/socket_test_util.h
index 75f3a37..c7c0e12 100644
--- a/net/socket/socket_test_util.h
+++ b/net/socket/socket_test_util.h
@@ -189,6 +189,9 @@ class MockSocketArray {
   std::vector<T*> sockets_;
 };
 
+class MockTCPClientSocket;
+class MockSSLClientSocket;
+
 // ClientSocketFactory which contains arrays of sockets of each type.
 // You should first fill the arrays using AddMock{SSL,}Socket. When the factory
 // is asked to create a socket, it takes next entry from appropriate array.
@@ -200,12 +203,13 @@ class MockClientSocketFactory : public ClientSocketFactory {
   void AddMockSSLSocket(MockSSLSocket* socket);
   void ResetNextMockIndexes();
 
-  // Return |index|-th ClientSocket (starting from 0) that the factory created.
-  ClientSocket* GetMockTCPClientSocket(int index) const;
+  // Return |index|-th MockTCPClientSocket (starting from 0) that the factory
+  // created.
+  MockTCPClientSocket* GetMockTCPClientSocket(int index) const;
 
-  // Return |index|-th SSLClientSocket (starting from 0) that the factory
+  // Return |index|-th MockSSLClientSocket (starting from 0) that the factory
   // created.
-  SSLClientSocket* GetMockSSLClientSocket(int index) const;
+  MockSSLClientSocket* GetMockSSLClientSocket(int index) const;
 
   // ClientSocketFactory
   virtual ClientSocket* CreateTCPClientSocket(const AddressList& addresses);
@@ -219,8 +223,8 @@ class MockClientSocketFactory : public ClientSocketFactory {
   MockSocketArray<MockSSLSocket> mock_ssl_sockets_;
 
   // Store pointers to handed out sockets in case the test wants to get them.
-  std::vector<ClientSocket*> tcp_client_sockets_;
-  std::vector<SSLClientSocket*> ssl_client_sockets_;
+  std::vector<MockTCPClientSocket*> tcp_client_sockets_;
+  std::vector<MockSSLClientSocket*> ssl_client_sockets_;
 };
 
 class MockClientSocket : public net::SSLClientSocket {
@@ -271,7 +275,11 @@ class MockTCPClientSocket : public MockClientSocket {
   virtual int Write(net::IOBuffer* buf, int buf_len,
                     net::CompletionCallback* callback);
 
+  net::AddressList addresses() const { return addresses_; }
+
  private:
+  net::AddressList addresses_;
+
   net::MockSocket* data_;
   int read_offset_;
   net::MockRead read_data_;