[e10s] Disallow mozSetFileNameArray (and the file input value setter) in content-process privileged scripts

RESOLVED FIXED in Firefox 40

Status

()

Core
HTML: Form Submission
RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: darktrojan, Assigned: jld)

Tracking

(Blocks: 2 bugs)

unspecified
mozilla40
x86_64
Linux
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(e10s+, firefox40 fixed)

Details

Attachments

(4 attachments)

(Reporter)

Description

3 years ago
Created attachment 8578339 [details]
test-frame.js

STR:
Load a form with a file input control.
Load the attached frame script.
Call messageManager.sendAsyncMessage("setfiles", ["/path/to/a/file"]). The value of the control should change.
Try to submit the form.

In a non-e10s browser, the form submits. In an e10s browser, a request appears in the console, but it is immediately aborted.

Updated

3 years ago
tracking-e10s: --- → ?
Jed, did this change recently for sandboxing?
Flags: needinfo?(jld)
(Assignee)

Comment 2

3 years ago
I haven't restricted mozSetFileNameArray (or the |value| setter, which should be more or less the same thing) — even in the absence of add-ons, there are still things we can't break written around the idea of the control having a “value” that's a filesystem path.  (This includes SessionStore: bug 1122855.)  The closest I got to it was bug 1068838, where I added a new method for directly setting the underlying sequence of File objects, and converted a bunch of tests to use that API and create their File objects in the chrome process instead.

Linux desktop e10s sandboxing is currently nonexistent, so it's not that either.

In any case, calling mozSetFileNameArray (and/or setting |value|) means reading an arbitrary file, so we should carefully consider whether we want to support it in e10s frame scripts.
Flags: needinfo?(jld)
Flags: needinfo?(wmccloskey)
Hmm, I'm not sure why this is failing then.

