Closed Bug 790373 Opened 8 years ago Closed 7 years ago

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

Categories

(Core :: Plug-ins, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
Tracking Status
firefox15 --- wontfix
firefox16 --- wontfix
firefox17 --- wontfix
firefox18 --- wontfix
firefox19 --- fixed
firefox-esr10 --- wontfix
firefox-esr17 --- wontfix
b2g18 --- unaffected

People

(Reporter: tnikkel, Assigned: smichaud)

References

Details

(Keywords: csectype-wildptr, sec-critical, Whiteboard: to be fixed by bug 598397 [adv-main19+])

Crash Data

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.
This code only exists for Carbon plugins, and we are planning on getting rid of Carbon plugins in bug 598397.
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.
Crash Signature: [@ nsGfxScrollFrameInner::ScrollToImpl] [@ nsGfxScrollFrameInner::AsyncScrollCallback] [@ @0x0 | nsGfxScrollFrameInner::AsyncScrollCallback] [@ nsGfxScrollFrameInner::ScrollToWithOrigin] [@ @0x0 | nsGfxScrollFrameInner::ScrollToWithOrigin]
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.
Depends on: 598397
Steven, odd indeed. Josh when are we expecting to be rid of carbon plugins?
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.
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.
(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.
Steven, can you get any traction here?
Assignee: nobody → smichaud
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.
Whiteboard: to be fixed by bug 598397
And to be clear of the crash in bug 788436 was not the potential problem described in comment 0.
(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.
Marking as fixed because Carbon plugins have been removed.
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Unaffected for b2g18 because B2G doesn't have plugins \o/
Whiteboard: to be fixed by bug 598397 → to be fixed by bug 598397 [adv-main19+]
Group: core-security
You need to log in before you can comment on or make changes to this bug.