1369014 - [e10s] Cannot open with drag & drop local html file(file: protocol) from Bookmarks and Explorer

Status: VERIFIED FIXED

(Core :: DOM: Security, defect, P1)

Description

[Alice0775 White]

The problem is reproducible on Nightly55 2017-05-31, but not on 2017-05-30.

Reproducible: always

Steps To Reproduce:
1. Drag local html file from Explorer
2. Drop content area of about:home

Actual Results:
Content area is blank

Security Error: Content at moz-nullprincipal:{c07267cc-c9f3-42ce-8560-82eb2eec1fd7} may not load or link to file:///D:/fuku/downloads/Bug%201368521%20-%20Selection%20invalidation%20is%20broken.html.


Expected Results:
Local html file should display properly

Comment 1

[Alice0775 White]

Regression window:
https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=2a5ce0b6f2cd2d6567e307942990329697661ac1&tochange=c95cd85e640081adf0cbe047a41bc45478bbf3d5

Regressed by: Bug 1363977

Comment 2

[Christoph Kerschbaumer [:ckerschb]]

(In reply to Alice0775 White from comment #0)
> Security Error: Content at
> moz-nullprincipal:{c07267cc-c9f3-42ce-8560-82eb2eec1fd7} may not load or
> link to
> file:///D:/fuku/downloads/Bug%201368521%20-
> %20Selection%20invalidation%20is%20broken.html.

Hm, I can not reproduce that on Linux. Potentially that's windows only. Either way, I'll have someone look at that - assigning to myself in the meantime. However, the problem is most likely caused by the NullPrincipal fallback here:

https://hg.mozilla.org/mozilla-central/rev/c95cd85e6400#l5.23

Comment 3

[Christoph Kerschbaumer [:ckerschb]]

rrh - mid air collision :-(

Comment 4

[Alice0775 White]

I can also reproduce the problem on Ubuntu16.04 x64.

Security Error: Content at moz-nullprincipal:{995c907c-de85-4e78-a7f3-474e89b75f31} may not load or link to file:///home/fuku/Desktop/Bug%201368521%20-%20Selection%20invalidation%20is%20broken.html.

So, this is not windows specific problem.

Comment 5

[Christoph Kerschbaumer [:ckerschb]]

(In reply to Alice0775 White from comment #0)
> The problem is reproducible on Nightly55 2017-05-31, but not on 2017-05-30.
> 
> Reproducible: always
> 
> Steps To Reproduce:
> 1. Drag local html file from Explorer
> 2. Drop content area of about:home

Maybe I get number 2. wrong in the STRs. How do you do that exactly? For me the file just opens fine, but if you can reproduce on Ubuntu, then I should be able to reproduce as well.

Comment 6

[Alice0775 White]

Steps To Reproduce:
1. Open about:home or any web page
2. Drag a local html file from Explorer or Desktop
3. Drop it on content area of step1

Alternate Steps To Reproduce:
1. Bookmark a local html file (file:/path/to/file.html)
2. Open about:home or any web page
3. Drag the bookmark and drop it on content area of step2

Comment 7

[Alice0775 White]

And e10s should be enabled to reproduce the problem.

Comment 8

[Kamil Jozwiak [:kjozwiak]]

Attachment: debuglog.txt

Reproducible under macOS 10.12.5 x64 as well. Used the STR from comment #7:

* visited https://pages.github.com/
* saved the page as test.html via "File -> Save Page As" (ensure you select Web Page, HTML Only)
* once you have the page saved on the desktop/file system open about:home
* drag and drop the file into the browser

You'll notice the error under the browser console which was described in comment#0. The tab will constantly show the "loading" animation until the tab has been closed. I've attached a log file that I got when running the STR in a debug version of m-c.

Platforms used:

* macOS 10.12.5 x64 - Reproduced
* Win 10 x64 Pro VM - Reproduced
* Ubuntu 16.04.2 LTS x64 - Reproduced

Comment 9

[Christoph Kerschbaumer [:ckerschb]]

Olli, I revisited the changes we made to nsDocShellTreeOwner::HandleEvent [1] and I think what we do is wrong. In particular, if you visit www.bar.com and then drag and drop a local file into the browser window, then the triggeringPrincipal is the page that is currently open, in that case bar.com. The following message is then displayed in the console:

> Security Error: Content at https://www.bar.com/ may not load or link to file:///home/ckerschb/Desktop/foo.html.

The principal that triggered the load was not the page (not bar.com), but rather the user who actually dragged and dropped the local file, right? In that case systemPrincipal would be correct in my opinion, but then we are back to the problem of the compromised content process sending a systemprincipal ...

Any suggestions on how to move forward?

[1] https://hg.mozilla.org/mozilla-central/rev/c95cd85e6400#l5.23

Comment 10

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

No, system principal coming from child process is wrong.
When moving from non-file to file-process, perhaps there could be some special code for that?
Perhaps Bob has an opinion.

Comment 11

[Christoph Kerschbaumer [:ckerschb]]

Tooru, it seems you implemented Bug 92737. I think we could use a little bit of your help. In particular, how is DnD supposed to work? What we would like to do is, pass the right principal that triggered to load to (ulimately) LoadURI[WithOptions] within nsDocshell. Now we shouldn't be using the systemPrincipal when going from child to parent process and using a NullPrincipal causes this regression. Could you maybe define the steps involved in DnD and we can take it from there?

Comment 12

[Tooru Fujisawa [:arai]]

I'll post the detail tonight.

Comment 13

[Tooru Fujisawa [:arai]]

Briefly, when items are dropped onto remote content and the event is not prevented by web content, the items are converted into links, and those links are sent to parent process, and opened into existing or newly opened tabs.

Then, about principals and compromised child process issue, I thought dropped links (URLs, text of URLs, or files) should be opened separately from the current browser content's privilege.  (of course it means compromised child process can open any local files in current or new tab tho...)  maybe I'm misunderstanding what should happen in that case?


Anyway, here's the list of things that happen when items are dropped onto remote content

[dragover event on remote content]

1. nsDocShellTreeOwner::HandleEvent with eventType="dragover"

https://dxr.mozilla.org/mozilla-central/rev/bdb2387396b4a74dfefb7c983733eed3625e906a/docshell/base/nsDocShellTreeOwner.cpp#977-979
> NS_IMETHODIMP
> nsDocShellTreeOwner::HandleEvent(nsIDOMEvent* aEvent)
> {
> ...
>     nsAutoString eventType;
>     aEvent->GetType(eventType);
>     if (eventType.EqualsLiteral("dragover")) {

2. check if the links can be handled

https://dxr.mozilla.org/mozilla-central/rev/bdb2387396b4a74dfefb7c983733eed3625e906a/docshell/base/nsDocShellTreeOwner.cpp#980-981
> NS_IMETHODIMP
> nsDocShellTreeOwner::HandleEvent(nsIDOMEvent* aEvent)
> {
> ...
>       bool canDropLink = false;
>       handler->CanDropLink(dragEvent, false, &canDropLink);

https://dxr.mozilla.org/mozilla-central/rev/bdb2387396b4a74dfefb7c983733eed3625e906a/dom/base/contentAreaDropListener.js#134
> ContentAreaDropListener.prototype =
> {
> ...
>   canDropLink: function(aEvent, aAllowSameDocument)
>   {

3. preventDefault the event if canDropLink

https://dxr.mozilla.org/mozilla-central/rev/bdb2387396b4a74dfefb7c983733eed3625e906a/docshell/base/nsDocShellTreeOwner.cpp#982-983
> NS_IMETHODIMP
> nsDocShellTreeOwner::HandleEvent(nsIDOMEvent* aEvent)
> {
> ...
>       if (canDropLink) {
>         aEvent->PreventDefault();


[drop event on remote content]

1. nsDocShellTreeOwner::HandleEvent with eventType="drop"

https://dxr.mozilla.org/mozilla-central/rev/bdb2387396b4a74dfefb7c983733eed3625e906a/docshell/base/nsDocShellTreeOwner.cpp#985
> NS_IMETHODIMP
> nsDocShellTreeOwner::HandleEvent(nsIDOMEvent* aEvent)
> {
> ...
>     } else if (eventType.EqualsLiteral("drop")) {

2. get the list of handlable links from dropped item

https://dxr.mozilla.org/mozilla-central/rev/bdb2387396b4a74dfefb7c983733eed3625e906a/docshell/base/nsDocShellTreeOwner.cpp#986-991
> NS_IMETHODIMP
> nsDocShellTreeOwner::HandleEvent(nsIDOMEvent* aEvent)
> {
> ...
>       nsIWebNavigation* webnav = static_cast<nsIWebNavigation*>(mWebBrowser);
> 
>       uint32_t linksCount;
>       nsIDroppedLinkItem** links;
>       if (webnav &&
>           NS_SUCCEEDED(handler->DropLinks(dragEvent, true, &linksCount, &links))) {

https://dxr.mozilla.org/mozilla-central/rev/bdb2387396b4a74dfefb7c983733eed3625e906a/dom/base/contentAreaDropListener.js#189-212
> ContentAreaDropListener.prototype =
> {
> ...
>   dropLinks: function(aEvent, aDisallowInherit, aCount)
>   {
> ...
>     return links;
>   },

3. if there's one or more links that can be handled

3a. if it's Firefox's tab

https://dxr.mozilla.org/mozilla-central/rev/bdb2387396b4a74dfefb7c983733eed3625e906a/docshell/base/nsDocShellTreeOwner.cpp#992-996
> NS_IMETHODIMP
> nsDocShellTreeOwner::HandleEvent(nsIDOMEvent* aEvent)
> {
> ...
>         if (linksCount >= 1) {
>           nsCOMPtr<nsIWebBrowserChrome> webBrowserChrome = GetWebBrowserChrome();
>           if (webBrowserChrome) {
>             nsCOMPtr<nsITabChild> tabChild = do_QueryInterface(webBrowserChrome);
>             if (tabChild) {

3a.1. pass links to parent process to handle links

all links are handled in parent process

https://dxr.mozilla.org/mozilla-central/rev/bdb2387396b4a74dfefb7c983733eed3625e906a/docshell/base/nsDocShellTreeOwner.cpp#1009,1010
> NS_IMETHODIMP
> nsDocShellTreeOwner::HandleEvent(nsIDOMEvent* aEvent)
> {
> ...
>               nsresult rv = tabChild->RemoteDropLinks(linksCount, links,
>                                                       triggeringPrincipal);

3a.2. send links to parent process

https://dxr.mozilla.org/mozilla-central/rev/bdb2387396b4a74dfefb7c983733eed3625e906a/dom/ipc/TabChild.cpp#780
> NS_IMETHODIMP
> TabChild::RemoteDropLinks(uint32_t aLinksCount,
>                           nsIDroppedLinkItem** aLinks,
>                           nsIPrincipal* aTriggeringPrincipal)
> {
> ...
>   bool sent = SendDropLinks(linksArray, triggeringPrincipalInfo);

3a.3. pass links to browser

https://dxr.mozilla.org/mozilla-central/rev/bdb2387396b4a74dfefb7c983733eed3625e906a/dom/ipc/PBrowser.ipdl#199
> nested(upto inside_cpow) sync protocol PBrowser
> {
> ...
>     async DropLinks(nsString[] aLinks, PrincipalInfo aTriggeringPrincipalInfo);

https://dxr.mozilla.org/mozilla-central/rev/bdb2387396b4a74dfefb7c983733eed3625e906a/dom/ipc/TabParent.cpp#533
> mozilla::ipc::IPCResult
> TabParent::RecvDropLinks(nsTArray<nsString>&& aLinks,
>                          const PrincipalInfo& aTriggeringPrincipalInfo)
> {
> ...
>     browser->DropLinks(aLinks.Length(), links.get(), triggeringPrincipal);

3a.4. pass links to handler

https://dxr.mozilla.org/mozilla-central/rev/bdb2387396b4a74dfefb7c983733eed3625e906a/dom/interfaces/base/nsIBrowser.idl#30-32
>   void dropLinks(in unsigned long linksCount,
>                  [array, size_is(linksCount)] in wstring links,
>                  in nsIPrincipal aTriggeringPrincipal);

https://dxr.mozilla.org/mozilla-central/rev/bdb2387396b4a74dfefb7c983733eed3625e906a/toolkit/content/widgets/browser.xml#1494
>       <method name="dropLinks">
>         <parameter name="aLinksCount"/>
>         <parameter name="aLinks"/>
>         <parameter name="aTriggeringPrincipal"/>
>         <body><![CDATA[
> ...
>           this.droppedLinkHandler(null, links, aTriggeringPrincipal);

3a.5. load tabs for the links

https://dxr.mozilla.org/mozilla-central/rev/bdb2387396b4a74dfefb7c983733eed3625e906a/browser/base/content/browser.js#5918-5924
> function handleDroppedLink(event, urlOrLinks, nameOrTriggeringPrincipal, triggeringPrincipal) {
> ...
>       gBrowser.loadTabs(urls, {
>         inBackground,
>         replace: true,
>         allowThirdPartyFixup: false,
>         postDatas,
>         userContextId,
>         triggeringPrincipal,
>       });

3a.6. load the 1st link in current browser

https://dxr.mozilla.org/mozilla-central/rev/bdb2387396b4a74dfefb7c983733eed3625e906a/browser/base/content/tabbrowser.xml#1683
>       <method name="loadTabs">
>         <parameter name="aURIs"/>
>         <parameter name="aLoadInBackground"/>
>         <parameter name="aReplace"/>
>         <body><![CDATA[
> ...
>               browser.loadURIWithFlags(aURIs[0], {
>                 flags, postData: aPostDatas[0],
>                 triggeringPrincipal: aTriggeringPrincipal,
>               });

3a.7. add tabs for links except the 1st one

https://dxr.mozilla.org/mozilla-central/rev/bdb2387396b4a74dfefb7c983733eed3625e906a/browser/base/content/tabbrowser.xml#1707-1717
>       <method name="loadTabs">
>         <parameter name="aURIs"/>
>         <parameter name="aLoadInBackground"/>
>         <parameter name="aReplace"/>
>         <body><![CDATA[
> ...
>           for (let i = 1; i < aURIs.length; ++i) {
>             let tab = this.addTab(aURIs[i], {
>               skipAnimation: true,
>               allowThirdPartyFixup: aAllowThirdPartyFixup,
>               postData: aPostDatas[i],
>               userContextId: aUserContextId,
>               triggeringPrincipal: aTriggeringPrincipal,
>             });
>             if (targetTabIndex !== -1)
>               this.moveTabTo(tab, ++tabNum);
>           }

3a.8. load the link in newly created browser

https://dxr.mozilla.org/mozilla-central/rev/bdb2387396b4a74dfefb7c983733eed3625e906a/browser/base/content/tabbrowser.xml#2543-2550
>       <method name="addTab">
>         <parameter name="aURI"/>
>         <parameter name="aReferrerURI"/>
>         <parameter name="aCharset"/>
>         <parameter name="aPostData"/>
>         <parameter name="aOwner"/>
>         <parameter name="aAllowThirdPartyFixup"/>
>         <body>
>           <![CDATA[
> ...
>                 b.loadURIWithFlags(aURI, {
>                   flags,
>                   triggeringPrincipal: aTriggeringPrincipal,
>                   referrerURI: aNoReferrer ? null : aReferrerURI,
>                   referrerPolicy: aReferrerPolicy,
>                   charset: aCharset,
>                   postData: aPostData,
>                 });


3b. if it's not Firefox's tab

links are dropped onto XUL browser/iframe or some embedding

3b.1. load the 1st link

the 1st link is handled in content process,
and remaining links are just ignored

https://dxr.mozilla.org/mozilla-central/rev/bdb2387396b4a74dfefb7c983733eed3625e906a/docshell/base/nsDocShellTreeOwner.cpp#1018-1024
> NS_IMETHODIMP
> nsDocShellTreeOwner::HandleEvent(nsIDOMEvent* aEvent)
> {
> ...
>           nsAutoString url;
>           if (NS_SUCCEEDED(links[0]->GetUrl(url))) {
>             if (!url.IsEmpty()) {
>               webnav->LoadURI(url.get(), 0, nullptr, nullptr, nullptr,
>                               nsContentUtils::GetSystemPrincipal());

Comment 14

[Bob Owen (:bobowen)]

(In reply to Olli Pettay [:smaug] from comment #10)
> No, system principal coming from child process is wrong.
> When moving from non-file to file-process, perhaps there could be some
> special code for that?
> Perhaps Bob has an opinion.

I'm not too sure over all the ins and outs of triggering principals.

From Tooru's comments it seems that the child should be involved in the decision, so what I did for bug 1175267 (where I bypass the child altogether) is possibly wrong.
Not totally sure if the child is bypassed when they are dropped on the actual content.

I would have thought that the triggering principal should be the same as if the URL had just been typed into a tab.

The current mechanism seems wrong where the child can always send up messages to perform this sort of operation, it seems like it should be more some sort of request from the parent to the child that contains all of the information about the operation it needs to make a decision.
The child would then "decide" if the operation should continue and send a message, perhaps with some sort of token passed down from the parent to allow the operation to continue.
That way the child wouldn't be able to arbitrarily trigger these sorts of things.

By the way, this reminds me that I think billm said in Hawaii that the check for http content linking to file content was in the child, so I think a compromised process might be able to cause file:// URIs to be opened anyway.
If this is still true we should get a bug filed on this.
At least with the separate file content process that load won't happen in the same process.

Comment 15

[Tooru Fujisawa [:arai]]

so, maybe we should handle drop event also on parent process, in the following places (there we just ignore those remote events currently), and wait for the content process to see if the web content handles those events (if it calls preventDefault), and handle those links in parent process if not handled in web content?

I'm not sure if those things can be done asynchronously tho (since they're event handlers...)


https://dxr.mozilla.org/mozilla-central/rev/bdb2387396b4a74dfefb7c983733eed3625e906a/toolkit/content/widgets/browser.xml#1568-1571
>   <binding id="browser" extends="xul:browser" role="outerdoc">
>       <handler event="dragover" group="system">
>       <![CDATA[
> ...
>         // No need to handle "dragover" in e10s, since nsDocShellTreeOwner.cpp in the child process
>         // handles that case using "@mozilla.org/content/dropped-link-handler;1" service.
>         if (this.isRemoteBrowser)
>           return;

https://dxr.mozilla.org/mozilla-central/rev/bdb2387396b4a74dfefb7c983733eed3625e906a/toolkit/content/widgets/browser.xml#1581-1584
>         // No need to handle "drop" in e10s, since nsDocShellTreeOwner.cpp in the child process
>         // handles that case using "@mozilla.org/content/dropped-link-handler;1" service.
>         if (!this.droppedLinkHandler || event.defaultPrevented || this.isRemoteBrowser)
>           return;

Comment 16

[Christoph Kerschbaumer [:ckerschb]]

Thanks for all the input, but it seems this needs a little more work. I filed Bug 1370843 to get that resolved. Within this bug we should revert changes to get the problem resolved. I suppose we just backout the changes for nsDocShellTreeOwner.

Comment 17

[Christoph Kerschbaumer [:ckerschb]]

Attachment: bug_1369014_docshelltreeowner_backout.patch

Comment 18

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

Comment on attachment 8875268 [details] [diff] [review]
bug_1369014_docshelltreeowner_backout.patch

Assuming droppedLinkHandler works ok without principal, r+

Comment 19

[Christoph Kerschbaumer [:ckerschb]]

(In reply to Olli Pettay [:smaug] from comment #18)
> Assuming droppedLinkHandler works ok without principal, r+

Yes it does. I also verified that dragging and dropping a local html file into the browser works again.

Comment 20

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/de0c74d43a7c
Do not pass explicitly pass a triggeringPrincipal within nsDocShellTreeOwner::HandleEvent. r=smaug

Comment 21

[Ryan VanderMeulen [:RyanVM]]

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

Comment 22

[Ciprian Georgiu, Desktop QA]

I have reproduce this issue with a Nightly build from 2017-05-31 using Ubuntu 16.04 x64.

Verified fixed on 55 beta 3 (20170619071723) across platforms:
- Windows 10 x64
- macOS 10.12.5
- Ubuntu 16.04 x64 LTS