Last Comment Bug 891292 - (CVE-2013-1723) NativeKey should not continue handling key message if widget is destroyed after dispatching event
(CVE-2013-1723)
: NativeKey should not continue handling key message if widget is destroyed aft...
Status: RESOLVED FIXED
[adv-main24+] possibly high/critical ...
: csectype-uaf, sec-moderate
Product: Core
Classification: Components
Component: Widget: Win32 (show other bugs)
: Trunk
: x86_64 Windows 8
: -- normal (vote)
: mozilla25
Assigned To: Masayuki Nakano [:masayuki] (Mozilla Japan)
:
: Jim Mathies [:jimm]
Mentors:
Depends on:
Blocks: 501496
  Show dependency treegraph
 
Reported: 2013-07-09 03:02 PDT by Masayuki Nakano [:masayuki] (Mozilla Japan)
Modified: 2014-11-19 20:11 PST (History)
7 users (show)
ryanvm: in‑testsuite-
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
wontfix
-
wontfix
+
fixed
+
fixed
-
wontfix
unaffected


Attachments
Don't dispatch key event and plugin event on destroyed widget (9.22 KB, patch)
2013-07-09 04:08 PDT, Masayuki Nakano [:masayuki] (Mozilla Japan)
jmathies: review+
bajaj.bhavana: approval‑mozilla‑aurora+
Details | Diff | Splinter Review

Description Masayuki Nakano [:masayuki] (Mozilla Japan) 2013-07-09 03:02:19 PDT
I realized that widget::NativeKey doesn't stop handling key message even if widget is destroyed by dispatching key event.

I'm not sure the risk of this bug, though.
Comment 1 Masayuki Nakano [:masayuki] (Mozilla Japan) 2013-07-09 04:04:01 PDT
The code was moved from nsWindow by bug 855975 (fixed on 24) but the bug isn't its regression.

So, if we need to fix this bug 23 and earlier too, we need to port the patch. I'm not sure if it's necessary.
Comment 2 Masayuki Nakano [:masayuki] (Mozilla Japan) 2013-07-09 04:08:13 PDT
Created attachment 772594 [details] [diff] [review]
Don't dispatch key event and plugin event on destroyed widget

I'm not sure how to write the commit message for security bug.
Comment 3 Masayuki Nakano [:masayuki] (Mozilla Japan) 2013-07-09 04:08:26 PDT
https://tbpl.mozilla.org/?tree=Try&usebuildbot=1&rev=25d9f4ff7869
Comment 4 Masayuki Nakano [:masayuki] (Mozilla Japan) 2013-07-09 06:37:04 PDT
Comment on attachment 772594 [details] [diff] [review]
Don't dispatch key event and plugin event on destroyed widget

https://tbpl.mozilla.org/?tree=Try&usebuildbot=1&rev=25d9f4ff7869

Event dispatchers should behave as "consumed" when the widget is destroyed by dispatched event listeners.

Additionally, let's crash if it tries to dispatch an event when the widget is already destroyed. Then, we can avoid to dispatch event on destroyed widget at least from widget::NativeKey.
Comment 5 Jim Mathies [:jimm] 2013-07-09 06:40:09 PDT
Comment on attachment 772594 [details] [diff] [review]
Don't dispatch key event and plugin event on destroyed widget

Review of attachment 772594 [details] [diff] [review]:
-----------------------------------------------------------------