Ben, is there a way to send a DOM File object using the message manager? That would be the right way to do this. Then they could call mozSetFileArray in the frame script.
Flags: needinfo?(wmccloskey) → needinfo?(bent.mozilla)
Yeah, message manager should be able to send blobs/files already. We have tests for this kind of thing (http://mxr.mozilla.org/mozilla-central/source/dom/base/test/test_ipc_messagemanager_blob.html).
Flags: needinfo?(bent.mozilla)
(Assignee)

Comment 5

3 years ago
(In reply to Bill McCloskey (:billm) from comment #3)
> Ben, is there a way to send a DOM File object using the message manager?
> That would be the right way to do this. Then they could call mozSetFileArray
> in the frame script.

Note that mozSetFileArray landed in 38 (see bug 1068838), so code that needs to support older versions would need to check for it and fall back to setting the filename.

Also, I ran into something that's probably the same bug: trying to use an e10s file submission form that was restored from SessionStore (see, as mentioned above, bug 1122855).  The child process thinks it has a File, but this happens when trying to submit:

[Child 84786] WARNING: This file has not been opened (or could not be opened). Sending an invalid file descriptor to the other process!: file /home/jld/src/gecko-dev/netwerk/base/nsFileStreams.cpp, line 571
[Parent 84749] WARNING: Received a bad file descriptor index!: file /home/jld/src/gecko-dev/netwerk/base/nsFileStreams.cpp, line 611
(Assignee)

Comment 6

3 years ago
(In reply to Jed Davis [:jld] from comment #5)
> [Child 84786] WARNING: This file has not been opened (or could not be
> opened). Sending an invalid file descriptor to the other process!: file
> /home/jld/src/gecko-dev/netwerk/base/nsFileStreams.cpp, line 571

Some debug-by-printf suggests that it's the "has not been opened" case.
Yeah, session restore will have to be smarter and re-send those FDs if this is a case we care about.
(Assignee)

Comment 8

3 years ago
So it looks as if this might never have worked — using mozSetFileNameArray in a content process yields a File object that can be read with a FileReader but not submitted (or stored in IndexedDB).  There are tests that would notice this, but they're marked as disabled for e10s, probably because they'd fail.

It might be better to just make mozSetFileNameArray throw if it's called in a content process, rather than doing something that doesn't really work.
(Assignee)

Comment 9

3 years ago
(In reply to Jed Davis [:jld] {UTC-7} from comment #8)
> It might be better to just make mozSetFileNameArray throw if it's called in
> a content process, rather than doing something that doesn't really work.

Tests that break:

dom/html/test/test_bug143220.html — testing the file value getter; just needs a File with a path
dom/base/test/test_bug345339.html — tests that form value persists over reload; could use Blob
dom/html/test/forms/test_{max,min,pattern,required,step}_attribute.html — probably can use Blob?

So this looks doable.
(Assignee)

Comment 10

3 years ago
Because mozSetFileNameArray has always been confusingly broken in frame scripts, and fixing it is hard to justify given that we'd want to remove it at some point (as a special case of removing frame script APIs that directly access the filesystem) anyway, I'm going to repurpose this bug for disallowing it in that context.

mozSetFileArray effectively replaces it, but it needs to be documented first, and the question raised by bug 1146116 about how mozSetFileArray interacts with cross-origin wrappers should be answered.
Assignee: nobody → jld
Depends on: 1146116
Summary: [e10s] Calling mozSetFileNameArray from frame script prevents form submission → [e10s] Disallow mozSetFileNameArray (and the file input value setter) in frame scripts

Updated

3 years ago
Blocks: 984139
tracking-e10s: ? → +
(Assignee)

Comment 11

3 years ago
(In reply to Jed Davis [:jld] {UTC-7} from comment #9)
> dom/html/test/test_bug143220.html — testing the file value getter; just
> needs a File with a path

Slight problem: trying to read the full path of a remote File/Blob on a MOZ_CHILD_PERMISSIONS build (i.e., on B2G) will kill the child process and assertion-fail the parent.  This includes reading the value property of a file input element from a privileged context.  See BlobParent::RecvGetFilePath (which does the check) and BlobChild::RemoteBlobImpl::GetMozFullPathInternal (which doesn't check before calling SendGetFilePath).

I'm not sure what *should* happen in that case.  I can see why we'd want to apply the principle of least privilege to giving the child process the full path.  But… it seems strange to have the file input's value getter throw an exception (or return an empty string) in chrome context when it would return something useful (the leaf name) in content context.  On the other hand, having it return the full path in the chrome process and the leaf name in the content process strikes me as a way to get annoying bugs.
(Assignee)

Comment 12

3 years ago
Created attachment 8586552 [details] [diff] [review]
Step 1: Fix assorted forms mochitests for e10s-compatibility.

This also removes `skip-if = e10s` from the five tests that were so afflicted, including test_formSubmission.html.
Attachment #8586552 - Flags: review?(bugs)
(Assignee)

Comment 13

3 years ago
Created attachment 8586553 [details] [diff] [review]
Step 2: Work around SessionStore dependency on current brokenness.
Attachment #8586553 - Flags: review?(ttaubert)
(Assignee)

Comment 14

3 years ago
Created attachment 8586563 [details] [diff] [review]
Step 3: Disallow mozSetFileNameArray in content processes.

I wonder if it would be worth making this post an error to the Web Console, with advice about what to do instead, in addition to throwing an exception.  Right now it's just the exception, and that might not be sufficiently illuminating for add-on developers.

I'll update MDN appropriately before this lands in any case.
Attachment #8586563 - Flags: review?(ehsan)

Comment 15

3 years ago
Comment on attachment 8586563 [details] [diff] [review]
Step 3: Disallow mozSetFileNameArray in content processes.

Review of attachment 8586563 [details] [diff] [review]:
-----------------------------------------------------------------

If you want to warn, you can use nsIDocument::WarnOnceAbout.
Attachment #8586563 - Flags: review?(ehsan) → review+
Comment on attachment 8586563 [details] [diff] [review]
Step 3: Disallow mozSetFileNameArray in content processes.

This is odd. Frame script in inprocesstabchildglobal could use 
mozSetFileNameArray, but not in tabchildglobal.
But perhaps that isn't too bad.
Comment on attachment 8586552 [details] [diff] [review]
Step 1: Fix assorted forms mochitests for e10s-compatibility.

rs+
Attachment #8586552 - Flags: review?(bugs) → review+
Attachment #8586553 - Flags: review?(ttaubert) → review+
(Assignee)

Comment 19

3 years ago
I did a try run of this last week, with these three patches plus bug 1146116 rolled into one changeset: https://treeherder.mozilla.org/#/jobs?repo=try&revision=02a24d7d0d4e
Keywords: checkin-needed
(Assignee)

Comment 20

3 years ago
(In reply to Olli Pettay [:smaug] (New review requests after Easter, please, Friday and Monday are holidays in Finland) from comment #16)
> This is odd. Frame script in inprocesstabchildglobal could use 
> mozSetFileNameArray, but not in tabchildglobal.

That's a good point — the API isn't broken for the in-process case the way it is when out-of-process, but to minimize the confusion of add-on authors there should be at least a warning for the in-process case.  I think this can be a followup, and it might belong in (or related to) bug 1150395.
(Assignee)

Updated

3 years ago
Summary: [e10s] Disallow mozSetFileNameArray (and the file input value setter) in frame scripts → [e10s] Disallow mozSetFileNameArray (and the file input value setter) in content-process privileged scripts
(Assignee)

Updated

3 years ago
Blocks: 1151095
https://hg.mozilla.org/mozilla-central/rev/d1f61a3f4fc3
https://hg.mozilla.org/mozilla-central/rev/b82726a9882e
https://hg.mozilla.org/mozilla-central/rev/2cd7fb610499
Status: NEW → RESOLVED
Last Resolved: 3 years ago
status-firefox40: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla40
You need to log in before you can comment on or make changes to this bug.