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

9 files changed, 226 insertions, 86 deletions

diff --git a/net/base/net_error_list.h b/net/base/net_error_list.h
index c750295..745227e 100644
--- a/net/base/net_error_list.h
+++ b/net/base/net_error_list.h
@@ -257,6 +257,9 @@ NET_ERROR(PAC_SCRIPT_FAILED, -327)
 // satisfy the range requested.
 NET_ERROR(REQUEST_RANGE_NOT_SATISFIABLE, -328)
 
+// The identity used for authentication is invalid.
+NET_ERROR(MALFORMED_IDENTITY, -329)
+
 // The cache does not have the requested entry.
 NET_ERROR(CACHE_MISS, -400)
 
diff --git a/net/base/net_util.cc b/net/base/net_util.cc
index 5f6b548..b8c8add 100644
--- a/net/base/net_util.cc
+++ b/net/base/net_util.cc
@@ -1169,6 +1169,14 @@ std::string GetHostName() {
   return std::string(buffer);
 }
 
+void GetIdentityFromURL(const GURL& url,
+                        std::wstring* username,
+                        std::wstring* password) {
+  UnescapeRule::Type flags = UnescapeRule::SPACES;
+  *username = UnescapeAndDecodeUTF8URLComponent(url.username(), flags);
+  *password = UnescapeAndDecodeUTF8URLComponent(url.password(), flags);
+}
+
 void AppendFormattedHost(const GURL& url,
                          const std::wstring& languages,
                          std::wstring* output,
diff --git a/net/base/net_util.h b/net/base/net_util.h
index 89386bf8..4d7e0aa 100644
--- a/net/base/net_util.h
+++ b/net/base/net_util.h
@@ -77,6 +77,12 @@ std::string NetAddressToString(const struct addrinfo* net_address);
 // Returns the hostname of the current system. Returns empty string on failure.
 std::string GetHostName();
 
+// Extracts the unescaped username/password from |url|, saving the results
+// into |*username| and |*password|.
+void GetIdentityFromURL(const GURL& url,
+                        std::wstring* username,
+                        std::wstring* password);
+
 // Return the value of the HTTP response header with name 'name'.  'headers'
 // should be in the format that URLRequest::GetResponseHeaders() returns.
 // Returns the empty string if the header is not found.
diff --git a/net/base/net_util_unittest.cc b/net/base/net_util_unittest.cc
index 2047b14..6a01ec9 100644
--- a/net/base/net_util_unittest.cc
+++ b/net/base/net_util_unittest.cc
@@ -530,6 +530,71 @@ TEST(NetUtilTest, FileURLConversion) {
   EXPECT_FALSE(net::FileURLToFilePath(GURL("filefoobar"), &output));
 }
 
+TEST(NetUtilTest, GetIdentityFromURL) {
+  struct {
+    const char* input_url;
+    const wchar_t* expected_username;
+    const wchar_t* expected_password;
+  } tests[] = {
+    {
+      "http://username:password@google.com",
+      L"username",
+      L"password",
+    },
+    { // Test for http://crbug.com/19200
+      "http://username:p@ssword@google.com",
+      L"username",
+      L"p@ssword",
+    },
+    { // Username contains %20.
+      "http://use rname:password@google.com",
+      L"use rname",
+      L"password",
+    },
+    { // Keep %00 as is.
+      "http://use%00rname:password@google.com",
+      L"use%00rname",
+      L"password",
+    },
+    { // Use a '+' in the username.
+      "http://use+rname:password@google.com",
+      L"use+rname",
+      L"password",
+    },
+    { // Use a '&' in the password.
+      "http://username:p&ssword@google.com",
+      L"username",
+      L"p&ssword",
+    },
+  };
+  for (size_t i = 0; i < ARRAYSIZE_UNSAFE(tests); ++i) {
+    SCOPED_TRACE(StringPrintf("Test[%d]: %s", i, tests[i].input_url));
+    GURL url(tests[i].input_url);
+
+    std::wstring username, password;
+    net::GetIdentityFromURL(url, &username, &password);
+
+    EXPECT_EQ(tests[i].expected_username, username);
+    EXPECT_EQ(tests[i].expected_password, password);
+  }
+}
+
+// Try extracting a username which was encoded with UTF8.
+TEST(NetUtilTest, GetIdentityFromURL_UTF8) {
+  GURL url(WideToUTF16(L"http://foo:\x4f60\x597d@blah.com"));
+
+  EXPECT_EQ("foo", url.username());
+  EXPECT_EQ("%E4%BD%A0%E5%A5%BD", url.password());
+
+  // Extract the unescaped identity.
+  std::wstring username, password;
+  net::GetIdentityFromURL(url, &username, &password);
+
+  // Verify that it was decoded as UTF8.
+  EXPECT_EQ(L"foo", username);
+  EXPECT_EQ(L"\x4f60\x597d", password);
+}
+
 // Just a bunch of fake headers.
 const wchar_t* google_headers =
     L"HTTP/1.1 200 OK\n"
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,
diff --git a/net/http/http_network_transaction.cc b/net/http/http_network_transaction.cc
index 1c39524..8fd0700 100644
--- a/net/http/http_network_transaction.cc
+++ b/net/http/http_network_transaction.cc
@@ -1808,7 +1808,7 @@ bool HttpNetworkTransaction::SelectNextAuthIdentityToTry(
     auth_identity_[target].source = HttpAuth::IDENT_SRC_URL;
     auth_identity_[target].invalid = false;
     // Extract the username:password from the URL.
-    GetIdentifyFromUrl(request_->url,
+    GetIdentityFromURL(request_->url,
                        &auth_identity_[target].username,
                        &auth_identity_[target].password);
     embedded_identity_used_ = true;
@@ -1848,15 +1848,6 @@ bool HttpNetworkTransaction::SelectNextAuthIdentityToTry(
   return false;
 }
 
-// static
-void HttpNetworkTransaction::GetIdentifyFromUrl(const GURL& url,
-                                                std::wstring* username,
-                                                std::wstring* password) {
-  UnescapeRule::Type flags = UnescapeRule::SPACES;
-  *username = UnescapeAndDecodeUTF8URLComponent(url.username(), flags);
-  *password = UnescapeAndDecodeUTF8URLComponent(url.password(), flags);
-}
-
 std::string HttpNetworkTransaction::AuthChallengeLogMessage() const {
   std::string msg;
   std::string header_val;
diff --git a/net/http/http_network_transaction.h b/net/http/http_network_transaction.h
index 7a0d635..d3b114d 100644
--- a/net/http/http_network_transaction.h
+++ b/net/http/http_network_transaction.h
@@ -62,8 +62,6 @@ class HttpNetworkTransaction : public HttpTransaction {
 
  private:
   FRIEND_TEST(HttpNetworkTransactionTest, ResetStateForRestart);
-  FRIEND_TEST(HttpNetworkTransactionTest, GetIdentifyFromUrl);
-  FRIEND_TEST(HttpNetworkTransactionTest, GetIdentifyFromUrl_UTF8);
 
   // This version of IOBuffer lets us use a string as the real storage and
   // "move" the data pointer inside the string before using it to do actual IO.
@@ -263,12 +261,6 @@ class HttpNetworkTransaction : public HttpTransaction {
   // was found.
   bool SelectNextAuthIdentityToTry(HttpAuth::Target target);
 
-  // Extract the unescaped username/password from |url|, saving the results
-  // into |*username| and |*password|.
-  static void GetIdentifyFromUrl(const GURL& url,
-                                 std::wstring* username,
-                                 std::wstring* password);
-
   // Searches the auth cache for an entry that encompasses the request's path.
   // If such an entry is found, updates auth_identity_[target] and
   // auth_handler_[target] with the cache entry's data and returns true.
diff --git a/net/http/http_network_transaction_unittest.cc b/net/http/http_network_transaction_unittest.cc
index dbe14e7..32185c4 100644
--- a/net/http/http_network_transaction_unittest.cc
+++ b/net/http/http_network_transaction_unittest.cc
@@ -3676,69 +3676,4 @@ TEST_F(HttpNetworkTransactionTest, BypassHostCacheOnRefresh) {
   EXPECT_EQ(ERR_NAME_NOT_RESOLVED, rv);
 }
 
-TEST_F(HttpNetworkTransactionTest, GetIdentifyFromUrl) {
-  struct {
-    const char* input_url;
-    const wchar_t* expected_username;
-    const wchar_t* expected_password;
-  } tests[] = {
-    {
-      "http://username:password@google.com",
-      L"username",
-      L"password",
-    },
-    { // Test for http://crbug.com/19200
-      "http://username:p@ssword@google.com",
-      L"username",
-      L"p@ssword",
-    },
-    { // Username contains %20.
-      "http://use rname:password@google.com",
-      L"use rname",
-      L"password",
-    },
-    { // Keep %00 as is.
-      "http://use%00rname:password@google.com",
-      L"use%00rname",
-      L"password",
-    },
-    { // Use a '+' in the username.
-      "http://use+rname:password@google.com",
-      L"use+rname",
-      L"password",
-    },
-    { // Use a '&' in the password.
-      "http://username:p&ssword@google.com",
-      L"username",
-      L"p&ssword",
-    },
-  };
-  for (size_t i = 0; i < ARRAYSIZE_UNSAFE(tests); ++i) {
-    SCOPED_TRACE(StringPrintf("Test[%d]: %s", i, tests[i].input_url));
-    GURL url(tests[i].input_url);
-
-    std::wstring username, password;
-    HttpNetworkTransaction::GetIdentifyFromUrl(url, &username, &password);
-
-    EXPECT_EQ(tests[i].expected_username, username);
-    EXPECT_EQ(tests[i].expected_password, password);
-  }
-}
-
-// Try extracting a username which was encoded with UTF8.
-TEST_F(HttpNetworkTransactionTest, GetIdentifyFromUrl_UTF8) {
-  GURL url(WideToUTF16(L"http://foo:\x4f60\x597d@blah.com"));
-
-  EXPECT_EQ("foo", url.username());
-  EXPECT_EQ("%E4%BD%A0%E5%A5%BD", url.password());
-
-  // Extract the unescaped identity.
-  std::wstring username, password;
-  HttpNetworkTransaction::GetIdentifyFromUrl(url, &username, &password);
-
-  // Verify that it was decoded as UTF8.
-  EXPECT_EQ(L"foo", username);
-  EXPECT_EQ(L"\x4f60\x597d", password);
-}
-
 }  // namespace net