security: support array arguments in shell execution functions (1.2.x backport)#6912
Conversation
There was a problem hiding this comment.
Pull request overview
Backport to Cacti 1.2.x that extends internal execution helpers to accept array-form arguments, aiming to reduce command-injection risk by escaping individual tokens rather than relying on pre-concatenated strings.
Changes:
__rrd_execute()now accepts an array$command_lineand escapes each element withcacti_escapeshellarg()before joining.exec_background()now accepts an array$argsand escapes each element withcacti_escapeshellarg()before joining.exec_background()also accepts an array$redirect_argsand joins elements with spaces.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| lib/rrd.php | Adds array input support for RRDtool command execution by escaping and joining tokens. |
| lib/poller.php | Adds array input support for background process arguments (escaped) and redirect arguments (joined). |
…e (1.2.x backport) - Accept array $args in exec_background(), auto-escape via cacti_escapeshellarg() - Accept array $command_line in __rrd_execute(), auto-escape via cacti_escapeshellarg() - Backward compatible: string arguments still work unchanged Addresses GHSA-8522-5p3m-754c (High) - Authenticated RCE via Host Variable Injection Signed-off-by: Thomas Vincent <thomasvincent@gmail.com>
b3fdce8 to
ebc9101
Compare
| $cmd = array_shift($command_line); | ||
| $command_line = $cmd . ' ' . implode(' ', array_map('cacti_escapeshellarg', $command_line)); |
There was a problem hiding this comment.
When $command_line is an array, array_shift() can return null (e.g., empty array), which would produce an invalid $command_line and later calls like substr()/logging/proc I/O will behave unpredictably. Consider validating that the array is non-empty and that the first element is a non-empty command token, and fail fast (return false and/or log an error) if not.
| $cmd = array_shift($command_line); | |
| $command_line = $cmd . ' ' . implode(' ', array_map('cacti_escapeshellarg', $command_line)); | |
| if (empty($command_line)) { | |
| cacti_log('RRDTool command execution failed: empty command array provided to __rrd_execute()', false, 'SYSTEM', POLLER_VERBOSITY_MEDIUM); | |
| return false; | |
| } | |
| $cmd = array_shift($command_line); | |
| if (!is_string($cmd) || $cmd === '') { | |
| cacti_log('RRDTool command execution failed: invalid command token provided to __rrd_execute()', false, 'SYSTEM', POLLER_VERBOSITY_MEDIUM); | |
| return false; | |
| } | |
| $escaped_args = array_map('cacti_escapeshellarg', $command_line); | |
| if (!empty($escaped_args)) { | |
| $command_line = $cmd . ' ' . implode(' ', $escaped_args); | |
| } else { | |
| $command_line = $cmd; | |
| } |
|
|
||
| if (is_array($command_line)) { | ||
| $cmd = array_shift($command_line); | ||
| $command_line = $cmd . ' ' . implode(' ', array_map('cacti_escapeshellarg', $command_line)); |
There was a problem hiding this comment.
In the array form, only the arguments after the first element are escaped. If the first element ($cmd) can contain whitespace/newlines (even accidentally), it defeats the goal of treating each array entry as an atomic token. Consider either escaping/quoting the first element too (in a way RRDtool accepts) or validating it against an allowed token pattern (no spaces/control chars).
| $command_line = $cmd . ' ' . implode(' ', array_map('cacti_escapeshellarg', $command_line)); | |
| $command_line = cacti_escapeshellarg($cmd) . ' ' . implode(' ', array_map('cacti_escapeshellarg', $command_line)); |
Summary
Surgical edits, +12/-0 lines. Pure additions, no existing code modified.
$argsinexec_background(), auto-escape each element viacacti_escapeshellarg()$command_linein__rrd_execute(), auto-escape each elementSecurity
GHSA-8522-5p3m-754c (High) - Authenticated RCE via Host Variable Injection
Test plan