From 1e2f360ade2546f671c9dbc81e6a3bc8f74b0b85 Mon Sep 17 00:00:00 2001 From: Benjamin Berg Date: Mon, 9 Nov 2020 14:59:33 +0100 Subject: [PATCH] device: Fix possible race condition when checking claimed state We already check the claimed state in advance during authorization. This makes sense, as we can avoid authorization if the API has been used incorrectly. However, as the mainloop is running and handling other request the claimed state might change at any point until the method handler is actually running. As such, check the claimed state again in each method. Doing so fixes the possible race condition. --- src/device.c | 52 ++++++++++++++++++++++++++++++++++++++++++---------- 1 file changed, 42 insertions(+), 10 deletions(-) diff --git a/src/device.c b/src/device.c index 089003c..1258fd3 100644 --- a/src/device.c +++ b/src/device.c @@ -699,8 +699,14 @@ static gboolean fprint_device_claim (FprintDBusDevice *dbus_dev, FprintDevice *rdev = FPRINT_DEVICE (dbus_dev); FprintDevicePrivate *priv = fprint_device_get_instance_private(rdev); g_autoptr(GMutexLocker) locked = NULL; + g_autoptr(GError) error = NULL; char *sender, *user; + if (!_fprint_device_check_claimed (rdev, invocation, STATE_UNCLAIMED, &error)) { + g_dbus_method_invocation_return_gerror (invocation, error); + return TRUE; + } + locked = g_mutex_locker_new (&priv->lock); g_assert_null (priv->session); @@ -756,9 +762,15 @@ static void dev_close_cb(FpDevice *dev, GAsyncResult *res, void *user_data) static gboolean fprint_device_release (FprintDBusDevice *dbus_dev, GDBusMethodInvocation *invocation) { + g_autoptr(GError) error = NULL; FprintDevice *rdev = FPRINT_DEVICE (dbus_dev); FprintDevicePrivate *priv = fprint_device_get_instance_private(rdev); + if (!_fprint_device_check_claimed (rdev, invocation, STATE_CLAIMED, &error)) { + g_dbus_method_invocation_return_gerror (invocation, error); + return TRUE; + } + if (priv->current_cancellable) { if (priv->current_action == ACTION_ENROLL) { g_warning("Enrollment was in progress, stopping it"); @@ -950,6 +962,11 @@ static gboolean fprint_device_verify_start (FprintDBusDevice *dbus_dev, g_autoptr(GError) error = NULL; guint finger_num = finger_name_to_num (finger_name); + if (!_fprint_device_check_claimed (rdev, invocation, STATE_CLAIMED, &error)) { + g_dbus_method_invocation_return_gerror (invocation, error); + return TRUE; + } + if (priv->current_action != ACTION_NONE) { if (priv->current_action == ACTION_ENROLL) { g_set_error(&error, FPRINT_ERROR, FPRINT_ERROR_ALREADY_IN_USE, @@ -1051,6 +1068,11 @@ static gboolean fprint_device_verify_stop (FprintDBusDevice *dbus_dev, FprintDevicePrivate *priv = fprint_device_get_instance_private(rdev); GError *error = NULL; + if (!_fprint_device_check_claimed (rdev, invocation, STATE_CLAIMED, &error)) { + g_dbus_method_invocation_return_gerror (invocation, error); + return TRUE; + } + if (priv->current_action == ACTION_NONE) { g_set_error(&error, FPRINT_ERROR, FPRINT_ERROR_NO_ACTION_IN_PROGRESS, "No verification in progress"); @@ -1260,6 +1282,11 @@ static gboolean fprint_device_enroll_start (FprintDBusDevice *dbus_dev, FprintDevicePrivate *priv = fprint_device_get_instance_private(rdev); int finger_num = finger_name_to_num (finger_name); + if (!_fprint_device_check_claimed (rdev, invocation, STATE_CLAIMED, &error)) { + g_dbus_method_invocation_return_gerror (invocation, error); + return TRUE; + } + if (finger_num == -1) { g_set_error(&error, FPRINT_ERROR, FPRINT_ERROR_INVALID_FINGERNAME, "Invalid finger name"); @@ -1304,7 +1331,12 @@ static gboolean fprint_device_enroll_stop (FprintDBusDevice *dbus_dev, { FprintDevice *rdev = FPRINT_DEVICE (dbus_dev); FprintDevicePrivate *priv = fprint_device_get_instance_private(rdev); - GError *error = NULL; + g_autoptr(GError) error = NULL; + + if (!_fprint_device_check_claimed (rdev, invocation, STATE_CLAIMED, &error)) { + g_dbus_method_invocation_return_gerror (invocation, error); + return TRUE; + } if (priv->current_action != ACTION_ENROLL) { if (priv->current_action == ACTION_NONE) { @@ -1319,7 +1351,6 @@ static gboolean fprint_device_enroll_stop (FprintDBusDevice *dbus_dev, } else g_assert_not_reached (); g_dbus_method_invocation_return_gerror (invocation, error); - g_error_free (error); return TRUE; } @@ -1514,6 +1545,12 @@ static gboolean fprint_device_delete_enrolled_fingers2 (FprintDBusDevice *dbus_d FprintDevice *rdev = FPRINT_DEVICE (dbus_dev); FprintDevicePrivate *priv = fprint_device_get_instance_private(rdev); g_autoptr(GMutexLocker) locked = NULL; + g_autoptr(GError) error = NULL; + + if (!_fprint_device_check_claimed (rdev, invocation, STATE_CLAIMED, &error)) { + g_dbus_method_invocation_return_gerror (invocation, error); + return TRUE; + } locked = g_mutex_locker_new (&priv->lock); @@ -1595,6 +1632,9 @@ action_authorization_handler (GDBusInterfaceSkeleton *interface, g_assert_not_reached (); } + /* This is just a quick check in order to avoid authentication if + * the user cannot make the call at this time anyway. + * The method handler itself is required to check again! */ if (!_fprint_device_check_claimed (rdev, invocation, required_state, &error)) { return handle_unauthorized_access (rdev, invocation, error); @@ -1613,14 +1653,6 @@ action_authorization_handler (GDBusInterfaceSkeleton *interface, return handle_unauthorized_access (rdev, invocation, error); } - /* By this time some other invocation might have beaten the one that - * arrived earlier, as an user might slower in authenticating, so - * we need to check again wheter the device is claimed */ - if (!_fprint_device_check_claimed (rdev, invocation, required_state, - &error)) { - return handle_unauthorized_access (rdev, invocation, error); - } - g_debug ("Authorization granted to %s for device %s!", fp_device_get_name (priv->dev), g_dbus_method_invocation_get_sender (invocation));