255119 - Don't show redirect content after denying automatic redirect to javascript: URL

Status: RESOLVED FIXED

(Core :: Networking: HTTP, enhancement)

Description

[Jesse Ruderman]

Mozilla disallows redirects to javascript: URLs, but it displays the page that
comes along with the redirect, which is likely to contain a javascript: link. 
We should not show this page, because it's unlikely to be useful except for
exploiting XSS holes.  IE and Opera both give strange error messages instead of
showing the page that comes along with the redirect.

Examples:
http://www.squarefree.com/redirect/
http://www.stuttgart-tourist.de/redirect.cgi?javascript:alert(5)

Comment 1

[Darin Fisher]

Should we do the same for all blocked redirects?  Or, should we conditionalize
it on the URL scheme (i.e., only do it for javascript:)?

Comment 2

[Jesse Ruderman]

What other redirects do we block?

Comment 3

[Christian :Biesinger (don't email me, ping me on IRC)]

interesting, I stumbled on this piece of code recently.

it seems to be this place here:
http://lxr.mozilla.org/seamonkey/source/netwerk/protocol/http/src/nsHttpChannel.cpp#729
(the else-branch)

Contrarily, it seems that for cached redirects, the page isn't displayed:
http://lxr.mozilla.org/seamonkey/source/netwerk/protocol/http/src/nsHttpChannel.cpp#400

Any reason for this discrepancy?

> What other redirects do we block?

"any for which CheckLoadURI fails". (or the http event sink)

Comment 4

[Jesse Ruderman]

Fixing this for data: URLs without also fixing bug 211999 would break the current version of Hixie's data URI kitchen <http://software.hixie.ch/utilities/cgi/data/data>.

Comment 5

[Christian :Biesinger (don't email me, ping me on IRC)]

Attachment: so is this the desired behaviour?

Comment 6

[Darin Fisher]

Hmm... doesn't this cause problems for 300 responses that lack a Location header?  Also seems like a problem for 307 POSTs that get redirected and result in a prompt.  The page sent with a redirect response may be intended to be shown to the user in some cases.  At least, that seems to be the intent of HTTP/1.1.

Comment 7

[Darin Fisher]

Comment on attachment 210969 [details] [diff] [review]
so is this the desired behaviour? 

marking review- based on previous comment.

Comment 8

[Josh Aas]

Christian - since you haven't touched this in a while I'm reassigning to nobody so that we can easily tell we need a new owner. Let me know if you still want to work on it.

Comment 9

[Patrick McManus [:mcmanus]]

safari has a special error page for this ('javascript cannot be used in this way').. but chrome just cancels the request (i.e. shows nothing new at all and gives you no feedback there was an error).. I don't have ie handy to check, but it looks like this is such an edge case we don't need to do anything fancy in handling it as long as we meet the requirement not to display the response body in this case.

Comment 10

[Patrick McManus [:mcmanus]]

Attachment: patch 1

detects DOM_BAD_URI error, which currently just aborts the redirection and not the original transaction, and replaces it with CORRUPTED_CONTENT - which is the error page we use for header response smuggling and other cases where the HTTP response isn't acceptable to process. It could be made fancier, but this is a real edge case certainly aligned with ill intent, so I think this is fine.

Previous patch to this bug was rejected because it cleared the page for all redirections where we could not follow the location header (perhaps there was no such header, or perhaps it couldn't be reached) and that was too broad. This restricts itself to cases where we _choose_ not to chase the location header because of policy.

Comment 11

[Mozilla RelEng Bot]

Try run for f56633230ec2 is complete.
Detailed breakdown of the results available here:
    https://tbpl.mozilla.org/?tree=Try&rev=f56633230ec2
Results (out of 39 total builds):
    success: 34
    warnings: 5
Builds available at http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/[e-mail]-f56633230ec2

Comment 12

[Patrick McManus [:mcmanus]]

Comment on attachment 560670 [details] [diff] [review]
patch 1

phhhhbbbbt. (try fail/leak).

Comment 13

[Mozilla RelEng Bot]

Try run for 69f7a4ca99a7 is complete.
Detailed breakdown of the results available here:
    https://tbpl.mozilla.org/?tree=Try&rev=69f7a4ca99a7
Results (out of 3 total builds):
    exception: 1
    failure: 2
Builds available at http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/[e-mail]-69f7a4ca99a7

Comment 14

[Mozilla RelEng Bot]

Try run for aee95dffd411 is complete.
Detailed breakdown of the results available here:
    https://tbpl.mozilla.org/?tree=Try&rev=aee95dffd411
Results (out of 41 total builds):
    success: 39
    warnings: 2
Builds available at http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/[e-mail]-aee95dffd411

Comment 15

[Patrick McManus [:mcmanus]]

Attachment: patch 2

comment 14 is a valid try run (modulo unrelated random orange).

This version restricts the body suppression to redirects rejected for bad-uri reasons that are trying to change the protocol to something non-http.

Comment 16

[Honza Bambas (:mayhemer)]

Comment on attachment 561183 [details] [diff] [review]
patch 2

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

Nice solution.

BTW, this also affects loading file:/// and actually any local or resource URI, probably not an issue, actually a nice benefit?

Some |bool NS_SchemeIs(nsIChannel* channel, const char* scheme)| function would be useful...  Feel free to introduce one in this patch, based also on comments bellow.  I can see heavy-looking checks for multiple schemes on lot of other places.

r=honzab with just few nits.

::: netwerk/protocol/http/nsHttpChannel.cpp
@@ +1070,5 @@
> +    if (rv == NS_ERROR_DOM_BAD_URI && mRedirectURI) {
> +        PRBool isHTTP = PR_FALSE;
> +        mRedirectURI->SchemeIs("http", &isHTTP);
> +        if (!isHTTP)
> +            mRedirectURI->SchemeIs("https", &isHTTP);

You should check for the result and fall back to FALSE on failure.

@@ +1076,5 @@
> +        if (!isHTTP) {
> +            // This was a blocked attempt to redirect to subvert the system and 
> +            // redirect to another protocol (perhaps javascript:)
> +            // In that case we want to throw an error instead of displaying the
> +            // non-redirected response body. see bug 255119.

bz doesn't like referring bugs.  I didn't ask him why, but please remove the reference.

::: netwerk/test/unit/test_nojsredir.js
@@ +17,5 @@
> +function setupChannel(url) {
> +    var ios = Components.classes["@mozilla.org/network/io-service;1"].
> +                         getService(Ci.nsIIOService);
> +    var chan = ios.newChannel("http://localhost:4444" + url, "", null);
> +    var httpChan = chan.QueryInterface(Components.interfaces.nsIHttpChannel);

Looks like you don't need this QI.

@@ +22,5 @@
> +    return httpChan;
> +}
> +
> +function startIter() {
> +    var channel = setupChannel(tests[index].url );

No space after url.

@@ +28,5 @@
> +}
> +
> +function completeIter(request, data, ctx) {
> +    do_check_true(data.length == tests[index].datalen);
> +    if (++index < tests.length ) {

No space after length.

Comment 17

[Jason Duell]

> > see bug 255119.
> bz doesn't like referring bugs.  I didn't ask him why

IIRC the idea is that generally one can get the bug number from a piece of logic by looking at hg blame.

Comment 18

[Patrick McManus [:mcmanus]]

Thanks Honza!

(In reply to Honza Bambas (:mayhemer) from comment #16)

> BTW, this also affects loading file:/// and actually any local or resource
> URI, probably not an issue, actually a nice benefit?

yes, done by design.

> 
> Some |bool NS_SchemeIs(nsIChannel* channel, const char* scheme)| function

I didn't do that here, but we should add something along those lines.

(In reply to Jason Duell (:jduell) from comment #17)

> IIRC the idea is that generally one can get the bug number from a piece of
> logic by looking at hg blame.

IMO, that's really the wrong trade off when trying to explain a piece of logic. I'm not saying every change needs a bug # in the code, I'm just saying if it helps explain the logic beyond what is reasonable to comment then a bug number is a useful footnote. hg blame is useful, but it is not equivalent - it can often be quite a bit of pain due to reorgs, history purges (already happened once, I heard discussion of doing it again during work week as part of a hypothetical transition to git) and similar things.

Comment 19

[Jason Duell]

bz: Patrick:

I agree that there are times when citing a bug # in a comment is the tersest way to explain what some code is up to, and that hg blame is a potentially transient way to be able to look it up.  So I'm not opposed to it always (just only when needed).

Yes, this is a terrible forum for this.  I suppose this could go all the way to dev.platform or some other large forum.  I'm trying to get work done! :)

Comment 20

[Boris Zbarsky [:bzbarsky]]

The usual problem is that if we had a bug# with every checkin or even every comment, we'd have bug numbers all over the code.  Sometimes ones which are no longer relevant (in that the code has changed but the comment has not).  We in fact have that problem in some modules.

So the question is where to draw the line.  That's usually up to whoever gets to maintain the code.  ;)

Comment 21

[Honza Bambas (:mayhemer)]

IMO, instead of bug# there should be a comment how you came up to the solution, where the error takes from and why use this error only, why not to convert it to content load denial when redirecting to http(s), but do that for any other protocol, etc.  A good reasonably long comment explaining it in a simple english is good think to have here, because the change is not obvious.  More comments is always good..

Comment 22

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/7431723f8242

Comment 23

[Eric Shepherd [:sheppy]]

Documented:

https://developer.mozilla.org/en/HTTP#More_on_redirection_responses

And mentioned on Firefox 9 for developers.