Closed Bug 891292 (CVE-2013-1723) Opened 11 years ago Closed 11 years ago

NativeKey should not continue handling key message if widget is destroyed after dispatching event

Categories

(Core :: Widget: Win32, defect)

x86_64
Windows 8
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla25
Tracking Status
firefox22 --- wontfix
firefox23 - wontfix
firefox24 + fixed
firefox25 + fixed
firefox-esr17 - wontfix
b2g18 --- unaffected

People

(Reporter: masayuki, Assigned: masayuki)

References

Details

(Keywords: csectype-uaf, sec-moderate, Whiteboard: [adv-main24+] possibly high/critical if we're underestimating the ease of winning the race)

Attachments

(1 file)

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.
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.
I'm not sure how to write the commit message for security bug.
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.
Attachment #772594 - Flags: review?(jmathies)
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!)
(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...
(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.
Attachment #772594 - Flags: review?(jmathies) → review+
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.
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?
https://hg.mozilla.org/mozilla-central/rev/40644f9ee83e

Is this something we could test?
Status: ASSIGNED → RESOLVED
Closed: 11 years ago
Flags: in-testsuite?
Resolution: --- → FIXED
Target Milestone: --- → mozilla25
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.
Attachment #772594 - Flags: sec-approval?
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.
(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.
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.
It might be good thing the nsWindow::DispatchEvent() checks mDestroyCalled first.
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.
Flags: needinfo?(release-mgmt)
(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.
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.
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.
Whiteboard: possibly high/critical if we're underestimating the ease of winning the race
Clearing release management needinfo?. This is only a sec-moderate now.
Flags: needinfo?(release-mgmt)
Attachment #772594 - Flags: sec-approval?
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.
Attachment #772594 - Flags: approval-mozilla-aurora?
Attachment #772594 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
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.
Flags: in-testsuite? → in-testsuite-
This is not a recent regression. So, I believe that this is not so important. We shouldn't take new regression risk on 23.
Whiteboard: possibly high/critical if we're underestimating the ease of winning the race → [adv-main24+] possibly high/critical if we're underestimating the ease of winning the race
Alias: CVE-2013-1723
Group: core-security
You need to log in before you can comment on or make changes to this bug.