Closed Bug 1281071 (CVE-2016-9070) Opened 8 years ago Closed 8 years ago

Sidebar bookmark can have reference to chrome window

Categories

(Core :: DOM: Core & HTML, defect)

Product:
Core
Component:
DOM: Core & HTML
Version:
50 Branch
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla50
Tracking Flags:
Tracking Status
firefox49 --- wontfix
firefox-esr45 --- wontfix
firefox50 --- fixed

People

(Reporter: qab, Assigned: bzbarsky)

Details

(Keywords: sec-moderate, Whiteboard: [post-critsmash-triage][adv-main50+] btpp-active)

Attachments

(2 files)

Possible patch
1.78 KB, patch
bholley
: review+
Details | Diff | Splinter Review
Patch now with test
6.43 KB, patch
Details | Diff | Splinter Review 
User Agent: Mozilla/5.0 (Windows NT 6.3; WOW64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/51.0.2704.103 Safari/537.36

Steps to reproduce:

----------Sidebar-Source----------------------------------------
<html>
<head></head>
<body>
<textarea id="hell"></textarea>
<br>
<button id="butt">go</button>
<script>
butt.onclick=function(){
	window.location='javascript:'+hell.value+';void(0)';
}
</script>
</body>
</html>
----------------base64-Encoded-------------------------------------data:text/html;base64,PGh0bWw+CjxoZWFkPjwvaGVhZD4KPGJvZHk+Cjx0ZXh0YXJlYSBpZD0iaGVsbCI+PC90ZXh0YXJlYT4KPGJyPgo8YnV0dG9uIGlkPSJidXR0Ij5nbzwvYnV0dG9uPgo8c2NyaXB0PgpidXR0Lm9uY2xpY2s9ZnVuY3Rpb24oKXsKCXdpbmRvdy5sb2NhdGlvbj0namF2YXNjcmlwdDonK2hlbGwudmFsdWUrJzt2b2lkKDApJzsKfQo8L3NjcmlwdD4KPC9ib2R5Pgo8L2h0bWw+
-----------------------------------------------------------------------

1. Open Firefox (tested on nightly and stable) and hit CTRL+SHIFT+B to open bookmark library.
2. Create new bookmark with the location of the base64 encoded data uri above.
3. Make sure to check 'load this bookmark in the sidebar' and add
4. Open a new tab then execute the bookmark, it should open on the side.
5. Open the web console of the new tab (to view the error)

6. Execute the following and observe behavior (by copy pasting it inside the textarea and pressing 'go'):

-  alert(content) => Error: "Error: Permission denied to access property Symbol.toPrimitive" {file: "javascript:alert(content);void(0)" line: 1}]
@javascript:alert(content);void(0):1:1


- content.close() => [JavaScript Error: "Error: Permission denied to access property "close"" {file: "javascript:content.close();void(0)" line: 1}]
@javascript:content.close();void(0):1:1

- close.bind(content)() => [JavaScript Warning: "Scripts may not close windows that were not opened by script." {file: "javascript:close.bind(content)();void(0)" line: 1}]
Note: The above javascript warning was displayed inside the console opened in newtab.

- var q = new mozRTCPeerConnection;q.createOffer(Map,close.bind(content)); => [JavaScript Warning: "Scripts may not close windows that were not opened by script." {file: "about:newtab" line: 0}]

note: notice the file: that originated the error.




Actual results:

It seems like javascript is executed in the chrome context though so far I can only execute close(), blur() and focus()


Expected results:

Block script execution from content to chrome window. 

