Last Comment Bug 475636 - Disallow refresh to javascript uris
: Disallow refresh to javascript uris
Status: RESOLVED FIXED
[sg:moderate] xss hazard
: fixed1.9.1, verified1.9.0.9
Product: Core
Classification: Components
Component: Document Navigation (show other bugs)
: Trunk
: All All
: P2 normal (vote)
: ---
Assigned To: Jonas Sicking (:sicking)
:
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2009-01-27 16:18 PST by Jonas Sicking (:sicking)
Modified: 2009-07-12 14:36 PDT (History)
13 users (show)
jonas: blocking1.9.1+
dveditz: blocking1.9.0.9+
dveditz: wanted1.9.0.x+
dveditz: wanted1.8.1.x+
bzbarsky: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
WIP (6.01 KB, patch)
2009-02-25 16:33 PST, Jonas Sicking (:sicking)
no flags Details | Diff | Review
Patch to fix (20.67 KB, patch)
2009-02-26 14:09 PST, Jonas Sicking (:sicking)
no flags Details | Diff | Review
Patch -w (18.22 KB, patch)
2009-02-26 14:12 PST, Jonas Sicking (:sicking)
bzbarsky: review-
bzbarsky: superreview-
Details | Diff | Review
Patch to fix (21.74 KB, patch)
2009-03-06 08:59 PST, Jonas Sicking (:sicking)
no flags Details | Diff | Review
Patch -w (19.42 KB, patch)
2009-03-06 09:00 PST, Jonas Sicking (:sicking)
bzbarsky: review+
bzbarsky: superreview+
dveditz: approval1.9.0.9-
Details | Diff | Review
Fixed patch (17.55 KB, patch)
2009-03-13 11:08 PDT, Jonas Sicking (:sicking)
dveditz: approval1.9.0.9+
Details | Diff | Review
1.8.0 branch patch (13.98 KB, patch)
2009-04-08 05:05 PDT, Martin Stránský
no flags Details | Diff | Review
from bonsai (as checked into 1.9.0) (15.34 KB, patch)
2009-04-27 02:05 PDT, Alexander Sack
no flags Details | Diff | Review
for 1.8 based on bonsai (18.47 KB, patch)
2009-04-28 04:50 PDT, Alexander Sack
no flags Details | Diff | Review
1.8 backport for nsINestedURI and NS_GetInnermostURI (19.94 KB, patch)
2009-04-28 04:54 PDT, Alexander Sack
no flags Details | Diff | Review
1.8 backport for nsINestedURI and NS_GetInnermostURI (better diff options) (19.28 KB, patch)
2009-04-28 05:08 PDT, Alexander Sack
bzbarsky: review-
Details | Diff | Review
for 1.8 based on bonsai (better diff options) (18.37 KB, patch)
2009-04-28 05:09 PDT, Alexander Sack
bzbarsky: review-
Details | Diff | Review
re-make the patch based-on Alexander, according to Comment #33 From Boris Zbarsky. (15.46 KB, patch)
2009-05-14 23:33 PDT, Peng Wu
bzbarsky: review-
Details | Diff | Review
Re-make the patch, according to Boris Zbarsky's comment. (15.57 KB, patch)
2009-05-20 21:06 PDT, Peng Wu
bzbarsky: review-
Details | Diff | Review
Patch for fixes refresh uri, according to Boris Zbarsky's comment. (15.46 KB, patch)
2009-06-12 02:04 PDT, Peng Wu
bzbarsky: review+
Details | Diff | Review

Description Jonas Sicking (:sicking) 2009-01-27 16:18:25 PST
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] 2009-01-28 15:55:39 PST
Doubt this makes 1.9.1 in time for 1.9.0.7, pushing out.
Comment 2 Damon Sicore (:damons) 2009-02-17 10:33:41 PST
Jonas, any progress here?
Comment 3 Jonas Sicking (:sicking) 2009-02-25 16:33:26 PST
Created attachment 364212 [details] [diff] [review]
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) 2009-02-26 14:09:37 PST
Created attachment 364393 [details] [diff] [review]
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) 2009-02-26 14:12:31 PST
Created attachment 364395 [details] [diff] [review]
Patch -w

For your reviewing pleasure.
Comment 6 Boris Zbarsky [:bz] 2009-03-04 10:47:57 PST
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) 2009-03-06 08:59:31 PST
Created attachment 365916 [details] [diff] [review]
Patch to fix

