365.66 KB, application/pdf
1.27 KB, patch
|Details | Diff | Splinter Review|
1.31 KB, patch
|Details | Diff | Splinter Review|
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.
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 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.
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!
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.
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.
(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/integration/mozilla-inbound/rev/b4ceccae32b575d6109c1b8c053809e8238dcb63 Bug 1246054 - Fix an erroneous nsNPObjWrapper assertion. r=froydnj.
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?
Group: dom-core-security → core-security-release
Comment on attachment 8720112 [details] [diff] [review] ESR38 patch This is a slightly different version of the patch that applies to ESR38.
Attachment #8720112 - Flags: review+
(Now with a better commit message.)
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
Version: unspecified → 38 Branch
You need to log in before you can comment on or make changes to this bug.