|
|
DescriptionBlock 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
Messages
Total messages: 33 (6 generated)
lgarron@chromium.org changed reviewers: + davidben@chromium.org
On 2014/12/02 19:34:17, lgarron wrote: This should presumably have tests. Currently looking into it.
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)?
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.
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(...)?
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.
Ready for a close-up.
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.
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.
The CQ bit was checked by lgarron@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/770343003/120001
The CQ bit was unchecked by commit-bot@chromium.org
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...)
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
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!
The CQ bit was checked by lgarron@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/770343003/120001
Message was sent while issue was closed.
Committed patchset #7 (id:120001)
Message was sent while issue was closed.
Patchset 7 (id:??) landed as https://crrev.com/b6cf19c7b9dd536405c3c4f80876411733c9d5a5 Cr-Commit-Position: refs/heads/master@{#306959}
Message was sent while issue was closed.
phistuck@gmail.com changed reviewers: + phistuck@gmail.com
Message was sent while issue was closed.
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?
Message was sent while issue was closed.
mmenke@chromium.org changed reviewers: + mmenke@chromium.org
Message was sent while issue was closed.
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.
Message was sent while issue was closed.
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/.
Message was sent while issue was closed.
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. :)
Message was sent while issue was closed.
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.
Message was sent while issue was closed.
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.
Message was sent while issue was closed.
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"
Message was sent while issue was closed.
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.
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. |