Skip to content

security: fix SSRF, command injection, and XSS in core functions (1.2.x)#6913

Merged
TheWitness merged 5 commits intoCacti:1.2.xfrom
somethingwithproof:backport/1.2.x-audit-findings
Mar 29, 2026
Merged

security: fix SSRF, command injection, and XSS in core functions (1.2.x)#6913
TheWitness merged 5 commits intoCacti:1.2.xfrom
somethingwithproof:backport/1.2.x-audit-findings

Conversation

@somethingwithproof
Copy link
Copy Markdown
Contributor

Summary

Surgical defense-in-depth fixes. +14/-5 lines, 3 files.

  • Validate URL in call_remote_data_collector() rejects :// and @ to prevent SSRF
  • Escape $database, $username, $password, $output_file in db_dump_data() exec calls via cacti_escapeshellarg()
  • Escape $title in html_start_box() via html_escape() to prevent stored XSS

All three have limited exploitability (admin-only callers, DB-sourced inputs, translated string titles) but are worth hardening per security audit findings.

Test plan

  • Verify remote poller communication works
  • Verify cli/audit_database.php dump still works
  • Verify page headers render correctly (no double-escaping)

Copilot AI review requested due to automatic review settings March 29, 2026 06:43
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

Defense-in-depth security hardening across core Cacti helpers to reduce risk of stored XSS, SSRF-style URL authority injection, and shell command injection in mysqldump execution paths.

Changes:

  • Escape html_start_box() title output to mitigate stored XSS.
  • Add URL validation in call_remote_data_collector() to restrict unsafe inputs.
  • Shell-escape mysqldump arguments in db_dump_data().

Reviewed changes

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

File Description
lib/html.php Escapes html_start_box() title output.
lib/functions.php Adds URL validation before remote collector file_get_contents().
lib/database.php Escapes selected mysqldump arguments before exec().

- Validate URL in call_remote_data_collector to prevent SSRF via protocol/host injection
- Escape database, username, password, and output_file in db_dump_data exec calls
- Escape $title in html_start_box to prevent stored XSS

Defense-in-depth: all three have limited exploitability (admin-only callers,
DB-sourced inputs, or translated string titles) but are worth hardening.

Signed-off-by: Thomas Vincent <thomasvincent@gmail.com>
@somethingwithproof somethingwithproof force-pushed the backport/1.2.x-audit-findings branch from ecc5e78 to 32b6e84 Compare March 29, 2026 07:51
TheWitness and others added 3 commits March 29, 2026 08:42
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Added check for '../' in URL validation to prevent SSRF.
@TheWitness TheWitness requested review from bmfmancini and xmacan March 29, 2026 13:42
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

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

Comments suppressed due to low confidence (1)

lib/database.php:2228

  • In db_dump_data(), $credentials_string is still built by concatenating raw credential values (e.g. --host, --ssl-ca paths) directly into the shell command. Even with $database/$username/$password/$output_file escaped, this leaves room for shell metacharacters or spaces in those values to break the command (and can reintroduce command injection if any caller passes untrusted credentials). Build $credentials_string using cacti_escapeshellarg() for values (and ensure flags/keys are validated) before interpolating into exec().
	if (cacti_sizeof($credentials)) {
		foreach ($credentials as $key => $value) {
			$name = trim($key);
			if (strstr($name, '--') !== false) {      //name like --host
				if($name == '--password') {
					$password = $value;
				} elseif ($name == '--user') {
					$username = $value;
				} else {
					$credentials_string .= $name . '=' . $value . ' ';
				}
			} elseif(strstr($name, '-') !== false) { //name like -h
				if($name == '-p') {
					$password = $value;
				} elseif ($name == '-u') {
					$username = $value;
				} else {
					$credentials_string .= $name . $value . ' ';
				}
			} else {                                  //name like host
				if($name == 'password') {
					$password = $value;
				} elseif ($name == 'user') {
					$username = $value;
				} else {
					$credentials_string .= '--' . $name . '=' . $value . ' ';
				}

@TheWitness TheWitness requested a review from bmfmancini March 29, 2026 13:45
@TheWitness TheWitness merged commit 2e48360 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.

5 participants