Closed Bug 1284897 Opened 8 years ago Closed 7 years ago

64 bit Flash Player has storage permissions issues

Categories

(Core Graveyard :: Plug-ins, defect, P3)

51 Branch
x86_64
Windows 7
defect

Tracking

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

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

People

(Reporter: mirh, Assigned: handyman)

References

Details

(Keywords: 64bit, regression, Whiteboard: sbwn1)

Attachments

(6 files, 27 obsolete files)

16.06 KB, image/png
Details
24.30 KB, patch
Details | Diff | Splinter Review
14.82 KB, patch
Details | Diff | Splinter Review
21.92 KB, patch
Details | Diff | Splinter Review
6.11 KB, patch
Details | Diff | Splinter Review
32.12 KB, patch
jimm
: review+
Details | Diff | Splinter Review
Save/export any Flash content from any 64 bit Firefox session (for example https://infostat.bancaditalia.it/inquiry/EP/taxo)

Windows save file window complains about missing permissions in every folder.
I found the problem was introduced somewhere between 41.0b5 and 41.0b6
https://ftp.mozilla.org/pub/firefox/releases/41.0b5/win64/en-US/
https://ftp.mozilla.org/pub/firefox/releases/41.0b6/win64/en-US/

Probably related to bug 1165981?
Has Regression Range: --- → yes
Ok, it's related to bug 1185532 on the other hand. 
I can confirm setting MOZ_ALLOW_WEAKER_SANDBOX env var and dom.ipc.plugins.sandbox-level.flash=1 fix the issue.
I can't get the URL in question to show any content at all. Do you have another example of this, or more precise steps that anyone can use to reproduce? I don't really understand what kind of save/export operation we're talking about here.

In general the fact that the Flash player can't touch the filesystem is the intended functioning of the sandbox.
Flags: needinfo?(mirh)
Component: Shell Integration → Plug-ins
Product: Firefox → Core
The URL above works just fine here tbh. 

You can even try this then https://www.fenoxo.com/play/playtits.html
Press Data, then Load File.
Flags: needinfo?(mirh)
Bob, do you have a marking or tracker bug for things caused by Flash sandboxing?
Flags: needinfo?(bobowen.code)
I think they're all set to block bug 1185532, I'll also flag for our triage (although we're not actively working on any NPAPI sandboxing issues at the moment).
Blocks: 1185532
Flags: needinfo?(bobowen.code)
Whiteboard: sb?
Whiteboard: sb? → sbwn1
Priority: -- → P3
(In reply to mirh from comment #0)
> Save/export any Flash content from any 64 bit Firefox session (for example
> https://infostat.bancaditalia.it/inquiry/EP/taxo)
> 
> Windows save file window complains about missing permissions in every folder.

I'm not able to reproduce this currently. Although I'm a little surprised by this. Need to investigate. what level 2 sandbox is allowing for file write permissions, and also determine if these write are happening in the content process.
Status: UNCONFIRMED → NEW
Ever confirmed: true
I reconfirm what I stated. MOZ_ALLOW_WEAKER_SANDBOX env var and dom.ipc.plugins.sandbox-level.flash=1 fixes it. 

The only exception I realized today being when pluginreg.dat isn't present (because first launch or because I deleted it). In that case -oddly- flash works normally.
Version: 50 Branch → 51 Branch
I don't think this is valid.  The bug behavior seems to be exactly what the sandbox is designed for, as bsmedberg mentioned in comment 3.  Indeed, comment 0 believed that all folders are forbidden but some are not.  Specifically, the ones whitelisted by the sandbox here:

https://dxr.mozilla.org/mozilla-central/rev/29beaebdfaccbdaeb4c1ee5a43a9795ab015ef49/dom/plugins/ipc/PluginProcessParent.cpp#94

STR:

1. Go to the page from comment 4 in an x64 build:

https://www.fenoxo.com/play/playtits.html

(Due to an unrelated compositor issue that triggers an assert in debug runs, this currently needs to be done with a release build or this needs to be commented out:
https://dxr.mozilla.org/mozilla-central/rev/29beaebdfaccbdaeb4c1ee5a43a9795ab015ef49/gfx/layers/d3d11/TextureD3D11.cpp#657 )

2. Press "New Game", create a new character by randomly picking buttons until you get to the start of the game.  (When the Tutorial option comes up, press "Skip").

3. As soon as this happens (one of the options you see should be "Rest" I think), press the Save/Load button (down-arrow in the bottom-left of the window).  Press "Save File".  

4. Go to basically any folder (say, C:\).  Enter a file name to save to.  Press "Save" and get the following message (see the forbidden.png attachment):

"You don't have permission to save in this location.  Contact the administrator to obtain permission.  Would you like to save in the David Parks folder instead?"

(you can't save in your home folder, either)

5. Enter this into the filename field in the file browser:

%APPDATA%\Macromedia\Flash Player

This will browse the that folder.  Enter a file name and press save.  Voila, the file saves.  That is because this folder is whitelisted by the sandbox.

----

NIing :jimm to see if this is indeed WontFix.
Flags: needinfo?(jmathies)
Attached image forbidden.png
Why "Save File" (which is under user's control) should be forbidden in the first place?
We had a good meeting about this bug with bobowen this morning. dparks is going to do some investigative work to see if we can hook into these file operations.
Flags: needinfo?(jmathies)
Assignee: nobody → davidp99
Commenting out some failing asserts from other bugs.  Not part of final patch but needed to be able to test the solution posted here.
Comment on attachment 8807197 [details] [diff] [review]
WIP:  Track names of files that have been given special sandbox access permissions

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

As far as the chromium changes are concerned they look like they'll be easy to reapply.

My slight concern is the unconditional removal of the QueryBroker calls as this will affect all child processes.
In theory this will actually be quicker, assuming we don't get many requests for non-allowed files, but I'm not sure.
These have been around from the first chromium commit, so maybe they were here from when this code was a stand alone sandbox which google bought.
Although they added something similar for the registry_interceptor a year or so ago, so I guess they were getting a lot of registry requests at some point.

Perhaps we could add telemetry for when the broker doesn't grant permission to confirm it's not a problem.
Not sure how much of a pain that will be given where this code is.
Attachment #8807197 - Flags: feedback+
(In reply to Bob Owen (:bobowen) from comment #18)
>
> My slight concern is the unconditional removal of the QueryBroker calls as
> this will affect all child processes.

The only time those tests would have been useful is when they block an access -- otherwise they duplicate the operations that the broker is getting ready to do anyway.  While this blocked access may happen, even commonly, in plugins, I assume we don't really care about the speed of blocked-file-access in that case.  That leaves file access initiated by the browser itself, which I assume should pretty much never request a file it doesn't have sandbox permission to.  On top of that, as of right now, we only block _write_ access anyway, which is a small fraction of file access requests.  I can't even come up with a farfetched case.

> Perhaps we could add telemetry for when the broker doesn't grant permission
> to confirm it's not a problem.
> Not sure how much of a pain that will be given where this code is.

Eh, I can learn how to add it if we still want it.
Attachment #8807196 - Attachment is obsolete: true
Tests:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=1ae584ad9b080a6f5643b48a30c5f26be24a75bd

To try out the patches --

---

Test #1: This test shows that the write-access is available.

Follow steps 1-4 in the STR in comment 9.  With this patch, the save file works fine.  Also note that you can then "Load File" to confirm you can load what you saved.

---

Test #2: This test shows that read-access is also working (when the sandbox read access protection is turned on).  Note that this test cannot currently be run with nightly because flash sandbox level 11, which blocks read access in the user's folder, is defined by this patch.

0. Put a tiny thumbnail png directly into your home folder.
1. Set dom.ipc.plugins.sandbox-level.flash to 11 and restart the browser.
2. Go to http://www.tinywebgallery.com/en/tfu/web_demo1.php
3. Press the "Add Files" button
4. Navigate the file picker to your home folder and select the png.

If no error window pops up declaring access is forbidden then joy!
Attachment #8809242 - Attachment description: Add opcodes to nsWindowsDllInterceptor for GetSaveFileNameW, GetOpenFileNameW and ImmReleaseContext. → Add opcodes to nsWindowsDllInterceptor for GetSaveFileNameW, GetOpenFileNameW, SetCapture and ImmReleaseContext.
The remaining bad nsWindowsDllInterceptor hooks are listed in Bug 1316415.
Attachment #8809242 - Attachment description: Add opcodes to nsWindowsDllInterceptor for GetSaveFileNameW, GetOpenFileNameW, SetCapture and ImmReleaseContext. → 1. Add opcodes to nsWindowsDllInterceptor for GetSaveFileNameW, GetOpenFileNameW, SetCapture and ImmReleaseContext.
Attachment #8809243 - Attachment description: Add missing hooked methods to TestDllInterceptor. Add ASSERTions to nsWindowsDllInterceptor → 2. Add missing hooked methods to TestDllInterceptor. Add ASSERTions to nsWindowsDllInterceptor
Attachment #8809244 - Attachment description: Track names of files that have been given special sandbox access permissions → 3. Track names of files that have been given special sandbox access permissions
Attachment #8809245 - Attachment description: Chromium sandbox changes: Add test for special filesystem access → 4. Chromium sandbox changes: Add test for special filesystem access
Attachment #8809246 - Attachment description: Hooked GetSaveFileNameW/GetOpenFileNameW → 5. Hooked GetSaveFileNameW/GetOpenFileNameW
Test #1 in comment 25 is currently very difficult to run due to a graphics glitch that causes the screen to no longer be rendered fully.  For the time being, that test is nearly worthless.  But I've got a new one...

---

STR:

1. Go to https://permadi.com/tutorial/flash-save-file-dialog/Flash-save-file-example-with-filereference.html
2. Press "Save Picture"
3. Navigate to your home directory and enter a file name.

Expected:
File is saved.

Actual:
The error window attached to this bug (forbidden.png)

-----

The test works fine with the patch.  Test #2 from comment 25 is still valid.

---

NOTE: I have a third example of a plugin that saves a file:

http://www.tinywebgallery.com/en/tfu/web_demo2.php

There is a "Download" button that you can use to save any of the images on that server.  This DOES NOT WORK with the patches here.  The file access is still rejected, albeit with a different error.  This is because, once you select the file to save to, the plugin FIRST tries to open "FlashTmp0.tmp" inside of the folder containing the file that you picked.  This is, of course, still blocked since the user hasn't given permission to use it.  So this is by design.
Comment on attachment 8809242 [details] [diff] [review]
1. Add opcodes to nsWindowsDllInterceptor for GetSaveFileNameW, GetOpenFileNameW, SetCapture and ImmReleaseContext.

Hey Aaron,

This is a beefy change but a fair amount is due to renaming nBytes to nOrigBytes (since I now have nTrampBytes so leaving it seemed confusing).  Still, the change is hefty on top of that so I made some minor calls to try to reduce the diff complexity.  Namely, the COPY_CODES macro (I hate C macros, which is why I feel the need to justify myself here).  You mentioned before that this could use a good refactor and I haven't helped but I was going more for 'easy to review'.  Hopefully, it's good enough for now.

See comment 25 and comment 27 for ways to try this out.
Attachment #8809242 - Flags: review?(aklotz)
Attachment #8809243 - Flags: review?(aklotz)
Comment on attachment 8809244 [details] [diff] [review]
3. Track names of files that have been given special sandbox access permissions

Bob,

You may want to check out my comments in comment 19, comment 25 and comment 27.  I've already got one case that this doesn't 'fix' in comment 27 but I don't think there is a good way to handle that one.
Attachment #8809244 - Flags: review?(bobowencode)
Attachment #8809245 - Flags: review?(bobowencode)
Attachment #8809246 - Flags: review?(bobowencode)
Just to let you know that it will probably be Monday before I get to these.
Attachment #8809243 - Flags: review?(aklotz) → review+
[Tracking Requested - why for this release]:

We would like to fix this 64-bit Flash bug in Firefox 52 (or earlier :) because we plan to run a Funnelcake experiment comparing 32- and 64-bit Firefox users in the Firefox 52 release.
Comment on attachment 8809244 [details] [diff] [review]
3. Track names of files that have been given special sandbox access permissions

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

Sorry this has taken me so long to get through.
Also, apologies for all the nit-picking.

Generally, this looks good, but there are some outstanding questions and some moving around/tidying to do.
I might want a build peer to look at this as well (probably glandium).

By the way, would it make sense to put the Plugin parts in this patch into Part 5?

::: dom/plugins/ipc/PluginModuleParent.cpp
@@ +775,5 @@
>      // If we registered for audio notifications, stop.
>      mozilla::plugins::PluginUtilsWin::RegisterForAudioDeviceChanges(this,
>                                                                      false);
> +
> +    mSandboxPermissionService.RemovePermissionsForPlugin(mPluginId);

I'm a bit confused, is the mPluginId the child process ID, I thought it came from the tag and was  just incremented as the tags load.

And in the other patches we seem to be holding the permissions against the process ID.

::: dom/plugins/ipc/PluginModuleParent.h
@@ +677,5 @@
>      nsCString mProfile;
>      bool mIsBlocklisted;
>      static bool sInstantiated;
> +#ifdef XP_WIN
> +    mozilla::SandboxPermissions mSandboxPermissionService;

nit: mSandboxPermissions?

Also, would this work without MOZ_SANDBOX defined?

::: dom/plugins/ipc/PluginProcessParent.cpp
@@ +81,5 @@
>          return;
>      }
>  
>      // Higher than level 2 currently removes the users own rights.
> +    if (aSandboxLevel > 2 && aSandboxLevel < 10) {

Can't we just get rid of these?

::: dom/plugins/ipc/PluginUtilsWin.h
@@ +6,5 @@
>  
>  #ifndef dom_plugins_PluginUtilsWin_h
>  #define dom_plugins_PluginUtilsWin_h 1
>  
> +enum class nsresult : uint32_t;

Why do we need this change?

@@ +23,5 @@
> + * Returns true if the user has granted the sandboxed plugin process the
> + * requested permission to open the file.
> + */
> +extern bool UserGrantedFileAccess(uint32_t aProcessId, const wchar_t* aFilename,
> +                                  uint32_t aAccess, uint32_t aDisposition);

I don't understand why this is here.

::: js/xpconnect/src/XPCShellImpl.cpp
@@ +1528,5 @@
>          } else {
>            NS_WARNING("Failed to initialize broker services, sandboxed "
>                       "processes will fail to start.");
>          }
> +        if (aShellData->sandboxPermissionsService) {

I wonder if we really need this for xpcshell.

We'll need a peer to review it, if we do.

::: security/sandbox/win/PermissionsService.cpp
@@ +1,2 @@
> +/* -*- Mode: C++; tab-width: 2; indent-tabs-mode: nil; c-basic-offset: 2 -*- */
> +/* This Source Code Form is subject to the terms of the Mozilla Public

nit: missing vim line

@@ +11,5 @@
> +
> +namespace mozilla {
> +namespace sandboxing {
> +
> +static PermissionsService sPermissionsService;

I think this will create a static initializer, which we try to avoid.
Can we use a local static within the function instead?

@@ +13,5 @@
> +namespace sandboxing {
> +
> +static PermissionsService sPermissionsService;
> +// Grants permission to the NT internal file "FOO:ZONE.IDENTIFIER"
> +// for every file "FOO" that has permission.

nit: these comments are a bit confusing, I don't understand how a constant grants permission.

@@ +22,5 @@
> +static const std::wstring ZONE_ID_DATA_STR(L":ZONE.IDENTIFIER:$DATA");
> +
> +bool
> +StringEndsWith(const std::wstring& str, const std::wstring& strEnding) {
> +  if (strEnding.size() > str.size()) 

nit: whitespace and curly brackets for one line if

Also, all these functions should have opening curly bracket on following line.

@@ +28,5 @@
> +  return std::equal(strEnding.rbegin(), strEnding.rend(), str.rbegin());
> +}
> +
> +// Converts NT device internal filenames to normal user-space by stripping
> +// the prefix symbols from the drive specifier.

nit: this comment doesn't mention the suffix changes

@@ +61,5 @@
> +  std::wstring filename = GetPlainFileName(aFilename);
> +  auto itPermission = permissions.find(filename);
> +  if (itPermission == permissions.end() || !itPermission->second) {
> +    // Add/update the file in the permissions map.
> +    permissions[filename] = aPermitWrite;

Instead of searching twice why not just:
 
permissions[aFilename] |= aPermitWrite;

@@ +74,5 @@
> +  // There are 3 types of permissions:
> +  // * Those available w/ read-only permission
> +  // * Those available w/ read-only AND read-write permission
> +  // * Those always forbidden.
> +  bool needsWrite = (aAccess & FILE_WRITE_DATA) ||

We should do this check just before we need it.

@@ +86,5 @@
> +                     (aAccess & FILE_LIST_DIRECTORY) ||
> +                     (aAccess & FILE_TRAVERSE) ||
> +                     (aAccess & STANDARD_RIGHTS_EXECUTE);
> +
> +  if (isForbidden) {

I think we should have:
 
const uint32_t FORBIDDEN_FLAGS = FILE_EXECUTE | FILE... etc.
 
then 
if (aAccess & FORBIDDEN_FLAGS) {
 
similar thing for checking for write access.

@@ +97,5 @@
> +  }
> +
> +  std::wstring filename = GetPlainFileName(aFilename);
> +  auto itPermission = permissions->second.find(filename);
> +  if (itPermission != permissions->second.end()) {

nit: I think it's easier to follow this if we do == here and return false, then we can do then needAccess test below.

::: security/sandbox/win/PermissionsService.h
@@ +1,2 @@
> +/* -*- Mode: C++; tab-width: 2; indent-tabs-mode: nil; c-basic-offset: 2 -*- */
> +// vim:set ts=2 sts=2 sw=2 et cin:

nit: this doesn't quite match the one at https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/Coding_Style

@@ +4,5 @@
> + * License, v. 2.0. If a copy of the MPL was not distributed with this
> + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
> +
> +#ifndef security_sandbox_PermissionsService_h
> +#define security_sandbox_PermissionsService_h 1

with the change in the export in moz.build this should become:
mozilla_sandboxing_PermissionsService_h

and no need for the 1

@@ +9,5 @@
> +
> +#include <unordered_map>
> +
> +// The PermissionsService is supposed to be considered architecturally
> +// a "sibling" of the chromium services (BrokerService, etc).  Those objects

Given this, I think it makes more sense for this to live in security\sandbox\chromium-shim\sandbox\win

@@ +20,5 @@
> +/*
> + * Represents additional permissions granted to sandboxed processes.
> + * The members are virtual so that the object can be created in any
> + * library that links with libsandbox_s and then shared with and used
> + * by libXUL, which does not link with libsandbox_s.  

nit: whitespace

@@ +31,5 @@
> +  /*
> +   * Allow future access to aFilename by the plugin instance with the given ID.
> +   */
> +  virtual void GrantFileAccess(uint32_t aPluginId, const wchar_t* aFilename,
> +                              bool aPermitWrite);

nit: indent another space, same below

Also can these be pure virtual?

@@ +52,5 @@
> +  typedef std::unordered_map<std::wstring, bool> FilePermissionMap;
> +
> +  // Maps from Plugin Instance ID to map of user-granted file permissions for
> +  // that plugin instance.
> +  typedef std::unordered_map<uint32_t, FilePermissionMap> ProcessFilePermissionMap;

Should this be process ID?

::: security/sandbox/win/src/sandboxpermissions/moz.build
@@ +8,5 @@
> +    'sandboxPermissions.cpp',
> +]
> +
> +EXPORTS += [
> +    'sandboxPermissions.h',

This file will go and this can be included in EXPORTS.mozilla.sandboxing in the security/sandbox/moz.build

::: security/sandbox/win/src/sandboxpermissions/sandboxPermissions.cpp
@@ +1,1 @@
> +/* -*- Mode: C++; tab-width: 2; indent-tabs-mode: nil; c-basic-offset: 2 -*- */

I think that this should just be in security/sandbox/win/ and with a captital S.

I think that the target and broker were originally in separate folders because they were built separately (and sat under the Windows chromium sandbox code hence the src), but they're not any more.

@@ +14,5 @@
> +sandboxing::PermissionsService* SandboxPermissions::sPermissionsService = nullptr;
> +
> +void
> +SandboxPermissions::GrantFileAccess(uint32_t aPluginId, const wchar_t* aFilename,
> +                            bool aPermitWrite)

nit: some more indentation issues in this file.

::: security/sandbox/win/src/sandboxpermissions/sandboxPermissions.h
@@ +4,5 @@
> + * License, v. 2.0. If a copy of the MPL was not distributed with this
> + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
> +
> +#ifndef __SECURITY_SANDBOX_SANDBOXPERMISSIONS_H__
> +#define __SECURITY_SANDBOX_SANDBOXPERMISSIONS_H__

nit: mozilla_sandboxing_SandboxPermissions_h

@@ +34,5 @@
> +  /*
> +   * Allow future access to aFilename by the plugin instance with the given ID.
> +   */
> +  void GrantFileAccess(uint32_t aPluginId, const wchar_t* aFilename,
> +                              bool aPermitWrite);

nit: some more indentation issues in this file.

@@ +54,5 @@
> +};
> +
> +} // mozilla
> +
> +#endif

nit: #endif // mozilla_sandboxing_SandboxPermissions_h
Attachment #8809244 - Flags: review?(bobowencode)
Comment on attachment 8809245 [details] [diff] [review]
4. Chromium sandbox changes: Add test for special filesystem access

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

::: security/sandbox/chromium/sandbox/win/src/filesystem_dispatcher.cc
@@ +120,5 @@
> +
> +  // If the policies forbid access (any result other than ASK_BROKER),
> +  // then check for user-granted access to file.
> +  if (ASK_BROKER != result &&
> +      mozilla::sandboxing::PermissionsService::GetInstance()->UserGrantedFileAccess(

nit: maybe wrap like this (same below):

      mozilla::sandboxing::PermissionsService::GetInstance()->
        UserGrantedFileAccess(ipc->client_info->process_id, filename,
                              desired_access, create_disposition)) {
Attachment #8809245 - Flags: review?(bobowencode) → review+
Comment on attachment 8809246 [details] [diff] [review]
5. Hooked GetSaveFileNameW/GetOpenFileNameW

As this is all plugin stuff I'm going to bounce this to jimm. (He'll be better at working through all the Windows data marshalling as well.)

Jim - you'll probably want to have a look at Part 3 for context.
Attachment #8809246 - Flags: review?(bobowencode) → review?(jmathies)
Comment on attachment 8809246 [details] [diff] [review]
5. Hooked GetSaveFileNameW/GetOpenFileNameW

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

minor nits and a question. nothing major.

::: dom/plugins/ipc/PluginMessageUtils.cpp
@@ +160,5 @@
> +  // Filter is double-NULL terminated.  mFilter should include the double-NULL.
> +  mHasFilter = aLpofn->lpstrFilter != nullptr;
> +  if (mHasFilter) {
> +    uint32_t dNullIdx = 0;
> +    while (aLpofn->lpstrFilter[dNullIdx] != L'\0' || 

nit - white space

::: dom/plugins/ipc/PluginModuleChild.cpp
@@ +2195,5 @@
> +BOOL
> +PostToPluginThread(GetFileNameFunc aFunc, void* aLpofn) {
> +    // synchronously run from the main thread
> +    // Start a semaphore at 0.  We Release the semaphore (bringing its count to 1)
> +    // when the synchronous call is done.

nit - please clean up the formatting in above comment block.

@@ +2208,5 @@
> +        new GetFileNameTask(aFunc, aLpofn, semaphore, &returnValue);
> +    ProcessChild::message_loop()->PostTask(task.forget());
> +    DWORD err = WaitForSingleObject(semaphore, INFINITE);
> +    if (err != WAIT_FAILED) {
> +        CloseHandle(semaphore);

we have some helper classes that close these down. You might be able to use one of those and remove these close calls. 

http://searchfox.org/mozilla-central/source/xpcom/base/nsWindowsHelpers.h

@@ +2209,5 @@
> +    ProcessChild::message_loop()->PostTask(task.forget());
> +    DWORD err = WaitForSingleObject(semaphore, INFINITE);
> +    if (err != WAIT_FAILED) {
> +        CloseHandle(semaphore);
> +        return returnValue;

AFAICT here, 'returnValue' doesn't have a default value and if the wait failed you're returning whatever undefined value returnValue holds.

@@ +2211,5 @@
> +    if (err != WAIT_FAILED) {
> +        CloseHandle(semaphore);
> +        return returnValue;
> +    }
> +    PLUGIN_LOG_DEBUG(("Error while waiting for GetKeyState semaphore: %d",

nit - this is no longer a GetKeyState semaphore

@@ +2241,5 @@
> +        }
> +        return ret;
> +    }
> +
> +    switch (aFunc) {

We fall into this when e10s is disabled and oopp is enabled?

@@ +2248,5 @@
> +    case SAVE_FUNC:
> +        return sGetSaveFileNameWPtrStub(aLpofn);
> +    }
> +
> +    MOZ_ASSERT(false, "Illegal GetFileNameFunc value");

Looks like MOZ_ASSERT_UNREACHABLE has the same functionality.
Attachment #8809246 - Flags: review?(jmathies) → review-
Sorry about the delay on my remaining review. I need to go through that one with a fine-toothed comb and about six distinct tables from the Intel processor manuals. :-)

I'll have to take care of it when I'm back from PTO.
Tentatively tracking this flash sandboxing issue on win64 for 52
How come the 12 most recent versions of Firefox have this bug unsolved? (41-52)
review ping
Flags: needinfo?(aklotz)
Comment on attachment 8809242 [details] [diff] [review]
1. Add opcodes to nsWindowsDllInterceptor for GetSaveFileNameW, GetOpenFileNameW, SetCapture and ImmReleaseContext.

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

Sorry for the long delay. LGTM for the most part, but I'd like to see the changes to the 0x2B SUB opcode once more before r+.

::: xpcom/build/nsWindowsDllInterceptor.h
@@ +560,5 @@
>      JumpPatch()
>        : mHookOffset(0), mJumpAddress(0), mType(JumpType::Jmp)
>      {
>      }
>  

Is AddJumpPatch used anywhere anymore?

@@ +596,5 @@
> +        aCode[offset + 2 + 4 + 1] = 8;
> +        *reinterpret_cast<int64_t*>(aCode + offset + 2 + 4 + 2) = mJumpAddress;
> +        return offset + 2 + 4 + 2 + 8;
> +      }
> +      else {

style nit: else should be between curlies

@@ +736,4 @@
>          // MOV 0xB8: http://ref.x86asm.net/coder32.html#xB8
> +        nOrigBytes += 5;
> +      } else if (origBytes[nOrigBytes] == 0x33 &&
> +                 (origBytes[nOrigBytes+1] & 0xc0) == 0xc0) {

Can you use kMaskMod and kModReg constants for this expression instead of 0xc0 for readability? Thanks

@@ +770,5 @@
>          // jmp [disp32]
> +        nOrigBytes += 6;
> +      } else if (origBytes[nOrigBytes] == 0xc2) {
> +        // ret imm16.  We can't handle this but it happens.  We don't ASSERT but we do fail.
> +        fprintf(stderr, "Cannot hook method -- RET opcode found");

Use NS_WARNING for this

@@ +865,4 @@
>            // sub r, byte
> +          COPY_CODES(3);
> +        } else if (origBytes[nOrigBytes] == 0x83 &&
> +                   (origBytes[nOrigBytes + 1] & 0xf8) == 0xc0) {

s/0xf8/kMaskMod | kMaskReg/
s/0xc0/kModReg/

@@ +873,4 @@
>            // and [r+d], imm8
> +          COPY_CODES(5);
> +        } else if (origBytes[nOrigBytes] == 0x2b) {
> +          // sub

More info about operands, please.
This should be sub r64, r/m64

@@ +873,5 @@
>            // and [r+d], imm8
> +          COPY_CODES(5);
> +        } else if (origBytes[nOrigBytes] == 0x2b) {
> +          // sub
> +          COPY_CODES(2);

This doesn't look like 2 bytes to me. I think we need to be checking the modr/m byte.

@@ +986,5 @@
>            return;
>          }
> +        COPY_CODES(2 + nModRmSibBytes);
> +      } else if (origBytes[nOrigBytes] == 0xd1 &&
> +                  (origBytes[nOrigBytes+1] & 0xc0) == 0xc0) {

Use kMaskMod for 0xc0 in and
Use kModReg for 0xc0 in equality comparison

@@ +1008,5 @@
> +        nTrampBytes = jump.GenerateJump(tramp);
> +        nOrigBytes += 5;
> +      } else if (origBytes[nOrigBytes] == 0xff) {
> +        COPY_CODES(1);
> +        if ((origBytes[nOrigBytes] & 0xf8) == 0xf0) {

s/0xf8/(kMaskMod | kMaskReg)/
Attachment #8809242 - Flags: review?(aklotz) → review-
Flags: needinfo?(aklotz)
Blocks: 1329328
Addresses Aaron's review in comment 40.

Most everything was pretty straightforward.

> @@ +873,5 @@
> >            // and [r+d], imm8
> > +          COPY_CODES(5);
> > +        } else if (origBytes[nOrigBytes] == 0x2b) {
> > +          // sub
> > +          COPY_CODES(2);
> 
> This doesn't look like 2 bytes to me. I think we need to be checking the
> modr/m byte.
> 

Indeed.  The sequence belongs to both GetSaveFileNameW and ImmNotifyIME on Win10.  I needed to check
ModR/M -- the case is the "sub r64, r64" case, which is indeed two bytes (so the code still ends up working the same).
Attachment #8809242 - Attachment is obsolete: true
The nightmare continues...

This patch _would_ work... save for a bug in Mercurial.  I don't know about in general but for some reason this patch has created a folder on my HD that the Administrator doesn't have permission to access.  So when I try to apply the patch I get "Access Denied".  I don't know if this happens on other machines.  Apart from that, this patch addresses Bob's review in comment 32 and would be fine to review.



> >      // Higher than level 2 currently removes the users own rights.
> > +    if (aSandboxLevel > 2 && aSandboxLevel < 10) {
> 
> Can't we just get rid of these?

I had randomly inserted that to be able to test the difference between the
old setting and this patch.  I've now removed the entire block.  For future
bug archaeology, the removed code is:

>    // Higher than level 2 currently removes the users own rights.
>    if (aSandboxLevel > 2 && aSandboxLevel < 10) {
>        AddSandboxAllowedFile(aAllowedFilesRead, dirSvc, NS_WIN_HOME_DIR);
>        AddSandboxAllowedFile(aAllowedFilesRead, dirSvc, NS_WIN_HOME_DIR,
>                              NS_LITERAL_STRING("\\*"));
>    }


---

> ::: js/xpconnect/src/XPCShellImpl.cpp
> @@ +1528,5 @@
> >          } else {
> >            NS_WARNING("Failed to initialize broker services, sandboxed "
> >                       "processes will fail to start.");
> >          }
> > +        if (aShellData->sandboxPermissionsService) {
> 
> I wonder if we really need this for xpcshell.
> 
> We'll need a peer to review it, if we do.

I don't actually know what its for but I added the initialization of the
file service here because it XPCShell already calls GetInitializedBrokerServices
and this will make it work with brokered file access.  I can remove this but I
think that only makes sense if we don't want the process being sandboxed here
to work with our brokered file mechanism.

In other words, my reasoning has been that this should be added whereever
SandboxInitialization initializes BrokerServices.  I'd have added the
SandboxPermission code to BrokerServices, making its inclusion implicit...
but that's Chromium code and I wanted to make those changes as minimal as
possible.

---

> @@ +31,5 @@
> > +  /*
> > +   * Allow future access to aFilename by the plugin instance with the given ID.
> > +   */
> > +  virtual void GrantFileAccess(uint32_t aPluginId, const wchar_t* aFilename,
> > +                              bool aPermitWrite);
> 
> Also can these be pure virtual?

The methods are actually defined in the .cpp file.  They are only virtual because:

/*
 * Represents additional permissions granted to sandboxed processes.
 * The members are virtual so that the object can be created in any
 * library that links with libsandbox_s and then shared with and used
 * by libXUL, which does not link with libsandbox_s.
 */

Note that this is the only reason why we can use e.g. BrokerServices [1]
in libXUL as well -- because its members are virtual and use the vtable
instead of statically linking.  I just followed that pattern.

---

> ::: security/sandbox/win/src/sandboxpermissions/moz.build
> @@ +8,5 @@
> > +    'sandboxPermissions.cpp',
> > +]
> > +
> > +EXPORTS += [
> > +    'sandboxPermissions.h',
> 
> This file will go and this can be included in EXPORTS.mozilla.sandboxing in
> the security/sandbox/moz.build

sandboxPermissions follows the pattern estabilshed by sandboxTarget and
sandboxBroker (from the same folder).  Specifically, they contain their own moz.builds as
well.  I think this is because they don't build to libsandbox_s like 
security/sandbox/moz.build does -- they contain this line:

> FINAL_LIBRARY = 'xul'

---

> ::: security/sandbox/win/src/sandboxpermissions/sandboxPermissions.cpp
> @@ +1,1 @@
> > +/* -*- Mode: C++; tab-width: 2; indent-tabs-mode: nil; c-basic-offset: 2 -*- */
> 
> I think that this should just be in security/sandbox/win/ and with a
> captital S.

same deal as above.

---

> I think that the target and broker were originally in separate folders
> because they were built separately (and sat under the Windows chromium
> sandbox code hence the src), but they're not any more.
> 

I think they still are.  Their moz.builds are just included by
security/sandbox/moz.build [2].

---

In comment 16, you had said:

> Perhaps we could add telemetry for when the broker doesn't grant
> permission to confirm it's not a problem.
> Not sure how much of a pain that will be given where this code is.

I added a callback in the case of permissions failure so that I can now write telemetry.
I'm recording it in the patch as SANDBOX_BLOCKED_FILE.


---

[1] https://dxr.mozilla.org/mozilla-central/rev/96cb95af530477edb66ae48d98c18533476e57bb/security/sandbox/chromium/sandbox/win/src/sandbox.h#48
[2] https://dxr.mozilla.org/mozilla-central/rev/96cb95af530477edb66ae48d98c18533476e57bb/security/sandbox/moz.build#20
Attachment #8809244 - Attachment is obsolete: true
Addresses jimm's review in comment 35.

> @@ +2241,5 @@
> > +        }
> > +        return ret;
> > +    }
> > +
> > +    switch (aFunc) {
> 
> We fall into this when e10s is disabled and oopp is enabled?

Honestly, I don't know.  The code is based on the GetKeyState override code [1].
I figure the condition is for some unusual case like non-e10s... but I think its
probably with oopp _disabled_.  



[1] https://dxr.mozilla.org/mozilla-central/rev/96cb95af530477edb66ae48d98c18533476e57bb/dom/plugins/ipc/PluginModuleChild.cpp#2134
Attachment #8809246 - Attachment is obsolete: true
Comment on attachment 8829027 [details] [diff] [review]
1. Add opcodes to nsWindowsDllInterceptor for GetSaveFileNameW, GetOpenFileNameW, SetCapture and ImmReleaseContext.

Fixed hg issue with a version upgrade.  Setting patches for review.
Attachment #8829027 - Flags: review?(aklotz)
Attachment #8829029 - Flags: review?(bobowencode)
Attachment #8829032 - Flags: review?(jmathies)
Comment on attachment 8829032 [details] [diff] [review]
5. Hooked GetSaveFileNameW/GetOpenFileNameW

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

::: dom/plugins/ipc/PluginModuleChild.cpp
@@ +2155,5 @@
> +BOOL WINAPI PMCGetSaveFileNameW(LPOPENFILENAMEW lpofn);
> +BOOL WINAPI PMCGetOpenFileNameW(LPOPENFILENAMEW lpofn);
> +
> +// Runnable that performs GetOpenFileNameW and GetSaveFileNameW
> +// on the main thread so that the can be

nit - so that the (call)?

@@ +2196,5 @@
> +
> +// static
> +BOOL
> +PostToPluginThread(GetFileNameFunc aFunc, void* aLpofn) {
> +    // Synchronously run from the main thread.  Start a semaphore at 0.

lets add an AssertPluginThread();
Attachment #8829032 - Flags: review?(jmathies) → review+
Comment on attachment 8829029 [details] [diff] [review]
3. Track names of files that have been given special sandbox access permissions

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

Just a few more things, r+ with those.

Can you ask glandium to review the build changes once you have fixed these and xpcshell changes if you want to keep those.
There's the data review as well.

(In reply to David Parks (dparks) [:handyman] from comment #43)

> > > +  virtual void GrantFileAccess(uint32_t aPluginId, const wchar_t* aFilename,
> > > +                              bool aPermitWrite);
> > 
> > Also can these be pure virtual?
> 
> The methods are actually defined in the .cpp file.  They are only virtual
> because:

Don't know what I was thinking here. :)

> > I think that the target and broker were originally in separate folders
> > because they were built separately (and sat under the Windows chromium
> > sandbox code hence the src), but they're not any more.
> > 
> 
> I think they still are.  Their moz.builds are just included by
> security/sandbox/moz.build [2].

I meant that the sandbox broker was built into a separate DLL, but anyway I've realised that I probably need to move what is already in security/sandbox/win, before these can all move.
So lets leave this like the others as you say and I'll move them all together later on.

::: js/xpconnect/shell/xpcshell.cpp
@@ +57,5 @@
>  #if defined(XP_WIN) && defined(MOZ_SANDBOX)
>      shellData.sandboxBrokerServices =
>        mozilla::sandboxing::GetInitializedBrokerServices();
> +    shellData.sandboxPermissionsService =
> +      mozilla::sandboxing::GetPermissionsService();

I take your point over including this for xpcshell, as this is conceptually part of BrokerServices.
However, unless we're going to have xpcshell tests for the plugin file access, I'm still not sure we really need it.

Anyway, we'll flag glandium in the next round of reviews for the build changes and he can review for xpcshell as well, if you want to keep these.

::: security/sandbox/chromium-shim/sandbox/win/permissionsService.cpp
@@ +38,5 @@
> +  std::transform(nameCopy.begin(), nameCopy.end(), nameCopy.begin(), towupper);
> +  if (StringEndsWith(nameCopy, ZONE_ID_DATA_STR)) {
> +    nameCopy = nameCopy.substr(0, nameCopy.size() - ZONE_ID_DATA_STR.size());
> +  }
> +  else if (StringEndsWith(nameCopy, ZONE_IDENTIFIER_STR)) {

nit: else on same line as }

@@ +47,5 @@
> +
> +/* static */ PermissionsService*
> +PermissionsService::GetInstance()
> +{
> +  static PermissionsService sPermissionsService;

I guess this is OK not being a pointer as we shouldn't be getting any calls from children that might call this by the time this gets destroyed.

Although now this is in chromium-shim maybe this could inherit from SingletonBase<PersmissionService> like BrokerServicesBase and we'd get this for free.

@@ +57,5 @@
> +{
> +}
> +
> +void
> +PermissionsService::GrantFileAccess(uint32_t aPluginId,

All of these should be aProcessId and in other files.

@@ +63,5 @@
> +                                    bool aPermitWrite)
> +{
> +  FilePermissionMap& permissions = mProcessFilePermissions[aPluginId];
> +  std::wstring filename = GetPlainFileName(aFilename);
> +  auto itPermission = permissions.find(filename);

This line can go as well.

@@ +128,5 @@
> +  return false;
> +}
> +
> +void
> +PermissionsService::RemovePermissionsForPlugin(uint32_t aPluginId)

I Think this should RemovePermissionsForProcess.

::: security/sandbox/chromium-shim/sandbox/win/permissionsService.h
@@ +34,5 @@
> +   * accesses that were denied.
> +   * Parameters are the filename and a boolean indicating the access request
> +   * was read-only (false) or read-write (true)
> +   */
> +  typedef void (*FileAccessViolationFunc)(const wchar_t*, bool);

We should remove the filename if we're not using it.

::: security/sandbox/win/src/sandboxpermissions/moz.build
@@ +11,5 @@
> +EXPORTS += [
> +    'sandboxPermissions.h',
> +]
> +
> +for var in ('UNICODE', '_UNICODE', 'SANDBOX_EXPORTS'):

You don't need SANDBOX_EXPORTS here.
Like I said we'll ask glandium to review the build parts.

::: security/sandbox/win/src/sandboxpermissions/sandboxPermissions.h
@@ +46,5 @@
> +  /*
> +   * Returns true if the user has granted the sandboxed plugin instance the
> +   * requested permission to open the file.
> +   */
> +  bool UserGrantedFileAccess(uint32_t aPluginId, const wchar_t* aFilename,

Is this actually needed?
Doesn't the sandbox code call the PermissionsService directly.

::: toolkit/xre/nsAppRunner.cpp
@@ +3005,5 @@
> +#if defined(MOZ_SANDBOX) && defined(XP_WIN)
> +static
> +void ReportSandboxBlockedAccess(const wchar_t* aFilename, bool aWriteAccess)
> +{
> +  Telemetry::Accumulate(Telemetry::SANDBOX_BLOCKED_FILE, aWriteAccess ? 1 : 0);

I think this is going to be confusing as read access blocked looks like it wasn't blocked.

Ideally having two separate count histograms would be good I think, say SANDBOX_BLOCKED_FILE_READ/WRITE.
Attachment #8829029 - Flags: review?(bobowencode) → review+
Attachment #8829027 - Flags: review?(aklotz) → review+
Refreshing patches.  Removed SetCapture test as SetCapture is no longer hooked in the code base.
Attachment #8829027 - Attachment is obsolete: true
Attachment #8829028 - Attachment is obsolete: true
Includes most changes from bobowen's review in comment 49.

I'm requesting some feedback from Bob for some mild questions:

> Can you ask glandium to review the build changes once you have fixed these
> and xpcshell changes if you want to keep those.

I think I've removed all of the xpcshell stuff.  Can you confirm?

Also, I don't actually know what the glandium review is about.  Do I still need it without the xpcshell changes?

> 
> @@ +47,5 @@
> > +
> > +/* static */ PermissionsService*
> > +PermissionsService::GetInstance()
> > +{
> > +  static PermissionsService sPermissionsService;
> 
> I guess this is OK not being a pointer as we shouldn't be getting any calls
> from children that might call this by the time this gets destroyed.
> 
> Although now this is in chromium-shim maybe this could inherit from
> SingletonBase<PersmissionService> like BrokerServicesBase and we'd get this
> for free.
> 

I started down this road but realized that the amount of scaffolding it would require was pretty large and probably unwarranted.  The Chromium code can use the SingletonBase class because it exposes BrokerServices, which is an abstract base class that is (confusingly) implemented by BrokerServicesBase.  BrokerServicesBase is then used directly in Chromium code.  I could implement a subclass internal to libsandbox so I could use this, and then use that in the Chromium code, but...

---

I'm also requesting a data review from bsmedberg for the telemetry code that counts the number of times file accesses were blocked by the sandbox.
Attachment #8829029 - Attachment is obsolete: true
Attachment #8835127 - Flags: feedback?(bobowencode)
Attachment #8835127 - Flags: feedback?(benjamin)
Refreshing patch
Attachment #8829030 - Attachment is obsolete: true
Refreshing patch.  Includes jimm's review notes from comment 48.
Attachment #8829032 - Attachment is obsolete: true
Comment on attachment 8835127 [details] [diff] [review]
3. Track names of files that have been given special sandbox access permissions (r=bobowen)

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

Looks good.
Yes, that's all the xpcshell changes removed, but I'd still like glandium to review the build changes.
glandium - you might want to cast your eye over the Bootstrap and XREAppData changes as well.
Attachment #8835127 - Flags: review?(mh+mozilla)
Attachment #8835127 - Flags: feedback?(bobowencode)
Attachment #8835127 - Flags: feedback+
Comment on attachment 8835127 [details] [diff] [review]
3. Track names of files that have been given special sandbox access permissions (r=bobowen)

The histogram descriptions need more details. Is this specifically for file accesses in the plugin processes or also for content? Which process is this histogram actually recorded in? Is this recorded once per open, or if a process tries to write to the same file multiple times does it increment each time? Without reading the code, I imagine technically this actually blocks the open() call, not the read/write call.

Also if this is windows-specific or currently only implemented on Windows, please document that. Do you think you'll have enough information from the opt-in population, or do you need to also check the release population for this?
Attachment #8835127 - Flags: feedback?(benjamin) → feedback-
Comment on attachment 8835127 [details] [diff] [review]
3. Track names of files that have been given special sandbox access permissions (r=bobowen)

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

::: security/sandbox/win/src/sandboxpermissions/moz.build
@@ +14,5 @@
> +
> +for var in ('UNICODE', '_UNICODE'):
> +    DEFINES[var] = True
> +
> +DISABLE_STL_WRAPPING = True

DISABLE_STL_WRAPPING doesn't sound like a good idea combined with FINAL_LIBRARY = 'xul'... makes me think we should probably reject that straight ahead (although there might be exceptional cases where it's needed).
Attachment #8835127 - Flags: review?(mh+mozilla)
> DISABLE_STL_WRAPPING doesn't sound like a good idea combined with
> FINAL_LIBRARY = 'xul'... makes me think we should probably reject that
> straight ahead (although there might be exceptional cases where it's needed).

Fair enough.  This moz.build is just a copy of sandboxbroker/moz.build -- I don't know what that's for but it really doesn't sound relevant here.  Removed.

I've also removed the Telemetry so this should be about ready to commit.  I've left the hooks for the Telemetry callback in case we decide to restore it.
Attachment #8835127 - Attachment is obsolete: true
Attachment #8837354 - Flags: review?(mh+mozilla)
Comment on attachment 8837354 [details] [diff] [review]
3. Track names of files that have been given special sandbox access permissions (r=bobowen)

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

r+ for the build system things, modulo a nit.

::: security/sandbox/win/src/sandboxpermissions/moz.build
@@ +19,5 @@
> +    '/security/sandbox/win',
> +]
> +
> +OS_LIBS += [
> +    'dbghelp',

The code in this new sandboxPermissions.cpp file is not using anything that would require dbghelp, so this is actually not needed.
Attachment #8837354 - Flags: review?(mh+mozilla) → review+
Includes glandium's change
Attachment #8837354 - Attachment is obsolete: true
freshening patch
Attachment #8835128 - Attachment is obsolete: true
freshening patch
Attachment #8835130 - Attachment is obsolete: true
Attachment #8838343 - Attachment description: 3. Track names of files that have been given special sandbox access permissions (r=bobowen) → 3. Track names of files that have been given special sandbox access permissions (r=bobowen,glandium)
Keywords: checkin-needed
Attachment #8807200 - Attachment is obsolete: true
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/12e17d5f0fa9
Add opcodes to nsWindowsDllInterceptor for GetSaveFileNameW, GetOpenFileNameW and ImmReleaseContext. r=aklotz
https://hg.mozilla.org/integration/mozilla-inbound/rev/21497a4e3bde
Add missing hooked methods to TestDllInterceptor. r=aklotz
https://hg.mozilla.org/integration/mozilla-inbound/rev/3ea8b8a18515
Add mechanism to libsandbox_s to track names of files that have been given special sandbox access permissions (PermissionsService). r=bobowen, r=glandium
https://hg.mozilla.org/integration/mozilla-inbound/rev/a5171791437f
Add mechanism to libsandbox_s to track names of files that have been given special sandbox access permissions (PermissionsService). r=bobowen
https://hg.mozilla.org/integration/mozilla-inbound/rev/19b2fcee13a9
Hook GetSaveFileNameW/GetOpenFileNameW to record and grant a sandboxed process permission to access user-chosen files. r=jimm
Keywords: checkin-needed
Win8's dragging its feet, but at least on Win7 that crashes rather a lot in xpcshell, like https://treeherder.mozilla.org/logviewer.html#?job_id=78176054&repo=mozilla-inbound

Backed out in https://hg.mozilla.org/integration/mozilla-inbound/rev/c3ebb1458e91
So thats what the xpcshell stuff did.

Apologies -- I'd totally forgotten to run a full suite of tests since that stuff came out.  This trivial change should fix the crashes -- it just adds an additional null check in SandboxPermissions::RemovePermissionsForProcess to make sure the sandboxing setup is even being used.

This time, tests:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=7d420b0d9eff0f72ae259cde2c2b6606d1a509ea

Bob will be available for reviews on the 25th.
Attachment #8838343 - Attachment is obsolete: true
Attachment #8838668 - Flags: review+
Thanks Bob!  Lets try again...
Keywords: checkin-needed
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/a176abd99d2b
Add opcodes to nsWindowsDllInterceptor for GetSaveFileNameW, GetOpenFileNameW and ImmReleaseContext. r=aklotz
https://hg.mozilla.org/integration/mozilla-inbound/rev/4e81ec8850dc
Add missing hooked methods to TestDllInterceptor. r=aklotz
https://hg.mozilla.org/integration/mozilla-inbound/rev/71b9ac06a60a
Add mechanism to libsandbox_s to track names of files that have been given special sandbox access permissions (PermissionsService). r=bobowen, r=glandium
https://hg.mozilla.org/integration/mozilla-inbound/rev/0740284125d3
Add mechanism to libsandbox_s to track names of files that have been given special sandbox access permissions (PermissionsService). r=bobowen
https://hg.mozilla.org/integration/mozilla-inbound/rev/c35afe490583
Hook GetSaveFileNameW/GetOpenFileNameW to record and grant a sandboxed process permission to access user-chosen files. r=jimm
Keywords: checkin-needed
Refreshing all the patches for the hell of it.
Attachment #8838341 - Attachment is obsolete: true
Flags: needinfo?(davidp99)
Added new sync (err.. intr but whatever) message to ipc/ipdl/sync-messages.ini.

The GetFileName message is sent from the plugin process to chrome because sandbox blocks the needed Windows call in the plugin process.  It proxies the call for Flash.  GetSaveFileName/GetOpenFileName, the actual proxied methods, now run on the chrome process, and are themselves synchronous -- this is standard Windows behavior for file-picker dialog boxes.

I'm not yet sure what the IPC review is looking for -- let me know if you need more info.
Attachment #8838345 - Attachment is obsolete: true
Attachment #8839775 - Flags: review?(benjamin)
(In reply to David Parks (dparks) [:handyman] from comment #76)
> Created attachment 8839775 [details] [diff] [review]
> 5. Hooked GetSaveFileNameW/GetOpenFileNameW (r=jimm)
> 
> Added new sync (err.. intr but whatever) message to
> ipc/ipdl/sync-messages.ini.
> 
> The GetFileName message is sent from the plugin process to chrome because
> sandbox blocks the needed Windows call in the plugin process.  It proxies
> the call for Flash.  GetSaveFileName/GetOpenFileName, the actual proxied
> methods, now run on the chrome process, and are themselves synchronous --
> this is standard Windows behavior for file-picker dialog boxes.
> 
> I'm not yet sure what the IPC review is looking for -- let me know if you
> need more info.

David, go ahead and land this, I'll ok the added ipc call. If there are issues we can address them in a follow up. This is just another plugin intr call, of which there are many.
Flags: needinfo?(davidp99)
Attachment #8839775 - Flags: review?(benjamin) → review+
Trying this again...
Flags: needinfo?(davidp99)
Keywords: checkin-needed
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/628e6086fc27
Add opcodes to nsWindowsDllInterceptor for GetSaveFileNameW, GetOpenFileNameW and ImmReleaseContext. r=aklotz
https://hg.mozilla.org/integration/mozilla-inbound/rev/e2609138ebe1
Add missing hooked methods to TestDllInterceptor. r=aklotz
https://hg.mozilla.org/integration/mozilla-inbound/rev/7be63656e4fb
Add mechanism to libsandbox_s to track names of files that have been given special sandbox access permissions (PermissionsService). r=bobowen, r=glandium
https://hg.mozilla.org/integration/mozilla-inbound/rev/6ecd19d25822
Add mechanism to libsandbox_s to track names of files that have been given special sandbox access permissions (PermissionsService). r=bobowen
https://hg.mozilla.org/integration/mozilla-inbound/rev/8804a3f8fb15
Hook GetSaveFileNameW/GetOpenFileNameW to record and grant a sandboxed process permission to access user-chosen files. r=jimm
Keywords: checkin-needed
wontfix for 53 per jimm on IRC
Depends on: 1342448
Depends on: 1342690
Blocks: 1344845
Blocks: 1357489
Blocks: 1366256
Depends on: 1410140
Depends on: 1411631
Product: Core → Core Graveyard
You need to log in before you can comment on or make changes to this bug.