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 == epsv_line.npos || epsv_line.length() - start < 6)

[eroman, 2016/11/28 22:51:57]

This is more commonly written as |StringPiece::npos|, can you switch to that
style? Also, may as well change the bound to 7 since that is the minimum for
success it looks like.

[mmenke, 2016/11/28 23:15:48]

Done.

     return false;
 
-  return true;
+  // Make sure we have "(<d><d><d>...", where <d> is not a number.
+  if (isdigit(epsv_line[start + 1]) ||
+      epsv_line[start + 1] != epsv_line[start + 2] ||
+      epsv_line[start + 1] != epsv_line[start + 3]) {

[mmenke, 2016/11/28 22:28:18]

This is still ugly, but it seems a bit better to me than using raw pointers with
null checks.

[eroman, 2016/11/28 22:51:57]

Can you extract epsv_line[start+1] to a variable?

[mmenke, 2016/11/28 23:15:48]

Done.

+    return false;
+  }
+
+  // Skip over those characters.
+  start += 4;
+
+  // Make sure there's a terminal <d>.
+  size_t end = epsv_line.find(epsv_line[start - 1], start);

[eroman, 2016/11/28 22:51:57]

Same comment. Rather than using epsv_line[start-1] please use a common variable
for "<d>".

[mmenke, 2016/11/28 23:15:48]

Done.

+  if (end == epsv_line.npos)

[eroman, 2016/11/28 22:51:57]

StringPiece::npos

[mmenke, 2016/11/28 23:15:48]

Done.

+    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 29 matching lines @@
   // 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;
 
   // 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))
+  if (!ParseInt32(pieces[4], ParseIntFormat::NON_NEGATIVE, &p0))

[eroman, 2016/11/28 22:51:57]

ParseUint32() ?

or should we add a ParseUint8() ?

[mmenke, 2016/11/28 23:15:48]

Went with ParseUint32 (Don't think adding a new method is worth it, unless there
are other places that need it)

     return false;
-  if (!base::StringToInt(pieces[5], &p1))
+  if (!ParseInt32(pieces[5], ParseIntFormat::NON_NEGATIVE, &p1))
     return false;
+  if (p0 >= 256 || p1 >= 256)

[eroman, 2016/11/28 22:51:57]

optional nit: I think it would be clearer writte as "p0 > 0xFF || p1 > 0xFF"

[mmenke, 2016/11/28 23:15:48]

Done.

+    return false;
+
   *port = (p0 << 8) + p1;
-
   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