From 7a7423a05755802b21a13913053bf889446c2463 Mon Sep 17 00:00:00 2001
From: =?utf8?q?Tim=20D=C3=BCsterhus?= <duesterhus@woltlab.com>
Date: Wed, 13 Jan 2021 11:30:15 +0100
Subject: [PATCH] Do not take X locks in read-only methods of the MFA Setup
 class

This was a fun one. Apparently even a query locking exactly a single row by
leveraging an UNIQUE KEY outside of a transaction can deadlock with another
transaction.

Sending two requests to regenerate MFA backup codes at the same time caused the
following to happen within the database:

T1: Within a transaction locks the Setup by using Setup::lock(), putting an X
    onto the PRIMARY KEY.
T2: Outside of a transaction reads a Setup using Setup::find(), hitting exactly
    a single entry within the `userID` UNIQUE KEY (i.e. a specific
    `(userID, objectTypeID)` tuple).
    -> This puts an X onto the `userID` UNIQUE KEY.
    -> This wants to also put an X onto the corresponding PRIMARY KEY.
    -> The PRIMARY KEY is already locked by T1.
    -> This query (and thus this transaction) waits within this specific query
       for the lock to be granted.
T1: Within the transaction calls Setup::find() with the same parameters as T2.
    -> This needs to put the X onto the same row in the `userID` UNIQUE KEY.
    -> This row is locked by T2.
    -> This transaction needs to wait for that lock to be granted.

Now T1 needs to wait for T2 which already waits for T1 and we're experiencing a
deadlock.

Fix this issue by not taking an X within Setup::find() (i.e. removing the `FOR
UPDATE`). I've verified that nothing calls Setup::find() within a transaction
(without locking the Setup by a different means). Thus this change does not
result in a difference with regard to lock safety. Everything that needs to
modify a Setup already calls Setup::lock() which locks the PRIMARY KEY only.

Fixes #3874
---
 .../files/lib/system/user/multifactor/Setup.class.php       | 6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)

diff --git a/wcfsetup/install/files/lib/system/user/multifactor/Setup.class.php b/wcfsetup/install/files/lib/system/user/multifactor/Setup.class.php
index 1fd920740c..e15c42585e 100644
--- a/wcfsetup/install/files/lib/system/user/multifactor/Setup.class.php
+++ b/wcfsetup/install/files/lib/system/user/multifactor/Setup.class.php
@@ -115,8 +115,7 @@ final class Setup implements IIDObject {
 		$sql = "SELECT	*
 			FROM	wcf".WCF_N."_user_multifactor
 			WHERE	userID = ?
-				AND objectTypeID = ?
-			FOR UPDATE";
+				AND objectTypeID = ?";
 		$statement = WCF::getDB()->prepareStatement($sql);
 		$statement->execute([
 			$user->userID,
@@ -139,8 +138,7 @@ final class Setup implements IIDObject {
 	public static function getAllForUser(User $user): array {
 		$sql = "SELECT	*
 			FROM	wcf".WCF_N."_user_multifactor
-			WHERE	userID = ?
-			FOR UPDATE";
+			WHERE	userID = ?";
 		$statement = WCF::getDB()->prepareStatement($sql);
 		$statement->execute([$user->userID]);
 		
-- 
2.20.1