1824634 - UAF Crashes in [@ g_type_check_instance] from g_signal_emit from emit_closed_in_idle with libibus

Status: RESOLVED FIXED

(Core :: Widget: Gtk, defect)

Description

[Ryan VanderMeulen [:RyanVM]]

As noted in bug 1688592, we're still seeing UAF-looking crashes with this signature in 109+ and on ESR102.

Crash report: https://crash-stats.mozilla.org/report/index/dfd43b63-65cc-4d58-9c2c-9be200230323

Reason: SIGSEGV / SI_KERNEL

Top 10 frames of crashing thread:

0  libgobject-2.0.so.0  g_type_check_instance  gobject/gtype.c:4135
1  libgobject-2.0.so.0  g_signal_emit_valist  gobject/gsignal.c:3274
2  libgobject-2.0.so.0  g_signal_emit  gobject/gsignal.c:3551
3  libgobject-2.0.so.0  g_closure_invoke  gobject/gclosure.c:810
4  libgobject-2.0.so.0  signal_emit_unlocked_R.isra.0  gobject/gsignal.c:3739
5  libgobject-2.0.so.0  g_signal_emit_valist  gobject/gsignal.c:3495
6  libgobject-2.0.so.0  g_signal_emit  gobject/gsignal.c:3551
7  libibus-1.0.so.5  libibus-1.0.so.5@0x39a01  
8  libgobject-2.0.so.0  g_object_run_dispose  gobject/gobject.c:1226
9  libgobject-2.0.so.0  g_closure_invoke  gobject/gclosure.c:810

Comment 1

[Martin Stránský [:stransky] (ni? me)]

It looks similar to Bug 1811637. It comes from libibus so looks like we're getting callback from the library after free.
But I have no idea what's the libibus for or where it's used.

Comment 2

[Martin Stránský [:stransky] (ni? me)]

Looks like you need to use IBus as input module in Gtk:
https://wiki.archlinux.org/title/IBus

Comment 3

[Karl Tomlinson (:karlt)]

From the report in comment 0:

7 libibus-1.0.so.5 libibus-1.0.so.5@0x39a01

libibus-1.0.so.5 A04F4A0BA3DDDB1C07836C494F33C2A30 libibus-1.0.so.5

% cargo install dump_syms
% dpkg --extract libibus-1.0-5_1.5.23-2_amd64.deb .
% ~/.cargo/bin/dump_syms usr/lib/x86_64-linux-gnu/libibus-1.0.so.5 | grep MODULE
MODULE Linux x86_64 A04F4A0BA3DDDB1C07836C494F33C2A30 libibus-1.0.so.5

https://packages.debian.org/bullseye/libibus-1.0-5-dbgsym is not available.

8 libgobject-2.0.so.0 g_object_run_dispose gobject/gobject.c:1226

libgobject-2.0.so.0 E8FBE0D078B28035F652AD6C14F3EF210 libgobject-2.0.so.0

The filenames are no longer extracted from the symbolic links, so version information is lost.

https://packages.debian.org/bullseye/libglib2.0-0 indicates 2.66.8 and dump_syms confirms libglib2.0-0_2.66.8-1_amd64.deb, so
https://gitlab.gnome.org/GNOME/glib/-/blob/dde05fd4fcd1d96d1693723ae1d7f37ecd991215/gobject/gobject.c#L1226

9 libgobject-2.0.so.0 g_closure_invoke gobject/gclosure.c:810

https://gitlab.gnome.org/GNOME/glib/-/blob/dde05fd4fcd1d96d1693723ae1d7f37ecd991215/gobject/gclosure.c#L810

13 libgio-2.0.so.0 emit_closed_in_idle gio/gdbusconnection.c:1379

https://gitlab.gnome.org/GNOME/glib/-/blob/dde05fd4fcd1d96d1693723ae1d7f37ecd991215/gio/gdbusconnection.c#L1379

https://docs.gtk.org/gio/signal.DBusConnection.closed.html says

The cause of this event can be

• If g_dbus_connection_close() is called. In this case remote_peer_vanished is set to FALSE and error is NULL.
• If the remote peer closes the connection. In this case remote_peer_vanished is set to TRUE and error is set.
• If the remote peer sends invalid or malformed data. In this case remote_peer_vanished is set to FALSE and error is set.

Looking at ibus-1.5.23/src/ibusbus.c, there are some calls to g_dbus_connection_close(), but I don't see any way for these to be triggered from the ibus GtkIMContext (ibus-1.5.23/client/gtk3/), so I doubt this could be triggered by content.

Comment 4

[Karl Tomlinson (:karlt)]

https://crash-stats.mozilla.org/report/index/b470d82e-34ad-4923-96db-5c6a60230405 is a report from Ubuntu 22.04.2 LTS on 2023-04-05

7 libibus-1.0.so.5 libibus-1.0.so.5@0x37ef1

