Skip to content

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

Open
somethingwithproof wants to merge 11 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 11 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

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

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>
Thomas Vincent and others added 2 commits July 13, 2026 01:50
… IN() deletes

mactrack_view_macs.php: wrap the drp_action hidden-input value with html_escape() in both form_actions() and form_aggregated_actions() (defense in depth; drp_action is already int-validated via get_filter_request_var), and escape the aggregated serialize($row_array) hidden input for parity with the MAC form.

Parameterize the two interpolated 'row_id IN (...)' statements (aggregated delete and the row lookup) with db_execute_prepared()/db_fetch_assoc_prepared() and ?-placeholders. The bound values were already numeric-validated (sanitize_unserialize_selected_items / input_validate_input_number), so this is defense in depth and brings the file in line with the PR's PreparedStatementConsistencyTest.

Co-Authored-By: Oz <oz-agent@warp.dev>
…_devices.php

form_mactrack_actions(): html_escape() the device_name/hostname printed into the confirmation <li> list. These values originate from SNMP discovery (e.g. sysName) and were emitted unescaped (stored XSS). Mirrors the escaping already applied to mactrack_device_edit().

Copy SNMP Settings (drp_action==7): the UPDATE built its SET clause by interpolating host-table values with bare single quotes, allowing second-order SQL injection when a value contains a quote. Values are now bound via db_execute_prepared() placeholders; the column name ($field_name) remains interpolated as it is a trusted key from the static $fields_mactrack_snmp_item definition.

Co-Authored-By: Oz <oz-agent@warp.dev>
somethingwithproof pushed a commit to somethingwithproof/plugin_mactrack that referenced this pull request Jul 13, 2026
mactrack_macauth.php: api_mactrack_maca_save() interpolated the user-supplied MAC address into an UPDATE ... LIKE "...%" (first-order SQLi); now bound via db_execute_prepared() with a ? placeholder. api_mactrack_maca_remove() called db_execute_prepared() while concatenating the (stored) mac_address into the SQL with no bound params (second-order SQLi + misuse of the prepared helper); the three DELETEs now bind mac_address as a parameter.

mactrack_devices.php: Copy SNMP Settings (drp_action==7) built its UPDATE SET clause by interpolating host-table values with bare quotes; values are now bound with placeholders (column name stays interpolated as a trusted static-array key). Mirrors the fix already applied on the Cacti#334 branch.

mactrack_device_types.php, mactrack_sites.php, mactrack_snmp.php: (int)-cast the remaining raw numeric request vars (type_id, site_id, and id used in snmp_id= fragments for move_item_up/down and get_sequence) — defense-in-depth over the FILTER_VALIDATE_INT request validation. 1.2.31-idiomatic, PHP 7.4-safe.

Co-Authored-By: Oz <oz-agent@warp.dev>
Device name, IP, MAC, and port values are echoed into HTML lists.

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