fix(oauth): wrap token rotation in a transaction, only rotate if the token hasn't been modified since we have read it

Signed-off-by: Julien Veyssier <julien-nc@posteo.net>

Author: julien-nc

apps/oauth2/lib/Controller/OauthApiController.php

@@ -24,6 +24,7 @@
 use OCP\Authentication\Exceptions\ExpiredTokenException;
 use OCP\Authentication\Exceptions\InvalidTokenException;
 use OCP\DB\Exception;
+use OCP\IDBConnection;
 use OCP\IRequest;
 use OCP\Security\Bruteforce\IThrottler;
 use OCP\Security\ICrypto;
@@ -47,6 +48,7 @@ public function __construct(
 		private LoggerInterface $logger,
 		private IThrottler $throttler,
 		private ITimeFactory $timeFactory,
+		private IDBConnection $db,
 	) {
 		parent::__construct($appName, $request);
 	}
@@ -177,27 +179,55 @@ public function getToken(
 
 		// Rotate the apptoken (so the old one becomes invalid basically)
 		$newToken = $this->secureRandom->generate(72, ISecureRandom::CHAR_ALPHANUMERIC);
+		$newCode = $this->secureRandom->generate(128, ISecureRandom::CHAR_ALPHANUMERIC);
+		$newEncryptedToken = $this->crypto->encrypt($newToken, $newCode);
+		$tokenRotated = false;
 
-		$appToken = $this->tokenProvider->rotate(
-			$appToken,
-			$decryptedToken,
-			$newToken
-		);
+		$this->db->beginTransaction();
+		try {
+			$appToken = $this->tokenProvider->rotate(
+				$appToken,
+				$decryptedToken,
+				$newToken
+			);
+			$tokenRotated = true;
+
+			// Expiration is in 1 hour again
+			$appToken->setExpires($this->time->getTime() + 3600);
+			$this->tokenProvider->updateToken($appToken);
+
+			$updatedRows = $this->accessTokenMapper->rotateToken(
+				$accessToken->getId(),
+				$code,
+				$newCode,
+				$newEncryptedToken,
+				$grant_type === 'authorization_code',
+			);
+
+			if ($updatedRows !== 1) {
+				$this->db->rollBack();
+				// tokenProvider->rotate() updates the auth token cache, so we have to clear the new token on rollback
+				$this->tokenProvider->invalidateToken($newToken);
+				$response = new JSONResponse([
+					'error' => 'invalid_request',
+				], Http::STATUS_BAD_REQUEST);
+				$response->throttle(['invalid_request' => 'token already redeemed']);
+				return $response;
+			}
 
-		// Expiration is in 1 hour again
-		$appToken->setExpires($this->time->getTime() + 3600);
-		$this->tokenProvider->updateToken($appToken);
+			$this->db->commit();
+		} catch (\Throwable $e) {
+			if ($this->db->inTransaction()) {
+				$this->db->rollBack();
+			}
 
-		// Generate a new refresh token and encrypt the new apptoken in the DB
-		$newCode = $this->secureRandom->generate(128, ISecureRandom::CHAR_ALPHANUMERIC);
-		$accessToken->setHashedCode(hash('sha512', $newCode));
-		$accessToken->setEncryptedToken($this->crypto->encrypt($newToken, $newCode));
-		// increase the number of delivered oauth token
-		// this helps with cleaning up DB access token when authorization code has expired
-		// and it never delivered any oauth token
-		$tokenCount = $accessToken->getTokenCount();
-		$accessToken->setTokenCount($tokenCount + 1);
-		$this->accessTokenMapper->update($accessToken);
+			if ($tokenRotated) {
+				// tokenProvider->rotate() updates the auth token cache, so we have to clear the new token on rollback
+				$this->tokenProvider->invalidateToken($newToken);
+			}
+
+			throw $e;
+		}
 
 		$this->throttler->resetDelay($this->request->getRemoteAddress(), 'login', ['user' => $appToken->getUID()]);
 

apps/oauth2/lib/Db/AccessTokenMapper.php

@@ -82,4 +82,31 @@ public function cleanupExpiredAuthorizationCode(): void {
 			->andWhere($qb->expr()->lt('code_created_at', $qb->createNamedParameter($maxTokenCreationTs, IQueryBuilder::PARAM_INT)));
 		$qb->executeStatement();
 	}
+
+	/**
+	 * Rotate an access token only if it still matches the caller's previously-read state.
+	 *
+	 * @param int $id
+	 * @param string $oldCode
+	 * @param string $newCode
+	 * @param string $encryptedToken
+	 * @param bool $expectAuthorizationCodeState Require the token to still be unused
+	 * @return int Number of updated rows
+	 */
+	public function rotateToken(int $id, string $oldCode, string $newCode, string $encryptedToken, bool $expectAuthorizationCodeState): int {
+		$qb = $this->db->getQueryBuilder();
+		$qb
+			->update($this->tableName)
+			->set('hashed_code', $qb->createNamedParameter(hash('sha512', $newCode)))
+			->set('encrypted_token', $qb->createNamedParameter($encryptedToken))
+			->set('token_count', $qb->createFunction('token_count + 1'))
+			->where($qb->expr()->eq('id', $qb->createNamedParameter($id, IQueryBuilder::PARAM_INT)))
+			->andWhere($qb->expr()->eq('hashed_code', $qb->createNamedParameter(hash('sha512', $oldCode))));
+
+		if ($expectAuthorizationCodeState) {
+			$qb->andWhere($qb->expr()->eq('token_count', $qb->createNamedParameter(0, IQueryBuilder::PARAM_INT)));
+		}
+
+		return $qb->executeStatement();
+	}
 }