N°2154 - Security breach

datamodels/2.x/itop-config/config.php

@@ -24,54 +24,30 @@
  * @license     http://opensource.org/licenses/AGPL-3.0
  */
 
+use Combodo\iTop\Config\Validator\iTopConfigAstValidator;
+use Combodo\iTop\Config\Validator\iTopConfigSyntaxValidator;
+
 require_once(APPROOT.'application/application.inc.php');
 require_once(APPROOT.'application/itopwebpage.class.inc.php');
 require_once(APPROOT.'application/startup.inc.php');
 require_once(APPROOT.'application/loginwebpage.class.inc.php');
 
 
+/**
+ * @param $sContents
+ * @param $oP
+ *
+ * @throws \Exception
+ */
 function TestConfig($sContents, $oP)
 {
-	try
-	{
-		ini_set('display_errors', 1);
-		ob_start();
-        // in PHP < 7.0.0 syntax errors are in output
-        // in PHP >= 7.0.0 syntax errors are thrown as Error
-        $sSafeContent = preg_replace(array('#^\s*<\?php#', '#\?>\s*$#'), '', $sContents);
-        eval('if(0){'.trim($sSafeContent).'}');
-        $sNoise = trim(ob_get_contents());
-		ob_end_clean();
-    }
-    catch (Error $e)
-    {
-        // ParseError only thrown in PHP7
-		throw new Exception('Error in configuration: '.$e->getMessage().' at line '.$e->getLine());
-	}
-	if (strlen($sNoise) > 0)
-	{
-		if (preg_match("/(Error|Parse error|Notice|Warning): (.+) in \S+ : eval\(\)'d code on line (\d+)/i", strip_tags($sNoise), $aMatches))
-		{
-			$sMessage = $aMatches[2];
-			$sLine = $aMatches[3];
-			$iLine = (int) $sLine;
+	/// 1- first check if there is no malicious code
+	$oiTopConfigValidator = new iTopConfigAstValidator();
+	$oiTopConfigValidator->validate($sContents);
 
-			// Highlight the line
-			$aLines = explode("\n", $sContents);
-			$iStart = 0;
-			for ($i = 0 ; $i < $iLine - 1; $i++) $iStart += strlen($aLines[$i]);
-			$iEnd = $iStart + strlen($aLines[$iLine - 1]);
-			$iTotalLines = count($aLines);
-
-			$sMessage = Dict::Format('config-parse-error', $sMessage, $sLine);
-			throw new Exception($sMessage);
-		}
-		else
-		{
-			// Note: sNoise is an html output, but so far it was ok for me (e.g. showing the entire call stack) 
-			throw new Exception('Syntax error in configuration file: <tt>'.$sNoise.'</tt>');
-		}
-	}
+	/// 2 - only after we are sure that there is no malicious cade, we can perform a syntax check!
+	$oiTopConfigValidator = new iTopConfigSyntaxValidator();
+	$oiTopConfigValidator->validate($sContents, true);
 }
 
 /**

datamodels/2.x/itop-config/module.itop-config.php

@@ -20,7 +20,10 @@ SetupWebPage::AddModule(
 		//
 		'datamodel' => array(
 			'model.itop-config.php',
-		),
+			'src/Validator/ConfigNodesVisitor.php',
+			'src/Validator/iTopConfigAstValidator.php',
+			'src/Validator/iTopConfigSyntaxValidator.php',
+),
 		'webservice' => array(),
 		'dictionary' => array(
 			'en.dict.itop-config.php',

datamodels/2.x/itop-config/src/Validator/ConfigNodesVisitor.php

@@ -0,0 +1,143 @@
+<?php
+/**
+ * Created by Bruno DA SILVA, working for Combodo
+ * Date: 31/12/2019
+ * Time: 14:12
+ */
+
+namespace Combodo\iTop\Config\Validator;
+
+
+use PhpParser\Node;
+use PhpParser\NodeVisitorAbstract;
+
+class ConfigNodesVisitor extends NodeVisitorAbstract
+{
+	private $aAllowedNodeClasses = array();
+
+	public function __construct()
+	{
+		$this->aAllowedNodeClasses = array(
+			Node\Scalar::class,
+
+			Node\Name::class,
+
+			Node\Const_::class,
+
+			Node\Expr\Array_::class,
+			Node\Expr\ArrayDimFetch::class,
+			Node\Expr\ArrayItem::class,
+			Node\Expr\Assign::class,
+			Node\Expr\AssignOp::class,
+			Node\Expr\AssignRef::class,
+			Node\Expr\BinaryOp::class,
+			Node\Expr\BitwiseNot::class,
+			Node\Expr\BooleanNot::class,
+			Node\Expr\Cast::class,
+			Node\Expr\ClassConstFetch::class,
+			Node\Expr\ConstFetch::class,
+			Node\Expr\Instanceof_::class,
+			Node\Expr\Isset_::class,
+			Node\Expr\List_::class,
+			Node\Expr\PostDec::class,
+			Node\Expr\PostInc::class,
+			Node\Expr\PreDec::class,
+			Node\Expr\PreInc::class,
+			Node\Expr\Print_::class,
+			Node\Expr\Ternary::class,
+			Node\Expr\UnaryMinus::class,
+			Node\Expr\UnaryPlus::class,
+			Node\Expr\Variable::class,
+
+			Node\Stmt\Const_::class,
+			Node\Stmt\Global_::class,
+		);
+	}
+
+	/**
+	 * @param \PhpParser\Node $node
+	 *
+	 * @return int|\PhpParser\Node|void|null
+	 * @throws \Exception
+	 */
+	public function enterNode(Node $node)
+	{
+		$this->ValidateNode($node);
+	}
+
+	/**
+	 * @param \PhpParser\Node $node
+	 *
+	 * @throws \Exception
+	 */
+	public function ValidateNode(Node $node)
+	{
+		foreach ($this->aAllowedNodeClasses as $sAllowedNodeClass)
+		{
+			if ($node instanceof $sAllowedNodeClass)
+			{
+				return;
+			}
+		}
+
+		$this->ThrowInvalidConf($node);
+	}
+
+	/**
+	 * @param \PhpParser\Node $node
+	 *
+	 * @throws \Exception
+	 */
+	private function ThrowInvalidConf(Node $node)
+	{
+		if (in_array('name', $node->getSubNodeNames()))
+		{
+			$sMessage = sprintf(
+				"Invalid configuration: %s of type %s is forbidden in line %d",
+				$node->name,
+				$node->getType(),
+				$node->getLine()
+			);
+		}
+		elseif (in_array('class', $node->getSubNodeNames()))
+		{
+
+			if (in_array('name', $node->class->getSubNodeNames()))
+			{
+				$sMessage = sprintf(
+					"Invalid configuration: usage of the class '%s' (%s) is forbidden in line %d",
+					is_object($node->class) ? $node->class->name : $node->class,
+					$node->getType(),
+					$node->getLine()
+				);
+			}
+			else
+			{
+				$sMessage = sprintf(
+					"Invalid configuration: usage of %s is forbidden in line %d",
+					$node->getType(),
+					$node->getLine()
+				);
+			}
+		}
+		elseif ($node->hasAttribute('name'))
+		{
+			$sMessage = sprintf(
+				"Invalid configuration: %s of type %s is forbidden in line %d",
+				$node->getAttribute('name'),
+				$node->getType(),
+				$node->getLine()
+			);
+		}
+		else
+		{
+			$sMessage = sprintf(
+				"Invalid configuration: %s is forbidden in line %d",
+				$node->getType(),
+				$node->getLine()
+			);
+		}
+
+		throw new \Exception($sMessage);
+	}
+}

