Chromium Code Reviews
Help | Chromium Project | Gerrit Changes | Sign in
(53)

Issue 770343003: Block port 443 for all protocols other than HTTPS or WSS. (Closed)

Created:
4 years, 6 months ago by lgarron
Modified:
4 years, 6 months ago
Reviewers:
davidben, PhistucK, mmenke
CC:
cbentzel+watch_chromium.org, palmer
Base URL:
https://chromium.googlesource.com/chromium/src.git@master
Project:
chromium
Visibility:
Public.

Description

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}

Patch Set 1 #

Patch Set 2 : Also list wss next to https in comments next to ports. #

Patch Set 3 : Fix comment. #

Patch Set 4 : More grammar fixes. #

Total comments: 2

Patch Set 5 : Refactor scheme logic into IsEffectivePortAllowedByScheme(). #

Patch Set 6 : Unit test. #

Total comments: 3

Patch Set 7 : Add link to issue in comment next to port 443 on the (default) blocked list. #

Total comments: 5
Unified diffs Side-by-side diffs Delta from patch set Stats (+62 lines, -5 lines) Patch
M net/base/net_util.h View 1 2 3 4 1 chunk +9 lines, -0 lines 0 comments Download
M net/base/net_util.cc View 1 2 3 4 5 6 3 chunks +29 lines, -0 lines 5 comments Download
M net/http/http_stream_factory_impl_job.cc View 1 2 3 4 1 chunk +5 lines, -5 lines 0 comments Download
M net/url_request/url_request_unittest.cc View 1 2 3 4 5 1 chunk +19 lines, -0 lines 0 comments Download

Messages

