CVE-2022-24780: Improper Control of Generation of Code ('Code Injection') (CWE-94)
value: Combodo iTop is a web based IT Service Management tool. In versions prior to 2.7.6 and 3.0.0, users of the iTop user portal can send TWIG code to the server by forging specific http queries, and execute arbitrary code on the server using http server user privileges. This issue is fixed in versions 2.7.6 and 3.0.0. There are currently no known workarounds.
CWE description: The product constructs all or part of a code segment using externally-influenced input from an upstream component, but it does not neutralize or incorrectly neutralizes special elements that could modify the syntax or behavior of the intended code segment.
CVSS v3 base score: 8.8
Published: 2022-04-05T19:15Z
Repository: https://github.com/Combodo/iTop
Language: PHP
Files changed: datamodels/2.x/itop-portal-base/portal/src/Form/ObjectFormManager.php, datamodels/2.x/itop-portal-base/portal/src/Helper/ObjectFormHandlerHelper.php, sources/form/formmanager.class.inc.php
Commit date: 2021-11-25 16:07:40 +0100
Commit message: N°4384 Security hardening

Diff: diff --git a/datamodels/2.x/itop-portal-base/portal/src/Form/ObjectFormManager.php b/datamodels/2.x/itop-portal-base/portal/src/Form/ObjectFormManager.php
index 3fac09f482..2705b8c4db 100644
--- a/datamodels/2.x/itop-portal-base/portal/src/Form/ObjectFormManager.php
+++ b/datamodels/2.x/itop-portal-base/portal/src/Form/ObjectFormManager.php
@@ -89,30 +89,38 @@ class ObjectFormManager extends FormManager
 	 * Creates an instance of \Combodo\iTop\Portal\Form\ObjectFormManager from JSON data that must contain at least :
 	 * - formobject_class : The class of the object that is being edited/viewed
 	 * - formmode : view|edit|create
 	 * - values for parent
 	 *
+	 * @param bool $bTrustContent if false then won't allow TWIG content
+	 *
 	 * @inheritDoc
 	 * @throws \Exception
+	 * @throws \SecurityException if twig content is present and $bTrustContent is false
 	 */
