Hook functions in plugin-container for Flash to configure Flash protected mode off

VERIFIED FIXED in Firefox 35

Status

()

Core
Plug-ins
VERIFIED FIXED
3 years ago
2 years ago

People

(Reporter: bsmedberg, Assigned: bsmedberg)

Tracking

unspecified
mozilla37
x86
Windows Vista
Points:
---
Dependency tree / graph
Bug Flags:
qe-verify +

Firefox Tracking Flags

(firefox35 verified, firefox36 verified, firefox37 verified)

Details

Attachments

(1 attachment)

(Assignee)

Description

3 years ago
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

3 years ago
Group: mozilla-employee-confidential
(Assignee)

Updated

3 years ago
Blocks: 1110215
(Assignee)

Updated

3 years ago
Assignee: aklotz → benjamin
(Assignee)

Comment 1

3 years ago
Filename "mms.cfg" in the directories above.
(Assignee)

Comment 2

3 years ago
Created attachment 8535305 [details] [diff] [review]
flash-hook
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. :-)
(Assignee)

Comment 5

3 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.
Attachment #8535305 - Flags: review- → review+
(Assignee)

Comment 6

3 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

3 years ago
Flags: qe-verify+
Keywords: qaurgent, qawanted
(Assignee)

Comment 8

3 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

3 years ago
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)
(Assignee)

Comment 10

3 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

3 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.
https://hg.mozilla.org/mozilla-central/rev/7bd6fd85defc
Status: NEW → RESOLVED
Last Resolved: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla37
(Assignee)

Comment 13

3 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+
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

3 years ago
https://hg.mozilla.org/releases/mozilla-beta/rev/6b3bd23e6ffb
status-firefox35: affected → fixed
Flags: needinfo?(benjamin)
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)
(Assignee)

Comment 19

3 years ago
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
(Assignee)

Updated

3 years ago
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.
Status: RESOLVED → VERIFIED
status-firefox35: fixed → verified
status-firefox36: fixed → verified
status-firefox37: fixed → verified
Keywords: qaurgent, qawanted
(Assignee)

Comment 22

2 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)
(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)

Updated

2 years ago
Depends on: 1123966

Comment 24

2 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

2 years ago
Yes, bug 1119941.
(Assignee)

Updated

2 years ago
Blocks: 1133000
(Assignee)

Updated

2 years ago
Blocks: 1133003
You need to log in before you can comment on or make changes to this bug.