1171203 - http redirect and hsts can loop

Status: RESOLVED FIXED

(Core :: Networking: HTTP, defect)

Description

[Ralph Giles (:rillian)]

If a page serves a 301 redirect from https to itself on http, but also sends an HSTS header, we can loop forever, redirecting from one version of the page to another.

E.g. www.microsoft.com/web/downloads/platform.aspx

Via https://twitter.com/ericlaw/status/606209561855287296

Comment 1

[Ralph Giles (:rillian)]

I think it would be better if we detected such redirect loops and showed an error page instead.

Comment 2

[Ralph Giles (:rillian)]

Hmm. Going back to the tab, I see we do detect redirect loops and show an error page. Maybe the bug is just that we don't stop the tab spinner. It still says 'Connecting' while the content frame says:

  The page isn't redirecting properly

  Firefox has detected that the server is redirecting the request for this address in a way that will never complete.

      This problem can sometimes be caused by disabling or refusing to accept cookies.

Comment 3

[Patrick McManus [:mcmanus]]

wfm.. I get the error page and no spinner.. judging from the tweet comments that's roughly chrome's behavior as well.

Comment 4

[Dave Garrett]

Any redirect from HTTPS with HSTS to the same domain via HTTP should probably be treated as a security error with a message to the effect of:

"This site has requested secure connections only, however it is misconfigured and attempting to force itself to load insecurely instead."

The current message tells people to fiddle with their cookie settings which won't help and could just confuse them.

Comment 5

[Honza Bambas (:mayhemer)]

Confirming that we go here through the standard redirect logic and allow only a limited (20?) redirects in a single chain.  Then we end up with an error page.

This is probably just about introducing a new error code and a new error page.  I don't know from top of my head about anything existing.

Comment 6

[jose.kahan]

Hi, I confirm that this is a real issue and results in both server DOS risk and also in the browser's local resource consumption.

Here's how to reproduce it. Tested with firefox 39.0 on linux/32

Setup:

1) Have a URI that sends back the HSTS header. 
For simplicity, let's assume this is https://example.com/

In my particular setup, if you browse this URL, the reply will include these two
headers:

  Strict-Transport-Security: max-age=86400; includeSubdomains; preload
  Content-Security-Policy: upgrade-insecure-requests

2) Have another URI that sends a redirect to HTTP if you're browsing it from HTTPS
Let's call it http://example.com/redirect_test/

I set this one up with apache with this .htaccess (adapt as needed)

[[
RewriteEngine on
RewriteCondition RewriteCond %{HTTPS} on
RewriteRule .*  http://example.com/redirect_test/ [L]
]]

Test scenario:

1) Browse https://example.com/
   --> This will set up both the HSTS and CSP for that domain

2) Browse http://example.com/redirect_test/ <- notice we're using http, not https

-> The browser will intercept our request and send it as https.
-> The server will reply with a 302 to http://example.com/redirect_test/
   In my particular setup, the 302 includes the following two headers described in 1.
-> The browser will override the redirect to http and try again https://example.com/redirect_test

This results in an infinite loop that floods the server with requests and starts using too much local resources on the box where the browser is running. In the few seconds I ran it, I got 1400 hits on the test server, all sent thru HTTPS. On the local box, firefox went from 1% CPU use to 114% before "stabilizing" at around 70%, with churning sounds from the HDD.

The same test on google-chrome results in an infinite loop being correctly detected.

Feel free to contact me if you needed further info or want to try to reproduce this against our test server.

-j

Comment 7

[Honza Bambas (:mayhemer)]

This turns out to be a really funny bug :)  Because we do two redirects (one artificial and other is handling of 302), the value of the redirect countdown in nsHttpChannel::AsyncProcessRedirection never hits '0' - it's always an odd number and we simply underflow the counter to 255 (there is a min() to crop) sooner that there is a chance to do the check for '== 0'.  Hence, we loop forever.

Two things to fix here: 
- make the counter unable to underflow
- introduce a better error message (I'll file a followup for that)

Comment 8

[Honza Bambas (:mayhemer)]

Attachment: v1 (underflow fix, internal redirects not counted)

- underflow check
- internal redirects excluded from the countdown ; I think we should not bind internal redirects by this limit, we use it for proxy fail over that is limited by the list of available proxies and also for various other stuff like retry after cache read failure etc

Comment 9

[Honza Bambas (:mayhemer)]

Comment on attachment 8776008 [details] [diff] [review]
v1 (underflow fix, internal redirects not counted)

https://treeherder.mozilla.org/#/jobs?repo=try&revision=6f487089377e

Comment 10

[Patrick McManus [:mcmanus]]

Comment on attachment 8776008 [details] [diff] [review]
v1 (underflow fix, internal redirects not counted)

Review of attachment 8776008 [details] [diff] [review]:
-----------------------------------------------------------------

I really do think redirect loops should terminate, even with internal redirects. Infinite loops are horrible failure modes.

Comment 11

[Honza Bambas (:mayhemer)]

Attachment: v2 (underflow check)

True is that the v1 patch opens a chance for an indefinite loop by internal redirs.  I was thinking of having a different limit for internals and normal redirects, but that would be an overkill.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=85dd7971c0c6

Comment 12

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/872c3c931fab
Fix redirection limit underflow, don't count internal redirect to this limit, r=mcmanus

Comment 13

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/872c3c931fab