From 72eadbde6b4be81132f99076df564ac3314d5a56 Mon Sep 17 00:00:00 2001 From: Simon McVittie Date: Tue, 2 Feb 2021 10:25:40 +0000 Subject: [PATCH 1/3] gdbus: Improve readability by avoiding ternary operator Signed-off-by: Simon McVittie --- gio/gdbusaddress.c | 12 ++++++++++-- 1 file changed, 10 insertions(+), 2 deletions(-) diff --git a/gio/gdbusaddress.c b/gio/gdbusaddress.c index 4ec81efb2..f1795cdf4 100644 --- a/gio/gdbusaddress.c +++ b/gio/gdbusaddress.c @@ -1324,7 +1324,11 @@ g_dbus_address_get_for_bus_sync (GBusType bus_type, switch (bus_type) { case G_BUS_TYPE_SYSTEM: - ret = !is_setuid ? g_strdup (g_getenv ("DBUS_SYSTEM_BUS_ADDRESS")) : NULL; + if (is_setuid) + ret = NULL; + else + ret = g_strdup (g_getenv ("DBUS_SYSTEM_BUS_ADDRESS")); + if (ret == NULL) { ret = g_strdup ("unix:path=/var/run/dbus/system_bus_socket"); @@ -1332,7 +1336,11 @@ g_dbus_address_get_for_bus_sync (GBusType bus_type, break; case G_BUS_TYPE_SESSION: - ret = !is_setuid ? g_strdup (g_getenv ("DBUS_SESSION_BUS_ADDRESS")) : NULL; + if (is_setuid) + ret = NULL; + else + ret = g_strdup (g_getenv ("DBUS_SESSION_BUS_ADDRESS")); + if (ret == NULL) { ret = get_session_address_platform_specific (&local_error); From 375cbee175e6fec833045150019435c75cfb4930 Mon Sep 17 00:00:00 2001 From: Simon McVittie Date: Tue, 2 Feb 2021 20:38:41 +0000 Subject: [PATCH 2/3] gdbus: Rename a variable to be less misleading We're using "setuid" here as shorthand for any elevated privileges that should make us distrust the caller: setuid, setgid, filesystem capabilities, more obscure Linux things that set the AT_SECURE flag (such as certain AppArmor transitions), and their equivalents on other operating systems. This is fine if we do it consistently, but I'm about to add a check for whether we are *literally* setuid, which would be particularly confusing without a rename. Signed-off-by: Simon McVittie --- gio/gdbusaddress.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/gio/gdbusaddress.c b/gio/gdbusaddress.c index f1795cdf4..897d2f1c1 100644 --- a/gio/gdbusaddress.c +++ b/gio/gdbusaddress.c @@ -1280,7 +1280,7 @@ g_dbus_address_get_for_bus_sync (GBusType bus_type, GCancellable *cancellable, GError **error) { - gboolean is_setuid = GLIB_PRIVATE_CALL (g_check_setuid) (); + gboolean has_elevated_privileges = GLIB_PRIVATE_CALL (g_check_setuid) (); gchar *ret, *s = NULL; const gchar *starter_bus; GError *local_error; @@ -1324,7 +1324,7 @@ g_dbus_address_get_for_bus_sync (GBusType bus_type, switch (bus_type) { case G_BUS_TYPE_SYSTEM: - if (is_setuid) + if (has_elevated_privileges) ret = NULL; else ret = g_strdup (g_getenv ("DBUS_SYSTEM_BUS_ADDRESS")); @@ -1336,7 +1336,7 @@ g_dbus_address_get_for_bus_sync (GBusType bus_type, break; case G_BUS_TYPE_SESSION: - if (is_setuid) + if (has_elevated_privileges) ret = NULL; else ret = g_strdup (g_getenv ("DBUS_SESSION_BUS_ADDRESS")); From 3f11992875e426066881661e1be7db725f1a59d3 Mon Sep 17 00:00:00 2001 From: Simon McVittie Date: Tue, 2 Feb 2021 20:52:03 +0000 Subject: [PATCH 3/3] gdbus: Use DBUS_SESSION_BUS_ADDRESS if AT_SECURE but not setuid This is against my better judgement, but it's the least bad regression fix I can think of. If we don't do this, at least gnome-keyring-daemon (setcap) and msmtp (setgid) are known to regress. Resolves: https://gitlab.gnome.org/GNOME/glib/-/issues/2305 Bug-Debian: https://bugs.debian.org/cgi-bin/bugreport.cgi?bug=981420 Bug-Debian: https://bugs.debian.org/cgi-bin/bugreport.cgi?bug=981555 Signed-off-by: Simon McVittie --- gio/gdbusaddress.c | 26 ++++++++++++++++++++++++-- 1 file changed, 24 insertions(+), 2 deletions(-) diff --git a/gio/gdbusaddress.c b/gio/gdbusaddress.c index 897d2f1c1..0044cd3c6 100644 --- a/gio/gdbusaddress.c +++ b/gio/gdbusaddress.c @@ -1337,9 +1337,31 @@ g_dbus_address_get_for_bus_sync (GBusType bus_type, case G_BUS_TYPE_SESSION: if (has_elevated_privileges) - ret = NULL; + { +#ifdef G_OS_UNIX + if (geteuid () == getuid ()) + { + /* Ideally we shouldn't do this, because setgid and + * filesystem capabilities are also elevated privileges + * with which we should not be trusting environment variables + * from the caller. Unfortunately, there are programs with + * elevated privileges that rely on the session bus being + * available. We already prevent the really dangerous + * transports like autolaunch: and unixexec: when our + * privileges are elevated, so this can only make us connect + * to the wrong AF_UNIX or TCP socket. */ + ret = g_strdup (g_getenv ("DBUS_SESSION_BUS_ADDRESS")); + } + else +#endif + { + ret = NULL; + } + } else - ret = g_strdup (g_getenv ("DBUS_SESSION_BUS_ADDRESS")); + { + ret = g_strdup (g_getenv ("DBUS_SESSION_BUS_ADDRESS")); + } if (ret == NULL) {