datamodels/2.x/itop-config/src/Validator/iTopConfigAstValidator.php

@@ -0,0 +1,44 @@
+<?php
+/**
+ * Created by Bruno DA SILVA, working for Combodo
+ * Date: 31/12/2019
+ * Time: 12:29
+ */
+
+namespace Combodo\iTop\Config\Validator;
+
+
+use PhpParser\NodeTraverser;
+use PhpParser\ParserFactory;
+
+class iTopConfigAstValidator
+{
+	/**
+	 * validate.
+	 *
+	 * @param $sConfig
+	 * @param \PhpParser\Parser|null $oParser
+	 *
+	 * @throws \Exception
+	 */
+	public function validate($sConfig)
+	{
+		$oParser = (new ParserFactory())->create(ParserFactory::PREFER_PHP7);
+
+		$oNodeVisitor = new ConfigNodesVisitor();
+
+		try {
+			$aInitialNodes = $oParser->parse($sConfig);
+		} catch (\Error $e) {
+			$sMessage = 'Invalid configuration: '. \Dict::Format('config-parse-error', $e->getMessage(), $e->getLine());
+			throw new \Exception($sMessage, 0, $e);
+		}catch (\Exception $e) {
+			$sMessage = 'Invalid configuration: '. \Dict::Format('config-parse-error', $e->getMessage(), $e->getLine());
+			throw new \Exception($sMessage, 0, $e);
+		}
+
+		$oTraverser = new NodeTraverser();
+		$oTraverser->addVisitor($oNodeVisitor);
+		$oTraverser->traverse($aInitialNodes);
+	}
+}

