1424107 - (CVE-2018-5181) Drag and dropping file: URL into new tab with rel=noopener allows opening locally stored files

Status: RESOLVED FIXED

(Core :: DOM: Copy & Paste and Drag & Drop, defect)

Description

[Abdulrahman Alqabandi]

Attachment: qw.html

User Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/62.0.3202.94 Safari/537.36

Steps to reproduce:

Tested latest nightly. Open attached PoC and follow steps.


Actual results:

We can drag and drop a file url link into a rel=noopener tab and we will end up loading local files.

Video(unlisted): https://www.youtube.com/watch?v=W2KHyfS6_7I


Expected results:

Similar to Bug 1379842, this should not be allowed. 

An argument could be made that this is intentional because you can drag and drop anchor tags from other browsers into a mozilla tab and it will open. Or drag and drop a file bookmark works. 

But I think its best not to allow links coming from normal web content to do this, one suggestion is to somehow detect dragged objects that contain file:// url that the window/document its dragged from has no access to and change this dragged objects values from the file:// url to about:blank.

According to logs, the following error occurs:
Security Error: Content at http://localhost/qw.html may not load or link to ...

I think whatever function throws this error could also change the url dragged as well as i suggested above.

Comment 1

[:Gijs (he/him)]

:arai, given that you looked at bug 1379842, can you look at this as well? Thank you!

Comment 2

[Tooru Fujisawa [:arai]]

I'll first try to block it on drop, and if it fails, I'll check if it's possible to clear the dragging item

Comment 3

[Tooru Fujisawa [:arai]]

this issue happens only with e10s.
non-e10s mode can block it on dropping the item.

I'll check the difference.

Comment 4

[Tooru Fujisawa [:arai]]

so, it should be this part.

https://searchfox.org/mozilla-central/rev/88439dc6c5b02e167032374071cdc697a056efa1/dom/base/contentAreaDropListener.js#125-132
>     let principal;
>     if (sourceNode) {
>       principal = this._getTriggeringPrincipalFromSourceNode(sourceNode);
>     } else {
>       // Use file:/// as the default uri so that drops of file URIs are always
>       // allowed.
>       principal = secMan.createCodebasePrincipal(ioService.newURI("file:///"), {});
>     }

Comment 5

[Tooru Fujisawa [:arai]]

we're checking here
https://searchfox.org/mozilla-central/rev/88439dc6c5b02e167032374071cdc697a056efa1/docshell/base/nsDocShellTreeOwner.cpp#966
we don't have source node or source principal information on content process.

now I'm looking for the way to pass similar info
https://searchfox.org/mozilla-central/rev/88439dc6c5b02e167032374071cdc697a056efa1/widget/nsGUIEventIPC.h#291-311

maybe, simpler data like "whether it's from web or not".

Comment 6

[Tooru Fujisawa [:arai]]

We could get the URL of the source node's principal here:
https://searchfox.org/mozilla-central/rev/88439dc6c5b02e167032374071cdc697a056efa1/dom/events/EventStateManager.cpp#1350-1360
like:
https://searchfox.org/mozilla-central/rev/88439dc6c5b02e167032374071cdc697a056efa1/dom/base/contentAreaDropListener.js#138-145

and pass it to content process, and check like this:
https://searchfox.org/mozilla-central/rev/88439dc6c5b02e167032374071cdc697a056efa1/dom/base/contentAreaDropListener.js#133
>     secMan.checkLoadURIStrWithPrincipal(principal, uriString, flags);

apparently some checks inside checkLoadURIStrWithPrincipal can be done only with URL, so we might be able to add
another method (checkLoadURIStrWithURI?) that receives source URI instead of principal.
https://searchfox.org/mozilla-central/rev/88439dc6c5b02e167032374071cdc697a056efa1/caps/nsScriptSecurityManager.cpp#1048
https://searchfox.org/mozilla-central/rev/88439dc6c5b02e167032374071cdc697a056efa1/caps/nsScriptSecurityManager.cpp#563


another approach would be, call checkLoadURIStrWithPrincipal on parent process side, and pass the result to content process when sending drag event,
and use it instead when checking principal.


smaug, how do you think about those approaches?
or is there simpler way to pass equivalent info? (like, passing source node itself or principal itself to content process)

Comment 7

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

One can't pass nodes across processes.

I'm not sure I understand the approaches.
https://searchfox.org/mozilla-central/rev/88439dc6c5b02e167032374071cdc697a056efa1/dom/events/EventStateManager.cpp#1350-1360 runs in parent, so which principal are you talking about?

But yes, somehow propagating the principal data around would be good. Not perfect if a child process is compromised though.

Comment 8

[Tooru Fujisawa [:arai]]

Thanks!

I'll try passing principal URI around.

Comment 9

[Tooru Fujisawa [:arai]]

looks like we can create principal from URI.
so, after passing principal's URI from parent to content process, we can create corresponding principal from it and
use it for checkLoadURIStrWithPrincipal.

https://searchfox.org/mozilla-central/rev/88439dc6c5b02e167032374071cdc697a056efa1/dom/base/contentAreaDropListener.js#131
>       principal = secMan.createCodebasePrincipal(ioService.newURI("file:///"), {});

Comment 10

[Tooru Fujisawa [:arai]]

Attachment: Pass the source node principal across content processes in drag-and-drop.

Here's the additional data flow:
  1. EventStateManager in the parent process gets the source node principal's URI
     and send it to content process via RealDragEvent
  2. the content process sets the source node principal's URI to nsIDragSession
  3. ContentAreaDropListener gets the source node principal's URI from DataTransfer (it may be better just retrieve it directly from drag session, let me know how you think)
     and create principal from it, instead of "file:///"

Can I have feedback on the above approach?

Comment 11

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

Comment on attachment 8944154 [details] [diff] [review]
Pass the source node principal across content processes in drag-and-drop.



