Use overflow-safe string-to-int parsing methods for FTP ports.

net/ftp/ftp_network_transaction.cc

 // Copyright (c) 2012 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "net/ftp/ftp_network_transaction.h"
 
 #include "base/bind.h"
 #include "base/bind_helpers.h"
 #include "base/compiler_specific.h"
 #include "base/metrics/histogram_macros.h"
 #include "base/strings/string_number_conversions.h"
 #include "base/strings/string_split.h"
 #include "base/strings/string_util.h"
 #include "base/strings/utf_string_conversions.h"
 #include "base/values.h"
 #include "net/base/address_list.h"
 #include "net/base/escape.h"
 #include "net/base/net_errors.h"
+#include "net/base/parse_number.h"
 #include "net/base/port_util.h"
 #include "net/base/url_util.h"
 #include "net/ftp/ftp_request_info.h"
 #include "net/ftp/ftp_util.h"
 #include "net/log/net_log.h"
 #include "net/log/net_log_event_type.h"
 #include "net/log/net_log_source.h"
 #include "net/socket/client_socket_factory.h"
 #include "net/socket/stream_socket.h"
 #include "url/url_constants.h"
@@ skipping 87 matching lines @@
   }
 }
 
 // From RFC 2428 Section 3:
 //   The text returned in response to the EPSV command MUST be:
 //     <some text> (<d><d><d><tcp-port><d>)
 //   <d> is a delimiter character, ideally to be |
 bool ExtractPortFromEPSVResponse(const FtpCtrlResponse& response, int* port) {
   if (response.lines.size() != 1)
     return false;
-  const char* ptr = response.lines[0].c_str();
-  while (*ptr && *ptr != '(')
-    ++ptr;
-  if (!*ptr)
-    return false;
-  char sep = *(++ptr);
-  if (!sep || isdigit(sep) || *(++ptr) != sep || *(++ptr) != sep)
-    return false;
-  if (!isdigit(*(++ptr)))
-    return false;
-  *port = *ptr - '0';
-  while (isdigit(*(++ptr))) {
-    *port *= 10;
-    *port += *ptr - '0';
-  }
-  if (*ptr != sep)
+
+  base::StringPiece epsv_line(response.lines[0]);
+  size_t start = epsv_line.find('(');
+  // If the line doesn't have a '(' or doesn't have enough characters after the
+  // first '(', fail.
+  if (start == base::StringPiece::npos || epsv_line.length() - start < 7)
     return false;
 
-  return true;
+  char separator = epsv_line[start + 1];
+
+  // Make sure we have "(<d><d><d>...", where <d> is not a number.
+  if (isdigit(separator) || epsv_line[start + 2] != separator ||

[eroman, 2016/11/28 23:26:17]

I am not sure that we should be using isdigit() here.

From consulting various documentation, it sounds like std::isdigit() is
*generally* locale-insensitive, but not necessarily. Some implementations may
recognize additional characters as digits :(

I think it would be better to just test [0-9] directly. Or I guess could even
call ParseUint32() on the single-character string piece for consistency.

Generally speaking I don't think we should ever be using c library functions
(which depend on the current locale) when parsing network protocols 

+      epsv_line[start + 3] != separator) {
+    return false;
+  }
+
+  // Skip over those characters.
+  start += 4;
+
+  // Make sure there's a terminal <d>.
+  size_t end = epsv_line.find(separator, start);
+  if (end == base::StringPiece::npos)
+    return false;
+
+  return ParseInt32(epsv_line.substr(start, end - start),
+                    ParseIntFormat::NON_NEGATIVE, port);
 }
 
 // There are two way we can receive IP address and port.
 // (127,0,0,1,23,21) IP address and port encapsulated in ().
 // 127,0,0,1,23,21  IP address and port without ().
 //
 // See RFC 959, Section 4.1.2
 bool ExtractPortFromPASVResponse(const FtpCtrlResponse& response, int* port) {
   if (response.lines.size() != 1)
     return false;
@@ skipping 26 matching lines @@
     line = line.substr(paren_pos + 1, closing_paren_pos - paren_pos - 1);
   }
 
   // Split the line into comma-separated pieces and extract
   // the last two.
   std::vector<base::StringPiece> pieces = base::SplitStringPiece(
       line, ",", base::TRIM_WHITESPACE, base::SPLIT_WANT_ALL);
   if (pieces.size() != 6)
     return false;
 
+  LOG(ERROR) << "here? " << pieces[4].as_string();
+  LOG(ERROR) << "here? " << pieces[5].as_string();
   // 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.
-  int p0, p1;
-  if (!base::StringToInt(pieces[4], &p0))
+  uint32_t p0, p1;
+  if (!ParseUint32(pieces[4], &p0))
     return false;
-  if (!base::StringToInt(pieces[5], &p1))
+  if (!ParseUint32(pieces[5], &p1))
     return false;
+  if (p0 > 0xFF || p1 > 0xFF)
+    return false;
+
+  LOG(ERROR) << "here2?";
   *port = (p0 << 8) + p1;
-
+  LOG(ERROR) << "here3? " << *port;
   return true;
 }
 
 }  // namespace
 
 FtpNetworkTransaction::FtpNetworkTransaction(
     HostResolver* resolver,
     ClientSocketFactory* socket_factory)
     : command_sent_(COMMAND_NONE),
       io_callback_(base::Bind(&FtpNetworkTransaction::OnIOComplete,
@@ skipping 1133 matching lines @@
   if (!had_error_type[type]) {
     had_error_type[type] = true;
     UMA_HISTOGRAM_ENUMERATION("Net.FtpDataConnectionErrorHappened",
         type, NUM_OF_NET_ERROR_TYPES);
   }
   UMA_HISTOGRAM_ENUMERATION("Net.FtpDataConnectionErrorCount",
       type, NUM_OF_NET_ERROR_TYPES);
 }
 
 }  // namespace net