Review comments fixed
Comment 8 Jonas Sicking (:sicking) 2009-03-06 09:00:14 PST
Created attachment 365917 [details] [diff] [review]
Patch -w
Comment 9 Boris Zbarsky [:bz] 2009-03-06 11:01:01 PST
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) 2009-03-10 15:38:18 PDT
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) 2009-03-10 16:06:51 PDT
Checked in to 1.9.1
http://hg.mozilla.org/releases/mozilla-1.9.1/rev/0fd67a5bb370
Comment 12 Boris Zbarsky [:bz] 2009-03-10 16:49:11 PDT
in-testsuite+, right?
Comment 13 Jonas Sicking (:sicking) 2009-03-10 23:01:55 PDT
Yes
Comment 14 Daniel Veditz [:dveditz] 2009-03-13 10:23:17 PDT
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) 2009-03-13 11:08:10 PDT
Created attachment 367254 [details] [diff] [review]
Fixed patch
Comment 16 Daniel Veditz [:dveditz] 2009-03-16 14:49:07 PDT
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) 2009-03-18 12:41:17 PDT
Checked in to 1.9.0 branch last night.
Comment 18 Al Billings [:abillings] 2009-03-23 14:43:51 PDT
Verified for 1.9.0.8 based on checked in test.
Comment 19 Martin Stránský 2009-04-08 05:05:54 PDT
Created attachment 371655 [details] [diff] [review]
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ý 2009-04-08 05:08:12 PDT
Comment on attachment 371655 [details] [diff] [review]
1.8.0 branch patch

Can you please check this one?
Comment 21 Boris Zbarsky [:bz] 2009-04-08 09:18:46 PDT
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) 2009-04-08 11:04:46 PDT
Oh crap! I'll write a patch for 1.9.0 that fixes this
Comment 23 Jonas Sicking (:sicking) 2009-04-08 13:53:46 PDT
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] 2009-04-09 09:42:56 PDT
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] 2009-04-09 09:47:31 PDT
(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 2009-04-25 23:17:04 PDT
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 2009-04-27 02:05:14 PDT
Created attachment 374743 [details] [diff] [review]
from bonsai (as checked into 1.9.0)
Comment 28 Alexander Sack 2009-04-28 04:50:26 PDT
Created attachment 374889 [details] [diff] [review]
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 2009-04-28 04:54:29 PDT
Created attachment 374891 [details] [diff] [review]
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 2009-04-28 05:08:04 PDT
Created attachment 374894 [details] [diff] [review]
1.8 backport for nsINestedURI and NS_GetInnermostURI (better diff options)

same but with better diff options for review.
Comment 31 Alexander Sack 2009-04-28 05:09:54 PDT
Created attachment 374895 [details] [diff] [review]
for 1.8 based on bonsai (better diff options)
Comment 32 Boris Zbarsky [:bz] 2009-04-28 08:36:02 PDT
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 [:bz] 2009-04-28 08:38:08 PDT
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 [:bz] 2009-04-28 08:38:38 PDT
Oh, and in any case, I see no need for the simple nested URI stuff on 1.8 right now.
Comment 35 Peng Wu 2009-05-14 23:33:16 PDT
Created attachment 377634 [details] [diff] [review]
re-make the patch based-on Alexander, according to Comment #33 From  Boris Zbarsky.

Use IJARURI according to Boris Zbarsky.
Comment 36 Peng Wu 2009-05-14 23:34:57 PDT
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 [:bz] 2009-05-18 13:25:15 PDT
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 2009-05-20 21:02:52 PDT
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 2009-05-20 21:06:09 PDT
Created attachment 378785 [details] [diff] [review]
Re-make the patch, according to Boris Zbarsky's comment.
Comment 40 Peng Wu 2009-05-20 22:08:25 PDT
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 [:bz] 2009-05-21 13:53:26 PDT
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 2009-06-12 02:04:22 PDT
Created attachment 382934 [details] [diff] [review]
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 [:bz] 2009-06-12 11:53:28 PDT
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 2009-07-12 13:52:25 PDT
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 [:bz] 2009-07-12 14:00:54 PDT
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 2009-07-12 14:09:28 PDT
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 [:bz] 2009-07-12 14:17:30 PDT
General comment: commenting about details of a security-sensitive bug in a non-security-sensitive one is stupid, no?
Comment 48 Jonas Sicking (:sicking) 2009-07-12 14:33:10 PDT
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 [:bz] 2009-07-12 14:36:40 PDT
Just to be clear, I think the bug mentioned in comment 46 is invalid and shouldn't be security-sensitive to start with.

Note You need to log in before you can comment on or make changes to this bug.