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)

58 Branch
defect
Not set
normal

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)

Attached file 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.
:arai, given that you looked at bug 1379842, can you look at this as well? Thank you!
Flags: needinfo?(arai.unmht)
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)
Flags: needinfo?(arai.unmht)
this issue happens only with e10s. non-e10s mode can block it on dropping the item. I'll check the difference.
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:///"), {}); > }
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".
Flags: needinfo?(arai.unmht)
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)
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)
Thanks! I'll try passing principal URI around.
Flags: needinfo?(arai.unmht)
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:///"), {});
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 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+
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.
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.
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.
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);
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
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)
I'm having trouble to understand why file:/// is used. Could you check the blame to see why it was added?
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 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+
Thank you for reviewing :) I'll land it tomorrow.
Group: firefox-core-security → core-security
Component: Untriaged → Drag and Drop
Product: Firefox → Core
[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+
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 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?
https://hg.mozilla.org/integration/mozilla-inbound/rev/fc5cbc7e0d7ff28ed81f802a4973e2e8634cfe61 Bug 1424107 - Pass the triggering principal URI across processes in drag-and-drop. r=smaug
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla60
Depends on: 1437118
Blocks: 1437405
Whiteboard: [adv-main60+]
Alias: CVE-2018-5181
Depends on: 1509384
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: