cc rsleevi since I see you suggested the current behavior in https://bugs.chromium.org/p/chromium/issues/detail?id=602773#c9.

Thanks for the heads up. This is further expanded upon in https://www.iana.org/assignments/iana-ipv6-special-registry/iana-ipv6-special-registry.xhtml (IPv4 mapped addresses are reserved, not globally routable, not forwardable).

While it's true that it's reserved by protocol (and thus has special semantics), we did not have any use case for it. Note that IsReserved doesn't necessarily mean it's private (whether for local use or 'not routable'). That said, I'm not sure why either feature should be excluding such addresses.

Should we change SocketWatcher and the phisting filter to use a different check than "is reserved"?

I'm not sure on whether we should change, because I'm not sure why they use the existing logic they use :)

Apologies, it was an unowned question. Do we know who would know more about why either feature wants to exclude reserved addresses? And what their desired/intended behaviour is?

Sorry for the delayed response.

socket watcher uses this because it wants to exclude RTT observations that are not from publically accessible IP addresses. This was done because of privacy reasons (in case if we ever expose transport RTT estimates to Web) and because the observations from private networks are not likely a good indicator of the quality of the public network.

Adding nparker@ from safe browsing.

There are some other consumers of this API too. It seems the logic for mixed-content detection in Blink also uses this?

If socket watcher is the only one misusing this API, then we should consider adding the extra-checks to socket watcher. If there are multiple consumers, then probably IPAddress is a better place? We should at least add a comment in IPAddress header file that reserved address != private/unroutable address.

Thoughts?

tbansal: How do you want to measure "publicly accessible"? It's sort of a broad term - so I'm curious if things like multicast addresses are (or are not) interested for NQE, or 'special' IPs, etc? It sounds like you definitely want to exclude what might traditionally be called 'local' networks - but trying to figure out where in the many axis of comment #2 we want to draw the line.

I wouldn't even say 'misusing' the API - just different expectations. That said, if the API is setting bad expectations, we can consider changing the API. I'm CC'ing mike - from looking at the WebKit usage, it's just histogramming usage, but if we ended up making security decisions on it, being able to use a v4-mapped V6 address to bypass restrictions would probably be surprising :)

(FWIW, this is a great bug, and a good catch of mixed expectations)

multicast and special IPs are of interest to NQE. The only addresses that are not of interest are the private addresses (those that can be reused across networks).

Yes this has security implications, but it (currently) only affects the CSD Phishing system, which is a minor part of Safe Browsing's protection -- so it's not a P1 there.

In  https://crbug.com/450684  I'd like something like IsIpRfc1918() to skip some download warnings if we're confident it's a "local" network.  Does such a function exist? 

Not to the extent of my knowledge.  Also, what would such a function for for IPv6?  RFC 1918 predates that.  Just return false?  Or return true for the similar IPv6 ranges?  RFC 1918 also doesn't mention 127.*..

You're right, we'd need something more broad than RFC1918. So something more like "Is this IP likely to be unique and routable on the Internet." Should include

* RFC1918 v4 addrs
* loopback v4 (127.*)
* IPV6 that's not a global unicast addr. Maybe all but 2000::/3? (per https://www.iana.org/assignments/ipv6-address-space/ipv6-address-space.xhtml)

@nparker: But that'd exclude the network that originally got this bug filed (namely, the ::ffff:0:0/96 range of IPv4-mapped addresses - https://tools.ietf.org/html/rfc4291#section-2.5.5.2), while including ones like 6to4 ( https://tools.ietf.org/html/rfc3056#section-2 ) :)

Removing the network label - this issue is more with consumers, rather than a network stack issue, so adding labels of some consumers.  Also doing this to punt it out of the network bug triage queue.

nparker to follow up on this.

Alright having read through IsReserved() again, I think it's sufficient for Safe Browsing's purposes, except if it's returning true for IP4-mapped addrs. I tried making one (http://[::ffff:23c5:280a]/, aka 35.197.40.10) and it definitely routes from Google's networks just fine, which means it's abusable.

