1225970 - [non-e10s][GTK 3.4] crash from spinning event loop during resize paint on Ubuntu12.04LTS

Status: VERIFIED FIXED

(Core :: Widget: Gtk, defect)

Description

[Karl Tomlinson (:karlt)]

+++ This bug was initially created as a clone of Bug #1225520 +++
Bug 1225520 has a long CC list from the time when bug 726483 was public.
This report will have a shorter CC list.

testcase and crash reported in bug 626963 comment 58 and Bug 726483:

1. load testcase attachment 596531 [details] of Bug 726483
2. resize window several times (Picking the corner of the window with the mouse, and move the mouse in a circular motion)
3. close window (with the window manager close button)


I can reproduce the crash on the bad build(Nightly 2012-Feb-12) of Bug 726483 comment#2 on ubuntu12.04LTS.
And also I can reproduce the crash on Beta43.0b4, Auror44.0a2 and Nightly45.0a1.

bp-7a00b537-adde-4d94-a7ad-5f4262151117  : Beta43.0b4
bp-cd49549e-7cc7-4436-b8da-1e4fa2151117  : Aurora44.0a2 with disabled e10s
bp-7806d909-9049-4f16-bfa0-c05aa2151117  : Nightly45.0a1 with disabled e10s

When enabled e10s, browser does not crash. so, this seems to be non-e10s only bug.

Comment 1

[Karl Tomlinson (:karlt)]

This bug is about a GTK widget (MozContainer with STR here) used in _gtk_widget_draw_internal() [1] after free during the "draw" signal emission.

The first UaF is g_return_val_if_fail (GTK_IS_WIDGET (widget), NULL) in
gtk_widget_get_style_context() which will produce a safe null-deref unless
inst->g_class->g_type is correct.  Bug 860254 therefore makes this harder to
exploit because an exploit needs to know the necessary value.

It does not affect GTK+ versions 3.6 and more recent because the code using
the widget after signal emission has been removed [2].

GObject holds a strong reference to the widget during signal emission but
_gtk_widget_draw_internal used the widget after emission.

gtk_container_propagate_draw() [3] calls _gtk_widget_draw_internal
without a strong reference to the widget.  The call happens when the widget
does not have its own window but draws the window of its parent widget.  While
the "draw" signal is emitted on the parent GtkWindow widget, there is a strong
reference to the parent widget, but gtk_bin_forall() calls into the child
widget (MozContainer) without holding a reference to that widget.  |forall|
implementations in general don't hold references.  |forall| implementations of
widgets with multiple children will fail to handle changes in the children
list, but fortunately the only case when Gecko receives draw signals on a
widget using the parent widget is when the parent widget has only one child.

gtk_widget_send_expose() also calls _gtk_widget_draw_internal without an
explicit strong reference to the widget.  In the stack below there is a strong
reference to the GtkWindow widget from the check-resize signal emission, but
gtk_widget_send_expose can also be triggered by gdk_window_update_idle() at
which point there is no strong reference.  This call _gtk_widget_draw_internal
happens on widgets which have their own window, including the GtkWindow widget
and sometimes the MozContainer widget.  There is a strong reference to the
GdkWindow in the expose event being processed, but the widget is obtained from
the GdkWindow without adding a strong reference.

[1] https://git.gnome.org/browse/gtk+/tree/gtk/gtkwidget.c?id=3.4.2#n5846
[2] https://git.gnome.org/browse/gtk+/commit/gtk/gtkwidget.c?id=04c5fdaca675e324ca3c5fab0c4fac4b579d0d41
[3] https://git.gnome.org/browse/gtk+/tree/gtk/gtkcontainer.c?id=3.4.2#n3342
[4] https://git.gnome.org/browse/gtk+/tree/gtk/gtkbin.c?id=3.4.2#n169

#0  _gtk_style_context_coalesce_animation_areas (context=0x0, widget=0x93c01228)
    at /build/buildd/gtk+3.0-3.4.2/./gtk/gtkstylecontext.c:3341
#1  0xf764a61c in _gtk_widget_draw_internal (clip_to_size=<optimized out>, cr=0xc08ac000, 
    widget=0x93c01228) at /build/buildd/gtk+3.0-3.4.2/./gtk/gtkwidget.c:5847
#2  _gtk_widget_draw_internal (widget=0x93c01228, cr=0xc08ac000, clip_to_size=1)
    at /build/buildd/gtk+3.0-3.4.2/./gtk/gtkwidget.c:5804
#3  0xf745883e in gtk_container_propagate_draw (container=0x93d5f420, child=0x93c01228, 
    cr=0xc08ac000) at /build/buildd/gtk+3.0-3.4.2/./gtk/gtkcontainer.c:3342
#4  0xf74588cc in gtk_container_draw_child (child=0x93c01228, client_data=0xffffaef8)
    at /build/buildd/gtk+3.0-3.4.2/./gtk/gtkcontainer.c:3189
#5  0xf74065df in gtk_bin_forall (container=0x93d5f420, include_internals=1, 
    callback=0xf74588a0 <gtk_container_draw_child>, callback_data=0xffffaef8)
    at /build/buildd/gtk+3.0-3.4.2/./gtk/gtkbin.c:170
#6  0xf7456cf7 in gtk_container_forall (container=0x93d5f420, 
    callback=0xf74588a0 <gtk_container_draw_child>, callback_data=0xffffaef8)
    at /build/buildd/gtk+3.0-3.4.2/./gtk/gtkcontainer.c:2014
#7  0xf7456fa9 in gtk_container_draw (widget=0x93d5f420, cr=0xc08ac000)
    at /build/buildd/gtk+3.0-3.4.2/./gtk/gtkcontainer.c:3206
#8  0xf7656fa2 in gtk_window_draw (widget=0x93d5f420, cr=0xc08ac000)
    at /build/buildd/gtk+3.0-3.4.2/./gtk/gtkwindow.c:7682
#9  0xf74f0351 in _gtk_marshal_BOOLEAN__BOXEDv (closure=0xf7a05730, return_value=0xffffb0d8, 
    instance=0x93d5f420, args=0xffffb1ac "", marshal_data=0xf7656f40 <gtk_window_draw>, 
    n_params=1, param_types=0xf7a6a1c8) at /build/buildd/gtk+3.0-3.4.2/./gtk/gtkmarshalers.c:130
#10 0xf7637690 in gtk_widget_draw_marshallerv (closure=0xf7a05730, return_value=0xffffb0d8, 
    instance=0x93d5f420, args=0xffffb1ac "", marshal_data=0xf7656f40 <gtk_window_draw>, 
    n_params=1, param_types=0xf7a6a1c8) at /build/buildd/gtk+3.0-3.4.2/./gtk/gtkwidget.c:850
#11 0xf6d9b623 in g_type_class_meta_marshalv () from /usr/lib32/libgobject-2.0.so.0
#12 0xf6d9cf6a in _g_closure_invoke_va () from /usr/lib32/libgobject-2.0.so.0
#13 0xf6db5d03 in g_signal_emit_valist () from /usr/lib32/libgobject-2.0.so.0
#14 0xf6db6872 in g_signal_emit () from /usr/lib32/libgobject-2.0.so.0
#15 0xf764a656 in _gtk_widget_draw_internal (clip_to_size=<optimized out>, cr=0xc08ac000, 
    widget=0x93d5f420) at /build/buildd/gtk+3.0-3.4.2/./gtk/gtkwidget.c:5828
#16 _gtk_widget_draw_internal (widget=0x93d5f420, cr=0xc08ac000, clip_to_size=1)
    at /build/buildd/gtk+3.0-3.4.2/./gtk/gtkwidget.c:5804
#17 0xf764a7f3 in gtk_widget_send_expose (widget=0x93d5f420, event=0xffffb304)
    at /build/buildd/gtk+3.0-3.4.2/./gtk/gtkwidget.c:6211
#18 0xf74f006a in gtk_main_do_event (event=0xffffb304)
    at /build/buildd/gtk+3.0-3.4.2/./gtk/gtkmain.c:1637
#19 0xf730c63c in _gdk_event_emit (event=0xffffb304)
    at /build/buildd/gtk+3.0-3.4.2/./gdk/gdkevents.c:69
#20 0xf73220d2 in _gdk_window_process_updates_recurse (window=0x93fcb270, expose_region=0x93cdcee0)
    at /build/buildd/gtk+3.0-3.4.2/./gdk/gdkwindow.c:3883
#21 0xf7340343 in gdk_x11_window_process_updates_recurse (window=0x93fcb270, region=0x93cdcee0)
    at /build/buildd/gtk+3.0-3.4.2/./gdk/x11/gdkwindow-x11.c:4850
#22 0xf7321512 in gdk_window_process_updates_internal (window=<optimized out>)
    at /build/buildd/gtk+3.0-3.4.2/./gdk/gdkwindow.c:4069
