harden(mactrack): validate AJAX inputs and escape shell args in subprocess dispatch#334
Open
somethingwithproof wants to merge 11 commits into
Open
harden(mactrack): validate AJAX inputs and escape shell args in subprocess dispatch#334somethingwithproof wants to merge 11 commits into
somethingwithproof wants to merge 11 commits into
Conversation
Contributor
There was a problem hiding this comment.
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()) fordevice_id/ifIndex/site_idprior to use in the AJAX rescan/site-scan handlers. - Escaped PHP binary path, script path, and DB-derived IDs via
cacti_escapeshellarg()before invokingpassthru(). - Captured
passthru()exit codes and added anerrorfield to the JSON payload on non-zero exit.
Comments suppressed due to low confidence (1)
lib/mactrack_functions.php:3194
- Same as
mactrack_rescan():erroris populated in the JSON response, but the current caller only displaysdata.content. If the subprocess exits non-zero with no output, users may still get an empty dialog; consider rendering the error intocontentor updating the JS client to handledata.error.
$data['content'] = ob_get_clean();
if ($exit_code !== 0) {
$data['error'] = 'site_scan process exited with code ' . intval($exit_code);
}
This was referenced May 17, 2026
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>
2fb9d14 to
0c45b3e
Compare
… 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>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
get_filter_request_var()beforeget_request_var()inmactrack_rescan()andmactrack_site_scan()to enforce the Cacti input-validation contract at AJAX shell-dispatch entry points$webfromglobal $config, $web;inmactrack_site_scan()to prevent the parameter from being silently clobbered by the global scope valuecacti_escapeshellarg()to all three argument positions in the passthru command: PHP binary path, script path, and DB-sourced ID tokenspassthru()exit code and propagate subprocess failures into the JSON error payload instead of returning ambiguous empty contentTest plan
mactrack_rescan()andmactrack_site_scan()execute correctly when called with a validdevice_id/site_iddevice_idresults in validation rejection before DB lookup and subprocess launcherrorkey appears in JSON responsemactrack_site_scan(true)correctly passes--webflag to the subprocess after removing theglobal $webclobber🤖 Generated with Claude Code