From f5de808c7c6686e3d41017ca277a2fd82850845c Mon Sep 17 00:00:00 2001 From: jf-cbd <121934370+jf-cbd@users.noreply.github.com> Date: Fri, 13 Dec 2024 15:09:18 +0100 Subject: [PATCH 1/5] Security hardening (#685) * security hardening --- .../portal/src/Controller/ObjectController.php | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/datamodels/2.x/itop-portal-base/portal/src/Controller/ObjectController.php b/datamodels/2.x/itop-portal-base/portal/src/Controller/ObjectController.php index c9104cf9f..e6cb08a16 100644 --- a/datamodels/2.x/itop-portal-base/portal/src/Controller/ObjectController.php +++ b/datamodels/2.x/itop-portal-base/portal/src/Controller/ObjectController.php @@ -1246,7 +1246,8 @@ class ObjectController extends BrickController $bIgnoreSilos = $oScopeValidator->IsAllDataAllowedForScope(UserRights::ListProfiles(), $sObjectClass); $aParams = array('objects_id' => $aObjectIds); $oSearch = DBObjectSearch::FromOQL("SELECT $sObjectClass WHERE id IN (:objects_id)"); - if ($bIgnoreSilos === true) + $oScopeValidator->AddScopeToQuery($oSearch, $sObjectClass); + if ($bIgnoreSilos === true) { $oSearch->AllowAllData(); } From 95aa444ee683775f5fe22232591514ff39325864 Mon Sep 17 00:00:00 2001 From: jf-cbd Date: Fri, 13 Dec 2024 16:48:13 +0100 Subject: [PATCH 2/5] Security hardening --- .../portal/src/Controller/ObjectController.php | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/datamodels/2.x/itop-portal-base/portal/src/Controller/ObjectController.php b/datamodels/2.x/itop-portal-base/portal/src/Controller/ObjectController.php index e6cb08a16..c2366c6e5 100644 --- a/datamodels/2.x/itop-portal-base/portal/src/Controller/ObjectController.php +++ b/datamodels/2.x/itop-portal-base/portal/src/Controller/ObjectController.php @@ -1246,7 +1246,11 @@ class ObjectController extends BrickController $bIgnoreSilos = $oScopeValidator->IsAllDataAllowedForScope(UserRights::ListProfiles(), $sObjectClass); $aParams = array('objects_id' => $aObjectIds); $oSearch = DBObjectSearch::FromOQL("SELECT $sObjectClass WHERE id IN (:objects_id)"); - $oScopeValidator->AddScopeToQuery($oSearch, $sObjectClass); + if (!$oScopeValidator->AddScopeToQuery($oSearch, $sObjectClass) + ) { + IssueLog::Warning(__METHOD__ . ' at line ' . __LINE__ . ' : User #' . UserRights::GetUserId() . ' not allowed to read ' . $sObjectClass . ' object.'); + throw new HttpException(Response::HTTP_NOT_FOUND, Dict::S('UI:ObjectDoesNotExist')); + } if ($bIgnoreSilos === true) { $oSearch->AllowAllData(); From 1fa50f695dc6f085d7bb849967fafb561f363958 Mon Sep 17 00:00:00 2001 From: jf-cbd Date: Mon, 16 Dec 2024 10:47:06 +0100 Subject: [PATCH 3/5] Security hardening --- .../src/Controller/ObjectController.php | 36 ++++++++++++++++++- 1 file changed, 35 insertions(+), 1 deletion(-) diff --git a/datamodels/2.x/itop-portal-base/portal/src/Controller/ObjectController.php b/datamodels/2.x/itop-portal-base/portal/src/Controller/ObjectController.php index ee2c728d1..d180c84b1 100644 --- a/datamodels/2.x/itop-portal-base/portal/src/Controller/ObjectController.php +++ b/datamodels/2.x/itop-portal-base/portal/src/Controller/ObjectController.php @@ -1334,6 +1334,11 @@ class ObjectController extends BrickController $bIgnoreSilos = $oScopeValidator->IsAllDataAllowedForScope(UserRights::ListProfiles(), $sObjectClass); $aParams = array('objects_id' => $aObjectIds); $oSearch = DBObjectSearch::FromOQL("SELECT $sObjectClass WHERE id IN (:objects_id)"); + if (!$oScopeValidator->AddScopeToQuery($oSearch, $sObjectClass) + ) { + IssueLog::Warning(__METHOD__ . ' at line ' . __LINE__ . ' : User #' . UserRights::GetUserId() . ' not allowed to read ' . $sObjectClass . ' object.'); + throw new HttpException(Response::HTTP_NOT_FOUND, Dict::S('UI:ObjectDoesNotExist')); + } if ($bIgnoreSilos === true) { $oSearch->AllowAllData(); } @@ -1389,6 +1394,10 @@ class ObjectController extends BrickController $aObjectAttCodes = $oRequestManipulator->ReadParam('aObjectAttCodes', array(), FILTER_UNSAFE_RAW); $aLinkAttCodes = $oRequestManipulator->ReadParam('aLinkAttCodes', array(), FILTER_UNSAFE_RAW); $sDateTimePickerWidgetParent = $oRequestManipulator->ReadParam('sDateTimePickerWidgetParent', array(), FILTER_UNSAFE_RAW); + if (!MetaModel::IsLinkClass($sLinkClass)) { + IssueLog::Warning(__METHOD__.' at line '.__LINE__.' : User #'.UserRights::GetUserId().' asked for wrong lnk class '.$sLinkClass); + throw new HttpException(Response::HTTP_NOT_FOUND, Dict::S('UI:ObjectDoesNotExist')); + } if (empty($sObjectClass) || empty($aObjectIds) || empty($aObjectAttCodes)) { IssueLog::Info(__METHOD__.' at line '.__LINE__.' : sObjectClass, aObjectIds and aObjectAttCodes expected, "'.$sObjectClass.'", "'.implode('/', @@ -1423,10 +1432,35 @@ class ObjectController extends BrickController // Prepare link data $aObjectData = $this->PrepareObjectInformation($oObject, $aObjectAttCodes); // New link object (needed for renderers) - $oNewLink = new $sLinkClass(); + $aAttCodes = MetaModel::GetAttributesList($sLinkClass, ['AttributeExternalKey']); + $sAttCodeToObject = ''; + foreach ($aAttCodes as $sAttCode) { + $oAttDef = MetaModel::GetAttributeDef($sLinkClass, $sAttCode); + /** @var \AttributeExternalKey $oAttDef */ + if ($oAttDef->GetTargetClass() === $sObjectClass) { + $sAttCodeToObject = $sAttCode; + } + } + if ($sAttCodeToObject === '') { + IssueLog::Warning(__METHOD__.' at line '.__LINE__.' : User #'.UserRights::GetUserId().' asked for incoherent lnk class '.$sLinkClass.' with object class '.$sObjectClass); + throw new HttpException(Response::HTTP_NOT_FOUND, Dict::S('UI:ObjectDoesNotExist')); + } + $oNewLink = MetaModel::NewObject($sLinkClass, [ + $sAttCodeToObject => $oObject->GetKey(), // so later placeholders in filters will be applied on external keys on the same link + ]); foreach ($aLinkAttCodes as $sAttCode) { $oAttDef = MetaModel::GetAttributeDef($sLinkClass, $sAttCode); + /** @var \Combodo\iTop\Form\Field\SelectObjectField $oField */ $oField = $oAttDef->MakeFormField($oNewLink); + if ($oAttDef::GetFormFieldClass() === '\\Combodo\\iTop\\Form\\Field\\SelectObjectField') { + $oFieldSearch = $oField->GetSearch(); + $sFieldClass = $oFieldSearch->GetClass(); + if ($oScopeValidator->AddScopeToQuery($oFieldSearch, $sFieldClass)){ + $oField->SetSearch($oFieldSearch); + } else { + $oField->SetSearch(DBObjectSearch::FromOQL("SELECT $sFieldClass WHERE 1=0")); + } + } // Prevent datetimepicker popup to be truncated if ($oField instanceof DateTimeField) { $oField->SetDateTimePickerWidgetParent($sDateTimePickerWidgetParent); From 960129316d4897a3a10dc30c204a47e517764d63 Mon Sep 17 00:00:00 2001 From: Eric Espie Date: Mon, 16 Dec 2024 10:56:06 +0100 Subject: [PATCH 4/5] =?UTF-8?q?N=C2=B08050=20-=20:white=5Fcheck=5Fmark:=20?= =?UTF-8?q?Fix=20regression=20due=20to=20directories=20filter=20changes?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- .../Service/InterfaceDiscovery/InterfaceDiscoveryTest.php | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/tests/php-unit-tests/unitary-tests/sources/Service/InterfaceDiscovery/InterfaceDiscoveryTest.php b/tests/php-unit-tests/unitary-tests/sources/Service/InterfaceDiscovery/InterfaceDiscoveryTest.php index 6695dc4b3..5f3689072 100644 --- a/tests/php-unit-tests/unitary-tests/sources/Service/InterfaceDiscovery/InterfaceDiscoveryTest.php +++ b/tests/php-unit-tests/unitary-tests/sources/Service/InterfaceDiscovery/InterfaceDiscoveryTest.php @@ -87,7 +87,7 @@ class InterfaceDiscoveryTest extends ItopDataTestCase $this->assertGreaterThan(0, count($this->oInterfaceDiscovery->FindItopClasses(iUIBlockFactory::class))); $this->AssertDirectoryListingEquals([ 'autoload_classmaps.php', - '310db363d8e32bfcf57cbb3800912ea2_iUIBlockFactory.php' + '1ab1e62be3e9984a8176deeb20f049b1_iUIBlockFactory.php' ], $this->sCacheRootDir.'/InterfaceDiscovery'); } @@ -99,7 +99,7 @@ class InterfaceDiscoveryTest extends ItopDataTestCase MetaModel::GetConfig()->Set('developer_mode.enabled', false); $this->assertGreaterThan(0, count($this->oInterfaceDiscovery->FindItopClasses(iUIBlockFactory::class))); - $this->AssertDirectoryListingEquals(['310db363d8e32bfcf57cbb3800912ea2_iUIBlockFactory.php'], $this->sCacheRootDir.'/InterfaceDiscovery'); + $this->AssertDirectoryListingEquals(['1ab1e62be3e9984a8176deeb20f049b1_iUIBlockFactory.php'], $this->sCacheRootDir.'/InterfaceDiscovery'); } private function GivenClassMap(array $aClassMap): void From 47cd8bce3156282a1c13506a6d08acda431f57f4 Mon Sep 17 00:00:00 2001 From: jf-cbd Date: Mon, 16 Dec 2024 11:05:16 +0100 Subject: [PATCH 5/5] Security hardening --- .../src/Controller/ObjectController.php | 41 ++++++++++++++++++- 1 file changed, 39 insertions(+), 2 deletions(-) diff --git a/datamodels/2.x/itop-portal-base/portal/src/Controller/ObjectController.php b/datamodels/2.x/itop-portal-base/portal/src/Controller/ObjectController.php index a1db8de6f..84c300cd9 100644 --- a/datamodels/2.x/itop-portal-base/portal/src/Controller/ObjectController.php +++ b/datamodels/2.x/itop-portal-base/portal/src/Controller/ObjectController.php @@ -1299,6 +1299,11 @@ class ObjectController extends BrickController $bIgnoreSilos = $this->oScopeValidatorHelper->IsAllDataAllowedForScope(UserRights::ListProfiles(), $sObjectClass); $aParams = array('objects_id' => $aObjectIds); $oSearch = DBObjectSearch::FromOQL("SELECT $sObjectClass WHERE id IN (:objects_id)"); + if (!$this->oScopeValidatorHelper->AddScopeToQuery($oSearch, $sObjectClass) + ) { + IssueLog::Warning(__METHOD__.' at line '.__LINE__.' : User #'.UserRights::GetUserId().' not allowed to read '.$sObjectClass.' object.'); + throw new HttpException(Response::HTTP_NOT_FOUND, Dict::S('UI:ObjectDoesNotExist')); + } if ($bIgnoreSilos === true) { $oSearch->AllowAllData(); } @@ -1349,7 +1354,10 @@ class ObjectController extends BrickController $aObjectAttCodes = $this->oRequestManipulatorHelper->ReadParam('aObjectAttCodes', array(), FILTER_UNSAFE_RAW, FILTER_REQUIRE_ARRAY); $aLinkAttCodes = $this->oRequestManipulatorHelper->ReadParam('aLinkAttCodes', array(), FILTER_UNSAFE_RAW, FILTER_REQUIRE_ARRAY); $sDateTimePickerWidgetParent = $this->oRequestManipulatorHelper->ReadParam('sDateTimePickerWidgetParent', array(), FILTER_UNSAFE_RAW, FILTER_REQUIRE_ARRAY); - + if (!MetaModel::IsLinkClass($sLinkClass)) { + IssueLog::Warning(__METHOD__.' at line '.__LINE__.' : User #'.UserRights::GetUserId().' asked for wrong lnk class '.$sLinkClass); + throw new HttpException(Response::HTTP_NOT_FOUND, Dict::S('UI:ObjectDoesNotExist')); + } if (empty($sObjectClass) || empty($aObjectIds) || empty($aObjectAttCodes)) { IssueLog::Info(__METHOD__.' at line '.__LINE__.' : sObjectClass, aObjectIds and aObjectAttCodes expected, "'.$sObjectClass.'", "'.implode('/', $aObjectIds).'" given.'); @@ -1360,6 +1368,10 @@ class ObjectController extends BrickController $bIgnoreSilos = $this->oScopeValidatorHelper->IsAllDataAllowedForScope(UserRights::ListProfiles(), $sObjectClass); $aParams = array('objects_id' => $aObjectIds); $oSearch = DBObjectSearch::FromOQL("SELECT $sObjectClass WHERE id IN (:objects_id)"); + if (!$this->oScopeValidatorHelper->AddScopeToQuery($oSearch, $sObjectClass)) { + IssueLog::Warning(__METHOD__.' at line '.__LINE__.' : User #'.UserRights::GetUserId().' not allowed to read '.$sObjectClass.' object.'); + throw new HttpException(Response::HTTP_NOT_FOUND, Dict::S('UI:ObjectDoesNotExist')); + } if ($bIgnoreSilos === true) { $oSearch->AllowAllData(); @@ -1378,10 +1390,35 @@ class ObjectController extends BrickController // Prepare link data $aObjectData = $this->PrepareObjectInformation($oObject, $aObjectAttCodes); // New link object (needed for renderers) - $oNewLink = new $sLinkClass(); + $aAttCodes = MetaModel::GetAttributesList($sLinkClass, ['AttributeExternalKey']); + $sAttCodeToObject = ''; + foreach ($aAttCodes as $sAttCode) { + $oAttDef = MetaModel::GetAttributeDef($sLinkClass, $sAttCode); + /** @var \AttributeExternalKey $oAttDef */ + if ($oAttDef->GetTargetClass() === $sObjectClass) { + $sAttCodeToObject = $sAttCode; + } + } + if ($sAttCodeToObject === '') { + IssueLog::Warning(__METHOD__.' at line '.__LINE__.' : User #'.UserRights::GetUserId().' asked for incoherent lnk class '.$sLinkClass.' with object class '.$sObjectClass); + throw new HttpException(Response::HTTP_NOT_FOUND, Dict::S('UI:ObjectDoesNotExist')); + } + $oNewLink = MetaModel::NewObject($sLinkClass, [ + $sAttCodeToObject => $oObject->GetKey(), // so later placeholders in filters will be applied on external keys on the same link + ]); foreach ($aLinkAttCodes as $sAttCode) { $oAttDef = MetaModel::GetAttributeDef($sLinkClass, $sAttCode); + /** @var \Combodo\iTop\Form\Field\SelectObjectField $oField */ $oField = $oAttDef->MakeFormField($oNewLink); + if ($oAttDef::GetFormFieldClass() === '\\Combodo\\iTop\\Form\\Field\\SelectObjectField') { + $oFieldSearch = $oField->GetSearch(); + $sFieldClass = $oFieldSearch->GetClass(); + if ($this->oScopeValidatorHelper->AddScopeToQuery($oFieldSearch, $sFieldClass)){ + $oField->SetSearch($oFieldSearch); + } else { + $oField->SetSearch(DBObjectSearch::FromOQL("SELECT $sFieldClass WHERE 1=0")); + } + } // Prevent datetimepicker popup to be truncated if ($oField instanceof DateTimeField) { $oField->SetDateTimePickerWidgetParent($sDateTimePickerWidgetParent);