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)
Tracking
(firefox50 affected, firefox51 wontfix, firefox52 wontfix, firefox53 wontfix, firefox54 fixed)
RESOLVED
FIXED
mozilla54
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
Reporter | ||
Comment 1•7 years ago
|
||
This seems like an issue with the native NPAPI sandbox.
Reporter | ||
Updated•7 years ago
|
Has STR: --- → yes
OS: Unspecified → Windows 10
Hardware: Unspecified → x86_64
Updated•7 years ago
|
Flags: needinfo?(jmathies)
Whiteboard: sb?
Comment 3•7 years ago
|
||
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
Blocks: support-win64
Status: UNCONFIRMED → NEW
status-firefox50:
--- → affected
status-firefox51:
--- → affected
status-firefox52:
--- → affected
status-firefox53:
--- → affected
Ever confirmed: true
Assignee | ||
Comment 4•7 years ago
|
||
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)
Comment 5•7 years ago
|
||
David, if the FlashTmp filenames are predictable, can you permit file writes to those temporary filenames in addition to the download's real filename?
Comment 6•7 years ago
|
||
(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)
Comment 7•7 years ago
|
||
> 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)
Reporter | ||
Comment 8•7 years ago
|
||
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.
Updated•7 years ago
|
Flags: needinfo?(davidp99)
Assignee | ||
Comment 9•7 years ago
|
||
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)
Comment 10•7 years ago
|
||
(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)
Updated•7 years ago
|
Whiteboard: sb? → sbwn1
Assignee | ||
Comment 11•7 years ago
|
||
(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)
Assignee | ||
Comment 12•7 years ago
|
||
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 | ||
Updated•7 years ago
|
Assignee: nobody → davidp99
Comment 13•7 years ago
|
||
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-
Assignee | ||
Comment 14•7 years ago
|
||
> 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)
Assignee | ||
Comment 15•7 years ago
|
||
Fixing a typo....
Attachment #8837344 -
Attachment is obsolete: true
Attachment #8837344 -
Flags: review?(bobowencode)
Attachment #8837346 -
Flags: review?(bobowencode)
Comment 16•7 years ago
|
||
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)
Assignee | ||
Comment 17•7 years ago
|
||
> 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 18•7 years ago
|
||
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+
Updated•7 years ago
|
Flags: needinfo?(bobowencode)
Updated•7 years ago
|
Blocks: win64-rollout
Assignee | ||
Comment 19•7 years ago
|
||
Attachment #8838690 -
Attachment is obsolete: true
Assignee | ||
Updated•7 years ago
|
Keywords: checkin-needed
Comment 20•7 years ago
|
||
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
Comment 21•7 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/0f64b24c40c4
Status: NEW → RESOLVED
Closed: 7 years ago
status-firefox54:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla54
Comment 22•7 years ago
|
||
Too late for 51 and we are about to ship 52 this week. Mark 51 won't fix.
Comment 23•7 years ago
|
||
Too late for 52, and I guess wontfix for 53 as in https://bugzilla.mozilla.org/show_bug.cgi?id=1284897#c81
Reporter | ||
Comment 24•7 years ago
|
||
Just clearing my needsinfo. It looks like that was resolved.
Flags: needinfo?(jeclark)
Updated•2 years ago
|
Product: Core → Core Graveyard
You need to log in
before you can comment on or make changes to this bug.
Description
•