libibus-1.0.so.5 957AE47D6695ACB403C1527113D0D01B0 libibus-1.0.so.5

libibus-1.0-5_1.5.26-4_amd64.deb is 89E4DAE3DD9CFE448F7E77AB5BA21E1F0

https://crash-stats.mozilla.org/report/index/5ab416d2-1fe3-42be-b7a8-80d010230504 is a report from Arch Linux 2023-04-25, but Arch have updated that package and I don't know how to find older versions.

Comment 5

[Karl Tomlinson (:karlt)]

Mike, do you know whether dbgsym packages for libibus are available for bullseye?

Comment 6

[Mike Hommey [:glandium]]

http://deb.debian.org/debian-debug/pool/main/i/ibus/libibus-1.0-5-dbgsym_1.5.23-2_amd64.deb

Comment 7

[Karl Tomlinson (:karlt)]

8 libgobject-2.0.so.0 g_object_run_dispose gobject/gobject.c:1226

ibus_proxy_connection_closed_cb() (for the "closed" DBusConnection signal) just calls ibus_proxy_destroy() on an IBusProxy.
If not IBUS_IN_DESTRUCTION, then ibus_proxy_destroy() would tail call g_object_run_dispose() on the proxy.

7 libibus-1.0.so.5 libibus-1.0.so.5@0x39a01

% addr2line -if -e usr/lib/debug/.build-id/0b/4a4fa0dda31cdb07836c494f33c2a30737d9c7.debug 0x39a01 
ibus_proxy_dispose
./src/ibusproxy.c:102

https://github.com/ibus/ibus/blob/1.5.23/src/ibusproxy.c#L102

6 libgobject-2.0.so.0 g_signal_emit gobject/gsignal.c:3551

If not IBUS_DESTROYED and not IBUS_IN_DESTRUCTION, then ibus_proxy_dispose() emits "destroy" on the proxy.

3 libgobject-2.0.so.0 g_closure_invoke gobject/gclosure.c:810
2 libgobject-2.0.so.0 g_signal_emit gobject/gsignal.c:3551

We are missing a frame where the "destroy" signal handler on a proxy is emitting another signal, presumably in a tail call.

A likely candidate is _ibus_context_destroy_cb(), which handles "destroy" on a IBusInputContext (which is an IBusProxy), unrefs the IBusInputContext and nulls the pointer from the IBusIMContext, clears pre-edit state on the IBusIMContext, and finally emits "preedit-changed" and "preedit-end" signals on the IBusIMContext.

_ibus_context_destroy_cb() would be consistent with the stack in this similar report, where IMContextWrapper::OnChangeCompositionNative() is called to handle "preedit-changed".

Signal emission keeps a reference to the IBusInputContext object, but no reference is held to the user_data IBusIMContext.

1 libgobject-2.0.so.0 g_signal_emit_valist gobject/gsignal.c:3274

https://gitlab.gnome.org/GNOME/glib/-/blob/dde05fd4fcd1d96d1693723ae1d7f37ecd991215/gobject/gsignal.c#L3274
g_type_check_instance() appears to have a bad pointer to the object on which this signal is emitted.

If "preedit-changed" handling resulted in the IBusIMContext being released, then nothing else keeps the IBusIMContext alive for the "preedit-end" signal.
The IBusIMContext is owned by a GtkIMMulticontext, which is owned by a IMContextWrapper and released when the nsWindow is Destroy()ed.

It seems plausible that IMContextWrapper's handling of "preedit-changed" might result in the nsWindow being destroyed.
IMContextWrapper expects that might happen. I guess key event dispatch could trigger the window close or even the parent process might spin a nested event loop for a dialog.

Comment 8

[Karl Tomlinson (:karlt)]

