1128064 - crash in nsPluginInstanceOwner::GetDocument(nsIDocument**)

Status: RESOLVED DUPLICATE of bug 1141081

(Core Graveyard :: Plug-ins, defect)

Description

[Mats Palmgren (inactive)]

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*)
...

Comment 1

[Daniel Veditz [:dveditz]]

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

Comment 2

[Georg Fritzsche [:gfritzsche]]

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

Comment 3

[(No longer employed by Mozilla) Aaron Klotz]

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

Comment 4

[Georg Fritzsche [:gfritzsche]]

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?

Comment 5

[Jim Mathies [:jimm]]

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.

Comment 6

[Steven Michaud [:smichaud] (Retired)]

> Looks like mContent is null here

Actually, given the 0xffffffffffffffff crash address, its contents might have been memory-poisoned.  See bug 1018360 comment #0.

Comment 7

[Steven Michaud [:smichaud] (Retired)]

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

Comment 8

[Andrew McCreight [:mccr8]]

Any ideas here?

Comment 9

[(No longer employed by Mozilla) Aaron Klotz]

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.

Comment 10

[(No longer employed by Mozilla) Aaron Klotz]

Attachment: Add null check for mContent

This takes care of the worst part, at least.

Comment 11

[(No longer employed by Mozilla) Aaron Klotz]

https://hg.mozilla.org/integration/mozilla-inbound/rev/2b44490589d8

Comment 12

[(No longer employed by Mozilla) Aaron Klotz]

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.

Comment 13

[Andrew McCreight [:mccr8]]

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

Comment 14

[(No longer employed by Mozilla) Aaron Klotz]

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?

Comment 15

[Andrew McCreight [:mccr8]]

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 16

[(No longer employed by Mozilla) Aaron Klotz]

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.

Comment 17

[Lawrence Mandel [:lmandel] (use needinfo)]

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

Comment 18

[(No longer employed by Mozilla) Aaron Klotz]

Yes.

Comment 19

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

Comment on attachment 8578256 [details] [diff] [review]
Add null check for mContent

sec-approval+ along with Beta and Aurora approval.

Comment 20

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

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

Comment 21

[Ryan VanderMeulen [:RyanVM]]

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

Comment 22

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

This missed the ESR31 windows for 31.6.

Comment 23

[Jim Mathies [:jimm]]

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

Comment 24

[(No longer employed by Mozilla) Aaron Klotz]

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

Comment 25

[Sylvestre Ledru [:Sylvestre]]

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