From 908c381018ba95ee1c13d11d35414db3248782d8 Mon Sep 17 00:00:00 2001 From: Christoph Wurst Date: Thu, 14 Mar 2024 15:05:41 +0100 Subject: [PATCH] fix(session): Replace remember-me tokens in transaction Signed-off-by: Christoph Wurst --- lib/private/AllConfig.php | 13 +++----- lib/private/Server.php | 1 + lib/private/User/Session.php | 64 +++++++++++++++++++++++++++--------- lib/public/IConfig.php | 3 +- 4 files changed, 56 insertions(+), 25 deletions(-) diff --git a/lib/private/AllConfig.php b/lib/private/AllConfig.php index ab4359c798f93..967da8417ab1e 100644 --- a/lib/private/AllConfig.php +++ b/lib/private/AllConfig.php @@ -345,19 +345,12 @@ public function getUserKeys($userId, $appName) { } } - /** - * Delete a user value - * - * @param string $userId the userId of the user that we want to store the value under - * @param string $appName the appName that we stored the value under - * @param string $key the key under which the value is being stored - */ - public function deleteUserValue($userId, $appName, $key) { + public function deleteUserValue($userId, $appName, $key): bool { // TODO - FIXME $this->fixDIInit(); $qb = $this->connection->getQueryBuilder(); - $qb->delete('preferences') + $rowsAffected = $qb->delete('preferences') ->where($qb->expr()->eq('userid', $qb->createNamedParameter($userId, IQueryBuilder::PARAM_STR))) ->andWhere($qb->expr()->eq('appid', $qb->createNamedParameter($appName, IQueryBuilder::PARAM_STR))) ->andWhere($qb->expr()->eq('configkey', $qb->createNamedParameter($key, IQueryBuilder::PARAM_STR))) @@ -366,6 +359,8 @@ public function deleteUserValue($userId, $appName, $key) { if (isset($this->userCache[$userId][$appName])) { unset($this->userCache[$userId][$appName][$key]); } + + return $rowsAffected > 0; } /** diff --git a/lib/private/Server.php b/lib/private/Server.php index 76c73383b2e37..830005b71c826 100644 --- a/lib/private/Server.php +++ b/lib/private/Server.php @@ -548,6 +548,7 @@ public function __construct($webRoot, \OC\Config $config) { $c->get(ISecureRandom::class), $c->getLockdownManager(), $c->get(LoggerInterface::class), + $c->get(IDBConnection::class), $c->get(IEventDispatcher::class) ); /** @deprecated 21.0.0 use BeforeUserCreatedEvent event with the IEventDispatcher instead */ diff --git a/lib/private/User/Session.php b/lib/private/User/Session.php index ab6ec9f38d30f..27cb3ea0eff5e 100644 --- a/lib/private/User/Session.php +++ b/lib/private/User/Session.php @@ -48,6 +48,7 @@ use OC_User; use OC_Util; use OCA\DAV\Connector\Sabre\Auth; +use OCP\AppFramework\Db\TTransactional; use OCP\AppFramework\Utility\ITimeFactory; use OCP\Authentication\Exceptions\ExpiredTokenException; use OCP\Authentication\Exceptions\InvalidTokenException; @@ -55,6 +56,7 @@ use OCP\EventDispatcher\IEventDispatcher; use OCP\Files\NotPermittedException; use OCP\IConfig; +use OCP\IDBConnection; use OCP\IRequest; use OCP\ISession; use OCP\IUser; @@ -67,6 +69,7 @@ use OCP\User\Events\UserFirstTimeLoggedInEvent; use OCP\Util; use Psr\Log\LoggerInterface; +use function in_array; /** * Class Session @@ -91,6 +94,9 @@ * @package OC\User */ class Session implements IUserSession, Emitter { + + use TTransactional; + /** @var Manager $manager */ private $manager; @@ -116,6 +122,7 @@ class Session implements IUserSession, Emitter { private $lockdownManager; private LoggerInterface $logger; + private IDBConnection $dbConnection; /** @var IEventDispatcher */ private $dispatcher; @@ -127,6 +134,7 @@ public function __construct(Manager $manager, ISecureRandom $random, ILockdownManager $lockdownManager, LoggerInterface $logger, + IDBConnection $dbConnection, IEventDispatcher $dispatcher ) { $this->manager = $manager; @@ -137,6 +145,7 @@ public function __construct(Manager $manager, $this->random = $random; $this->lockdownManager = $lockdownManager; $this->logger = $logger; + $this->dbConnection = $dbConnection; $this->dispatcher = $dispatcher; } @@ -905,24 +914,49 @@ public function loginWithCookie($uid, $currentToken, $oldSessionId) { return false; } - // get stored tokens - $tokens = $this->config->getUserKeys($uid, 'login_token'); - // test cookies token against stored tokens - if (!in_array($currentToken, $tokens, true)) { - $this->logger->info('Tried to log in but could not verify token', [ + /* + * Run token lookup and replacement in a transaction + * + * The READ COMMITTED isolation level causes the database to serialize + * the DELETE query, making it possible to detect if two concurrent + * processes try to replace the same login token. + * Replacing more than once doesn't work because the app token behind + * the session can only be replaced once. + */ + $newToken = $this->atomic(function () use ($uid, $currentToken): ?string { + // get stored tokens + $tokens = $this->config->getUserKeys($uid, 'login_token'); + // test cookies token against stored tokens + if (!in_array($currentToken, $tokens, true)) { + $this->logger->error('Tried to log in {uid} but could not find token {token} in database', [ + 'app' => 'core', + 'token' => $currentToken, + 'uid' => $uid, + 'user' => $uid, + ]); + return false; + } + // replace successfully used token with a new one + if (!$this->config->deleteUserValue($uid, 'login_token', $currentToken)) { + $this->logger->error('Tried to log in {uid} but ran into concurrent session revival', [ + 'app' => 'core', + 'token' => $currentToken, + 'uid' => $uid, + 'user' => $uid, + ]); + return null; + } + $newToken = $this->random->generate(32); + $this->config->setUserValue($uid, 'login_token', $newToken, (string)$this->timeFactory->getTime()); + $this->logger->debug('Remember-me token {token} for {uid} replaced by {newToken}', [ 'app' => 'core', + 'token' => $currentToken, + 'newToken' => $newToken, + 'uid' => $uid, 'user' => $uid, ]); - return false; - } - // replace successfully used token with a new one - $this->config->deleteUserValue($uid, 'login_token', $currentToken); - $newToken = $this->random->generate(32); - $this->config->setUserValue($uid, 'login_token', $newToken, (string)$this->timeFactory->getTime()); - $this->logger->debug('Remember-me token replaced', [ - 'app' => 'core', - 'user' => $uid, - ]); + return $newToken; + }, $this->dbConnection); try { $sessionId = $this->session->getId(); diff --git a/lib/public/IConfig.php b/lib/public/IConfig.php index 706e4776221df..025d2b2de73ea 100644 --- a/lib/public/IConfig.php +++ b/lib/public/IConfig.php @@ -241,9 +241,10 @@ public function getAllUserValues(string $userId): array; * @param string $userId the userId of the user that we want to store the value under * @param string $appName the appName that we stored the value under * @param string $key the key under which the value is being stored + * @return bool whether a value has been deleted * @since 8.0.0 */ - public function deleteUserValue($userId, $appName, $key); + public function deleteUserValue($userId, $appName, $key): bool; /** * Delete all user values