This bug was filed from the Socorro interface and is report bp-9754679b-a4c5-4ae0-a935-298c92150118. ============================================================= It seems the volume of this signature started to go up in v37: 37.0a2 69.27 % 284 38.0a1 15.85 % 65 37.0a1 14.39 % 59 36.0b2 0.49 % 2 (over the past 28 days) The Crash Address 0x5a5a5a5e worries me since it indicates a state that might be exploitable. All reported crashes are on Windows. Most of the crashes have mozilla::psm::SyncRunnableBase::Run() on the stack: nsPluginInstanceOwner::GetDocument(nsIDocument**) nsPluginStreamListenerPeer::GetInterfaceGlobal(nsID const&, void**) nsPluginStreamListenerPeer::GetInterface(nsID const&, void**) nsInterfaceRequestorAgg::GetInterface(nsID const&, void**) mozilla::net::nsHttpConnection::GetInterface(nsID const&, void**) nsInterfaceRequestorAgg::GetInterface(nsID const&, void**) nsGetInterface::operator()(nsID const&, void**) nsCOMPtr_base::assign_from_helper(nsCOMPtr_helper const&, nsID const&) xul.dll@0x190f6df @0x7ffdf6cb GetInputBits PreviousCertRunnable::RunOnTargetThread() mozilla::psm::SyncRunnableBase::Run() nsThread::ProcessNextEvent(bool, bool*) ...

This looks like an exploitable use after free, but I don't know whether to blame plugins, networking, PSM, or e10s here.

Aaron, do you think this could have something to do with bug 998863? That one rode 37.

(In reply to Georg Fritzsche [:gfritzsche] from comment #2) > Aaron, do you think this could have something to do with bug 998863? That > one rode 37. I'd be surprised if it were, but I can't definitively say no.

Ok, i think the other candidate, the e10s plugin work, rode mostly on 36. jimm maybe you can confirm? Any likely candidates you can think of?

Better stack on nightly - https://crash-stats.mozilla.com/report/index/5c3946b3-4c69-4960-bbbb-989792150128 Looks like mContent is null here - http://hg.mozilla.org/mozilla-central/annotate/38e4719e71af/dom/plugins/base/nsPluginInstanceOwner.cpp#l588 Due to random access of this api through this SyncRunnableBase thing? I really have no idea what's going on here, but we could try to patch up the null ptr problem in GetDocument.

> Looks like mContent is null here Actually, given the 0xffffffffffffffff crash address, its contents might have been memory-poisoned. See bug 1018360 comment #0.

> https://crash-stats.mozilla.com/report/index/5c3946b3-4c69-4960-bbbb-989792150128 If you look at the raw dump for this, you'll notice that rcx == 0x5a5a5a5a5a5a5a5a.

Any ideas here?

I see a couple of problems. First, of course, is a missing nullptr check in GetDocument. I also see https://dxr.mozilla.org/mozilla-central/source/dom/plugins/base/nsPluginHost.cpp#806 where we don't check the content pointer before initializing instanceOwner. Then, of course, we need to go down the rabbit hole of discovering how null content pointer could end up there in the first place. I haven't had time to do that yet.

This takes care of the worst part, at least.

Comment on attachment 8578256 [details] [diff] [review] Add null check for mContent Approval Request Comment [Feature/regressing bug #]: Plugins [User impact if declined]: Crashes, potential security issue [Describe test coverage new/current, TreeHerder]: Has been on inbound since yesterday [Risks and why]: None. Trivial patch adding null pointer check. [String/UUID change made/needed]: None.

This should have gotten sec-approval before landing, assuming that this is fixing more than just null derefs.

Sorry, I wasn't made aware of the whole sec-approval process until just now. It is just a null deref check. Should I flag it anyway?

If it is a null check preventing use-after-frees, then yes. It matters less now that the patch has already landed, but the information in the form is useful for deciding what is affected, etc.

Comment on attachment 8578256 [details] [diff] [review] Add null check for mContent [Security approval request comment] How easily could an exploit be constructed based on the patch? Any potential exploit would require additional knowledge about plugin workings and the relationship between plugins and content. Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem? No, it just mentions adding a null pointer check. Which older supported branches are affected by this flaw? All supported branches. If not all supported branches, which bug introduced the flaw? N/A Do you have backports for the affected branches? If not, how different, hard to create, and risky will they be? This patch should be easy to merge into affected branches. How likely is this patch to cause regressions; how much testing does it need? Simple null pointer check. Will not cause regressions.

(In reply to Aaron Klotz [:aklotz] (please use needinfo) from comment #16) > Which older supported branches are affected by this flaw? > All supported branches. Does that include ESR 31?

Yes.

Comment on attachment 8578256 [details] [diff] [review] Add null check for mContent sec-approval+ along with Beta and Aurora approval.

Do we have an ESR31 patch? It should go in too after ESR31 approval.

https://hg.mozilla.org/mozilla-central/rev/2b44490589d8 https://hg.mozilla.org/releases/mozilla-aurora/rev/1a341ad8d30d https://hg.mozilla.org/releases/mozilla-beta/rev/e92558fa59eb

This missed the ESR31 windows for 31.6.

I think we need to reopen or refile, this isn't fixed according to crash stats: https://crash-stats.mozilla.com/report/list?product=Firefox&signature=nsPluginInstanceOwner%3A%3AGetDocument%28nsIDocument**%29#tab-reports I see reports on 3-24

The remaining issues in this bug are mitigated by bug 1141081, so I'm going to set this one as a dupe.

OK, as we are already tracking bug 1141081, we are going to untrack this one.