Treat reserved IP addresses as mixed content.

Treat reserved IP addresses as mixed content.

This CL adds a platform method to determine if a hostname is an IPv4 or
IPv6 address which has been reserved by IANA. The content-side of this
patch will hook the new method up to net::IsReservedIPAddress.

This patch will have no practical effect on the web, as the method it
defines always returns false. Subsequent patches will break things in
the ways we want to break them.

BUG=378566

[Mike West, 2014-07-31 10:38:24]

Jochen, WDYT of this approach?

More importantly, do you have suggestions for testing this change after landing
the Chromium side? Do we have any mechanism for running layout tests on a
non-127.0.0.1 address? :)

-mike

[jochen (gone - plz use gerrit), 2014-07-31 14:55:58]

uh, I think I'm not really qualified, maybe Adam?

at least on linux, all 127/8 addresses connect to the local host.

[Mike West, 2014-07-31 15:25:49]

On 2014/07/31 14:55:58, jochen wrote:
> uh, I think I'm not really qualified, maybe Adam?

Now that folks are responding on the net-dev thread, I'll put this on the
backburner until networky folks decide whether we should do this in Blink, or in
the network stack.
 
> at least on linux, all 127/8 addresses connect to the local host.

The goal is to block "reserved" addresses when loaded on non-"reserved" pages.
That is, `example.com` shouldn't be able to poke at my router. If we only run
tests from 127.0.0.1, that behavior is going to be difficult to verify.

[Mike West, 2014-08-01 14:25:31]

Would you mind taking a look at this approach, Adam? The relevant net-dev thread
is here:
https://groups.google.com/a/chromium.org/d/msg/net-dev/oyUB2bWKGuE/JvX7XYLevlwJ

The Chromium-side of this patch is here:
https://codereview.chromium.org/430193002/

I don't have a good idea how to put together a layout test for this patch, since
HTTP layout tests run on 127.0.0.1, and therefore would never trigger this
blocking behavior. If you don't have an idea off the top of your head, I'll poke
at blink-dev/infra. :)

Thanks!

-mike

[abarth-chromium, 2014-08-01 18:09:45]

Mike, I think you should feel like you're the owner of this code now, which
means it's your decision what direction to head in an what approach to take.  In
that light, I'd recommend that you have someone from the security tmean like
palmer or rsleevi approve this CL to validate the policy change.

https://codereview.chromium.org/417153004/diff/1/Source/core/loader/MixedCont...
File Source/core/loader/MixedContentChecker.cpp (right):

https://codereview.chromium.org/417153004/diff/1/Source/core/loader/MixedCont...
Source/core/loader/MixedContentChecker.cpp:63: return
SecurityOrigin::create(url)->isReservedIPAddress();
Why do we need to create a SecurityOrigin just to call
Platform::current()->isHostnameReservedIPAddress with the host?  I guess we need
to unpack inner URLs?

https://codereview.chromium.org/417153004/diff/1/Source/platform/weborigin/Se...
File Source/platform/weborigin/SecurityOrigin.cpp (right):

https://codereview.chromium.org/417153004/diff/1/Source/platform/weborigin/Se...
Source/platform/weborigin/SecurityOrigin.cpp:449: return
blink::Platform::current()->isHostnameReservedIPAddress(m_host);
No need for blink::

[Mike West, 2014-08-02 15:21:46]

Mind taking a look, Ryan?

https://codereview.chromium.org/417153004/diff/1/Source/core/loader/MixedCont...
File Source/core/loader/MixedContentChecker.cpp (right):

https://codereview.chromium.org/417153004/diff/1/Source/core/loader/MixedCont...
Source/core/loader/MixedContentChecker.cpp:63: return
SecurityOrigin::create(url)->isReservedIPAddress();
On 2014/08/01 18:09:45, abarth wrote:
> Why do we need to create a SecurityOrigin just to call
> Platform::current()->isHostnameReservedIPAddress with the host?  I guess we
need
> to unpack inner URLs?

I can skip SecurityOrigin entirely and just call the platform method here. It
seemed like something that conceptually belonged to the SecurityOrigin, like
'isSecure', but as long as this is the only callsite I'll skip that bit.

[Mike West, 2014-08-02 15:21:47]

Mind taking a look, Ryan?

[Ryan Sleevi, 2014-08-04 23:55:45]

I was going to punt this to Adam, but I see he's already punted to me. Since he
mentioned security team, I'm going to half-punt to palmer.

From a networking perspective, I think this is correct, but I'd like to see
tests. If there is trouble testing, I'd like to know what I can do to help you,
since I imagine it's "tricky". 

From an policy side, do you have a way to quantify how this will affect users?
Do you have metrics that look at the incidence of mixed content? Can you bucket
these errors for mixed realm separately? For SSL related policy changes, I've
found it increasingly necessary to gather 'real world' metrics first. Of course,
it may be that 'real world' metrics aren't meaningful here - because we believe
the users most effected aren't opted into metrics, and because we believe the
security risk is reasonable enough to break some eggs. +palmer@ for the last bit
(since we probably do)

