Block port 443 for all protocols other than HTTPS or WSS.

Block port 443 for all protocols other than HTTPS or WSS.

This addresses the history leak (on non-preloaded HSTS sites) from https://crbug.com/436451:

  "If we ask Chrome to load http://example.com:443, it will definitely fail, because Chrome will make plain-text HTTP request to port 443 of the server. However, if example.com is a Known HSTS Host of Chrome (meaning either the user has visited https://example.com before, or it is on the HSTS preload list), it will send request to https://example.com:443, and the request will succeed. We can use JavaScript to differentiate the two cases, since in the first case, onerror event is triggered, while in the second case, onload event is triggered.

  Therefore, a malicious website can include well-chosen cross-domain images and use this trick to brute-force a list of domains that users have visited. Note that the list could only contain HSTS-enabled but not preloaded websites."

BUG=436451
Committed: https://crrev.com/b6cf19c7b9dd536405c3c4f80876411733c9d5a5
Cr-Commit-Position: refs/heads/master@{#306959}

[lgarron, 2014-12-02 19:34:16]

lgarron@chromium.org changed reviewers:
+ davidben@chromium.org

[lgarron, 2014-12-02 19:36:55]

On 2014/12/02 19:34:17, lgarron wrote:

This should presumably have tests. Currently looking into it.

[lgarron, 2014-12-02 19:40:49]

On 2014/12/02 19:36:55, lgarron wrote:
> On 2014/12/02 19:34:17, lgarron wrote:
> 
> This should presumably have tests. Currently looking into it.

Also, do we usually put a (temporary?) flag as a workaround (e.g.
--allow-port-443-for-all-protocols)?

[davidben, 2014-12-02 20:39:45]

https://codereview.chromium.org/770343003/diff/60001/net/http/http_stream_fac...
File net/http/http_stream_factory_impl_job.cc (right):

https://codereview.chromium.org/770343003/diff/60001/net/http/http_stream_fac...
net/http/http_stream_factory_impl_job.cc:627: bool is_port_allowed =
IsPortAllowedByDefault(origin_.port());
This was in the original, but it's really weird that we compute this, throw it
away, and then call it again as part of the scheme-specific ones. How about
turning this into an if/else chain.

[lgarron, 2014-12-02 20:49:00]

https://codereview.chromium.org/770343003/diff/60001/net/http/http_stream_fac...
File net/http/http_stream_factory_impl_job.cc (right):

https://codereview.chromium.org/770343003/diff/60001/net/http/http_stream_fac...
net/http/http_stream_factory_impl_job.cc:627: bool is_port_allowed =
IsPortAllowedByDefault(origin_.port());
On 2014/12/02 20:39:45, David Benjamin wrote:
> This was in the original, but it's really weird that we compute this, throw it
> away, and then call it again as part of the scheme-specific ones. How about
> turning this into an if/else chain.

What about a more general helper function called IsPortAllowedByScheme(...)?

[lgarron, 2014-12-02 22:47:42]

On 2014/12/02 20:49:00, lgarron wrote:
>
https://codereview.chromium.org/770343003/diff/60001/net/http/http_stream_fac...
> File net/http/http_stream_factory_impl_job.cc (right):
> 
>
https://codereview.chromium.org/770343003/diff/60001/net/http/http_stream_fac...
> net/http/http_stream_factory_impl_job.cc:627: bool is_port_allowed =
> IsPortAllowedByDefault(origin_.port());
> On 2014/12/02 20:39:45, David Benjamin wrote:
> > This was in the original, but it's really weird that we compute this, throw
it
> > away, and then call it again as part of the scheme-specific ones. How about
> > turning this into an if/else chain.
> 
> What about a more general helper function called IsPortAllowedByScheme(...)?

For anyone (like me) who didn't know the existence/syntax of port overrides on
the commandline, this works: --explicitly-allowed-ports=443
So, if we block HTTP over port 443, there is already in existing workaround for
anyone who *really* needs to use it for testing.

[lgarron, 2014-12-03 00:24:31]

Ready for a close-up.

[davidben, 2014-12-03 20:16:11]

lgtm with two comments. Thanks!

https://codereview.chromium.org/770343003/diff/100001/net/base/net_util.cc
File net/base/net_util.cc (right):

https://codereview.chromium.org/770343003/diff/100001/net/base/net_util.cc#ne...
net/base/net_util.cc:110: 443,  // https / wss
Would be good to have a comment or bug reference here and/or at line 148.
Without knowing (or having forgotten) the context, it would be very confusing to
me why http://blah:443 is forbidden, but https://blah:80 isn't.

I dunno what our story is for CLs (which are public once landed) having
explanations of security-relevant bugs in the comments. You may need to ask
someone on security. I suppose a bug reference isn't going to be any more
interesting than the commit message. (Also your commit message explains the bug
anyway.)

https://codereview.chromium.org/770343003/diff/100001/net/base/net_util.cc#ne...
net/base/net_util.cc:333: return true;
Nit: 2 spaces.

[lgarron, 2014-12-04 01:16:12]

https://codereview.chromium.org/770343003/diff/100001/net/base/net_util.cc
File net/base/net_util.cc (right):

https://codereview.chromium.org/770343003/diff/100001/net/base/net_util.cc#ne...
net/base/net_util.cc:110: 443,  // https / wss
On 2014/12/03 20:16:11, David Benjamin wrote:
> Would be good to have a comment or bug reference here and/or at line 148.
> Without knowing (or having forgotten) the context, it would be very confusing
to
> me why http://blah:443 is forbidden, but https://blah:80 isn't.
> 
> I dunno what our story is for CLs (which are public once landed) having
> explanations of security-relevant bugs in the comments. You may need to ask
> someone on security. I suppose a bug reference isn't going to be any more
> interesting than the commit message. (Also your commit message explains the
bug
> anyway.)

After talking with Chris Palmer/Mike West, I'm going to make this patch (and the
corresponding issue) public

