One possible fix is: only ping the hostname, not the full URL. This prevents leaking the path. There is a comment in CommonNameMismatchHandler::GetSuggestedUrl: "The full URL should be pinged, not just the new hostname." But doesn't give any reason.

Also in CommonNameMismatchHandler::OnURLFetchComplete, we need to check whether landing_url and suggested_url have the same domain. This prevents MITM from redirecting users to other domains.

Another possible fix is: stop redirecting whenever the next redirect target is not HTTPS. But I am not sure if this is easy to implement or not.

Interesting, thanks for the report! The exploit scenario is indeed very narrowly scoped, but it seems worthwhile to see if we can do more here. I think the approach of refusing to follow a cross-origin redirect on the ping probably makes sense.

Re #2: I think the argument for pinging the full URL is that a HTTPS server might end up redirecting to some other, non-secure page based on the contents of the URL it receives. Therefore pinging the stripped origin might result in a false negative. (That said, a server might well refuse a HEAD request with a HTTP/405, and that results in a false negative today).

The simple approach to fixing this would be to set:

  url_fetcher_->SetStopOnRedirect(true);

This will prevent all redirections (and data leaks) because we'll end with a 3xx response code and treat our URL fixup guess as invalid. This will create the possibility of more false-negatives (say, if the server redirects to itself by adding a trailing slash to the URL) but it would be simple.

It seems like there has to be some way to do a redirect filter (e.g. for blocking redirects that result in mixed content?) but I couldn't find one on the URLFetcher.

Severity-Low because of the very limited scope here.

meacer, assigning to you :)

(assuming that iOS is impacted too so OS-All. Please correct me if I'm wrong)

xiaoyin.l: Thanks for the report, and
elawrence: Thanks for investigating.

The more complicated alternative is to dump URLFetcher altogether and send url requests and handle redirects on our own, but I don't think it's worth the effort  given the low number of errors this feature fixes. SetStopOnRedirect seems sufficient to me.

Re #6: It occurs to me that one false negative we'll introduce if we ban redirections altogether is that https://hotmail.com will stop working in Chrome.

Would it be crazy to add a SetStopOnNonSecureRedirect() flag to URLFetcher?

Thank you for investigating!

Re #6, 7: An alternative way I can think of is: can we call URLFetcher recursively in the OnURLFetchComplete handler? If it is possible, we can call url_fetcher_->SetStopOnRedirect(true); and in OnURLFetchComplete, we check if the redirect destination is HTTPS. If it is HTTPS, we fetch the new URL, and this covers the hotmail.com case; if it is not HTTPS, we return error.

elawrence: By the way, this issue was inspired by your Hotmail tweet you posted yesterday. Thank you for your tweet!

> Would it be crazy to add a SetStopOnNonSecureRedirect() flag to URLFetcher?

Sounds reasonable to me, but not sure what net/ will say :)

Re #8: Recursive url fetches should also work, and we'd be able to filter by origin et.c. as well if we do that. We'll need to limit the number of hops though.

However, it would be non trivial work and I'm not sure it'll be justified given the low usage of this feature. I plan to explore making this feature more effective and perhaps we can do it at that time.

For now I'll simply do the SetStopOnRedirect fix.

I noticed that we actually do check for redirects by expecting the response code to be HTTP 200 in CommonNameMismatchHandler::OnURLFetchComplete, so I'm trying to understand under which circumstances we would get a redirect without HTTP 200. Is Fiddler's autoredirect doing this in the original report?

My understanding is that when SetStopOnRedirect is not set, the URLFetcher automatically chases redirects until a final response (non-3xx status) is received. Only at that point is our OnURLFetchComplete callback invoked, at which point (in the bad case) we've already potentially leaked URL data to a non-secure redirect Location. 

Well, yes that makes sense :)

I have an incomplete CL here: https://codereview.chromium.org/2865753003

The test doesn't work correctly as we get responsecode == -1 for redirects, and show an interstitial. I won't be able to get to this until June, so if anyone wants to take it, feel free :)

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/0b201e50b370d8269aa52aa4fddbcf323241b07f

commit 0b201e50b370d8269aa52aa4fddbcf323241b07f
Author: meacer <meacer@chromium.org>
Date: Wed Jun 21 21:10:02 2017

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}

[modify] https://crrev.com/0b201e50b370d8269aa52aa4fddbcf323241b07f/chrome/browser/ssl/common_name_mismatch_handler.cc
[modify] https://crrev.com/0b201e50b370d8269aa52aa4fddbcf323241b07f/chrome/browser/ssl/ssl_browser_tests.cc

Bit late, but should be fixed now.

Will this bug get a CVE?

+awhalley for the CVE question. It might make sense to outline the criteria in https://chromium.googlesource.com/chromium/src/+/master/docs/security/faq.md, as this topic comes up semi-regularly.

Yep, this will get a CVE - we issue them when the fix is included in a stable release. For M61 that'll be early September.

Thank you! Also, does this issue qualify for a bug bounty?

Re: #21 - Adding reward-topanel for consideration. 

The scope of the attack is so limited that it was rated Severity Low and as such likely will be below the bar.

I'm afraid the VRP panel declined to award, but many thanks for the report!

I see. Thank you for the fix!

This bug has been closed for more than 14 weeks. Removing security view restrictions.

For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot