475636 - Disallow refresh to javascript uris

Status: RESOLVED FIXED

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

Description

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

This is a spinoff from bug 461413.

We should not allow meta-refresh nor Refresh headers to 'redirect' to javascript URIs. I think we already disallow Location-header redirects to javascript uris because there are lots of sites that host generic redirecters that don't check the URI.

It's entirely possible that that is the case for Refresh redirects too.

IE already blocks javascript uris so it's unlikely that we'll break the web.

Comment 1

[Daniel Veditz [:dveditz]]

Doubt this makes 1.9.1 in time for 1.9.0.7, pushing out.

Comment 2

[Damon Sicore (:damons)]

Jonas, any progress here?

Comment 3

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Attachment: WIP

This fixes things so that refresh to JS-URI is no longer permitted. Or at least I think it does, the refresh handling code is an amazing maze with way too many entry points.

However I realized that refresh to data: is basically the same thing. However it would be a little bit more sad to block refreshing to a data:-uri. So instead I think we should make sure that the data:-uri page doesn't get the principal of the original page.

For some reason though we do end up with the original page principal. I tried to look at the docshell code, but found myself in a maze of twisty passages, all alike.

Comment 4

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Attachment: Patch to fix

Talked to bz last night and finally understood how things are supposed to work, and where it went wrong.

Specifically what happens here is that since there is no JS on the stack, we decide to inherit owner even though the refresh-code doesn't specifically request this.

What I did to fix this was to add a flag to nsIDocShellLoadInfo that tells the docshell not to be clever and do guessing on when the caller doesn't really mean what it's requesting.

Comment 5

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Attachment: Patch -w

For your reviewing pleasure.

Comment 6

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 364395 [details] [diff] [review]
Patch -w

>+++ src.1a35ee50317e/docshell/base/nsDocShell.cpp    2009-02-26 
>+    PRBool explicitOwner = PR_FALSE;

How about |ownerIsExplicit|?  Same for the idl.

>@@ -4821,16 +4839,24 @@ nsDocShell::SetupRefreshURIFromHeader(ns

>+            if (NS_SUCCEEDED(rv)) {
>+                PRBool isjs = PR_TRUE;
>+                if (NS_FAILED(uri->SchemeIs("javascript", &isjs)) || isjs) {
>+                    return NS_ERROR_FAILURE;

Please test NS_URIChainHasFlags(uri, nsIProtocolHandler::URI_OPENING_EXECUTES_SCRIPT, ) instead.

I'd like to see a patch with that change.

Comment 7

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Attachment: Patch to fix

Review comments fixed

Comment 8

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Attachment: Patch -w

Comment 9

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 365917 [details] [diff] [review]
Patch -w

Looks ok, except for that unrelated nsExpatDriver change.  Make sure to not check that in!

Comment 10

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Checked in
http://hg.mozilla.org/mozilla-central/rev/6328cc687abe

Though somehow that nsExpatDriver change made it in, so removed it in
http://hg.mozilla.org/mozilla-central/rev/3b40510faeff

Comment 11

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Checked in to 1.9.1
http://hg.mozilla.org/releases/mozilla-1.9.1/rev/0fd67a5bb370

Comment 12

[Boris Zbarsky [:bzbarsky]]

in-testsuite+, right?

Comment 13

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Yes

Comment 14

[Daniel Veditz [:dveditz]]

Comment on attachment 365917 [details] [diff] [review]
Patch -w

Jonas: would like to approve this, but I'm worried old rev maintainers will try to merge this patch as-is and pick up the expat-driver change. Can we get a version of the patch without that?

Comment 15

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Attachment: Fixed patch

Comment 16

[Daniel Veditz [:dveditz]]

Comment on attachment 367254 [details] [diff] [review]
Fixed patch

Approved for 1.9.0.8, a=dveditz for release-drivers

Comment 17

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Checked in to 1.9.0 branch last night.

Comment 18

[Al Billings [:abillings - ex-MoCo]]

Verified for 1.9.0.8 based on checked in test.

Comment 19

[Martin Stránský [:stransky] (ni? me)]

Attachment: 1.8.0 branch patch

I've used uri->SchemeIs() in nsDocShell::SetupRefreshURIFromHeader() because nsIProtocolHandler::URI_OPENING_EXECUTES_SCRIPT is not defined in 1.8.0...

Comment 20

[Martin Stránský [:stransky] (ni? me)]

Comment on attachment 371655 [details] [diff] [review]
1.8.0 branch patch

Can you please check this one?

Comment 21

[Boris Zbarsky [:bzbarsky]]

That's changing an interface on a stable branch, no?

I'm not sure why exactly we did that on 1.9.0, for that matter.  Jonas?

Comment 22

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Oh crap! I'll write a patch for 1.9.0 that fixes this

Comment 23

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Actually, the patch I checked in on the 1.9.0 branch did the correct interface fu. I do remember that I found that last minute before checking in and fixed up the patch without bothering with another round of reviews.

So we're all good on the 1.9.0 branch.

Comment 24

[Daniel Veditz [:dveditz]]

This does show one reason why "as checked in" attachments are important for security fixes (at least) that are likely to be ported to other branches. And also why those doing ports would be wise to double-check with cvs/hg/bonsai before back-porting.

Comment 25

[Daniel Veditz [:dveditz]]

(and yes, I do live in glass houses, having single-handedly caused the 2.0.0.11 release by missing part of a checkin when doing a back-port for 2.0.0.10)

Comment 26

[georgi - hopefully not receiving bugspam]

meta refresh redirects are allowed for 'data:text/html;,', though XSS doesn't seem easy. are meta redirects to data: allowed on purpose?

Comment 27

[Alexander Sack]

Attachment: from bonsai (as checked into 1.9.0)

Comment 28

[Alexander Sack]

Attachment: for 1.8 based on bonsai

This needs another patch that backports NS_GetInnermostURI, nsINestedURI and friends; will attach that separately.

Comment 29

[Alexander Sack]

Attachment: 1.8 backport for nsINestedURI and NS_GetInnermostURI

prerequisites for attachment 374889 [details] [diff] [review] on 1.8 branch (see comment 28)

Comment 30

[Alexander Sack]

Attachment: 1.8 backport for nsINestedURI and NS_GetInnermostURI (better diff options)

same but with better diff options for review.

Comment 31

[Alexander Sack]

Attachment: for 1.8 based on bonsai (better diff options)

Comment 32

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 374894 [details] [diff] [review]
1.8 backport for nsINestedURI and NS_GetInnermostURI (better diff options)

How does this help you?

Comment 33

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 374895 [details] [diff] [review]
for 1.8 based on bonsai (better diff options)

The NS_GetInnermostURI call doesn't do you any good on 1.8, since you didn't port any of the _implementations_ of it.

What you want to do here instead is a loop while QIing to nsIJARURI, like we do in other places on 1.8.  Unless you really want to backport the whole thing about jar URIs implementing nsINestedURI or something...

Comment 34

[Boris Zbarsky [:bzbarsky]]

Oh, and in any case, I see no need for the simple nested URI stuff on 1.8 right now.

Comment 35

[Peng Wu]

Attachment: re-make the patch based-on Alexander, according to Comment #33 From Boris Zbarsky.

Use IJARURI according to Boris Zbarsky.

Comment 36

[Peng Wu]

Re-make the patch based-on Alexander's patch, according to Comment #33 From Boris
Zbarsky.

Use IJARURI according to Boris Zbarsky's comment.

Sorry for the typo.

Comment 37

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 377634 [details] [diff] [review]
re-make the patch based-on Alexander, according to Comment #33 From  Boris Zbarsky.

>Index: docshell/base/nsDocShell.cpp
>@@ -4495,6 +4520,25 @@ nsDocShell::SetupRefreshURIFromHeader(ns
>+                    jarURI->GetJARFile(getter_AddRefs(uri));

That's wrong; it'll change the URI we refresh to, which is not hwat you want here.  You presumably want to set innerURI to uri before you start the loop, change it in the loop, and then use it for the schemeIs check after the loop.

>+                nsCOMPtr<nsIURI> innerURI(do_QueryInterface(jarURI));
>+                NS_ENSURE_TRUE(innerURI, NS_ERROR_FAILURE);

This will always return failure, since the loop only exits when jarURI is null.  Did you happen to actually test this code, by any chance?

Comment 38

[Peng Wu]

Sorry, I am new to Mozilla, and I am exhausted last Friday.
Last time I just copy the code from http://mxr.mozilla.org/mozilla1.8.0/source/dom/src/base/nsLocation.cpp.
I am also confused by the code.
Thanks for your comments, I understand the errors in the code.

And I setup the MochiTest environment yesterday, and test the new patch, it seems work now.
Thanks for reviewing.

Comment 39

[Peng Wu]

Attachment: Re-make the patch, according to Boris Zbarsky's comment.

Comment 40

[Peng Wu]

Sorry, I test this bug on Firefox 2.0.0.14. It seems Firefox 2.0.0.14 is not affected by this bug.
As the test case is using window.parent.postMessage, which is not supported by Firefox 2.0, I changed it to use document.write(), to see the test results.
If I am wrong, please correct me, thanks.

Comment 41

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 378785 [details] [diff] [review]
Re-make the patch, according to Boris Zbarsky's comment.

> It seems Firefox 2.0.0.14 is not affected by this bug.

Then you're testing wrong.

>+                nsCOMPtr<nsIJARURI> jarURI(do_QueryInterface(uri));
>+                nsCOMPtr<nsIURI> innerURI;
>+                while (jarURI) {
>+                    jarURI->GetJARFile(getter_AddRefs(innerURI));
>+                    if (!innerURI)
>+                        break;
>+                    jarURI = do_QueryInterface(innerURI);
>+                }
>+                innerURI = do_QueryInterface(jarURI);
>+                if (!innerURI)
>+                    innerURI = uri;

That code does the same thing as this code:

  innerURI = uri;

because GetJARFile is never going to hand back null.

And note that "test the code" doesn't just mean "test whether it fixes the bug"; it also means "does the code still work otherwise?".  This patch breaks all refreshes, just like your previous patch.

Please stop asking for reviews on patches that would fail a simple "does it fix the bug and does refresh to other kinds of URIs still work?" test, ok?

Comment 42

[Peng Wu]

Attachment: Patch for fixes refresh uri, according to Boris Zbarsky's comment.

Hi Boris Zbarsky,
  Sorry for the last two mistakes in previous patches. This time I did check on some sites to test refresh functionality, it really work now.
  To save your time, I posted the changed code here:
+
+            if (NS_SUCCEEDED(rv)) {
+                nsCOMPtr<nsIURI> innerURI = uri;
+                nsCOMPtr<nsIJARURI> jarURI(do_QueryInterface(innerURI));
+                while (jarURI) {
+                    jarURI->GetJARFile(getter_AddRefs(innerURI));
+                    jarURI = do_QueryInterface(innerURI);
+                }
+                NS_ENSURE_TRUE(innerURI, NS_ERROR_FAILURE);
+
+                PRBool isjs = PR_TRUE;
+                rv = innerURI->SchemeIs("javascript", &isjs);
+                NS_ENSURE_SUCCESS(rv, rv);
+
+                if (isjs) {
+                    return NS_ERROR_FAILURE;
+                }
+            }
+

  Hopefully this is what you want me to write. Please spend a little time to look at the new patch. Thanks for your corrections on my patches.

Comment 43

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 382934 [details] [diff] [review]
Patch for fixes refresh uri, according to Boris Zbarsky's comment.

Yeah, there we go.

Comment 44

[MustLive]

It's good that Mozilla fixed this hole - disallowed refresh redirection to javascript URIs.

As I wrote in my advisory Cross-Site Scripting vulnerabilities in Mozilla, Internet Explorer, Opera and Chrome (http://websecurity.com.ua/3275/), not only Firefox (FF <= 3.0.8) is vulnerable, but also Mozilla (old versions), IE6, Opera and Chrome.

Also I mentioned in other post at my site, when I talked about such attacks via refresh header, that Mozilla fixed this hole only in Firefox (FF 3.0.9), but not in SeaMonkey. Which is the selective approach and better to fix in both of them ;-).

> IE already blocks javascript uris so it's unlikely that we'll break the web.

Jonas, you wrote that IE blocks javascript URIs. Tell me, please, what version of IE did you mention. Because IE6 doesn't blocks such URIs.

Comment 45

[Boris Zbarsky [:bzbarsky]]

MustLive, this has been fixed in Gecko 1.0.9 and the patch attached in comment 42 would fix this in Gecko 1.8.1.  Seamonkey currently ships off the (no longer actively maintained) Gecko 1.8.1.  Once they ship a release based on Gecko 1.9 or get the 1.8.1 fix checked in, they'll have the bug fixed.

Comment 46

[MustLive]

But I need to note, that there is a way to bypass this protection in Firefox. Besides, as I mentioned above, in SeaMonkey this hole was not fixed (hope it'll be fixed soon, as Boris point me out).

I wrote about this bypass method in Bug 503789 - Cross-Site Scripting vulnerability in Firefox (https://bugzilla.mozilla.org/show_bug.cgi?id=503789).

> However I realized that refresh to data: is basically the same thing. However
it would be a little bit more sad to block refreshing to a data:-uri. So
instead I think we should make sure that the data:-uri page doesn't get the
principal of the original page.

After I read few first posts in current Bugzilla's entry, I found that Jonas mentioned about this method. But guys, with only removing relation between data: page and original page, you not completely fixed XSS attack. You just removed some attack vectors (like cookie stealing), but other vectors of XSS attacks are still possible. So XSS attack via redirecting to data: URI is still possible, as I mentioned in Bug 503789.

Comment 47

[Boris Zbarsky [:bzbarsky]]

General comment: commenting about details of a security-sensitive bug in a non-security-sensitive one is stupid, no?

Comment 48

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Marking comment 46, and this one private.

I don't remember which version of IE I tested, probably IE7 or IE8. And it's also possible that I did something wrong in my testing of course.

Comment 49

[Boris Zbarsky [:bzbarsky]]

Just to be clear, I think the bug mentioned in comment 46 is invalid and shouldn't be security-sensitive to start with.