Closed Bug 1329328 Opened 7 years ago Closed 7 years ago

64-bit Firefox on Win10 - Flash Downloads result in OS Permission errors

Categories

(Core Graveyard :: Plug-ins, defect)

50 Branch
x86_64
Windows 10
defect
Not set
normal

Tracking

(firefox50 affected, firefox51 wontfix, firefox52 wontfix, firefox53 wontfix, firefox54 fixed)

RESOLVED FIXED
mozilla54
Tracking Status
firefox50 --- affected
firefox51 --- wontfix
firefox52 --- wontfix
firefox53 --- wontfix
firefox54 --- fixed

People

(Reporter: jeclark, Assigned: handyman)

References

()

Details

(Whiteboard: sbwn1)

Attachments

(1 file, 4 obsolete files)

User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10_12_2) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/55.0.2883.95 Safari/537.36

Steps to reproduce:

Go here: 
http://demo.element-it.com/Examples/multipow/index.htm
Scroll down to the live example under "Parameters related to feature of view/download..."
Click Browse and choose a file (a JPG is a good choice)
Once the file uploads, there will be a small green download arrow at the bottom right of the thumbnail.  Click it.



Actual results:

Observe that on 32-bit Firefox, the download works as expected
On 64-bit Firefox, you get  "You don't have permission to save in this location.  Contact the administrator to obtain permission" for any directory.


Expected results:

The file should be saved in the default download location
This seems like an issue with the native NPAPI sandbox.
Has STR: --- → yes
OS: Unspecified → Windows 10
Hardware: Unspecified → x86_64
Component: Untriaged → Plug-ins
Product: Firefox → Core
->jimm for sandboxing triage
Flags: needinfo?(jmathies)
Flags: needinfo?(jmathies)
Whiteboard: sb?
I can reproduce this error with 64-bit Firefox 50 and Nightly 53.

Here is a direct link to the upload/download test case:

http://demo.element-it.com/Examples/multipow/flash_upload_download.html
Status: UNCONFIRMED → NEW
Ever confirmed: true
This is pretty much the same bug as bug 1284897.

First, on the STR, I do not get the "You don't have permission to save in this location." popup window... unless I change the file download location.  The default is the same as the uploaded file's folder+filename.  For this default file location, I instead get a popup window asking if I'm sure I want to overwrite.  Pressing "yes" leads it to close the popup and the plugin draws a spinner over the image.  The plugin eventually removes the spinner and draws a red exclamation mark over the image.  Mousing over the exclamation mark shows a tooltip that says "Status: Error occurred".

OTOH, if I change the file path/name to save to, then I get the behavior in the STR (a popup window with the "You don't have permission..." error message).  Like in bug 1284897, the plugin's %APPDATA% subfolder is actually fine, albeit not convenient.  So we can see that this is the same bug.

That said, bug 1284897 will not fix this bug.  The problem is the plugin's behavior.  bug 1284897 allows file access to files chosen by the user in file pickers (like the one we select the upload file in before pressing the "Upload" button in the STR).  So it would allow saving whereever the user wished... if not for the fact that the plugin has internal behavior that tries to create temp files in the same folder.  Since the user didn't select the temp files in any file picker, those file accesses are denied, so the save fails even with the fix to bug 1284897.  FYI, with the patches in bug 1284897, we no longer get the "You don't have permission..." popup -- instead we get the "Status: Error occurred" case for any path.

Specifically, I tried the STR with a file on my Desktop.  In addition to accessing that file, the plugin also tried to access:

L"\\??\\C:\\Users\\David Parks\\Desktop\\"
L"\\??\\C:\\Users\\David Parks\\Desktop\\FlashTmp0.tmp"

...both of which were denied.