The theory is that this could be used in the future to execute code in chrome context using other exploits.
(In reply to Abdulrahman Alqabandi from comment #0)
> It seems like javascript is executed in the chrome context though so far I
> can only execute close(), blur() and focus()

What leads you to think this is the case? The document doesn't have the system principal AFAICT. It has a reference to the window currently loaded in the tab, which it can only access if it's same-origin with that thing. Note that 'content' is just defined on Window ( https://dxr.mozilla.org/mozilla-central/source/dom/webidl/Window.webidl#397 ), so the reference also exists in the main content window itself. This is useful if the page in the sidebar *is* same-origin with the page you have open in the tab. AFAICT you're not actually successfully breaking the cross-compartment barrier between the sidebar and the main window content, nor does the JS-as-run have system privileges. Bobby or you can correct me if I'm missing something here.
Flags: needinfo?(qab)
Flags: needinfo?(bobbyholley)
Oh - and additionally, you could already use javascript: or data: URIs running in the content browser directly when creating those as bookmarks / bookmarklets, which is a broadly recognized feature-with-security-issues. So even if you could access the content from that sidebar I'm not sure I follow how that alone gains you anything over running javascript: URIs directly against the content window from a bookmark.
> What leads you to think this is the case? The document doesn't have the system principal AFAICT.

The main reason I think so is that when I directly access close or blur its blocked, using the other methods, it seems like it fires. Also the fact that the console shows a javascript error and claims the source of the error is 'about:newtab' brings me to the conclusion that there is some confusion going on.

>and additionally, you could already use javascript: or data: URIs running in the content browser directly when creating those as bookmarks / bookmarklets,

Bookmarketed javascript: urls dont work on privileged windows. I believe javascript: uris are blocked even if the user manually enters it in the address bar within a privileged window.

As far as I know you can't reproduce the behavior I mentioned using non-sidebar bookmarks on privileged windows.   

Also, if you open a non-sidebar bookmark of a data: uri while a privileged window is open, it wont result in data: having any higher privileges, as its just content.
Flags: needinfo?(qab)
I guess its not that we have code running as chrome, but that non-chrome content is accessing (or at least executing) chrome functions which is not supposed to be the case.
Also, the only other way I'm aware of gaining a reference to a chrome owned window is to perform window.open() within a content and then having the user manually navigate to something like 'about:newtab' in the newly opened window. Which would give the same results as here.

Pretty much requires an equal amount of user interaction here.

So the only real question here is whether the behavior as far as executing close(),blur() and focus() is expected behavior or not. My argument is that a content window should never be able to effect a higher privileged window in any way.

So, is this behavior indicative of a potential bug in the future? I will leave that to you guys.
(In reply to Abdulrahman Alqabandi from comment #4)
> I guess its not that we have code running as chrome, but that non-chrome
> content is accessing (or at least executing) chrome functions which is not
> supposed to be the case.

Well, no, it's executing content functions (you're using 'close' from the scope in which the JS is evaluated, ie content) which you've rebound onto chrome-privileged objects, and that then fails with something other than a security error, which is somewhat surprising. I believe the reasoning is that it's possible for web content to close windows it's opened, even when not same-content. It's surprising content.close() doesn't produce the same error as close.bind(content). On the web, "even" windowRef.close() works (irrespective of what's loaded in that window). I also verified that you can use the close rebinding from the sidebar on windows that *were* opened by script. That is:

1. open some website
2. in the web console, run e.g. w = window.open("http://www.mozilla.org/"); - you probably need to allow popups for this to work well and open a window;
3. open the sidebar bookmark from comment #0
4. run 'close.bind(content)();'

will close the tab. That's a lot of effort/specificity for a dos-type issue though...

I think it would make sense if .bind, .call, .apply and friends did a cross-compartment check on arguments, but I don't know if that would be a web-breaking change. Bobby or Boris can likely comment as to whether that's feasible.

Do the blur and focus calls actually do anything? I don't see any user-visible changes, but maybe I'm missing something?

Anyway, all of these would be Core/DOM changes... we can move the bug, but equally I'm not sure to what degree we think sidebars are special here (cf comment #5) and if there's something we should do in the Firefox front-end to disallow this. We could prevent data/js URIs from loading in the sidebar, but I don't know if that would actually accomplish anything security-wise.
Interesting example. I always assumed that only windows that opened another window (through open()) were able to close tabs. But seems like any window can close a window thats been opened by some other window. 

>Do the blur and focus calls actually do anything? I don't see any user-visible changes, but maybe I'm missing something?

I know focus() does, you can test this by reproducing the steps in Comment 0 (with focus instead of close) and just make your firefox window smaller so that you can see the scrollbars inside the newtab window. Once you execute the focus.blur immediately hit the down arrow key and you will see that you're focused in the newtab.

To test the opposite just click anywhere in the sidebar and try hitting the down arrow and you wont be focused on newtab but rather the sidebar. 

So we know these functions are being executed/accessed to an extent.
> Once you execute the focus.blur
meant focus.bind
So, here are some general points from a 100-ft view:
* As defense-in-depth, we want to avoid allowing untrusted content to get a reference to a chrome window, even though there's no security issue with that per se.
* There are additional levels of mitigation here if the this is only accessible from code executed in the bookmark sidebar. Still worth avoiding if we can though.
* Chrome Window / Location objects do not expose the usual cross-origin methods. However, as Abdulrahman correctly points you, you can still call/apply cross-origin getters to it, because methods marked with the [CrossOrigin*] attributes in webidl do an unconditional unwrap of any security wrapper around |this|. This is probably worth fixing as defense-in-depth, and wouldn't be hard - we just need to replace the UncheckedUnwrap call at [1] with a slightly-smarter helper method that fails if the helper method is not a CrossOriginXrayWrapper instance. Should be a quick patch and extremely unlikely to break anything.
* I don't think it's useful to mess around with adding security checks to bind/call/apply. The above fix should be sufficient.

In summary, the two things to do with this bug are:
* Investigate if there's any way we're exposing a chrome window reference to untrusted content, and eliminate it if it's practical.
* Tighten down the usage of cross-origin-accessible accessors per the suggestion above.

[1] http://searchfox.org/mozilla-central/rev/970569ad57ac4436ff31aa2ac63f38ed3ee2932d/dom/bindings/Codegen.py#4147
Flags: needinfo?(bobbyholley)
> slightly-smarter helper method that fails if the helper method is not a CrossOriginXrayWrapper instance.

You mean if something being unwrapped is not.

Can we just check the flags that UncheckedUnwrap (optionally) outputs, or is the state we care about not reflected in those flags?
(In reply to Boris Zbarsky [:bz] (Out June 25-July 6) from comment #10)
> > slightly-smarter helper method that fails if the helper method is not a CrossOriginXrayWrapper instance.
> 
> You mean if something being unwrapped is not.

Right.
 
> Can we just check the flags that UncheckedUnwrap (optionally) outputs, or is
> the state we care about not reflected in those flags?

It is not reflected precisely, but we could pretty easily do that. Though we could actually just check for IS_XRAY_WRAPPER_FLAG (which is reflected), which is probably good enough.

That might be more complicated than we need though. We still need to check whether the thing is a wrapper at all, since if it's not then the flags won't be there. So we basically want the code to do this:

if (IsCrossCompartmentWrapper(obj) && !IsXrayWrapper(obj)) {
  // throw
}
obj = UncheckedUnwrap(obj, /* stopAtOuter = */ false);
No, because we don't want to throw for a transparent CCW, right?

The right thing might be more like:

if (IsXrayWrapper(obj)) {
  obj = UncheckedUnwrap(obj, /* stopAtOuter = */ false);
} else {
  obj = CheckedUnwrap(obj, /* stopAtOuter = */ false);
}
if (!obj) {
  // security error, throw.
}
(In reply to Bobby Holley (busy) from comment #9)
> So, here are some general points from a 100-ft view:
> * As defense-in-depth, we want to avoid allowing untrusted content to get a
> reference to a chrome window, even though there's no security issue with
> that per se.

Does this effectively mean making window.content return null if the window it points to is chrome privileged, or am I misreading you and/or is there a better way to do this?
Group: firefox-core-security → core-security
Component: Untriaged → DOM
Flags: needinfo?(bobbyholley)
Product: Firefox → Core
(In reply to :Gijs Kruitbosch from comment #13)
> (In reply to Bobby Holley (busy) from comment #9)
> > So, here are some general points from a 100-ft view:
> > * As defense-in-depth, we want to avoid allowing untrusted content to get a
> > reference to a chrome window, even though there's no security issue with
> > that per se.
> 
> Does this effectively mean making window.content return null if the window
> it points to is chrome privileged, or am I misreading you and/or is there a
> better way to do this?

IIUC the issue here is that we have a content docshell with a chrome window which embeds a content window as an iframe. Is that right, or am I misunderstanding the situation?

In general it seems better to avoid having chrome windows in content docshells. But perhaps that's unavoidable here?
Flags: needinfo?(bobbyholley)
(In reply to Bobby Holley (busy) from comment #14)
> IIUC the issue here is that we have a content docshell with a chrome window
> which embeds a content window as an iframe. Is that right, or am I
> misunderstanding the situation?

Close, but not quite.

The sidebar in browser.xul has a <browser> (id=sidebar, no type attribute, so AIUI it ends up being a type=chrome browser - document.getElementById("sidebar").frameLoader.docShell.itemType returns 0) which loads http://searchfox.org/mozilla-central/source/browser/base/content/web-panels.xul which is a privileged page ("chrome window"). That page itself has another XUL <browser> (type=content) that loads the webpage (so yes, kind of like an iframe). The webpage that's loaded in the sidebar is content-privileged, it has the data: URI from comment #0.

However, it has a reference to the webpage loaded *in the browser for the current tab* - which is in a separate part of the document tree (ie not anywhere under the browser#sidebar I started this explanation with). Because the main tabbrowser is type=primary-content, that's what "window.content" refers to in anything under the browser.xul-based docshell tree. It is that primary-content browser that has a chrome-privileged page in, in some cases, such as about:newtab or about:preferences or about:config, etc. etc. (basically any about: page that doesn't have URI_SAFE_FOR_UNTRUSTED_CONTENT that you load in a tab).

> In general it seems better to avoid having chrome windows in content
> docshells. But perhaps that's unavoidable here?

It's just software, right? ;-)

We do this for a number of about: pages that are chrome privileged. So to avoid this, we would either:
1) make the docshell we load them in non-content
2) make the pages unprivileged/non-chrome.

For (1), changing the type attribute of the browser and reinserting it into the DOM (without which the type attribute change has no effect) whenever we load a chrome-privileged page would likely lead to functional bugs because the XBL binding state gets lost that way, so that doesn't seem very attractive. Though, to be completely fair, we have some code for this type of thing for when we switch between remote/non-remote browsers (e.g. about:preferences loads in the parent process, webpages don't, and so we do all kinds of evil switches when you back/forward through history/navigation between those things). That code has "issues", but we could potentially look into this if we thought that was actually feasible and important.

For (2), I believe people are working to make about:newtab unprivileged on and off. It hasn't been a priority compared to a lot of other things people want to change about about:newtab. Even if we did that, though, the same problem would continue for things like about:preferences, about:debugging, and about:config, which are much harder to make unprivileged.

TL;DR: this feels like it would be difficult

(In reply to Abdulrahman Alqabandi from comment #5)
> Also, the only other way I'm aware of gaining a reference to a chrome owned
> window is to perform window.open() within a content and then having the user
> manually navigate to something like 'about:newtab' in the newly opened
> window. Which would give the same results as here.

Is this at all fixable in the DOM/bindings layer? As in, can we just proxy in a null-ish object for any case where the referring item is content-privileged / in a content-privileged compartment, and the referred-to item is chrome-privileged?
Flags: needinfo?(bzbarsky)
Flags: needinfo?(bobbyholley)
(In reply to :Gijs Kruitbosch from comment #15)
> Because the main tabbrowser is type=primary-content

Err, I meant content-primary.
(In reply to :Gijs Kruitbosch from comment #15)
> TL;DR: this feels like it would be difficult

Ok - I don't think it's worth it given that this only happens within the bookmark bar. The Xray thing should probably be sufficient.
Flags: needinfo?(bobbyholley)
> Is this at all fixable in the DOM/bindings layer? 

Which "this"?

The proposal up in comment 9 to comment 12 is to make it so if you have a window reference from non-chrome stuff to a chrome-privileged window then any property access on it throws _and_ using your own DOM methods/getters/setters with it as "this" also throws.  We already do the former; we would add the latter.

Past that, I'm not sure what the proposal is, exactly, for binding stuff.
Flags: needinfo?(bzbarsky)
(In reply to Boris Zbarsky [:bz] (Out June 25-July 6) from comment #18)
> > Is this at all fixable in the DOM/bindings layer? 
> 
> Which "this"?
> 
> The proposal up in comment 9 to comment 12 is to make it so if you have a
> window reference from non-chrome stuff to a chrome-privileged window then
> any property access on it throws _and_ using your own DOM
> methods/getters/setters with it as "this" also throws.  We already do the
> former; we would add the latter.

Sounds good!
Attached patch Possible patch — — Splinter Review 
Does this fix the issue?
Flags: needinfo?(gijskruitbosch+bugs)
Comment on attachment 8764418 [details] [diff] [review]
Possible patch

Review of attachment 8764418 [details] [diff] [review]:
-----------------------------------------------------------------

lgtm. Add a mochitest-chrome in xponnect with a content iframe, and make sure you can't call/apply?

r=me with that.
Attachment #8764418 - Flags: review+
Doesn't warrant a full bounty, but I think a small one is justified for drawing our attention to the call/apply defense-in-depth issue on chrome windows.
Flags: sec-bounty?

    
  
Assignee: nobody → bzbarsky
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true

    
    

    
  
(In reply to Boris Zbarsky [:bz] (Out June 25-July 6) from comment #20)
> Created attachment 8764418 [details] [diff] [review]
> Possible patch
> 
> Does this fix the issue?

Yes.
Flags: needinfo?(gijskruitbosch+bugs)

    
    

    
  
Bobby thinks this is sec-moderate.
Keywords: sec-moderate
Group: core-security → dom-core-security
Whiteboard: btpp-active

    
    

    
  
https://hg.mozilla.org/integration/mozilla-inbound/rev/b0ed81adfff6

I don't know that we need to backport this anywhere, exactly.

    
    

    
  
https://hg.mozilla.org/mozilla-central/rev/b0ed81adfff6
Status: ASSIGNED → RESOLVED
Closed: 8 years ago

          status-firefox50:
          --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla50
Group: dom-core-security → core-security-release
Flags: sec-bounty? → sec-bounty+
Flags: qe-verify-
Whiteboard: btpp-active → [post-critsmash-triage] btpp-active

    
    

    
  
How far back did this issue go?
Flags: needinfo?(bzbarsky)
Whiteboard: [post-critsmash-triage] btpp-active → [post-critsmash-triage][adv-main50+] btpp-active

    
    

    
  
"forever", afaict.
Flags: needinfo?(bzbarsky)
Alias: CVE-2016-9070
Group: core-security-release
Component: DOM → DOM: Core & HTML

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






  

    
  







  

    

      
Attachment

      

      
      
      
      
    

    

      

        

          

            
General

            

              Creator: 



            

            
Created: 

            
Updated: 

            
Size: