Bug 1379842 (CVE-2017-7812)

Web content can open local files by hooking drag and drop to outside of content

VERIFIED FIXED in Firefox 56

Status

()

defect
P3
normal
VERIFIED FIXED
2 years ago
a year ago

People

(Reporter: arai, Assigned: arai)

Tracking

({sec-moderate})

Trunk
mozilla57
Points:
---

Firefox Tracking Flags

(firefox-esr52 wontfix, firefox55 wontfix, firefox56 verified, firefox57 verified, firefox58 verified)

Details

(Whiteboard: [domsecurity-backlog2][adv-main56+][post-critsmash-triage])

Attachments

(2 attachments, 1 obsolete attachment)

(Assignee)

Description

2 years ago
separated from bug 1370843 and bug 1379838 (see bug 1379838 for details)
(just in case, not adding related bugs to "Blocks" to avoid hint)

Steps To Reproduce:
  1. open Nightly with e10s enabled
  2. open the attached HTML
  3. select "THIS" text in the file
  4. drag and drop the text to empty area of tabs bar

Actual result:
  a new tab for local file is opened

Expected result:
  maybe prevent it
(Assignee)

Comment 1

2 years ago
Posted file testcase
(Assignee)

Comment 2

2 years ago
maybe better describing the details here too.

web content can modify the dataTransfer in dragstart event, to put a link to local,
and when the item is dropped to outside of content, like tabs bar, the link is opened,
bypassing privilege check.
Group: core-security → dom-core-security
(Assignee)

Comment 3

2 years ago
I'm not sure how to answer to the bug 1370843 comment, without explaining bug 1370843 and this bug.

thinking about the case that items are dragged from outside of Firefox, the patch in bug 1370843 should be fine.
but if the items are dragged from web content (the case that I totally overlooked), it doesn't work as expected (actually, I'm not sure what's really expected...),
any items won't be opened since the items aren't holded in parent process.

what would be the option in security point of view?
Flags: needinfo?(ckerschb)
(Assignee)

Comment 4

2 years ago
(In reply to Tooru Fujisawa [:arai] from comment #3)
> without explaining
> bug 1370843 and this bug.

I meant bug 1379838 and this bug.
(In reply to Tooru Fujisawa [:arai] from comment #3)
> thinking about the case that items are dragged from outside of Firefox, the
> patch in bug 1370843 should be fine.
> but if the items are dragged from web content (the case that I totally
> overlooked), it doesn't work as expected (actually, I'm not sure what's
> really expected...),
> any items won't be opened since the items aren't holded in parent process.
> 
> what would be the option in security point of view?

Looking at the spec [1] it seems that modifying the data within "dragstart" is fine as long as we are not in 'drop' mode:
> If e is dragstart, set the drag data store mode to the read/write mode.
> If e is drop, set the drag data store mode to the read-only mode.

If we are dropping (which we already can detect in the parent process, right?) then we should switch to read/only mode for content, so that the following 'setData()' calls are simply ignored:
> window.addEventListener("dragstart", function(event) {
>  var dt = event.dataTransfer;
>  dt.setData("text/plain", "file:///");
> });

Ultimately, if in drop mode, then we should reuse the information we have already stored in the parent. E.g. use the data from mVerifyDropLinks from Bug 1370843.

[1] https://html.spec.whatwg.org/multipage/dnd.html#dom-datatransfer-setdata
Flags: needinfo?(ckerschb)
(Assignee)

Comment 6

2 years ago
(In reply to Christoph Kerschbaumer [:ckerschb] from comment #5)
> Looking at the spec [1] it seems that modifying the data within "dragstart"
> is fine as long as we are not in 'drop' mode:
> > If e is dragstart, set the drag data store mode to the read/write mode.
> > If e is drop, set the drag data store mode to the read-only mode.

e is the event and when "dragstart" event it won't be "drop".
so, yes, data can be modified by web content if the dragstart happens inside web content.


> If we are dropping (which we already can detect in the parent process,
> right?) then we should switch to read/only mode for content, so that the
> following 'setData()' calls are simply ignored:

which case do you mean by "we are dropping" ?


> > window.addEventListener("dragstart", function(event) {
> >  var dt = event.dataTransfer;
> >  dt.setData("text/plain", "file:///");
> > });
> 
> Ultimately, if in drop mode, then we should reuse the information we have
> already stored in the parent. E.g. use the data from mVerifyDropLinks from
> Bug 1370843.

the issue is that, "when we store what information" on parent process,
if web content can (and is allowed to) modify the information.

possible scenario would be:
  1. store not-yet-modified dataTransfer information before dragstart
  2. store possibly-modified dataTransfer information after dragstart
  3. store possibly-modified dataTransfer information before drop event

1 means we'll ignore data modified by web content.
2 and 3 would use information from possibly-compromised child process...
(In reply to Tooru Fujisawa [:arai] from comment #6)
> the issue is that, "when we store what information" on parent process,
> if web content can (and is allowed to) modify the information.

I guess I can't completely follow, but maybe I don't fully understand how it all works together. My thinking is, that if within SendRealDragEvent(), we check:
> if (aEvent.mMessage == eDrop) {
If that branch is entered, then we store all links to be dragged/dropped within a member variable on the parent, e.g. mVerifyDropLinks [1]. *AND* we also mark the datatransfer to be read only from that point on, so that a compromised child can't modify those links.
 
> possible scenario would be:
>   1. store not-yet-modified dataTransfer information before dragstart
>   2. store possibly-modified dataTransfer information after dragstart
>   3. store possibly-modified dataTransfer information before drop event
> 
> 1 means we'll ignore data modified by web content.
I think this is what we should do, at least that's how interpret the spec.

Maybe what would help me is a usecase when a child should be allowed to modify the datatransfer that doesn't end up in a drop event within the browser.

[1] https://bugzilla.mozilla.org/attachment.cgi?id=8884336&action=diff
(Assignee)

Comment 8

2 years ago
iiuc, dataTransfer is already made read-only for drop event.
(trying to modify dataTransfer there throws error, at least on web content)

then, about the timing, there are 2 cases, items are dragged from outside of Firefox, and items are dragged from inside of Firefox (maybe web content).
for former case, what we observe first would be dragover (or dragenter?), so we should store information at that point, and in that case web content cannot modify the data.
for latter case, we'll trigger dragstart, so we should store the information before running any event handler (so that the data won't be modified, but possibly empty for some case).

then, about testing, I think we cannot test every case in automated tests (if we can, we won't use the current complicated way...)
maybe it needs manual testing, and also the current testing way would become invalid (since the assumption about who can modify the data could become invalid).

smaug, can I also have your opinion here?
Flags: needinfo?(bugs)
Not sure opinion on what.

How do other browsers behave?
Flags: needinfo?(bugs)
(Assignee)

Comment 10

2 years ago
about modifying dataTransfer, Safari and Chrome use data modified by web content (in bug 1379844 test case)

about opening file:/// from web content, Safari opens Finder when I do the testcase in comment #0,
and Chrome does nothing (ignores).
(Assignee)

Comment 11

2 years ago
(In reply to Olli Pettay [:smaug] from comment #9)
> Not sure opinion on what.

I meant, whether we should allow the web content to modify the data to open them or not.
and also in any case, how we should implement it.
(Assignee)

Comment 12

2 years ago
what should I do about bug 1370843 needinfo, regarding this bug and others (bug 1379838 and bug 1379838)?
how could we go forward?
Flags: needinfo?(ckerschb)
(In reply to Tooru Fujisawa [:arai] from comment #12)
> what should I do about bug 1370843 needinfo, regarding this bug and others
> (bug 1379838 and bug 1379838)?
> how could we go forward?

I think for this bug, Chrome's behavior seems reasonable to me, can we also ignore file:/// from web content? I am not sure what the best way to implement that is though.

As for other bugs:
* bug 1379838, bug 1379838: They don't seem like super serious concerns to me (though they are valid). Anyway, I don't think it's a big concern. The most important one of the three, is definitely this one.
* bug 1370843: Once we have fixed this bug I think we can move on with bug 1370843. Feel free to remove your ni? until then.

Btw, would you be willing to implement this bug?
Flags: needinfo?(ckerschb)
(Assignee)

Comment 14

2 years ago
(In reply to Tooru Fujisawa [:arai] from comment #12)
> others (bug 1379838 and bug 1379838)?

oops, the bug number was wrong.
one of them should be bug 1379844.


(In reply to Christoph Kerschbaumer [:ckerschb] from comment #13)
> Btw, would you be willing to implement this bug?

I think I can look into this next week (I'm a bit busy this week)
Priority: -- → P3
Whiteboard: [domsecurity-backlog2]
(Assignee)

Comment 15

2 years ago
sorry for being late for working on this.

about bug 1370843, given that web content is allowed to modify the dataTransfer, I guess what we need to do is:
  1. store dataTransfer data in parent process if the drag-and-drop is from outside of web content
     (like, drag-and-drop from desktop)
  2. let web content modify dataTransfer in dragstart event, if they want
  3a. if the URLs returned from content process are same as stored, open them with system principal
      (since it's what user dropped)
  3b. if the URLs returned from content process are different than stored (or nothing is stored),
      open them with the content's principal
      (so, if the web content is not file:///, they cannot open file:///)

with that way, the testcases failed in bug 1370843 comment #25 could keep working.

about opening too many tabs, we could add limitation only to 3b case.


I'll look into this bug hopefully tonight, or tomorrow.
(Assignee)

Comment 16

2 years ago
maybe I overlooked about compromised content process issue in step 3b in previous comment.
(Assignee)

Comment 17

2 years ago
so, the issue here is that simply because we're not passing the page's principal on e10s, that results in using system principal:

https://dxr.mozilla.org/mozilla-central/rev/52285ea5e54c73d3ed824544cef2ee3f195f05e6/toolkit/content/widgets/browser.xml#1573
>           this.droppedLinkHandler(null, links);


compared to non-e10s case:

https://dxr.mozilla.org/mozilla-central/rev/52285ea5e54c73d3ed824544cef2ee3f195f05e6/browser/base/content/tabbrowser.xml#7291-7301
>           let triggeringPrincipal = dt.mozSourceNode
>             ? dt.mozSourceNode.nodePrincipal
>             : Services.scriptSecurityManager.getSystemPrincipal();
>           this.tabbrowser.loadTabs(urls, {
> ...
>             triggeringPrincipal,
>           });

so, I think it should be addressed at the same time as bug 1370843, in the way described in comment #15.

smaug, do you know if/how we can get the page's (or drag start node's) principal in parent process?
Flags: needinfo?(bugs)
I don't know and I assume no.

Also, remember that child process may lie about its principal, so we shouldn't really trust it if possible.
Flags: needinfo?(bugs)
(Assignee)

Comment 19

2 years ago
Sorry, I was mixing totally different code paths.
the code used when dropping onto tab is following, both on e10s and non-e10s:

> https://dxr.mozilla.org/mozilla-central/rev/
> 52285ea5e54c73d3ed824544cef2ee3f195f05e6/browser/base/content/tabbrowser.
> xml#7291-7301
> >           let triggeringPrincipal = dt.mozSourceNode
> >             ? dt.mozSourceNode.nodePrincipal
> >             : Services.scriptSecurityManager.getSystemPrincipal();
> >           this.tabbrowser.loadTabs(urls, {
> > ...
> >             triggeringPrincipal,
> >           });

on non-e10s, `dt.mozSourceNode` is BODY or something inside the content, and `dt.mozSourceNode.nodePrincipal` returns the page's principal,
on e10s, `dt.mozSourceNode` is browser, and `dt.mozSourceNode.nodePrincipal` returns system principal.
we should not use `dt.mozSourceNode.nodePrincipal` when it's browser (or browser's nodePrincipal should return content's principal)
(Assignee)

Comment 20

2 years ago
Looks like browser element has contentPrincipal property, and using it can block dropping file:/// from http://.

with this change, ContentAreaDropListener#_validateURI throws error while
ContentAreaDropListener#dropLinks, in drop event handler in tabbrowser,
and no tab is added when file:/// text is dropped onto tab.

I guess we need similar change for other places that uses mozSourceNode (like browser.xml), and also nsDocShellTreeOwner::HandleEvent, for the case that link is dropped onto other firefox window.


Then, I have the following question:
  1. what's the appropriate way to detect xul:browser element?
     (currently this patch is using nodeName, but I don't think it's correct way)
  2. is contentPrincipal affected by compromised content process?
Assignee: nobody → arai.unmht
Status: NEW → ASSIGNED
Attachment #8894396 - Flags: feedback?(bugs)
Check localName and namespaceURI to detect xul:browser?

contentPrincipal is affected by compromised content process, as far as I see.
Comment on attachment 8894396 [details] [diff] [review]
Check content principal when dragging and dropping from browser.

But I guess this approach does make sense.
Attachment #8894396 - Flags: feedback?(bugs)
(Assignee)

Comment 23

2 years ago
thank you for your feedback :)

so, here's what we achieve with the patch:

  1. by checking contentPrincipal, non-file:/// page cannot open file:/// page through modifying drag-and-drop dataTransfer
     (the main target of this bug)
  2. contentPrincipal cannot become system principal even if the content process is compromised
     (considering bug 1370843 etc)
  3. principal gets checked inside parent process, so compromised content process cannot affect that logic

and what we need to check:

  4. whether compromised normal (not-file) content process can return file:/// principal through contentPrincipal or not
     (so, whether we have a logic that checks the consistency between contentPrincipal and the type of the content process)

anyway, issue related to compromised content process can be left to other bug, as long as this solution can be used even after addressing it.
and looks like that's the case.  addressing compromised content process can be handled around contentPrincipal getter or somewhere near there.
(Assignee)

Comment 24

2 years ago
Added nsIDroppedLinkHandler.getTriggeringPrincipal method that handles xul:browser case, and used it everywhere, and also _getTriggeringPrincipalFromSourceNode for internal use.

with this change, the case that drag-and-drop to other window also can block file:/// from non-file:///.
Attachment #8894396 - Attachment is obsolete: true
Attachment #8895174 - Flags: review?(bugs)
Attachment #8895174 - Flags: review?(bugs) → review+
(Assignee)

Comment 25

2 years ago
Thank you for reviewing.
I'll land this shortly after try run, since it's sec-moderate

https://wiki.mozilla.org/Security/Bug_Approval_Process#Process_for_Security_Bugs
> Core-security bug fixes should just be landed by a developer without any explicit approval if:
> A) The bug has a sec-low, sec-moderate, sec-other, or sec-want rating.
(Assignee)

Comment 27

2 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/9de512d55bb4e3ba087329f6867d703e76e50a5f
Bug 1379842 - Check content principal when dragging and dropping from browser. r=smaug
https://hg.mozilla.org/mozilla-central/rev/9de512d55bb4
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla57
Please nominate this for Beta approval when you get a chance.
Flags: needinfo?(arai.unmht)
(Assignee)

Comment 30

2 years ago
Comment on attachment 8895174 [details] [diff] [review]
Check content principal when dragging and dropping from browser.

same patch can be applied to beta

Approval Request Comment
> [Feature/Bug causing the regression]

somewhat related to bug 92737, but looks like pre-existing on e10s configuration.

> [User impact if declined]

attacker can open local file from web page, with some user's action

> [Is this code covered by automated tests?]

No

> [Has the fix been verified in Nightly?]

Yes

> [Needs manual test from QE? If yes, steps to reproduce]

open attachment 8885039 [details] and follow the instruction there

> [List of other uplifts needed for the feature/fix]

none

> [Is the change risky?]

less risky

> [Why is the change risky/not risky?]

just reduces the privilege for page load.
might hit some add-on compat issue tho.

> [String changes made/needed]

None
(but there's IDL change)
Flags: needinfo?(arai.unmht)
Attachment #8895174 - Flags: approval-mozilla-beta?
Group: dom-core-security → core-security-release
Comment on attachment 8895174 [details] [diff] [review]
Check content principal when dragging and dropping from browser.

Fix a security issue. Beta56+.
Attachment #8895174 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Flags: qe-verify+
Alias: CVE-2017-7812
Whiteboard: [domsecurity-backlog2] → [domsecurity-backlog2][adv-main56+]
Whiteboard: [domsecurity-backlog2][adv-main56+] → [domsecurity-backlog2][adv-main56+][post-critsmash-triage]
I managed to reproduce this issue using STR from Comment 0, on Firefox 56.0a1 (2017-07-10), under Windows 10x64.
The issue is no longer reproducible on Firefox 56.0, Firefox 57.0b2devEd, or on Firefox 58.0a1 (2017-09-21).
Tests were performed under Windows 10x64, Ubuntu 16.04x64 and under macOS10.12.
Status: RESOLVED → VERIFIED
Flags: qe-verify+
(Assignee)

Comment 34

a year ago
ni? myself to think about esr uplift
Flags: needinfo?(arai.unmht)
(Assignee)

Comment 35

a year ago
this patch depends on bug 1363977, that depends on bug 1309049 and bug 1314254, which is huge.
I'm not sure if they can be safely uplifted, or if this patch can be modified to skip them...

(response to bug 1317023)
smaug, I think this patch is not easily uplifted to esr52, and also this patch contains IDL change.
do you think it's better investigate some more about the way to uplift?
Flags: needinfo?(arai.unmht) → needinfo?(bugs)
This is a sec-moderate only. How eager are we to fix this in esr52?
Perhaps Daniel has an opinion?

FWIW, I don't think bug 1314254 is really relevant, nor bug 1309049.
Sure, one would need to rebase patches quite a bit, but functionality shouldn't change.
Flags: needinfo?(bugs) → needinfo?(dveditz)
I'm not all that eager -- let's skip it.
Flags: needinfo?(dveditz)
Group: core-security-release
You need to log in before you can comment on or make changes to this bug.