Closed
Bug 1300851
Opened 6 years ago
Closed 6 years ago
nsWebShellWindow::Initialize() can create a window with an expanded principal
Categories
(Core :: DOM: Core & HTML, defect)
Core
DOM: Core & HTML
Tracking
()
RESOLVED
FIXED
mozilla51
Tracking | Status | |
---|---|---|
firefox51 | --- | fixed |
People
(Reporter: ehsan.akhgari, Assigned: ehsan.akhgari)
References
Details
Attachments
(1 file)
2.65 KB,
patch
|
bzbarsky
:
review+
|
Details | Diff | Splinter Review |
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?
Assignee | ||
Updated•6 years ago
|
Flags: needinfo?(bzbarsky)
![]() |
||
Comment 1•6 years ago
|
||
No, and no.... I'm not even quite sure how we reach this point with some random JS on the stack...
Flags: needinfo?(bzbarsky)
Assignee | ||
Comment 2•6 years ago
|
||
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.
Assignee | ||
Updated•6 years ago
|
Flags: needinfo?(bzbarsky)
![]() |
||
Comment 3•6 years ago
|
||
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)
Assignee | ||
Comment 4•6 years ago
|
||
(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)
![]() |
||
Comment 5•6 years ago
|
||
> 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)
![]() |
||
Comment 6•6 years ago
|
||
Back to people to loop in, I think you, me, bholley is pretty much it...
Comment 7•6 years ago
|
||
(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•6 years ago
|
||
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.
Assignee | ||
Comment 9•6 years ago
|
||
(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•6 years ago
|
||
> 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.
Assignee | ||
Comment 11•6 years ago
|
||
(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)
![]() |
||
Comment 12•6 years ago
|
||
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)
Assignee | ||
Comment 13•6 years ago
|
||
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
Assignee | ||
Comment 14•6 years ago
|
||
(Of course the latter part of this equation is already dealt with in bug 1300831.)
Assignee | ||
Comment 15•6 years ago
|
||
Attachment #8789091 -
Flags: review?(bzbarsky)
![]() |
||
Comment 16•6 years ago
|
||
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+
Assignee | ||
Comment 17•6 years ago
|
||
(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•6 years ago
|
||
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
Comment 19•6 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/c38a17c2a52e
Status: NEW → RESOLVED
Closed: 6 years ago
status-firefox51:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla51
Updated•3 years ago
|
Component: DOM → DOM: Core & HTML
You need to log in
before you can comment on or make changes to this bug.
Description
•