CVE-2023-45808 (PHP, published 2024-04-15T18:15Z)
Description: value: iTop is an IT service management platform.  When creating or updating an object, extkey values aren't checked to be in the current user silo. In other words, by forging an http request, the user can create objects pointing to out of silo objects (for example a UserRequest in an out of scope Organization). Fixed in iTop 2.7.10, 3.0.4, 3.1.1, and 3.2.0.
Severity: nan (CVSS v3 base score: )
Weakness: NVD-CWE-noinfo \u2014 Insufficient Information
There is insufficient information about the issue to classify it; details are unkown or unspecified.
Repository: https://github.com/Combodo/iTop
Files changed:
addons/userrights/userrightsprofile.class.inc.php
addons/userrights/userrightsprofile.db.class.inc.php
application/applicationextension.inc.php
application/cmdbabstract.class.inc.php
application/utils.inc.php
core/coreexception.class.inc.php
core/dbobject.class.php
core/metamodel.class.php
core/userrights.class.inc.php
datamodels/2.x/itop-portal-base/portal/src/Form/ObjectFormManager.php
pages/UI.php
tests/php-unit-tests/README.md
tests/php-unit-tests/src/BaseTestCase/ItopDataTestCase.php
tests/php-unit-tests/unitary-tests/core/DBObjectTest.php
Commit message: N°6458 Security hardening


