1670672 - Never render redirect body (Controllable 302 to a ws://, wss://, or resource:// scheme executes an XSS payload)

Status: NEW

(Core :: DOM: Navigation, defect, P3)

Description

[Tom Ritter [:tjr]]

As described in https://www.gremwell.com/firefox-xss-302 if the body of a 302 redirect contains an HTML payload, it will be rendered if the Location header is to a ws://, wss://, or resource:// URI.

Compared to other browsers, Firefox is unique in this behavior, although Chrome will render it if the Location header is empty.

(I'm conjecturing about 'render the HTML payload', the example uses <script>alert()</script> and that executes)

Comment 1

[Tom Ritter [:tjr]]

Tagging a couple of people to get the ball rolling w.r.t. what should happen/how to fix it...

Comment 2

[Anne (:annevk)]

For navigation step 10-13 of https://html.spec.whatwg.org/#process-a-navigate-fetch handle this. If fetched via other means step 4 of https://fetch.spec.whatwg.org/#concept-http-redirect-fetch handles this.

Since it's about executing script I guess this is the former. In principle what should happen is that https://html.spec.whatwg.org/#process-a-navigate-url-scheme runs and we display an error message since nobody gets to take over these schemes. (I think that for standardized schemes such as ws and wss we can make that a bit more official though and I can do once we make this public.)

(DOM: Navigation might be a more appropriate component.)

Comment 3

[Daniel Veditz [:dveditz]]

When does https://fetch.spec.whatwg.org/#concept-http-redirect-fetch kick in? That one says if the Location is not a HTTP(S) scheme then return a network error.

In any case I don't think we should ever run scripts on these pages if we show them.

Comment 4

[Anne (:annevk)]

Daniel, when <img>, fetch(), background-image et al see a redirect.

Comment 5

[:Gijs (he/him)]

bug 1052887 is potentially related (not linking because "see also" links are always public, unlike depends/blocks, for some reason)

I don't have any great insight here, unfortunately. If "resource" works, does "chrome" work too? What about "file" ?

The whole thing is kinda weird - I would have expected this to be treated the same as redirecting to mailto:, or any protocol the website is not allowed to access - if the content is rendering that seems like a DOM problem rather than a frontend one, so I'm not the best person to advise. Either an error page or about:blank (depending on what the spec says) seems better. Could try :kmag or :nika or :ckerschb if this ends up stuck.

Comment 6

[Daniel Veditz [:dveditz]]

The idea of rendering the content was (I think!) that if the UA didn't understand the Location: header maybe a human could make something out of the body. But any content there is even more likely to run around a web site's security efforts, worse than 404 pages, and we should just never render them, ever. Let people view-source it if they need details. In the vast vast majority of web servers these days there is no body.

Comment 7

[Daniel Veditz [:dveditz]]

The initial report is public so this bug can be, too. A handful of open redirects I tested on big sites filter out non-http(s) schemes so hopefully this isn't a widespread risk for most users. Still is a problem for targeted attacks against vulnerable servers.

Comment 8

[Olli Pettay [:smaug][[e-mail]]]

Perhaps we could change 302 handling in necko and have consistent behavior then everywhere.

Comment 9

[Anne (:annevk)]

Correct me if I'm wrong, but as I identified in comment 2 this is not a networking issue as navigation handles redirects itself. And what the HTML Standard says here is what we should do I think. Act as if there was a network error.

Comment 10

[Nika Layzell [:nika] (ni? for response)]

(In reply to Anne (:annevk) from comment #9)

Correct me if I'm wrong, but as I identified in comment 2 this is not a networking issue as navigation handles redirects itself. And what the HTML Standard says here is what we should do I think. Act as if there was a network error.

If we do this in necko, then we don't need to pretend that it's a network error, we can have the 302 with a non-http(s) location header actually be a network error, which seems preferable. We also don't really handle HTTP redirects in navigation separately, we handle meta refresh tags manually, but normal HTTP redirects are handled by necko just like how we handle other redirects. I could be misunderstanding what https://fetch.spec.whatwg.org/#concept-http-redirect-fetch is saying, but it seems like it should be applying to all network requests, not just navigations.

Comment 11

[Anne (:annevk)]

HTML's navigate invokes fetch with a request whose redirect mode is "manual": https://html.spec.whatwg.org/#process-a-navigate-fetch. This means it doesn't hit https://fetch.spec.whatwg.org/#concept-http-redirect-fetch directly. It only does so for redirects to HTTP(S) schemes.

The reason HTML's navigate does this is because each individual fetch might have to be handled by a different service worker. So if you have a redirect chain of A -> B -> C, each of A, B, and C might have to run through a registered service worker.

The other reason HTML's navigate handles redirects manually is because it needs to handle some non-HTTP(S) schemes in the Location header and dispatch those appropriately. E.g., a redirect to mailto: or zoommtg: "succeeds". Whereas in fetch (and therefore for <img> and such) that would be a network error. (To elaborate more, fetch safelists HTTP(S) schemes, whereas HTML essentially allows most schemes, and blocklists some of them, including ws: and wss:.)

Perhaps this setup could be revamped in a way whereby fetch takes more responsibility, which I take it from your comment is how Gecko handles it, but it would still have special cases for navigation.

Hope that helps.

Comment 13

[Tom Schuster]

https://treeherder.mozilla.org/jobs?repo=try&revision=5baa79bff5e2c4191a11db03aaa94553d783fbf4

Comment 14

[Tom Schuster]

Seems like just always showing an error message when a redirect fails doesn't work, e.g. for the GetDontFollowRedirects case we apparently shouldn't do this. I am going to file a dependent bug for a minimal fix, that should be uncontroversial to land from my point of view..