Closed Bug 493601 (CVE-2009-2467) Opened 15 years ago Closed 15 years ago

Investigate crash involving Flash module unloading

Categories

(Core Graveyard :: Plug-ins, defect)

x86
Windows XP
defect
Not set
normal

Tracking

(Not tracked)

VERIFIED FIXED
mozilla1.9.2a1

People

(Reporter: bsterne, Assigned: jimm)

References

Details

(Keywords: verified1.9.0.12, verified1.9.1.1, Whiteboard: [sg:critical?] PoC removed at request of reporter)

Attachments

(1 file, 1 obsolete file)

Attila Suszter reported this crash and proof-of-concept to security@m.o..  It appears to be caused by navigating the page and therefore unloading the Flash player while a dialog presented by the Flash player is still visible.

It helped me to increase the meta-refresh time in index.html to a higher number so the dialog has time to appear.  I don't have a Windows debug build available, so I wasn't able to provide more information about the call stack leading to the crash.  I'll attach the PoC momentarily.

Possibly related to bug 470487?
Confirming the crash on a 1.9.1 debug build. The stack trace is pretty worthless though at the time of the crash.
Flags: blocking1.9.1?
Hmm, this is exactly what the code that Mats Palmgren added at http://hg.mozilla.org/mozilla-central/file/4a32b3101121/layout/generic/nsObjectFrame.cpp#l1975 was to protect against. The plugin in question at the time that code was added was the Real plugin IIRC.
That's the same code in question for bug 470487, right?
(In reply to comment #4)
> That's the same code in question for bug 470487, right?

The code that landed in bug 420886 caused some of the problems I found while testing on 470487. However, I never reproduced the exact same stack so I can't say for sure that the original crash in 470487 was caused by that patch.

This crash happens with and without the patch in 420886.

Brandon, can you provide some details on what this flash app is doing?
I decompiled the Flash app and pulled out ~200K of JavaScript.  It wasn't obvious what it was trying to do.  There is a lot of code from libraries like Google Analytics, but perhaps those were just used as a starting basis for fuzzing.

Perhaps Attila (the reporter) can shed some light on what the Flash is doing here.
Need to investigate, also cc'ing Kev for Adobe-loop-in!
Flags: blocking1.9.1? → blocking1.9.1-
It looks like the script warning dialog gets created during tear down, after our delayed shutdown code executes and the real tear down begins. Also, the parent on the dialog points to the main fx frame.  I added a bit of code in our wm_destroy event handler to enumerate child windows and found that there were none.
I don't think the code in the flash really matters, it's just a lot of library code so Flash will throw up its "slow script" modal dialog.
Flags: wanted1.9.1.x+
Flags: wanted1.9.0.x+
Flags: blocking1.9.0.12?
Flags: blocking1.9.0.12? → blocking1.9.0.12+
I think I've found the problem here, working on a fix.
Attached patch wm_user bounce patch v.1 (obsolete) — Splinter Review
The event loop depth tracking we added works well for tracking depth for native events, but in this one case we are processing the user+1 events through a runnable. The thread that delivered these was both responsible for running the freezing script and generating the script warning dialog all in one shot. When we come around to destroy the plug-in, the event loop depth hasn't changed, so we go ahead and destroyed all the plug-in resources. The crash was occuring as the thread unwound.

This just bounces these events through the native event processing unwinding the stack. It shouldn't alter the timing of the overall stream of user events.
Attachment #380268 - Flags: review?(jst)
Assignee: nobody → jmathies
Comment on attachment 380268 [details] [diff] [review]
wm_user bounce patch v.1

Changing reviewer to josh since jst is on vacation until after 1.9.0.12 is done.
Attachment #380268 - Flags: review?(jst) → review?(joshmoz)
Attachment #380268 - Flags: review?(jmathies)
Comment on attachment 380268 [details] [diff] [review]
wm_user bounce patch v.1

Brendan, can you review win code? I wrote the patch so I don't think my r+ counts for much. ;)
Attachment #380268 - Flags: review?(jmathies) → review?(brendan)
Comment on attachment 380268 [details] [diff] [review]
wm_user bounce patch v.1

>+  if (msg == sWM_CUSTOMMSG) {
>+      // See PluginWindowEvent::Run() below.
>+      LRESULT res = TRUE;
>+      NS_TRY_SAFE_CALL_RETURN(res, ::CallWindowProc((WNDPROC)win->GetWindowProc(), hWnd, WM_USER+1, wParam, lParam),
>+                              nsnull, inst);
>+      return res;
>+  }

Nit: indentation basic offset is 2, not 4 as is used in the if for the code in the then block.

>+
>+  if (GetMsg() == WM_USER+1) {
>+    // XXX Unwind issues related to runnable event callback depth for this
>+    // event and destruction of the plugin. (Bug 493601)
>+    ::PostMessage(hWnd, sWM_CUSTOMMSG, GetWParam(), GetLParam());
>+  }
>+  else {
>+    // Currently not used, but added so that processing events here
>+    // is more generic.
>+    NS_TRY_SAFE_CALL_VOID(::CallWindowProc(win->GetWindowProc(), 
>+                          hWnd, 
>+                          GetMsg(), 
>+                          GetWParam(), 
>+                          GetLParam()),
>+                          nsnull, inst);

This goes a little over my non-Windows head. I can sr, dependent on josh's r+. Josh if you need a Windows guru try asking around on IRC.

/be
Attachment #380268 - Flags: review?(brendan) → superreview+
Attachment #380268 - Flags: review?(bent.mozilla)
Comment on attachment 380268 [details] [diff] [review]
wm_user bounce patch v.1

I'll review this soon but in the mean time I'd like to have an additional Windows reviewer, maybe Ben can help.
Attachment #380268 - Flags: review?(bent.mozilla) → review+
Comment on attachment 380268 [details] [diff] [review]
wm_user bounce patch v.1

So I'm not technically a windows/widget peer, you'd probably get better help from Ere Maijala (ere). Nevertheless this patch is pretty simple.

>+static UINT sWM_CUSTOMMSG = 0;

I'd give this a better name, something that describes a little about what it means.

>+  if (msg == sWM_CUSTOMMSG) {

I'd move this block into ProcessFlashMessageDelayed since it's really part of the same machinery.

And even though it seems impossible I'd assert that sWM_CUSTOMMSG != 0 here since that's technically WM_NULL and a valid message.

>+    sWM_CUSTOMMSG = ::RegisterWindowMessage(NS_PLUGIN_CUSTOM_MSG_ID);

The MSDN docs have this to say about RegisterWindowMessage:

  Only use RegisterWindowMessage when more than one application must process the
  same message. For sending private messages within a window class, an
  application can use any integer in the range WM_USER through 0x7FFF.

I know that plugins mess this pretty picture up a little, but are there no WM_USER messages reserved for the browser in plugin land?

>+  if (GetMsg() == WM_USER+1) {

If we're going to hardcode WM_USER+1 in more than one place it might be better to #define it somewhere with a name that makes sense. Some part of me thinks that it's a bad idea to add little hacks like this to something that is supposed to be generic (PluginWindowEvent) but then I remember that this is in the plugin code and the argument is ridiculous on its face ;-)
Attachment #380268 - Flags: review?(emaijala)
(In reply to comment #16)
> (From update of attachment 380268 [details] [diff] [review])
> So I'm not technically a windows/widget peer, you'd probably get better help
> from Ere Maijala (ere). Nevertheless this patch is pretty simple.
> 
> >+static UINT sWM_CUSTOMMSG = 0;
> 
> I'd give this a better name, something that describes a little about what it
> means.
> 
> >+  if (msg == sWM_CUSTOMMSG) {
> 
> I'd move this block into ProcessFlashMessageDelayed since it's really part of
> the same machinery.
> 
> And even though it seems impossible I'd assert that sWM_CUSTOMMSG != 0 here
> since that's technically WM_NULL and a valid message.
> 
> >+    sWM_CUSTOMMSG = ::RegisterWindowMessage(NS_PLUGIN_CUSTOM_MSG_ID);

Updated.

> 
> The MSDN docs have this to say about RegisterWindowMessage:
> 
>   Only use RegisterWindowMessage when more than one application must process
> the
>   same message. For sending private messages within a window class, an
>   application can use any integer in the range WM_USER through 0x7FFF.
> 
> I know that plugins mess this pretty picture up a little, but are there no
> WM_USER messages reserved for the browser in plugin land?

Anything seems possible, hence my reason for using this. It assures a unique message value system wide. Different instances will get the same value back registered under the string identifier, so system resource use will be minimal.

> 
> >+  if (GetMsg() == WM_USER+1) {
> 
> If we're going to hardcode WM_USER+1 in more than one place it might be better
> to #define it somewhere with a name that makes sense.

Added.

> Some part of me thinks
> that it's a bad idea to add little hacks like this to something that is
> supposed to be generic (PluginWindowEvent) but then I remember that this is in
> the plugin code and the argument is ridiculous on its face ;-)

It's a fix for a hack that is quite old which addresses a new problem brought on by some rather invasive code that landed a while back. For the release at least it's the simplest solution. The user flooding problem probably deserves some new investigation. That kind of work doesn't fit into the time frame we have here though AFAICT.

Will post an update here shortly.
Attachment #380268 - Attachment is obsolete: true
Attachment #380268 - Flags: review?(joshmoz)
Attachment #380268 - Flags: review?(emaijala)
Attachment #382646 - Flags: superreview?(joshmoz)
Attachment #382646 - Flags: review?(emaijala)
+static UINT sWM_FLASHBOUNCEMSG = 0;

statics default to 0, no need for = 0.
(In reply to comment #19)
> +static UINT sWM_FLASHBOUNCEMSG = 0;
> 
> statics default to 0, no need for = 0.

I'd prefer to leave that as is, there's no harm in being explicit in the initial value.
(In reply to comment #20)
> (In reply to comment #19)
> > +static UINT sWM_FLASHBOUNCEMSG = 0;
> > 
> > statics default to 0, no need for = 0.
> 
> I'd prefer to leave that as is, there's no harm in being explicit in the
> initial value.

Historically, and still AFAIK, there is harm in a small way: explicit initializers, even 0, besides being needlessly verbose (explicitness here only helps readers who don't know C/C++ rules), puts the variable in initialized data instead of BSS ("Blank Starting Storage" or variations; not sure what the original acronym was). This takes up space in the executable.

/be
Whiteboard: [needs r=emaijala/josh]
Comment on attachment 382646 [details] [diff] [review]
wm_user bounce patch v.2

I have no objection to this though I'm inexperienced with win32. It would be great if we could get emaijala to review.

Is this something we should get Adobe to address? If so lets file a bug on that and get in touch with the right people so we can eventually remove any fix on our end ASAP.
Attachment #382646 - Flags: superreview?(joshmoz) → review+
Whiteboard: [needs r=emaijala/josh] → [needs r=emaijala]
Attachment #382646 - Flags: review?(emaijala) → review?(doug.turner)
Comment on attachment 382646 [details] [diff] [review]
wm_user bounce patch v.2

Dougt: is this one you could look at for the 1.9.0 branch?
Comment on attachment 382646 [details] [diff] [review]
wm_user bounce patch v.2

Looks good to me too.
Attachment #382646 - Flags: review?(doug.turner) → review+
Whiteboard: [needs r=emaijala]
Jim: Anything else this needs or is this ready for 1.9.0?
(In reply to comment #25)
> Jim: Anything else this needs or is this ready for 1.9.0?

http://hg.mozilla.org/mozilla-central/rev/96af54657102

Anybody with a working cvs/1.9.0 local branch care to land this for 3.0?
Status: NEW → RESOLVED
Closed: 15 years ago
Resolution: --- → FIXED
Jim, is this Windows only? Just to make we verify the correct platforms.
Target Milestone: --- → mozilla1.9.2a1
Version: 1.9.0 Branch → Trunk
(In reply to comment #27)
> Jim, is this Windows only? Just to make we verify the correct platforms.

Yep.
Comment on attachment 382646 [details] [diff] [review]
wm_user bounce patch v.2

Currently just wanted on 1.9.1, requesting approval.
Attachment #382646 - Flags: approval1.9.1?
(In reply to comment #22)
> Is this something we should get Adobe to address? If so lets file a bug on that
> and get in touch with the right people so we can eventually remove any fix on
> our end ASAP.

Jim - can you respond? I'd like to make sure a bug gets filed if it makes sense.
(In reply to comment #30)
> (In reply to comment #22)
> > Is this something we should get Adobe to address? If so lets file a bug on that
> > and get in touch with the right people so we can eventually remove any fix on
> > our end ASAP.
> 
> Jim - can you respond? I'd like to make sure a bug gets filed if it makes
> sense.

It's part of their implementation, they use user messages internally as some sort of primary timing / functionality pump mechanism. There's nothing wrong with doing this, it has more to do with these events getting fed through our event processing system.

The original fix landed about nine years ago. (See bug 132759)  Personally I've never seen flooding of our our event queue from these events but I'm on a fairly fast system. That might not be the case for systems that are resource starved. We might kick off a bug to investigate whether or not the original fix is needed anymore.
(In reply to comment #31)
> (In reply to comment #30)
> > (In reply to comment #22)
> > > Is this something we should get Adobe to address? If so lets file a bug on that
> > > and get in touch with the right people so we can eventually remove any fix on
> > > our end ASAP.
> > 
> > Jim - can you respond? I'd like to make sure a bug gets filed if it makes
> > sense.
> 
> It's part of their implementation, they use user messages internally as some
> sort of primary timing / functionality pump mechanism. There's nothing wrong
> with doing this, it has more to do with these events getting fed through our
> event processing system.
> 
> The original fix landed about nine years ago. (See bug 132759)  Personally I've
> never seen flooding of our our event queue from these events but I'm on a
> fairly fast system. That might not be the case for systems that are resource
> starved. We might kick off a bug to investigate whether or not the original fix
> is needed anymore.

Err, actually seven years ago - 9-2002.
(In reply to comment #26)
> Anybody with a working cvs/1.9.0 local branch care to land this for 3.0?

fwiw, this needs approval before landing on 1.9.0.
Attachment #382646 - Flags: approval1.9.0.12?
(In reply to comment #33)
> (In reply to comment #26)
> > Anybody with a working cvs/1.9.0 local branch care to land this for 3.0?
> 
> fwiw, this needs approval before landing on 1.9.0.

It's already marked as blocking1.9.0.12.
(In reply to comment #34)
> (In reply to comment #33)
> > (In reply to comment #26)
> > > Anybody with a working cvs/1.9.0 local branch care to land this for 3.0?
> > 
> > fwiw, this needs approval before landing on 1.9.0.
> 
> It's already marked as blocking1.9.0.12.

As stated at the top of the Firefox3.0 tinderbox page, "Bugs must have approval1.9.0.12+ from the 1.9.0-branch triage team". This has always been the case for stable branches.
Comment on attachment 382646 [details] [diff] [review]
wm_user bounce patch v.2

Approved for 1.9.0.12, a=dveditz for release-drivers

Please check in with an appropriately vague comment (like "better handling of Flash bounce message"?) because FF3.5 won't get this fix until the 3.5.1 update
Attachment #382646 - Flags: approval1.9.0.12? → approval1.9.0.12+
Renominating for blocking 1.9.1. Since we're fixing topcrashers in the RC, it might be worth taking this as well. We also plan to fix it in 1.9.0.12, which will be out shortly after 1.9.1 ships.

The topcrash part of this was "fixed" in bug 470487, but that wasn't the real fix since it's still a topcrash (#4 in Firefox 3.5 RC).
Flags: blocking1.9.1- → blocking1.9.1?
Obviously since you nominated it you've got a firm sense of potential for regression, right? Or a good set of tests we can run things through to make sure we don't regress it further?
Jim: Can you weigh in on the scope of the patch and risk in taking it this late in the 1.9.1 cycle? (I might want it, but if you tell me how risky it is, I might change my mind...)
Flags: blocking1.9.1? → blocking1.9.1-
Whiteboard: [3.5.1?]
Fixed on 1.9.0 branch

Checking in base/src/nsPluginNativeWindowWin.cpp;
/cvsroot/mozilla/modules/plugin/base/src/nsPluginNativeWindowWin.cpp,v  <--  nsPluginNativeWindowWin.cpp
new revision: 1.29; previous revision: 1.28

Is this needed on the 1.8 branch for SeaMonkey?
Flags: wanted1.8.1.x?
Keywords: fixed1.9.0.12
Flags: blocking1.9.1.1?
Whiteboard: [3.5.1?]
(In reply to comment #39)
> Jim: Can you weigh in on the scope of the patch and risk in taking it this late
> in the 1.9.1 cycle? (I might want it, but if you tell me how risky it is, I
> might change my mind...)

I really don't see much risk in this. My only concern are the added events running through our native event process which might cause some sort of performance issue. (It doesn't appear we've had any issues on trunk.)

The other question is whether or not this is a valid security risk. There was a bunch of "spray heap" type code in the script that came with the poc that I was never entirely sure would actually be effective. The crash was from a stack unwind in a plugin event procedure, how script running in the page could leverage that is not clear to me. Someone who understands js interaction in that case could probably shed better light on it.

The crash occurs in a corner case where we have a slow script warning in the flash plugin (user content related issue) with an unload of the page using script in the html. That to me seems like something authors could avoid. If there's no legitimate security issue here, then I would say skipping 1.9.1 would be ok. We could let it bake on trunk for a while and land it for the first 3.5 update.
Verified for 1.9.0.12 with Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.0.12pre) Gecko/2009063005 GranParadiso/3.0.12pre (.NET CLR 3.5.30729) using PoC. Verified crash in 1.9.0.11 first.
Just for the record:
0:000> !exploitable -v
HostMachine\HostUser
Executing Processor Architecture is x86
Debuggee is in User Mode
Debuggee is a live user mode debugging session on the local machine
Event Type: Exception
*** ERROR: Symbol file could not be found.  Defaulted to export symbols for C:\Program Files\Mozilla Firefox 3\xul.dll - 
Exception Faulting Address: 0x4d75c9e
First Chance Exception Type: STATUS_ACCESS_VIOLATION (0xC0000005)
Exception Sub-Type: Data Execution Protection (DEP) Violation

Exception Hash (Major/Minor): 0x35022a3c.0x354a2a76

Stack Trace:
<Unloaded_NPSWF32.dll>+0x135c9e
<Unloaded_NPSWF32.dll>+0xdcb51
xul!gfxSkipCharsIterator::GetSkippedOffset+0x9c1
xul!XRE_main+0x1122
Instruction Address: 0x4d75c9e

Description: Data Execution Prevention Violation
Short Description: DEPViolation
Exploitability Classification: EXPLOITABLE
Recommended Bug Title: Exploitable - Data Execution Prevention Violation starting at <Unloaded_NPSWF32.dll>+0x135c9e (Hash=0x35022a3c.0x354a2a76)

User mode DEP access violations are exploitable.
Whiteboard: [sg:critical?]
Fixed this in 1.9.0.12, so we should fix it in 1.9.1.1.
Flags: blocking1.9.1.1? → blocking1.9.1.1+
Attachment #382646 - Flags: approval1.9.1? → approval1.9.1.1?
Comment on attachment 382646 [details] [diff] [review]
wm_user bounce patch v.2

Approved for 1.9.1.1. a=ss for release-drivers.
Attachment #382646 - Flags: approval1.9.1.1? → approval1.9.1.1+
Jim: can you verify that this is fixed in latest-mozilla1.9.1 nightly or better yet the 3.5.1 release candidate: ftp://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/3.5.1-candidates/build1/
I've seen one crash with the candidate build 1 on XP. But I'm not able to reproduce. It happened directly after I checked the crash with 3.5.

bp-e153ed28-1d10-4166-a917-185312090716
(In reply to comment #47)
> Jim: can you verify that this is fixed in latest-mozilla1.9.1 nightly or better
> yet the 3.5.1 release candidate:
> ftp://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/3.5.1-candidates/build1/

Verified fixed using the 3.5.1 build you posted and the PoC posted here.
Status: RESOLVED → VERIFIED
Jim, do you have any response to my comment 48? As I said I was able to crash 3.5.1 on Windows once by testing the PoC.
(In reply to comment #50)
> Jim, do you have any response to my comment 48? As I said I was able to crash
> 3.5.1 on Windows once by testing the PoC.

Any ideas on how we can reproduce? Do you have a crash stack?

All I have to go on is my own testing and the verified 1.9.0.12 Al provided. I tested it again a few times this morning with no problems. QA should also try and verify as well.
I tested with Mozilla/5.0 (Windows; U; Windows NT 6.0; en-US; rv:1.9.1.1) Gecko/20090715 Firefox/3.5.1 (.NET CLR 3.5.30729)

I was not able to make it crash, on repeated attempts. After I open the index.html file in the PoC, the browser becomes unresponsive momentarily, and then an Adobe Flash Player dialog comes up saying: "A script in this movie is causing Adobe Flash Player 10 to run slowly..."

If I click Yes to stop the script I'm back to normal, and if I click No, it hangs momentarily and then the dialog comes up again. But no crash. 3.5 crashes consistently, on the other hand.
Whiteboard: [sg:critical?] → [sg:critical?] PoC removed at request of reporter
(In reply to comment #48)
> I've seen one crash with the candidate build 1 on XP. But I'm not able to
> reproduce. It happened directly after I checked the crash with 3.5.
> 
> bp-e153ed28-1d10-4166-a917-185312090716

Sorry I didn't see your crash link. Not much info in there to go on.

If qa would care to do additional testing please email me, I have a copy of the PoC saved locally.
Group: core-security
Blocks: 507216
Alias: CVE-2009-2467
Product: Core → Core Graveyard
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: