From 14e05505c60ec4ead2ee203064f8d679e36c1bf9 Mon Sep 17 00:00:00 2001 From: Tobias Mueller Date: Tue, 5 Jan 2021 08:38:15 +0100 Subject: usb: defensively destructure GVariants to prevent crashes The data is coming in from the outside, so it is untrusted. We check for the size of the variant, because otherwise we might crash at runtime, since g_variant_get_child might leave variables, i.e. GVariantIters, uninitialised (NULL) which then causes g_variant_iter_loop to crash, since it doesn't check for NULL. The check for g_variant_n_children is arguably ugly, because g_variant_get_child will eventually perform that check. So we focus on the returned NULL in case of g_variant_get_child_value or, in case of g_variant_get_child, whether the memory has not changed. The documentation is, for my taste, not as clear as it could or should be. It mentions that it is "an error" to provide an index higher than what the GVariant contains. But it doesn't mention how it communicates that error nor how to deal with it. --- plugins/usb-protection/gsd-usb-protection-manager.c | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/plugins/usb-protection/gsd-usb-protection-manager.c b/plugins/usb-protection/gsd-usb-protection-manager.c index 142760ac..accaa8e7 100644 --- a/plugins/usb-protection/gsd-usb-protection-manager.c +++ b/plugins/usb-protection/gsd-usb-protection-manager.c @@ -207,6 +207,7 @@ is_usbguard_allow_rule_present (GVariant *rules) g_debug ("Detecting rule..."); g_variant_get (rules, "a(us)", &iter); + g_return_val_if_fail (iter != NULL, FALSE); while (g_variant_iter_loop (iter, "(us)", &number, &value)) { if (g_strcmp0 (value, ALLOW_ALL) == 0) { g_debug ("Detected rule!"); @@ -237,6 +238,7 @@ usbguard_listrules_cb (GObject *source_object, rules = g_variant_get_child_value (result, 0); g_variant_unref (result); + g_return_if_fail (rules != NULL); if (!is_usbguard_allow_rule_present (rules)) add_usbguard_allow_rule (user_data); @@ -440,6 +442,7 @@ is_hid_or_hub (GVariant *device, } g_variant_get_child (device, PRESENCE_ATTRIBUTES, "a{ss}", &iter); + g_return_val_if_fail (iter != NULL, FALSE); while (g_variant_iter_loop (iter, "{ss}", &name, &value)) { if (g_strcmp0 (name, WITH_INTERFACE) == 0) { g_auto(GStrv) interfaces_splitted = NULL; @@ -466,6 +469,7 @@ is_hardwired (GVariant *device) g_autofree gchar *value = NULL; g_variant_get_child (device, PRESENCE_ATTRIBUTES, "a{ss}", &iter); + g_return_val_if_fail (iter != NULL, FALSE); while (g_variant_iter_loop (iter, "{ss}", &name, &value)) { if (g_strcmp0 (name, WITH_CONNECT_TYPE) == 0) { return g_strcmp0 (value, "hardwired") == 0; @@ -483,6 +487,7 @@ auth_device (GsdUsbProtectionManager *manager, if (manager->usb_protection_devices == NULL) return; + g_return_if_fail (g_variant_n_children (device) >= POLICY_DEVICE_ID); g_variant_get_child (device, POLICY_DEVICE_ID, "u", &device_id); g_debug ("Authorizing device %u", device_id); authorize_device(manager->usb_protection_devices, @@ -538,6 +543,7 @@ on_usbguard_signal (GDBusProxy *proxy, return; } + g_return_if_fail (g_variant_n_children (parameters) >= PRESENCE_EVENT); g_variant_get_child (parameters, PRESENCE_EVENT, "u", &device_event); if (device_event != EVENT_INSERT) { g_debug ("Device hat not been inserted (%d); ignoring", device_event); @@ -566,6 +572,7 @@ on_usbguard_signal (GDBusProxy *proxy, } g_variant_get_child (parameters, PRESENCE_ATTRIBUTES, "a{ss}", &iter); + g_return_if_fail (iter != NULL); while (g_variant_iter_loop (iter, "{ss}", &name, &device_name)) { if (g_strcmp0 (name, NAME) == 0) g_debug ("A new USB device has been connected: %s", device_name); @@ -658,6 +665,7 @@ on_usb_protection_signal (GDBusProxy *proxy, return; parameter = g_variant_get_child_value (parameters, 2); + g_return_if_fail (parameter != NULL); update_usb_protection_store (user_data, parameter); } -- cgit v1.2.1