[PATCH BlueZ 0/1] mpris-proxy: Fix possible crash on OBEX transfer

[PATCH BlueZ 0/1] mpris-proxy: Fix possible crash on OBEX transfer

[Kai-Chuan Hsieh]

This fixes a mpris-proxy crash (NULL dereference) that triggers on every
OBEX transfer, most visibly when a remote peer pushes a file to the host
over Bluetooth OBEX Object Push.

find_player_by_obex() is called from obex_property_changed() on every
Status change of any org.bluez.obex.Transfer1 object. Since the OBEX
g_dbus_client_new() client is rooted at "/", mpris-proxy proxies all of
obexd's objects, including the server-side transfer objects obexd
registers for an incoming push. Those never belong to a player, so the
list walk dereferences a NULL player->obex / session->obex and crashes.
A second, narrower crash exists in obex_get_image() when a failed
CreateSession leaves the inner proxy NULL and a Track change carries an
ImgHandle.

The affected code path (BIP cover art / find_player_by_obex) was
introduced between 5.77 and 5.80 and is still present in 5.88/master.

This change consolidates two earlier fixes that were proposed but never
merged, because the project moved off GitHub pull requests:

  - PR #2098 (Bastien Nocera, "mpris-proxy: Fix possible crash"),
    https://github.com/bluez/bluez/pull/2098 -- passed CI, auto-closed.
  - issue #2569 / PR #2631, the additional inner-proxy (session->obex)
    guard for the failed-CreateSession case,
    https://github.com/bluez/bluez/issues/2569
    https://github.com/bluez/bluez/pull/2631

Resending here on linux-bluetooth so it can land for the next release.
The change is confined to tools/mpris-proxy.c and only adds NULL / early-
return guards before existing dereferences, plus a g_clear_pointer()
when releasing session->obex; it adds no new dereferences.

Downstream bug report (reproducer and SRU analysis):
https://bugs.launchpad.net/ubuntu/+source/bluez/+bug/2172906


Kai-Chuan Hsieh (1):
  mpris-proxy: Fix possible crash on OBEX transfer

 tools/mpris-proxy.c | 20 ++++++++++++++++----
 1 file changed, 16 insertions(+), 4 deletions(-)

-- 
2.43.0

[PATCH BlueZ 1/1] mpris-proxy: Fix possible crash on OBEX transfer

[Kai-Chuan Hsieh]

find_player_by_obex() and related helpers dereference player->obex and
its inner ->obex proxy without checking for NULL. A player legitimately
carries no OBEX session (player->obex == NULL) in three ordinary cases
and stays on the players list in each:

  - register_player(): the remote exposes no ObexPort (no AVRCP cover
    art);
  - device_property_changed(): the device disconnected;
  - obex_proxy_removed(): obexd went away.

find_player_by_obex() is reached from obex_property_changed() on every
Status change of any org.bluez.obex.Transfer1 object. Because
g_dbus_client_new() roots the OBEX client at "/", mpris-proxy proxies
all of obexd's objects, including the server-side transfers obexd
registers at /org/bluez/obex/server/sessionN/transferM for an incoming
push. Those paths never belong to a player; walking the list faults on
the first player whose session is NULL. So any peer pushing a file over
OBEX crashes mpris-proxy as soon as one registered player lacks a
cover-art session (the common configuration).

In addition, connect_obex_session() can leave session->obex NULL after a
failed CreateSession. In that state a Track change carrying an ImgHandle
reaches obex_get_image(), where g_dbus_proxy_get_path(NULL) returns NULL
and "path + strlen(...)" forms a garbage pointer passed to
g_strconcat(), crashing the process.

Guard all four player->obex / session->obex dereference sites, and use
g_clear_pointer() when releasing session->obex so a stale pointer cannot
be reused.

This folds together the fix originally proposed by Bastien Nocera in
https://github.com/bluez/bluez/pull/2098 and the additional inner-proxy
guard tracked in https://github.com/bluez/bluez/issues/2569 and
https://github.com/bluez/bluez/pull/2631, neither of which landed.

Signed-off-by: Kai-Chuan Hsieh <kaichuan.hsieh@canonical.com>
---
 tools/mpris-proxy.c | 20 ++++++++++++++++----
 1 file changed, 16 insertions(+), 4 deletions(-)

diff --git a/tools/mpris-proxy.c b/tools/mpris-proxy.c
index e06b9ea..8547fa9 100644
--- a/tools/mpris-proxy.c
+++ b/tools/mpris-proxy.c
@@ -1306,7 +1306,7 @@ static int parse_track_entry(DBusMessageIter *entry, const char *key,
 		const char *handle, *path;
 		char *filename, *uri;
 
-		if (!player || !player->obex)
+		if (!player || !player->obex || !player->obex->obex)
 			return -EINVAL;
 
 		path = g_dbus_proxy_get_path(player->obex->obex);
@@ -2614,9 +2614,14 @@ static void obex_get_image(struct player *player, const char *handle)
 	DBusMessage *msg;
 	DBusMessageIter iter, array;
 	struct obex_session *obex_session = player->obex;
-	const char *path = g_dbus_proxy_get_path(obex_session->obex);
+	const char *path;
 	char *filename;
 
+	if (!obex_session || !obex_session->obex)
+		return;
+
+	path = g_dbus_proxy_get_path(obex_session->obex);
+
 	player->filename = g_strconcat(g_get_tmp_dir(), "/",
 				path + strlen(BLUEZ_OBEX_CLIENT_PATH "/"),
 				"-", handle, NULL);
@@ -2674,7 +2679,7 @@ static void device_property_changed(GDBusProxy *proxy, const char *name,
 			player->obex = NULL;
 	}
 
-	g_dbus_proxy_unref(session->obex);
+	g_clear_pointer(&session->obex, g_dbus_proxy_unref);
 	g_dbus_proxy_unref(session->device);
 	obex_sessions = g_slist_remove(obex_sessions, session);
 	g_free(session);
@@ -2816,7 +2821,14 @@ static struct player *find_player_by_obex(const char *path)
 	for (l = players; l; l = l->next) {
 		struct player *player = l->data;
 		struct obex_session *session = player->obex;
-		const char *obex_path = g_dbus_proxy_get_path(session->obex);
+		const char *obex_path;
+
+		if (session == NULL || session->obex == NULL)
+			continue;
+
+		obex_path = g_dbus_proxy_get_path(session->obex);
+		if (obex_path == NULL)
+			continue;
 
 		if (g_str_has_prefix(path, obex_path))
 			return player;
-- 
2.43.0

RE: mpris-proxy: Fix possible crash on OBEX transfer

[bluez.test.bot]

[-- Attachment #1: Type: text/plain, Size: 857 bytes --]

This is automated email and please do not reply to this email!

Dear submitter,

Thank you for submitting the patches to the linux bluetooth mailing list.
This is a CI test results with your patch series:
PW Link:https://patchwork.kernel.org/series/1203891/

---Test result---

Test Summary:
CheckPatch                    PASS      0.40 seconds
GitLint                       PASS      0.26 seconds
BuildEll                      PASS      18.18 seconds
BluezMake                     PASS      273.85 seconds
CheckSmatch                   PASS      239.94 seconds
bluezmakeextell               PASS      89.60 seconds
TestFunctional                PASS      1071.05 seconds
IncrementalBuild              PASS      270.98 seconds
ScanBuild                     PASS      960.31 seconds



https://github.com/bluez/bluez/pull/2770

---
Regards,
Linux Bluetooth