nsWebShellWindow::Initialize() can create a window with an expanded principal

RESOLVED FIXED in Firefox 51

Status

()

defect
RESOLVED FIXED
3 years ago
4 months ago

People

(Reporter: Ehsan, Assigned: Ehsan)

Tracking

unspecified
mozilla51
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox51 fixed)

Details

Attachments

(1 attachment)

Similar to bug 1300831, <http://searchfox.org/mozilla-central/source/xpfe/appshell/nsWebShellWindow.cpp#218> can create an about:blank window with an expanded principal.

We should probably make sure that we don't have an expanded principal in this case.  Boris, thoughts?  Also, any thoughts on what to do here when the subject principal _is_ expanded?
Flags: needinfo?(bzbarsky)
No, and no....  I'm not even quite sure how we reach this point with some random JS on the stack...
Flags: needinfo?(bzbarsky)
Sorry, what is the no answering here?  I'm not sure how to interpret your response.

(In reply to Boris Zbarsky [:bz] from comment #1)
> I'm not even quite sure how we reach this point with some
> random JS on the stack...

Two examples, possibly among others: nsIAppShellService.createTopLevelWindow() and nsIAppShellService.hiddenPrivateWindow/DOMWindow.
Flags: needinfo?(bzbarsky)
The "no" is answering your two questions: I have no general thoughts and no specific thoughts on what to do here if the subject is expanded...

It sounds like window.open() from expanded-principal code could land here, basically.  What principal should we give the about:blank in that situation?  I suppose we could pick one of the principals inside the expanded principal or something....
Flags: needinfo?(bzbarsky)
(In reply to Boris Zbarsky [:bz] from comment #3)
> The "no" is answering your two questions: I have no general thoughts and no
> specific thoughts on what to do here if the subject is expanded...

Sadface.  :(  Any suggestions on other people to loop in?  We need to figure out _something_ to do here.

> It sounds like window.open() from expanded-principal code could land here,
> basically.  What principal should we give the about:blank in that situation?
> I suppose we could pick one of the principals inside the expanded principal
> or something....

Hmm.  How would window.open() get to here?  If I'm reading this code correctly, that shouldn't happen.  Comment 2 is the examples of call sites we should worry about here.
Flags: needinfo?(bzbarsky)
> Any suggestions on other people to loop in?

Unfortunately, no.

> How would window.open() get to here?

Via nsWindowWatcher::OpenWindowInternal which in the "need to open a new window" (i.e. not new tab) case calls nsWindowWatcher::CreateChromeWindow, which calls nsAppStartup::CreateChromeWindow2 which calls nsIAppShell::CreateTopLevelWindow (or goes through nsXULWindow::CreateNewWindow which still ends up calling CreatetTopLevelWindow on the appshell).
Flags: needinfo?(bzbarsky)
Back to people to loop in, I think you, me, bholley is pretty much it...
(In reply to Boris Zbarsky [:bz] from comment #3)
> The "no" is answering your two questions: I have no general thoughts and no
> specific thoughts on what to do here if the subject is expanded...
> 
> It sounds like window.open() from expanded-principal code could land here,
> basically.  What principal should we give the about:blank in that situation?
> I suppose we could pick one of the principals inside the expanded principal
> or something....

Assuming that the |window| in |window.open| is non-expanded, it seems like we should use the principal of |window|, since presumably the nsEP subsumes that principal, and that's the basically the principal that the nsEP is impersonating at the moment.
That doesn't seem unreasonable, ok.

Here's a question.  When calling through a Web IDL binding, should SubjectPrincipal() just fudge things when the subject is expanded and instead us the principal of "this" or something?  I'm not talking about implementation difficulty at the moment, but just conceptually.
(In reply to Boris Zbarsky [:bz] from comment #8)
> That doesn't seem unreasonable, ok.

I'll give that a shot.

> Here's a question.  When calling through a Web IDL binding, should
> SubjectPrincipal() just fudge things when the subject is expanded and
> instead us the principal of "this" or something?  I'm not talking about
> implementation difficulty at the moment, but just conceptually.

That would be super nice!  Not sure how we would do that though.
> That would be super nice!  Not sure how we would do that though.

Well, one plausible implementation plan would go something like this:

1)  Fix bug 1297393 (this is the hard part!).
2)  Make the binding code that determines the principal to pass in munge nsEP as desired.

That avoids us having to do any work in the common (hopefully!) case when the callee does not care about the subject principal.

For the specific case of things like window.open, we could in the shorter term put this sort of logic in Xrays.  That is, for the specific case of calling an Xrayed binding method/getter/setter with nsEP as subject, we could enter the target compartment.  It has the drawback of adding yet another method of operation to Xrays, but maybe that's ok.
(In reply to Boris Zbarsky [:bz] from comment #10)
> > That would be super nice!  Not sure how we would do that though.
> 
> Well, one plausible implementation plan would go something like this:
> 
> 1)  Fix bug 1297393 (this is the hard part!).
> 2)  Make the binding code that determines the principal to pass in munge
> nsEP as desired.
> 
> That avoids us having to do any work in the common (hopefully!) case when
> the callee does not care about the subject principal.
> 
> For the specific case of things like window.open, we could in the shorter
> term put this sort of logic in Xrays.  That is, for the specific case of
> calling an Xrayed binding method/getter/setter with nsEP as subject, we
> could enter the target compartment.  It has the drawback of adding yet
> another method of operation to Xrays, but maybe that's ok.

Is the concrete advantage as a result of this work that bug 1300833 essentially won't matter any more?  If yes, it's hard to justify doing that work right now...
Flags: needinfo?(bzbarsky)
The concrete advantage is that we won't have to worry about whack-a-mole with subject principals being nsEP, because subject principals will never be nsEP.
Flags: needinfo?(bzbarsky)
I'm forking the discussion about preventing subject principals from being expanded into bug 1301225.

I found out some new information about what's going on here.

First things first, the code in comment 0, added by bug 789773, uses the subject principal or the system in case that is null.  However, nsWebShellWindow::Initialize() is called from nsAppShellService::CreateTopLevelWindow() (this happens indirectly through JustCreateTopWindow.  CreateTopLevelWindow() then goes on to call nsGlobalWindow::SetInitialPrincipalToSubject() which doesn't allow expanded or system principals for non-chrome docshells!

But that condition was last modified in bug 996069, where we were hitting this assertion <http://searchfox.org/mozilla-central/source/dom/base/nsGlobalWindow.cpp#2453>:

  // DOMWindow with nsEP is not supported, we have to make sure
  // no one creates one accidentally.
  nsCOMPtr<nsIExpandedPrincipal> nsEP = do_QueryInterface(aPrincipal);
  MOZ_RELEASE_ASSERT(!nsEP, "DOMWindow with nsEP is not supported");

Given that this assertion exists, we can definitely be sure that we never have an expanded principal here, either for chrome or content docshells.  Also similarly, we need to make sure both of these places in the code actually behave the same way.

Doing this will alleviate the expanded principal concern here, pending bug 1301225.
Assignee: nobody → ehsan
(Of course the latter part of this equation is already dealt with in bug 1300831.)
Comment on attachment 8789091 [details] [diff] [review]
Don't use expanded principals when creating the about:blank content viewer eagerly

>@@ -209,18 +209,24 @@ nsresult nsWebShellWindow::Initialize(nsIXULWindow* aParent,

Maybe assert that mDocShell->ItemType() == nsIDocShellTreeItem::typeChrome?

r=me
Attachment #8789091 - Flags: review?(bzbarsky) → review+
(In reply to Boris Zbarsky [:bz] from comment #16)
> Comment on attachment 8789091 [details] [diff] [review]
> Don't use expanded principals when creating the about:blank content viewer
> eagerly
> 
> >@@ -209,18 +209,24 @@ nsresult nsWebShellWindow::Initialize(nsIXULWindow* aParent,
> 
> Maybe assert that mDocShell->ItemType() == nsIDocShellTreeItem::typeChrome?

That's being done a few lines above, but sure.
Pushed by eakhgari@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/c38a17c2a52e
Don't use expanded principals when creating the about:blank content viewer eagerly; r=bzbarsky
https://hg.mozilla.org/mozilla-central/rev/c38a17c2a52e
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla51
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.