::: widget/windows/KeyboardLayout.cpp
@@ +746,5 @@
>  NativeKey::DispatchKeyEvent(nsKeyEvent& aKeyEvent,
>                              const MSG* aMsgSentToPlugin) const
>  {
> +  if (mWidget->Destroyed()) {
> +    MOZ_CRASH("NativeKey tries to dispatch a key event on destroyed widget");

So this is a safety catch, we should never hit this?

@@ +1092,5 @@
>    } else {
>      WinUtils::GetMessage(&msg, mMsg.hwnd, aFirstMsg, aLastMsg);
>    }
> +  if (mWidget->Destroyed()) {
> +    MOZ_CRASH("NativeKey tries to dispatch a plugin event on destroyed widget");

ditto? Seems a little hard core to crash the app, unless our expectation is that this is totally unexpected. (in widget, nothing is totally unexpected in my experience!)
Comment 6 Masayuki Nakano [:masayuki] (Mozilla Japan) 2013-07-09 07:08:00 PDT
(In reply to Jim Mathies [:jimm] from comment #5)
> Comment on attachment 772594 [details] [diff] [review]
> Don't dispatch key event and plugin event on destroyed widget
> 
> Review of attachment 772594 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: widget/windows/KeyboardLayout.cpp
> @@ +746,5 @@
> >  NativeKey::DispatchKeyEvent(nsKeyEvent& aKeyEvent,
> >                              const MSG* aMsgSentToPlugin) const
> >  {
> > +  if (mWidget->Destroyed()) {
> > +    MOZ_CRASH("NativeKey tries to dispatch a key event on destroyed widget");
> 
> So this is a safety catch, we should never hit this?

Yes.

> 
> @@ +1092,5 @@
> >    } else {
> >      WinUtils::GetMessage(&msg, mMsg.hwnd, aFirstMsg, aLastMsg);
> >    }
> > +  if (mWidget->Destroyed()) {
> > +    MOZ_CRASH("NativeKey tries to dispatch a plugin event on destroyed widget");
> 
> ditto? Seems a little hard core to crash the app, unless our expectation is
> that this is totally unexpected. (in widget, nothing is totally unexpected
> in my experience!)

So, what code do you like? If we use MOZ_ASSERT() and just return in release build, perhaps, we will never get bug report for it...
Comment 7 Jim Mathies [:jimm] 2013-07-09 07:45:01 PDT
(In reply to Masayuki Nakano (:masayuki) (Mozilla Japan) from comment #6)
> (In reply to Jim Mathies [:jimm] from comment #5)
> > Comment on attachment 772594 [details] [diff] [review]
> > Don't dispatch key event and plugin event on destroyed widget
> > 
> > Review of attachment 772594 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > ::: widget/windows/KeyboardLayout.cpp
> > @@ +746,5 @@
> > >  NativeKey::DispatchKeyEvent(nsKeyEvent& aKeyEvent,
> > >                              const MSG* aMsgSentToPlugin) const
> > >  {
> > > +  if (mWidget->Destroyed()) {
> > > +    MOZ_CRASH("NativeKey tries to dispatch a key event on destroyed widget");
> > 
> > So this is a safety catch, we should never hit this?
> 
> Yes.
> 
> > 
> > @@ +1092,5 @@
> > >    } else {
> > >      WinUtils::GetMessage(&msg, mMsg.hwnd, aFirstMsg, aLastMsg);
> > >    }
> > > +  if (mWidget->Destroyed()) {
> > > +    MOZ_CRASH("NativeKey tries to dispatch a plugin event on destroyed widget");
> > 
> > ditto? Seems a little hard core to crash the app, unless our expectation is
> > that this is totally unexpected. (in widget, nothing is totally unexpected
> > in my experience!)
> 
> So, what code do you like? If we use MOZ_ASSERT() and just return in release
> build, perhaps, we will never get bug report for it...

This is fine, just wanted to be sure.
Comment 8 Masayuki Nakano [:masayuki] (Mozilla Japan) 2013-07-09 08:52:45 PDT
Thank you for your review.

By the way, do you think we need to port the patch for 23? The patch probably available with 24. However, due to the change of bug 501496, this patch isn't available with 23.
Comment 9 Masayuki Nakano [:masayuki] (Mozilla Japan) 2013-07-09 08:55:20 PDT
https://hg.mozilla.org/integration/mozilla-inbound/rev/40644f9ee83e
Comment 10 Al Billings [:abillings] 2013-07-10 10:18:32 PDT
What is the impact of this security issue? This is in term of a security rating.

This issue should have been rated and gone through the sec-approval process from https://wiki.mozilla.org/Security/Bug_Approval_Process. 

Can you please add sec-approval? to the patch and answer the questions in the template?
Comment 11 Ryan VanderMeulen [:RyanVM] 2013-07-10 11:32:20 PDT
https://hg.mozilla.org/mozilla-central/rev/40644f9ee83e

Is this something we could test?
Comment 12 Masayuki Nakano [:masayuki] (Mozilla Japan) 2013-07-10 20:16:31 PDT
Comment on attachment 772594 [details] [diff] [review]
Don't dispatch key event and plugin event on destroyed widget

# I've already landed the patch to m-i (and m-c), though, because I've not known the bug. I'm sorry.

[Security approval request comment]
> How easily could an exploit be constructed based on the patch?

If attacker knows the dispatching key events cause DOM key events, they can guess the security issue. 

> Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem?

I'm not sure "the patch paint bulls-eye". The removed or added comment may give hint for the attacker.

> Which older supported branches are affected by this flaw?

I'm not sure, the code was just moved from nsWindow at 24, so, 23 and earlier (probably at least 3) also has this bug.

> If not all supported branches, which bug introduced the flaw?

So, this is not a new regression.

> Do you have backports for the affected branches? If not, how different, hard to create, and risky will they be?

Patch for Firefox 23 or earlier (i.e., 17 ESR) is completely different because the code was moved by bug 855975.

> How likely is this patch to cause regressions; how much testing does it need?

If this patch has a bug, keyboard input handling may be broken, so, we need to test it with a lot of Nightly testers.

# I'm not sure if this bug allows attack actually. But similar bugs are reported on other platforms (bug 402505, bug 688208).
# I found a protector in nsWindow::DispatchEvent() now. http://mxr.mozilla.org/mozilla-central/source/widget/windows/nsWindow.cpp#3597 This may block most cases in this patch. However, RemoveMessageAndDispatchPluginEvent() and DispatchKeyPressEventsWithKeyboardLayout() ignore the status.
Comment 13 Masayuki Nakano [:masayuki] (Mozilla Japan) 2013-07-10 20:21:18 PDT
Perhaps, dead key input causes this security issue.

For example, If "`" key typed twice with Spanish keyboard layout, keypress event will be fired twice from DispatchKeyPressEventsWithKeyboardLayout(). If the first keypress causes closing the window, the second keypress dispatch is performed on the destroyed widget.

# I'm not sure if the event dispatching works on destroyed widget.
Comment 14 Masayuki Nakano [:masayuki] (Mozilla Japan) 2013-07-10 20:28:01 PDT
(In reply to Ryan VanderMeulen [:RyanVM UTC-4] from comment #11)
> https://hg.mozilla.org/mozilla-central/rev/40644f9ee83e
> 
> Is this something we could test?

There is no way to test it for now. nsIWidget::SynthesizeNativeKeyEvent() emulates dead key input incompletely.
Comment 15 Masayuki Nakano [:masayuki] (Mozilla Japan) 2013-07-10 20:50:42 PDT
Perhaps, there are two possibility of happening this issue.

1. When a windowless plugin (only Flash Player receives the plugin event) closes the window with native key event, keypress event may be fired on the destroyed widget.
2. When dead key causes inputting two characters and the keypress event for first keypress event closes the window, the second keypress event is fired on the destroyed widget.

I'm not sure if windowless Flash Player uses the plugin event (native event) for dispatching their internal keyboard event to the Flash application. So, I'm not sure if #1 is possible case.

#2 is the possible case. However, user needs to fail to input dead key since two characters need to be inputted by second native key event.
Comment 16 Masayuki Nakano [:masayuki] (Mozilla Japan) 2013-07-10 20:53:01 PDT
It might be good thing the nsWindow::DispatchEvent() checks mDestroyCalled first.
Comment 17 Al Billings [:abillings] 2013-07-11 13:32:49 PDT
Needinfo'ing Release Management and adding Dan Veditz since this went in without approval.

comment 12 points out that patching it in Firefox 23 and earlier will require a different patch. What do we want to do here?

Masayuki, please prepare and nominate a Firefox 24 (Aurora) patch for this soon. Does this affect Boot2Gecko?

Also, we need to figure out the risk rating for this issue before we can make a decision about Firefox 23 and ESR17.
Comment 18 Masayuki Nakano [:masayuki] (Mozilla Japan) 2013-07-11 17:48:23 PDT
(In reply to Al Billings [:abillings] from comment #17)
> Needinfo'ing Release Management and adding Dan Veditz since this went in
> without approval.

I should say again, I'm sorry.

> comment 12 points out that patching it in Firefox 23 and earlier will
> require a different patch. What do we want to do here?

If we just return from nsWindow::DispatchEvent() first on 23 (and ESR 17), the patch must be safe and easy to test.

> Masayuki, please prepare and nominate a Firefox 24 (Aurora) patch for this
> soon. Does this affect Boot2Gecko?

I'll do it. However, I'm on vacation until Monday, so, I'll do it early next week.

This is a bug only on Windows, so, it's safe on other platform including b2g.

> Also, we need to figure out the risk rating for this issue before we can
> make a decision about Firefox 23 and ESR17.

Yeah, I'm not sure if dispatching an event on destroyed widget actually has risk. The reason why I judged this as security issue is bug 402505 was a security bug.
Comment 19 Al Billings [:abillings] 2013-07-12 09:57:40 PDT
Ah, good to know this is windows only.

We'll get some feedback on this and move on. As far as it going in, it happens! We'll work it all out well enough.
Comment 20 Daniel Veditz [:dveditz] 2013-07-12 11:02:01 PDT
I wouldn't be too worried about back-porting before Aurora if the dead-key case were the only one... It would only affect people you could convince to type that kind of sequence and somehow you'd have to get the dead widget's memory reallocated in some useful way between the two key strokes. sec-moderate for that.

If plugin content can script native key events such that case #1 is possible then this is more worrying. You'd still have to deal with the narrow timing window, but I bet you could win that race often enough by allocating lots of stuff on another thread. Hm, not in Flash though, because that's now running in a separate process. A Worker then, possibly.

We haven't even demonstrated the ability to make this crash yet, so let's go with sec-moderate for now, and just land in Aurora.
Comment 21 Al Billings [:abillings] 2013-07-15 14:16:50 PDT
Clearing release management needinfo?. This is only a sec-moderate now.
Comment 22 Masayuki Nakano [:masayuki] (Mozilla Japan) 2013-07-16 02:43:36 PDT
Comment on attachment 772594 [details] [diff] [review]
Don't dispatch key event and plugin event on destroyed widget

[Approval Request Comment]
Bug caused by (feature/regressing bug #): not a regression
User impact if declined: attacker might be able to crash Gecko when the user type dead key, or typing on windowless flash player (not confirmed)
Testing completed (on m-c, etc.): already landed on m-c last week.
Risk to taking this patch (and alternatives if risky): If there are some bugs to try to dispatch key event after widget is destroyed, this patch causes (safe) crash.
String or IDL/UUID changes made by this patch: none

See also above comments.
Comment 23 Ryan VanderMeulen [:RyanVM] 2013-07-19 18:45:12 PDT
https://hg.mozilla.org/releases/mozilla-aurora/rev/fbba0c968440

Does this need to be land on Fx23 still? If not, please set status-firefox23 to wontfix.
Comment 24 Masayuki Nakano [:masayuki] (Mozilla Japan) 2013-07-22 23:43:04 PDT
This is not a recent regression. So, I believe that this is not so important. We shouldn't take new regression risk on 23.

Note You need to log in before you can comment on or make changes to this bug.