1900212 - onworks.net - Search broken when "HTTPS-Only Mode" is active

Status: RESOLVED DUPLICATE of bug 1747230

(Core :: DOM: Security, defect)

Description

[08xjcec48]

Steps to reproduce:

• Check Enable HTTPS-Only Mode in all windows in your settings
• Open https://www.onworks.net/
• Use the search bar

Actual results:

The page isn’t redirecting properly. Clicking on Try Again loads the results.

Comment 1

[08xjcec48]

Please their other domain too: https://www.offidocs.com/.

Comment 2

[Daniel Veditz [:dveditz]]

Quick drive-by:
Search on the site does a POST to https://www.onworks.net/component/search/
that returns a 303 to http://www.onworks.net/component/search/?searchword=626c6f626279&searchphrase=all

If you load that as http: the site gives you a 301 to the same URL but https://, which loads fine.

In HTTPs-only mode we load that directly as the https:// form, essentially skipping the initial site redirect. For some reason we then get a redirect-loop error. Why don't these intermediate redirects show up in DevTools? If the site is slow do we have a fallback http: probe, which seeing it redirect to the thing we thought was slow looks like a loop? But I thought in https-"ONLY" we would never make an insecure probe without the user OK'ing the fallback. Otherwise it's not a safe replacement for EFF's Privacy Badger.

If I load http://www.onworks.net/component/search/?searchword=626c6f626279&searchphrase=all at the top-level with HTTPS-Only on I don't have any problems (this probably mirrors the reporter's "Try Again" success). The problem seems to be because this navigation starts as a redirect. It also changes method from POST to GET, but that's common, right? In fact way more common than 307. And 302 is super common and should be the same as 303. (why isn't the site using a permanent 301 redirect? because of the parameters that expire?)

My actual search term got turned into that "searchword" code that has been working while I play with it, but I don't know if that's a dictionary reference or more of a session token kind of thing that will expire

Comment 3

[Malte Jürgens [:maltejur]]

What seems to be happening here: POST https://www.onworks.net/component/search/ is redirecting to GET http://www.onworks.net/component/search/?{the post body as query parameters}. Our HTTPS-Only code currently detects redirect loops by taking a look at the redirect chain. That redirect chain is also stored in the content processes, and because of that the query parameters are trimmed of there (that was introduced in Bug 1715785). That means it sees a redirect from https://www.onworks.net/component/search/ to http://www.onworks.net/component/search/, so the exact same path on the same host, but with a http instead of a https scheme. HTTPS-Only then assumes that upgrading and changing the scheme back to https is useless, as it would that would get redirected to a http scheme again and land in a loop. This is not the case of course, as HTTPS-Only doesn't factor in the different query parameters and the different request type.

HTTPS-Only not factoring in query parameters is a known issue (Bug 1747230) that :manuel is actively working on. With the almost finished patch for that bug, this bug is not reproducible for me anymore. So once that patch lands, this bug should be fixed.

But I think there is another bug here. I would expect to see the "Secure Site Not Available" error page in the case we detect a HTTPS-Only redirect loop, with the option to "Continue to HTTP Site". But instead, we are getting the "The page isn’t redirecting properly" error page. It seems like the loop detection cancels the channel, but other HTTPS-Only code called later to display the correct error page doesn't feel responsible for that canceled channel. I will look into this a bit further, but this problem may also change/go away with Manuels patch.

Comment 4

[Daniel Veditz [:dveditz]]

Apart from the query parameter issue, our redirect-loop-detection should definitely take the method into account. Servers can give different responses to different methods (even with empty bodies, and not just POST/GET). In the POST/GET case the response is likely the same on most servers, but there's no harm letting it go one more redirect when the methods don't match before detecting the loop just in case it's one of the quirky servers.

We might get away ignoring the difference between POST and GET most of the time (not this time 😀), but if it's anything else that's definitely going to cause problems (but will be extremely rare).

Comment 5

[Malte Jürgens [:maltejur]]

Apart from the query parameter issue, our redirect-loop-detection should definitely take the method into account

Yes, that is a good point. I think I will write a small testcase where this would be a problem and then open a bug for that. Maybe Manuel can also integrate checking the method directly into his patch.

If the site is slow do we have a fallback http: probe, which seeing it redirect to the thing we thought was slow looks like a loop? But I thought in https-"ONLY" we would never make an insecure probe without the user OK'ing the fallback. Otherwise it's not a safe replacement for EFF's Privacy Badger.

That is unrelated to this bug, but is indeed generally done after 3s. We only do a GET request to the hostname though, the path and query are not included there. So there is not a lot of information that will "leak" because of that request, but the hostname still will. I think I agree with you that that is not ideal behavior. But the alternative would be to either let the https request time out, or display the warning directly after 3s, even if the site may just be slow and respond after 4s on both http and https. Christoph, do you have some more context on this?

Comment 6

[Christoph Kerschbaumer [:ckerschb]]

That is unrelated to this bug, but is indeed generally done after 3s. We only do a GET request to the hostname though, the path and query are not included there. So there is not a lot of information that will "leak" because of that request, but the hostname still will. I think I agree with you that that is not ideal behavior. But the alternative would be to either let the https request time out, or display the warning directly after 3s, even if the site may just be slow and respond after 4s on both http and https. Christoph, do you have some more context on this?

Back when we implemented https-only mode we encountred that some sites do sit behind a firefwall and do not respond to https requests at all resulting in ~90 second timeouts which drastically downgrades an end users experience.

We then empirically came up with the idea to send an http background requests after 3,000ms where we strip any kind of query and path information as well as all kinds of cookies and what not, just to figure out if the server responds. If that background request returns a signal faster then the upgraded https request, then we take that as a strong indicator that the upgraded https request will result in a time-out and we cancel the request and show the interstitial page.

Please note that the majority of upgraded https requests return a signal before the 3,000ms so we don't even send the http background request. If that is not the case, then it's true that we leak the domain name to the network because of the http request.

Please further note that it's also possible that a OCSP request is issued in which case we would leak the domain name to the network also/anyway.

Comment 7

[Malte Jürgens [:maltejur]]

I will dupe this against Bug 1747230, as that is the core of the issue here. The patch attached to that bug will land soon and fix this issue.

For the HTTP request method discussion, I have opened Bug 1901498.