Fix security. Make bypass of checkPHPCode more difficult

Dolibarr · Dec 18, 2024 · 27ac538 · 27ac538
1 parent 5a88a40
commit 27ac538
Show file tree

Hide file tree

Showing 2 changed files with 37 additions and 17 deletions.
diff --git a/htdocs/core/lib/website2.lib.php b/htdocs/core/lib/website2.lib.php
@@ -694,8 +694,8 @@ function showWebsiteTemplates(Website $website)
 
 /**
  * Check a new string containing only php code (including <php tag)
- * - Block if bad code in the new string.
- * - Block also if user has no permission to change PHP code.
+ * - Block if user has no permission to change PHP code.
+ * - Block also if bad code found in the new string.
  *
  * @param	string		$phpfullcodestringold		PHP old string (before the change). For example "<?php echo 'a' ?><php echo 'b' ?>"
  * @param	string		$phpfullcodestring			PHP new string. For example "<?php echo 'a' ?><php echo 'c' ?>"
@@ -742,54 +742,62 @@ function checkPHPCode(&$phpfullcodestringold, &$phpfullcodestring)
 		$forbiddenphpmethods = array('invoke', 'invokeArgs');	// Method of ReflectionFunction to execute a function
 
 		foreach ($forbiddenphpstrings as $forbiddenphpstring) {
-			if (preg_match('/'.preg_quote($forbiddenphpstring, '/').'/ms', $phpfullcodestring)) {
+			if (preg_match('/'.preg_quote($forbiddenphpstring, '/').'/ims', $phpfullcodestring)) {
 				$error++;
 				setEventMessages($langs->trans("DynamicPHPCodeContainsAForbiddenInstruction", $forbiddenphpstring), null, 'errors');
 				break;
 			}
 		}
-		foreach ($forbiddenphpfunctions as $forbiddenphpcommand) {
-			if (preg_match('/'.$forbiddenphpcommand.'\s*\(/ms', $phpfullcodestring)) {
+		foreach ($forbiddenphpfunctions as $forbiddenphpfunction) {
+			if (preg_match('/'.$forbiddenphpfunction.'\s*\(/ims', $phpfullcodestring)) {
 				$error++;
-				setEventMessages($langs->trans("DynamicPHPCodeContainsAForbiddenInstruction", $forbiddenphpcommand), null, 'errors');
+				setEventMessages($langs->trans("DynamicPHPCodeContainsAForbiddenInstruction", $forbiddenphpfunction), null, 'errors');
 				break;
 			}
 		}
 		foreach ($forbiddenphpmethods as $forbiddenphpmethod) {
-			if (preg_match('/->'.$forbiddenphpmethod.'/ms', $phpfullcodestring)) {
+			if (preg_match('/->'.$forbiddenphpmethod.'/ims', $phpfullcodestring)) {
 				$error++;
 				setEventMessages($langs->trans("DynamicPHPCodeContainsAForbiddenInstruction", $forbiddenphpmethod), null, 'errors');
 				break;
 			}
 		}
 	}
 
-	// This char can be used to execute RCE for example using with echo `ls`
+	// This char can be used to execute RCE for example by using  echo `ls`
 	if (!$error) {
 		$forbiddenphpchars = array();
 		if (!getDolGlobalString('WEBSITE_PHP_ALLOW_DANGEROUS_CHARS')) {    // If option is not on, we disallow functions to execute commands
 			$forbiddenphpchars = array("`");
 		}
 		foreach ($forbiddenphpchars as $forbiddenphpchar) {
-			if (preg_match('/'.$forbiddenphpchar.'/ms', $phpfullcodestring)) {
+			if (preg_match('/'.$forbiddenphpchar.'/ims', $phpfullcodestring)) {
 				$error++;
 				setEventMessages($langs->trans("DynamicPHPCodeContainsAForbiddenInstruction", $forbiddenphpchar), null, 'errors');
 				break;
 			}
 		}
 	}
 
-	// Deny dynamic functions '${a}('  or  '$a[b]('  => So we refuse '}('  and  ']('
+	// Deny code to call a function obfuscated with comment, like  "exec/*...*/ ('ls')";
 	if (!$error) {
-		if (preg_match('/[}\]]\(/ims', $phpfullcodestring)) {
+		if (preg_match('/\*\/\s*\(/ims', $phpfullcodestring)) {
+				$error++;
+				setEventMessages($langs->trans("DynamicPHPCodeContainsAForbiddenInstruction", $forbiddenphpmethod), null, 'errors');
+		}
+	}
+
+	// Deny dynamic functions  '${a}('  or  '$a[b]('  => So we refuse '}('  and  ']('
+	if (!$error) {
+		if (preg_match('/[}\]]\s*\(/ims', $phpfullcodestring)) {
 			$error++;
 			setEventMessages($langs->trans("DynamicPHPCodeContainsAForbiddenInstruction", ']('), null, 'errors');
 		}
 	}
 
-	// Deny dynamic functions '$xxx('
+	// Deny dynamic functions '$xxx(' or '$xxx ('  or '$xxx" ('
 	if (!$error) {
-		if (preg_match('/\$[a-z0-9_\-\/\*]+\(/ims', $phpfullcodestring)) {
+		if (preg_match('/\$[a-z0-9_\-\/\*\"]+\s*\(/ims', $phpfullcodestring)) {
 			$error++;
 			setEventMessages($langs->trans("DynamicPHPCodeContainsAForbiddenInstruction", '$...('), null, 'errors');
 		}

diff --git a/test/phpunit/WebsiteTest.php b/test/phpunit/WebsiteTest.php
@@ -65,11 +65,11 @@
 	print "Load permissions for admin user nb 1\n";
 	$user->fetch(1);
 	$user->loadRights();
-
-	if (empty($user->rights->website)) {
-		$user->rights->website = new stdClass();
-	}
 }
+if (empty($user->rights->website)) {
+	$user->rights->website = new stdClass();
+}
+
 $conf->global->MAIN_DISABLE_ALL_MAILS = 1;
 
 
@@ -143,6 +143,18 @@ public function testCheckPHPCode()
 		print __METHOD__." result checkPHPCode=".$result."\n";
 		$this->assertEquals($result, 1, 'checkPHPCode did not detect the string was dangerous');
 
+		$t = '';
+		$s = '<?php eXec  ("eee"); ?>';
+		$result = checkPHPCode($t, $s);
+		print __METHOD__." result checkPHPCode=".$result."\n";
+		$this->assertEquals($result, 1, 'checkPHPCode did not detect the string was dangerous');
+
+		$t = '';
+		$s = '<?php $a="xec"; "e$a" ("ee"); ?>';
+		$result = checkPHPCode($t, $s);
+		print __METHOD__." result checkPHPCode=".$result."\n";
+		$this->assertEquals($result, 1, 'checkPHPCode did not detect the string was dangerous');
+
 		$t = '';
 		$s = '<?php $_="{"; $_=($_^"<").($_^">;").($_^"/"); ?><?=${\'_\'.$_}["_"](${\'_\'.$_}["__"]);?>';
 		$result = checkPHPCode($t, $s);