More correctly handle username and password in FtpNetworkTransaction.

- prevent newline injection attacks
- correctly unescape credentials provided in the URL

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

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

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

2 files changed, 143 insertions, 3 deletions

diff --git a/net/ftp/ftp_network_transaction.cc b/net/ftp/ftp_network_transaction.cc
index cf4a773..753aee2 100644
--- a/net/ftp/ftp_network_transaction.cc
+++ b/net/ftp/ftp_network_transaction.cc
@@ -24,6 +24,23 @@ const char kCRLF[] = "\r\n";
 
 const int kCtrlBufLen = 1024;
 
+namespace {
+
+// Returns true if |input| can be safely used as a part of FTP command.
+bool IsValidFTPCommandString(const std::string& input) {
+  // RFC 959 only allows ASCII strings.
+  if (!IsStringASCII(input))
+    return false;
+
+  // Protect agains newline injection attack.
+  if (input.find_first_of("\r\n") != std::string::npos)
+    return false;
+
+  return true;
+}
+
+}  // namespace
+
 namespace net {
 
 FtpNetworkTransaction::FtpNetworkTransaction(
@@ -58,9 +75,7 @@ int FtpNetworkTransaction::Start(const FtpRequestInfo* request_info,
   request_ = request_info;
 
   if (request_->url.has_username()) {
-    username_ = UTF8ToWide(request_->url.username());
-    if (request_->url.has_password())
-      password_ = UTF8ToWide(request_->url.password());
+    GetIdentityFromURL(request_->url, &username_, &password_);
   } else {
     username_ = L"anonymous";
     password_ = L"chrome@example.com";
@@ -160,6 +175,13 @@ int FtpNetworkTransaction::SendFtpCommand(const std::string& command,
   DCHECK(!write_command_buf_);
   DCHECK(!write_buf_);
 
+  if (!IsValidFTPCommandString(command)) {
+    // Callers should validate the command themselves and return a more specific
+    // error code.
+    NOTREACHED();
+    return Stop(ERR_UNEXPECTED);
+  }
+
   command_sent_ = cmd;
 
   write_command_buf_ = new IOBufferWithSize(command.length() + 2);
@@ -518,6 +540,10 @@ int FtpNetworkTransaction::DoCtrlWriteComplete(int result) {
 // USER Command.
 int FtpNetworkTransaction::DoCtrlWriteUSER() {
   std::string command = "USER " + WideToUTF8(username_);
+
+  if (!IsValidFTPCommandString(command))
+    return Stop(ERR_MALFORMED_IDENTITY);
+
   next_state_ = STATE_CTRL_READ;
   return SendFtpCommand(command, COMMAND_USER);
 }
@@ -547,6 +573,10 @@ int FtpNetworkTransaction::ProcessResponseUSER(
 // PASS command.
 int FtpNetworkTransaction::DoCtrlWritePASS() {
   std::string command = "PASS " + WideToUTF8(password_);
+
+  if (!IsValidFTPCommandString(command))
+    return Stop(ERR_MALFORMED_IDENTITY);
+
   next_state_ = STATE_CTRL_READ;
   return SendFtpCommand(command, COMMAND_PASS);
 }
diff --git a/net/ftp/ftp_network_transaction_unittest.cc b/net/ftp/ftp_network_transaction_unittest.cc
index b3603c0..95b1e8e 100644
--- a/net/ftp/ftp_network_transaction_unittest.cc
+++ b/net/ftp/ftp_network_transaction_unittest.cc
@@ -339,6 +339,36 @@ class FtpMockControlSocketEvilPasv : public FtpMockControlSocketFileDownload {
   DISALLOW_COPY_AND_ASSIGN(FtpMockControlSocketEvilPasv);
 };
 
+class FtpMockControlSocketEvilLogin : public FtpMockControlSocketFileDownload {
+ public:
+  FtpMockControlSocketEvilLogin(const char* expected_user,
+                                const char* expected_password)
+      : expected_user_(expected_user),
+        expected_password_(expected_password) {
+  }
+
+  virtual MockWriteResult OnWrite(const std::string& data) {
+    if (InjectFault())
+      return MockWriteResult(true, data.length());
+    switch (state()) {
+      case PRE_USER:
+        return Verify(std::string("USER ") + expected_user_ + "\r\n", data,
+                      PRE_PASSWD, "331 Password needed\r\n");
+      case PRE_PASSWD:
+        return Verify(std::string("PASS ") + expected_password_ + "\r\n", data,
+                      PRE_SYST, "230 Welcome\r\n");
+      default:
+        return FtpMockControlSocketFileDownload::OnWrite(data);
+    }
+  }
+
+ private:
+  const char* expected_user_;
+  const char* expected_password_;
+
+  DISALLOW_COPY_AND_ASSIGN(FtpMockControlSocketEvilLogin);
+};
+
 class FtpNetworkTransactionTest : public PlatformTest {
  public:
   FtpNetworkTransactionTest()
@@ -584,6 +614,86 @@ TEST_F(FtpNetworkTransactionTest, DownloadTransactionEvilPasvUnsafeHost) {
   EXPECT_FALSE(data_socket->addresses().head()->ai_next);
 }
 
+TEST_F(FtpNetworkTransactionTest, DownloadTransactionEvilLoginBadUsername) {
+  FtpMockControlSocketEvilLogin ctrl_socket("hello%0Aworld", "test");
+  ExecuteTransaction(&ctrl_socket, "ftp://hello%0Aworld:test@host/file", OK);
+}
+
+TEST_F(FtpNetworkTransactionTest, DownloadTransactionEvilLoginBadPassword) {
+  FtpMockControlSocketEvilLogin ctrl_socket("test", "hello%0Dworld");
+  ExecuteTransaction(&ctrl_socket, "ftp://test:hello%0Dworld@host/file", OK);
+}
+
+TEST_F(FtpNetworkTransactionTest, DownloadTransactionSpaceInLogin) {
+  FtpMockControlSocketEvilLogin ctrl_socket("hello world", "test");
+  ExecuteTransaction(&ctrl_socket, "ftp://hello%20world:test@host/file", OK);
+}
+
+TEST_F(FtpNetworkTransactionTest, DownloadTransactionSpaceInPassword) {
+  FtpMockControlSocketEvilLogin ctrl_socket("test", "hello world");
+  ExecuteTransaction(&ctrl_socket, "ftp://test:hello%20world@host/file", OK);
+}
+
+TEST_F(FtpNetworkTransactionTest, EvilRestartUser) {
+  FtpMockControlSocket ctrl_socket1;
+  ctrl_socket1.InjectFailure(FtpMockControlSocket::PRE_PASSWD,
+                             FtpMockControlSocket::PRE_QUIT,
+                             "530 Login authentication failed\r\n");
+  mock_socket_factory_.AddMockSocket(&ctrl_socket1);
+
+  FtpRequestInfo request_info = GetRequestInfo("ftp://host/file");
+
+  ASSERT_EQ(ERR_IO_PENDING,
+            transaction_.Start(&request_info, &callback_, NULL));
+  EXPECT_EQ(ERR_FAILED, callback_.WaitForResult());
+
+  MockRead ctrl_reads[] = {
+    MockRead("220 host TestFTPd\r\n"),
+    MockRead("221 Goodbye!\r\n"),
+    MockRead(false, OK),
+  };
+  MockWrite ctrl_writes[] = {
+    MockWrite("QUIT\r\n"),
+  };
+  StaticMockSocket ctrl_socket2(ctrl_reads, ctrl_writes);
+  mock_socket_factory_.AddMockSocket(&ctrl_socket2);
+  ASSERT_EQ(ERR_IO_PENDING, transaction_.RestartWithAuth(L"foo\nownz0red",
+                                                         L"innocent",
+                                                         &callback_));
+  EXPECT_EQ(ERR_MALFORMED_IDENTITY, callback_.WaitForResult());
+}
+
+TEST_F(FtpNetworkTransactionTest, EvilRestartPassword) {
+  FtpMockControlSocket ctrl_socket1;
+  ctrl_socket1.InjectFailure(FtpMockControlSocket::PRE_PASSWD,
+                             FtpMockControlSocket::PRE_QUIT,
+                             "530 Login authentication failed\r\n");
+  mock_socket_factory_.AddMockSocket(&ctrl_socket1);
+
+  FtpRequestInfo request_info = GetRequestInfo("ftp://host/file");
+
+  ASSERT_EQ(ERR_IO_PENDING,
+            transaction_.Start(&request_info, &callback_, NULL));
+  EXPECT_EQ(ERR_FAILED, callback_.WaitForResult());
+
+  MockRead ctrl_reads[] = {
+    MockRead("220 host TestFTPd\r\n"),
+    MockRead("331 User okay, send password\r\n"),
+    MockRead("221 Goodbye!\r\n"),
+    MockRead(false, OK),
+  };
+  MockWrite ctrl_writes[] = {
+    MockWrite("USER innocent\r\n"),
+    MockWrite("QUIT\r\n"),
+  };
+  StaticMockSocket ctrl_socket2(ctrl_reads, ctrl_writes);
+  mock_socket_factory_.AddMockSocket(&ctrl_socket2);
+  ASSERT_EQ(ERR_IO_PENDING, transaction_.RestartWithAuth(L"innocent",
+                                                         L"foo\nownz0red",
+                                                         &callback_));
+  EXPECT_EQ(ERR_MALFORMED_IDENTITY, callback_.WaitForResult());
+}
+
 TEST_F(FtpNetworkTransactionTest, DirectoryTransactionFailUser) {
   FtpMockControlSocketDirectoryListing ctrl_socket;
   TransactionFailHelper(&ctrl_socket,