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)

x86
Windows Vista
defect
Not set
normal

Tracking

(firefox35 verified, firefox36 verified, firefox37 verified)

VERIFIED FIXED
mozilla37
Tracking Status
firefox35 --- verified
firefox36 --- verified
firefox37 --- verified

People

(Reporter: benjamin, Assigned: benjamin)

References

Details

Attachments

(1 file)

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.
Group: mozilla-employee-confidential
Blocks: 1110215
Assignee: aklotz → benjamin
Filename "mms.cfg" in the directories above.
Attached patch flash-hookSplinter Review
Attachment #8535305 - Flags: review?(aklotz)
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-
(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. :-)
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.
Attachment #8535305 - Flags: review- → review+
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.
Flags: qe-verify+
Keywords: qaurgent, qawanted
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?
Attachment #8535305 - Flags: approval-mozilla-aurora?
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)
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)
We mostly care about the latest Flash version, although if we have time to test earlier versions, that might be useful additional testing.
https://hg.mozilla.org/mozilla-central/rev/7bd6fd85defc
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla37
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+
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
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.
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)
Let's file a new one to keep branch landing status clear.
Flags: needinfo?(benjamin)
(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
Depends on: 1112709
(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.
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)
(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)
Depends on: 1123966
(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?
Yes, bug 1119941.
Blocks: 1133000
Blocks: 1133003
Product: Core → Core Graveyard
You need to log in before you can comment on or make changes to this bug.