(In reply to Karl Tomlinson (:karlt) from comment #7)

7 libibus-1.0.so.5 libibus-1.0.so.5@0x39a01

% addr2line -if -e usr/lib/debug/.build-id/0b/4a4fa0dda31cdb07836c494f33c2a30737d9c7.debug 0x39a01 
ibus_proxy_dispose
./src/ibusproxy.c:102

bug 1688592 comment 0 even had that information from January 2021.

Comment 9

[Karl Tomlinson (:karlt)]

Attachment: Bug 1824634 disconnect from GtkIMContext OnDestroyWindow r?masayuki

which will allow a GtkIMContext to outlive the IMContextWrapper.

Comment 10

[Karl Tomlinson (:karlt)]

Attachment: Bug 1824634 hold GtkIMContext reference a little longer than preedit-changed r?masayuki

Depends on D182865

Comment 11

[Karl Tomlinson (:karlt)]

Comment on attachment 9342518 [details]
Bug 1824634 hold GtkIMContext reference a little longer than preedit-changed r?masayuki

Security Approval Request

• How easily could an exploit be constructed based on the patch?: I doubt this can be triggered by content. AFAICT STR would need to include closing the input method server (at a similar time to closing a window, which might be triggered by content). Perhaps this might be desktop session logout or change of input method.
• Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem?: Yes
• Which older supported branches are affected by this flaw?: all
• If not all supported branches, which bug introduced the flaw?: None
• Do you have backports for the affected branches?: No
• If not, how different, hard to create, and risky will they be?: Easy. The patches are small and code has not changed in recent years.
• How likely is this patch to cause regressions; how much testing does it need?: Unlikely. It holds an object alive for short time longer.
• Is Android affected?: No

Comment 12

[Tom Ritter [:tjr]]

Comment on attachment 9342517 [details]
Bug 1824634 disconnect from GtkIMContext OnDestroyWindow r?masayuki

Approved to land and uplift

Comment 13

[Karl Tomlinson (:karlt)]

Attachment: ESR102 part 2: hold GtkIMContext reference a little longer than preedit-changed

Comment 14

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/1d267baf7c8b
disconnect from GtkIMContext OnDestroyWindow r=masayuki
https://hg.mozilla.org/integration/autoland/rev/b16b51143c51
hold GtkIMContext reference a little longer than preedit-changed r=masayuki

Comment 15

[Karl Tomlinson (:karlt)]

Attachment: Bug 1824634 disconnect from GtkIMContext OnDestroyWindow r?masayuki

which will allow a GtkIMContext to outlive the IMContextWrapper.

Original Revision: https://phabricator.services.mozilla.com/D182865

Comment 16

[Karl Tomlinson (:karlt)]

Attachment: Bug 1824634 hold GtkIMContext reference a little longer than preedit-changed r?masayuki

Original Revision: https://phabricator.services.mozilla.com/D182866

Comment 17

[Phabricator Automation]

Uplift Approval Request

• User impact if declined: UaF
• Steps to reproduce for manual QE testing: unknown
• Fix verified in Nightly: no
• Is Android affected?: no
• Explanation of risk level: The changes hold an object alive for short time longer.
• Risk associated with taking this patch: Low
• String changes made/needed: None
• Needs manual QE test: no
• Code covered by automated testing: no

Comment 18

[Karl Tomlinson (:karlt)]

Attachment: Bug 1824634 disconnect from GtkIMContext OnDestroyWindow r?masayuki

which will allow a GtkIMContext to outlive the IMContextWrapper.

Original Revision: https://phabricator.services.mozilla.com/D182865

Comment 19

[Karl Tomlinson (:karlt)]

Attachment: Bug 1824634 hold GtkIMContext reference a little longer than preedit-changed r?masayuki

Original Revision: https://phabricator.services.mozilla.com/D182866

Comment 20

[Phabricator Automation]

Uplift Approval Request

• Fix verified in Nightly: no
• Steps to reproduce for manual QE testing: unknown
• User impact if declined: UaF
• Code covered by automated testing: no
• Needs manual QE test: no
• Risk associated with taking this patch: low
• String changes made/needed: none
• Explanation of risk level: The changes hold an object alive for short time longer.
• Is Android affected?: no

Comment 21

[Karl Tomlinson (:karlt)]

Comment on attachment 9342517 [details]
Bug 1824634 disconnect from GtkIMContext OnDestroyWindow r?masayuki

ESR Uplift Approval Request

• If this is not a sec:{high,crit} bug, please state case for ESR consideration: sec-high
• User impact if declined: UaF
• Fix Landed on Version: 117
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky): The changes hold an object alive for a short time longer.

Comment 22

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/1d267baf7c8b
https://hg.mozilla.org/mozilla-central/rev/b16b51143c51

Comment 23

[Dianna Smith [:diannaS]]

Comment on attachment 9343615 [details]
Bug 1824634 disconnect from GtkIMContext OnDestroyWindow r?masayuki

Approved for 116.0b5

Comment 24

[Dianna Smith [:diannaS]]

Comment on attachment 9343616 [details]
Bug 1824634 hold GtkIMContext reference a little longer than preedit-changed r?masayuki

Approved for 116.0b5

Comment 25

[Pulsebot]

https://hg.mozilla.org/releases/mozilla-beta/rev/2df126aac28c
https://hg.mozilla.org/releases/mozilla-beta/rev/7a01db8a8e8a

Comment 26

[Dianna Smith [:diannaS]]

Comment on attachment 9342517 [details]
Bug 1824634 disconnect from GtkIMContext OnDestroyWindow r?masayuki

Approved for 102.14esr

Comment 27

[Pulsebot]

https://hg.mozilla.org/releases/mozilla-esr102/rev/4ab5ee7f04c2
https://hg.mozilla.org/releases/mozilla-esr102/rev/0b6fc2a1f4d4

Comment 28

[Pulsebot]

https://hg.mozilla.org/releases/mozilla-esr115/rev/7ce9193147e6
https://hg.mozilla.org/releases/mozilla-esr115/rev/8fe4702175c3