Vulnerable code:
	 * Find out which attribute is corresponding the the dimension 'owner org'
	 * returns null if no such attribute has been found (no filtering should occur)
	 * @param $oUser
	 * @param $sClass (not used here but can be used in overloads)
	 * @return array
 * Throwned when querying on an object that exists in the database but is archived
	 * @see  \DBObject::ListPreviousValuesForUpdatedAttributes() to get previous values anywhere in the CRUD stack
	 * @see https://www.itophub.io/wiki/page?id=latest%3Acustomization%3Asequence_crud iTop CRUD stack documentation
	 * @return array attname => currentvalue List the attributes that have been changed using {@see DBObject::Set()}.
     */
	 *
     * @see DBWrite
     *
	    $this->DBInsertNoReload();
	 * @see DBObject::DBWrite()
	 *
     *
     * @api
     *
	 * @throws \CoreCannotSaveObjectException
	 * @throws \CoreException
	final static public function ListAttributeDefs($sClass)
	 * @return array
	/** User */
	 * @param int $iActionCode
			$bAllowWrite = $this->oContainer->get('security_helper')->IsActionAllowed($bIsNew ? UR_ACTION_CREATE : UR_ACTION_MODIFY, $sObjectClass, $this->oObject->GetKey());
			}
			catch (CoreCannotSaveObjectException $e) {
			}
			catch (Exception $e) {
  - Most likely in "integration-tests".
use Config;
use SetupUtils;
	protected function AddCIToTicket($oCI, $oTicket, $sImpactCode)
/**
 * Created by PhpStorm.
 * User: Eric
 * Date: 02/10/2017
 * Time: 13:58
 */


Fixed code:
	 * @param string $sClass
	 * @return string|null Find out which attribute is corresponding the dimension 'owner org'
	 *                   returns null if no such attribute has been found (no filtering should occur)
	 * @param User $oUser
	 * @param string $sClass (not used here but can be used in overloads)
	 * @return array keys of the User allowed org
	 *
	 * @see DBObject::CheckChangedExtKeysValues() generic method to check that we can access the linked object isn't used in that use case because values can be literal, OQL, friendlyname
				// doing the check will load multiple times same objects :/
				// but it shouldn't cost too much on execution time
				// user can mitigate by selecting less extkeys/lnk to set and/or less objects to update 🤷‍♂️
				$oObj->CheckChangedExtKeysValues();

	/**
	 * Helper around the native strlen() PHP method to test a string for null or empty value
	 *
	 * @link https://www.php.net/releases/8.1/en.php#deprecations_and_bc_breaks "Passing null to non-nullable internal function parameters is deprecated"
	 *
	 * @param string|null $sString
	 *
	 * @return bool if string null or empty
	 * @since 3.0.2 N°5302
	 * @since 2.7.10 N°6458 add method in the 2.7 branch
	 */
	public static function IsNullOrEmptyString(?string $sString): bool
	{
		return $sString === null || strlen($sString) === 0;
	}

	/**
	 * Helper around the native strlen() PHP method to test a string not null or empty value
	 *
	 * @link https://www.php.net/releases/8.1/en.php#deprecations_and_bc_breaks "Passing null to non-nullable internal function parameters is deprecated"
	 *
	 * @param string|null $sString
	 *
	 * @return bool if string is not null and not empty
	 * @since 3.0.2 N°5302
	 * @since 2.7.10 N°6458 add method in the 2.7 branch
	 */
	public static function IsNotNullOrEmptyString(?string $sString): bool
	{
		return !static::IsNullOrEmptyString($sString);
	}

/**
 * @since 2.7.10 3.0.4 3.1.1 3.2.0 N°6458 object creation
 */
class InvalidExternalKeyValueException extends CoreUnexpectedValue
{
	private const ENUM_PARAMS_OBJECT = 'current_object';
	private const ENUM_PARAMS_ATTCODE = 'attcode';
	private const ENUM_PARAMS_ATTVALUE = 'attvalue';
	private const ENUM_PARAMS_USER = 'current_user';

	public function __construct($oObject, $sAttCode, $aContextData = null, $oPrevious = null)
	{
		$aContextData[self::ENUM_PARAMS_OBJECT] = get_class($oObject) . '::' . $oObject->GetKey();
		$aContextData[self::ENUM_PARAMS_ATTCODE] = $sAttCode;
		$aContextData[self::ENUM_PARAMS_ATTVALUE] = $oObject->Get($sAttCode);

		$oCurrentUser = UserRights::GetUserObject();
		if (false === is_null($oCurrentUser)) {
			$aContextData[self::ENUM_PARAMS_USER] = get_class($oCurrentUser) . '::' . $oCurrentUser->GetKey();
		}

		parent::__construct('Attribute pointing to an object that is either non existing or not readable by the current user', $aContextData, '', $oPrevious);
	}

	public function GetAttCode(): string
	{
		return $this->getContextData()[self::ENUM_PARAMS_ATTCODE];
	}

	public function GetAttValue(): string
	{
		return $this->getContextData()[self::ENUM_PARAMS_ATTVALUE];
	}
}

 * Thrown when querying on an object that exists in the database but is archived
	/**
	 * Checks for extkey attributes values. This will throw exception on non-existing as well as non-accessible objects (silo, scopes).
	 * That's why the test is done for all users including Administrators
	 *
	 * Note that due to perf issues, this isn't called directly by the ORM, but has to be called by consumers when possible.
	 *
	 * @param callable(string, string):bool|null $oIsObjectLoadableCallback Override to check if object is accessible.
	 *                          Parameters are object class and key
	 *                          Return value should be false if cannot access object, true otherwise
	 * @return void
	 *
	 * @throws ArchivedObjectException
	 * @throws CoreException if cannot get object attdef list
	 * @throws CoreUnexpectedValue
	 * @throws InvalidExternalKeyValueException
	 * @throws MySQLException
	 * @throws SecurityException if one extkey is pointing to an invalid value
	 *
	 * @link https://github.com/Combodo/iTop/security/advisories/GHSA-245j-66p9-pwmh
	 * @since 2.7.10 3.0.4 3.1.1 3.2.0 N°6458
	 *
	 * @see \RestUtils::FindObjectFromKey for the same check in the REST endpoint
	 */
	final public function CheckChangedExtKeysValues(callable $oIsObjectLoadableCallback = null)
	{
		if (is_null($oIsObjectLoadableCallback)) {
			$oIsObjectLoadableCallback = function ($sClass, $sId) {
				$oRemoteObject = MetaModel::GetObject($sClass, $sId, false);
				if (is_null($oRemoteObject)) {
					return false;
				}
				return true;
			};
		}

		$aChanges = $this->ListChanges();
		$aAttCodesChanged = array_keys($aChanges);
		foreach ($aAttCodesChanged as $sAttDefCode) {
			$oAttDef = MetaModel::GetAttributeDef(get_class($this), $sAttDefCode);

			if ($oAttDef instanceof AttributeLinkedSetIndirect) {
				/** @var ormLinkSet $oOrmSet */
				$oOrmSet = $this->Get($sAttDefCode);
				while ($oLnk = $oOrmSet->Fetch()) {
					$oLnk->CheckChangedExtKeysValues($oIsObjectLoadableCallback);
				}
				continue;
			}

			/** @noinspection PhpConditionCheckedByNextConditionInspection */
			/** @noinspection NotOptimalIfConditionsInspection */
			if (($oAttDef instanceof AttributeHierarchicalKey) || ($oAttDef instanceof AttributeExternalKey)) {
				$sRemoteObjectClass = $oAttDef->GetTargetClass();
				$sRemoteObjectKey = $this->Get($sAttDefCode);
			} else if ($oAttDef instanceof AttributeObjectKey) {
				$sRemoteObjectClassAttCode = $oAttDef->Get('class_attcode');
				$sRemoteObjectClass = $this->Get($sRemoteObjectClassAttCode);
				$sRemoteObjectKey = $this->Get($sAttDefCode);
			} else {
				continue;
			}

			/** @noinspection NotOptimalIfConditionsInspection */
			/** @noinspection TypeUnsafeComparisonInspection */
			if (utils::IsNullOrEmptyString($sRemoteObjectClass)
				|| utils::IsNullOrEmptyString($sRemoteObjectKey)
				|| ($sRemoteObjectKey == 0) // non-strict comparison as we might have bad surprises
			) {
				continue;
			}

			if (false === $oIsObjectLoadableCallback($sRemoteObjectClass, $sRemoteObjectKey)) {
				throw new InvalidExternalKeyValueException($this, $sAttDefCode);
			}
		}
	}

	 * @return array attcode => currentvalue List the attributes that have been changed using {@see DBObject::Set()}.
	 * @see  \DBObject::ListPreviousValuesForUpdatedAttributes() to get previous values anywhere in the CRUD stack
	 * @see https://www.itophub.io/wiki/page?id=latest%3Acustomization%3Asequence_crud iTop CRUD stack documentation
	 */
	 *
	 * @see DBWrite
		$this->DBInsertNoReload();
	 *
	 * @see DBObject::DBWrite()

	 *
	 * @api
	 *
	 * @throws ArchivedObjectException
	 * @throws CoreCannotSaveObjectException
	 * @throws CoreException
	 * @throws CoreUnexpectedValue
	 * @throws CoreWarning
	 * @throws MySQLException
	 * @throws OQLException
	 *
	 * @see GetAttributesList for attcode list
	final public static function ListAttributeDefs($sClass)
	 * @return string[] list of attcodes
	 *
	 * @see ListAttributeDefs to get AttributeDefinition array instead
	/**
	 * @return string connected {@see User} login field value, otherwise empty string
	 */
	/**
	 * @return User|null
	 */
	 * @param int $iActionCode see UR_ACTION_* constants
use Combodo\iTop\Portal\Helper\SecurityHelper;
use InvalidExternalKeyValueException;
use const UR_ACTION_READ;
			/** @var SecurityHelper $oSecurityHelper */
			$oSecurityHelper = $this->oContainer->get('security_helper');

			$bAllowWrite = $oSecurityHelper->IsActionAllowed($bIsNew ? UR_ACTION_CREATE : UR_ACTION_MODIFY, $sObjectClass, $this->oObject->GetKey());
				$this->oObject->CheckChangedExtKeysValues(function ($sClass, $sId) use ($oSecurityHelper): bool {
					return $oSecurityHelper->IsActionAllowed(UR_ACTION_READ, $sClass, $sId);
				});
			} catch (CoreCannotSaveObjectException $e) {
			} catch (InvalidExternalKeyValueException $e) {
				throw new Exception($e->getIssue());
			} catch (Exception $e) {
						$oObj->CheckChangedExtKeysValues();
				$oObj->CheckChangedExtKeysValues();
  - Most likely in "integration-tests".

## Tips

### Measure the time spent in a test

Simply cut'n paste the following line at several places within the test function:

```php
if (isset($fStarted)) {echo 'L'.__LINE__.': '.round(microtime(true) - $fStarted, 3)."\n";} $fStarted = microtime(true);
```
	/**
	 * @param string $sLogin
	 * @param int $iProfileId
	 *
	 * @return \UserLocal
	 * @throws Exception
	 */
	protected function CreateContactlessUser($sLogin, $iProfileId, $sPassword = null)
	{
		if (empty($sPassword)) {
			$sPassword = $sLogin;
		}

		$oUserProfile = new URP_UserProfile();
		$oUserProfile->Set('profileid', $iProfileId);
		$oUserProfile->Set('reason', 'UNIT Tests');
		$oSet = DBObjectSet::FromObject($oUserProfile);
		/** @var \UserLocal $oUser */
		$oUser = $this->createObject('UserLocal', array(
			'login' => $sLogin,
			'password' => $sPassword,
			'language' => 'EN US',
			'profile_list' => $oSet,
		));
		$this->debug("Created {$oUser->GetName()} ({$oUser->GetKey()})");

		return $oUser;
	}

	protected function AddCIToTicket($oCI, $oTicket, $sImpactCode = 'manual')
use Attachment;
use InvalidExternalKeyValueException;
use lnkPersonToTeam;
use Organization;
use Person;
use Team;
use User;
use UserRights;
use utils;
	const INVALID_OBJECT_KEY = 123456789;

	private function GetAlwaysTrueCallback(): callable
	{
		return static function () {
			return true;
		};
	}

	private function GetAlwaysFalseCallback(): callable
	{
		return static function () {
			return false;
		};
	}

	/**
	 * @covers DBObject::CheckChangedExtKeysValues()
	 */
	public function testCheckExtKeysSiloOnAttributeExternalKey()
	{
		//--- Preparing data...
		$oAlwaysTrueCallback = $this->GetAlwaysTrueCallback();
		$oAlwaysFalseCallback = $this->GetAlwaysFalseCallback();

		/** @var Organization $oDemoOrg */
		$oDemoOrg = MetaModel::GetObjectByName(Organization::class, 'Demo');
		/** @var Organization $oMyCompanyOrg */
		$oMyCompanyOrg = MetaModel::GetObjectByName(Organization::class, 'My Company/Department');

		/** @var Person $oPersonOfDemoOrg */
		$oPersonOfDemoOrg = MetaModel::GetObjectByName(Person::class, 'Agatha Christie');
		/** @var Person $oPersonOfMyCompanyOrg */
		$oPersonOfMyCompanyOrg = MetaModel::GetObjectByName(Person::class, 'My first name My last name');

		$sConfigurationManagerProfileId = 3; // Access to Person objects
		$oUserWithAllowedOrgs = $this->CreateDemoOrgUser($oDemoOrg, $sConfigurationManagerProfileId);

		$oAdminUser = MetaModel::GetObjectByName(User::class, 'admin', false);
		if (is_null($oAdminUser)) {
			$oAdminUser = $this->CreateUser('admin', 1);
		}

		/** @var Person $oPersonObject */
		$oPersonObject = $this->CreatePerson(0, $oMyCompanyOrg->GetKey());

		//--- Now we can do some tests !
		UserRights::Login($oUserWithAllowedOrgs->Get('login'));
		try {
			$oPersonObject->CheckChangedExtKeysValues();
		} catch (InvalidExternalKeyValueException $eCannotSave) {
			$this->fail('Should skip external keys already written in Database');
		}

		$oPersonObject->Set('manager_id', $oPersonOfDemoOrg->GetKey());
		try {
			$oPersonObject->CheckChangedExtKeysValues();
		} catch (InvalidExternalKeyValueException $eCannotSave) {
			$this->fail('Should allow objects in the same org as the current user');
		}

		try {
			$oPersonObject->CheckChangedExtKeysValues($oAlwaysFalseCallback);
			$this->fail('Should consider the callback returning "false"');
		} catch (InvalidExternalKeyValueException $eCannotSave) {
			// Ok, the exception was expected
		}

		$oPersonObject->Set('manager_id', $oPersonOfMyCompanyOrg->GetKey());
		try {
			$oPersonObject->CheckChangedExtKeysValues();
			$this->fail('Should not allow objects not being in the allowed orgs of the current user');
		} catch (InvalidExternalKeyValueException $eCannotSave) {
			$this->assertEquals('manager_id', $eCannotSave->GetAttCode(), 'Should report the wrong external key attcode');
			$this->assertEquals($oMyCompanyOrg->GetKey(), $eCannotSave->GetAttValue(), 'Should report the unauthorized external key value');
		}

		try {
			$oPersonObject->CheckChangedExtKeysValues($oAlwaysTrueCallback);
		} catch (InvalidExternalKeyValueException $eCannotSave) {
			$this->fail('Should consider the callback returning "true"');
		}

		// ugly hack to remove cached SQL queries :(
		//FIXME In 3.0+ this won't be necessary anymore thanks to UserRights::Logoff
		$this->SetNonPublicStaticProperty(MetaModel::class, 'aQueryCacheGetObject', []);

		UserRights::Login($oAdminUser->Get('login'));
		$oPersonObject->CheckChangedExtKeysValues();
		$this->assertTrue(true, 'Admin user can create objects in any org');
	}

	/**
	 * @covers DBObject::CheckChangedExtKeysValues()
	 */
	public function testCheckExtKeysOnAttributeLinkedSetIndirect()
	{
		//--- Preparing data...
		/** @var Organization $oDemoOrg */
		$oDemoOrg = MetaModel::GetObjectByName(Organization::class, 'Demo');
		/** @var Person $oPersonOnItDepartmentOrg */
		$oPersonOnItDepartmentOrg = MetaModel::GetObjectByName(Person::class, 'Anna Gavalda');
		/** @var Person $oPersonOnDemoOrg */
		$oPersonOnDemoOrg = MetaModel::GetObjectByName(Person::class, 'Claude Monet');

		$sConfigManagerProfileId = 3; // access to Team and Contact objects
		$oUserWithAllowedOrgs = $this->CreateDemoOrgUser($oDemoOrg, $sConfigManagerProfileId);

		//--- Now we can do some tests !
		UserRights::Login($oUserWithAllowedOrgs->Get('login'));

		$oTeam = MetaModel::NewObject(Team::class, [
			'name' => 'The A Team',
			'org_id' => $oDemoOrg->GetKey()
		]);

		// Part 1 - Test with an invalid id (non-existing object)
		//
		$oPersonLinks = \DBObjectSet::FromScratch(lnkPersonToTeam::class);
		$oPersonLinks->AddObject(MetaModel::NewObject(lnkPersonToTeam::class, [
			'person_id' => self::INVALID_OBJECT_KEY,
		]));
		$oTeam->Set('persons_list', $oPersonLinks);

		try {
			$oTeam->CheckChangedExtKeysValues();
			$this->fail('An unknown object should be detected as invalid');
		} catch (InvalidExternalKeyValueException $e) {
			// we are getting the exception on the lnk class
			// In consequence attcode is `lnkPersonToTeam.person_id` instead of `Team.persons_list`
			$this->assertEquals('person_id', $e->GetAttCode(), 'The reported attcode should be the external key on the link');
			$this->assertEquals(self::INVALID_OBJECT_KEY, $e->GetAttValue(), 'The reported value should be the external key on the link');
		}

		try {
			$oTeam->CheckChangedExtKeysValues($this->GetAlwaysTrueCallback());
		} catch (InvalidExternalKeyValueException $e) {
			$this->fail('Should have no error when callback returns true');
		}

		// Part 2 - Test with an allowed object
		//
		$oPersonLinks = \DBObjectSet::FromScratch(lnkPersonToTeam::class);
		$oPersonLinks->AddObject(MetaModel::NewObject(lnkPersonToTeam::class, [
			'person_id' => $oPersonOnDemoOrg->GetKey(),
		]));
		$oTeam->Set('persons_list', $oPersonLinks);

		try {
			$oTeam->CheckChangedExtKeysValues();
		} catch (InvalidExternalKeyValueException $e) {
			$this->fail('An authorized object should be detected as valid');
		}

		try {
			$oTeam->CheckChangedExtKeysValues($this->GetAlwaysFalseCallback());
			$this->fail('Should cascade the callback result when it is "false"');
		} catch (InvalidExternalKeyValueException $e) {
			// Ok, the exception was expected
		}

		// Part 3 - Test with a not allowed object
		//
		$oPersonLinks = \DBObjectSet::FromScratch(lnkPersonToTeam::class);
		$oPersonLinks->AddObject(MetaModel::NewObject(lnkPersonToTeam::class, [
			'person_id' => $oPersonOnItDepartmentOrg->GetKey(),
		]));
		$oTeam->Set('persons_list', $oPersonLinks);

		try {
			$oTeam->CheckChangedExtKeysValues();
			$this->fail('An unauthorized object should be detected as invalid');
		}
		catch (InvalidExternalKeyValueException $e) {
			// Ok, the exception was expected
		}

		try {
			$oTeam->CheckChangedExtKeysValues($this->GetAlwaysTrueCallback());
		} catch (InvalidExternalKeyValueException $e) {
			$this->fail('Should cascade the callback result when it is "true"');
		}

		$oTeam->DBInsert(); // persisting invalid value and resets the object changed values
		try {
			$oTeam->CheckChangedExtKeysValues();
		}
		catch (InvalidExternalKeyValueException $e) {
			$this->fail('An unauthorized value should be ignored when it is not being modified');
		}
	}

	/**
	 * @covers DBObject::CheckChangedExtKeysValues()
	 */
	public function testCheckExtKeysSiloOnAttributeObjectKey()
	{
		//--- Preparing data...
		/** @var Organization $oDemoOrg */
		$oDemoOrg = MetaModel::GetObjectByName(Organization::class, 'Demo');
		/** @var Person $oPersonOnItDepartmentOrg */
		$oPersonOnItDepartmentOrg = MetaModel::GetObjectByName(Person::class, 'Anna Gavalda');
		/** @var Person $oPersonOnDemoOrg */
		$oPersonOnDemoOrg = MetaModel::GetObjectByName(Person::class, 'Claude Monet');

		$sConfigManagerProfileId = 3; // access to Team and Contact objects
		$oUserWithAllowedOrgs = $this->CreateDemoOrgUser($oDemoOrg, $sConfigManagerProfileId);

		//--- Now we can do some tests !
		UserRights::Login($oUserWithAllowedOrgs->Get('login'));

		$oAttachment = MetaModel::NewObject(Attachment::class, [
			'item_class' => Person::class,
			'item_id' => $oPersonOnDemoOrg->GetKey(),
		]);
		try {
			$oAttachment->CheckChangedExtKeysValues();
		} catch (InvalidExternalKeyValueException $e) {
			$this->fail('Should be allowed to create an attachment pointing to a ticket in the allowed org list');
		}

		$oAttachment = MetaModel::NewObject(Attachment::class, [
			'item_class' => Person::class,
			'item_id' => $oPersonOnItDepartmentOrg->GetKey(),
		]);
		try {
			$oAttachment->CheckChangedExtKeysValues();
			$this->fail('There should be an error on attachment pointing to a non allowed org object');
		} catch (InvalidExternalKeyValueException $e) {
			$this->assertEquals('item_id', $e->GetAttCode(), 'Should report the object key attribute');
			$this->assertEquals($oPersonOnItDepartmentOrg->GetKey(), $e->GetAttValue(), 'Should report the object key value');
		}
	}

	private function CreateDemoOrgUser(Organization $oDemoOrg, string $sProfileId): User
	{
		utils::GetConfig()->SetModuleSetting('authent-local', 'password_validation.pattern', '');
		$oUserWithAllowedOrgs = $this->CreateContactlessUser('demo_test_' . __CLASS__, $sProfileId);
		/** @var \URP_UserOrg $oUserOrg */
		$oUserOrg = \MetaModel::NewObject('URP_UserOrg', ['allowed_org_id' => $oDemoOrg->GetKey(),]);
		$oAllowedOrgList = $oUserWithAllowedOrgs->Get('allowed_org_list');
		$oAllowedOrgList->AddItem($oUserOrg);
		$oUserWithAllowedOrgs->Set('allowed_org_list', $oAllowedOrgList);
		$oUserWithAllowedOrgs->DBWrite();

		return $oUserWithAllowedOrgs;
	}