[palmer, 2014-08-05 00:05:51]

> I was going to punt this to Adam, but I see he's already punted to me. Since
he
> mentioned security team, I'm going to half-punt to palmer.

I will steal 2nd base and then pivot to shoot a 3-pointer into the penalty box,
thus tying the score at 40 - love.

> From an policy side, do you have a way to quantify how this will affect users?
> Do you have metrics that look at the incidence of mixed content? Can you
bucket
> these errors for mixed realm separately? For SSL related policy changes, I've
> found it increasingly necessary to gather 'real world' metrics first. Of
course,
> it may be that 'real world' metrics aren't meaningful here - because we
believe
> the users most effected aren't opted into metrics, and because we believe the
> security risk is reasonable enough to break some eggs. +palmer@ for the last
bit
> (since we probably do)

Yeah, I'd be more inclined to say that public web content (securely-transported
or otherwise) cannot include private address content at all. The loads should
just fail.

For specifically securely-transported public web content, it should already show
up as mixed content since hopefully CAs are no longer signing/we are no longer
accepting certificates that have IP address subjects (reserved IPs or public
IPs! But at least not reserved ones!).

For content loaded from a reserved IP, it already can't be secure (can't get a
certificate for it), and so there's no harm in letting it pull resources in from
the public. But, https://10.0.0.134 (if it somehow works, like with a
locally-installed trust anchor) pulling in http://cdn.jquery.com/goats.js should
still get treated as mixed content, following the usual rules.

[Mike West, 2014-08-05 06:09:57]

On 2014/08/04 23:55:45, Ryan Sleevi wrote:
> I was going to punt this to Adam, but I see he's already punted to me. Since
he
> mentioned security team, I'm going to half-punt to palmer.

:)

> From a networking perspective, I think this is correct, but I'd like to see
> tests. If there is trouble testing, I'd like to know what I can do to help
you,
> since I imagine it's "tricky".

We don't currently seem to have a way of running layout tests on anything other
than 127.0.0.1. That's going to make testing the change difficult.

> From an policy side, do you have a way to quantify how this will affect users?
> Do you have metrics that look at the incidence of mixed content? Can you
bucket
> these errors for mixed realm separately? For SSL related policy changes, I've
> found it increasingly necessary to gather 'real world' metrics first. Of
course,
> it may be that 'real world' metrics aren't meaningful here - because we
believe
> the users most effected aren't opted into metrics, and because we believe the
> security risk is reasonable enough to break some eggs. +palmer@ for the last
bit
> (since we probably do)

I do expect it to break something somewhere, but I don't have a good feel for
how much. I'm happy to add UseCounters for this, and we'll possibly end up with
an enterprise policy to (temporarily) turn off these changes.

[Mike West, 2014-08-05 06:16:49]

On 2014/08/05 00:05:51, Chromium Palmer wrote:
> For specifically securely-transported public web content, it should already
show
> up as mixed content since hopefully CAs are no longer signing/we are no longer
> accepting certificates that have IP address subjects (reserved IPs or public
> IPs! But at least not reserved ones!).

That's a good point; I'll move this check under the existing HTTP-in-HTTPS
checks (which should help allay some of Ryan's performance concerns on the other
CL).

Note, however, that there's a probably large number of "public" DNS entries
pointing to reserved IP addresses. `localtest.me` -> 127.0.0.1, for example, or
`intranet.megacorp.com`. This CL doesn't address that case, but I think we need
to consider doing so in the future

> For content loaded from a reserved IP, it already can't be secure (can't get a
> certificate for it), and so there's no harm in letting it pull resources in
from
> the public. But, https://10.0.0.134 (if it somehow works, like with a
> locally-installed trust anchor) pulling in http://cdn.jquery.com/goats.js
should
> still get treated as mixed content, following the usual rules.

Right. That's how it's specified (step 6 of
http://w3c.github.io/webappsec/specs/mixedcontent/#should-block-fetch).

[Mike West, 2014-08-06 09:08:14]

Jochen, do you have opinions about passing a (Web)SecurityOrigin around from
core? Halp!

https://codereview.chromium.org/417153004/diff/60001/Source/core/loader/Mixed...
File Source/core/loader/MixedContentChecker.cpp (right):

https://codereview.chromium.org/417153004/diff/60001/Source/core/loader/Mixed...
Source/core/loader/MixedContentChecker.cpp:66: if
(!Platform::current()->isReservedIPAddress(originURL))
I would like to pass the SecurityOrigin to the platform layer directly. But I
can't because layering. :(

[Mike West, 2014-08-07 09:15:39]

Splitting this into the platform change
(https://codereview.chromium.org/446363002/), then the chromium side change
(https://codereview.chromium.org/430193002/), then the mixed content change
(TBD).