-	static function FromJSON($sJson)
+	public static function FromJSON($sJson, $bTrustContent = false)
 	{
-		if (is_array($sJson))
-		{
+		if (is_array($sJson)) {
 			$aJson = $sJson;
-		}
-		else
-		{
+		} else {
 			$aJson = json_decode($sJson, true);
 		}
 
+		if (false === $bTrustContent) {
+			/** @noinspection NestedPositiveIfStatementsInspection */
+			if (isset($aJson['formproperties']['layout']['type']) && ($aJson['formproperties']['layout']['type'] === 'twig')) {
+				// There will be an IssueLog above in the hierarchy due to the exception, but we are logging here so that we can output the JSON data !
+				IssueLog::Error('Portal received a query with forbidden twig content!', \LogChannels::PORTAL, ['formmanager_data' => $aJson]);
+				throw new \SecurityException('Twig content not allowed in this context!');
+			}
+		}
+
 		/** @var \Combodo\iTop\Portal\Form\ObjectFormManager $oFormManager */
 		$oFormManager = parent::FromJSON($sJson);
 
 		// Retrieving object to edit
-		if (!isset($aJson['formobject_class']))
-		{
+		if (!isset($aJson['formobject_class'])) {
 			throw new Exception('Object class must be defined in order to generate the form');
 		}
 		$sObjectClass = $aJson['formobject_class'];
 
 		if (!isset($aJson['formobject_id']))
@@ -149,18 +157,48 @@ class ObjectFormManager extends FormManager
 			}
 			$oFormManager->SetFormProperties($aJson['formproperties']);
 		}
 
 		// Retrieving callback urls
-		if (!isset($aJson['formcallbacks']))
-		{
+		if (!isset($aJson['formcallbacks'])) {
 			// TODO
 		}
 
 		return $oFormManager;
 	}
 
+	/**
+	 * @param string $sPostedFormManagerData received data from the browser
+	 * @param array $aOriginalFormProperties data generated server side
+	 *
+	 * @return bool true if the data are identical
+	 *
+	 * @since 2.7.6 3.0.0 N°4384 check formmanager_data
+	 */
+	public static function CanTrustFormLayoutContent($sPostedFormManagerData, $aOriginalFormProperties)
+	{
+		$aPostedFormManagerData = json_decode($sPostedFormManagerData, true);
+		$sPostedFormLayoutType = (isset($aPostedFormManagerData['formproperties']['layout']['type'])) ? $aPostedFormManagerData['formproperties']['layout']['type'] : '';
+
+		if ($sPostedFormLayoutType === 'xhtml') {
+			return true;
+		}
+
+		// we need to parse the content so that autoclose tags are returned correctly (`<div />` => `<div></div>`)
+		$oHtmlDocument = new \DOMDocument();
+
+		$sPostedFormLayoutContent = (isset($aPostedFormManagerData['formproperties']['layout']['content'])) ? $aPostedFormManagerData['formproperties']['layout']['content'] : '';
+		$oHtmlDocument->loadXML('<root>'.$sPostedFormLayoutContent.'</root>');
+		$sPostedFormLayoutRendered = $oHtmlDocument->saveHTML();
+
+		$sOriginalFormLayoutContent = (isset($aOriginalFormProperties['layout']['content'])) ? $aOriginalFormProperties['layout']['content'] : '';
+		$oHtmlDocument->loadXML('<root>'.$sOriginalFormLayoutContent.'</root>');
+		$sOriginalFormLayoutContentRendered = $oHtmlDocument->saveHTML();
+
+		return ($sPostedFormLayoutRendered === $sOriginalFormLayoutContentRendered);
+	}
+
 	/**
 	 *
 	 * @return \Symfony\Component\DependencyInjection\ContainerInterface
 	 */
 	public function GetContainer()
diff --git a/datamodels/2.x/itop-portal-base/portal/src/Helper/ObjectFormHandlerHelper.php b/datamodels/2.x/itop-portal-base/portal/src/Helper/ObjectFormHandlerHelper.php
index d114b1b053..ff4fcbd492 100644
--- a/datamodels/2.x/itop-portal-base/portal/src/Helper/ObjectFormHandlerHelper.php
+++ b/datamodels/2.x/itop-portal-base/portal/src/Helper/ObjectFormHandlerHelper.php
@@ -106,14 +106,14 @@ class ObjectFormHandlerHelper
 		$this->oContainer = $oContainer;
 	}
 
 	/**
 	 * @param \Symfony\Component\HttpFoundation\Request $oRequest
-	 * @param string                                    $sMode
-	 * @param string                                    $sObjectClass
-	 * @param string                                    $sObjectId
-	 * @param string                                    $aFormProperties
+	 * @param string $sMode
+	 * @param string $sObjectClass
+	 * @param string $sObjectId
+	 * @param array $aFormProperties
 	 *
 	 * @return array
 	 *
 	 * @throws \ArchivedObjectException
 	 * @throws \CoreException
@@ -125,13 +125,14 @@ class ObjectFormHandlerHelper
 		$aFormData = array();
 		$sOperation = $this->oRequestManipulator->ReadParam('operation', '');
 		$bModal = ($oRequest->isXmlHttpRequest() && empty($sOperation));
 
 		// - Retrieve form properties
+		$aOriginalFormProperties = ApplicationHelper::GetLoadedFormFromClass($this->aCombodoPortalInstanceConf['forms'], $sObjectClass, $sMode);
 		if ($aFormProperties === null)
 		{
-			$aFormProperties = ApplicationHelper::GetLoadedFormFromClass($this->aCombodoPortalInstanceConf['forms'], $sObjectClass, $sMode);
+			$aFormProperties = $aOriginalFormProperties;
 		}
 
 		// - Create and
 		if (empty($sOperation))
 		{
@@ -279,31 +280,28 @@ class ObjectFormHandlerHelper
 				->SetRenderer($oFormRenderer)
 				->SetFormProperties($aFormProperties);
 
 			$oFormManager->Build();
 			$aFormData['hidden_fields'] = $oFormManager->GetHiddenFieldsId();
-				// Check the number of editable fields
+			// Check the number of editable fields
 			$aFormData['editable_fields_count'] = $oFormManager->GetForm()->GetEditableFieldCount();
-		}
-		else
-		{
+		} else {
 			// Update / Submit / Cancel
+			/** @var \Combodo\iTop\Portal\Form\ObjectFormManager $sFormManagerClass */
 			$sFormManagerClass = $this->oRequestManipulator->ReadParam('formmanager_class', '', FILTER_UNSAFE_RAW);
 			$sFormManagerData = $this->oRequestManipulator->ReadParam('formmanager_data', '', FILTER_UNSAFE_RAW);
