Closed Bug 1451452 (CVE-2018-5165) Opened 6 years ago Closed 6 years ago

Flash Protected Mode enabled state is reverse of settings checkbox in 32-bit Firefox

Categories

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

x86
Windows
defect

Tracking

(firefox-esr52 unaffected, firefox59+ wontfix, firefox60+ verified, firefox61+ verified)

VERIFIED FIXED
mozilla61
Tracking Status
firefox-esr52 --- unaffected
firefox59 + wontfix
firefox60 + verified
firefox61 + verified

People

(Reporter: handyman, Assigned: handyman)

References

Details

(Keywords: regression, sec-low, Whiteboard: [adv-main60+])

Attachments

(2 files, 1 obsolete file)

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
The conditional in PluginModuleParent is bad but fixing it doesn't restore the sandbox (or, at least, doesn't restore the checkbox).  Still investigating.
NI jimm so you see this.  I'm still narrowing down the cause.
Flags: needinfo?(jmathies)
Attached image procexpwin7.jpg
I can't reproduce on Win7/32-bit release 59.0.2.
Flags: needinfo?(jmathies)
I can confirm the checkbox bug. Protected mode is enabled though. Once you confirm david, we can remove the security flag from this bug.
Flags: needinfo?(davidp99)
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.
Flags: needinfo?(davidp99)
Summary: Flash Protected Mode is off by default in 32-bit Firefox → Flash Protected Mode enabled state is reverse of settings checkbox in 32-bit Firefox
Blocks: 1414406
Assignee: davidp99 → nobody
Component: Security: Process Sandboxing → Plug-ins
Flags: needinfo?(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?
Flags: needinfo?(cpeterson) → needinfo?(davidp99)
Hardware: Unspecified → x86
(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.
Flags: needinfo?(davidp99)
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?
David, did you want to take this? Unassigned P1s make for sad times.
Flags: needinfo?(davidp99)
Blocks: 1454322
Downgrading to P2 until I find an owner. This isn't super serious but we want to get it fixed.
Flags: needinfo?(davidp99)
Priority: P1 → P2
Attached patch checkbox.patch (obsolete) — Splinter Review
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
Assignee: nobody → davidp99
Attachment #8968974 - Flags: review?(dtownsend)
I forgot that was just a build (cant build locally!).  Tests are here:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=5f14f0b1c9eacb877e16a48431ee198fd837008a
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.
Attachment #8968974 - Flags: review?(dtownsend) → review+
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
Attachment #8968974 - Attachment is obsolete: true
Group: core-security → dom-core-security
Keywords: checkin-needed
https://hg.mozilla.org/integration/mozilla-inbound/rev/1dfbb10696ab6ca83903852ae15cf6fac6155f10

Go ahead and request Beta approval on this too when you get a chance.
Group: dom-core-security → core-security-release
Flags: qe-verify+
Flags: needinfo?(davidp99)
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/1dfbb10696ab
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla61
Is there anything in bug 1454322 that wasn't covered here?  ie, can we dup 1454322 to this?
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
Flags: needinfo?(davidp99)
Attachment #8969005 - Flags: approval-mozilla-beta?
(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?
Flags: needinfo?(ryanvm)
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 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.
Attachment #8969005 - Flags: sec-approval?
Comment on attachment 8969005 [details] [diff] [review]
Properly display plugin settings checkboxes (r=mossop)

approvals given (including beta) after talking to Liz Henry.
Attachment #8969005 - Flags: sec-approval?
Attachment #8969005 - Flags: sec-approval+
Attachment #8969005 - Flags: approval-mozilla-beta?
Attachment #8969005 - Flags: approval-mozilla-beta+
Comment on attachment 8969005 [details] [diff] [review]
Properly display plugin settings checkboxes (r=mossop)

Let's uplift this for beta 16.
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.
Status: RESOLVED → VERIFIED
Flags: needinfo?(ciprian.georgiu)
Flags: needinfo?(ryanvm)
Whiteboard: [adv-main60+]
This bug is also verified as fixed on Beta 60.0b16, 32-bit build (20180426170554), under Windows 10 x64.
Flags: qe-verify+
Flags: needinfo?(ciprian.georgiu)
Alias: CVE-2018-5165
Group: core-security-release
Product: Core → Core Graveyard
You need to log in before you can comment on or make changes to this bug.