790373 - plugin scroll position listener callback may destroy listener list (crash of bug 788436)

Status: RESOLVED FIXED

(Core Graveyard :: Plug-ins, defect)

Description

[Timothy Nikkel (:tnikkel)]

smaug noticed that the crash in bug 788436 could be caused because while we are iterating over the list of scroll position listeners in nsGfxScrollFrameInner::ScrollToImpl the nsPluginInstanceOwner::ScrollPositionWillChange call could easily end up in plugin code that could potentially do anything, including change the list of scroll position listeners and cause the crash.

Filing this non-public bug so we can discuss this potential security problem.

Comment 1

[Timothy Nikkel (:tnikkel)]

This code only exists for Carbon plugins, and we are planning on getting rid of Carbon plugins in bug 598397.

Comment 2

[Daniel Veditz [:dveditz]]

Since we've seen instances involving both flash and silverlight it seems like the conditions are easy enough to cause should an attacker find this.

Comment 3

[Steven Michaud [:smichaud] (Retired)]

It's very odd that Flash and Silverlight can trigger crashes in code that's only used by Carbon plugins -- neither of them uses the Carbon event model.

Comment 4

[David Bolter [:davidb] (NeedInfo me for attention)]

Steven, odd indeed. Josh when are we expecting to be rid of carbon plugins?

Comment 5

[Steven Michaud [:smichaud] (Retired)]

I suspect that getting rid of the nsPluginInstanceOwner::ScrollPositionWillChange code won't help.  I suspect we end up there because of memory corruption, and if we got rid of that code we'd just crash somewhere else.

Note that (at bug 788436 comment #19) someone crashes in that code scrolling a page without any plugins!

Timothy Nikkel is creating a tryserver build for him with the nsPluginInstanceOwner::ScrollPositionWillChange code removed, just to see what happens.  So hopefully we'll know more in the not-too-distant future.

Comment 6

[Steven Michaud [:smichaud] (Retired)]

Another possibility, since the stacks at bug 788436 are so badly truncated, is that the memory corruption has made all those stacks invalid -- that the crashes aren't happening in nsPluginInstanceOwner::ScrollPositionWillChange at all.

Comment 7

[Steven Michaud [:smichaud] (Retired)]

(Following up comment #5 and comment #6)

Both of my hunches were wrong.

Bug 788436 was much more straightforward, and tnikkel has now fixed it.

Comment 8

[David Bolter [:davidb] (NeedInfo me for attention)]

Steven, can you get any traction here?

Comment 9

[Timothy Nikkel (:tnikkel)]

We found and fixed the crash of bug 788436. I don't think there is anything left that is important to do before we remove this code entirely as part of bug 598397.

Comment 10

[Timothy Nikkel (:tnikkel)]

And to be clear of the crash in bug 788436 was not the potential problem described in comment 0.

Comment 11

[Josh Aas]

(In reply to David Bolter [:davidb] from comment #4)
> Steven, odd indeed. Josh when are we expecting to be rid of carbon plugins?

I am planning to remove support for them near the beginning of the Firefox 19 release cycle.

Comment 12

[Andrew McCreight [:mccr8]]

Marking as fixed because Carbon plugins have been removed.

Comment 13

[Alex Keybl [:akeybl]]

Unaffected for b2g18 because B2G doesn't have plugins \o/