Really connect to the same server in FTP network transaction.

Also create necessary infrastructure to know the address
a client socket is connected to.

TEST=Covered by net_unittests.
BUG=35670

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

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

2 files changed, 24 insertions, 22 deletions

diff --git a/net/ftp/ftp_network_transaction.cc b/net/ftp/ftp_network_transaction.cc
index 2bfbda0..5448c34 100644
--- a/net/ftp/ftp_network_transaction.cc
+++ b/net/ftp/ftp_network_transaction.cc
@@ -1104,14 +1104,14 @@ int FtpNetworkTransaction::ProcessResponseQUIT(
 
 int FtpNetworkTransaction::DoDataConnect() {
   next_state_ = STATE_DATA_CONNECT_COMPLETE;
-  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));
+  AddressList data_address;
+  // Connect to the same host as the control socket to prevent PASV port
+  // scanning attacks.
+  int rv = ctrl_socket_->GetPeerAddress(&data_address);
+  if (rv != OK)
+    return Stop(rv);
+  data_address.SetPort(data_connection_port_);
+  data_socket_.reset(socket_factory_->CreateTCPClientSocket(data_address));
   return data_socket_->Connect(&io_callback_, load_log_);
 }
 
diff --git a/net/ftp/ftp_network_transaction_unittest.cc b/net/ftp/ftp_network_transaction_unittest.cc
index d9be57c..adf074e 100644
--- a/net/ftp/ftp_network_transaction_unittest.cc
+++ b/net/ftp/ftp_network_transaction_unittest.cc
@@ -610,14 +610,14 @@ class FtpNetworkTransactionTest : public PlatformTest {
     ASSERT_EQ(ERR_IO_PENDING,
               transaction_.Start(&request_info, &callback_, NULL));
     EXPECT_NE(LOAD_STATE_IDLE, transaction_.GetLoadState());
-    EXPECT_EQ(expected_result, callback_.WaitForResult());
+    ASSERT_EQ(expected_result, callback_.WaitForResult());
     EXPECT_EQ(FtpSocketDataProvider::QUIT, ctrl_socket->state());
     if (expected_result == OK) {
       scoped_refptr<IOBuffer> io_buffer(new IOBuffer(kBufferSize));
       memset(io_buffer->data(), 0, kBufferSize);
       ASSERT_EQ(ERR_IO_PENDING,
                 transaction_.Read(io_buffer.get(), kBufferSize, &callback_));
-      EXPECT_EQ(static_cast<int>(mock_data.length()),
+      ASSERT_EQ(static_cast<int>(mock_data.length()),
                 callback_.WaitForResult());
       EXPECT_EQ(mock_data, std::string(io_buffer->data(), mock_data.length()));
       if (transaction_.GetResponseInfo()->is_directory_listing) {
@@ -653,7 +653,7 @@ TEST_F(FtpNetworkTransactionTest, FailedLookup) {
   EXPECT_EQ(LOAD_STATE_IDLE, transaction_.GetLoadState());
   ASSERT_EQ(ERR_IO_PENDING,
             transaction_.Start(&request_info, &callback_, NULL));
-  EXPECT_EQ(ERR_NAME_NOT_RESOLVED, callback_.WaitForResult());
+  ASSERT_EQ(ERR_NAME_NOT_RESOLVED, callback_.WaitForResult());
   EXPECT_EQ(LOAD_STATE_IDLE, transaction_.GetLoadState());
 }
 
@@ -748,7 +748,7 @@ TEST_F(FtpNetworkTransactionTest, DownloadTransactionAcceptedDataConnection) {
   // Start the transaction.
   ASSERT_EQ(ERR_IO_PENDING,
             transaction_.Start(&request_info, &callback_, NULL));
-  EXPECT_EQ(OK, callback_.WaitForResult());
+  ASSERT_EQ(OK, callback_.WaitForResult());
 
   // The transaction fires the callback when we can start reading data.
   EXPECT_EQ(FtpSocketDataProvider::PRE_QUIT, ctrl_socket.state());
@@ -758,7 +758,7 @@ TEST_F(FtpNetworkTransactionTest, DownloadTransactionAcceptedDataConnection) {
   ASSERT_EQ(ERR_IO_PENDING,
             transaction_.Read(io_buffer.get(), kBufferSize, &callback_));
   EXPECT_EQ(LOAD_STATE_READING_RESPONSE, transaction_.GetLoadState());
-  EXPECT_EQ(static_cast<int>(mock_data.length()),
+  ASSERT_EQ(static_cast<int>(mock_data.length()),
             callback_.WaitForResult());
   EXPECT_EQ(LOAD_STATE_READING_RESPONSE, transaction_.GetLoadState());
   EXPECT_EQ(mock_data, std::string(io_buffer->data(), mock_data.length()));
@@ -775,7 +775,7 @@ TEST_F(FtpNetworkTransactionTest, DownloadTransactionAcceptedDataConnection) {
 
   // Make sure the transaction finishes cleanly.
   EXPECT_EQ(LOAD_STATE_IDLE, transaction_.GetLoadState());
-  EXPECT_EQ(OK, callback_.WaitForResult());
+  ASSERT_EQ(OK, callback_.WaitForResult());
   EXPECT_EQ(FtpSocketDataProvider::QUIT, ctrl_socket.state());
   EXPECT_EQ(LOAD_STATE_IDLE, transaction_.GetLoadState());
 }
@@ -833,7 +833,7 @@ TEST_F(FtpNetworkTransactionTest, DownloadTransactionEvilPasvUnsafeHost) {
   // Start the transaction.
   ASSERT_EQ(ERR_IO_PENDING,
             transaction_.Start(&request_info, &callback_, NULL));
-  EXPECT_EQ(OK, callback_.WaitForResult());
+  ASSERT_EQ(OK, callback_.WaitForResult());
 
   // The transaction fires the callback when we can start reading data. That
   // means that the data socket should be open.
@@ -843,11 +843,13 @@ TEST_F(FtpNetworkTransactionTest, DownloadTransactionEvilPasvUnsafeHost) {
   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);
+  // to the address we used for control connection (which could be 127.0.0.1
+  // or ::1 depending on whether we use IPv6).
+  const struct addrinfo* addrinfo = data_socket->addresses().head();
+  while (addrinfo) {
+    EXPECT_NE("1.2.3.4", NetAddressToString(addrinfo));
+    addrinfo = addrinfo->ai_next;
+  }
 }
 
 TEST_F(FtpNetworkTransactionTest, DownloadTransactionEvilLoginBadUsername) {
@@ -881,7 +883,7 @@ TEST_F(FtpNetworkTransactionTest, EvilRestartUser) {
 
   ASSERT_EQ(ERR_IO_PENDING,
             transaction_.Start(&request_info, &callback_, NULL));
-  EXPECT_EQ(ERR_FAILED, callback_.WaitForResult());
+  ASSERT_EQ(ERR_FAILED, callback_.WaitForResult());
 
   MockRead ctrl_reads[] = {
     MockRead("220 host TestFTPd\r\n"),
@@ -911,7 +913,7 @@ TEST_F(FtpNetworkTransactionTest, EvilRestartPassword) {
 
   ASSERT_EQ(ERR_IO_PENDING,
             transaction_.Start(&request_info, &callback_, NULL));
-  EXPECT_EQ(ERR_FAILED, callback_.WaitForResult());
+  ASSERT_EQ(ERR_FAILED, callback_.WaitForResult());
 
   MockRead ctrl_reads[] = {
     MockRead("220 host TestFTPd\r\n"),