datamodels/2.x/itop-config/src/Validator/iTopConfigSyntaxValidator.php

@@ -0,0 +1,105 @@
+<?php
+/**
+ * Created by Bruno DA SILVA, working for Combodo
+ * Date: 31/12/2019
+ * Time: 12:29
+ */
+
+namespace Combodo\iTop\Config\Validator;
+
+
+use PhpParser\NodeTraverser;
+use PhpParser\NodeVisitor;
+use PhpParser\Parser;
+use PhpParser\ParserFactory;
+
+class iTopConfigSyntaxValidator
+{
+	/**
+	 * validate
+	 *
+	 * @param $sConfig
+	 * @param $bAllowUnsecure
+	 */
+	public function validate($sConfig, $bAllowUnsecure)
+	{
+		exec('php -v', $aOutput, $iReturnVar);
+		$bCanRunCli = ($iReturnVar == 0);
+
+		if ($bCanRunCli)
+		{
+			$this->CheckSyntaxSecure($sConfig);
+		}
+		elseif($bAllowUnsecure)
+		{
+			$this->CheckSyntaxNotSecure($sConfig);
+		}
+		else
+		{
+			throw new \Exception('Cannot check configuration syntax: PHP CLI is not accessible.'."\n".implode("\n", $aOutput));
+		}
+	}
+
+	/**
+	 * This will use the php cli linter in order to check the syntax,
+	 *
+	 * The php cli may not be based on the same php version, but since the cron run using the cli, we can assume that it is well configured anyway...
+	 * Also, the config syntax is very limited so there should not be a problem with checking the validity against another php version
+	 *
+	 * @param $sConfig
+	 * @param $iReturnVar
+	 * @param $aOutput
+	 *
+	 * @return array
+	 */
+	private function CheckSyntaxSecure($sConfig)
+	{
+		$sTempFile = tempnam(sys_get_temp_dir(), 'syntax_check_me_').'.temp.txt';
+		file_put_contents($sTempFile, $sConfig);
+		exec("php -l $sTempFile 2>&1", $aOutput, $iReturnVar);
+		unlink($sTempFile);
+
+		if ($iReturnVar != 0)
+		{
+			throw new \Exception(implode("\n", $aOutput));
+		}
+	}
+
+	/**
+	 * @param $sRawConfig
+	 */
+	private function CheckSyntaxNotSecure($sRawConfig)
+	{
+		try
+		{
+			ini_set('display_errors', 1);
+			ob_start();
+			// in PHP < 7.0.0 syntax errors are in output
+			// in PHP >= 7.0.0 syntax errors are thrown as Error
+			$sConfig = preg_replace(array('#^\s*<\?php#', '#\?>\s*$#'), '', $sRawConfig);
+			eval('if(0){'.trim($sConfig).'}');
+			$sNoise = trim(ob_get_contents());
+			ob_end_clean();
+		}
+		catch (Error $e)
+		{
+			// ParseError only thrown in PHP7
+			throw new Exception('Error in configuration: '.$e->getMessage().' at line '.$e->getLine());
+		}
+		if (strlen($sNoise) > 0)
+		{
+			if (preg_match("/(Error|Parse error|Notice|Warning): (.+) in \S+ : eval\(\)'d code on line (\d+)/i", strip_tags($sNoise), $aMatches))
+			{
+				$sMessage = $aMatches[2];
+				$sLine = $aMatches[3];
+				$sMessage = Dict::Format('config-parse-error', $sMessage, $sLine);
+				throw new Exception($sMessage);
+			}
+			else
+			{
+				// Note: sNoise is an html output, but so far it was ok for me (e.g. showing the entire call stack)
+				throw new Exception('Syntax error in configuration file: <tt>'.$sNoise.'</tt>');
+			}
+		}
+	}
+}

datamodels/2.x/itop-config/test/Validator/iTopConfigAstValidatorTest.php

