1451452 - (CVE-2018-5165) Flash Protected Mode enabled state is reverse of settings checkbox in 32-bit Firefox

Status: VERIFIED FIXED

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

Description

[David Parks [:handyman]]

STR:

1. Grab a recent 32-bit build of Firefox.  I have seen this with the latest release from [1].
2. Run it and create a new profile
3. Go to about:addons and then to the plugins tag
4. Under Shockwave Flash, click "more"

Actual results:
A checkbox labeled "Enable Adobe Flash protected mode", unchecked

Expected results:
A checkbox labeled "Enable Adobe Flash protected mode", checked

-------

This means that the 32-bit builds are not running the Adobe Flash sandbox by default, and our plugin sandbox doesn't run at all under 32-bit.  IOW, 32-bit plugin processes are not currently sandboxed at all.

This looks to be because the setting is tied to the Firefox plugin sandbox level, which should only apply to 64-bit builds [2].  I'm pretty sure removing the condition will fix the bug.

Note that there is an issue with the 32-bit sandbox on nightly that influences this -- bug 1446499, which is how I found this one.  But that bug does not exist outside of nightly.

-------

[1] https://www.mozilla.org/en-US/firefox/all/
[2] https://searchfox.org/mozilla-central/rev/a0665934fa05158a5a943d4c8b277465910c029c/dom/plugins/ipc/PluginModuleParent.cpp#556

Comment 1

[David Parks [:handyman]]

The conditional in PluginModuleParent is bad but fixing it doesn't restore the sandbox (or, at least, doesn't restore the checkbox).  Still investigating.

Comment 2

[David Parks [:handyman]]

NI jimm so you see this.  I'm still narrowing down the cause.

Comment 3

[Jim Mathies [:jimm]]

Attachment: procexpwin7.jpg

I can't reproduce on Win7/32-bit release 59.0.2.

Comment 4

[Jim Mathies [:jimm]]

I can confirm the checkbox bug. Protected mode is enabled though. Once you confirm david, we can remove the security flag from this bug.

Comment 5

[David Parks [:handyman]]

It seems like the checkbox is the negation of the state -- if the checkbox _is_ checked to "Enable Adobe Flash protected mode" then the sandbox is actually _disabled_.  We can remove the secure flag if that's acceptable but I would vote not to.  I'm changing the name of the bug.

Comment 6

[Chris Peterson [:cpeterson]]

[Tracking Requested - why for this release]:

