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

Status: RESOLVED FIXED

Description

[:Ehsan Akhgari]

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?

Comment 1

[Boris Zbarsky [:bzbarsky, bz on IRC]]

No, and no....  I'm not even quite sure how we reach this point with some random JS on the stack...

Comment 2

[:Ehsan Akhgari]

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.

Comment 3

[Boris Zbarsky [:bzbarsky, bz on IRC]]

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

Comment 4

[:Ehsan Akhgari]

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

Comment 5

[Boris Zbarsky [:bzbarsky, bz on IRC]]

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

Comment 6

[Boris Zbarsky [:bzbarsky, bz on IRC]]

Back to people to loop in, I think you, me, bholley is pretty much it...

Comment 7

[Bobby Holley (:bholley)]

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

Comment 8

[Boris Zbarsky [:bzbarsky, bz on IRC]]

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.

Comment 9

[:Ehsan Akhgari]

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

Comment 10

[Boris Zbarsky [:bzbarsky, bz on IRC]]

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

Comment 11

[:Ehsan Akhgari]

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

Comment 12

[Boris Zbarsky [:bzbarsky, bz on IRC]]

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.

Comment 13

[:Ehsan Akhgari]

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.

Comment 14

[:Ehsan Akhgari]

(Of course the latter part of this equation is already dealt with in bug 1300831.)

Comment 15

[:Ehsan Akhgari]

Attachment: Don't use expanded principals when creating the about:blank content viewer eagerly

Comment 16

[Boris Zbarsky [:bzbarsky, bz on IRC]]

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

Comment 17

[:Ehsan Akhgari]

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

Comment 18

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/c38a17c2a52e
Don't use expanded principals when creating the about:blank content viewer eagerly; r=bzbarsky

Comment 19

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/c38a17c2a52e