@@ -0,0 +1,98 @@
+<?php
+/**
+ * Created by Bruno DA SILVA, working for Combodo
+ * Date: 31/12/2019
+ * Time: 12:31
+ */
+
+namespace Combodo\iTop\Config\Test\Validator;
+
+use Combodo\iTop\Config\Validator\iTopConfigAstValidator;
+use Combodo\iTop\Test\UnitTest\ItopTestCase;
+use PhpParser\Node;
+use PhpParser\PrettyPrinter\Standard;
+
+class iTopConfigAstValidatorTest extends ItopTestCase
+{
+
+	public function setUp()
+	{
+		parent::setUp();
+
+		require_once __DIR__.'/../../src/Validator/ConfigNodesVisitor.php';
+		require_once __DIR__.'/../../src/Validator/iTopConfigAstValidator.php';
+	}
+
+
+	/**
+	 * @dataProvider InvalidDataProvider
+	 * @param $sConf
+	 *
+	 * @throws \Exception
+	 */
+	public function testInvalid($sConf)
+	{
+		$oiTopConfigValidator = new iTopConfigAstValidator();
+		$this->expectException(\Exception::class);
+		try{
+			$oiTopConfigValidator->validate($sConf);
+		}catch (\Exception $e)
+		{
+			$this->assertStringStartsWith('Invalid configuration:', $e->getMessage());
+			throw $e;
+		}
+	}
+
+
+	public function InvalidDataProvider()
+	{
+		return array(
+			'invalid PHP' => array(
+				'sConf' => '<?php fiction Method(){}'
+			),
+			'function call' => array(
+				'sConf' => '<?php FunctionCall();'
+			),
+			'function declaration' => array(
+				'sConf' => '<?php function foo() {};'
+			),
+			'class instantiation' => array(
+				'sConf' => '<?php new Class {};'
+			),
+			'Class declaration' => array(
+				'sConf' => '<?php class foo {};'
+			),
+			'echo' => array(
+				'sConf' => '<?php echo "toto"; ?>'
+			),
+		);
+	}
+
+
+	/**
+	 * @dataProvider ValidDataProvider
+	 * @doesNotPerformAssertions
+	 *
+	 * @param $sConf
+	 *
+	 * @throws \Exception
+	 */
+	public function testValid($sConf)
+	{
+		$oiTopConfigValidator = new iTopConfigAstValidator();
+
+		$oiTopConfigValidator->validate($sConf);
+	}
+
+	public function ValidDataProvider()
+	{
+		return array(
+			'simple code' => array(
+				'sConf' => '<?php $var = array("toto"); ?>'
+			),
+			'class constant' => array(
+				'sConf' => '<?php $var = array(foo::bar);'
+			),
+		);
+	}
+}

datamodels/2.x/itop-config/test/Validator/iTopConfigSyntaxValidatorTest.php

@@ -0,0 +1,51 @@
+<?php
+/**
+ * Created by Bruno DA SILVA, working for Combodo
+ * Date: 31/12/2019
+ * Time: 12:31
+ */
+
+namespace Combodo\iTop\Config\Test\Validator;
+
+use Combodo\iTop\Config\Validator\iTopConfigAstValidator;
+use Combodo\iTop\Config\Validator\iTopConfigSyntaxValidator;
+use Combodo\iTop\Test\UnitTest\ItopTestCase;
+use PhpParser\Node;
+use PhpParser\PrettyPrinter\Standard;
+
+class iTopConfigAstValidatorTest extends ItopTestCase
+{
+
+	public function setUp()
+	{
+		parent::setUp();
+
+		require_once __DIR__.'/../../src/Validator/ConfigNodesVisitor.php';
+		require_once __DIR__.'/../../src/Validator/iTopConfigSyntaxValidator.php';
+	}
+
+
+	/**
+	 * @throws \Exception
+	 * @doesNotPerformAssertions
+	 */
+	public function testValidCode()
+	{
+		$oiTopConfigValidator = new iTopConfigSyntaxValidator();
+		$oiTopConfigValidator->validate("<?php \n echo 'foo'; ", false);
+	}
+
+	public function testThrowOnInvalidCode()
+	{
+		$oiTopConfigValidator = new iTopConfigSyntaxValidator();
+
+		$this->expectException(\Exception::class);
+		try{
+			$oiTopConfigValidator->validate("<?php \n zef;zefzef \n zdadz = azdazd \n zerfgzaezerfgzef>", false);
+		}catch (\Exception $e)
+		{
+			$this->assertStringStartsWith('PHP Parse error:  syntax error, unexpected \'zdadz\' (T_STRING)', $e->getMessage());
+			throw $e;
+		}
+	}
+}