Closed
Bug 1108035
Opened 9 years ago
Closed 9 years ago
Hook functions in plugin-container for Flash to configure Flash protected mode off
Categories
(Core Graveyard :: Plug-ins, defect)
Tracking
(firefox35 verified, firefox36 verified, firefox37 verified)
VERIFIED
FIXED
mozilla37
People
(Reporter: benjamin, Assigned: benjamin)
References
Details
Attachments
(1 file)
15.97 KB,
patch
|
bugzilla
:
review+
benjamin
:
approval-mozilla-aurora+
benjamin
:
approval-mozilla-beta+
|
Details | Diff | Splinter Review |
We want to have the ability to dynamically force-disable Flash protected mode. When Flash is launched, it reads a config file: “%WINDIR%\System32\Macromed\Flash” or “%WINDIR%\SysWOW64\Macromed\Flash” That file needs a new line "ProtectedMode = 0" which will disable protected mode. To do this, I'd like to hook the function that Adobe uses to open the file (probably OpenFileW, but I haven't checked), and replace the handle with a separate replaced or altered file. In Firefox, this should be controlled by the pref dom.ipc.plugins.flash.disable-protected-mode, which should default to false. This is primarily for and can be limited to 32-bit builds for now.
Assignee | ||
Updated•9 years ago
|
Group: mozilla-employee-confidential
Assignee | ||
Updated•9 years ago
|
Assignee: aklotz → benjamin
Assignee | ||
Comment 1•9 years ago
|
||
Filename "mms.cfg" in the directories above.
Assignee | ||
Comment 2•9 years ago
|
||
Attachment #8535305 -
Flags: review?(aklotz)
Comment 3•9 years ago
|
||
Comment on attachment 8535305 [details] [diff] [review] flash-hook Review of attachment 8535305 [details] [diff] [review]: ----------------------------------------------------------------- The patch looks good but I do have one concern about what happens if ProtectedMode already exists in the config file. I'd like to know more about that before r+'ing. ::: dom/plugins/ipc/PluginModuleChild.cpp @@ +1911,5 @@ > #endif > + > +#ifdef XP_WIN > + CleanupProtectedModeHook(); > +#endif When async init lands this call will need to be moved. If this patch lands first, I'll resolve this conflict on my end. Just a heads up. @@ +1924,5 @@ > +{ > + static const WCHAR kConfigFile[] = L"mms.cfg"; > + static const size_t kConfigLength = ArrayLength(kConfigFile) - 1; > + > + while (true) { // goto out, in sheep's clothing haha @@ +1970,5 @@ > + } > + } > + CloseHandle(original); > + } > + static const char kSettingString[] = "\nProtectedMode=0\n"; What if the config file already contains a ProtectedMode entry? Do we know what the behavior of Flash is around this case? Will Flash always use the latest value in the file? Or do we (yuck) need to be smarter about how we process the original config file?
Attachment #8535305 -
Flags: review?(aklotz) → review-
Comment 4•9 years ago
|
||
(In reply to Aaron Klotz [:aklotz] (please use needinfo) from comment #3) > Comment on attachment 8535305 [details] [diff] [review] > flash-hook > > Review of attachment 8535305 [details] [diff] [review]: > ----------------------------------------------------------------- > > The patch looks good but I do have one concern about what happens if > ProtectedMode already exists in the config file. I'd like to know more about > that before r+'ing. > > ::: dom/plugins/ipc/PluginModuleChild.cpp > @@ +1911,5 @@ > > #endif > > + > > +#ifdef XP_WIN > > + CleanupProtectedModeHook(); > > +#endif > > When async init lands this call will need to be moved. If this patch lands > first, I'll resolve this conflict on my end. Just a heads up. Actually I take this one back, this is OK in PluginModuleChild. :-)
Assignee | ||
Comment 5•9 years ago
|
||
I verified via local testing that Flash takes the first setting in the file: so if a user explicitly specifies ProtectedMode=1, then they get protected mode.
Updated•9 years ago
|
Attachment #8535305 -
Flags: review- → review+
Assignee | ||
Comment 6•9 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/7bd6fd85defc
Make sure to test this on all Windows versions, to check that we understand the disassembly and in case there's any DLL forwarding. And/or mark the cases that failed so they don't get counted in the experiment numbers. That may be a good thing to do anyway in case AddHook fails for other reasons.
Assignee | ||
Updated•9 years ago
|
Assignee | ||
Comment 8•9 years ago
|
||
Comment on attachment 8535305 [details] [diff] [review] flash-hook Approval Request Comment [Feature/regressing bug #]: New code for running an experiment [Describe test coverage new/current, TBPL]: Manual coverage only. This needs some additional external verification on various Windows versions. [Risks and why]: In the default configuration with the pref disabled, the risk of this is very low. For the experiment, the pref will be disabled for a set of users, and that will significantly change the behavior of Flash on Windows Vista+ and has significant risk. But we believe that risk is worth it for measurement. [String/UUID change made/needed]: None
Attachment #8535305 -
Flags: approval-mozilla-beta?
Assignee | ||
Updated•9 years ago
|
Attachment #8535305 -
Flags: approval-mozilla-aurora?
Comment 9•9 years ago
|
||
Benjamin, can you provide the QE team with some detailed test instructions? Some scenarios and what exactly the QE team should look for to make sure that this is properly covered. We'll make sure to cover this on Win XP, Win Vista, Win 7, Win 8 and Win 8.1. Should the behavior on Win XP be different from the other versions? Would this need to be covered on both x64 and x86 versions of the OS? Does the Flash version matter?
Flags: needinfo?(benjamin)
Assignee | ||
Comment 10•9 years ago
|
||
In the short term, win64 is less important. Testing details: * When protected mode is active, you will see a plugin-container.exe and two FlashPlayerPlugin_<version>.exe processes. * When protected mode is not active, you will see only a plugin-container.exe * Flash uses protected mode by default on Windows Vista+ * Flash does not use protected mode on Windows XP * For each version of Windows, please run a page which uses Flash with the pref dom.ipc.plugins.flash.disable-protected-mode both false (the default) and true. Make sure that Flash functions and appears to work correctly, using common top sites such as youtube, facebook games, etc. Also make sure that protected mode is enabled or disabled correctly according to the pref and windows version.
Flags: needinfo?(benjamin)
Assignee | ||
Comment 11•9 years ago
|
||
We mostly care about the latest Flash version, although if we have time to test earlier versions, that might be useful additional testing.
Comment 12•9 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/7bd6fd85defc
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla37
Assignee | ||
Comment 13•9 years ago
|
||
Comment on attachment 8535305 [details] [diff] [review] flash-hook got a=lsblakk over email
Attachment #8535305 -
Flags: approval-mozilla-beta?
Attachment #8535305 -
Flags: approval-mozilla-beta+
Attachment #8535305 -
Flags: approval-mozilla-aurora?
Attachment #8535305 -
Flags: approval-mozilla-aurora+
Comment 14•9 years ago
|
||
https://hg.mozilla.org/releases/mozilla-aurora/rev/b3ab3869d346 Needs rebasing for beta uplift.
status-firefox35:
--- → affected
status-firefox36:
--- → fixed
status-firefox37:
--- → fixed
Flags: needinfo?(benjamin)
Assignee | ||
Comment 15•9 years ago
|
||
https://hg.mozilla.org/releases/mozilla-beta/rev/6b3bd23e6ffb
Flags: needinfo?(benjamin)
Comment 16•9 years ago
|
||
We started testing this on Aurora/Developer Edition today since we had builds available with the fix. We made an etherpad [1] where we will track all the testing that will be done. Tomorrow we will continue testing with Firefox 35beta4 and latest Nightly across Windows Vista+ 32-bit. [1]: - https://etherpad.mozilla.org/FlashProtectedMode
Comment 17•9 years ago
|
||
We finished testing the fix across channels and platforms. As specified in the etherpad from Comment 16, we covered Windows Vista x86, Windows 7 x86, Windows 8 x86 and Windows 8.1 x86, targeting the latest beta, aurora and nightly channels. Our concern at this point: we're still seeing the FlashPlayerPlugin processes after setting the pref to true and restarting the browser. It still happens after shutting down the browser completely and starting it again, as well. Please note that this only happens on Windows 8.1 x86 and Windows 8 x86, across channels.
Comment 18•9 years ago
|
||
ni on bsmedberg - do you want us to reopen this bug because of the issue in Comment 17 or create a new bug to track the Windows 8.1 specific issue?
Flags: needinfo?(benjamin)
Assignee | ||
Comment 19•9 years ago
|
||
Let's file a new one to keep branch landing status clear.
Flags: needinfo?(benjamin)
Comment 20•9 years ago
|
||
(In reply to Benjamin Smedberg [:bsmedberg] from comment #19) > Let's file a new one to keep branch landing status clear. The bug tracking this issue is Bug 1112709
Comment 21•9 years ago
|
||
(In reply to Marcia Knous [:marcia - use needinfo] from comment #20) > (In reply to Benjamin Smedberg [:bsmedberg] from comment #19) > > Let's file a new one to keep branch landing status clear. > > The bug tracking this issue is Bug 1112709 Since the issue related to this fix is tracked in a separate bug, I'm marking this one as verified.
Status: RESOLVED → VERIFIED
Assignee | ||
Comment 22•9 years ago
|
||
Marcia, now that bug 1112709 is fixed can you verify that this works correctly on Win8/8.1? You should be able to use nightly and just flip the pref.
Flags: needinfo?(mozillamarcia.knous)
Comment 23•9 years ago
|
||
(In reply to Benjamin Smedberg [:bsmedberg] from comment #22) > Marcia, now that bug 1112709 is fixed can you verify that this works > correctly on Win8/8.1? You should be able to use nightly and just flip the > pref. Testing results are on Line 33 of https://etherpad.mozilla.org/FlashProtectedMode
Flags: needinfo?(mozillamarcia.knous)
Comment 24•9 years ago
|
||
(In reply to Benjamin Smedberg [:bsmedberg] from comment #0) > We want to have the ability to dynamically force-disable Flash protected > mode. > > When Flash is launched, it reads a config file: > > “%WINDIR%\System32\Macromed\Flash” or > “%WINDIR%\SysWOW64\Macromed\Flash” > > That file needs a new line "ProtectedMode = 0" which will disable protected > mode. > > To do this, I'd like to hook the function that Adobe uses to open the file > (probably OpenFileW, but I haven't checked), and replace the handle with a > separate replaced or altered file. > > In Firefox, this should be controlled by the pref > dom.ipc.plugins.flash.disable-protected-mode, which should default to false. On my system this was set to true, is this turned on by default now?
Assignee | ||
Comment 25•9 years ago
|
||
Yes, bug 1119941.
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
•