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)
Tracking
(firefox50 wontfix, firefox51 wontfix, firefox52+ wontfix, firefox53 wontfix, firefox54 fixed)
RESOLVED
FIXED
mozilla54
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 | |
3. Track names of files that have been given special sandbox access permissions (r=bobowen,glandium)
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.
Comment 3•8 years ago
|
||
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)
Updated•8 years ago
|
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)
Comment 5•8 years ago
|
||
Bob, do you have a marking or tracker bug for things caused by Flash sandboxing?
Flags: needinfo?(bobowen.code)
Comment 6•8 years ago
|
||
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).
Updated•8 years ago
|
Whiteboard: sb? → sbwn1
Updated•8 years ago
|
Priority: -- → P3
Updated•8 years ago
|
Blocks: npapi-sandbox-64bit
Keywords: 64bit
Comment 7•8 years ago
|
||
(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.
Updated•8 years ago
|
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
Assignee | ||
Comment 9•8 years ago
|
||
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)
Assignee | ||
Comment 10•8 years ago
|
||
Reporter | ||
Comment 11•8 years ago
|
||
Why "Save File" (which is under user's control) should be forbidden in the first place?
Comment 12•8 years ago
|
||
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)
Updated•8 years ago
|
Assignee: nobody → davidp99
Assignee | ||
Comment 13•8 years ago
|
||
Assignee | ||
Comment 14•8 years ago
|
||
Assignee | ||
Comment 15•8 years ago
|
||
Assignee | ||
Comment 16•8 years ago
|
||
Assignee | ||
Comment 17•8 years ago
|
||
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 18•8 years ago
|
||
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+
Assignee | ||
Comment 19•8 years ago
|
||
(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.
Assignee | ||
Comment 20•8 years ago
|
||
Attachment #8807199 -
Attachment is obsolete: true
Assignee | ||
Comment 21•8 years ago
|
||
Attachment #8807198 -
Attachment is obsolete: true
Assignee | ||
Comment 22•8 years ago
|
||
Attachment #8807197 -
Attachment is obsolete: true
Assignee | ||
Comment 23•8 years ago
|
||
Assignee | ||
Comment 24•8 years ago
|
||
Attachment #8807196 -
Attachment is obsolete: true
Assignee | ||
Comment 25•8 years ago
|
||
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!
Assignee | ||
Updated•8 years ago
|
Attachment #8809242 -
Attachment description: Add opcodes to nsWindowsDllInterceptor for GetSaveFileNameW, GetOpenFileNameW and ImmReleaseContext. → Add opcodes to nsWindowsDllInterceptor for GetSaveFileNameW, GetOpenFileNameW, SetCapture and ImmReleaseContext.
Assignee | ||
Comment 26•8 years ago
|
||
The remaining bad nsWindowsDllInterceptor hooks are listed in Bug 1316415.
Assignee | ||
Updated•8 years ago
|
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.
Assignee | ||
Updated•8 years ago
|
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
Assignee | ||
Updated•8 years ago
|
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
Assignee | ||
Updated•8 years ago
|
Attachment #8809245 -
Attachment description: Chromium sandbox changes: Add test for special filesystem access → 4. Chromium sandbox changes: Add test for special filesystem access
Assignee | ||
Updated•8 years ago
|
Attachment #8809246 -
Attachment description: Hooked GetSaveFileNameW/GetOpenFileNameW → 5. Hooked GetSaveFileNameW/GetOpenFileNameW
Assignee | ||
Comment 27•8 years ago
|
||
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.
Assignee | ||
Comment 28•8 years ago
|
||
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)
Assignee | ||
Updated•8 years ago
|
Attachment #8809243 -
Flags: review?(aklotz)
Assignee | ||
Comment 29•8 years ago
|
||
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)
Assignee | ||
Updated•8 years ago
|
Attachment #8809245 -
Flags: review?(bobowencode)
Assignee | ||
Updated•8 years ago
|
Attachment #8809246 -
Flags: review?(bobowencode)
Comment 30•8 years ago
|
||
Just to let you know that it will probably be Monday before I get to these.
Updated•8 years ago
|
Blocks: support-win64
Updated•7 years ago
|
Attachment #8809243 -
Flags: review?(aklotz) → review+
Comment 31•7 years ago
|
||
[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.
status-firefox50:
--- → wontfix
status-firefox51:
--- → affected
status-firefox52:
--- → affected
status-firefox53:
--- → affected
tracking-firefox52:
--- → ?
Comment 32•7 years ago
|
||
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 33•7 years ago
|
||
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 34•7 years ago
|
||
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 35•7 years ago
|
||
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-
Comment 36•7 years ago
|
||
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.
Comment 38•7 years ago
|
||
How come the 12 most recent versions of Firefox have this bug unsolved? (41-52)
Comment 40•7 years ago
|
||
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-
Updated•7 years ago
|
Flags: needinfo?(aklotz)
Assignee | ||
Comment 41•7 years ago
|
||
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
Assignee | ||
Comment 43•7 years ago
|
||
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
Assignee | ||
Comment 44•7 years ago
|
||
Reviewed in comment 33.
Attachment #8809245 -
Attachment is obsolete: true
Assignee | ||
Comment 45•7 years ago
|
||
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
Assignee | ||
Comment 46•7 years ago
|
||
Tests look good: https://treeherder.mozilla.org/#/jobs?repo=try&revision=e9dce55fc8e5e9aed6465be956b29291f9a57206&selectedJob=70790869 (Patch #3 seems to have applied fine here. Grr.)
Assignee | ||
Comment 47•7 years ago
|
||
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)
Assignee | ||
Updated•7 years ago
|
Attachment #8829029 -
Flags: review?(bobowencode)
Assignee | ||
Updated•7 years ago
|
Attachment #8829032 -
Flags: review?(jmathies)
Comment 48•7 years ago
|
||
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 49•7 years ago
|
||
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+
Updated•7 years ago
|
Attachment #8829027 -
Flags: review?(aklotz) → review+
Assignee | ||
Comment 50•7 years ago
|
||
Refreshing patches
Assignee | ||
Comment 51•7 years ago
|
||
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
Assignee | ||
Comment 52•7 years ago
|
||
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)
Assignee | ||
Comment 53•7 years ago
|
||
Refreshing patch
Attachment #8829030 -
Attachment is obsolete: true
Assignee | ||
Comment 54•7 years ago
|
||
Refreshing patch. Includes jimm's review notes from comment 48.
Attachment #8829032 -
Attachment is obsolete: true
Assignee | ||
Comment 55•7 years ago
|
||
New tests: https://treeherder.mozilla.org/#/jobs?repo=try&revision=e3f30b6bc87dc0e38f4419f07a788a9f1e368ce8
Comment 56•7 years ago
|
||
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 57•7 years ago
|
||
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 58•7 years ago
|
||
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)
Assignee | ||
Comment 59•7 years ago
|
||
> 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 60•7 years ago
|
||
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+
Assignee | ||
Comment 61•7 years ago
|
||
freshening patch
Attachment #8835116 -
Attachment is obsolete: true
Assignee | ||
Comment 62•7 years ago
|
||
freshening patch
Attachment #8835118 -
Attachment is obsolete: true
Assignee | ||
Comment 63•7 years ago
|
||
Includes glandium's change
Attachment #8837354 -
Attachment is obsolete: true
Assignee | ||
Comment 64•7 years ago
|
||
freshening patch
Attachment #8835128 -
Attachment is obsolete: true
Assignee | ||
Comment 65•7 years ago
|
||
freshening patch
Attachment #8835130 -
Attachment is obsolete: true
Assignee | ||
Updated•7 years ago
|
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)
Assignee | ||
Updated•7 years ago
|
Keywords: checkin-needed
Updated•7 years ago
|
Attachment #8807200 -
Attachment is obsolete: true
Comment 66•7 years ago
|
||
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
Comment 67•7 years ago
|
||
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
Updated•7 years ago
|
status-firefox54:
--- → affected
Keywords: regression
Assignee | ||
Comment 68•7 years ago
|
||
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
Updated•7 years ago
|
Attachment #8838668 -
Flags: review+
Updated•7 years ago
|
Blocks: win64-rollout
Comment 70•7 years ago
|
||
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
Comment 71•7 years ago
|
||
Backed out for "Unknown sync IPC message PPluginModule::GetFileName"; needs review from an IPC peer now: https://hg.mozilla.org/integration/mozilla-inbound/rev/04b72382940faf1a2f7175ba3f28e4407ba23f9c https://hg.mozilla.org/integration/mozilla-inbound/rev/1112eb3a5f8c764f78440edb38079e17372a2d4b https://hg.mozilla.org/integration/mozilla-inbound/rev/4c7e6ba1ab9b30bf68edc49f49d84b82075c8e20 https://hg.mozilla.org/integration/mozilla-inbound/rev/883e0e945f7a5f43a465d3e4a0763b6f5ee5fbcd https://hg.mozilla.org/integration/mozilla-inbound/rev/19f1d4210d56f8c164cc02baeec2e42ca2ef792d Push with failures: https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&revision=c35afe490583c88e2c6a7041aad3d1451b3e82bc Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=79168110&repo=mozilla-inbound > /home/worker/workspace/build/src/dom/plugins/ipc/PPluginModule.ipdl:172: error: Unknown sync IPC message PPluginModule::GetFileName See https://groups.google.com/forum/#!topic/mozilla.dev.platform/WMisYpcuO74
Flags: needinfo?(davidp99)
Assignee | ||
Comment 72•7 years ago
|
||
Refreshing all the patches for the hell of it.
Attachment #8838341 -
Attachment is obsolete: true
Flags: needinfo?(davidp99)
Assignee | ||
Comment 76•7 years ago
|
||
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)
Comment 77•7 years ago
|
||
(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)
Updated•7 years ago
|
Attachment #8839775 -
Flags: review?(benjamin) → review+
Assignee | ||
Comment 78•7 years ago
|
||
Trying this again...
Flags: needinfo?(davidp99)
Keywords: checkin-needed
Comment 79•7 years ago
|
||
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
Comment 80•7 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/628e6086fc27 https://hg.mozilla.org/mozilla-central/rev/e2609138ebe1 https://hg.mozilla.org/mozilla-central/rev/7be63656e4fb https://hg.mozilla.org/mozilla-central/rev/6ecd19d25822 https://hg.mozilla.org/mozilla-central/rev/8804a3f8fb15
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla54
Comment 81•7 years ago
|
||
wontfix for 53 per jimm on IRC
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
•