1206564 - crash from spinning event loop during resize paint

Status: RESOLVED FIXED

(Core :: Widget: Gtk, defect)

Description

[Karl Tomlinson (:karlt)]

+++ This bug was initially created as a clone of Bug #726483 +++

1. load testcase reported in bug 626963 comment 58
2. resize window
3. close window (with the window manager close button)

This bug is about fixing the uaf reported in https://bugzilla.mozilla.org/show_bug.cgi?id=726483#c9

The fix here is not a complete solution to bug 726483.

Comment 1

[Karl Tomlinson (:karlt)]

Attachment: skip copying of listeners

Comment 2

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 8663497 [details] [diff] [review]
skip copying of listeners

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

It's kind of sad that nothing prevents a callback from deleting the nsIWidgetListener while it's on the call stack :-(. But since they're not refcounted I guess we can't fix that easily.

Comment 3

[Karl Tomlinson (:karlt)]

I was reproducing this on the 2015-09-09 nightly asan build, but it's not reproducing now, so I can't verify the fix or determine a regression range.

Comment 4

[Karl Tomlinson (:karlt)]

The code immediately involved here is old going back to Gecko 19
http://hg.mozilla.org/mozilla-central/rev/a0158d370785

It's possible that other changes have altered the order of events since then, but without better understanding, I'm assuming that all currently supported older revisions could be affected.

B2G does not use this widget code.

Comment 5

[Karl Tomlinson (:karlt)]

Comment on attachment 8663497 [details] [diff] [review]
skip copying of listeners

[Security approval request comment]
>How easily could an exploit be constructed based on the patch?
The exploitable crash is hard to reproduce.  Usually the STR produce a safe null deref.

A concern is that GTK3 builds (moving to beta about now I assume) don't generate the safe null deref.  With early GTK3 versions there is a different UaF tracked in bug 726483.  Later GTK3 versions are not affected by bug 726483, but may be affected by this bug.  Lee has tested GTK3 asan builds, which have not reproduced.

>Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem?
The name of the method changed hints at association with the "resize" event required in STR.

>Which older supported branches are affected by this flaw?
Assuming all supported branches with GTK ports.

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

Do you have backports for the affected branches? If not, how different, hard to create, and risky will they be?
The code has not changed for a long time so the patch should apply with offset to older branches.

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

It's a simple change, with relatively low risk.

Comment 6

[Al Billings [:abillings - ex-MoCo]]

Comment on attachment 8663497 [details] [diff] [review]
skip copying of listeners

sec-approval+. Let's get this in. We'll want it on affected branches as well so please create and/or nominate patches there once it is on trunk.

Comment 7

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/ddd169d6bd65

Comment 8

[Karl Tomlinson (:karlt)]

[Tracking Requested - why for this release]: sec-high

Comment 9

[Karl Tomlinson (:karlt)]

Comment on attachment 8663497 [details] [diff] [review]
skip copying of listeners

[Approval Request Comment]
Fix Landed on Version: 44

Approval Request Comment
[Feature/regressing bug #]: bug 793501 or later
[User impact if declined]: sec-high
[Describe test coverage new/current, TreeHerder]: none.
[Risks and why]: It's a simple change, with relatively low risk.
[String/UUID change made/needed]: none.

Comment 10

[Sylvestre Ledru [:Sylvestre]]

Comment on attachment 8663497 [details] [diff] [review]
skip copying of listeners

Fix a sec-high, taking it.

Comment 11

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-beta/rev/4d92cee6e6b9

Comment 12

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/663c99a20a9b

Comment 13

[Carsten Book [:Tomcat]]

 https://hg.mozilla.org/releases/mozilla-esr38/rev/5fc105753b57

Comment 14

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Updating tracking flag to indicate that this bug was fixed in esr38.4.0, see comment 13.

Comment 15

[Bogdan Maris, Desktop QA]

I can't seem to reproduce the initial crash using old Nightly debug asan build from 2015.09.20 and 2015.09.09 on Ubuntu 14.04 64-bit and 13.10 64-bit following steps from comment 0, so I can't verify if this is fixed. Is there any other way I could reproduce the initial issue?
Otherwise I will have to remove qe verify.

Comment 16

[Karl Tomlinson (:karlt)]

(In reply to Bogdan Maris, QA [:bogdan_maris] from comment #15)
> I can't seem to reproduce the initial crash using old Nightly debug asan
> build from 2015.09.20 and 2015.09.09 on Ubuntu 14.04 64-bit and 13.10 64-bit
> following steps from comment 0, so I can't verify if this is fixed. Is there
> any other way I could reproduce the initial issue?

That's consistent with comment 3.  The same builds that were reproducing are not reproducing for me now with the same gtk and glib system libraries.
I don't know of any way to verify.

Comment 17

[Bogdan Maris, Desktop QA]

(In reply to Karl Tomlinson (ni?:karlt) from comment #16)
> (In reply to Bogdan Maris, QA [:bogdan_maris] from comment #15)
> > I can't seem to reproduce the initial crash using old Nightly debug asan
> > build from 2015.09.20 and 2015.09.09 on Ubuntu 14.04 64-bit and 13.10 64-bit
> > following steps from comment 0, so I can't verify if this is fixed. Is there
> > any other way I could reproduce the initial issue?
> 
> That's consistent with comment 3.  The same builds that were reproducing are
> not reproducing for me now with the same gtk and glib system libraries.
> I don't know of any way to verify.

Thanks for your reply Karl. Unable to verify so dropping off our radar.