Total messages: 33 (6 generated)
lgarron
4 years, 6 months ago (2014-12-02 19:34:17 UTC) #2
lgarron
On 2014/12/02 19:34:17, lgarron wrote: This should presumably have tests. Currently looking into it.
4 years, 6 months ago (2014-12-02 19:36:55 UTC) #3
lgarron
On 2014/12/02 19:36:55, lgarron wrote: > On 2014/12/02 19:34:17, lgarron wrote: > > This should ...
4 years, 6 months ago (2014-12-02 19:40:49 UTC) #4
lgarron
4 years, 6 months ago (2014-12-02 19:41:29 UTC) #5
davidben
https://codereview.chromium.org/770343003/diff/60001/net/http/http_stream_factory_impl_job.cc File net/http/http_stream_factory_impl_job.cc (right): https://codereview.chromium.org/770343003/diff/60001/net/http/http_stream_factory_impl_job.cc#newcode627 net/http/http_stream_factory_impl_job.cc:627: bool is_port_allowed = IsPortAllowedByDefault(origin_.port()); This was in the original, ...
4 years, 6 months ago (2014-12-02 20:39:45 UTC) #6
lgarron
https://codereview.chromium.org/770343003/diff/60001/net/http/http_stream_factory_impl_job.cc File net/http/http_stream_factory_impl_job.cc (right): https://codereview.chromium.org/770343003/diff/60001/net/http/http_stream_factory_impl_job.cc#newcode627 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 ...
4 years, 6 months ago (2014-12-02 20:49:00 UTC) #7
lgarron
On 2014/12/02 20:49:00, lgarron wrote: > https://codereview.chromium.org/770343003/diff/60001/net/http/http_stream_factory_impl_job.cc > File net/http/http_stream_factory_impl_job.cc (right): > > https://codereview.chromium.org/770343003/diff/60001/net/http/http_stream_factory_impl_job.cc#newcode627 > ...
4 years, 6 months ago (2014-12-02 22:47:42 UTC) #8
lgarron
4 years, 6 months ago (2014-12-02 22:55:23 UTC) #9
lgarron
Ready for a close-up.
4 years, 6 months ago (2014-12-03 00:24:31 UTC) #10
davidben
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#newcode110 net/base/net_util.cc:110: 443, // https / ...
4 years, 6 months ago (2014-12-03 20:16:11 UTC) #11
lgarron
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#newcode110 net/base/net_util.cc:110: 443, // https / wss On 2014/12/03 20:16:11, David ...
4 years, 6 months ago (2014-12-04 01:16:12 UTC) #12
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/770343003/120001
4 years, 6 months ago (2014-12-04 01:19:03 UTC) #14
commit-bot: I haz the power
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/builds/38855)
4 years, 6 months ago (2014-12-04 02:10:26 UTC) #16
lgarron
The trybots broke on three tests: HttpNetworkTransactionTest.DoNotUseSpdySessionForHttpOverTunnel HttpNetworkTransactionTest.DoNotUseSpdySessionForHttp HttpNetworkTransactionTest.UseSpdySessionForHttpWhenForced They were introduced in https://chromium.googlesource.com/chromium/src/+/8450d7292c94c2ef59b922b22c4c3694d19f6b1d%5E%21/net/http/http_network_transaction_spdy2_unittest.cc due ...
4 years, 6 months ago (2014-12-04 06:00:15 UTC) #17
Ryan Hamilton
On 2014/12/04 06:00:15, lgarron wrote: > The trybots broke on three tests: > > HttpNetworkTransactionTest.DoNotUseSpdySessionForHttpOverTunnel ...
4 years, 6 months ago (2014-12-04 19:58:21 UTC) #18
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/770343003/120001
4 years, 6 months ago (2014-12-05 01:08:10 UTC) #20
commit-bot: I haz the power
Committed patchset #7 (id:120001)
4 years, 6 months ago (2014-12-05 02:08:31 UTC) #21
commit-bot: I haz the power
Patchset 7 (id:??) landed as https://crrev.com/b6cf19c7b9dd536405c3c4f80876411733c9d5a5 Cr-Commit-Position: refs/heads/master@{#306959}
4 years, 6 months ago (2014-12-05 02:09:14 UTC) #22
PhistucK
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#newcode330 net/base/net_util.cc:330: int array_size = arraysize(kAllowedHttpsOrWssPorts); Just a drive by - ...
4 years, 6 months ago (2014-12-05 09:02:31 UTC) #24
mmenke
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#newcode127 net/base/net_util.cc:127: 995, // pop3+ssl Per discussion on the bug, wonder ...
4 years, 6 months ago (2014-12-05 21:44:40 UTC) #26
davidben
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#newcode127 net/base/net_util.cc:127: 995, // pop3+ssl On 2014/12/05 21:44:40, mmenke wrote: > ...
4 years, 6 months ago (2014-12-05 21:48:07 UTC) #27
mmenke
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#newcode127 net/base/net_util.cc:127: 995, // pop3+ssl On 2014/12/05 21:48:07, David Benjamin wrote: ...
4 years, 6 months ago (2014-12-05 21:54:29 UTC) #28
PhistucK
I know a lot of stuff that would break (though, a lot of them are ...
4 years, 6 months ago (2014-12-05 21:54:33 UTC) #29
mmenke
On 2014/12/05 21:54:33, PhistucK wrote: > I know a lot of stuff that would break ...
4 years, 6 months ago (2014-12-05 21:56:44 UTC) #30
mmenke
On 2014/12/05 21:56:44, mmenke wrote: > On 2014/12/05 21:54:33, PhistucK wrote: > > I know ...
4 years, 6 months ago (2014-12-05 21:57:39 UTC) #31
mmenke
On 2014/12/05 21:57:39, mmenke wrote: > On 2014/12/05 21:56:44, mmenke wrote: > > On 2014/12/05 ...
4 years, 6 months ago (2014-12-05 21:59:32 UTC) #32
lgarron
4 years, 6 months ago (2014-12-08 21:17:26 UTC) #33
Message was sent while issue was closed.
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.

Powered by Google App Engine
This is Rietveld 408576698