1363977 - Have loadTabs() provide the correct triggeringPrincipal

Status: RESOLVED FIXED

(Core :: DOM: Security, enhancement, P2)

Comment 1

[Christoph Kerschbaumer [:ckerschb]]

Attachment: bug_1363977_provide_triggering_loadtabs.patch

Gijs, finding the right triggeringPrincipal in frontend code might be quite a challenge :-( I think it's best if we file multiple bugs with chopped down usescases, like this one.

I tried to find the right triggeringPrincipal in all the cases, but as I said, it's challenging. I was wondering if you can help me a little; here is a list of all the cases where I currently use systemPrincipal within this patch. In some of those cases we actually want to use some other principal:

* accessible/tests/mochitest/relations/test_tabbrowser.xul
* accessible/tests/mochitest/tree/test_tabbrowser.xul
XUL tests which can use systemPrincipal as the triggeringPrincipal.

* browser/base/content/browser.js
Seems like using the systemPrincipal is wrong within _delayedStartup(), we use window.arguments[8] as the triggeringPrincipal when calling loadURI, but that might not be set in all the cases we want. Any Suggestions?

* browser/base/content/sync/aboutSyncTabs.js
Using the systemPrincipal within openSelected() seems wrong, what could we use?

* browser/components/syncedtabs/TabListComponent.js
I think that might be ok to use the systemprincipal because we call _getChromeWindow, but also not completely sure.

* browser/tools/mozscreenshots/mozscreenshots/extension/configurations/Tabs.jsm
Probably OK to use SystemPrincipal because we are loading about: pages, right?

Comment 2

[:Gijs (he/him)]

(In reply to Christoph Kerschbaumer [:ckerschb] from comment #1)
> Created attachment 8867117 [details] [diff] [review]
> bug_1363977_provide_triggering_loadtabs.patch
> 
> Gijs, finding the right triggeringPrincipal in frontend code might be quite
> a challenge :-( I think it's best if we file multiple bugs with chopped down
> usescases, like this one.
> 
> I tried to find the right triggeringPrincipal in all the cases, but as I
> said, it's challenging. I was wondering if you can help me a little; here is
> a list of all the cases where I currently use systemPrincipal within this
> patch. In some of those cases we actually want to use some other principal:
> 
> * accessible/tests/mochitest/relations/test_tabbrowser.xul
> * accessible/tests/mochitest/tree/test_tabbrowser.xul
> XUL tests which can use systemPrincipal as the triggeringPrincipal.

Yes.

> * browser/base/content/browser.js
> Seems like using the systemPrincipal is wrong within _delayedStartup(), we
> use window.arguments[8] as the triggeringPrincipal when calling loadURI, but
> that might not be set in all the cases we want. Any Suggestions?

Not really - in what cases don't we have a triggering principal? Can we throw instead and actually find the cases where we don't have a triggering principal? Maybe we need to split this off into its own patch or something?

> * browser/base/content/sync/aboutSyncTabs.js
> Using the systemPrincipal within openSelected() seems wrong, what could we
> use?

I think system is right here - the user selects tabs to load, and there's no telling what URIs we sync in from other places. We should just make sure the loads happen with principal inheritance disabled. I also think about:sync-tabs is going to go away, but I don't know when... Mark might know?

> * browser/components/syncedtabs/TabListComponent.js
> I think that might be ok to use the systemprincipal because we call
> _getChromeWindow, but also not completely sure.

Yes, same as for the other sync bit.

> *
> browser/tools/mozscreenshots/mozscreenshots/extension/configurations/Tabs.jsm
> Probably OK to use SystemPrincipal because we are loading about: pages,
> right?

I don't think your reasoning works - triggering principal is about where the request to load came from, not what we're loading - but system principal is still right, mostly because this is really test code (rather than shipping code) anyway.

Comment 3

[:Gijs (he/him)]

Comment on attachment 8867117 [details] [diff] [review]
bug_1363977_provide_triggering_loadtabs.patch

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

::: browser/base/content/browser.js
@@ +1387,5 @@
>        } else {
>          // Note: loadOneOrMoreURIs *must not* be called if window.arguments.length >= 3.
>          // Such callers expect that window.arguments[0] is handled as a single URI.
> +        // same problem as above
> +        loadOneOrMoreURIs(uriToLoad, Services.scriptSecurityManager.getSystemPrincipal());

Not sure about this and the above.

@@ +2067,5 @@
>        gBrowser &&
>        gBrowser.selectedTab.pinned)
>      where = "tab";
>  
> +  let triggeringPrincipal = aEvent.target.ownerDocument.nodePrincipal;

This won't work in e10s, or if it does it'll be using the chrome document so this is really just an obfuscation of 'system principal'.

@@ +2079,5 @@
>      urls = homePage.split("|");
>      var loadInBackground = getBoolPref("browser.tabs.loadBookmarksInBackground", false);
> +    gBrowser.loadTabs(urls, {
> +      inBackground: loadInBackground,
> +      triggeringPrincipal: triggeringPrincipal,

nit: you don't need to repeat 'triggeringPrincipal' here

@@ +5998,5 @@
>          replace: true,
>          allowThirdPartyFixup: false,
>          postDatas,
>          userContextId,
> +        triggeringPrincipal: event.target.ownerDocument.nodePrincipal,

This won't work correctly in e10s, I think. We probably need to pass the principal from the content process.

::: browser/base/content/tabbrowser.xml
@@ +6948,5 @@
>              allowThirdPartyFixup: true,
>              targetTab,
>              newIndex,
>              userContextId,
> +            triggeringPrincipal: event.target.ownerDocument.nodePrincipal,

Again, this is probably system? Is that right? Can we make it explicit if so?

::: browser/components/places/PlacesUIUtils.jsm
@@ +958,5 @@
> +    browserWindow.gBrowser.loadTabs(urls, {
> +      inBackground: loadInBackground,
> +      replace: false,
> +      triggeringPrincipal: aEvent.target.ownerDocument.nodePrincipal,
> +     });

Nit: indentation.

Also, this seems likely to be another way of saying 'system' again.

Comment 4

[Mark Hammond [:markh] [:mhammond]]

(In reply to :Gijs from comment #2)
> > * browser/base/content/sync/aboutSyncTabs.js
> > Using the systemPrincipal within openSelected() seems wrong, what could we
> > use?
> 
> I think system is right here - the user selects tabs to load, and there's no
> telling what URIs we sync in from other places. We should just make sure the
> loads happen with principal inheritance disabled. I also think
> about:sync-tabs is going to go away, but I don't know when... Mark might
> know?

Probably soon, and probably by Edouard :) Edouard, do you have a bug on file?

Comment 5

[Christoph Kerschbaumer [:ckerschb]]

Attachment: bug_1363977_provide_triggering_loadtabs_tests.patch

Splitting the tests (and test code):
* accessible/tests/mochitest/relations/test_tabbrowser.xul
* accessible/tests/mochitest/tree/test_tabbrowser.xul
* browser/tools/mozscreenshots/mozscreenshots/extension/configurations/Tabs.jsm
into a separate patch.

Comment 6

[Christoph Kerschbaumer [:ckerschb]]

Attachment: bug_1363977_provide_triggering_loadtabs.patch

Gijs, here is a summary of all the changes to all the files based on your feedback:

* browser/base/content/browser.js
I filed 1365232 (blocking this one) to provide the right triggeringPrincipal within _delayedStartup().
Within BrowserGoHome() we use the SystemPrincipal explicitly. I suppose that is the homepage that a user can set, right? In that case the URI can't be influenced by the web and I would imagine using systemprincipal is correct.
Please note that since that event.target.ownerDocument.nodePrincipal is probably not working (or equal to systemprincipal) I think we can remove the argument 'aTriggeringPrincipal' from loadOneOreMoreURIs, right?
For hanldeDroppedLink could we potentially use hrefAndLinkNodeForClickEvent() and then use linkNode.ownerDocument.nodePrincipal like other code in that file does? Potentially it's the same problem though because 'event' most likely is null if dropped from content process, right? If we need to pass the triggeringPrincipal from content process, how would I do that? In other words, I don't know where handleDroppedLink() is called at all.


* browser/base/content/sync/aboutSyncTabs.js
* browser/components/syncedtabs/TabListComponent.js
As per comment 2, SystemPrincipal is correct.


* browser/base/content/tabbrowser.xml
Using systemPrincipal explictly instead of event.target.ownerDocument.nodePrincipal.


* components/places/PlacesUIUtils.jsm
Using SystemPrincipal explicitly instead of event.target.ownerDocument.nodePrincipal.

Comment 7

[:Gijs (he/him)]

Comment on attachment 8868134 [details] [diff] [review]
bug_1363977_provide_triggering_loadtabs_tests.patch

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

rs=me

Comment 8

[Edouard Oger [:eoger]]

> Probably soon, and probably by Edouard :) Edouard, do you have a bug on file?

We do now: bug 1365273, I'll probably take this later this week.

Comment 9

[:Gijs (he/him)]

Comment on attachment 8868135 [details] [diff] [review]
bug_1363977_provide_triggering_loadtabs.patch

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

Closer...

(In reply to Christoph Kerschbaumer [:ckerschb] from comment #6)
> For hanldeDroppedLink could we potentially use
> hrefAndLinkNodeForClickEvent() and then use
> linkNode.ownerDocument.nodePrincipal like other code in that file does?
> Potentially it's the same problem though because 'event' most likely is null
> if dropped from content process, right? If we need to pass the
> triggeringPrincipal from content process, how would I do that? In other
> words, I don't know where handleDroppedLink() is called at all.

https://dxr.mozilla.org/mozilla-central/source/browser/base/content/tabbrowser.xml#2215

sets it on the tabbrowser. It is then called when dropping links on non-remote browsers from here:

https://dxr.mozilla.org/mozilla-central/rev/3e166b6838931b3933ca274331f9e0e115af5cc0/toolkit/content/widgets/browser.xml#1592

and XUL <browser>s implement an nsIBrowser method called 'dropLinks' which passes no event:

https://dxr.mozilla.org/mozilla-central/rev/3e166b6838931b3933ca274331f9e0e115af5cc0/toolkit/content/widgets/browser.xml#1475

which gets called from native consumers in the e10s case, AIUI. I guess they should pass on the principal, if any. :-\

::: browser/base/content/browser.js
@@ +2090,5 @@
> +      inBackground: false,
> +      replace: true,
> +      // loadOneOrMoreURIs is only called for startup hence it's safe to
> +      // use the systemPrincipal as the triggeringPrincipal.
> +      triggeringPrincipal: Services.scriptSecurityManager.getSystemPrincipal(),

This comment is wrong - it's called from _delayedStartup, which is per-window, so there's no guarantee that that's really on startup, and it is also called from the BrowserGoHome() code above (where system principal is fine).

@@ +5985,5 @@
>          allowThirdPartyFixup: false,
>          postDatas,
>          userContextId,
> +        // need to pass triggeringPrincipal from content process
> +        triggeringPrincipal: Services.scriptSecurityManager.getSystemPrincipal(),

Like tabbrowser.xml, this should use event.mozSourceNode.

::: browser/base/content/tabbrowser.xml
@@ +6964,5 @@
>              allowThirdPartyFixup: true,
>              targetTab,
>              newIndex,
>              userContextId,
> +            triggeringPrincipal: Services.scriptSecurityManager.getSystemPrincipal(),

These are dropped links. There's a manual security check earlier on, but ideally this should use the right principal based on something on the `event`. You should be able to use `mozSourceNode` to get a principal off.

Comment 10

[Christoph Kerschbaumer [:ckerschb]]

Attachment: bug_1363977_provide_triggering_loadtabs.patch

Gijs, thanks for your help and sorry this consumes so much of your time, but our frontend code really gives me a headache sometimes :-(

(In reply to :Gijs from comment #9)
> @@ +5985,5 @@
> >          allowThirdPartyFixup: false,
> >          postDatas,
> >          userContextId,
> > +        // need to pass triggeringPrincipal from content process
> > +        triggeringPrincipal: Services.scriptSecurityManager.getSystemPrincipal(),
> 
> Like tabbrowser.xml, this should use event.mozSourceNode.

Just making sure I get this right. I see that in one case we are not passing an 'event' but in the other we are. So in the case we are passing an event we could do something like 'let triggeringPrinciapl = event.dataTransfer.mozSourceNode.ownerDocument.nodePrincipal' - but in the case we are not passing an event we should explicitly pass a triggeringPrincipal, right? That would require us to change DropLinks [1] to pass a triggeringPrincipal, and then obviously send/recv DroppedLinks, correct?

[1] https://dxr.mozilla.org/mozilla-central/source/dom/ipc/TabParent.cpp#548
 
> ::: browser/base/content/tabbrowser.xml
> @@ +6964,5 @@
> >              allowThirdPartyFixup: true,
> >              targetTab,
> >              newIndex,
> >              userContextId,
> > +            triggeringPrincipal: Services.scriptSecurityManager.getSystemPrincipal(),
> 
> These are dropped links. There's a manual security check earlier on, but
> ideally this should use the right principal based on something on the
> `event`. You should be able to use `mozSourceNode` to get a principal off.

Similar here, scrolling through the file and looking at comments it seems that mozSourceNode might be null if the drag originated in an external application. I am wondering, isn't that code really triggered by the user in the end? E.g. isn't the user explicitly dragging and dropping links here? If it is, then in my opinion the load is triggered by the user's explicit actions in which case the systemprincipal is correct. For example, if a user types https://foo.com into the address bar, then we also use systemPrincipal as the triggeringPrincipal.


Anyway, before I go ahead and make changes to Send/Recv DroppedLinks I wanted to make sure this is what you are suggesting. If so I would go ahead and implement those changes obviously, but I am not entirely sure this is what we should do.

Comment 11

[:Gijs (he/him)]

(In reply to Christoph Kerschbaumer [:ckerschb] from comment #10)
> Created attachment 8868259 [details] [diff] [review]
> bug_1363977_provide_triggering_loadtabs.patch
> 
> Gijs, thanks for your help and sorry this consumes so much of your time, but
> our frontend code really gives me a headache sometimes :-(

No worries. Security code gives me a headache sometimes, so we're even. :-)

> (In reply to :Gijs from comment #9)
> > @@ +5985,5 @@
> > >          allowThirdPartyFixup: false,
> > >          postDatas,
> > >          userContextId,
> > > +        // need to pass triggeringPrincipal from content process
> > > +        triggeringPrincipal: Services.scriptSecurityManager.getSystemPrincipal(),
> > 
> > Like tabbrowser.xml, this should use event.mozSourceNode.
> 
> Just making sure I get this right. I see that in one case we are not passing
> an 'event' but in the other we are. So in the case we are passing an event
> we could do something like 'let triggeringPrinciapl =
> event.dataTransfer.mozSourceNode.ownerDocument.nodePrincipal'

I think ... mozSourceNode.nodePrincipal ? Like, I think that property exists on every node.

> - but in the
> case we are not passing an event we should explicitly pass a
> triggeringPrincipal, right? That would require us to change DropLinks [1] to
> pass a triggeringPrincipal, and then obviously send/recv DroppedLinks,
> correct?

Yes. Could do it in a separate bug if that's easier.

> [1] https://dxr.mozilla.org/mozilla-central/source/dom/ipc/TabParent.cpp#548
>  
> > ::: browser/base/content/tabbrowser.xml
> > @@ +6964,5 @@
> > >              allowThirdPartyFixup: true,
> > >              targetTab,
> > >              newIndex,
> > >              userContextId,
> > > +            triggeringPrincipal: Services.scriptSecurityManager.getSystemPrincipal(),
> > 
> > These are dropped links. There's a manual security check earlier on, but
> > ideally this should use the right principal based on something on the
> > `event`. You should be able to use `mozSourceNode` to get a principal off.
> 
> Similar here, scrolling through the file and looking at comments it seems
> that mozSourceNode might be null if the drag originated in an external
> application. I am wondering, isn't that code really triggered by the user in
> the end? E.g. isn't the user explicitly dragging and dropping links here? If
> it is, then in my opinion the load is triggered by the user's explicit
> actions in which case the systemprincipal is correct. For example, if a user
> types https://foo.com into the address bar, then we also use systemPrincipal
> as the triggeringPrincipal.

People can use various means of essentially clickjacking to make it "easy" for the user to do this. There are already extant security checks for some of this (where the drop won't succeed based on the principal on mozSourceNode). We don't let the user click links to privileged pages on unprivileged pages, either. So I think we should restrict these drops as much as we can.

I think for external loads, we should restrict to, err, unrestricted URLs (so http(s): links but not resource: or chrome: or about: -- could probably specialcase file:, maybe).

Comment 12

[:Gijs (he/him)]

Comment on attachment 8868259 [details] [diff] [review]
bug_1363977_provide_triggering_loadtabs.patch

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

::: browser/base/content/browser.js
@@ +2088,5 @@
>    try {
> +    gBrowser.loadTabs(aURIString.split("|"), {
> +      inBackground: false,
> +      replace: true,
> +      triggeringPrincipal: Services.scriptSecurityManager.getSystemPrincipal(),

Isn't this still called from _delayedStartup? In which case, I'm not convinced this is correct (ie I don't know that we don't end up here for content _blank loads or "open x in new private window" or somesuch, in which case system principal doesn't seem correct. :-(

@@ +5978,5 @@
>      }
>      if (lastLocationChange == gBrowser.selectedBrowser.lastLocationChange) {
> +      let mozSourceNode = event.dataTransfer.mozSourceNode;
> +      let triggeringPrincipal = mozSourceNode
> +          ? mozSourceNode.ownerDocument.nodePrincipal

s/ownerDocument.//

@@ +5979,5 @@
>      if (lastLocationChange == gBrowser.selectedBrowser.lastLocationChange) {
> +      let mozSourceNode = event.dataTransfer.mozSourceNode;
> +      let triggeringPrincipal = mozSourceNode
> +          ? mozSourceNode.ownerDocument.nodePrincipal
> +          : Services.scriptSecurityManager.getSystemPrincipal();

But I would prefer something more strict here. Don't know if we *can* do something more strict, but there we are... Do you have ideas, given my previous comment?

Comment 13

[Christoph Kerschbaumer [:ckerschb]]

Attachment: bug_1363977_provide_triggering_loadtabs.patch

Gijs, thanks for your feedback. Here is a list of things that I updated for the final patch:

* The calls originating from _delayedStartup() will be handled within Bug 1365232. (loadOneOrMoreURIs() will also be handled within Bug 1365232)
* BrowserGoHome can use systemPrincipal
* I updated handleDroppedLink to receive a triggeringPrincipal:
  * in the case we have an event, we use event.dataTransfer.mozSourceNode.nodePrincipal, and
  * in the other case we pass a triggeringPrincipal all the way from nsDocShellTreeOwner where we can use the systemPrincipal as the triggeringPrincipal (simliar to the LoadURI() case right underneath in nsDocShellTreeOwner).
* Only remaining question: What do we do in tabbrowser.xml? Can we keep what we have in this bug? Ultimately, whatever security checks we already provide is great. If we need addtional security checks, like in case when dropping links from an external application, we should do that in a separate patch, unless you want it to happen in this bug. What do you think?

Comment 14

[:Gijs (he/him)]

Comment on attachment 8870015 [details] [diff] [review]
bug_1363977_provide_triggering_loadtabs.patch

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

Someone else should review the DOM e10s code. I think we should use the event in nsDocShellTreeOwner. I think the LoadURI() case should potentially use that, too, but that might want to be a separate bug.

::: browser/base/content/tabbrowser.xml
@@ +6983,5 @@
> +          // mozSourceNode is null if the drag event originated
> +          // in an external application.
> +          let triggeringPrincipal = dt.mozSourceNode
> +            ? dt.mozSourceNode.nodePrincipal
> +            : Services.scriptSecurityManager.getSystemPrincipal();

I agree we can leave this for a followup.

::: docshell/base/nsDocShellTreeOwner.cpp
@@ +994,5 @@
>            if (webBrowserChrome) {
>              nsCOMPtr<nsITabChild> tabChild = do_QueryInterface(webBrowserChrome);
>              if (tabChild) {
> +              nsresult rv = tabChild->RemoteDropLinks(linksCount, links,
> +                                                      nsContentUtils::GetSystemPrincipal());

You can get the principal off the source node here, right, because you have the source node on the dragEvent instance?

Comment 15

[Christoph Kerschbaumer [:ckerschb]]

(In reply to :Gijs from comment #14)
> Someone else should review the DOM e10s code. I think we should use the
> event in nsDocShellTreeOwner. I think the LoadURI() case should potentially
> use that, too, but that might want to be a separate bug.

Yes, I think so too. I'll flag smaug for review. If he is fine with using the principal on the dragEvent then I'll file a follow up to have LoaddURI() within nsDocShellTreeOwner to also use the same principal.

> ::: browser/base/content/tabbrowser.xml
> I agree we can leave this for a followup.

I filed Bug 1367038 to follow up on that.
 
> ::: docshell/base/nsDocShellTreeOwner.cpp
> You can get the principal off the source node here, right, because you have
> the source node on the dragEvent instance?

That seems to work - thanks.

Comment 16

[Christoph Kerschbaumer [:ckerschb]]

Attachment: bug_1363977_provide_triggering_loadtabs.patch

Smaug, could you please review the dom/ and docshell/ parts?

To provide some background. Ultimately we would like to set the triggeringPrincipal for a docshell loads where the load actually happens. So that within Bug 1333030 we can assert that LoadURI()[withOptions()] always receives a valid triggeringPricnipal. Hence we have to pass the right principal from frontend code all the way to the backend. Gijs already provided lots of feedback for this bug and I think we have the right solution within this patch. Gijs would prefer if a dom-peer could review those parts though. Let me know if you want me to provide any additonal information.

Comment 17

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8870396 [details] [diff] [review]
bug_1363977_provide_triggering_loadtabs.patch


>-TabParent::RecvDropLinks(nsTArray<nsString>&& aLinks)
>+TabParent::RecvDropLinks(nsTArray<nsString>&& aLinks,
>+                         const PrincipalInfo& aTriggeringPrincipalInfo)
> {
>   nsCOMPtr<nsIBrowser> browser = do_QueryInterface(mFrameElement);
>   if (browser) {
>     UniquePtr<const char16_t*[]> links;
>     links = MakeUnique<const char16_t*[]>(aLinks.Length());
>     for (uint32_t i = 0; i < aLinks.Length(); i++) {
>       links[i] = aLinks[i].get();
>     }
>-    browser->DropLinks(aLinks.Length(), links.get());
>+    nsCOMPtr<nsIPrincipal> triggeringPrincipal =
>+      PrincipalInfoToPrincipal(aTriggeringPrincipalInfo);
>+    browser->DropLinks(aLinks.Length(), links.get(), triggeringPrincipal);
This looks a bit scary. PrincipalInfo may contain also system principal.
Compromised child process could send such principal.
Can we please return IPC_FAILED if aTriggeringPrincipalInfo is system principal.

with that, r+

Comment 18

[Christoph Kerschbaumer [:ckerschb]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=4cdb7e2b151e4657037ef9a0759e6f4ca6d606ad

Comment 19

[Christoph Kerschbaumer [:ckerschb]]

Attachment: bug_1363977_provide_triggering_loadtabs_tests.patch

Comment 20

[Christoph Kerschbaumer [:ckerschb]]

Comment on attachment 8872382 [details] [diff] [review]
bug_1363977_provide_triggering_loadtabs_tests.patch

Hey Kit, loadTabs() now receives a triggeringPrincipal as an argument. Could you please review test_TabListComponent.js to make sure I am doing the right thing? All the other tests have already been reviewed by Gijs. Thanks!

Comment 21

[Christoph Kerschbaumer [:ckerschb]]

Attachment: bug_1363977_provide_triggering_loadtabs.patch

(In reply to Olli Pettay [:smaug] from comment #17)
> >-TabParent::RecvDropLinks(nsTArray<nsString>&& aLinks)
> >+TabParent::RecvDropLinks(nsTArray<nsString>&& aLinks,
> >+                         const PrincipalInfo& aTriggeringPrincipalInfo)
> >+    browser->DropLinks(aLinks.Length(), links.get(), triggeringPrincipal);
> This looks a bit scary. PrincipalInfo may contain also system principal.
> Compromised child process could send such principal.
> Can we please return IPC_FAILED if aTriggeringPrincipalInfo is system
> principal.

While I agree with the idea behind your suggestion I ran into a problem. E.g. when running test_browser_drop.xul the mozSourceNode is null and hence we can't query the nodePrincipal off that node within nsDocShellTreeOwner. In other cases within this patch we fall back to using the systemPrincipal in that case. But also because of that we can't return IPC_FAILURE if the principal equals a systemPrincipal within RecvDropLinks. What do you think? Acceptable to land with the systemPrincipal fallback within nsDocShellTreeOwner and also not having the IPC_FAILURE within RecvDropLinks?

Comment 22

[Olli Pettay [:smaug][[e-mail]]]

I don't think we should ever trust system principal coming from child process. 
nsDocShellTreeOwner should always be able to get to some docshell with some document loaded to it.
And there is also the event target there.

Or hmm, which all data items do we need from datatransfer? 
DataTransferItem stores a principal.

Comment 23

[Lina Butler [:lina]]

Comment on attachment 8872382 [details] [diff] [review]
bug_1363977_provide_triggering_loadtabs_tests.patch

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

LGTM!

Comment 24

[Christoph Kerschbaumer [:ckerschb]]

Attachment: bug_1363977_provide_triggering_loadtabs.patch

(In reply to Olli Pettay [:smaug] from comment #22)
> Or hmm, which all data items do we need from datatransfer? 
> DataTransferItem stores a principal.

As discussed with smaug over IRC, it's probably best if we fall back to using a NullPrincipal in case we can not query a principal from mozSourceNode. Since every content process might be compromised we should never perform security checks on a received SystemPrincipal.

Hence I updated:
* HandleEvent() to fall back to a NullPrincipal if we can't query a principal from the datatransfer, and
* RecvDropLinks to return IPC_FAIL if it receives a systemprincipal as the triggeringPrincipal.

Tests pass locally, but let's make sure:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=73aaf47865ee6f786da9f5bd70a3d338c35a1703

Comment 25

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/316c45d626f1
Have loadTabs() tests provide the correct triggeringPrincipal. r=gijs,kit
https://hg.mozilla.org/integration/mozilla-inbound/rev/c95cd85e6400
Have loadTabs() provide the correct triggeringPrincipal. r=gijs,smaug

Comment 26

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/316c45d626f1
https://hg.mozilla.org/mozilla-central/rev/c95cd85e6400