#23 0xf73217a5 in gdk_window_process_updates (window=0x93fcb270, update_children=1)
    at /build/buildd/gtk+3.0-3.4.2/./gdk/gdkwindow.c:4265
#24 0xf7656c5b in gtk_window_move_resize (window=0x93d5f420)
    at /build/buildd/gtk+3.0-3.4.2/./gtk/gtkwindow.c:7258
#25 gtk_window_check_resize (container=0x93d5f420)
    at /build/buildd/gtk+3.0-3.4.2/./gtk/gtkwindow.c:6302
#26 0xf3dfaf46 in wrap_gtk_window_check_resize(_GtkContainer*) ()
   from /mnt/sda10/karl/nightlies/2015-11-17/firefox/libxul.so
#27 0xf6d9ea39 in g_cclosure_marshal_VOID__VOIDv () from /usr/lib32/libgobject-2.0.so.0
#28 0xf6d9b623 in g_type_class_meta_marshalv () from /usr/lib32/libgobject-2.0.so.0
#29 0xf6d9cf6a in _g_closure_invoke_va () from /usr/lib32/libgobject-2.0.so.0
#30 0xf6db5d03 in g_signal_emit_valist () from /usr/lib32/libgobject-2.0.so.0
---Type <return> to continue, or q <return> to quit---
#31 0xf6db6872 in g_signal_emit () from /usr/lib32/libgobject-2.0.so.0
#32 0xf74566ea in gtk_container_check_resize (container=0x93d5f420)
    at /build/buildd/gtk+3.0-3.4.2/./gtk/gtkcontainer.c:1771
#33 0xf7456a0b in gtk_container_idle_sizer (data=0x0)
    at /build/buildd/gtk+3.0-3.4.2/./gtk/gtkcontainer.c:1661
#34 0xf73029de in gdk_threads_dispatch (data=0x8ba67650)
    at /build/buildd/gtk+3.0-3.4.2/./gdk/gdk.c:763
#35 0xf6c95d71 in g_idle_dispatch () from /usr/lib32/libglib-2.0.so.0
#36 0xf6c99479 in g_main_context_dispatch () from /usr/lib32/libglib-2.0.so.0
#37 0xf6c997f0 in g_main_context_iterate () from /usr/lib32/libglib-2.0.so.0
#38 0xf6c998b9 in g_main_context_iteration () from /usr/lib32/libglib-2.0.so.0
#39 0xf2688856 in nsAppShell::ProcessNextNativeEvent(bool) ()
   from /mnt/sda10/karl/nightlies/2015-11-17/firefox/libxul.so
#40 0xf26879e8 in nsBaseAppShell::OnProcessNextEvent(nsIThreadInternal*, bool) ()
   from /mnt/sda10/karl/nightlies/2015-11-17/firefox/libxul.so
#41 0xf25cf0b0 in nsThread::ProcessNextEvent(bool, bool*) ()
   from /mnt/sda10/karl/nightlies/2015-11-17/firefox/libxul.so
#42 0xf25d85e5 in NS_ProcessNextEvent(nsIThread*, bool) ()
   from /mnt/sda10/karl/nightlies/2015-11-17/firefox/libxul.so
#43 0xf25ef5f1 in mozilla::ipc::MessagePump::Run(base::MessagePump::Delegate*) ()
   from /mnt/sda10/karl/nightlies/2015-11-17/firefox/libxul.so
#44 0xf2b98bec in MessageLoop::Run() () from /mnt/sda10/karl/nightlies/2015-11-17/firefox/libxul.so
#45 0xf3dd7368 in nsBaseAppShell::Run() ()
   from /mnt/sda10/karl/nightlies/2015-11-17/firefox/libxul.so
#46 0xf442c6ca in nsAppStartup::Run() ()
   from /mnt/sda10/karl/nightlies/2015-11-17/firefox/libxul.so
#47 0xf4469a9f in XREMain::XRE_mainRun() ()
   from /mnt/sda10/karl/nightlies/2015-11-17/firefox/libxul.so
#48 0xf446c511 in XREMain::XRE_main(int, char**, nsXREAppData const*) ()
   from /mnt/sda10/karl/nightlies/2015-11-17/firefox/libxul.so
#49 0xf446c75c in XRE_main () from /mnt/sda10/karl/nightlies/2015-11-17/firefox/libxul.so
#50 0x08050f16 in do_main(int, char**, nsIFile*) ()
#51 0x0804c543 in main ()

Comment 2