-			if (empty($sFormManagerClass) || empty($sFormManagerData))
-			{
+			if (empty($sFormManagerClass) || empty($sFormManagerData)) {
 				IssueLog::Error(__METHOD__.' at line '.__LINE__.' : Parameters formmanager_class and formamanager_data must be defined.');
 				throw new HttpException(Response::HTTP_INTERNAL_SERVER_ERROR, 'Parameters formmanager_class and formmanager_data must be defined.');
 			}
 
-			/** @var \Combodo\iTop\Portal\Form\ObjectFormManager $oFormManager */
-			$oFormManager = $sFormManagerClass::FromJSON($sFormManagerData);
+			$bTrustContent = $sFormManagerClass::CanTrustFormLayoutContent($sFormManagerData, $aOriginalFormProperties);
+			$oFormManager = $sFormManagerClass::FromJSON($sFormManagerData, $bTrustContent);
 			$oFormManager->SetContainer($this->oContainer);
 
 			// Applying action rules if present
-			if (($oFormManager->GetActionRulesToken() !== null) && ($oFormManager->GetActionRulesToken() !== ''))
-			{
+			if (($oFormManager->GetActionRulesToken() !== null) && ($oFormManager->GetActionRulesToken() !== '')) {
 				$aActionRules = ContextManipulatorHelper::DecodeRulesToken($oFormManager->GetActionRulesToken());
 				$oObj = $oFormManager->GetObject();
 				$this->oContextManipulator->PrepareObject($aActionRules, $oObj);
 				$oFormManager->SetObject($oObj);
 			}
diff --git a/sources/form/formmanager.class.inc.php b/sources/form/formmanager.class.inc.php
index feeb2359ef..4f20d1f4c4 100644
--- a/sources/form/formmanager.class.inc.php
+++ b/sources/form/formmanager.class.inc.php
@@ -38,11 +38,12 @@ abstract class FormManager
 	/**
 	 * Creates an instance of \Combodo\iTop\Form\FormManager from JSON data that must contain at least :
 	 * - formrenderer_class : The class of the FormRenderer to use in the FormManager
 	 * - formrenderer_endpoint : The endpoint of the renderer
 	 *
-	 * @param string $sJson
+	 * @param string|string[] $sJson
+	 *
 	 * @return $this
 	 */
 	static function FromJSON($sJson)
 	{
 		// Overload in child class when needed

Vulnerable code: 	static function FromJSON($sJson)
		if (is_array($sJson))
		{
		}
		else
		{
		if (!isset($aJson['formobject_class']))
		{
		if (!isset($aJson['formcallbacks']))
		{
	 * @param string                                    $sMode
	 * @param string                                    $sObjectClass
	 * @param string                                    $sObjectId
	 * @param string                                    $aFormProperties
			$aFormProperties = ApplicationHelper::GetLoadedFormFromClass($this->aCombodoPortalInstanceConf['forms'], $sObjectClass, $sMode);
				// Check the number of editable fields
		}
		else
		{
			if (empty($sFormManagerClass) || empty($sFormManagerData))
			{
			/** @var \Combodo\iTop\Portal\Form\ObjectFormManager $oFormManager */
			$oFormManager = $sFormManagerClass::FromJSON($sFormManagerData);
			if (($oFormManager->GetActionRulesToken() !== null) && ($oFormManager->GetActionRulesToken() !== ''))
			{
	 * @param string $sJson
Fixed code: 	 * @param bool $bTrustContent if false then won't allow TWIG content
	 *
	 * @throws \SecurityException if twig content is present and $bTrustContent is false
	public static function FromJSON($sJson, $bTrustContent = false)
		if (is_array($sJson)) {
		} else {
		if (false === $bTrustContent) {
			/** @noinspection NestedPositiveIfStatementsInspection */
			if (isset($aJson['formproperties']['layout']['type']) && ($aJson['formproperties']['layout']['type'] === 'twig')) {
				// There will be an IssueLog above in the hierarchy due to the exception, but we are logging here so that we can output the JSON data !
				IssueLog::Error('Portal received a query with forbidden twig content!', \LogChannels::PORTAL, ['formmanager_data' => $aJson]);
				throw new \SecurityException('Twig content not allowed in this context!');
			}
		}

		if (!isset($aJson['formobject_class'])) {
		if (!isset($aJson['formcallbacks'])) {
	/**
	 * @param string $sPostedFormManagerData received data from the browser
	 * @param array $aOriginalFormProperties data generated server side
	 *
	 * @return bool true if the data are identical
	 *
	 * @since 2.7.6 3.0.0 N°4384 check formmanager_data
	 */
	public static function CanTrustFormLayoutContent($sPostedFormManagerData, $aOriginalFormProperties)
	{
		$aPostedFormManagerData = json_decode($sPostedFormManagerData, true);
		$sPostedFormLayoutType = (isset($aPostedFormManagerData['formproperties']['layout']['type'])) ? $aPostedFormManagerData['formproperties']['layout']['type'] : '';

		if ($sPostedFormLayoutType === 'xhtml') {
			return true;
		}

		// we need to parse the content so that autoclose tags are returned correctly (`<div />` => `<div></div>`)
		$oHtmlDocument = new \DOMDocument();

		$sPostedFormLayoutContent = (isset($aPostedFormManagerData['formproperties']['layout']['content'])) ? $aPostedFormManagerData['formproperties']['layout']['content'] : '';
		$oHtmlDocument->loadXML('<root>'.$sPostedFormLayoutContent.'</root>');
		$sPostedFormLayoutRendered = $oHtmlDocument->saveHTML();

		$sOriginalFormLayoutContent = (isset($aOriginalFormProperties['layout']['content'])) ? $aOriginalFormProperties['layout']['content'] : '';
		$oHtmlDocument->loadXML('<root>'.$sOriginalFormLayoutContent.'</root>');
		$sOriginalFormLayoutContentRendered = $oHtmlDocument->saveHTML();

		return ($sPostedFormLayoutRendered === $sOriginalFormLayoutContentRendered);
	}

	 * @param string $sMode
	 * @param string $sObjectClass
	 * @param string $sObjectId
	 * @param array $aFormProperties
		$aOriginalFormProperties = ApplicationHelper::GetLoadedFormFromClass($this->aCombodoPortalInstanceConf['forms'], $sObjectClass, $sMode);
			$aFormProperties = $aOriginalFormProperties;
			// Check the number of editable fields
		} else {
			/** @var \Combodo\iTop\Portal\Form\ObjectFormManager $sFormManagerClass */
			if (empty($sFormManagerClass) || empty($sFormManagerData)) {
			$bTrustContent = $sFormManagerClass::CanTrustFormLayoutContent($sFormManagerData, $aOriginalFormProperties);
			$oFormManager = $sFormManagerClass::FromJSON($sFormManagerData, $bTrustContent);
			if (($oFormManager->GetActionRulesToken() !== null) && ($oFormManager->GetActionRulesToken() !== '')) {
	 * @param string|string[] $sJson
	 *