Skip to content

Commit 2b60a7a

Browse files
author
Marius Burkard
committed
WIP: change system exec calls to safe variant
1 parent 2d6d9eb commit 2b60a7a

22 files changed

+246
-130
lines changed

‎interface/lib/app.inc.php‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -78,7 +78,7 @@ public function __get($prop) {
7878

7979
$this->uses($prop);
8080
if(property_exists($this, $prop)) return $this->{$prop};
81-
else return null;
81+
else trigger_error('Undefined property ' . $name . ' of class app', E_USER_WARNING);
8282
}
8383

8484
public function __destruct() {

‎interface/lib/classes/functions.inc.php‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -451,9 +451,9 @@ public function generate_ssh_key($client_id, $username = ''){
451451
if(file_exists($id_rsa_file)) unset($id_rsa_file);
452452
if(file_exists($id_rsa_pub_file)) unset($id_rsa_pub_file);
453453
if(!file_exists($id_rsa_file) && !file_exists($id_rsa_pub_file)) {
454-
exec('ssh-keygen -t rsa -C '.$username.'-rsa-key-'.time().' -f '.$id_rsa_file.' -N ""');
454+
$app->system->exec_safe('ssh-keygen -t rsa -C ? -f ? -N ""', $username.'-rsa-key-'.time(), $id_rsa_file);
455455
$app->db->query("UPDATE client SET created_at = UNIX_TIMESTAMP(), id_rsa = ?, ssh_rsa = ? WHERE client_id = ?", @file_get_contents($id_rsa_file), @file_get_contents($id_rsa_pub_file), $client_id);
456-
exec('rm -f '.$id_rsa_file.' '.$id_rsa_pub_file);
456+
$app->system->exec_safe('rm -f ? ?', $id_rsa_file, $id_rsa_pub_file);
457457
} else {
458458
$app->log("Failed to create SSH keypair for ".$username, LOGLEVEL_WARN);
459459
}

‎interface/lib/classes/system.inc.php‎

Lines changed: 40 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -31,6 +31,8 @@
3131
class system {
3232

3333
var $client_service = null;
34+
private $_last_exec_out = null;
35+
private $_last_exec_retcode = null;
3436

3537
public function has_service($userid, $service) {
3638
global $app;
@@ -52,8 +54,43 @@ public function has_service($userid, $service) {
5254
return false;
5355
}
5456
}
55-
} //* End Class
56-
57-
?>
5857

58+
public function last_exec_out() {
59+
return $this->_last_exec_out;
60+
}
61+
62+
public function last_exec_retcode() {
63+
return $this->_last_exec_retcode;
64+
}
65+
66+
public function exec_safe($cmd) {
67+
$arg_count = func_num_args();
68+
if($arg_count > 1) {
69+
$args = func_get_args();
5970

71+
$pos = 0;
72+
$a = 0;
73+
foreach($args as $value) {
74+
$a++;
75+
76+
$pos = strpos($cmd, '?', $pos);
77+
if($pos === false) {
78+
break;
79+
}
80+
$value = escapeshellarg($value);
81+
$cmd = substr_replace($cmd, $value, $pos, 1);
82+
$pos += strlen($value);
83+
}
84+
}
85+
86+
$this->_last_exec_out = null;
87+
$this->_last_exec_retcode = null;
88+
return exec($cmd, $this->_last_exec_out, $this->_last_exec_retcode);
89+
}
90+
91+
public function system_safe($cmd) {
92+
call_user_func_array(array($this, 'exec_safe'), func_get_args());
93+
return implode("\n", $this->_last_exec_out);
94+
}
95+
96+
} //* End Class

‎interface/lib/classes/validate_dkim.inc.php‎

Lines changed: 4 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -49,10 +49,13 @@ function get_error($errmsg) {
4949
* Validator function for private DKIM-Key
5050
*/
5151
function check_private_key($field_name, $field_value, $validator) {
52+
global $app;
53+
5254
$dkim_enabled=$_POST['dkim'];
5355
if ($dkim_enabled == 'y') {
5456
if (empty($field_value)) return $this->get_error($validator['errmsg']);
55-
exec('echo '.escapeshellarg($field_value).'|openssl rsa -check', $output, $result);
57+
$app->system->exec_safe('echo ?|openssl rsa -check', $field_value);
58+
$result = $app->system->last_exec_retcode();
5659
if($result != 0) return $this->get_error($validator['errmsg']);
5760
}
5861
}

‎interface/web/mail/ajax_get_json.php‎

Lines changed: 6 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -54,8 +54,8 @@
5454
if ($dkim_strength=='') $dkim_strength = 2048;
5555

5656
$rnd_val = $dkim_strength * 10;
57-
exec('openssl rand -out ../../temp/random-data.bin '.$rnd_val.' 2> /dev/null', $output, $result);
58-
exec('openssl genrsa -rand ../../temp/random-data.bin '.$dkim_strength.' 2> /dev/null', $privkey, $result);
57+
$app->system->exec_safe('openssl rand -out ../../temp/random-data.bin '.$rnd_val.' 2> /dev/null', $output, $result);
58+
$app->system->exec_safe('openssl genrsa -rand ../../temp/random-data.bin '.$dkim_strength.' 2> /dev/null', $privkey, $result);
5959
unlink("../../temp/random-data.bin");
6060
$dkim_private='';
6161
foreach($privkey as $values) $dkim_private=$dkim_private.$values."\n";
@@ -79,12 +79,14 @@
7979
$selector = 'invalid domain or selector';
8080
}
8181
unset($dkim_public);
82-
exec('echo '.escapeshellarg($dkim_private).'|openssl rsa -pubout -outform PEM 2> /dev/null',$pubkey,$result);
82+
$app->system->exec_safe('echo ?|openssl rsa -pubout -outform PEM 2> /dev/null', $dkim_private);
83+
$pubkey = $app->system->last_exec_out();
8384
foreach($pubkey as $values) $dkim_public=$dkim_public.$values."\n";
8485
$selector = $dkim_selector;
8586
} else {
8687
unset($dkim_public);
87-
exec('echo '.escapeshellarg($dkim_private).'|openssl rsa -pubout -outform PEM 2> /dev/null',$pubkey,$result);
88+
$app->system->exec_safe('echo ?|openssl rsa -pubout -outform PEM 2> /dev/null', $dkim_private);
89+
$pubkey = $app->system->last_exec_out();
8890
foreach($pubkey as $values) $dkim_public=$dkim_public.$values."\n";
8991
$selector = $dkim_selector;
9092
}

‎server/lib/app.inc.php‎

Lines changed: 16 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -69,6 +69,22 @@ function __construct() {
6969

7070
}
7171

72+
public function __get($name) {
73+
$valid_names = array('functions', 'getconf', 'letsencrypt', 'modules', 'plugins', 'services', 'system');
74+
if(!in_array($name, $valid_names)) {
75+
trigger_error('Undefined property ' . $name . ' of class app', E_USER_WARNING);
76+
}
77+
if(property_exists($this, $name)) {
78+
return $this->{$name};
79+
}
80+
$this->uses($name);
81+
if(property_exists($this, $name)) {
82+
return $this->{$name};
83+
} else {
84+
trigger_error('Undefined property ' . $name . ' of class app', E_USER_WARNING);
85+
}
86+
}
87+
7288
function setCaller($caller) {
7389
$this->_calling_script = $caller;
7490
}

‎server/lib/classes/aps_installer.inc.php‎

Lines changed: 11 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -395,7 +395,7 @@ private function prepareFiles($task, $sxe)
395395
mkdir($this->document_root, 0777, true);
396396
}
397397
} else {
398-
exec("rm -Rf ".escapeshellarg($this->local_installpath).'*');
398+
$app->system->exec_safe("rm -Rf ?*", $this->local_installpath);
399399
}
400400
} else {
401401
mkdir($this->local_installpath, 0777, true);
@@ -412,7 +412,7 @@ private function prepareFiles($task, $sxe)
412412
|| ($this->extractZip($this->packages_dir.'/'.$task['path'], 'scripts', $this->local_installpath.'install_scripts/') === false) )
413413
{
414414
// Clean already extracted data
415-
exec("rm -Rf ".escapeshellarg($this->local_installpath).'*');
415+
$app->system->exec_safe("rm -Rf ?*", $this->local_installpath);
416416
throw new Exception('Unable to extract the package '.$task['path']);
417417
}
418418

@@ -423,11 +423,11 @@ private function prepareFiles($task, $sxe)
423423
$owner_res = $app->db->queryOneRecord("SELECT system_user, system_group FROM web_domain WHERE domain = ?", $main_domain['value']);
424424
$this->file_owner_user = $owner_res['system_user'];
425425
$this->file_owner_group = $owner_res['system_group'];
426-
exec('chown -R '.$this->file_owner_user.':'.$this->file_owner_group.' '.escapeshellarg($this->local_installpath));
426+
$app->system->exec_safe('chown -R ?:? ?', $this->file_owner_user, $this->file_owner_group, $this->local_installpath);
427427

428428
//* Chown stats directory back
429429
if(is_dir($this->local_installpath.'stats')) {
430-
exec('chown -R root:root '.escapeshellarg($this->local_installpath.'stats'));
430+
$app->system->exec_safe('chown -R root:root ?', $this->local_installpath.'stats');
431431
}
432432
}
433433
}
@@ -554,7 +554,9 @@ private function doInstallation($task, $sxe)
554554

555555
$shell_retcode = true;
556556
$shell_ret = array();
557-
exec('php '.escapeshellarg($this->local_installpath.'install_scripts/'.$cfgscript).' install 2>&1', $shell_ret, $shell_retcode);
557+
$app->system->exec_safe('php ? install 2>&1', $this->local_installpath.'install_scripts/'.$cfgscript);
558+
$shell_ret = $app->system->last_exec_out();
559+
$shell_retcode = $app->system->last_exec_retcode();
558560
$shell_ret = array_filter($shell_ret);
559561
$shell_ret_str = implode("\n", $shell_ret);
560562

@@ -566,11 +568,11 @@ private function doInstallation($task, $sxe)
566568
else
567569
{
568570
// The install succeeded, chown newly created files too
569-
exec('chown -R '.$this->file_owner_user.':'.$this->file_owner_group.' '.escapeshellarg($this->local_installpath));
571+
$app->system->exec_safe('chown -R ?:? ?', $this->file_owner_user, $this->file_owner_group, $this->local_installpath);
570572

571573
//* Chown stats directory back
572574
if(is_dir($this->local_installpath.'stats')) {
573-
exec('chown -R root:root '.escapeshellarg($this->local_installpath.'stats'));
575+
$app->system->exec_safe('chown -R root:root ?', $this->local_installpath.'stats');
574576
}
575577

576578
$app->dbmaster->query('UPDATE aps_instances SET instance_status = ? WHERE id = ?', INSTANCE_SUCCESS, $task['instance_id']);
@@ -597,8 +599,9 @@ private function doInstallation($task, $sxe)
597599
*/
598600
private function cleanup($task, $sxe)
599601
{
602+
global $app;
600603
chdir($this->local_installpath);
601-
exec("rm -Rf ".escapeshellarg($this->local_installpath).'install_scripts');
604+
$app->system->exec_safe("rm -Rf ?", $this->local_installpath.'install_scripts');
602605
}
603606

604607

‎server/lib/classes/cron.d/100-monitor_email_quota.inc.php‎

Lines changed: 3 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -90,7 +90,7 @@ public function onRunJob() {
9090
$email_parts = explode('@', $mb['email']);
9191
$filename = $mb['maildir'].'/.quotausage';
9292
if(!file_exists($filename) && $dovecot) {
93-
exec('doveadm quota recalc -u '.$email);
93+
$app->system->exec_safe('doveadm quota recalc -u ?', $email);
9494
}
9595
if(file_exists($filename) && !is_link($filename)) {
9696
$quotafile = file($filename);
@@ -99,7 +99,8 @@ public function onRunJob() {
9999
$app->log("Mail storage $email: " . $storage_value[1], LOGLEVEL_DEBUG);
100100
unset($quotafile);
101101
} else {
102-
exec('du -s '.escapeshellcmd($mb['maildir']), $out);
102+
$app->system->exec_safe('du -s ?', $mb['maildir']);
103+
$out = $app->system->last_exec_out();
103104
$parts = explode(' ', $out[0]);
104105
$data[$email]['used'] = intval($parts[0])*1024;
105106
unset($out);

‎server/lib/classes/cron.d/150-awstats.inc.php‎

Lines changed: 8 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -71,16 +71,16 @@ public function onRunJob() {
7171
$log_folder .= '/' . $subdomain_host;
7272
unset($tmp);
7373
}
74-
$logfile = escapeshellcmd($rec['document_root'].'/' . $log_folder . '/'.$yesterday.'-access.log');
74+
$logfile = $rec['document_root'].'/' . $log_folder . '/'.$yesterday.'-access.log';
7575
if(!@is_file($logfile)) {
76-
$logfile = escapeshellcmd($rec['document_root'].'/' . $log_folder . '/'.$yesterday.'-access.log.gz');
76+
$logfile = $rec['document_root'].'/' . $log_folder . '/'.$yesterday.'-access.log.gz';
7777
if(!@is_file($logfile)) {
7878
continue;
7979
}
8080
}
8181
$web_folder = (($rec['type'] == 'vhostsubdomain' || $rec['type'] == 'vhostalias') ? $rec['web_folder'] : 'web');
82-
$domain = escapeshellcmd($rec['domain']);
83-
$statsdir = escapeshellcmd($rec['document_root'].'/'.$web_folder.'/stats');
82+
$domain = $rec['domain'];
83+
$statsdir = $rec['document_root'].'/'.$web_folder.'/stats';
8484
$awstats_pl = $web_config['awstats_pl'];
8585
$awstats_buildstaticpages_pl = $web_config['awstats_buildstaticpages_pl'];
8686

@@ -117,8 +117,8 @@ public function onRunJob() {
117117
}
118118

119119
if(!@is_dir($statsdir)) mkdir($statsdir);
120-
$username = escapeshellcmd($rec['system_user']);
121-
$groupname = escapeshellcmd($rec['system_group']);
120+
$username = $rec['system_user'];
121+
$groupname = $rec['system_group'];
122122
chown($statsdir, $username);
123123
chgrp($statsdir, $groupname);
124124
if(is_link('/var/log/ispconfig/httpd/'.$domain.'/yesterday-access.log')) unlink('/var/log/ispconfig/httpd/'.$domain.'/yesterday-access.log');
@@ -138,7 +138,7 @@ public function onRunJob() {
138138
// awstats_buildstaticpages.pl -update -config=mydomain.com -lang=en -dir=/var/www/domain.com/'.$web_folder.'/stats -awstatsprog=/path/to/awstats.pl
139139
// $command = "$awstats_buildstaticpages_pl -update -config='$domain' -lang=".$conf['language']." -dir='$statsdir' -awstatsprog='$awstats_pl'";
140140

141-
$command = "$awstats_buildstaticpages_pl -month='$awmonth' -year='$awyear' -update -config='$domain' -lang=".$conf['language']." -dir='$statsdir' -awstatsprog='$awstats_pl'";
141+
$command = escapeshellcmd($awstats_buildstaticpages_pl) . ' -month=' . escapeshellarg($awmonth) . ' -year=' . escapeshellarg($awyear) . ' -update -config=' . escapeshellarg($domain) . ' -lang=' . escapeshellarg($conf['language']) . ' -dir=' . escapeshellarg($statsdir) . ' -awstatsprog=' . escapeshellarg($awstats_pl);
142142

143143
if (date("d") == 2) {
144144
$awmonth = date("m")-1;
@@ -178,7 +178,7 @@ public function onRunJob() {
178178
chgrp($rec['document_root']."/".$web_folder."/stats/index.php", $rec['system_group']);
179179
}
180180

181-
exec('chown -R '.$username.':'.$groupname.' '.$statsdir);
181+
$app->system->exec_safe('chown -R ?:? ?', $username, $groupname, $statsdir);
182182
}
183183

184184

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

Lines changed: 7 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -102,11 +102,11 @@ function setConfigVar( $filename, $varName, $varValue, $append = 0 ) {
102102
}
103103
}
104104

105-
$domain = escapeshellcmd($rec['domain']);
106-
$statsdir = escapeshellcmd($rec['document_root'].'/'.(($rec['type'] == 'vhostsubdomain' || $rec['type'] == 'vhostalias') ? $rec['web_folder'] : 'web').'/stats');
105+
$domain = $rec['domain'];
106+
$statsdir = $rec['document_root'].'/'.(($rec['type'] == 'vhostsubdomain' || $rec['type'] == 'vhostalias') ? $rec['web_folder'] : 'web').'/stats';
107107
$webalizer = '/usr/bin/webalizer';
108108
$webalizer_conf_main = '/etc/webalizer/webalizer.conf';
109-
$webalizer_conf = escapeshellcmd($rec['document_root'].'/log/webalizer.conf');
109+
$webalizer_conf = $rec['document_root'].'/log/webalizer.conf';
110110

111111
if(is_file($statsdir.'/index.php')) unlink($statsdir.'/index.php');
112112

@@ -122,13 +122,13 @@ function setConfigVar( $filename, $varName, $varValue, $append = 0 ) {
122122

123123

124124
if(!@is_dir($statsdir)) mkdir($statsdir);
125-
$username = escapeshellcmd($rec['system_user']);
126-
$groupname = escapeshellcmd($rec['system_group']);
125+
$username = $rec['system_user'];
126+
$groupname = $rec['system_group'];
127127
chown($statsdir, $username);
128128
chgrp($statsdir, $groupname);
129-
exec("$webalizer -c $webalizer_conf -n $domain -s $domain -r $domain -q -T -p -o $statsdir $logfile");
129+
$app->system->exec_safe("$webalizer -c ? -n ? -s ? -r ? -q -T -p -o ? ?", $webalizer_conf, $domain, $domain, $domain, $statsdir, $logfile);
130130

131-
exec('chown -R '.$username.':'.$groupname.' '.$statsdir);
131+
exec('chown -R ?:? ?', $username, $groupname, $statsdir);
132132
}
133133

134134

0 commit comments

Comments
 (0)