The solution in bug 1284897 was designed to be an intelligent way to handle the problem of granting special access to plugins.  We figured it would automatically handle most cases but we didn't have an estimate for how many plugins would still fail.  NIing Bob so he knows we have another Flash instance that won't automatically work with our file permissions idea.  Is this bug a WONTFIX?
Flags: needinfo?(bobowencode)
David, if the FlashTmp filenames are predictable, can you permit file writes to those temporary filenames in addition to the download's real filename?
(In reply to David Parks [:handyman] from comment #4)

> The solution in bug 1284897 was designed to be an intelligent way to handle
> the problem of granting special access to plugins.  We figured it would
> automatically handle most cases but we didn't have an estimate for how many
> plugins would still fail.  NIing Bob so he knows we have another Flash
> instance that won't automatically work with our file permissions idea.  Is
> this bug a WONTFIX?

Thanks, I think any WONTFIX decision would have to come from product.

Maybe we can work around it as Chris suggests, but it would be nice to get confirmation from Adobe over the naming convention and also, any other temporary files they might attempt to write.
Flags: needinfo?(bobowencode)
> L"\\??\\C:\\Users\\David Parks\\Desktop\\"

what call are they making here? Some sort of stat call to confirm it exists?

> L"\\??\\C:\\Users\\David Parks\\Desktop\\FlashTmp0.tmp"

Can we redirect *.tmp file access to the low integrity plugin temp directory?

> confirmation from Adobe over the naming convention and also, any other temporary files they might attempt to write.

Definitely. Jeremy, is can you document a bit more what flash is trying to do here?
Flags: needinfo?(jeclark)
Sure.  I would usually have someone debug it and provide a more technical analysis, but it seemed like a clear break in behavior between the 32 and 64-bit processes and I figured it was straightforward.  I'm happy to get someone to provide a more in-depth description of what we're actually doing here.
Depends on: 1284897
Flags: needinfo?(davidp99)
Hey Jeromie,

We're pretty sure we understand what's causing this issue on our side but we just need a little info on how Flash might be creating it's temp files.  Namely, in the page referenced in comment 0, we see Flash attempt to create the temp 'save to' file for write (using CreateFileW) .  I imagine this is a "multi-part file save" process and is part of Flash... a Google search suggests that FlashTmp0.tmp is not unique to the plugin on that page.

Note that I have tried quite a few times to reproduce the folder open call (where it tried to open the Desktop folder in comment 4) and haven't seen that behavior again.  It may not have been part of general-purpose Flash temp file code.  Jeromie, maybe someone on your side can clarify whether it ever needs to open the folder?  Hacking around that would be trickier...

If Flash is using such a scheme for temp files then we can specially grant it access to files with such names.  For this, we'd have to know what Flash's temp file naming scheme is exactly.

NI to jimm so he knows whats going on.
Flags: needinfo?(davidp99) → needinfo?(jmathies)
(In reply to David Parks (dparks) [:handyman] from comment #9)
> Note that I have tried quite a few times to reproduce the folder open call
> (where it tried to open the Desktop folder in comment 4) and haven't seen
> that behavior again.  It may not have been part of general-purpose Flash
> temp file code.  Jeromie, maybe someone on your side can clarify whether it
> ever needs to open the folder?  Hacking around that would be trickier...

> L"\\??\\C:\\Users\\David Parks\\Desktop\\"
> L"\\??\\C:\\Users\\David Parks\\Desktop\\FlashTmp0.tmp"

Can we try failing the first call on the folder and succeeding the temp file open in the second to see what happens?
Flags: needinfo?(jmathies) → needinfo?(davidp99)
Whiteboard: sb? → sbwn1
(In reply to Jim Mathies [:jimm] from comment #10)
> (In reply to David Parks (dparks) [:handyman] from comment #9)
> > Note that I have tried quite a few times to reproduce the folder open call
> > (where it tried to open the Desktop folder in comment 4) and haven't seen
> > that behavior again.  It may not have been part of general-purpose Flash
> > temp file code.
> 
> Can we try failing the first call on the folder and succeeding the temp file
> open in the second to see what happens?

I meant that I no longer see the first call on the folder.  Just the temp file.  The folder call must have been due to something else, not Flash's temp file creation.
Flags: needinfo?(davidp99)
First, these patches are to be applied atop the patches in bug 1284897, which hasn't landed just yet.

In comment 11, I said that the folder accesses were no longer happening.  Now they are.  Somehow, I missed that... after not missing it.  But it doesn't matter.  I still forbid the folder accesses but the file save works anyway with this patch.

The patch permits access to files named "FLASH%d.TMP".  In addition, it exposes the file permissions stuff to new Win32 APIs, like NtSetInformationFile and NtQueryAttributesFile, as it turns out they are needed for the temp-file to save-file behavior to work.
Attachment #8836206 - Flags: review?(bobowencode)
Assignee: nobody → davidp99
Comment on attachment 8836206 [details] [diff] [review]
Permit sandboxed processes to access Flash temp files

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

I don't think we can do it like this.
If I understand correctly we are always allowing flashtmp[0-9]*.tmp to be written anywhere, by any child process.

I think we should only allow the flashtmp access in the directory where we've allowed the corresponding file access.
Attachment #8836206 - Flags: review?(bobowencode) → review-
> I think we should only allow the flashtmp access in the directory where
> we've allowed the corresponding file access.

Thats fine with me.  Added restriction to folders with write-permitted files.
Attachment #8836206 - Attachment is obsolete: true
Attachment #8837344 - Flags: review?(bobowencode)
Fixing a typo....
Attachment #8837344 - Attachment is obsolete: true
Attachment #8837344 - Flags: review?(bobowencode)
Attachment #8837346 - Flags: review?(bobowencode)
Comment on attachment 8837346 [details] [diff] [review]
Permit sandboxed processes to access Flash temp files

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

Let's just change GetPlainFileName to test for the flashtmp pattern as return a dummy path with the extension and number removed i.e. <path>\FLASHTMP

Then in GrantFileAccess after the existing code:

  if (aPermitWrite) {
    // lines of code to create dummyFlashtmp = <dir>\FLASHTMP
    permissions[dummyFlashtmp] = true;
  }

I think the rest of the code can remain the same then.

I've turned off review requests because I'm going on PTO, but needinfo me on the new patch and I'll review.
Attachment #8837346 - Flags: review?(bobowencode)
> Let's just change GetPlainFileName to test for the flashtmp pattern as
> return a dummy path with the extension and number removed i.e.
> <path>\FLASHTMP

Makes no difference to me.  I've used FLASHTMP0.TMP instead of FLASHTMP as the token simply to avoid granting permission to one extra file.

Tests:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=b1d9bbaa23d8a2c735e3ca0340329253e676082e
Attachment #8837346 - Attachment is obsolete: true
Flags: needinfo?(bobowencode)
Comment on attachment 8838690 [details] [diff] [review]
Permit sandboxed processes to access Flash temp files

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

>   int idx = 8,
>       len = aFilename.length();

nit: define with separate statements please.
Attachment #8838690 - Flags: review+
Flags: needinfo?(bobowencode)
Keywords: checkin-needed
Pushed by cbook@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/0f64b24c40c4
Permit sandboxed processes to access Flash temporary files. r=bobowen
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/0f64b24c40c4
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla54
Too late for 51 and we are about to ship 52 this week. Mark 51 won't fix.
Just clearing my needsinfo. It looks like that was resolved.
Flags: needinfo?(jeclark)
Product: Core → Core Graveyard
You need to log in before you can comment on or make changes to this bug.