Closed Bug 1246054 (CVE-2016-1966) Opened 4 years ago Closed 4 years ago

Regressed bug 445229 exploitable plugin crash

Categories

(Core :: Plug-ins, defect)

38 Branch
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla47
Tracking Status
firefox44 - wontfix
firefox45 + fixed
firefox46 + fixed
firefox47 + fixed
firefox-esr38 45+ fixed

People

(Reporter: dveditz, Assigned: njn)

References

Details

(Keywords: regression, sec-high, Whiteboard: [adv-main45+][adv-esr38.7+][post-critsmash-triage]CESG REF: 54041238 / VULNERABILITY ID: 429485)

Attachments

(3 files, 1 obsolete file)

The CESG (https://www.cesg.gov.uk/) sent a vulnerability report to the security alias concerning a bug in NPAPI.

CESG REF: 54041238 / VULNERABILITY ID: 429485

Details in the attached PDF (unfortunately no testcase, as was true of the regressed bug 445229).

   "Firefox has suffered a regression of bug 445229. We believe there to
   be an incorrect assumption regarding the purpose of a certain variable
   assignment which is assumed to be obsolete. This may cause the NPAPI
   subsystem to crash. [...]

   The assignment at [0] is to re-validate a dangling pointer under
   certain conditions. [...]"

[0] refers to line 916 in
http://hg.mozilla.org/mozilla-central/file/ff99308cdefc/dom/plugins/base/nsJSNPRuntime.cpp#l1912  They continue

   "The comment, in both the before and after commits, explains the
   purpose of reloading the entry. There is an assumption that it is
   only required for the sake of the assertion. After changing the 
   assertion to a format that does not require 'entry', the re-assignment
   has been removed.

   The removal of the assignment leads to a dangling pointer dereference
   in a non-standard feature of Firefox (the NPAPI plugin subsystem).

   Proof of Concept

   The high-level PoC to trigger the vulnerability and cause a crash is
   as follows:

   1. write a NPAPI plug-in which has a function that creates and returns
   a new NPObject every time it is called;

   2. call that function in a loop from Javascript.

   The browser will likely crash when a HashTable Object resizes its
   underlying data storage."
Keywords: sec-high
Ugh. I will investigate.
I introduced the problem in bug 1246054 part 2. The mistake was simple and stupid: I removed the assignment to |entry|, presumably because I thought it was only used in the NS_ASSERTION, but it's actually re-used later in the function. The fix is to merely reinstate the assignment.
Comment on attachment 8716183 [details] [diff] [review]
Fix an erroneous nsNPObjWrapper assertion

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

Doh.
Attachment #8716183 - Flags: review?(nfroyd) → review+
Comment on attachment 8716183 [details] [diff] [review]
Fix an erroneous nsNPObjWrapper assertion

> [Security approval request comment]
> How easily could an exploit be constructed based on the patch?

It requires a specially-crafted NPAPI plug-in. The vulnerability report states the "Access Complexity" is "Medium" and the "Severity Score" is 7.1.

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

No. The commit message is deliberately slightly misleading, making it sound like it's just a badly-written assertion.

> Which older supported branches are affected by this flaw?

43 and onwards.

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

Bug 1246054.

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

The patch should backport trivially.

> How likely is this patch to cause regressions; how much testing does it need?

The patch is trivial and fixes a (in hindsight) very simple and obvious error that occurred during refactoring.

I have not tested the patch because I don't know how to write an NPAPI plug-in of the form described in the vulnerability report. (There was also no test made available for the original manifestation of this problem in bug 445229.) Nonetheless, it's clear that this patch almost certainly fixes the problem.
Attachment #8716183 - Flags: sec-approval?
Comment on attachment 8716183 [details] [diff] [review]
Fix an erroneous nsNPObjWrapper assertion

Approval Request Comment

[Feature/regressing bug #]: 1124973

[User impact if declined]: exploitable plugin crash

[Describe test coverage new/current, TreeHerder]: none, unfortunately. Requires a specially-crafted NPAPI plug-in to be written.

[Risks and why]: negligible. Patch is extremely simple.

[String/UUID change made/needed]: none.
Attachment #8716183 - Flags: approval-mozilla-beta?
Attachment #8716183 - Flags: approval-mozilla-aurora?
Comment on attachment 8716183 [details] [diff] [review]
Fix an erroneous nsNPObjWrapper assertion

The patch is definitely worth taking on Aurora and Beta. It might also be worth taking as a ride-along if we do a 44.0.1 release, because it is so simple.
Attachment #8716183 - Flags: approval-mozilla-release?
Comment on attachment 8716183 [details] [diff] [review]
Fix an erroneous nsNPObjWrapper assertion

sec-approval+ and Aurora and Beta approvals.

I'll leave ridealong decisions to release management.
Attachment #8716183 - Flags: sec-approval?
Attachment #8716183 - Flags: sec-approval+
Attachment #8716183 - Flags: approval-mozilla-beta?
Attachment #8716183 - Flags: approval-mozilla-beta+
Attachment #8716183 - Flags: approval-mozilla-aurora?
Attachment #8716183 - Flags: approval-mozilla-aurora+
Nicholas, my main concern with taking this patch as a ride-along is the fact that this has not been tested. Would it be possible for you to get this tested asap? Perhaps Aaron Klotz might be able to help. As a rule, I find it extremely disconcerting to take any fixes in RC/dot releases which have not been verified. Sorry!
Flags: needinfo?(n.nethercote)
Flags: needinfo?(aklotz)
I'm not really sure what I can do to move this along. I have no idea whether we have test coverage for that code path or not.
Flags: needinfo?(aklotz)
I'll take a look at what's involved in writing an NPAPI plug-in of the kind described in comment 0, but it won't be until Monday.
Flags: needinfo?(n.nethercote)
(In reply to Nicholas Nethercote [:njn] from comment #11)
> I'll take a look at what's involved in writing an NPAPI plug-in of the kind
> described in comment 0, but it won't be until Monday.

I just spent a couple of hours attempting this. I made some progress but ultimately couldn't get it to work. I may try some more, but if I can't get it to work I think it's reasonable to not take this to release, given that it's not sec-critical and requires an NPAPI plugin.
We already have an NPAPI plugin in-tree, dom/plugins/test/testplugin with a bunch of different test methods that are pretty easy to copy/crib from.
https://hg.mozilla.org/mozilla-central/rev/b4ceccae32b5
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla47
Comment on attachment 8716183 [details] [diff] [review]
Fix an erroneous nsNPObjWrapper assertion

See comment 9 and comment 12 on why this is not a must-fix for Fx44. I also checked with Dan and Al on this one and they concur.
Attachment #8716183 - Flags: approval-mozilla-release? → approval-mozilla-release-
Will this patch work for ESR-38 as-is?
Flags: needinfo?(n.nethercote)
Group: dom-core-security → core-security-release
Attached patch ESR38 patch (obsolete) — Splinter Review
Comment on attachment 8720112 [details] [diff] [review]
ESR38 patch

This is a slightly different version of the patch that applies to ESR38.
Flags: needinfo?(n.nethercote)
Attachment #8720112 - Flags: review+
Attachment #8720112 - Flags: approval-mozilla-esr38?
(Now with a better commit message.)
Attachment #8720112 - Attachment is obsolete: true
Attachment #8720112 - Flags: approval-mozilla-esr38?
Attachment #8720113 - Flags: review+
Attachment #8720113 - Flags: approval-mozilla-esr38?
Comment on attachment 8720113 [details] [diff] [review]
Fix an erroneous nsNPObjWrapper assertion

Sec-high issue that's fixed in 45+, taking it in esr38.7
Attachment #8720113 - Flags: approval-mozilla-esr38? → approval-mozilla-esr38+
Whiteboard: CESG REF: 54041238 / VULNERABILITY ID: 429485 → [post-critsmash-triage]CESG REF: 54041238 / VULNERABILITY ID: 429485
Whiteboard: [post-critsmash-triage]CESG REF: 54041238 / VULNERABILITY ID: 429485 → [adv-main45+][adv-esr38.7+][post-critsmash-triage]CESG REF: 54041238 / VULNERABILITY ID: 429485
Alias: CVE-2016-1966
Flags: in-testsuite?
Version: unspecified → 38 Branch
Group: core-security-release
You need to log in before you can comment on or make changes to this bug.