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

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect)

Description

[Geoff Lankow (:darktrojan)]

Attachment: 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.

Comment 1

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Jed, did this change recently for sandboxing?

Comment 2

[Jed Davis [:jld] ⟨⏰|UTC-7⟩ ⟦he/him⟧]

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.

Comment 3

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

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.

Comment 4

[Ben Turner (not reading bugmail, use the needinfo flag!)]

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).

Comment 5

[Jed Davis [:jld] ⟨⏰|UTC-7⟩ ⟦he/him⟧]

(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

Comment 6

[Jed Davis [:jld] ⟨⏰|UTC-7⟩ ⟦he/him⟧]

(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.

Comment 7

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Yeah, session restore will have to be smarter and re-send those FDs if this is a case we care about.

Comment 8

[Jed Davis [:jld] ⟨⏰|UTC-7⟩ ⟦he/him⟧]

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.

Comment 9

[Jed Davis [:jld] ⟨⏰|UTC-7⟩ ⟦he/him⟧]

(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.

Comment 10

[Jed Davis [:jld] ⟨⏰|UTC-7⟩ ⟦he/him⟧]

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.

Comment 11

[Jed Davis [:jld] ⟨⏰|UTC-7⟩ ⟦he/him⟧]

(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.

Comment 12

[Jed Davis [:jld] ⟨⏰|UTC-7⟩ ⟦he/him⟧]

Attachment: 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.

Comment 13

[Jed Davis [:jld] ⟨⏰|UTC-7⟩ ⟦he/him⟧]

Attachment: Step 2: Work around SessionStore dependency on current brokenness.

Comment 14

[Jed Davis [:jld] ⟨⏰|UTC-7⟩ ⟦he/him⟧]

Attachment: 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.

Comment 15

[(no longer active)]

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.

Comment 16

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

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.

Comment 17

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

But perhaps that isn't too bad.

Comment 18

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

Comment on attachment 8586552 [details] [diff] [review]
Step 1: Fix assorted forms mochitests for e10s-compatibility.

rs+

Comment 19

[Jed Davis [:jld] ⟨⏰|UTC-7⟩ ⟦he/him⟧]

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

Comment 20

[Jed Davis [:jld] ⟨⏰|UTC-7⟩ ⟦he/him⟧]

(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.

Comment 21

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/d1f61a3f4fc3
https://hg.mozilla.org/integration/mozilla-inbound/rev/b82726a9882e
https://hg.mozilla.org/integration/mozilla-inbound/rev/2cd7fb610499

I won't lie, https://treeherder.mozilla.org/logviewer.html#?job_id=6027122&repo=try makes me nervous, though.

Comment 22

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/d1f61a3f4fc3
https://hg.mozilla.org/mozilla-central/rev/b82726a9882e
https://hg.mozilla.org/mozilla-central/rev/2cd7fb610499