Skip to content

Commit

Permalink
Browse files Browse the repository at this point in the history
N°4289 Security hardening
  • Loading branch information
piRGoif committed Oct 21, 2021
1 parent a353317 commit 7757f1f
Show file tree
Hide file tree
Showing 3 changed files with 110 additions and 33 deletions.
99 changes: 67 additions & 32 deletions application/transaction.class.inc.php
Expand Up @@ -193,10 +193,32 @@ protected static function GetUserPrefix()
*/
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()
{
Expand All @@ -212,24 +234,36 @@ public static function GetNewTransactionId()
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)
{
Expand All @@ -243,52 +277,53 @@ public static function IsTransactionValid($id, $bRemoveTransaction = true)

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;
}

/**
Expand Down
17 changes: 16 additions & 1 deletion core/userrights.class.inc.php
Expand Up @@ -951,6 +951,21 @@ 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))
Expand Down Expand Up @@ -1211,7 +1226,7 @@ public static function ListProfiles($oUser = null)
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'];
}
Expand Down
27 changes: 27 additions & 0 deletions 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');
}
}

0 comments on commit 7757f1f

Please sign in to comment.