Closed Bug 1128064 Opened 9 years ago Closed 9 years ago

crash in nsPluginInstanceOwner::GetDocument(nsIDocument**)

Categories

(Core Graveyard :: Plug-ins, defect)

37 Branch
x86
Windows NT
defect
Not set
critical

Tracking

(firefox36 wontfix, firefox37- affected, firefox38- fixed, firefox39- fixed, firefox40 fixed, firefox-esr31 wontfix)

RESOLVED DUPLICATE of bug 1141081
mozilla40
Tracking Status
firefox36 --- wontfix
firefox37 - affected
firefox38 - fixed
firefox39 - fixed
firefox40 --- fixed
firefox-esr31 --- wontfix

People

(Reporter: MatsPalmgren_bugz, Assigned: bugzilla)

References

()

Details

(Keywords: crash, sec-high, Whiteboard: [adv-main38+])

Crash Data

Attachments

(1 file)

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.
Keywords: sec-high
Aaron, do you think this could have something to do with bug 998863? That one rode 37.
Flags: needinfo?(aklotz)
(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.
Flags: needinfo?(aklotz)
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?
Flags: needinfo?(jmathies)
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.
Flags: needinfo?(jmathies)
> 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.
Attachment #8578256 - Flags: review?(jmathies)
Attachment #8578256 - Flags: review?(jmathies) → review+
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.
Attachment #8578256 - Flags: approval-mozilla-beta?
Attachment #8578256 - Flags: approval-mozilla-aurora?
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?
Flags: needinfo?(continuation)
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.
Flags: needinfo?(continuation)
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.
Attachment #8578256 - Flags: sec-approval?
(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?
Assignee: nobody → aklotz
Flags: needinfo?(aklotz)
Yes.
Flags: needinfo?(aklotz)
Comment on attachment 8578256 [details] [diff] [review]
Add null check for mContent

sec-approval+ along with Beta and Aurora approval.
Attachment #8578256 - Flags: sec-approval?
Attachment #8578256 - Flags: sec-approval+
Attachment #8578256 - Flags: approval-mozilla-beta?
Attachment #8578256 - Flags: approval-mozilla-beta+
Attachment #8578256 - Flags: approval-mozilla-aurora?
Attachment #8578256 - Flags: approval-mozilla-aurora+
Do we have an ESR31 patch? It should go in too after ESR31 approval.
This missed the ESR31 windows for 31.6.
Whiteboard: [adv-main37+]
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
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Whiteboard: [adv-main37+]
The remaining issues in this bug are mitigated by bug 1141081, so I'm going to set this one as a dupe.
Status: REOPENED → RESOLVED
Closed: 9 years ago9 years ago
Resolution: --- → DUPLICATE
OK, as we are already tracking bug 1141081, we are going to untrack this one.
Target Milestone: mozilla39 → mozilla40
Whiteboard: [adv-main38+]
Group: core-security
Product: Core → Core Graveyard
You need to log in before you can comment on or make changes to this bug.