Update IPAddress::IsReserved() ranges.

net/base/ip_address.cc

 // Copyright (c) 2015 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/base/ip_address.h"
 
 #include "base/strings/string_piece.h"
 #include "base/strings/string_split.h"
 #include "net/base/ip_address_number.h"
 #include "net/base/parse_number.h"
 #include "url/gurl.h"
 #include "url/url_canon_ip.h"
 
 namespace {
 
 bool IPAddressPrefixCheck(const std::vector<uint8_t>& ip_address,

[eroman, 2016/04/18 22:52:39]

While you are here, do you mind documenting the implicit assumptions of this
function?

* ip_address is at least prefix_length_in_bits (bits) long
* ip_prefix is at least prefix_length_in_bits (bits) long

[martijnc, 2016/04/19 18:30:41]

Done.

-                          const unsigned char* ip_prefix,
+                          const uint8_t* ip_prefix,
                           size_t prefix_length_in_bits) {
   // Compare all the bytes that fall entirely within the prefix.
   int num_entire_bytes_in_prefix = prefix_length_in_bits / 8;

[eroman, 2016/04/18 18:13:15]

side-comment: This existing function is a bit of a mess when it comes to "int"
vs "size_t", which could result in truncation/overflow.   In practice it is only
safe to call this after doing size checks anyway so probably not a reachable
problem. Might be  nice to clean it up in the future though.

[martijnc, 2016/04/18 21:39:54]

Changed to size_t for consistency (think that's the best option here).
This still truncates when |prefix_length_in_bits| isn't divisible by 8 but that
seems expected.

[eroman, 2016/04/18 22:52:38]

Correct -- truncation in that case is working as intended.

   for (int i = 0; i < num_entire_bytes_in_prefix; ++i) {
     if (ip_address[i] != ip_prefix[i])
       return false;
   }
 
   // In case the prefix was not a multiple of 8, there will be 1 byte
   // which is only partially masked.
   int remaining_bits = prefix_length_in_bits % 8;
   if (remaining_bits != 0) {
     unsigned char mask = 0xFF << (8 - remaining_bits);
     int i = num_entire_bytes_in_prefix;
     if ((ip_address[i] & mask) != (ip_prefix[i] & mask))
       return false;
   }
   return true;
 }
 
+// Returns true if |ip_address| matches any of the reserved IPv4 ranges. This
+// method operates on a blacklist of reserved IPv4 ranges. Some ranges are
+// consolidated.
+// Sources for info:
+// www.iana.org/assignments/ipv4-address-space/ipv4-address-space.xhtml
+// www.iana.org/assignments/iana-ipv4-special-registry/
+// iana-ipv4-special-registry.xhtml
+bool IsReservedIPv4(const std::vector<uint8_t>& ip_address) {
+  // Different IP versions have different range reservations.
+  DCHECK(ip_address.size() == net::IPAddress::kIPv4AddressSize);

[eroman, 2016/04/18 18:13:15]

DCHECK_EQ

[martijnc, 2016/04/18 21:39:54]

Done.

+  struct {
+    const uint8_t address[4];
+    size_t prefix_length;
+  } static const kReservedIPv4Ranges[] = {

[eroman, 2016/04/18 18:13:15]

For the IPv4 ranges here, I confirmed that these correspond with the previously
recognized ones.

As far as whether we are missing some for ranges IPv4, I haven't fully analyzed
that data yet.

Feel free to proceed with this CL as-is.
I will post my analysis of the IPv4 ranges when I get the chance, and if we are
missing stuff can add it in a follow-up.

+      {{0, 0, 0, 0}, 8},     {{10, 0, 0, 0}, 8},      {{100, 64, 0, 0}, 10},
+      {{127, 0, 0, 0}, 8},   {{169, 254, 0, 0}, 16},  {{172, 16, 0, 0}, 12},
+      {{192, 0, 2, 0}, 24},  {{192, 88, 99, 0}, 24},  {{192, 168, 0, 0}, 16},
+      {{198, 18, 0, 0}, 15}, {{198, 51, 100, 0}, 24}, {{203, 0, 113, 0}, 24},
+      {{224, 0, 0, 0}, 3}};
+
+  for (const auto& range : kReservedIPv4Ranges)
+    if (IPAddressPrefixCheck(ip_address, range.address, range.prefix_length))
+      return true;
+
+  return false;
+}
+
+// Returns true if |ip_address| matches any of the reserved IPv6 ranges. This
+// method operates on a whitelist of non-reserved IPv6 ranges. All IPv6
+// addresses outside these ranges are reserved.
+// Sources for info:
+// www.iana.org/assignments/ipv6-address-space/ipv6-address-space.xhtml
+bool IsReservedIPv6(const std::vector<uint8_t>& ip_address) {
+  // Different IP versions have different range reservations.
+  DCHECK(ip_address.size() == net::IPAddress::kIPv6AddressSize);

[eroman, 2016/04/18 18:13:15]

DCHECK_EQ.

[martijnc, 2016/04/18 21:39:54]

Done.

+  struct {
+    const uint8_t address[16];

[eroman, 2016/04/18 18:13:15]

The largest prefix length we need for the ranges below is just 1 byte. But there
is a bit of extra clarity for using 2 bytes.

Can you change this to just the prefix portion:

  const uint8_t address_prefix[2];

And also reword "prefix_length" to "prefix_length_in_bits"

[martijnc, 2016/04/18 21:39:54]

Done & done.

+    size_t prefix_length;
+  } static const kPublicIPv6Ranges[] = {
+      {{0x20, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0}, 3},

[eroman, 2016/04/18 18:13:15]

Please add comments to these that identify what they are:

// 2000::/3  -- Global Unicast
...

// ff00::/8  -- Multicast
...

[martijnc, 2016/04/18 21:39:54]

Done.

+      {{0xff, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0}, 8},
+  };
+
+  bool matched = false;
+  for (const auto& range : kPublicIPv6Ranges)
+    if (IPAddressPrefixCheck(ip_address, range.address, range.prefix_length))
+      matched = true;

[eroman, 2016/04/18 18:13:15]

Can you early return here instead?
   return false;

(No need to check the remaining ranges when one is matched).

[martijnc, 2016/04/18 21:39:54]

Done.

+
+  return !matched;
+}
+
 }  // namespace
 
 namespace net {
 
 IPAddress::IPAddress() {}
 
 IPAddress::IPAddress(const IPAddressNumber& address) : ip_address_(address) {}
 
 IPAddress::IPAddress(const IPAddress& other) = default;
 
@@ skipping 36 matching lines @@
 }
 
 bool IPAddress::IsIPv6() const {
   return ip_address_.size() == kIPv6AddressSize;
 }
 
 bool IPAddress::IsValid() const {
   return IsIPv4() || IsIPv6();
 }
 
-// Don't compare IPv4 and IPv6 addresses (they have different range
-// reservations). Keep separate reservation arrays for each IP type, and
-// consolidate adjacent reserved ranges within a reservation array when
-// possible.
-// Sources for info:
-// www.iana.org/assignments/ipv4-address-space/ipv4-address-space.xhtml
-// www.iana.org/assignments/ipv6-address-space/ipv6-address-space.xhtml
-// They're formatted here with the prefix as the last element. For example:
-// 10.0.0.0/8 becomes 10,0,0,0,8 and fec0::/10 becomes 0xfe,0xc0,0,0,0...,10.
 bool IPAddress::IsReserved() const {
-  static const unsigned char kReservedIPv4[][5] = {
-      {0, 0, 0, 0, 8},     {10, 0, 0, 0, 8},      {100, 64, 0, 0, 10},
-      {127, 0, 0, 0, 8},   {169, 254, 0, 0, 16},  {172, 16, 0, 0, 12},
-      {192, 0, 2, 0, 24},  {192, 88, 99, 0, 24},  {192, 168, 0, 0, 16},
-      {198, 18, 0, 0, 15}, {198, 51, 100, 0, 24}, {203, 0, 113, 0, 24},
-      {224, 0, 0, 0, 3}};
-  static const unsigned char kReservedIPv6[][17] = {
-      {0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 8},
-      {0x40, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 2},
-      {0x80, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 2},
-      {0xc0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 3},
-      {0xe0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 4},
-      {0xf0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 5},
-      {0xf8, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 6},
-      {0xfc, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 7},
-      {0xfe, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 9},
-      {0xfe, 0x80, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 10},
-      {0xfe, 0xc0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 10},
-  };
-  size_t array_size = 0;
-  const unsigned char* array = nullptr;
-  switch (ip_address_.size()) {
-    case kIPv4AddressSize:
-      array_size = arraysize(kReservedIPv4);
-      array = kReservedIPv4[0];
-      break;
-    case kIPv6AddressSize:
-      array_size = arraysize(kReservedIPv6);
-      array = kReservedIPv6[0];
-      break;
-  }
-  if (!array)
-    return false;
-  size_t width = ip_address_.size() + 1;
-  for (size_t i = 0; i < array_size; ++i, array += width) {
-    if (IPAddressPrefixCheck(ip_address_, array, array[width - 1]))
-      return true;
+  if (IsIPv4()) {
+    return IsReservedIPv4(ip_address_);
+  } else if (IsIPv6()) {
+    return IsReservedIPv6(ip_address_);
   }
   return false;
 }
 
 bool IPAddress::IsZero() const {
   for (auto x : ip_address_) {
     if (x != 0)
       return false;
   }
 
@@ skipping 148 matching lines @@
 
 unsigned CommonPrefixLength(const IPAddress& a1, const IPAddress& a2) {
   return CommonPrefixLength(a1.bytes(), a2.bytes());
 }
 
 unsigned MaskPrefixLength(const IPAddress& mask) {
   return MaskPrefixLength(mask.bytes());
 }
 
 }  // namespace net