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

RESOLVED FIXED

Status

()

Core
Plug-ins
RESOLVED FIXED
5 years ago
3 years ago

People

(Reporter: tnikkel, Assigned: smichaud)

Tracking

({csectype-wildptr, sec-critical})

Trunk
csectype-wildptr, sec-critical
Points:
---

Firefox Tracking Flags

(firefox15 wontfix, firefox16 wontfix, firefox17 wontfix, firefox18 wontfix, firefox19 fixed, firefox-esr10 wontfix, firefox-esr17 wontfix, b2g18 unaffected)

Details

(Whiteboard: to be fixed by bug 598397 [adv-main19+], crash signature)

(Reporter)

Description

5 years ago
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.
(Reporter)

Comment 1

5 years ago
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]
Keywords: csec-wildptr, sec-critical
(Assignee)

Comment 3

5 years ago
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?
(Assignee)

Comment 5

5 years ago
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.
(Assignee)

Comment 6

5 years ago
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.
(Assignee)

Comment 7

5 years ago
(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
(Reporter)

Comment 9

5 years ago
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
(Reporter)

Comment 10

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

Comment 11

5 years ago
(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.
status-firefox-esr10: --- → wontfix
status-firefox15: --- → wontfix
status-firefox16: --- → wontfix
status-firefox17: --- → wontfix
status-firefox18: --- → wontfix
status-firefox19: --- → fixed
Marking as fixed because Carbon plugins have been removed.
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED

Updated

5 years ago
status-firefox-esr17: --- → wontfix

Updated

5 years ago
status-b2g18: --- → unaffected
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.