Disallow refresh to javascript uris

RESOLVED FIXED

Status

()

Core
Document Navigation
P2
normal
RESOLVED FIXED
8 years ago
8 years ago

People

(Reporter: sicking, Assigned: sicking)

Tracking

({fixed1.9.1, verified1.9.0.9})

Trunk
fixed1.9.1, verified1.9.0.9
Points:
---
Bug Flags:
blocking1.9.1 +
blocking1.9.0.9 +
wanted1.9.0.x +
wanted1.8.1.x +
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [sg:moderate] xss hazard)

Attachments

(5 attachments, 10 obsolete attachments)

17.55 KB, patch
Details | Diff | Splinter Review
15.34 KB, patch
Details | Diff | Splinter Review
19.28 KB, patch
Details | Diff | Splinter Review
18.37 KB, patch
Details | Diff | Splinter Review
15.46 KB, patch
Details | Diff | Splinter Review
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.
Flags: blocking1.9.1+
Flags: wanted1.9.0.x?
Flags: blocking1.9.0.7?

Updated

8 years ago
Whiteboard: [sg:low] xss hazard
Flags: wanted1.9.0.x?
Flags: wanted1.9.0.x+
Flags: blocking1.9.0.7?
Flags: blocking1.9.0.7+
Priority: -- → P2
Doubt this makes 1.9.1 in time for 1.9.0.7, pushing out.
Flags: blocking1.9.0.7+ → blocking1.9.0.8+
Whiteboard: [sg:low] xss hazard → [sg:moderate] xss hazard
Jonas, any progress here?
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.
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.
Attachment #364212 - Attachment is obsolete: true
Created attachment 364395 [details] [diff] [review]
Patch -w

For your reviewing pleasure.
Attachment #364395 - Flags: superreview?(bzbarsky)
Attachment #364395 - Flags: review?(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.
Attachment #364395 - Flags: superreview?(bzbarsky)
Attachment #364395 - Flags: superreview-
Attachment #364395 - Flags: review?(bzbarsky)
Attachment #364395 - Flags: review-
Created attachment 365916 [details] [diff] [review]
Patch to fix

Review comments fixed
Attachment #364393 - Attachment is obsolete: true
Attachment #364395 - Attachment is obsolete: true
Attachment #365916 - Flags: review?
Created attachment 365917 [details] [diff] [review]
Patch -w
Attachment #365917 - Flags: superreview?(bzbarsky)
Attachment #365917 - Flags: review?(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!
Attachment #365917 - Flags: superreview?(bzbarsky)
Attachment #365917 - Flags: superreview+
Attachment #365917 - Flags: review?(bzbarsky)
Attachment #365917 - Flags: review+

Updated

8 years ago
Keywords: checkin-needed
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
Status: NEW → RESOLVED
Last Resolved: 8 years ago
Resolution: --- → FIXED
Checked in to 1.9.1
http://hg.mozilla.org/releases/mozilla-1.9.1/rev/0fd67a5bb370
Keywords: checkin-needed → fixed1.9.1
Attachment #365917 - Flags: approval1.9.0.8?
in-testsuite+, right?
Flags: in-testsuite+
Yes
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?
Attachment #365917 - Flags: approval1.9.0.8? → approval1.9.0.8-
Attachment #365916 - Flags: review?
Created attachment 367254 [details] [diff] [review]
Fixed patch
Attachment #365916 - Attachment is obsolete: true
Attachment #365917 - Attachment is obsolete: true
Attachment #367254 - Flags: approval1.9.0.8?
Attachment #367254 - Flags: approval1.9.0.8? → approval1.9.0.8+
Comment on attachment 367254 [details] [diff] [review]
Fixed patch

Approved for 1.9.0.8, a=dveditz for release-drivers
Checked in to 1.9.0 branch last night.
Keywords: fixed1.9.0.8
Verified for 1.9.0.8 based on checked in test.
Keywords: fixed1.9.0.8 → verified1.9.0.8
Flags: wanted1.8.1.x+

Comment 19

8 years ago
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...

Updated

8 years ago
Attachment #371655 - Flags: review?(bzbarsky)

Comment 20

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

Can you please check this one?
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?
Oh crap! I'll write a patch for 1.9.0 that fixes this
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.
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.
(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)
Group: core-security
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

8 years ago
Created attachment 374743 [details] [diff] [review]
from bonsai (as checked into 1.9.0)

Updated

8 years ago
Attachment #371655 - Flags: review?(bzbarsky)

Comment 28

8 years ago
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.
Attachment #374889 - Flags: review?(bzbarsky)

Updated

8 years ago
Attachment #371655 - Attachment is obsolete: true

Comment 29

8 years ago
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)
Attachment #374891 - Flags: review?(bzbarsky)

Comment 30

8 years ago
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.
Attachment #374891 - Attachment is obsolete: true
Attachment #374894 - Flags: review?(bzbarsky)
Attachment #374891 - Flags: review?(bzbarsky)

Comment 31

8 years ago
Created attachment 374895 [details] [diff] [review]
for 1.8 based on bonsai (better diff options)
Attachment #374889 - Attachment is obsolete: true
Attachment #374895 - Flags: review?(bzbarsky)
Attachment #374889 - Flags: review?(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 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...
Attachment #374895 - Flags: review?(bzbarsky) → review-
Oh, and in any case, I see no need for the simple nested URI stuff on 1.8 right now.

Comment 35

8 years ago
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.
Attachment #377634 - Flags: approval1.8.1.next?

Comment 36

8 years ago
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.
Attachment #374894 - Flags: review?(bzbarsky) → review-
Attachment #377634 - Flags: review?(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?
Attachment #377634 - Flags: review?(bzbarsky)
Attachment #377634 - Flags: review-
Attachment #377634 - Flags: approval1.8.1.next?

Comment 38

8 years ago
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

8 years ago
Created attachment 378785 [details] [diff] [review]
Re-make the patch, according to Boris Zbarsky's comment.
Attachment #377634 - Attachment is obsolete: true
Attachment #378785 - Flags: review?(bzbarsky)

Comment 40

8 years ago
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 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?
Attachment #378785 - Flags: review?(bzbarsky) → review-

Comment 42

8 years ago
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.
Attachment #378785 - Attachment is obsolete: true
Comment on attachment 382934 [details] [diff] [review]
Patch for fixes refresh uri, according to Boris Zbarsky's comment.

Yeah, there we go.
Attachment #382934 - Flags: review+

Comment 44

8 years ago
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.
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

8 years ago
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.
General comment: commenting about details of a security-sensitive bug in a non-security-sensitive one is stupid, no?
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.
Just to be clear, I think the bug mentioned in comment 46 is invalid and shouldn't be security-sensitive to start with.
You need to log in before you can comment on or make changes to this bug.