CVE-2021-41245: Cross-Site Request Forgery (CSRF) (CWE-352)
value: Combodo iTop is a web based IT Service Management tool. In versions prior to 2.7.6 and 3.0.0, CSRF tokens generated by `privUITransactionFile` aren't properly checked. Versions 2.7.6 and 3.0.0 contain a patch for this issue. As a workaround, use the session implementation by adding in the iTop config file.
CWE description: The web application does not, or can not, sufficiently verify whether a well-formed, valid, consistent request was intentionally provided by the user who submitted the request.
CVSS v3 base score: 8.1
Published: 2022-04-05T15:15Z
Repository: https://github.com/Combodo/iTop
Language: PHP
Files changed: application/transaction.class.inc.php, core/userrights.class.inc.php, test/application/privUITransactionFileTest.php
Commit date: 2021-10-21 12:43:03 +0200
Commit message: N°4289 Security hardening

Diff: diff --git a/application/transaction.class.inc.php b/application/transaction.class.inc.php
index b83230a61c..053c718915 100644
--- a/application/transaction.class.inc.php
+++ b/application/transaction.class.inc.php
@@ -191,14 +191,36 @@ class privUITransactionSession
  * Useful when using an in-memory storage for the session which do not
  * guarantee mutual exclusion for writing
  */
 class privUITransactionFile
 {
+	/**
+	 * @return int
+	 * @throws \SecurityException if no connected user
+	 *
+	 * @since 2.6.5 2.7.6 3.0.0 N°4289 method creation
+	 */
+	private static function GetCurrentUserId() {
+		$iCurrentUserId = UserRights::GetConnectedUserId();
+		if ('' === $iCurrentUserId) {
+			throw new SecurityException('Cannot creation transaction_id when no user logged');
+		}
+
+		return $iCurrentUserId;
+	}
+
 	/**
 	 * Create a new transaction id, store it in the session and return its id
+	 *
 	 * @param void
+	 *
 	 * @return int The identifier of the new transaction
+	 *
+	 * @throws \SecurityException
+	 * @throws \Exception
+	 *
+	 * @since 2.6.5 2.7.6 3.0.0 security hardening + throws SecurityException if no user logged
 	 */
 	public static function GetNewTransactionId()
 	{
 		if (!is_dir(APPROOT.'data/transactions'))
 		{
@@ -210,28 +232,40 @@ class privUITransactionFile
 			if (!@mkdir(APPROOT.'data/transactions'))
 			{
 				throw new Exception('Failed to create the directory "'.APPROOT.'data/transactions". Ajust the rights on the parent directory or let an administrator create the transactions directory and give the web sever enough rights to write into it.');
 			}
 		}
+
 		if (!is_writable(APPROOT.'data/transactions'))
 		{
 			throw new Exception('The directory "'.APPROOT.'data/transactions" must be writable to the application.');
 		}
+
+		$iCurrentUserId = static::GetCurrentUserId();
+
 		self::CleanupOldTransactions();
-		$id = basename(tempnam(APPROOT.'data/transactions', static::GetUserPrefix()));
-		self::Info('GetNewTransactionId: Created transaction: '.$id);
 
-		return (string)$id;
+		$sTransactionIdFullPath = tempnam(APPROOT.'data/transactions', static::GetUserPrefix());
+		file_put_contents($sTransactionIdFullPath, $iCurrentUserId, LOCK_EX);
+
+		$sTransactionIdFileName = basename($sTransactionIdFullPath);
+		self::Info('GetNewTransactionId: Created transaction: '.$sTransactionIdFileName);
+
+		return $sTransactionIdFileName;
 	}
 
 	/**
 	 * Check whether a transaction is valid or not and (optionally) remove the valid transaction from
 	 * the session so that another call to IsTransactionValid for the same transaction id
 	 * will return false
+	 *
 	 * @param int $id Identifier of the transaction, as returned by GetNewTransactionId
 	 * @param bool $bRemoveTransaction True if the transaction must be removed
+	 *
 	 * @return bool True if the transaction is valid, false otherwise
+	 *
+	 * @since 2.6.5 2.7.6 3.0.0 N°4289 security hardening
 	 */
 	public static function IsTransactionValid($id, $bRemoveTransaction = true)
 	{
 		// Constraint the transaction file within APPROOT.'data/transactions'
 		$sTransactionDir = realpath(APPROOT.'data/transactions');
@@ -241,56 +275,57 @@ class privUITransactionFile
 			return false;
 		}
 
 		clearstatcache(true, $sFilepath);
 		$bResult = file_exists($sFilepath);
-		if ($bResult)
+
+		if (false === $bResult) {
+			self::Info("IsTransactionValid: Transaction '$id' not found. Pending transactions:\n".implode("\n", self::GetPendingTransactions()));
+			return false;
+		}
+
+		$iCurrentUserId = static::GetCurrentUserId();
+		$sTransactionIdUserId = file_get_contents($sFilepath);
+		if ($iCurrentUserId != $sTransactionIdUserId) {
+			self::Info("IsTransactionValid: Transaction '$id' not existing for current user. Pending transactions:\n".implode("\n", self::GetPendingTransactions()));
+			return false;
+		}
+
+		if ($bRemoveTransaction)
 		{
-			if ($bRemoveTransaction)
+			$bResult = @unlink($sFilepath);
+			if (!$bResult)
 			{
-				$bResult = @unlink($sFilepath);
-				if (!$bResult)
-				{
-					self::Error('IsTransactionValid: FAILED to remove transaction '.$id);
-				}
-				else
-				{
-					self::Info('IsTransactionValid: OK. Removed transaction: '.$id);
-				}
+				self::Error('IsTransactionValid: FAILED to remove transaction '.$id);
+			}
+			else
+			{
+				self::Info('IsTransactionValid: OK. Removed transaction: '.$id);
 			}
 		}
-		else
-		{
-			self::Info("IsTransactionValid: Transaction '$id' not found. Pending transactions for this user:\n".implode("\n", self::GetPendingTransactions()));
-		}
+
 		return $bResult;
 	}
 
 	/**
 	 * Removes the transaction specified by its id
 	 * @param int $id The Identifier (as returned by GetNewTransactionId) of the transaction to be removed.
 	 * @return bool true if the token can be removed
+	 *
+	 * @since 2.6.5 2.7.6 3.0.0 N°4289 security hardening
 	 */
 	public static function RemoveTransaction($id)
 	{
-		$sFilepath = APPROOT.'data/transactions/'.$id;
-		clearstatcache(true, $sFilepath);
-		if (!file_exists($sFilepath)) {
-			$bSuccess = false;
-			self::Error("RemoveTransaction: Transaction '$id' not found. Pending transactions for this user:\n"
+		/** @noinspection PhpRedundantOptionalArgumentInspection */
+		$bResult = static::IsTransactionValid($id, true);
+		if (false === $bResult) {
+			self::Error("RemoveTransaction: Transaction '$id' is invalid. Pending transactions:\n"
 				.implode("\n", self::GetPendingTransactions()));
-		} else {
-			$bSuccess = @unlink($sFilepath);
-		}
-
-		if (!$bSuccess) {
-			self::Error('RemoveTransaction: FAILED to remove transaction '.$id);
-		} else {
-			self::Info('RemoveTransaction: OK '.$id);
+			return false;
 		}
 
-		return $bSuccess;
+		return true;
 	}
 
 	/**
 	 * Cleanup old transactions which have been pending since more than 24 hours
 	 * Use filemtime instead of filectime since filectime may be affected by operations on the directory (like changing the access rights)
diff --git a/core/userrights.class.inc.php b/core/userrights.class.inc.php
index 12a936787a..1819c0a9e9 100644
--- a/core/userrights.class.inc.php
+++ b/core/userrights.class.inc.php
@@ -949,10 +949,25 @@ class UserRights
 	public static function GetRealUserObject()
 	{
 		return self::$m_oRealUser;
 	}
 
+	/**
+	 * @return int|string ID of the connected user : if impersonate then use {@see m_oRealUser}, else {@see m_oUser}. If no user set then return ''
+	 * @since 2.6.5 2.7.6 3.0.0 N°4289 method creation
+	 */
+	public static function GetConnectedUserId() {
+		if (false === is_null(static::$m_oRealUser)) {
+			return static::$m_oRealUser->GetKey();
+		}
+		if (false === is_null(static::$m_oUser)) {
+			return static::$m_oUser->GetKey();
+		}
+
+		return '';
+	}
+
 	public static function GetRealUserId()
 	{
 		if (is_null(self::$m_oRealUser))
 		{
 			return '';
@@ -1209,11 +1224,11 @@ class UserRights
 			$aProfiles = array();
 		}
 		elseif ((self::$m_oUser !== null) && ($oUser->GetKey() == self::$m_oUser->GetKey()))
 		{
 			// Data about the current user can be found into the session data
-			if (array_key_exists('profile_list', $_SESSION))
+			if ((false === utils::IsModeCLI()) && array_key_exists('profile_list', $_SESSION))
 			{
 				$aProfiles = $_SESSION['profile_list'];
 			}
 		}
 
diff --git a/test/application/privUITransactionFileTest.php b/test/application/privUITransactionFileTest.php
new file mode 100644
index 0000000000..c3df443cc5
--- /dev/null
+++ b/test/application/privUITransactionFileTest.php
@@ -0,0 +1,27 @@
+<?php
+class privUITransactionFileTest extends \Combodo\iTop\Test\UnitTest\ItopDataTestCase
+{
+	const USER_TEST_LOGIN = 'support_test_privUITransaction';
+
+	public function testIsTransactionValid() {
+		$this->CreateUser(static::USER_TEST_LOGIN, 5); // profile:5 is "Support agent"
+
+		// create token in the support user context
+		UserRights::Login(self::USER_TEST_LOGIN);
+		$sTransactionIdUserSupport = privUITransactionFile::GetNewTransactionId();
+		$bResult = privUITransactionFile::IsTransactionValid($sTransactionIdUserSupport, false);
+		$this->assertTrue($bResult, 'Token created by support user must be valid in the support user context');
+
+		// test token in the admin user context
+		UserRights::Login('admin');
+		$bResult = privUITransactionFile::IsTransactionValid($sTransactionIdUserSupport, false);
+		$this->assertFalse($bResult, 'Token created by support user must be invalid in the admin user context');
+		$bResult = privUITransactionFile::RemoveTransaction($sTransactionIdUserSupport);
+		$this->assertFalse($bResult, 'Token created by support user cannot be removed in the admin user context');
+
+		// test other methods in the support user context
+		UserRights::Login(self::USER_TEST_LOGIN);
+		$bResult = privUITransactionFile::RemoveTransaction($sTransactionIdUserSupport);
+		$this->assertTrue($bResult, 'Token created by support user must be removed in the support user context');
+	}
+}
\ No newline at end of file

Vulnerable code: 		$id = basename(tempnam(APPROOT.'data/transactions', static::GetUserPrefix()));
		self::Info('GetNewTransactionId: Created transaction: '.$id);
		return (string)$id;
		if ($bResult)
			if ($bRemoveTransaction)
				$bResult = @unlink($sFilepath);
				if (!$bResult)
				{
					self::Error('IsTransactionValid: FAILED to remove transaction '.$id);
				}
				else
				{
					self::Info('IsTransactionValid: OK. Removed transaction: '.$id);
				}
		else
		{
			self::Info("IsTransactionValid: Transaction '$id' not found. Pending transactions for this user:\n".implode("\n", self::GetPendingTransactions()));
		}
		$sFilepath = APPROOT.'data/transactions/'.$id;
		clearstatcache(true, $sFilepath);
		if (!file_exists($sFilepath)) {
			$bSuccess = false;
			self::Error("RemoveTransaction: Transaction '$id' not found. Pending transactions for this user:\n"
		} else {
			$bSuccess = @unlink($sFilepath);
		}

		if (!$bSuccess) {
			self::Error('RemoveTransaction: FAILED to remove transaction '.$id);
		} else {
			self::Info('RemoveTransaction: OK '.$id);
		return $bSuccess;
			if (array_key_exists('profile_list', $_SESSION))
Fixed code: 	/**
	 * @return int
	 * @throws \SecurityException if no connected user
	 *
	 * @since 2.6.5 2.7.6 3.0.0 N°4289 method creation
	 */
	private static function GetCurrentUserId() {
		$iCurrentUserId = UserRights::GetConnectedUserId();
		if ('' === $iCurrentUserId) {
			throw new SecurityException('Cannot creation transaction_id when no user logged');
		}

		return $iCurrentUserId;
	}

	 *
	 *
	 *
	 * @throws \SecurityException
	 * @throws \Exception
	 *
	 * @since 2.6.5 2.7.6 3.0.0 security hardening + throws SecurityException if no user logged


		$iCurrentUserId = static::GetCurrentUserId();

		$sTransactionIdFullPath = tempnam(APPROOT.'data/transactions', static::GetUserPrefix());
		file_put_contents($sTransactionIdFullPath, $iCurrentUserId, LOCK_EX);

		$sTransactionIdFileName = basename($sTransactionIdFullPath);
		self::Info('GetNewTransactionId: Created transaction: '.$sTransactionIdFileName);

		return $sTransactionIdFileName;
	 *
	 *
	 *
	 * @since 2.6.5 2.7.6 3.0.0 N°4289 security hardening

		if (false === $bResult) {
			self::Info("IsTransactionValid: Transaction '$id' not found. Pending transactions:\n".implode("\n", self::GetPendingTransactions()));
			return false;
		}

		$iCurrentUserId = static::GetCurrentUserId();
		$sTransactionIdUserId = file_get_contents($sFilepath);
		if ($iCurrentUserId != $sTransactionIdUserId) {
			self::Info("IsTransactionValid: Transaction '$id' not existing for current user. Pending transactions:\n".implode("\n", self::GetPendingTransactions()));
			return false;
		}

		if ($bRemoveTransaction)
			$bResult = @unlink($sFilepath);
			if (!$bResult)
				self::Error('IsTransactionValid: FAILED to remove transaction '.$id);
			}
			else
			{
				self::Info('IsTransactionValid: OK. Removed transaction: '.$id);

	 *
	 * @since 2.6.5 2.7.6 3.0.0 N°4289 security hardening
		/** @noinspection PhpRedundantOptionalArgumentInspection */
		$bResult = static::IsTransactionValid($id, true);
		if (false === $bResult) {
			self::Error("RemoveTransaction: Transaction '$id' is invalid. Pending transactions:\n"
			return false;
		return true;
	/**
	 * @return int|string ID of the connected user : if impersonate then use {@see m_oRealUser}, else {@see m_oUser}. If no user set then return ''
	 * @since 2.6.5 2.7.6 3.0.0 N°4289 method creation
	 */
	public static function GetConnectedUserId() {
		if (false === is_null(static::$m_oRealUser)) {
			return static::$m_oRealUser->GetKey();
		}
		if (false === is_null(static::$m_oUser)) {
			return static::$m_oUser->GetKey();
		}

		return '';
	}

			if ((false === utils::IsModeCLI()) && array_key_exists('profile_list', $_SESSION))
<?php
class privUITransactionFileTest extends \Combodo\iTop\Test\UnitTest\ItopDataTestCase
{
	const USER_TEST_LOGIN = 'support_test_privUITransaction';

	public function testIsTransactionValid() {
		$this->CreateUser(static::USER_TEST_LOGIN, 5); // profile:5 is "Support agent"

		// create token in the support user context
		UserRights::Login(self::USER_TEST_LOGIN);
		$sTransactionIdUserSupport = privUITransactionFile::GetNewTransactionId();
		$bResult = privUITransactionFile::IsTransactionValid($sTransactionIdUserSupport, false);
		$this->assertTrue($bResult, 'Token created by support user must be valid in the support user context');

		// test token in the admin user context
		UserRights::Login('admin');
		$bResult = privUITransactionFile::IsTransactionValid($sTransactionIdUserSupport, false);
		$this->assertFalse($bResult, 'Token created by support user must be invalid in the admin user context');
		$bResult = privUITransactionFile::RemoveTransaction($sTransactionIdUserSupport);
		$this->assertFalse($bResult, 'Token created by support user cannot be removed in the admin user context');

		// test other methods in the support user context
		UserRights::Login(self::USER_TEST_LOGIN);
		$bResult = privUITransactionFile::RemoveTransaction($sTransactionIdUserSupport);
		$this->assertTrue($bResult, 'Token created by support user must be removed in the support user context');
	}
}