Adding a comment to this line, though.

[lgarron, 2014-12-04 01:17:53]

The CQ bit was checked by lgarron@chromium.org

[commit-bot: I haz the power, 2014-12-04 01:19:03]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/770343003/120001

[commit-bot: I haz the power, 2014-12-04 02:10:24]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-12-04 02:10:26]

Try jobs failed on following builders:
  ios_dbg_simulator on tryserver.chromium.mac
(http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_dbg_simulator...)

[lgarron, 2014-12-04 06:00:15]

The trybots broke on three tests:

HttpNetworkTransactionTest.DoNotUseSpdySessionForHttpOverTunnel
HttpNetworkTransactionTest.DoNotUseSpdySessionForHttp
HttpNetworkTransactionTest.UseSpdySessionForHttpWhenForced

They were introduced in
https://chromium.googlesource.com/chromium/src/+/8450d7292c94c2ef59b922b22c4c...
due to https://crbug.com/133176 (... which is still restricted?).

CCing rch@, who authored that commit.

rch@: Would it be alright to change the port on those unit tests to something
else than 443?
If not, what are the concerns/options?

Thanks,
»Lucas

[Ryan Hamilton, 2014-12-04 19:58:21]

On 2014/12/04 06:00:15, lgarron wrote:
> The trybots broke on three tests:
> 
> HttpNetworkTransactionTest.DoNotUseSpdySessionForHttpOverTunnel
> HttpNetworkTransactionTest.DoNotUseSpdySessionForHttp
> HttpNetworkTransactionTest.UseSpdySessionForHttpWhenForced
> 
> They were introduced in
>
https://chromium.googlesource.com/chromium/src/+/8450d7292c94c2ef59b922b22c4c...
> due to https://crbug.com/133176 (... which is still restricted?).
> 
> CCing rch@, who authored that commit.
> 
> rch@: Would it be alright to change the port on those unit tests to something
> else than 443?
> If not, what are the concerns/options?

Sure, that should be just fine!

[lgarron, 2014-12-05 01:06:58]

The CQ bit was checked by lgarron@chromium.org

[commit-bot: I haz the power, 2014-12-05 01:08:10]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/770343003/120001

[commit-bot: I haz the power, 2014-12-05 02:08:31]

Committed patchset #7 (id:120001)

[commit-bot: I haz the power, 2014-12-05 02:09:14]

