Skip to content

security: harden shell command execution against injection (1.2.x backport)#6902

Merged
TheWitness merged 6 commits intoCacti:1.2.xfrom
somethingwithproof:backport/1.2.x-shell-injection-hardening
Mar 29, 2026
Merged

security: harden shell command execution against injection (1.2.x backport)#6902
TheWitness merged 6 commits intoCacti:1.2.xfrom
somethingwithproof:backport/1.2.x-shell-injection-hardening

Conversation

@somethingwithproof
Copy link
Copy Markdown
Contributor

Summary

  • 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() 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
  • Add unit tests for graph_realtime shell and SQL script hardening

Security

Addresses GHSA-xq98-376r-hv9j (Critical) - Command Injection in RRDtool execution

Test plan

  • Verify ping functionality with special characters in hostname
  • Verify realtime graph rendering still works
  • Verify host reindex operates correctly
  • Verify RRD file ownership changes work via PHP native functions
  • Run vendor/bin/pest tests/Unit/GraphRealtimeShellTest.php tests/Unit/SqlScriptsTest.php

Copilot AI review requested due to automatic review settings March 28, 2026 20:53
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Security hardening backport to reduce command-injection risk across Cacti’s shell invocations (poller/realtime, SQL scripts, ping, and file ownership operations), with new unit tests intended to guard the changes.

Changes:

  • Escapes/normalizes shell command arguments (e.g., host_id/local_graph_id, script paths, hostnames) before shell_exec().
  • Replaces a chown shell invocation with PHP-native chown()/chgrp() in Boost.
  • Adds new unit test files covering the shell-hardening changes.

Reviewed changes

Copilot reviewed 9 out of 9 changed files in this pull request and generated 10 comments.

Show a summary per file
File Description
scripts/ss_sql.php Refactors mysqladmin command construction for escaping (currently broken due to missing execution/undefined variable).
scripts/sql.php Refactors mysqladmin invocation to use shell_exec() + escaping (currently broken and changes output semantics).
graph_realtime.php Casts graph ID to int and escapes PHP binary/script path before invoking realtime poller.
host.php Casts host_id to int and escapes PHP binary/script path for reindex shell invocation.
lib/ping.php Applies cacti_escapeshellarg() to hostname in ping shell calls.
lib/boost.php Replaces shell chown with chown()/chgrp() (but currently doesn’t check return values); also introduces a docblock typo.
lib/rrd.php Introduces multiple spelling regressions in comments.
tests/Unit/GraphRealtimeShellTest.php Adds Pest-style tests for realtime graph shell hardening (assertions don’t match current implementation).
tests/Unit/SqlScriptsTest.php Adds Pest-style tests for SQL scripts hardening (assertions don’t match current implementation; Pest not present in repo deps).
Comments suppressed due to low confidence (1)

scripts/ss_sql.php:46

  • In ss_sql(), $cmd is built but never executed and $result is used before it is assigned. This currently makes all the preg_replace() calls operate on an undefined variable and will always return 'U' (and may emit notices). Execute the command (e.g., via shell_exec) and initialize $result from its output (handling null) before applying preg_replace().
	$cmd = 'mysqladmin --host=' . cacti_escapeshellarg($database_hostname) . ' --user=' . cacti_escapeshellarg($database_username);

	if ($database_password != '') {
		$cmd .= ' --password=' . cacti_escapeshellarg($database_password);
	}

	$result = preg_replace('/: /', ':', $result);
	$result = preg_replace('/  /', ' ', $result);

@somethingwithproof
Copy link
Copy Markdown
Contributor Author

Security advisories addressed:

  • GHSA-xq98-376r-hv9j (Critical) - Command Injection in RRDtool execution

Verification: PHP lint clean, PHPStan level 5 clean, 108 Pest tests pass.

…kport)

- 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>
@somethingwithproof somethingwithproof force-pushed the backport/1.2.x-shell-injection-hardening branch from 40d6fcb to d21bdae Compare March 28, 2026 22:21
@TheWitness TheWitness requested review from bmfmancini and xmacan March 29, 2026 14:07
@TheWitness TheWitness merged commit 4e9969e into Cacti:1.2.x Mar 29, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants