211999 - [FIX] Can't redirect to data: urls

Status: RESOLVED FIXED

(Core :: Networking: HTTP, defect, P1)

Description

[Boris Zbarsky [:bzbarsky]]

The security check added in bug 195201 blocks redirects to not only javascript:
urls but also data: urls; there is no clear reason given for this (in the code,
the checkin comment, or bug 195201 itself)... Filing this bug to get that looked
at and either comment the code to explain the check or remove the data: check.

You can test the redirect-blocking at
http://software.hixie.ch/utilities/cgi/data/data

Comment 1

[Darin Fisher]

hmm, yeah.. any javascript embedded in a data: URL shouldn't be able to execute
when loaded via a <IMG> tag, so blocking data: URLs from HTTP redirects should
not have been necessary to fix that bug.  or, maybe i'm overlooking something??

Comment 2

[Jesse Ruderman]

If you allow redirects to data: URLs, you have to make sure that data:text/html
files loaded through redirects run with correct (or no) privileges.

Comment 3

[Darin Fisher]

wouldn't it make sense for a data: URI, resulting from a server side redirect,
to have the privileges of the original URI?  although i'm not sure that is even
a requirement for this bug.

Comment 4

[Jesse Ruderman]

The privs of the data: URL should be the same as the last redirecting URL (not
the page that originally linked to the URL).  I think this is the opposite of
what happens with referers.

Comment 5

[Darin Fisher]

jesse: yup, actually that was what i was thinking.. just wasn't what i wrote :-/

it should be easy enough to transfer the nsIPrincipal from the old channel to
the new channel in this case.  i'm just not sure where we would want that to
live at the moment.  perhaps in the HTTP code; however, necko isn't supposed to
have to know about these things.  there are plans to make necko not depend on
caps, and that would involve having caps hook in as an observer of redirects, so
maybe if that happens caps could easily at that time set the principal for the
new channel based on the old one if appropriate.  hmmm...

Comment 6

[Boris Zbarsky [:bzbarsky]]

caillon, any ideas here?

Comment 7

[Boris Zbarsky [:bzbarsky]]

*** Bug 253320 has been marked as a duplicate of this bug. ***

Comment 8

[Jesse Ruderman]

Bug 195201's fix made simple server-side redirectors not be vulnerable to XSS attacks.  To be consistent with that security goal, pages generated from data: URIs loaded through redirects would have to have their own principal (equal to itself, but not able to e.g. see the cookie from the site that did the redirect).

Comment 9

[Boris Zbarsky [:bzbarsky]]

You mean the problem that worries you is that I (site A) load data from site B and site B does a server-side redirect to a data: URI, which should then get the principals of B, not A?

That seems reasonable and not that hard to do.  Can you set up a testcase somewhere?

Comment 10

[Jesse Ruderman]

Not quite.  Giving it the principals of B is also somewhat dangerous, because there are many server-side redirectors that don't check what protocol they're redirecting to (and probably many that disallow redirects to "javascript:" but don't disallow redirects to "data:").  That's why I suggested not giving it the principals of A or B, but its own non-URL-based principal.

Comment 11

[Hixie (not reading bugmail)]

Just out of interest, is there a way for a site to make a data: URI have no rights at all? e.g. in the case I care about, the data: URI kitchen, I'm letting people create arbitrary data: URIs. I don't really want them to have the permissions of software.hixie.ch, I want them to have no permissions at all -- as if they'd typed that data: URI themselves, in the location bar.

Comment 12

[Boris Zbarsky [:bzbarsky]]

(In reply to comment #10)
> Not quite.  Giving it the principals of B is also somewhat dangerous

So the point is that some sites accept the URI to redirect to in the request?  And then just do it?  <sigh>...

Either way, that should be doable without too much trouble.  Can someone set up a testcase so I can test once I think I have a patch?

Comment 13

[Hixie (not reading bugmail)]

The CGI script in the URL field lets you create any random data: URI and will then redirect to it.

Comment 14

[Boris Zbarsky [:bzbarsky]]

Ah, excellent.  Jesse, what's a scenario you're worried about?  Want to data:-URLize it?

Comment 15

[Boris Zbarsky [:bzbarsky]]

So I just checked.  If we just allow redirects to data: URIs it should Just Work.  That is, the post-redirect document will have the URI principal for the data: URI.  Which means it will not be same-origin for either the original page nor the HTTP server that redirected to it.

So what's the problem with just allowing them, exactly?

Comment 16

[Boris Zbarsky [:bzbarsky]]

Attachment: Patch to fix

I've verified (by stepping through the process in a debugger, though this is also possible to verify via code inspection) that the new document gets a principal based on the data: URI.  This means that it will be same-origin with any other data: URI that has the exact same data and wasn't loaded directly from a web page.  Which seems reasonable to me, frankly.

In particular, given a server-side redirector at site B and given a site at site A that uses the redirector, the data: document will have the principals of neither A nor B, as desired.

Comment 17

[Jesse Ruderman]

Comment on attachment 209302 [details] [diff] [review]
Patch to fix

Sounds good (not a code review).

I think the data: URL that come from nowhere should get a principal that is only equal to itself (by pointer comparison), but that can be discussed elsewhere.

Comment 18

[Daniel Veditz [:dveditz]]

Comment on attachment 209302 [details] [diff] [review]
Patch to fix

I've tested it and seems safe (safer than clicking the data url in hixie's "your browser is broken" page!).

r=dveditz

Comment 19

[Daniel Veditz [:dveditz]]

It's a minor thing, but should this be fixed for 1.8.1? Looks like the patch would be in nsHttpChannel.cpp instead because bug 248052 is trunk-only.

Comment 20

[Boris Zbarsky [:bzbarsky]]

Fixed.  Filed bug 334407 for the data: thing.