Skip to content

Commit dd701e0

Browse files
somethingwithproofsomethingwithproof
authored
hardening: use cacti_escapeshellarg() in sql.php and ss_sql.php (Cacti#6793)
* hardening: use cacti_escapeshellarg() in sql.php and ss_sql.php Bare escapeshellarg() bypasses Cacti's internal escaping contract. Replace all three call sites in each script with cacti_escapeshellarg() so any future Cacti-level escaping hooks apply consistently. Update SqlScriptsTest to assert cacti_escapeshellarg() is present and that no bare escapeshellarg() calls (negative lookbehind) remain. Closes Cacti#6785 Signed-off-by: Thomas Vincent <thomasvincent@gmail.com> * fix: return 'U' on empty shell_exec output in scripts/sql.php Cacti data source scripts must return 'U' on error, never empty string. shell_exec() returns null on failure; the null-coalesce produced '' which Cacti treats differently from 'U'. Add ?: 'U' fallback and a test. Signed-off-by: Thomas Vincent <thomasvincent@gmail.com> * test: add ss_sql.php U-on-error assertion and fix interpolation guard regex Signed-off-by: Thomas Vincent <thomasvincent@gmail.com> --------- Signed-off-by: Thomas Vincent <thomasvincent@gmail.com>
1 parent 8c8a233 commit dd701e0

File tree

3 files changed

+81
-24
lines changed

3 files changed

+81
-24
lines changed

‎scripts/sql.php‎

Lines changed: 4 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -6,14 +6,15 @@
66

77
global $database_hostname, $database_username, $database_password;
88

9-
$cmd = 'mysqladmin -h ' . escapeshellarg($database_hostname) . ' -u ' . escapeshellarg($database_username);
9+
$cmd = 'mysqladmin -h ' . cacti_escapeshellarg($database_hostname) . ' -u ' . cacti_escapeshellarg($database_username);
1010

1111
if ($database_password != '') {
12-
$cmd .= ' -p' . escapeshellarg($database_password);
12+
$cmd .= ' -p' . cacti_escapeshellarg($database_password);
1313
}
1414

1515
$cmd .= " status | awk '{print \$6 }'";
1616

1717
$sql = shell_exec($cmd);
1818

19-
print trim($sql ?? '');
19+
// Cacti expects 'U' on error, not empty string or 0.
20+
print trim($sql ?? '') ?: 'U';

‎scripts/ss_sql.php‎

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

39-
$cmd = 'mysqladmin --host=' . escapeshellarg($database_hostname) . ' --user=' . escapeshellarg($database_username);
39+
$cmd = 'mysqladmin --host=' . cacti_escapeshellarg($database_hostname) . ' --user=' . cacti_escapeshellarg($database_username);
4040

4141
if ($database_password != '') {
42-
$cmd .= ' --password=' . escapeshellarg($database_password);
42+
$cmd .= ' --password=' . cacti_escapeshellarg($database_password);
4343
}
4444

4545
$cmd .= ' status';
@@ -53,5 +53,5 @@ function ss_sql() : string {
5353
$result = preg_replace('/Queries per second avg/', 'QPS', $result);
5454
$result = preg_replace('/Flush tables/', 'FlushTables', $result);
5555

56-
return trim($result);
56+
return trim($result) ?: 'U';
5757
}

‎tests/Unit/SqlScriptsTest.php‎

Lines changed: 74 additions & 18 deletions
Original file line numberDiff line numberDiff line change
@@ -13,13 +13,13 @@
1313
*/
1414

1515
/*
16-
* Tests for the backtick-to-shell_exec migration in scripts/sql.php
17-
* and scripts/ss_sql.php.
16+
* Tests for scripts/sql.php and scripts/ss_sql.php.
1817
*
19-
* PHP 8.4 deprecates the backtick operator. These scripts previously
20-
* used backticks to invoke mysqladmin, with unescaped variable
21-
* interpolation. The fix replaces backticks with shell_exec() and
22-
* wraps all user-supplied values in escapeshellarg().
18+
* Covers two hardening steps:
19+
* 1. Backtick-to-shell_exec migration (PHP 8.4 deprecates the backtick
20+
* operator; the fix replaces backticks with shell_exec()).
21+
* 2. cacti_escapeshellarg() wrapper (Cacti's internal contract; bare
22+
* escapeshellarg() bypasses any future Cacti-level escaping hooks).
2323
*/
2424

2525
$sqlPhpPath = __DIR__ . '/../../scripts/sql.php';
@@ -39,22 +39,29 @@
3939
expect($contents)->toContain('shell_exec(');
4040
});
4141

42-
test('sql.php escapes database_hostname with escapeshellarg', function () use ($sqlPhpPath) {
42+
test('sql.php escapes database_hostname with cacti_escapeshellarg', function () use ($sqlPhpPath) {
4343
$contents = file_get_contents($sqlPhpPath);
4444

45-
expect($contents)->toContain('escapeshellarg($database_hostname)');
45+
expect($contents)->toContain('cacti_escapeshellarg($database_hostname)');
4646
});
4747

48-
test('sql.php escapes database_username with escapeshellarg', function () use ($sqlPhpPath) {
48+
test('sql.php escapes database_username with cacti_escapeshellarg', function () use ($sqlPhpPath) {
4949
$contents = file_get_contents($sqlPhpPath);
5050

51-
expect($contents)->toContain('escapeshellarg($database_username)');
51+
expect($contents)->toContain('cacti_escapeshellarg($database_username)');
5252
});
5353

54-
test('sql.php escapes database_password with escapeshellarg', function () use ($sqlPhpPath) {
54+
test('sql.php escapes database_password with cacti_escapeshellarg', function () use ($sqlPhpPath) {
5555
$contents = file_get_contents($sqlPhpPath);
5656

57-
expect($contents)->toContain('escapeshellarg($database_password)');
57+
expect($contents)->toContain('cacti_escapeshellarg($database_password)');
58+
});
59+
60+
test('sql.php uses no bare escapeshellarg calls', function () use ($sqlPhpPath) {
61+
$contents = file_get_contents($sqlPhpPath);
62+
63+
// Negative lookbehind: match escapeshellarg( NOT preceded by cacti_
64+
expect(preg_match('/(?<!cacti_)escapeshellarg\(/', $contents))->toBe(0);
5865
});
5966

6067
test('sql.php handles null return from shell_exec', function () use ($sqlPhpPath) {
@@ -63,6 +70,13 @@
6370
expect($contents)->toContain("?? ''");
6471
});
6572

73+
test('sql.php returns U on empty/null shell_exec output', function () use ($sqlPhpPath) {
74+
$contents = file_get_contents($sqlPhpPath);
75+
76+
/* Cacti data source scripts must return 'U' on error, never empty string. */
77+
expect($contents)->toContain(": 'U'");
78+
});
79+
6680
// --- scripts/ss_sql.php: no backtick operators remain ---
6781

6882
test('ss_sql.php contains no backtick operators', function () use ($ssSqlPhpPath) {
@@ -77,22 +91,29 @@
7791
expect($contents)->toContain('shell_exec(');
7892
});
7993

80-
test('ss_sql.php escapes database_hostname with escapeshellarg', function () use ($ssSqlPhpPath) {
94+
test('ss_sql.php escapes database_hostname with cacti_escapeshellarg', function () use ($ssSqlPhpPath) {
95+
$contents = file_get_contents($ssSqlPhpPath);
96+
97+
expect($contents)->toContain('cacti_escapeshellarg($database_hostname)');
98+
});
99+
100+
test('ss_sql.php escapes database_username with cacti_escapeshellarg', function () use ($ssSqlPhpPath) {
81101
$contents = file_get_contents($ssSqlPhpPath);
82102

83-
expect($contents)->toContain('escapeshellarg($database_hostname)');
103+
expect($contents)->toContain('cacti_escapeshellarg($database_username)');
84104
});
85105

86-
test('ss_sql.php escapes database_username with escapeshellarg', function () use ($ssSqlPhpPath) {
106+
test('ss_sql.php escapes database_password with cacti_escapeshellarg', function () use ($ssSqlPhpPath) {
87107
$contents = file_get_contents($ssSqlPhpPath);
88108

89-
expect($contents)->toContain('escapeshellarg($database_username)');
109+
expect($contents)->toContain('cacti_escapeshellarg($database_password)');
90110
});
91111

92-
test('ss_sql.php escapes database_password with escapeshellarg', function () use ($ssSqlPhpPath) {
112+
test('ss_sql.php uses no bare escapeshellarg calls', function () use ($ssSqlPhpPath) {
93113
$contents = file_get_contents($ssSqlPhpPath);
94114

95-
expect($contents)->toContain('escapeshellarg($database_password)');
115+
// Negative lookbehind: match escapeshellarg( NOT preceded by cacti_
116+
expect(preg_match('/(?<!cacti_)escapeshellarg\(/', $contents))->toBe(0);
96117
});
97118

98119
test('ss_sql.php handles null return from shell_exec', function () use ($ssSqlPhpPath) {
@@ -101,6 +122,41 @@
101122
expect($contents)->toContain("?? ''");
102123
});
103124

125+
test('ss_sql.php returns U on empty/null shell_exec output', function () use ($ssSqlPhpPath) {
126+
$contents = file_get_contents($ssSqlPhpPath);
127+
128+
/* Cacti data source scripts must return 'U' on error, never empty string. */
129+
expect($contents)->toContain(": 'U'");
130+
});
131+
132+
// --- runtime: cacti_escapeshellarg is callable and ss_sql() falls back to 'U' ---
133+
134+
test('ss_sql() returns U when shell_exec produces no output', function () use ($ssSqlPhpPath) {
135+
/* Bootstrap cacti_escapeshellarg if global.php has not yet been loaded. */
136+
if (!function_exists('cacti_escapeshellarg')) {
137+
/* Minimal stub: delegate to the native call so arg-quoting still works. */
138+
function cacti_escapeshellarg(string $arg, bool $quote = true): string {
139+
return escapeshellarg($arg);
140+
}
141+
}
142+
143+
/* Provide dummy globals so the function can build its command string. */
144+
$GLOBALS['database_hostname'] = '127.0.0.1';
145+
$GLOBALS['database_username'] = 'cacti_test_no_such_user';
146+
$GLOBALS['database_password'] = '';
147+
148+
/* Include the script in "called by script server" mode so only the
149+
* function definition is loaded, not the top-level print statement. */
150+
$called_by_script_server = true;
151+
if (!function_exists('ss_sql')) {
152+
require $ssSqlPhpPath;
153+
}
154+
155+
/* mysqladmin will fail (bad credentials / no server), shell_exec returns
156+
* null or empty. ss_sql() must map that to 'U'. */
157+
expect(ss_sql())->toBe('U');
158+
});
159+
104160
// --- no raw variable interpolation in shell commands ---
105161

106162
test('sql.php does not interpolate variables directly in shell strings', function () use ($sqlPhpPath) {

0 commit comments

Comments
 (0)