(In reply to David Parks (dparks) [:handyman] from comment #5)
> It seems like the checkbox is the negation of the state -- if the checkbox
> _is_ checked to "Enable Adobe Flash protected mode" then the sandbox is
> actually _disabled_.  We can remove the secure flag if that's acceptable but
> I would vote not to.  I'm changing the name of the bug.

So Flash protected mode is still enabled by default? And this UI bug only becomes a security problem if the user starts toggling the "Enable Adobe Flash protected mode" checkbox (because checked=disabled and unchecked=enabled)?

This bug seems would be a good ride-along fix there is another dot release (59.0.3), but is probably doesn't necessitate a dot release on its own.

Should we reset users' dom.ipc.plugins.flash.disable-protected-mode pref values to re-enable Flash protected mode, in case users inadvertently disabled protected mode with the checkbox?

Comment 7

[David Parks [:handyman]]

(In reply to Chris Peterson [:cpeterson] from comment #6)
> So Flash protected mode is still enabled by default? And this UI bug only
> becomes a security problem if the user starts toggling the "Enable Adobe
> Flash protected mode" checkbox (because checked=disabled and
> unchecked=enabled)?

Correct.  By default, protected mode is on but "Enable Adobe Flash protected mode" is unchecked.

I don't see why we couldn't wait on this.  Its not that big a deal.  Definitely doesn't warrant sparking a new dot release.  I also wouldn't bother trying to finesse the checkbox in the event someone inadvertently disabled protected mode.

---

FYI, mozregression shows this as the regressing pushlog:

https://hg.mozilla.org/integration/autoland/pushloghtml?fromchange=69206021220625e9d7470fd16b0f1585ee7781c2&tochange=00dd7dfadff3816d02cd6d1bcce8836809d5e22f

jimm and I had reasoned our way to pluginPrefs.xul changes in this patch:

https://hg.mozilla.org/integration/autoland/rev/a44297e0db42#l2.41

I've marked the `inverted="true"` line as it looks to me to be the missing part.  But I don't know xul.  That might turn out not to be relevant at all.

Comment 8

[Dave Townsend [:mossop]]

Basically these lines need to be fixed for the appropriate pref: https://searchfox.org/mozilla-central/source/toolkit/mozapps/extensions/content/pluginPrefs.js#17,19

I can take this or review the code if someone else wants to do that?

Comment 9

[Ryan VanderMeulen [:RyanVM]]

David, did you want to take this? Unassigned P1s make for sad times.

Comment 10

[Jim Mathies [:jimm]]

Downgrading to P2 until I find an owner. This isn't super serious but we want to get it fixed.

Comment 11

[David Parks [:handyman]]

Attachment: checkbox.patch

Maybe a bit tardy (I lost my ability to do local 32-bit builds when I upgraded to VS2017) but here we go.

Tests:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=a849591371b394a3780d664694aad0ee1205f8bf&selectedJob=174273950

Comment 12

[David Parks [:handyman]]

I forgot that was just a build (cant build locally!).  Tests are here:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=5f14f0b1c9eacb877e16a48431ee198fd837008a

Comment 13

[Dave Townsend [:mossop]]

Comment on attachment 8968974 [details] [diff] [review]
checkbox.patch

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

::: toolkit/mozapps/extensions/content/pluginPrefs.js
@@ +16,5 @@
>  function init() {
>    for (let id of Object.keys(PREFS)) {
>      let checkbox = document.getElementById(id);
> +    var prefVal = Services.prefs.getBoolPref(PREFS[id].pref);
> +    checkbox.checked = PREFS[id].invert ? (!prefVal) : prefVal;

The brackets shouldn't be necessary here or below.

Comment 14

[David Parks [:handyman]]

Attachment: Properly display plugin settings checkboxes (r=mossop)

I wasn't sure about the JS conventions on parens (I would have requested them for C code but JS is always different).  I've taken them out.

Tests: https://treeherder.mozilla.org/#/jobs?repo=try&revision=cf7e65a313b502b2077bd309b34d9e72134173f4

Comment 15

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/1dfbb10696ab6ca83903852ae15cf6fac6155f10

Go ahead and request Beta approval on this too when you get a chance.

Comment 16

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/1dfbb10696ab

Comment 17

[Andrew Swan [:aswan]]

Is there anything in bug 1454322 that wasn't covered here?  ie, can we dup 1454322 to this?

Comment 18

[David Parks [:handyman]]

Comment on attachment 8969005 [details] [diff] [review]
Properly display plugin settings checkboxes (r=mossop)

Patch is looking good at this point.

Approval Request Comment
[Feature/Bug causing the regression]:
bug 1414406

[User impact if declined]:
The Flash sandbox activation status is the opposite of the checkbox in about:addons.

[Is this code covered by automated tests?]:
No

[Has the fix been verified in Nightly?]:
Yes

[Needs manual test from QE? If yes, steps to reproduce]: 
No but STR in comment 0

[List of other uplifts needed for the feature/fix]:
None

[Is the change risky?]:
No,

[Why is the change risky/not risky?]:
The (minor) change negates the firefox settings value for some checkbox settings.  It can't harm security and, since it compiles as part of opening the page, it shouldn't be able to crash.

[String changes made/needed]:
N/A

Comment 19

[Al Billings [:abillings - ex-MoCo]]

(In reply to Ryan VanderMeulen [:RyanVM] from comment #15)
> https://hg.mozilla.org/integration/mozilla-inbound/rev/
> 1dfbb10696ab6ca83903852ae15cf6fac6155f10
> 
> Go ahead and request Beta approval on this too when you get a chance.

Why didn't this get sec-approval before landing? Can someone fill out the sec-approval? template?

Comment 20

[Daniel Veditz [:dveditz]]

We don't need the sec-approval template, calling this a sec-moderate. Hardly any users are going to dig this far into the menus so the number who get confused and turn sandboxing OFF is probably small. Do we have any telemetry on that from 59? But turning off the sandbox--especially by the people who are specifically worried about it to the extent they found this setting--is dangerous for those users.

Comment 21

[David Parks [:handyman]]

Comment on attachment 8969005 [details] [diff] [review]
Properly display plugin settings checkboxes (r=mossop)

Oops.  My error.

[Security approval request comment]
How easily could an exploit be constructed based on the patch?
I'd say that an exploit can't be made from the patch because the security implication is that the user mistakenly believes they are in the sandbox when they are not, all _after_ they change the sandbox setting.  Having said that, I could envision a phishing attempt that tells people to "be safe by turning on this flash sandbox setting (which would actually turn it off -- thats this bug) and then visit <nefarious page>".

Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem?
No

Which older supported branches are affected by this flaw?
beta, release 

If not all supported branches, which bug introduced the flaw?
bug 1414406

Do you have backports for the affected branches? If not, how different, hard to create, and risky will they be?
This patch should apply

How likely is this patch to cause regressions; how much testing does it need?
Very unlikely to cause regressions.  Testing seems unnecessary.

Comment 22

[Al Billings [:abillings - ex-MoCo]]

Comment on attachment 8969005 [details] [diff] [review]
Properly display plugin settings checkboxes (r=mossop)

approvals given (including beta) after talking to Liz Henry.

Comment 24

[Liz Henry (:lizzard) (relman/hg->git project)]

Comment on attachment 8969005 [details] [diff] [review]
Properly display plugin settings checkboxes (r=mossop)

Let's uplift this for beta 16.

Comment 25

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/releases/mozilla-beta/rev/dd61ef4a8a42

Comment 26

[Ciprian Georgiu, Desktop QA]

Reproduced this bug on an affected Nightly build 2018-04-04, using STR from comment 0.

Verified fixed on latest Nightly 61.0a1 (2018-04-24) with a 32-bit build running Win 10 x64 and Win 7 x64.

Ni myself as a reminder to verify this on Beta 16 as well.

Comment 27

[Ciprian Georgiu, Desktop QA]

This bug is also verified as fixed on Beta 60.0b16, 32-bit build (20180426170554), under Windows 10 x64.