How about we exclude that range from IsReserved() since while officially the range is reserved, for all functional purposes it is like a regular IPv6 addr (correct me if that's wrong)? Alternately we could make a new IPAddress::IsPubliclyRoutable() with those semantics and call that from IsHostnameNonUnique() -- the latter is what I'd like all SB code to use.

re Comment #14: I think renaming the existing IsReserved to IsPubliclyRoutable(), and removing the v6 mapping, sounds reasonable.

I added PresentationAPI to see if their one usage - https://cs.chromium.org/chromium/src/chrome/browser/media/router/discovery/dial/dial_device_data.cc?rcl=c08fc3300a7dbc92472c16dcc2cd8983a3261e54&l=44 - also means this is reasonable, but from the read of the context and comment, I think it is.

Refreshed during triage.

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/4a78e3d9b8b302f7dfd4cc207aea835415495adb

commit 4a78e3d9b8b302f7dfd4cc207aea835415495adb
Author: Nathan Parker <nparker@chromium.org>
Date: Wed Apr 11 01:16:20 2018

Replace IPAddress::IsReserved() with IsPubliclyRoutable().

This fixes the fact that all of the "IPv-mapped to IPv6" addresses,
of the form ::ffff:/96, were marked as reserved when in fact they are
routable on the Internet. This means some code that thought it was
talking to a private IP was actually reachable via a public IP.

This switches all callers except those in third_party/blink, which I'll
do in in https://crrev.com/c/994276.

Bug:  755418 
Change-Id: Ic0002efae268a42c86e39541e0bb3808bd0a247c
Reviewed-on: https://chromium-review.googlesource.com/994332
Reviewed-by: Derek Cheng <imcheng@chromium.org>
Reviewed-by: Charlie Harrison <csharrison@chromium.org>
Reviewed-by: Matt Menke <mmenke@chromium.org>
Commit-Queue: Nathan Parker <nparker@chromium.org>
Cr-Commit-Position: refs/heads/master@{#549711}
[modify] https://crrev.com/4a78e3d9b8b302f7dfd4cc207aea835415495adb/chrome/browser/media/router/discovery/dial/dial_device_data.cc
[modify] https://crrev.com/4a78e3d9b8b302f7dfd4cc207aea835415495adb/chrome/browser/media/router/discovery/mdns/cast_media_sink_service_impl.cc
[modify] https://crrev.com/4a78e3d9b8b302f7dfd4cc207aea835415495adb/chrome/browser/page_load_metrics/observers/local_network_requests_page_load_metrics_observer.cc
[modify] https://crrev.com/4a78e3d9b8b302f7dfd4cc207aea835415495adb/chrome/browser/safe_browsing/client_side_detection_service.cc
[modify] https://crrev.com/4a78e3d9b8b302f7dfd4cc207aea835415495adb/chrome/browser/safe_browsing/client_side_detection_service_unittest.cc
[modify] https://crrev.com/4a78e3d9b8b302f7dfd4cc207aea835415495adb/extensions/browser/api/cast_channel/cast_channel_api.cc
[modify] https://crrev.com/4a78e3d9b8b302f7dfd4cc207aea835415495adb/net/base/ip_address.cc
[modify] https://crrev.com/4a78e3d9b8b302f7dfd4cc207aea835415495adb/net/base/ip_address.h
[modify] https://crrev.com/4a78e3d9b8b302f7dfd4cc207aea835415495adb/net/base/ip_address_unittest.cc
[modify] https://crrev.com/4a78e3d9b8b302f7dfd4cc207aea835415495adb/net/base/url_util.cc
[modify] https://crrev.com/4a78e3d9b8b302f7dfd4cc207aea835415495adb/net/base/url_util.h
[modify] https://crrev.com/4a78e3d9b8b302f7dfd4cc207aea835415495adb/net/nqe/network_quality_estimator_util.cc
[modify] https://crrev.com/4a78e3d9b8b302f7dfd4cc207aea835415495adb/net/nqe/socket_watcher.cc

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/514680fcc4906a3ecf603bc073780852749af9e2

commit 514680fcc4906a3ecf603bc073780852749af9e2
Author: Nathan Parker <nparker@chromium.org>
Date: Mon Apr 16 21:37:12 2018

Remove last IPAddress::IsReserved() by switching use in blink.

Bug:  755418 
Change-Id: I6265cc98f0f70d9387275cb1e346acdc1978f488
Reviewed-on: https://chromium-review.googlesource.com/994276
Commit-Queue: Nathan Parker <nparker@chromium.org>
Reviewed-by: Nico Weber <thakis@chromium.org>
Reviewed-by: Matt Menke <mmenke@chromium.org>
Cr-Commit-Position: refs/heads/master@{#551130}
[modify] https://crrev.com/514680fcc4906a3ecf603bc073780852749af9e2/net/base/ip_address.cc
[modify] https://crrev.com/514680fcc4906a3ecf603bc073780852749af9e2/net/base/ip_address.h
[modify] https://crrev.com/514680fcc4906a3ecf603bc073780852749af9e2/net/base/ip_address_unittest.cc
[modify] https://crrev.com/514680fcc4906a3ecf603bc073780852749af9e2/third_party/blink/renderer/platform/network/network_utils.cc
[modify] https://crrev.com/514680fcc4906a3ecf603bc073780852749af9e2/third_party/blink/renderer/platform/network/network_utils_test.cc

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/514680fcc4906a3ecf603bc073780852749af9e2

commit 514680fcc4906a3ecf603bc073780852749af9e2
Author: Nathan Parker <nparker@chromium.org>
Date: Mon Apr 16 21:37:12 2018

Remove last IPAddress::IsReserved() by switching use in blink.

Bug:  755418 
Change-Id: I6265cc98f0f70d9387275cb1e346acdc1978f488
Reviewed-on: https://chromium-review.googlesource.com/994276
Commit-Queue: Nathan Parker <nparker@chromium.org>
Reviewed-by: Nico Weber <thakis@chromium.org>
Reviewed-by: Matt Menke <mmenke@chromium.org>
Cr-Commit-Position: refs/heads/master@{#551130}
[modify] https://crrev.com/514680fcc4906a3ecf603bc073780852749af9e2/net/base/ip_address.cc
[modify] https://crrev.com/514680fcc4906a3ecf603bc073780852749af9e2/net/base/ip_address.h
[modify] https://crrev.com/514680fcc4906a3ecf603bc073780852749af9e2/net/base/ip_address_unittest.cc
[modify] https://crrev.com/514680fcc4906a3ecf603bc073780852749af9e2/third_party/blink/renderer/platform/network/network_utils.cc
[modify] https://crrev.com/514680fcc4906a3ecf603bc073780852749af9e2/third_party/blink/renderer/platform/network/network_utils_test.cc