[Karl Tomlinson (:karlt)]

Attachment: dispatch an event to release the widget after draw r?:roc

Comment 3

[Karl Tomlinson (:karlt)]

We could override the |forall| method of the GTK_TYPE_WINDOW class to address
the _gtk_widget_draw_internal call from gtk_container_propagate_draw, which
would resolve the testcase here, but that would not address the call from
gtk_widget_send_expose.

I think the best thing to do is take an extra reference to the widget and at the
end of handling the draw signal (after no more nested event loops can be run),
schedule an event to release the extra reference.  The event will not run
until after _gtk_widget_draw_internal completes.

Comment 4

[Karl Tomlinson (:karlt)]

Comment on attachment 8689317 [details] [diff] [review]
dispatch an event to release the widget after draw r?:roc

[Security approval request comment]
Note that STR were public until bug 726483 comment 9 and bug 1225520 comment 1.

> How easily could an exploit be constructed based on the patch?

Some understanding of Gecko's drawing and event dispatch model is required to
construct an exploit.  Exploiting requires unauthorized memory read through some other exploit.

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

The patches are not to the exact piece of code in interest, but it is close and not hard to find.

> Which older supported branches are affected by this flaw?

Branches using the GTK 3 port. 43 and more recent.

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

Bug 627699 and bug 1186229.

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

Trivial.

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

Unlikely.  Patch is simple.

Comment 5

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

sec-approval+. We should get this nominated for Aurora and Beta too.

Comment 6

[Karl Tomlinson (:karlt)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/4d0464f3e4bc

Comment 7

[Karl Tomlinson (:karlt)]

Comment on attachment 8689317 [details] [diff] [review]
dispatch an event to release the widget after draw r?:roc

Approval Request Comment
[Feature/regressing bug #]: GTK3 (bug 1186229)
[User impact if declined]: sec-high.
[Describe test coverage new/current, TreeHerder]:
manual test case in comment 0.
[Risks and why]: 
Low risk as patch simply extends the lifetime of a object already marked as
destroyed.
[String/UUID change made/needed]: None.

Comment 8

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/4d0464f3e4bc

Comment 9

[Liz Henry (:lizzard) (relman/hg->git project)]

Comment on attachment 8689317 [details] [diff] [review]
dispatch an event to release the widget after draw r?:roc

Fix for reproducible crash, part of gtk3 work. 
ok to uplift to aurora and beta.

Comment 10

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/dc1991dd52f1

Comment 11

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-beta/rev/e07497654dcc

Comment 12

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-b2g44_v2_5/rev/dc1991dd52f1

Comment 14

[Bogdan Maris, Desktop QA]

I managed to reproduce this crash on old Firefox 43 beta 4 (as expected):
bp-f209cad2-e5e7-4b5d-a47d-b6d482151127

Also reproduced on 43 beta 7 with a different signature (not expected):
bp-f235fb8e-a90f-4e85-a02d-809e02151127
bp-d7967b47-a3cb-4490-8517-1ab152151127

Unable to reproduce on latest Nightly 45.0a1 and latest Developer Edition 44.0a2.

Any ideas?

Comment 15

[Karl Tomlinson (:karlt)]

Thanks, Bogdan.

43 beta 7 was built against GTK2, while Nightly and Aurora versions were built against GTK3.  This bug report was for a GTK3 crash that was potentially a security bug and so I'm happy that Aurora no longer reproduces.  The crash with GTK2 is a safe NULL dereference, and we will be ceasing GTK2 builds before too long and so I'm not concerned about the crash with GTK2.

43 beta 6 (like 43 beta 4) was built against GTK3, and so should no longer crash.  If you can test that build, please, it would confirm.

Comment 16

[Bogdan Maris, Desktop QA]

(In reply to Karl Tomlinson (back Dec 21 ni?:karlt) from comment #15)
> Thanks, Bogdan.
> 
> 43 beta 7 was built against GTK2, while Nightly and Aurora versions were
> built against GTK3.  This bug report was for a GTK3 crash that was
> potentially a security bug and so I'm happy that Aurora no longer
> reproduces.  The crash with GTK2 is a safe NULL dereference, and we will be
> ceasing GTK2 builds before too long and so I'm not concerned about the crash
> with GTK2.
> 
> 43 beta 6 (like 43 beta 4) was built against GTK3, and so should no longer
> crash.  If you can test that build, please, it would confirm.

I tested on 12.04 32-bit using 43 beta 6 and I can confirm that the crash did not occur.