Patchset 7 (id:??) landed as
https://crrev.com/b6cf19c7b9dd536405c3c4f80876411733c9d5a5
Cr-Commit-Position: refs/heads/master@{#306959}

[PhistucK, 2014-12-05 09:02:29]

phistuck@gmail.com changed reviewers:
+ phistuck@gmail.com

[PhistucK, 2014-12-05 09:02:31]

https://codereview.chromium.org/770343003/diff/120001/net/base/net_util.cc
File net/base/net_util.cc (right):

https://codereview.chromium.org/770343003/diff/120001/net/base/net_util.cc#ne...
net/base/net_util.cc:330: int array_size = arraysize(kAllowedHttpsOrWssPorts);
Just a drive by -
Should this (and similar cases) be size_t instead?

[mmenke, 2014-12-05 21:44:39]

mmenke@chromium.org changed reviewers:
+ mmenke@chromium.org

[mmenke, 2014-12-05 21:44:40]

https://codereview.chromium.org/770343003/diff/120001/net/base/net_util.cc
File net/base/net_util.cc (right):

https://codereview.chromium.org/770343003/diff/120001/net/base/net_util.cc#ne...
net/base/net_util.cc:127: 995,  // pop3+ssl
Per discussion on the bug, wonder if we should be more aggressive here, and just
prohibit all ports from 0 to 1024 (1 to 1023?), except for ports explicitly
assigned to the protocol we're checking.

https://codereview.chromium.org/770343003/diff/120001/net/base/net_util.cc#ne...
net/base/net_util.cc:333: return true;
Indent here is wrong.

[davidben, 2014-12-05 21:48:07]

https://codereview.chromium.org/770343003/diff/120001/net/base/net_util.cc
File net/base/net_util.cc (right):

https://codereview.chromium.org/770343003/diff/120001/net/base/net_util.cc#ne...
net/base/net_util.cc:127: 995,  // pop3+ssl
On 2014/12/05 21:44:40, mmenke wrote:
> Per discussion on the bug, wonder if we should be more aggressive here, and
just
> prohibit all ports from 0 to 1024 (1 to 1023?), except for ports explicitly
> assigned to the protocol we're checking.

That would probably break stuff. I know of at least one deployment that uses
https://blah:444/.

[mmenke, 2014-12-05 21:54:29]

https://codereview.chromium.org/770343003/diff/120001/net/base/net_util.cc
File net/base/net_util.cc (right):

https://codereview.chromium.org/770343003/diff/120001/net/base/net_util.cc#ne...
net/base/net_util.cc:127: 995,  // pop3+ssl
On 2014/12/05 21:48:07, David Benjamin wrote:
> On 2014/12/05 21:44:40, mmenke wrote:
> > Per discussion on the bug, wonder if we should be more aggressive here, and
> just
> > prohibit all ports from 0 to 1024 (1 to 1023?), except for ports explicitly
> > assigned to the protocol we're checking.
> 
> That would probably break stuff. I know of at least one deployment that uses
> https://blah:444/.

We could have a "This link is in direct violation of RFC 3986, section 7.2."
interstitial, with a "Report violation to the FBI" link.  :)

[PhistucK, 2014-12-05 21:54:33]

I know a lot of stuff that would break (though, a lot of them are localhost
stuff).
And I do not think this is what the RFC says. Only well known protocols in this
range should be blocked, not this range no matter what.

[mmenke, 2014-12-05 21:56:44]

On 2014/12/05 21:54:33, PhistucK wrote:
> I know a lot of stuff that would break (though, a lot of them are localhost
> stuff).
> And I do not think this is what the RFC says. Only well known protocols in
this
> range should be blocked, not this range no matter what.

THat's not what the RFC says, I believe:

Applications should prevent dereference of a URI that specifies a TCP
port number within the "well-known port" range (0 - 1023) unless the
protocol being used to dereference that URI is compatible with the
protocol expected on that well-known port.

Within the well-known port range, not well-known ports within the well-known
port range.

[mmenke, 2014-12-05 21:57:39]

On 2014/12/05 21:56:44, mmenke wrote:
> On 2014/12/05 21:54:33, PhistucK wrote:
> > I know a lot of stuff that would break (though, a lot of them are localhost
> > stuff).
> > And I do not think this is what the RFC says. Only well known protocols in
> this
> > range should be blocked, not this range no matter what.
> 
> THat's not what the RFC says, I believe:
> 
> Applications should prevent dereference of a URI that specifies a TCP
> port number within the "well-known port" range (0 - 1023) unless the
> protocol being used to dereference that URI is compatible with the
> protocol expected on that well-known port.
> 
> Within the well-known port range, not well-known ports within the well-known
> port range.

Ahh, you're right...wasn't reading far enough.  "protocol expected on that
well-known port"

[mmenke, 2014-12-05 21:59:32]

On 2014/12/05 21:57:39, mmenke wrote:
> On 2014/12/05 21:56:44, mmenke wrote:
> > On 2014/12/05 21:54:33, PhistucK wrote:
> > > I know a lot of stuff that would break (though, a lot of them are
localhost
> > > stuff).
> > > And I do not think this is what the RFC says. Only well known protocols in
> > this
> > > range should be blocked, not this range no matter what.
> > 
> > THat's not what the RFC says, I believe:
> > 
> > Applications should prevent dereference of a URI that specifies a TCP
> > port number within the "well-known port" range (0 - 1023) unless the
> > protocol being used to dereference that URI is compatible with the
> > protocol expected on that well-known port.
> > 
> > Within the well-known port range, not well-known ports within the well-known
> > port range.
> 
> Ahh, you're right...wasn't reading far enough.  "protocol expected on that
> well-known port"

Worth noting that most ports in that range are official well known ports,
though.  666 is officially registered for Doom, for example, which I don't think
is compatible with HTTP.

[lgarron, 2014-12-08 21:17:26]

A revert of this CL (patchset #7 id:120001) has been created in
https://codereview.chromium.org/780943003/ by lgarron@chromium.org.

The reason for reverting is: Unfortunately, this fix didn't do enough to
mitigate the original problem (it's easy to tell if a site has been visited).

It's also incomplete on its own (i.e. it needs further changes to prevent the
HSTS redirect).

See https://crbug.com/436451#c30.