Implement a more secure way to use exec, system and shell_exec, fixes #5355

Author: Marius Burkard

interface/lib/classes/system.inc.php

@@ -65,6 +65,10 @@ public function last_exec_retcode() {
 
 	public function exec_safe($cmd) {
 		$arg_count = func_num_args();
+		if($arg_count != substr_count($cmd, '?') + 1) {
+			trigger_error('Placeholder count not matching argument list.', E_USER_WARNING);
+			return false;
+		}
 		if($arg_count > 1) {
 			$args = func_get_args();
 

server/lib/classes/aps_installer.inc.php

@@ -544,7 +544,6 @@ private function doInstallation($task, $sxe)
 			chmod($this->local_installpath.'install_scripts/'.$cfgscript, 0755);
 
 			// Change to the install folder (import for the exec() below!)
-			//exec('chown -R '.$this->file_owner_user.':'.$this->file_owner_group.' '.escapeshellarg($this->local_installpath));
 			chdir($this->local_installpath.'install_scripts/');
 
 			// Set the enviroment variables

server/lib/classes/cron.d/150-webalizer.inc.php

@@ -94,9 +94,9 @@ function setConfigVar( $filename, $varName, $varValue, $append = 0 ) {
 				$log_folder .= '/' . $subdomain_host;
 				unset($tmp);
 			}
-			$logfile = escapeshellcmd($rec['document_root'].'/' . $log_folder . '/'.$yesterday.'-access.log');
+			$logfile = $rec['document_root'].'/' . $log_folder . '/'.$yesterday.'-access.log';
 			if(!@is_file($logfile)) {
-				$logfile = escapeshellcmd($rec['document_root'].'/' . $log_folder . '/'.$yesterday.'-access.log.gz');
+				$logfile = $rec['document_root'].'/' . $log_folder . '/'.$yesterday.'-access.log.gz';
 				if(!@is_file($logfile)) {
 					continue;
 				}

server/lib/classes/cron.d/200-logfiles.inc.php

@@ -147,7 +147,7 @@ public function onRunJob() {
 			}
 
 			// rotate and compress the error.log 
-			$error_logfile = escapeshellcmd($rec['document_root'].'/' . $log_folder . '/error.log');
+			$error_logfile = $rec['document_root'].'/' . $log_folder . '/error.log';
 			// rename older files (move up by one)
 			$num = $log_retention;
 			while($num >= 1) {
@@ -187,7 +187,7 @@ public function onRunJob() {
 		$ispconfig_logfiles = array('ispconfig.log', 'cron.log', 'auth.log');
 		foreach($ispconfig_logfiles as $ispconfig_logfile) {
 			$num = $max_syslog;
-			$ispconfig_logfile = escapeshellcmd($conf['ispconfig_log_dir'].'/'.$ispconfig_logfile);
+			$ispconfig_logfile = $conf['ispconfig_log_dir'].'/'.$ispconfig_logfile;
 			// rename older files (move up by one)
 			while($num >= 1) {
 				if(is_file($ispconfig_logfile . '.' . $num . '.gz')) rename($ispconfig_logfile . '.' . $num . '.gz', $ispconfig_logfile . '.' . ($num + 1) . '.gz');

server/lib/classes/cron.d/500-backup_mail.inc.php

@@ -69,9 +69,9 @@ public function onRunJob() {
 			$records = $app->db->queryAllRecords("SELECT * FROM mail_user WHERE server_id = ? AND maildir != ''", intval($conf['server_id']));
 			if(is_array($records) && $run_backups) {
 				if(!is_dir($backup_dir)) {
-					mkdir(escapeshellcmd($backup_dir), $backup_dir_permissions, true);
+					mkdir($backup_dir, $backup_dir_permissions, true);
 				} else {
-					chmod(escapeshellcmd($backup_dir), $backup_dir_permissions);
+					chmod($backup_dir, $backup_dir_permissions);
 				}
 				system('which pigz > /dev/null', $ret);
 				if($ret === 0) {
@@ -122,7 +122,7 @@ public function onRunJob() {
 						if ($rec['maildir_format'] == 'mdbox') {
 							if (empty($this->tmp_backup_dir)) $this->tmp_backup_dir = $rec['maildir'];
 							// Create temporary backup-mailbox
-							exec("su -c ?", 'dsync backup -u "'.$rec["email"].'" mdbox:' . $this->tmp_backup_dir . '/backup');
+							$app->system->exec_safe("su -c ?", 'dsync backup -u "'.$rec["email"].'" mdbox:' . $this->tmp_backup_dir . '/backup');
 
 							if($backup_mode == 'userzip') {
 								$mail_backup_file.='.zip';

server/lib/classes/system.inc.php

@@ -1601,22 +1601,20 @@ function maildirmake($maildir_path, $user = '', $subfolder = '', $group = '') {
 		$mail_config = $app->getconf->get_server_config($conf["server_id"], 'mail');
 
 		if($subfolder != '') {
-			$dir = escapeshellcmd($maildir_path.'/.'.$subfolder);
+			$dir = $maildir_path.'/.'.$subfolder;
 		} else {
-			$dir = escapeshellcmd($maildir_path);
+			$dir = $maildir_path;
 		}
 
 		if(!is_dir($dir)) mkdir($dir, 0700, true);
 
 		if($user != '' && $user != 'root' && $this->is_user($user)) {
-			$user = escapeshellcmd($user);
 			if(is_dir($dir)) $this->chown($dir, $user);
 
 			$chown_mdsub = true;
 		}
 
 		if($group != '' && $group != 'root' && $this->is_group($group)) {
-			$group = escapeshellcmd($group);
 			if(is_dir($dir)) $this->chgrp($dir, $group);
 
 			$chgrp_mdsub = true;
@@ -1638,7 +1636,7 @@ function maildirmake($maildir_path, $user = '', $subfolder = '', $group = '') {
 			// Courier
 			if($mail_config['pop3_imap_daemon'] == 'courier') {
 				if(!is_file($maildir_path.'/courierimapsubscribed')) {
-					$tmp_file = escapeshellcmd($maildir_path.'/courierimapsubscribed');
+					$tmp_file = $maildir_path.'/courierimapsubscribed';
 					touch($tmp_file);
 					chmod($tmp_file, 0744);
 					chown($tmp_file, 'vmail');
@@ -1650,7 +1648,7 @@ function maildirmake($maildir_path, $user = '', $subfolder = '', $group = '') {
 			// Dovecot
 			if($mail_config['pop3_imap_daemon'] == 'dovecot') {
 				if(!is_file($maildir_path.'/subscriptions')) {
-					$tmp_file = escapeshellcmd($maildir_path.'/subscriptions');
+					$tmp_file = $maildir_path.'/subscriptions';
 					touch($tmp_file);
 					chmod($tmp_file, 0744);
 					chown($tmp_file, 'vmail');
@@ -2059,6 +2057,10 @@ public function last_exec_retcode() {
 
 	public function exec_safe($cmd) {
 		$arg_count = func_num_args();
+		if($arg_count != substr_count($cmd, '?') + 1) {
+			trigger_error('Placeholder count not matching argument list.', E_USER_WARNING);
+			return false;
+		}
 		if($arg_count > 1) {
 			$args = func_get_args();
 

server/mods-available/remoteaction_core_module.inc.php

@@ -150,7 +150,7 @@ private function _execActions() {
 					$parts = explode(':', $action['action_param']);
 					$veid = intval($parts[0]);
 					$template_cache_dir = '/vz/template/cache/';
-					$template_name = escapeshellcmd($parts[1]);
+					$template_name = $parts[1];
 					if($veid > 0 && $template_name != '' && is_dir($template_cache_dir)) {
 						$command = "vzdump --suspend --compress --stdexcludes --dumpdir ? ?";
 						$app->system->exec_safe($command, $template_cache_dir, $veid);