Skip to content

harden(mactrack): validate AJAX inputs and escape shell args in subprocess dispatch#334

Open
somethingwithproof wants to merge 6 commits into
Cacti:developfrom
somethingwithproof:hardening/mactrack
Open

harden(mactrack): validate AJAX inputs and escape shell args in subprocess dispatch#334
somethingwithproof wants to merge 6 commits into
Cacti:developfrom
somethingwithproof:hardening/mactrack

Conversation

@somethingwithproof
Copy link
Copy Markdown
Contributor

Summary

  • Add get_filter_request_var() before get_request_var() in mactrack_rescan() and mactrack_site_scan() to enforce the Cacti input-validation contract at AJAX shell-dispatch entry points
  • Remove $web from global $config, $web; in mactrack_site_scan() to prevent the parameter from being silently clobbered by the global scope value
  • Apply cacti_escapeshellarg() to all three argument positions in the passthru command: PHP binary path, script path, and DB-sourced ID tokens
  • Capture passthru() exit code and propagate subprocess failures into the JSON error payload instead of returning ambiguous empty content

Test plan

  • Confirm mactrack_rescan() and mactrack_site_scan() execute correctly when called with a valid device_id/site_id
  • Confirm that passing non-integer device_id results in validation rejection before DB lookup and subprocess launch
  • Trigger a scanner subprocess exit with non-zero code; verify error key appears in JSON response
  • Verify mactrack_site_scan(true) correctly passes --web flag to the subprocess after removing the global $web clobber

🤖 Generated with Claude Code

Copilot AI review requested due to automatic review settings May 17, 2026 10:02
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

This PR hardens the mactrack plugin’s AJAX-triggered scanner dispatch by validating request inputs, shell-escaping subprocess arguments, and reporting subprocess failures back in the JSON response.

Changes:

  • Added request filtering (get_filter_request_var()) for device_id/ifIndex/site_id prior to use in the AJAX rescan/site-scan handlers.
  • Escaped PHP binary path, script path, and DB-derived IDs via cacti_escapeshellarg() before invoking passthru().
  • Captured passthru() exit codes and added an error field to the JSON payload on non-zero exit.
Comments suppressed due to low confidence (1)

lib/mactrack_functions.php:3194

  • Same as mactrack_rescan(): error is populated in the JSON response, but the current caller only displays data.content. If the subprocess exits non-zero with no output, users may still get an empty dialog; consider rendering the error into content or updating the JS client to handle data.error.
		$data['content'] = ob_get_clean();

		if ($exit_code !== 0) {
			$data['error'] = 'site_scan process exited with code ' . intval($exit_code);
		}

Comment thread lib/mactrack_functions.php Outdated
Comment thread lib/mactrack_functions.php
Signed-off-by: Thomas Vincent <thomasvincent@gmail.com>
…code

Signed-off-by: Thomas Vincent <thomasvincent@gmail.com>
…emove global clobber

Signed-off-by: Thomas Vincent <thomasvincent@gmail.com>
…cti#327

- db_fetch_assoc_prepared for interfaces query in mactrack_functions.php
- db_fetch_row_prepared for host/mt_device lookups in mactrack_actions.php
- html_escape device_name/hostname in mactrack_devices.php
- unserialize allowed_classes guard + html_escape in mactrack_view_macs.php

Signed-off-by: Thomas Vincent <thomasvincent@gmail.com>
Signed-off-by: Thomas Vincent <thomasvincent@gmail.com>
…or surfacing

Make --web conditional on $web param in mactrack_site_scan (was always added).
Append subprocess exit-code errors to data['content'] so the AJAX client
shows failures rather than an empty dialog.

Signed-off-by: Thomas Vincent <thomasvincent@gmail.com>
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.

2 participants