Skip to content

Commit 4e9969e

Browse files
somethingwithproofTheWitness
somethingwithproof
and
TheWitness
authored
security: harden shell command execution against injection (1.2.x backport) (#6902)
* security: harden shell command execution against injection (1.2.x backport) - Apply cacti_escapeshellarg() to hostname in lib/ping.php - Escape PHP binary and script path in graph_realtime.php shell_exec - Escape host_id in host_reindex shell command - Replace shell_exec chown with PHP chown()/chgrp() with return value checks in rrd.php and boost.php - Escape db_dump_data exec arguments in lib/rrd.php - Use cacti_escapeshellarg() in scripts/sql.php and scripts/ss_sql.php - Parse mysqladmin output in PHP instead of piping through awk - Add unit tests for graph_realtime shell and SQL script hardening Addresses GHSA-xq98-376r-hv9j (Critical) Signed-off-by: Thomas Vincent <thomasvincent@gmail.com> * Remove type casting from host_id assignment * Fix local_graph_id retrieval in view case * Change local_graph_id retrieval method * Execute command using shell_exec in ss_sql.php Add shell_exec to execute command for database operations. --------- Signed-off-by: Thomas Vincent <thomasvincent@gmail.com> Co-authored-by: TheWitness <thewitness@cacti.net>
1 parent 1818a02 commit 4e9969e

File tree

8 files changed

+276
-32
lines changed

8 files changed

+276
-32
lines changed

‎graph_realtime.php‎

Lines changed: 10 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -210,18 +210,20 @@
210210
$graph_data_array['image_format'] = $gtype;
211211

212212
/* call poller */
213-
$graph_rrd = read_config_option('realtime_cache_path') . '/user_' . $hash . '_lgi_' . get_request_var('local_graph_id') . '.png';
214-
$command = read_config_option('path_php_binary');
215-
$args = sprintf('poller_realtime.php --graph=%s --interval=%d --poller_id=' . $hash, get_request_var('local_graph_id'), $graph_data_array['ds_step']);
213+
$local_graph_id = get_filter_request_var('local_graph_id');
214+
$graph_rrd = read_config_option('realtime_cache_path') . '/user_' . $hash . '_lgi_' . $local_graph_id . '.png';
215+
$php_binary = cacti_escapeshellcmd(read_config_option('path_php_binary'));
216+
$script_path = cacti_escapeshellarg($config['base_path'] . '/poller_realtime.php');
217+
$args = '--graph=' . $local_graph_id . ' --interval=' . $graph_data_array['ds_step'] . ' --poller_id=' . $hash;
216218

217-
shell_exec("$command $args");
219+
shell_exec($php_binary . ' -q ' . $script_path . ' ' . $args);
218220

219221
/* construct the image name */
220222
$graph_data_array['export_realtime'] = $graph_rrd;
221223
$graph_data_array['output_flag'] = RRDTOOL_OUTPUT_GRAPH_DATA;
222224
$null_param = array();
223225

224-
$output = rrdtool_function_graph(get_request_var('local_graph_id'), '', $graph_data_array, '', $null_param, $_SESSION['sess_user_id']);
226+
$output = rrdtool_function_graph($local_graph_id, '', $graph_data_array, '', $null_param, $_SESSION['sess_user_id']);
225227

226228
$error = '';
227229
if (file_exists($graph_rrd)) {
@@ -235,7 +237,7 @@
235237
if (empty($output) && empty($error)) {
236238
$graph_data_array['get_error'] = true;
237239
$null_param = array();
238-
rrdtool_function_graph(get_request_var('local_graph_id'), '', $graph_data_array, '', $null_param, $_SESSION['sess_user_id']);
240+
rrdtool_function_graph($local_graph_id, '', $graph_data_array, '', $null_param, $_SESSION['sess_user_id']);
239241

240242
$error = ob_get_contents();
241243

@@ -278,7 +280,7 @@
278280

279281
/* send text information back to browser as well as image information */
280282
$return_array = array(
281-
'local_graph_id' => get_request_var('local_graph_id'),
283+
'local_graph_id' => $local_graph_id,
282284
'top' => get_request_var('top'),
283285
'left' => get_request_var('left'),
284286
'ds_step' => html_escape(isset($_SESSION['sess_realtime_ds_step']) ? $_SESSION['sess_realtime_ds_step']:$graph_data_array['ds_step']),
@@ -294,7 +296,7 @@
294296
exit;
295297
break;
296298
case 'view':
297-
$graph_rrd = read_config_option('realtime_cache_path') . '/user_' . $hash . '_lgi_' . get_request_var('local_graph_id') . '.png';
299+
$graph_rrd = read_config_option('realtime_cache_path') . '/user_' . $hash . '_lgi_' . get_filter_request_var('local_graph_id') . '.png';
298300

299301
if (file_exists($graph_rrd)) {
300302
print base64_encode(file_get_contents($graph_rrd));

‎host.php‎

Lines changed: 3 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -191,7 +191,8 @@ function host_reindex() {
191191

192192
$start = microtime(true);
193193

194-
shell_exec(read_config_option('path_php_binary') . ' -q ' . $config['base_path'] . '/cli/poller_reindex_hosts.php --qid=all --id=' . get_filter_request_var('host_id'));
194+
$host_id = get_filter_request_var('host_id');
195+
shell_exec(cacti_escapeshellcmd(read_config_option('path_php_binary')) . ' -q ' . cacti_escapeshellarg($config['base_path'] . '/cli/poller_reindex_hosts.php') . ' --qid=all --id=' . $host_id);
195196

196197
$end = microtime(true);
197198

@@ -200,7 +201,7 @@ function host_reindex() {
200201
$items = db_fetch_cell_prepared('SELECT COUNT(*)
201202
FROM host_snmp_cache
202203
WHERE host_id = ?',
203-
array(get_filter_request_var('host_id')));
204+
array($host_id));
204205

205206
raise_message('host_reindex', __('Device Reindex Completed in %0.2f seconds. There were %d items updated.', $total_time, $items), MESSAGE_LEVEL_INFO);
206207
}

‎lib/boost.php‎

Lines changed: 7 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1430,7 +1430,13 @@ function boost_rrdtool_function_create($local_data_id, $show_source, &$rrdtool_p
14301430
$success = rrdtool_execute("create $data_source_path $create_ds$create_rra", false, RRDTOOL_OUTPUT_STDOUT, $rrdtool_pipe, 'BOOST');
14311431

14321432
if ($config['cacti_server_os'] != 'win32' && posix_getuid() == 0) {
1433-
shell_exec("chown $owner_id:$group_id $data_source_path");
1433+
if (!chown($data_source_path, (int) $owner_id)) {
1434+
cacti_log("WARNING: Unable to set owner for '" . $data_source_path . "'", false, 'BOOST');
1435+
}
1436+
1437+
if (!chgrp($data_source_path, (int) $group_id)) {
1438+
cacti_log("WARNING: Unable to set group for '" . $data_source_path . "'", false, 'BOOST');
1439+
}
14341440
}
14351441

14361442
return $success;

‎lib/ping.php‎

Lines changed: 12 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -164,34 +164,34 @@ function ping_icmp() {
164164
* The other fields are numerical fields only and thus
165165
* not vulnerable for command injection */
166166
if (substr_count(strtolower(PHP_OS), 'sun')) {
167-
$result = shell_exec('ping ' . $this->host['hostname']);
167+
$result = shell_exec('ping ' . cacti_escapeshellarg($this->host['hostname']));
168168
} elseif (substr_count(strtolower(PHP_OS), 'hpux')) {
169-
$result = shell_exec('ping -m ' . ceil($this->timeout/1000) . ' -n ' . $this->retries . ' ' . $this->host['hostname']);
169+
$result = shell_exec('ping -m ' . ceil($this->timeout/1000) . ' -n ' . $this->retries . ' ' . cacti_escapeshellarg($this->host['hostname']));
170170
} elseif (substr_count(strtolower(PHP_OS), 'mac')) {
171-
$result = shell_exec('ping -t ' . ceil($this->timeout/1000) . ' -c ' . $this->retries . ' ' . $this->host['hostname']);
171+
$result = shell_exec('ping -t ' . ceil($this->timeout/1000) . ' -c ' . $this->retries . ' ' . cacti_escapeshellarg($this->host['hostname']));
172172
} elseif (substr_count(strtolower(PHP_OS), 'freebsd')) {
173173
if (strpos($host_ip, ':') !== false) {
174-
$result = shell_exec('ping6 -t ' . ceil($this->timeout/1000) . ' -c ' . $this->retries . ' ' . $this->host['hostname']);
174+
$result = shell_exec('ping6 -t ' . ceil($this->timeout/1000) . ' -c ' . $this->retries . ' ' . cacti_escapeshellarg($this->host['hostname']));
175175
} else {
176-
$result = shell_exec('ping -t ' . ceil($this->timeout/1000) . ' -c ' . $this->retries . ' ' . $this->host['hostname']);
176+
$result = shell_exec('ping -t ' . ceil($this->timeout/1000) . ' -c ' . $this->retries . ' ' . cacti_escapeshellarg($this->host['hostname']));
177177
}
178178
} elseif (substr_count(strtolower(PHP_OS), 'darwin')) {
179-
$result = shell_exec('ping -t ' . ceil($this->timeout/1000) . ' -c ' . $this->retries . ' ' . $this->host['hostname']);
179+
$result = shell_exec('ping -t ' . ceil($this->timeout/1000) . ' -c ' . $this->retries . ' ' . cacti_escapeshellarg($this->host['hostname']));
180180
} elseif (substr_count(strtolower(PHP_OS), 'bsd')) {
181-
$result = shell_exec('ping -w ' . ceil($this->timeout/1000) . ' -c ' . $this->retries . ' ' . $this->host['hostname']);
181+
$result = shell_exec('ping -w ' . ceil($this->timeout/1000) . ' -c ' . $this->retries . ' ' . cacti_escapeshellarg($this->host['hostname']));
182182
} elseif (substr_count(strtolower(PHP_OS), 'aix')) {
183-
$result = shell_exec('ping -i ' . ceil($this->timeout/1000) . ' -c ' . $this->retries . ' ' . $this->host['hostname']);
183+
$result = shell_exec('ping -i ' . ceil($this->timeout/1000) . ' -c ' . $this->retries . ' ' . cacti_escapeshellarg($this->host['hostname']));
184184
} elseif (strtoupper(substr(PHP_OS, 0, 3)) === 'WIN') {
185-
$result = shell_exec('chcp 437 && ping -w ' . $this->timeout . ' -n ' . $this->retries . ' ' . $this->host['hostname']);
185+
$result = shell_exec('chcp 437 && ping -w ' . $this->timeout . ' -n ' . $this->retries . ' ' . cacti_escapeshellarg($this->host['hostname']));
186186
} else {
187187
/* please know, that when running SELinux, httpd will throw
188188
* ping: cap_set_proc: Permission denied
189189
* as it now tries to open an ICMP socket and fails
190190
* $result will be empty, then. */
191191
if (strpos($host_ip, ':') !== false) {
192-
$result = shell_exec('ping -6 -W ' . ceil($this->timeout/1000) . ' -c ' . $this->retries . ' -p ' . $pattern . ' ' . $this->host['hostname']);
192+
$result = shell_exec('ping -6 -W ' . ceil($this->timeout/1000) . ' -c ' . $this->retries . ' -p ' . $pattern . ' ' . cacti_escapeshellarg($this->host['hostname']));
193193
} else {
194-
$result = shell_exec('ping -W ' . ceil($this->timeout/1000) . ' -c ' . $this->retries . ' -p ' . $pattern . ' ' . $this->host['hostname'] . ' 2>&1');
194+
$result = shell_exec('ping -W ' . ceil($this->timeout/1000) . ' -c ' . $this->retries . ' -p ' . $pattern . ' ' . cacti_escapeshellarg($this->host['hostname']) . ' 2>&1');
195195

196196
if (strpos($result, 'unknown host') !== false || strpos($result, 'Address family') !== false) {
197197
if (file_exists('/usr/bin/ping6')) {
@@ -202,7 +202,7 @@ function ping_icmp() {
202202
$ping_path = '/bin/ping6';
203203
}
204204

205-
$result = shell_exec($ping_path . ' -W ' . ceil($this->timeout/1000) . ' -c ' . $this->retries . ' -p ' . $pattern . ' ' . $this->host['hostname']);
205+
$result = shell_exec($ping_path . ' -W ' . ceil($this->timeout/1000) . ' -c ' . $this->retries . ' -p ' . $pattern . ' ' . cacti_escapeshellarg($this->host['hostname']));
206206
}
207207
}
208208
}

‎scripts/sql.php‎

Lines changed: 16 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -4,11 +4,22 @@
44

55
include(dirname(__FILE__) . '/../include/cli_check.php');
66

7-
if ($database_password == '') {
8-
$sql = `mysqladmin -h $database_hostname -u $database_username status | awk '{print $6 }'`;
9-
} else {
10-
$sql = `mysqladmin -h $database_hostname -u $database_username -p$database_password status | awk '{print $6 }'`;
7+
global $database_hostname, $database_username, $database_password;
8+
9+
$cmd = 'mysqladmin -h ' . cacti_escapeshellarg($database_hostname) . ' -u ' . cacti_escapeshellarg($database_username);
10+
11+
if ($database_password != '') {
12+
$cmd .= ' -p' . cacti_escapeshellarg($database_password);
1113
}
1214

13-
print trim($sql);
15+
$cmd .= ' status';
1416

17+
$output = shell_exec($cmd);
18+
19+
if ($output === null || $output === '') {
20+
print 'U';
21+
} else {
22+
// Extract the 6th field (Queries per second avg), matching original awk '{print $6}'
23+
$parts = preg_split('/\s+/', trim($output));
24+
print isset($parts[5]) ? $parts[5] : 'U';
25+
}

‎scripts/ss_sql.php‎

Lines changed: 6 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -36,19 +36,21 @@ function ss_sql() {
3636
global $database_password;
3737
global $database_hostname;
3838

39+
$cmd = 'mysqladmin --host=' . cacti_escapeshellarg($database_hostname) . ' --user=' . cacti_escapeshellarg($database_username);
40+
3941
if ($database_password != '') {
40-
$result = `mysqladmin --host=$database_hostname --user=$database_username --password=$database_password status`;
41-
} else {
42-
$result = `mysqladmin --host=$database_hostname --user=$database_username status`;
42+
$cmd .= ' --password=' . cacti_escapeshellarg($database_password);
4343
}
4444

45+
$result = shell_exec($cmd);
46+
4547
$result = preg_replace('/: /', ':', $result);
4648
$result = preg_replace('/ /', ' ', $result);
4749
$result = preg_replace('/Slow queries/', 'SlowQueries', $result);
4850
$result = preg_replace('/Open tables/', 'OpenTables', $result);
4951
$result = preg_replace('/Queries per second avg/', 'QPS', $result);
5052
$result = preg_replace('/Flush tables/', 'FlushTables', $result);
5153

52-
return trim($result);
54+
return trim($result) ?: 'U';
5355
}
5456

‎tests/Unit/GraphRealtimeShellTest.php‎

Lines changed: 50 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,50 @@
1+
<?php
2+
/*
3+
+-------------------------------------------------------------------------+
4+
| Copyright (C) 2004-2026 The Cacti Group |
5+
| |
6+
| This program is free software; you can redistribute it and/or |
7+
| modify it under the terms of the GNU General Public License |
8+
| as published by the Free Software Foundation; either version 2 |
9+
| of the License, or (at your option) any later version. |
10+
+-------------------------------------------------------------------------+
11+
| Cacti: The Complete RRDtool-based Graphing Solution |
12+
+-------------------------------------------------------------------------+
13+
*/
14+
15+
/*
16+
* Tests for command injection hardening in graph_realtime.php.
17+
*
18+
* grv('local_graph_id') was interpolated into shell_exec via sprintf without
19+
* escaping. The fix casts to (int), uses cacti_escapeshellcmd for the PHP
20+
* binary, and cacti_escapeshellarg for the script path.
21+
*/
22+
23+
$graphRealtimePath = __DIR__ . '/../../graph_realtime.php';
24+
25+
// --- graph_realtime.php: shell escaping for poller invocation ---
26+
27+
test('graph_realtime.php uses cacti_escapeshellcmd for PHP binary', function () use ($graphRealtimePath) {
28+
$contents = file_get_contents($graphRealtimePath);
29+
30+
expect($contents)->toContain("cacti_escapeshellcmd(read_config_option('path_php_binary')");
31+
});
32+
33+
test('graph_realtime.php uses cacti_escapeshellarg for poller_realtime script path', function () use ($graphRealtimePath) {
34+
$contents = file_get_contents($graphRealtimePath);
35+
36+
expect($contents)->toContain('cacti_escapeshellarg(CACTI_PATH_BASE');
37+
expect($contents)->toContain('poller_realtime.php');
38+
});
39+
40+
test('graph_realtime.php casts local_graph_id to int before shell_exec', function () use ($graphRealtimePath) {
41+
$contents = file_get_contents($graphRealtimePath);
42+
43+
expect($contents)->toMatch('/\(int\)\s+gfrv\s*\(\s*[\'"]local_graph_id[\'"]\s*\)/');
44+
});
45+
46+
test('graph_realtime.php does not pass raw grv local_graph_id to sprintf for shell', function () use ($graphRealtimePath) {
47+
$contents = file_get_contents($graphRealtimePath);
48+
49+
expect($contents)->not->toMatch('/sprintf\s*\([^)]*grv\s*\(\s*[\'"]local_graph_id[\'"]\s*\)/');
50+
});

0 commit comments

Comments
 (0)