>+static void
>+GetPrincipalURIFromDataTransfer(nsCOMPtr<nsIDOMDataTransfer>& dataTransfer,
>+                                nsCString& principalURISpec)
>+{
>+  nsCOMPtr<nsIDOMNode> sourceNode;
>+  dataTransfer->GetMozSourceNode(getter_AddRefs(sourceNode));
>+  if (!sourceNode) {
>+    return;
>+  }
>+
>+  nsCOMPtr<nsIRemoteBrowser> browser = do_QueryInterface(sourceNode);
>+  nsCOMPtr<nsIPrincipal> principal;
>+  if (browser) {
>+    browser->GetContentPrincipal(getter_AddRefs(principal));
Hmm, is this quite right? What if the dnd started from an iframe which is different origin from the
top level page?



>+  /**
>+   * This is used only if the item is dragged from the other content process
>+   * and mozSourceNode is null.
>+   *
>+   * The URI spec of the mozSourceNode's principal.  This should be used by
>+   * nsIScriptSecurityManager.checkLoadURIStrWithPrincipalURISpec in the
>+   * dropped content process.
>+   */
>+  readonly attribute DOMString mozSourceNodePrincipalURISpec;
Why is this needed? I mean, we have the [ChromeOnly] attribute in the webidl, why we need this attribute in idl too?
The attribute is accessed apparently only from JS, so webidl should be fine.

But yeah, the basic idea here looks reasonable to me.

Comment 12

[Tooru Fujisawa [:arai]]

Thank you for your feedback :D

(In reply to Olli Pettay [:smaug] from comment #11)
> >+  nsCOMPtr<nsIRemoteBrowser> browser = do_QueryInterface(sourceNode);
> >+  nsCOMPtr<nsIPrincipal> principal;
> >+  if (browser) {
> >+    browser->GetContentPrincipal(getter_AddRefs(principal));
> Hmm, is this quite right? What if the dnd started from an iframe which is
> different origin from the
> top level page?

oh, then the following check sounds also wrong:
https://searchfox.org/mozilla-central/rev/b7e3ec2468d42fa59d86c03ec7afeb209813f1d4/dom/base/contentAreaDropListener.js#140-143
>   _getTriggeringPrincipalFromSourceNode: function(aSourceNode)
>   {
>     if (aSourceNode.localName == "browser" &&
>         aSourceNode.namespaceURI == "http://www.mozilla.org/keymaster/gatekeeper/there.is.only.xul") {
>       return aSourceNode.contentPrincipal;
>     }

I'll check what actually happens and how we can retrieve iframe's principal.
(in any case, I guess we should create helper function and call it from both places)


> >+   * The URI spec of the mozSourceNode's principal.  This should be used by
> >+   * nsIScriptSecurityManager.checkLoadURIStrWithPrincipalURISpec in the
> >+   * dropped content process.
> >+   */
> >+  readonly attribute DOMString mozSourceNodePrincipalURISpec;
> Why is this needed? I mean, we have the [ChromeOnly] attribute in the
> webidl, why we need this attribute in idl too?
> The attribute is accessed apparently only from JS, so webidl should be fine.

I thought we need the same thing in both.
I'll remove idl part.

Comment 13

[Tooru Fujisawa [:arai]]

iiuc, we cannot get iframe information from parent process, so actually we should set the URI info in content process.
I'll look into how "browser" is set as source node, and if we can get iframe info at that point.

Comment 14

[Tooru Fujisawa [:arai]]

if I understand correctly, 2 drag sessions are created separately for parent process and content process.
on parent process the source node becomes browser,
and on content process the source node becomes the actual node (like "a" element).

I think we should send the principal URI from content process to parent process, if we should support iframe.
I'll look into how actual data (like, link URL) is retrieved on parent process.
if the info is sent directly from content process, I think we extend use the message to pass principal URI as well.

Comment 15

[Tooru Fujisawa [:arai]]

we may need to fix this as well
https://searchfox.org/mozilla-central/rev/b7e3ec2468d42fa59d86c03ec7afeb209813f1d4/dom/ipc/TabParent.cpp#3437-3446
>       // Using system principal here, since once the data is on parent process
>       // side, it can be handled as being from browser chrome or OS.
> 
>       // We set aHidden to false, as we don't need to worry about hiding data
>       // from content in the parent process where there is no content.
>       // XXX: Nested Content Processes may change this
>       aDataTransfer->SetDataWithPrincipalFromOtherProcess(NS_ConvertUTF8toUTF16(item.flavor()),
>                                                           variant, i,
>                                                           nsContentUtils::GetSystemPrincipal(),
>                                                           /* aHidden = */ false);

Comment 16

[Tooru Fujisawa [:arai]]

looks like content process is sending the data from here

https://searchfox.org/mozilla-central/rev/b7e3ec2468d42fa59d86c03ec7afeb209813f1d4/widget/nsDragServiceProxy.cpp#33
> nsresult
> nsDragServiceProxy::InvokeDragSessionImpl(nsIArray* aArrayTransferables,
>                                           nsIScriptableRegion* aRegion,
>                                           uint32_t aActionType)
> {
> ...
>         mozilla::Unused <<
>           child->SendInvokeDragSession(dataTransfers, aActionType, surfaceData,
>                                        stride, static_cast<uint8_t>(dataSurface->GetFormat()),
>                                        dragRect);
> ...
>   mozilla::Unused << child->SendInvokeDragSession(dataTransfers, aActionType,
>                                                   mozilla::void_t(), 0, 0, dragRect);


I'll try adding principal data

Comment 17

[Tooru Fujisawa [:arai]]

Attachment: Pass the triggering principal URI across processes in drag-and-drop.

Here's the basic flow with this patch,
where drag starts from content process and dragged to another content process:

  1. in content process,
     nsDragServiceProxy::InvokeDragSessionImpl gets sourceNode's principal,
     and sends the principal URI with SendInvokeDragSession to parent process
  2. in parent process,
     TabParent stores the received principal URI to
     TabParent.mDragPrincipalURISpec
  3. in parent process,
     while creating drag event from pointer move,
     EventStateManager::GenerateDragGesture retrieve the principal URI from
     TabParent, in the following flow
       EventStateManager::GenerateDragGesture
       => DetermineDragTargetAndDefaultData
       => nsContentAreaDragDrop::GetDragData
       => DragDataProducer::Produce
       => TabParent::AddInitialDnDDataTo
     (and TabParent.mDragPrincipalURISpec is cleared here)
     and stores the principal URI into drag session
       DoDefaultDragStart
       => nsBaseDragService::InvokeDragSessionWithSelection
       => nsBaseDragService::InvokeDragSession
  4. in parent process,
     when dragging over chrome area,
     when sourceNode is not available or the sourceNode is xul:browser,
     ContentAreaDropListener retrieves the principal URI from the data
     transfer, that retrieves the principal URI from drag session,
     and create principal from the URI,
     and check if the dropped item is loadable with the principal
  5. in parent process,
     when dragging over content area,
     EventStateManager::DispatchCrossProcessEvent retrieves the principal URI
     form the drag session and sends the principal URI with SendRealDragEvent
     to content process
  6. in content process
     TabChild stores the received principal URI to drag session
  7. in content process,
     when dragging over content area,
     same as step 4,
     when sourceNode is not available or the sourceNode is xul:browser,
     ContentAreaDropListener retrieves the principal URI from the data
     transfer, that retrieves the principal URI from drag session,
     and create principal from the URI,
     and check if the dropped item is loadable with the principal

When the drag starts from outside of Firefox, or from browser chrome,
the principal URI is empty, and it fallbacks to system or file principal,
same as before.


Then, I have question about the last part.
Currently both system principal and file principal are used as a fallback,
for the case item is dragged from outside of the browser.
can we use only one of them? (like, use file principal everywhere)

https://searchfox.org/mozilla-central/rev/97cb0aa64ae51adcabff76fb3b5eb18368f5f8ab/dom/base/contentAreaDropListener.js#129-131
>       // Use file:/// as the default uri so that drops of file URIs are always
>       // allowed.
>       principal = secMan.createCodebasePrincipal(ioService.newURI("file:///"), {});

https://searchfox.org/mozilla-central/rev/97cb0aa64ae51adcabff76fb3b5eb18368f5f8ab/dom/base/contentAreaDropListener.js#154-158
>     // Bug 1367038: mozSourceNode is null if the drag event originated
>     // in an external application - needs better fallback!
>     let secMan = Cc["@mozilla.org/scriptsecuritymanager;1"].
>                    getService(Ci.nsIScriptSecurityManager);
>     return secMan.getSystemPrincipal();

https://searchfox.org/mozilla-central/rev/97cb0aa64ae51adcabff76fb3b5eb18368f5f8ab/dom/ipc/TabParent.cpp#3437-3446
>       // Using system principal here, since once the data is on parent process
>       // side, it can be handled as being from browser chrome or OS.
> ...
>       aDataTransfer->SetDataWithPrincipalFromOtherProcess(NS_ConvertUTF8toUTF16(item.flavor()),
>                                                           variant, i,
>                                                           nsContentUtils::GetSystemPrincipal(),
>                                                           /* aHidden = */ false);

https://searchfox.org/mozilla-central/rev/97cb0aa64ae51adcabff76fb3b5eb18368f5f8ab/dom/ipc/TabParent.cpp#564
>       triggeringPrincipal = nsContentUtils::GetSystemPrincipal();

Comment 18

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

I'm having trouble to understand why file:/// is used. Could you check the blame to see why it was added?

Comment 19

[Tooru Fujisawa [:arai]]

It's from bug 545714,
I don't see any specific reason why it was file:///, but I also don't see any reference to system principal there,
in the patch [1] or in contentAreaDropListener.js at that point. 
it looks like it's used just to allow any URL.

So, if system principal was used for the same reason in other places, I think we can use system principal everywhere.
(but I don't know if there can be some difference between them, in term of drag-and-drop)

[1] https://hg.mozilla.org/mozilla-central/rev/049064ae127ca72d93fabcd7a227ccde6e42dcce
[2] https://searchfox.org/mozilla-central/rev/cddef9fa4121185afd27ff3c19d8208cbb38b6dc/dom/base/contentAreaDropListener.js

Comment 20

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

Comment on attachment 8946528 [details] [diff] [review]
Pass the triggering principal URI across processes in drag-and-drop.


>+      if (sourceNode.nodePrincipal)
>+        return sourceNode.nodePrincipal;
Per mozilla coding style one should always use {} with if

>+      // TODO: Investigate and describe the difference between them,
>+      //       or use only one principal, maybe related to bug 1367038.
Yeah, I don't have good answer to this without looking all the blame leading to this code

> void
> EventStateManager::DetermineDragTargetAndDefaultData(nsPIDOMWindowOuter* aWindow,
>                                                      nsIContent* aSelectionTarget,
>                                                      DataTransfer* aDataTransfer,
>                                                      nsISelection** aSelection,
>-                                                     nsIContent** aTargetNode)
>+                                                     nsIContent** aTargetNode,
>+                                                     nsACString& principalURISpec)
aPrincipalURISpec



>   /**
>+   * The URI spec of the triggering principal.  This may differ than
this may be different than

>+   * sourceNode's principal when sourceNode is xul:browser and the drag is
>+   * triggered from a frame inside it.
>+   */
>+  [ChromeOnly]
>+  readonly attribute DOMString mozTriggeringPrincipalURISpec;
>+
>+  /**






>+static void
>+GetPrincipalURIFromNode(nsCOMPtr<nsIDOMNode>& sourceNode,
>+                        nsCString& principalURISpec)
aSourceNode, aPrincipalURISpec

>+{
>+  nsCOMPtr<nsINode> node = do_QueryInterface(sourceNode);
>+  if (!node) {
>+    return;
>+  }
>+
>+  nsCOMPtr<nsIPrincipal> principal = node->NodePrincipal();
>+  if (!principal) {
>+    return;
>+  }
NodePrincipal() returns always non-null value, as the method name hints (no Get* prefix)



>+    * @param  aPrincipalURISpec - the URI of the triggering principal of the
>+    *            drag, or an empty string if it's from browser chrome or OS
>     * @param  aTransferables - an array of transferables to be dragged
>     * @param  aRegion - a region containing rectangles for cursor feedback, 
>     *            in window coordinates.
>     * @param  aActionType - specified which of copy/move/link are allowed
>     * @param  aContentPolicyType - the contentPolicyType that will be
>     *           passed to the loadInfo when creating a new channel
>     *           (defaults to TYPE_OTHER)
>     */
>   void invokeDragSession (in nsIDOMNode aDOMNode,
>+                          in AUTF8String aPrincipalURISpec,
>                           in nsIArray aTransferables, 
>                           in nsIScriptableRegion aRegion,
>                           in unsigned long aActionType,
>                           [optional] in nsContentPolicyType aContentPolicyType);
> 
>   /**
>    * Starts a modal drag session using an image. The first four arguments are
>    * the same as invokeDragSession.
>@@ -86,30 +89,32 @@ interface nsIDragService : nsISupports
>   void invokeDragSessionWithImage(in nsIDOMNode aDOMNode,
>                                   in nsIArray aTransferableArray,
>                                   in nsIScriptableRegion aRegion,
>                                   in unsigned long aActionType,
>                                   in nsIDOMNode aImage,
>                                   in long aImageX,
>                                   in long aImageY,
>                                   in nsIDOMDragEvent aDragEvent,
>-                                  in nsIDOMDataTransfer aDataTransfer);
>+                                  in nsIDOMDataTransfer aDataTransfer,
>+                                  in AUTF8String aPrincipalURISpec);
> 
>   /**
>    * Start a modal drag session using the selection as the drag image.
>    * The aDragEvent must be supplied as the current screen coordinates of the
>    * event are needed to calculate the image location.
>    *
>    * Note: This method is deprecated for non-native code.
>    */
>   void invokeDragSessionWithSelection(in nsISelection aSelection,
>                                       in nsIArray aTransferableArray,
>                                       in unsigned long aActionType,
>                                       in nsIDOMDragEvent aDragEvent,
>-                                      in nsIDOMDataTransfer aDataTransfer);
>+                                      in nsIDOMDataTransfer aDataTransfer,
>+                                      in AUTF8String aPrincipalURISpec);

tiny bit weird that in the  latter two cases aPrincipalURISpec is the last param but in the first one it is the second param.
Perhaps second param in all the cases?

> 
>   /**
>     * The dom node that was originally dragged to start the session, which will be null if the
>     * drag originated outside the application.
>     */
>   readonly attribute nsIDOMNode sourceNode;
> 
>   /**
>+   * The URI spec of the triggering principal.  This may differ than
This may differ from

>+   * sourceNode's principal when sourceNode is xul:browser and the drag is
>+   * triggered from a frame inside it.
frame?
drag is triggered in a browsing context inside it

Comment 21

[Tooru Fujisawa [:arai]]

Thank you for reviewing :)
I'll land it tomorrow.

Comment 22

[Tooru Fujisawa [:arai]]

Attachment: Pass the triggering principal URI across processes in drag-and-drop. r=smaug

[Security approval request comment]
> How easily could an exploit be constructed based on the patch?
Maybe easy, but this issue itself is not so much critical.
Attacker can open local file in tab content, if user drags a crafted link into new tab content area.
(I'm not sure what actually can be done with it tho)

> Do comments in the patch, the check-in comment, or tests included in the
> patch paint a bulls-eye on the security problem?
Not directly.

> Which older supported branches are affected by this flaw?
Possibly all,
it has been exist from initial e10s drag-and-drop support.

> If not all supported branches, which bug introduced the flaw?
-

> Do you have backports for the affected branches?
> If not, how different, hard to create, and risky will they be?
No.
It will be easy to backport.

> How likely is this patch to cause regressions;
> how much testing does it need?
Not likely.

Unfortunately drag and drop across processes is not easily tested in automated tests.
So some manual test might be necessary.

Comment 23

[Daniel Veditz [:dveditz]]

wrong behavior, but not particularly damaging since it's a noopener context. Given the previous now-public drag and drop bugs we don't have to keep this hidden.

Comment 24

[Al Billings [:abillings - ex-MoCo]]

Comment on attachment 8948067 [details] [diff] [review]
Pass the triggering principal URI across processes in drag-and-drop. r=smaug

Clearing sec-approval since this is a sec-low issue.

Comment 25

[Tooru Fujisawa [:arai]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/fc5cbc7e0d7ff28ed81f802a4973e2e8634cfe61
Bug 1424107 - Pass the triggering principal URI across processes in drag-and-drop. r=smaug

Comment 26

[Tiberius Oros[:tiberius_oros]]

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