Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2865753003:
Stop on redirects while checking for www mismatches (Closed)
DescriptionStop on redirects while checking for www mismatches
SSLCommonNameMismatchHandling feature determines a suggested URL based on
the SubjectAlternateNames of the certificate and then pings this URL.
The suggested URL might be redirecting to an HTTP URL, leaking the path
of the original HTTPS URL.
This change prevents suggested URL pings from following redirects. This will
prevent leaking the path of HTTPS URLs at the expense of false negatives.
E.g. foo.com serves a cert for www.foo.com. www.foo.com redirects to bar.com.
This won't work anymore.
BUG=718676
Review-Url: https://codereview.chromium.org/2865753003
Cr-Commit-Position: refs/heads/master@{#481299}
Committed: https://chromium.googlesource.com/chromium/src/+/0b201e50b370d8269aa52aa4fddbcf323241b07f
Patch Set 1 #
Total comments: 4
Patch Set 2 : Add test, address elawrence comments #Patch Set 3 : Adjust comments #
Total comments: 4
Patch Set 4 : elawrence comments #Patch Set 5 : Fix tests #Patch Set 6 : Convert CHECK(false) to EXPECT_TRUE(false) #Patch Set 7 : Rebase #
Messages
Total messages: 26 (14 generated)
elawrence@chromium.org changed reviewers: + elawrence@chromium.org
https://codereview.chromium.org/2865753003/diff/1/chrome/browser/ssl/common_n... File chrome/browser/ssl/common_name_mismatch_handler.cc (right): https://codereview.chromium.org/2865753003/diff/1/chrome/browser/ssl/common_n... chrome/browser/ssl/common_name_mismatch_handler.cc:79: // Don't follow redirects to prevent ending up on non-HTTPS sites. Maybe: Don't follow redirects to prevent leaking URL data to HTTP sites. https://codereview.chromium.org/2865753003/diff/1/chrome/browser/ssl/common_n... chrome/browser/ssl/common_name_mismatch_handler.cc:127: landing_url.host() == suggested_url.host()) { Is there a reason that we need to hostname to be the same? If https://foo.com gives a cert for https://www.foo.com and pulling https://www.foo.com 301 redirects to https://auth.foo.com, shouldn't that be allowed? This is the pattern for Live.com and Hotmail.com today. (Having said that, with redirects disabled, this seems like an moot question).
Patchset #3 (id:40001) has been deleted
Description was changed from ========== Stop on redirects while checking for www mismatches BUG= ========== to ========== Stop on redirects while checking for www mismatches SSLCommonNameMismatchHandling feature determines a suggested URL based on the SubjectAlternateNames of the certificate and then pings this URL. The suggested URL might be redirecting to an HTTP URL, leaking the path of the original HTTPS URL. This change prevents suggested URL pings from following redirects. This will prevent leaking the path of HTTPS URLs at the expense of false negatives. E.g. foo.com serves a cert for www.foo.com. www.foo.com redirects to bar.com. This won't work anymore. BUG=718676 ==========
elawrence, PTAL? Thanks! https://codereview.chromium.org/2865753003/diff/1/chrome/browser/ssl/common_n... File chrome/browser/ssl/common_name_mismatch_handler.cc (right): https://codereview.chromium.org/2865753003/diff/1/chrome/browser/ssl/common_n... chrome/browser/ssl/common_name_mismatch_handler.cc:79: // Don't follow redirects to prevent ending up on non-HTTPS sites. On 2017/05/08 18:06:12, elawrence wrote: > Maybe: > > Don't follow redirects to prevent leaking URL data to HTTP sites. Done. https://codereview.chromium.org/2865753003/diff/1/chrome/browser/ssl/common_n... chrome/browser/ssl/common_name_mismatch_handler.cc:127: landing_url.host() == suggested_url.host()) { On 2017/05/08 18:06:12, elawrence wrote: > Is there a reason that we need to hostname to be the same? > > If https://foo.com gives a cert for https://www.foo.com and pulling > https://www.foo.com 301 redirects to https://auth.foo.com, shouldn't that be > allowed? This is the pattern for http://Live.com and http://Hotmail.com today. > > (Having said that, with redirects disabled, this seems like an moot question). Right, when redirects are disabled, this is only a sanity check. Converted to a DCHECK. As we discussed in the bug, live.com case will become a false negative with this change.
Thanks for working on this! The code overall LGTM, although I do worry about how big of an impact disabling cross-host HTTPS redirects might have. Two simple questions. https://codereview.chromium.org/2865753003/diff/60001/chrome/browser/ssl/ssl_... File chrome/browser/ssl/ssl_browser_tests.cc (right): https://codereview.chromium.org/2865753003/diff/60001/chrome/browser/ssl/ssl_... chrome/browser/ssl/ssl_browser_tests.cc:3589: // Ignore favicon requests. As someone new to this code, it would be nice if the comment explained why ignoring favicon requests is desirable. https://codereview.chromium.org/2865753003/diff/60001/chrome/browser/ssl/ssl_... chrome/browser/ssl/ssl_browser_tests.cc:3593: CHECK(request->url().SchemeIsCryptographic()) Can this check ever pass? Below, AddHostnameInterceptor is always called with the HTTP scheme.
Thanks! The current number of errors fixed by this feature is already small, so if this drops the number even further we might want to do something more clever like recursively pinging as discussed in the bug. https://codereview.chromium.org/2865753003/diff/60001/chrome/browser/ssl/ssl_... File chrome/browser/ssl/ssl_browser_tests.cc (right): https://codereview.chromium.org/2865753003/diff/60001/chrome/browser/ssl/ssl_... chrome/browser/ssl/ssl_browser_tests.cc:3589: // Ignore favicon requests. On 2017/06/03 18:45:11, elawrence wrote: > As someone new to this code, it would be nice if the comment explained why > ignoring favicon requests is desirable. Done. https://codereview.chromium.org/2865753003/diff/60001/chrome/browser/ssl/ssl_... chrome/browser/ssl/ssl_browser_tests.cc:3593: CHECK(request->url().SchemeIsCryptographic()) On 2017/06/03 18:45:11, elawrence wrote: > Can this check ever pass? Below, AddHostnameInterceptor is always called with > the HTTP scheme. No, it can never pass. Converted to CHECK(false).
The CQ bit was checked by meacer@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from elawrence@chromium.org Link to the patchset: https://codereview.chromium.org/2865753003/#ps80001 (title: "elawrence comments")
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
Fixed the tests, landing soon.
A minor change, landing now.
The CQ bit was checked by meacer@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from elawrence@chromium.org Link to the patchset: https://codereview.chromium.org/2865753003/#ps120001 (title: "Convert CHECK(false) to EXPECT_TRUE(false)")
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: win_chromium_compile_dbg_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...)
The CQ bit was checked by meacer@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from elawrence@chromium.org Link to the patchset: https://codereview.chromium.org/2865753003/#ps140001 (title: "Rebase")
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 140001, "attempt_start_ts": 1498071225282510,
"parent_rev": "2d9d6aaf08a2d59da6085329ff4de12315d1a583", "commit_rev":
"6ad98145d0666c2761295867fffcfe89bb584eb2"}
CQ is committing da patch.
Bot data: {"patchset_id": 140001, "attempt_start_ts": 1498071225282510,
"parent_rev": "9a050d0ecfd81a23517ebcdc80789bc43bd6fa10", "commit_rev":
"0b201e50b370d8269aa52aa4fddbcf323241b07f"}
Message was sent while issue was closed.
Description was changed from ========== Stop on redirects while checking for www mismatches SSLCommonNameMismatchHandling feature determines a suggested URL based on the SubjectAlternateNames of the certificate and then pings this URL. The suggested URL might be redirecting to an HTTP URL, leaking the path of the original HTTPS URL. This change prevents suggested URL pings from following redirects. This will prevent leaking the path of HTTPS URLs at the expense of false negatives. E.g. foo.com serves a cert for www.foo.com. www.foo.com redirects to bar.com. This won't work anymore. BUG=718676 ========== to ========== Stop on redirects while checking for www mismatches SSLCommonNameMismatchHandling feature determines a suggested URL based on the SubjectAlternateNames of the certificate and then pings this URL. The suggested URL might be redirecting to an HTTP URL, leaking the path of the original HTTPS URL. This change prevents suggested URL pings from following redirects. This will prevent leaking the path of HTTPS URLs at the expense of false negatives. E.g. foo.com serves a cert for www.foo.com. www.foo.com redirects to bar.com. This won't work anymore. BUG=718676 Review-Url: https://codereview.chromium.org/2865753003 Cr-Commit-Position: refs/heads/master@{#481299} Committed: https://chromium.googlesource.com/chromium/src/+/0b201e50b370d8269aa52aa4fddb... ==========
Message was sent while issue was closed.
Committed patchset #7 (id:140001) as https://chromium.googlesource.com/chromium/src/+/0b201e50b370d8269aa52aa4fddb... |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
