Closed Bug 1444291 Opened 7 years ago Closed 7 years ago

[Mac] Allow filesystem read access for the Flash sandbox so that file dialogs work

Categories

(Core :: Security: Process Sandboxing, enhancement, P1)

Product:
Core
Component:
Security: Process Sandboxing
Version:
60 Branch
Platform:
Unspecified
macOS
Type:
enhancement

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla61
Tracking Flags:
Tracking Status
firefox60 --- wontfix
firefox61 --- fixed

People

(Reporter: haik, Assigned: haik)

References

Details

Attachments

(3 files, 2 obsolete files)

Bug 1444291 - Part 3 - Add read access to the Mac Flash sandbox, support sandbox levels
59 bytes, text/x-review-board-request
Alex_Gaynor
: review+
Details
Bug 1444291 - Part 1 - Reduce Mac Flash sandbox cache and temp dir permissions
59 bytes, text/x-review-board-request
Alex_Gaynor
: review+
Details
Bug 1444291 - Part 2 - Add additional sysctl access to the Mac Flash sandbox
59 bytes, text/x-review-board-request
Alex_Gaynor
: review+
Details
The Mac Flash sandbox (which is not yet enabled by default) doesn't allow filesystem read access from the plugin process which is likely to cause dissatisfaction because it breaks Flash-based file uploaders which may be still used. This bug is to add read access and some other improvements before enabling the sandbox by default in Nightly (and limited to Nightly) in 61.
Assignee: nobody → haftandilian
Priority: -- → P1
Summary: [Mac] Allow filesystem read access to the Flash sandbox so that file dialogs work → [Mac] Allow filesystem read access for the Flash sandbox so that file dialogs work
I've posted some patches that add filesystem read-access using undocumented sandbox extensions as well as some miscellaneous changes to the plugin sandbox. More details below. Remove access to the DARWIN_USER_CACHE_DIR dir. Manual tests didn't turn up any issues with this directory removed. Using iosnoop(1m), I found that the cache dir was used for what appears to be GPU shader caches. Limit DARWIN_USER_TEMP_DIR read/write access to the FlashTmp subdirectory. Printing requires reading and writing to this directory. Manually tested. Adds sysctls that were found to be being requested by the plugin process. The absence of these were not causing problems, I've added them because they might be used for performance optimizations in Flash code or OS library code. Grant filesystem read access using sandbox extensions setup by file dialog machinery. I found that even after granting global read access, a collection of mach services is still required in order to display a non-buggy file dialog box. Given that, I think there is a /slight/ security benefit to using the sandbox extensions instead of granting global read access.
Comment on attachment 8957439 [details] Bug 1444291 - Part 1 - Reduce Mac Flash sandbox cache and temp dir permissions https://reviewboard.mozilla.org/r/226364/#review232356 ::: security/sandbox/mac/SandboxPolicies.h:641 (Diff revision 1) > (subpath "/private/etc/cups/ppd") > (literal "/private/var/run/cupsd")) > (allow user-preference-read > (preference-domain "org.cups.PrintingPrefs")) > + ; Temporary files read/written here during printing > + (allow file-read* file-write* Can `file-write*` be scoped down to `file-write-create` and `file-write-data` or something like that? (Should have asked this question when we landed the original patch, but better late than never!)
Comment on attachment 8957440 [details] Bug 1444291 - Part 2 - Add additional sysctl access to the Mac Flash sandbox https://reviewboard.mozilla.org/r/226366/#review232360
Attachment #8957440 - Flags: review?(agaynor) → review+
Before I review the substance of part 3, can you walk me through the plan here from a product perspective? When will use level 1 vs 2 in the product? Is this something a user will control or do we expect to eventually switch it for everyone (like with the content process levels)?
(In reply to Alex Gaynor [:Alex_Gaynor] from comment #7) > Before I review the substance of part 3, can you walk me through the plan > here from a product perspective? When will use level 1 vs 2 in the product? First, let me clarify that level 1 includes (essentially) global read-access in order to get file dialogs to work properly. Level 2 removes the read access. The approach I have in mind is to ship level 1 in build 61 along with a mechanism to opt-out of the Flash sandbox per-site and there is a proposed UI and some discussion on bug 1433577. I think we can turn on the sandbox in Nightly by default before the opt-out is ready to get some early feedback, but the sandbox shouldn't ride the trains without the opt-out mechanism. One problem I have with shipping level 2 is that the user experience is bad when an applet tries to open a file dialog. The dialog appears, but it looks very buggy and the user-impression is that this is a Firefox issue. If we have the development time/resources, I'd like to investigate more in this area: i.e., could we block file dialogs from appearing OR (better) could we use the same mechanism App Store apps use where the sandbox extension is only granted for the file that is selected in the file dialog. It could be that shipping level 2 with the opt-out would work well for users, but this approach is more conservative. > Is this something a user will control or do we expect to eventually switch > it for everyone (like with the content process levels)? The goal is to manage this like the content process levels and to not document the pref or require users to ever interact with it. The opt-out mechanism should be documented. Feedback on the overall approach is welcome of course.
See Also: → 1433577
Comment on attachment 8957439 [details] Bug 1444291 - Part 1 - Reduce Mac Flash sandbox cache and temp dir permissions https://reviewboard.mozilla.org/r/226364/#review232356 > Can `file-write*` be scoped down to `file-write-create` and `file-write-data` or something like that? (Should have asked this question when we landed the original patch, but better late than never!) I'll experiment with that.
Comment on attachment 8957441 [details] Bug 1444291 - Part 3 - Add read access to the Mac Flash sandbox, support sandbox levels https://reviewboard.mozilla.org/r/226362/#review233258 ::: dom/plugins/base/nsPluginTags.cpp:432 (Diff revision 1) > - // is enabled, we set the level to 1. > if (mIsFlashPlugin) { > - // Allow enabling the sandbox via the pref > - // security.sandbox.mac.flash.enabled or via the environment variable > - // MOZ_SANDBOX_MAC_FLASH_FORCE (which is useful while the sandbox is > - // off by default). > + if (NS_FAILED(Preferences::GetInt("dom.ipc.plugins.sandbox-level.flash", > + &mSandboxLevel))) { > + mSandboxLevel = 0; > + } This should make sure the level satisfies (0 <= level <= 2). Will add some error checking here that clamps the level. I'll also add it where we read the level from the command line in the child.
Comment on attachment 8957441 [details] Bug 1444291 - Part 3 - Add read access to the Mac Flash sandbox, support sandbox levels https://reviewboard.mozilla.org/r/226362/#review233286 Changes to the sandbox policy look ok. What do you think about, instead of having levels, having specific prefs + flags, so this would be `dom.ipc.plugins.sandbox.flash.allow-filedialog-read` (`bool`)? I think this would help us avoid some of the issues we ran into with levels on the content sandbox -- specifically we can vary them independently, and they are easier to document.
Comment on attachment 8957441 [details] Bug 1444291 - Part 3 - Add read access to the Mac Flash sandbox, support sandbox levels https://reviewboard.mozilla.org/r/226362/#review233286 I think that would be fine, but I also think levels are good here because we only really want to support one mode at a given time for each release. And I thought using the same pref name that we do on Windows was a good thing. (Admittedly the levels do mean different things and the sandboxes are totally different so maybe that's irrelevant.) I expect and really hope that the Flash sandbox isn't something we make major changes to over time. And I'm envisioning the only major change we make is to remove read-access later. So given that, I'd prefer to stick with levels, but I do acknowledge it could be a pain in the future if we are making changes we want to be able to toggle.
Comment on attachment 8957439 [details] Bug 1444291 - Part 1 - Reduce Mac Flash sandbox cache and temp dir permissions https://reviewboard.mozilla.org/r/226364/#review232356 > I'll experiment with that. Ran some manual tests and didn't encounter any problems with just file-write-create and file-write-data so I've made that change.
Comment on attachment 8957441 [details] Bug 1444291 - Part 3 - Add read access to the Mac Flash sandbox, support sandbox levels https://reviewboard.mozilla.org/r/226362/#review233258 > This should make sure the level satisfies (0 <= level <= 2). Will add some error checking here that clamps the level. I'll also add it where we read the level from the command line in the child. Added ClampFlashSandboxLevel() for this purpose. Added a check for MOZ_DISABLE_NPAPI_SANDBOX.
Comment on attachment 8957439 [details] Bug 1444291 - Part 1 - Reduce Mac Flash sandbox cache and temp dir permissions https://reviewboard.mozilla.org/r/226364/#review234004 ::: security/sandbox/mac/SandboxPolicies.h:641 (Diff revision 2) > (subpath "/private/etc/cups/ppd") > (literal "/private/var/run/cupsd")) > (allow user-preference-read > (preference-domain "org.cups.PrintingPrefs")) > + ; Temporary files read/written here during printing > + (allow file-read* file-write* Should have asked about the `file-write*` here as well :-(
Attachment #8957439 - Flags: review?(agaynor) → review+
Comment on attachment 8957441 [details] Bug 1444291 - Part 3 - Add read access to the Mac Flash sandbox, support sandbox levels https://reviewboard.mozilla.org/r/226362/#review234010
Attachment #8957441 - Flags: review?(agaynor) → review+
Comment on attachment 8957439 [details] Bug 1444291 - Part 1 - Reduce Mac Flash sandbox cache and temp dir permissions https://reviewboard.mozilla.org/r/226364/#review234004 > Should have asked about the `file-write*` here as well :-( No prob. Really I should have applied the change there too. I'll do some testing with the minimized file-write there and update the fix if that works out.
Try run with sandbox disabled: https://treeherder.mozilla.org/#/jobs?repo=try&revision=bafb731a56c4199ad073554f2fce3d01e73ebb9d With sandbox enabled (via dom.ipc.plugins.sandbox-level.flash=1, dom.ipc.plugins.sandbox-level.default=1): https://treeherder.mozilla.org/#/jobs?repo=try&revision=1e45c684bd5b1250acd2246bc9f87db9e8ce0b2c
Comment on attachment 8957439 [details] Bug 1444291 - Part 1 - Reduce Mac Flash sandbox cache and temp dir permissions https://reviewboard.mozilla.org/r/226364/#review234004 > No prob. Really I should have applied the change there too. I'll do some testing with the minimized file-write there and update the fix if that works out. I ran manual tests and didn't encounter any problems so I've changed the file-write to file-write-create and file-write-data.
Pushed by haftandilian@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/8ee3e65465ed Part 1 - Reduce Mac Flash sandbox cache and temp dir permissions r=Alex_Gaynor https://hg.mozilla.org/integration/autoland/rev/dfc31b6c9f53 Part 2 - Add additional sysctl access to the Mac Flash sandbox r=Alex_Gaynor https://hg.mozilla.org/integration/autoland/rev/48a9c2131347 Part 3 - Add read access to the Mac Flash sandbox, support sandbox levels r=Alex_Gaynor
Attachment #8957439 - Attachment is obsolete: true
Attachment #8957440 - Attachment is obsolete: true
Flags: needinfo?(haftandilian)
Attachment #8959437 - Flags: review?(agaynor) → review+
Attachment #8959438 - Flags: review?(agaynor) → review+
Attachment #8959437 - Flags: review+ → review?(agaynor)
Attachment #8959438 - Flags: review+ → review?(agaynor)
Backed out due to Android build failure. I've fixed the issue by #ifdef'ing the include of SandboxSettings.h. And I then botched re-submitting this to reviewboard with an hg push -c command instead of -r.
Comment on attachment 8959437 [details] Bug 1444291 - Part 1 - Reduce Mac Flash sandbox cache and temp dir permissions https://reviewboard.mozilla.org/r/228248/#review234138
Attachment #8959437 - Flags: review?(agaynor) → review+
Comment on attachment 8959438 [details] Bug 1444291 - Part 2 - Add additional sysctl access to the Mac Flash sandbox https://reviewboard.mozilla.org/r/228250/#review234140
Attachment #8959438 - Flags: review?(agaynor) → review+
Pushed by haftandilian@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/48cacb921dab Part 1 - Reduce Mac Flash sandbox cache and temp dir permissions r=Alex_Gaynor https://hg.mozilla.org/integration/autoland/rev/2103c836b739 Part 2 - Add additional sysctl access to the Mac Flash sandbox r=Alex_Gaynor https://hg.mozilla.org/integration/autoland/rev/5f14c440b68a Part 3 - Add read access to the Mac Flash sandbox, support sandbox levels r=Alex_Gaynor
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: