hardening: remove /bin/sh fallback from alert command execution by somethingwithproof · Pull Request #275 · Cacti/plugin_syslog

somethingwithproof · 2026-03-07T02:36:09Z

Summary

remove /bin/sh fallback execution path for alert commands
execute commands only when the first token resolves to an executable file
log explicit errors and return code 126 for non-executable command targets
add regression test ensuring shell fallback is not reintroduced
update changelog with issue hardening: remove /bin/sh fallback from alert command execution path #257

Fixes #257

Validation

php -l functions.php
php -l tests/regression/issue257_remove_shell_fallback_test.php
php tests/regression/issue257_remove_shell_fallback_test.php

Fixes Cacti#257 Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

Copilot

Pull request overview

Hardens Syslog plugin alert command execution by removing the explicit /bin/sh fallback path and adding a regression guard to prevent it from returning, with an accompanying changelog entry for issue #257.

Changes:

Updated syslog_process_alert() to only execute alert commands when the first token is an executable; otherwise log an error and return code 126.
Added a regression test that scans functions.php to ensure the /bin/sh fallback is not reintroduced.
Added a changelog entry referencing issue #257.

Reviewed changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 6 comments.

File	Description
`functions.php`	Removes explicit `/bin/sh` fallback; adds executability gate, error logging, and return code `126` for invalid targets.
`tests/regression/issue257_remove_shell_fallback_test.php`	Adds regression coverage to detect reintroduction of the `/bin/sh` fallback and related logic changes.
`CHANGELOG.md`	Documents the hardening change under `develop`.

Copilot · 2026-03-07T02:41:03Z

functions.php

+						if (cacti_sizeof($cparts) && is_executable($cparts[0])) {
 							exec($command, $output, $returnCode);
 						} else {
-							exec('/bin/sh ' . $command, $output, $returnCode);
+							$returnCode = 126;
+							cacti_log("SYSLOG ERROR: Alert command is not executable: '$command'", false, 'SYSTEM');


Tokenizing the command with a whitespace split and then checking is_executable($cparts[0]) will fail for valid commands where the executable is quoted (e.g. "/path/to/script" ...) or contains spaces. If quoted executables are expected to work, parse the first token with shell-like quoting rules (or at least strip surrounding quotes before calling is_executable) so the executability check matches what exec() will actually run.

Fixed — trim($cparts[0], '""') before is_executable() check.

Copilot · 2026-03-07T02:41:04Z

functions.php

 						} else {
-							exec('/bin/sh ' . $command, $output, $returnCode);
+							$returnCode = 126;
+							cacti_log("SYSLOG ERROR: Alert command is not executable: '$command'", false, 'SYSTEM');


The new error message "Alert command is not executable" can also be hit when the command relies on PATH lookup (e.g. "php ...") because is_executable() requires a filesystem path. If PATH-based commands are intentionally disallowed, make the error message explicit (e.g. require absolute path / full executable path) so users know how to fix their configuration.

Suggested change

cacti_log("SYSLOG ERROR: Alert command is not executable: '$command'", false, 'SYSTEM');

$executable = cacti_sizeof($cparts) ? $cparts[0] : '';

// Make it explicit that PATH-based commands are not supported

if ($executable !== '' && strpos($executable, DIRECTORY_SEPARATOR) === false) {

$reason = 'The executable must be specified using an absolute or full filesystem path; PATH-based lookups are not supported.';

} else {

$reason = 'The specified executable file is not found or is not marked as executable.';

}

cacti_log("SYSLOG ERROR: Alert command is not executable: '$command'. " . $reason, false, 'SYSTEM');

Copilot · 2026-03-07T02:41:04Z

functions.php

 						$logMessage = "SYSLOG NOTICE: Executing '$command'";

-						$cparts = explode(' ', $command);
+						$cparts     = preg_split('/\s+/', trim($command));
+						$output     = array();
+						$returnCode = 0;


The notice log message is constructed as "Executing '$command'" before validating executability, but in the non-executable branch the command is never executed. This results in a misleading log line that still claims execution (with return code 126). Consider only logging "Executing" after the executable check passes, or change the message in the else-path to indicate the command was skipped/invalid.

Fixed — log moved inside the is_executable branch.

Copilot · 2026-03-07T02:41:04Z

functions.php

+						$cparts     = preg_split('/\s+/', trim($command));
+						$output     = array();
+						$returnCode = 0;

-						if (is_executable($cparts[0])) {
+						if (cacti_sizeof($cparts) && is_executable($cparts[0])) {


Whitespace tokenization + is_executable($cparts[0]) will reject quoted executables or paths with spaces (e.g. "/path/to/script" ...). If those are expected to work, parse the first token with quoting rules (or at least strip wrapping quotes) before calling is_executable so legitimate commands aren't blocked.

Copilot · 2026-03-07T02:41:04Z

tests/regression/issue257_remove_shell_fallback_test.php

+if (strpos($functions, "exec('/bin/sh ' . \$command") !== false) {
+	fwrite(STDERR, "Shell fallback execution path is still present.\n");
+	exit(1);
+}
+
+if (substr_count($functions, "preg_split('/\\s+/', trim(\$command))") < 2) {
+	fwrite(STDERR, "Expected command tokenization update is missing.\n");
+	exit(1);
+}
+
+if (substr_count($functions, '$returnCode = 126;') < 2) {
+	fwrite(STDERR, "Expected non-executable return code handling is missing.\n");
+	exit(1);
+}
+
+if (substr_count($functions, "SYSLOG ERROR: Alert command is not executable: '\$command'") < 2) {


This regression test is very brittle because it relies on exact string fragments and counts in functions.php (e.g., requiring 2 occurrences). Minor refactors (changing quotes/whitespace, extracting a helper, etc.) could break the test without reintroducing the vulnerability. Prefer regex-based checks that look for a /bin/sh invocation pattern and for the presence of the executable-gating behavior, without hard-coding exact formatting or duplicate occurrences.

Suggested change

if (strpos($functions, "exec('/bin/sh ' . \$command") !== false) {

fwrite(STDERR, "Shell fallback execution path is still present.\n");

exit(1);

}

if (substr_count($functions, "preg_split('/\\s+/', trim(\$command))") < 2) {

fwrite(STDERR, "Expected command tokenization update is missing.\n");

exit(1);

}

if (substr_count($functions, '$returnCode = 126;') < 2) {

fwrite(STDERR, "Expected non-executable return code handling is missing.\n");

exit(1);

}

if (substr_count($functions, "SYSLOG ERROR: Alert command is not executable: '\$command'") < 2) {

// Fail if any exec() invocation still uses /bin/sh, regardless of formatting.

if (preg_match('/exec\s*$[^)]*\/bin\/sh[^)]*$/s', $functions)) {

fwrite(STDERR, "Shell fallback execution path is still present.\n");

exit(1);

}

// Ensure that the command tokenization using preg_split and $command is present,

// without relying on exact formatting or duplicate occurrences.

if (!preg_match('/preg_split\s*$[^)]*\$command[^)]*$/s', $functions)) {

fwrite(STDERR, "Expected command tokenization update is missing.\n");

exit(1);

}

// Ensure that non-executable return code handling (126) exists.

if (!preg_match('/\$returnCode\s*=\s*126\s*;/', $functions)) {

fwrite(STDERR, "Expected non-executable return code handling is missing.\n");

exit(1);

}

// Ensure that the non-executable command log message exists, but do not depend

// on the exact quoting or interpolation of $command or on multiple occurrences.

if (strpos($functions, 'SYSLOG ERROR: Alert command is not executable') === false) {

Fixed — test uses preg_match_all() patterns now.

Copilot · 2026-03-07T02:41:05Z

functions.php

 						$logMessage = "SYSLOG NOTICE: Executing '$command'";

-						$cparts = explode(' ', $command);
+						$cparts     = preg_split('/\s+/', trim($command));
+						$output     = array();
+						$returnCode = 0;


The notice log message is constructed as "Executing '$command'" before validating executability, but in the non-executable branch the command is never executed. This results in a misleading log line that still claims execution (with return code 126). Consider only logging "Executing" after the executable check passes, or change the message in the else-path to indicate the command was skipped/invalid.

Two issues with the command execution paths: 1. The 'Executing' log message was emitted even on the 126 (not executable) path because the log call was after the if/else block. Move the cacti_log call inside the if branch so it only fires when the command actually runs. 2. preg_split on whitespace yields $cparts[0] = '"path' for quoted paths like "/usr/bin/cmd" arg. Introduce $executable = trim($cparts[0], '"\\') so is_executable() sees a clean path. Also tighten regression test to use preg_match_all() patterns instead of exact substr_count() strings, which break on minor whitespace drift. Signed-off-by: Thomas Vincent <thomasvincent@gmail.com>

hardening: remove shell fallback from alert command execution

5f11e6c

Fixes Cacti#257 Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

Copilot AI review requested due to automatic review settings March 7, 2026 02:36

Copilot started reviewing on behalf of somethingwithproof March 7, 2026 02:36 View session

Copilot AI reviewed Mar 7, 2026

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

hardening: remove /bin/sh fallback from alert command execution#275

hardening: remove /bin/sh fallback from alert command execution#275
somethingwithproof wants to merge 2 commits intoCacti:developfrom
somethingwithproof:hardening/remove-shell-fallback-alert-command

somethingwithproof commented Mar 7, 2026

Uh oh!

Copilot AI left a comment

Uh oh!

Copilot AI Mar 7, 2026

Uh oh!

somethingwithproof Mar 7, 2026

Uh oh!

Copilot AI Mar 7, 2026

Uh oh!

Copilot AI Mar 7, 2026

Uh oh!

somethingwithproof Mar 7, 2026

Uh oh!

Copilot AI Mar 7, 2026

Uh oh!

somethingwithproof Mar 7, 2026

Uh oh!

Copilot AI Mar 7, 2026

Uh oh!

somethingwithproof Mar 7, 2026

Uh oh!

Copilot AI Mar 7, 2026

Uh oh!

somethingwithproof Mar 7, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

-							cacti_log("SYSLOG ERROR: Alert command is not executable: '$command'", false, 'SYSTEM');
+							$executable = cacti_sizeof($cparts) ? $cparts[0] : '';
+							// Make it explicit that PATH-based commands are not supported
+							if ($executable !== '' && strpos($executable, DIRECTORY_SEPARATOR) === false) {
+								$reason = 'The executable must be specified using an absolute or full filesystem path; PATH-based lookups are not supported.';
+							} else {
+								$reason = 'The specified executable file is not found or is not marked as executable.';
+							}
+							cacti_log("SYSLOG ERROR: Alert command is not executable: '$command'. " . $reason, false, 'SYSTEM');

Conversation

somethingwithproof commented Mar 7, 2026

Summary

Validation

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Copilot AI Mar 7, 2026

Choose a reason for hiding this comment

Uh oh!

somethingwithproof Mar 7, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Mar 7, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Mar 7, 2026

Choose a reason for hiding this comment

Uh oh!

somethingwithproof Mar 7, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Mar 7, 2026

Choose a reason for hiding this comment

Uh oh!

somethingwithproof Mar 7, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Mar 7, 2026

Choose a reason for hiding this comment

Uh oh!

somethingwithproof Mar 7, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Mar 7, 2026

Choose a reason for hiding this comment

Uh oh!

somethingwithproof Mar 7, 2026

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants