1246054 - (CVE-2016-1966) Regressed bug 445229 exploitable plugin crash

Status: RESOLVED FIXED

(Core Graveyard :: Plug-ins, defect)

Description

[Daniel Veditz [:dveditz]]

Attachment: CESG Vulenrability Report (pdf)

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."

Comment 1

[Nicholas Nethercote [inactive]]

Ugh. I will investigate.

Comment 2

[Nicholas Nethercote [inactive]]

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 3

[Nicholas Nethercote [inactive]]

Attachment: Fix an erroneous nsNPObjWrapper assertion

Comment 4

[Nathan Froyd [:froydnj]]

Comment on attachment 8716183 [details] [diff] [review]
Fix an erroneous nsNPObjWrapper assertion

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

Doh.

Comment 5

[Nicholas Nethercote [inactive]]

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.

Comment 6

[Nicholas Nethercote [inactive]]

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.

Comment 7

[Nicholas Nethercote [inactive]]

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.

Comment 8

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

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.

Comment 9

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

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!

Comment 10

[(No longer employed by Mozilla) Aaron Klotz]

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.

Comment 11

[Nicholas Nethercote [inactive]]

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.

Comment 12

[Nicholas Nethercote [inactive]]

(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.

Comment 13

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/0b891e31fc10
https://hg.mozilla.org/releases/mozilla-beta/rev/f0d2911a9a4e

Comment 14

[Benjamin Smedberg]

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.

Comment 15

[Nicholas Nethercote [inactive]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/b4ceccae32b575d6109c1b8c053809e8238dcb63
Bug 1246054 - Fix an erroneous nsNPObjWrapper assertion. r=froydnj.

Comment 16

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/b4ceccae32b5

Comment 17

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

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.

Comment 18

[Daniel Veditz [:dveditz]]

Will this patch work for ESR-38 as-is?

Comment 19

[Nicholas Nethercote [inactive]]

Attachment: ESR38 patch

Comment 20

[Nicholas Nethercote [inactive]]

Comment on attachment 8720112 [details] [diff] [review]
ESR38 patch

This is a slightly different version of the patch that applies to ESR38.

Comment 21

[Nicholas Nethercote [inactive]]

Attachment: Fix an erroneous nsNPObjWrapper assertion

(Now with a better commit message.)

Comment 22

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

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

Comment 23

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/releases/mozilla-esr38/rev/291c2f31c48c