Closed
Bug 1424107
(CVE-2018-5181)
Opened 7 years ago
Closed 7 years ago
Drag and dropping file: URL into new tab with rel=noopener allows opening locally stored files
Categories
(Core :: DOM: Copy & Paste and Drag & Drop, defect)
Tracking
()
RESOLVED
FIXED
mozilla60
Tracking | Status | |
---|---|---|
firefox60 | --- | fixed |
People
(Reporter: qab, Assigned: arai)
References
(Blocks 1 open bug)
Details
(Keywords: sec-low, Whiteboard: [adv-main60+])
Attachments
(2 files, 2 obsolete files)
392 bytes,
text/html
|
Details | |
49.58 KB,
patch
|
arai
:
review+
|
Details | Diff | Splinter Review |
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•7 years ago
|
||
:arai, given that you looked at bug 1379842, can you look at this as well? Thank you!
Flags: needinfo?(arai.unmht)
Assignee | ||
Comment 2•7 years ago
|
||
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
Assignee: nobody → arai.unmht
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true
Flags: needinfo?(arai.unmht)
Assignee | ||
Updated•7 years ago
|
Flags: needinfo?(arai.unmht)
Assignee | ||
Comment 3•7 years ago
|
||
this issue happens only with e10s.
non-e10s mode can block it on dropping the item.
I'll check the difference.
Assignee | ||
Comment 4•7 years ago
|
||
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:///"), {});
> }
Assignee | ||
Comment 5•7 years ago
|
||
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".
Assignee | ||
Updated•7 years ago
|
Flags: needinfo?(arai.unmht)
Assignee | ||
Comment 6•7 years ago
|
||
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)
Flags: needinfo?(bugs)
Comment 7•7 years ago
|
||
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.
Flags: needinfo?(bugs)
Assignee | ||
Comment 8•7 years ago
|
||
Thanks!
I'll try passing principal URI around.
Flags: needinfo?(arai.unmht)
Assignee | ||
Comment 9•7 years ago
|
||
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:///"), {});
Assignee | ||
Comment 10•7 years ago
|
||
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?
Flags: needinfo?(arai.unmht)
Attachment #8944154 -
Flags: feedback?(bugs)
Comment 11•7 years ago
|
||
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.
Attachment #8944154 -
Flags: feedback?(bugs) → feedback+
Assignee | ||
Comment 12•7 years ago
|
||
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.
Assignee | ||
Comment 13•7 years ago
|
||
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.
Assignee | ||
Comment 14•7 years ago
|
||
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.
Assignee | ||
Comment 15•7 years ago
|
||
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);
Assignee | ||
Comment 16•7 years ago
|
||
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
Assignee | ||
Comment 17•7 years ago
|
||
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();
Attachment #8944154 -
Attachment is obsolete: true
Attachment #8946528 -
Flags: review?(bugs)
Comment 18•7 years ago
|
||
I'm having trouble to understand why file:/// is used. Could you check the blame to see why it was added?
Assignee | ||
Comment 19•7 years ago
|
||
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•7 years ago
|
||
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
Attachment #8946528 -
Flags: review?(bugs) → review+
Assignee | ||
Comment 21•7 years ago
|
||
Thank you for reviewing :)
I'll land it tomorrow.
Group: firefox-core-security → core-security
Component: Untriaged → Drag and Drop
Product: Firefox → Core
Assignee | ||
Comment 22•7 years ago
|
||
[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.
Attachment #8946528 -
Attachment is obsolete: true
Attachment #8948067 -
Flags: sec-approval?
Attachment #8948067 -
Flags: review+
Comment 23•7 years ago
|
||
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.
Group: core-security
Keywords: sec-low
Comment 24•7 years ago
|
||
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.
Attachment #8948067 -
Flags: sec-approval?
Assignee | ||
Comment 25•7 years ago
|
||
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•7 years ago
|
||
bugherder |
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
status-firefox60:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla60
Updated•7 years ago
|
Whiteboard: [adv-main60+]
Updated•7 years ago
|
Alias: CVE-2018-5181
You need to log in
before you can comment on or make changes to this bug.
Description
•