Skip to content

fix(security): XSS, SQL injection, object injection, and command injection hardening#327

Closed
somethingwithproof wants to merge 14 commits into
Cacti:developfrom
somethingwithproof:refactor/modernization
Closed

fix(security): XSS, SQL injection, object injection, and command injection hardening#327
somethingwithproof wants to merge 14 commits into
Cacti:developfrom
somethingwithproof:refactor/modernization

Conversation

@somethingwithproof
Copy link
Copy Markdown
Contributor

@somethingwithproof somethingwithproof commented Apr 9, 2026

Summary

  • Apply html_escape() at all option-tag output boundaries across six view files and lib/mactrack_functions.php (14 distinct sites: site_name, device_name, hostname, description, sysDescr_match)
  • Replace string-concatenated SQL in mactrack_actions.php and mactrack_functions.php with db_fetch_row_prepared() / db_fetch_assoc_prepared() and ? placeholders
  • Harden unserialize() on POST data in mactrack_view_macs.php with ['allowed_classes' => false] to block PHP object injection
  • Replace unguarded $extra_args string variable in passthru() calls with inline (int) casts and cacti_escapeshellarg() on both the PHP binary and the script path; rename $command_string$script_path to document the bare-path invariant
  • Add a test suite (Unit, Handoff, Integration, Mutation, Smoke) with 30+ static-source assertions covering all five fixed surfaces

Security surfaces addressed

Surface Before After
XSS — option tags Raw $site['site_name'] etc. html_escape() at every boundary
SQL — host/device lookups String concatenation Prepared statements with ?
Object injection — POST unserialize unserialize($input) unserialize($input, ['allowed_classes' => false])
Command injection — PHP binary Raw read_config_option(...) cacti_escapeshellarg()
Command injection — script path Raw $command_string cacti_escapeshellarg($script_path)
Command injection — device/site IDs Uncast string from DB (int) cast inline

Test plan

  • vendor/bin/pest --testsuite Unit — XSS, unserialize, passthru hardening assertions
  • vendor/bin/pest --testsuite Handoff — setup structure and absence of unescaped patterns
  • vendor/bin/pest --testsuite Integration — no raw request-var SQL concatenation in root PHP files
  • vendor/bin/pest --testsuite Mutation — regression guards for all five bug categories
  • vendor/bin/pest --testsuite Smoke — PHP syntax check of all root and lib files, required hook function presence
  • Manual: confirm AJAX device rescan and site scan still work with cacti_escapeshellarg-wrapped paths

Copilot AI review requested due to automatic review settings April 9, 2026 21:09
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 aims to modernize the plugin for PHP 7.4 by enabling strict typing and updating legacy syntax patterns across the codebase.

Changes:

  • Added declare(strict_types=1); across many PHP entrypoints and library files.
  • Converted some legacy array(...) usages to short array syntax [...].
  • Updated various array-related helper calls—but several updates introduced invalid PHP syntax (parse errors) that must be corrected.

Reviewed changes

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

Show a summary per file
File Description
setup.php Adds strict types declaration.
poller_mactrack.php Introduces a parse error via is_[$ip_ranges] (should be is_array(...)).
Net/DNS2/Updater.php Introduces parse errors via in_[$rr, ...] (should be in_array(...)).
Net/DNS2/Socket/Streams.php Adds strict types declaration.
Net/DNS2/Socket/Sockets.php Adds strict types declaration.
Net/DNS2/Socket.php Adds strict types declaration.
Net/DNS2/RR/X25.php Adds strict types declaration.
Net/DNS2/RR/WKS.php Adds strict types declaration.
Net/DNS2/RR/URI.php Adds strict types declaration.
Net/DNS2/RR/TYPE65534.php Adds strict types declaration.
Net/DNS2/RR/TXT.php Adds strict types declaration.
Net/DNS2/RR/TSIG.php Adds strict types declaration.
Net/DNS2/RR/TLSA.php Adds strict types declaration.
Net/DNS2/RR/TKEY.php Adds strict types declaration.
Net/DNS2/RR/TALINK.php Adds strict types declaration.
Net/DNS2/RR/TA.php Adds strict types declaration.
Net/DNS2/RR/SSHFP.php Adds strict types declaration.
Net/DNS2/RR/SRV.php Adds strict types declaration.
Net/DNS2/RR/SPF.php Adds strict types declaration.
Net/DNS2/RR/SOA.php Adds strict types declaration.
Net/DNS2/RR/SMIMEA.php Adds strict types declaration.
Net/DNS2/RR/SIG.php Adds strict types declaration.
Net/DNS2/RR/RT.php Adds strict types declaration.
Net/DNS2/RR/RRSIG.php Adds strict types declaration.
Net/DNS2/RR/RP.php Adds strict types declaration.
Net/DNS2/RR/PX.php Adds strict types declaration.
Net/DNS2/RR/PTR.php Adds strict types declaration.
Net/DNS2/RR/OPT.php Adds strict types declaration.
Net/DNS2/RR/OPENPGPKEY.php Adds strict types declaration.
Net/DNS2/RR/NSEC3PARAM.php Adds strict types declaration.
Net/DNS2/RR/NSEC3.php Adds strict types declaration.
Net/DNS2/RR/NSEC.php Adds strict types declaration.
Net/DNS2/RR/NSAP.php Adds strict types declaration.
Net/DNS2/RR/NS.php Adds strict types declaration.
Net/DNS2/RR/NIMLOC.php Adds strict types declaration.
Net/DNS2/RR/NID.php Adds strict types declaration.
Net/DNS2/RR/NAPTR.php Adds strict types declaration.
Net/DNS2/RR/MX.php Adds strict types declaration.
Net/DNS2/RR/LP.php Adds strict types declaration.
Net/DNS2/RR/LOC.php Adds strict types declaration.
Net/DNS2/RR/L64.php Adds strict types declaration.
Net/DNS2/RR/L32.php Adds strict types declaration.
Net/DNS2/RR/KX.php Adds strict types declaration.
Net/DNS2/RR/KEY.php Adds strict types declaration.
Net/DNS2/RR/ISDN.php Adds strict types declaration.
Net/DNS2/RR/IPSECKEY.php Adds strict types declaration.
Net/DNS2/RR/HIP.php Adds strict types declaration.
Net/DNS2/RR/HINFO.php Adds strict types declaration.
Net/DNS2/RR/EUI64.php Adds strict types declaration.
Net/DNS2/RR/EUI48.php Adds strict types declaration.
Net/DNS2/RR/EID.php Adds strict types declaration.
Net/DNS2/RR/DS.php Adds strict types declaration.
Net/DNS2/RR/DNSKEY.php Adds strict types declaration.
Net/DNS2/RR/DNAME.php Adds strict types declaration.
Net/DNS2/RR/DLV.php Adds strict types declaration.
Net/DNS2/RR/DHCID.php Adds strict types declaration.
Net/DNS2/RR/CSYNC.php Adds strict types declaration.
Net/DNS2/RR/CNAME.php Adds strict types declaration.
Net/DNS2/RR/CERT.php Adds strict types declaration.
Net/DNS2/RR/CDS.php Adds strict types declaration.
Net/DNS2/RR/CDNSKEY.php Adds strict types declaration.
Net/DNS2/RR/CAA.php Adds strict types declaration.
Net/DNS2/RR/AVC.php Adds strict types declaration.
Net/DNS2/RR/ATMA.php Adds strict types declaration.
Net/DNS2/RR/APL.php Adds strict types declaration.
Net/DNS2/RR/ANY.php Adds strict types declaration.
Net/DNS2/RR/AMTRELAY.php Adds strict types declaration.
Net/DNS2/RR/AFSDB.php Adds strict types declaration.
Net/DNS2/RR/AAAA.php Adds strict types declaration.
Net/DNS2/RR/A.php Adds strict types declaration.
Net/DNS2/RR.php Adds strict types declaration.
Net/DNS2/Resolver.php Adds strict types declaration.
Net/DNS2/Question.php Adds strict types declaration.
Net/DNS2/PrivateKey.php Adds strict types declaration.
Net/DNS2/Packet/Response.php Adds strict types declaration.
Net/DNS2/Packet/Request.php Adds strict types declaration.
Net/DNS2/Packet.php Adds strict types declaration.
Net/DNS2/Notifier.php Introduces a parse error via in_[$rr, ...] (should be in_array(...)).
Net/DNS2/Lookups.php Adds strict types declaration.
Net/DNS2/Header.php Adds strict types declaration.
Net/DNS2/Exception.php Adds strict types declaration.
Net/DNS2/Cache/Shm.php Introduces parse errors via is_[$decoded] (should be is_array(...)).
Net/DNS2/Cache/File.php Introduces parse errors via is_[$decoded] (should be is_array(...)).
Net/DNS2/Cache.php Adds strict types declaration.
Net/DNS2/BitMap.php Adds strict types declaration.
Net/DNS2.php Introduces a parse error via is_[$nameservers] (should be is_array(...)).
mactrack_view_sites.php Adds strict types declaration.
mactrack_view_macs.php Adds strict types declaration.
mactrack_view_ips.php Introduces a parse error via is_[$ip_ranges] (should be is_array(...)).
mactrack_view_interfaces.php Introduces parse errors via mactrack_display_[] and function mactrack_display_[].
mactrack_view_graphs.php Adds strict types declaration.
mactrack_view_dot1x.php Adds strict types declaration.
mactrack_view_devices.php Adds strict types declaration.
mactrack_view_arp.php Adds strict types declaration.
mactrack_vendormacs.php Adds strict types declaration.
mactrack_utilities.php Adds strict types declaration.
mactrack_snmp.php Adds strict types declaration.
mactrack_sites.php Adds strict types declaration.
mactrack_scanner.php Introduces parse errors via call_user_func_[...] (should be call_user_func_array(...) or similar).
mactrack_resolver.php Adds strict types declaration.
mactrack_macwatch.php Adds strict types declaration.
mactrack_macauth.php Adds strict types declaration.
mactrack_import_ouidb.php Adds strict types declaration.
mactrack_devices.php Introduces parse errors via is_[] / in_[] patterns.
mactrack_device_types.php Introduces parse errors via is_[] / in_[] patterns.
mactrack_convert.php Converts array(...) to [...] for prepared statement args.
mactrack_ajax.php Adds strict types declaration.
mactrack_ajax_admin.php Adds strict types declaration.
mactrack_actions.php Introduces parse errors via is_[] and invalid function signature mactrack_device_action_[$action].
locales/po/index.php Adds strict types declaration.
locales/LC_MESSAGES/index.php Adds strict types declaration.
locales/index.php Adds strict types declaration.
lib/mactrack_vendors.php Adds strict types declaration.
lib/mactrack_trendnet.php Introduces parse errors via port_list_to_[] / in_[] patterns.
lib/mactrack_tplink.php Adds strict types declaration.
lib/mactrack_norbay.php Adds strict types declaration.
lib/mactrack_norbay_ng.php Adds strict types declaration.
lib/mactrack_linux.php Introduces parse errors via port_list_to_[] / in_[] patterns.
lib/mactrack_juniper.php Introduces parse errors via port_list_to_[] / in_[] patterns.
lib/mactrack_hp.php Adds strict types declaration.
lib/mactrack_hp_ngi.php Adds strict types declaration.
lib/mactrack_hp_ng.php Adds strict types declaration.
lib/mactrack_h3c_3com.php Introduces parse errors via port_list_to_[] / in_[]; also alters comment dump examples.
lib/mactrack_functions.php Introduces multiple parse errors (port_list_to_[], is_[], in_[]) impacting shared helpers.
lib/mactrack_foundry.php Adds strict types declaration.
lib/mactrack_extreme.php Adds strict types declaration.
lib/mactrack_enterasys.php Adds strict types declaration.
lib/mactrack_enterasys_N7.php Introduces parse errors via port_list_to_[] / in_[] patterns.
lib/mactrack_dlink.php Introduces parse errors via port_list_to_[] / in_[] patterns.
lib/mactrack_dell.php Introduces parse errors via port_list_to_[] / in_[] patterns.
lib/mactrack_cisco.php Introduces parse errors via in_[] patterns.
lib/mactrack_cabletron.php Introduces parse errors via port_list_to_[] / in_[] patterns.
lib/mactrack_aruba_oscx.php Introduces parse errors via port_list_to_[] / in_[]; also alters comment dump examples.
lib/mactrack_3com.php Introduces parse errors via port_list_to_[] / in_[] patterns.
lib/index.php Adds strict types declaration.
index.php Adds strict types declaration.
includes/database.php Adds strict types declaration.
images/index.php Adds strict types declaration.
.omc/sessions/ce3e60e4-0e62-483a-8587-78da29248e14.json Adds transient session metadata file (likely should not be committed).
.omc/sessions/b398cee1-3730-4624-a0f1-d0e0120b23cb.json Adds transient session metadata file (likely should not be committed).

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread poller_mactrack.php
Comment thread Net/DNS2/Updater.php
Comment thread Net/DNS2/Notifier.php Outdated
Comment thread Net/DNS2/Cache/Shm.php Outdated
Comment thread Net/DNS2/Cache/File.php Outdated
Comment thread lib/mactrack_3com.php
Comment thread lib/mactrack_h3c_3com.php
Comment thread lib/mactrack_aruba_oscx.php
Comment thread .omc/sessions/b398cee1-3730-4624-a0f1-d0e0120b23cb.json Outdated
Comment thread .omc/sessions/ce3e60e4-0e62-483a-8587-78da29248e14.json Outdated
Revert corrupted function calls introduced by refactoring tool:
- is_[$x] -> is_array($x)
- in_[$x, ...] -> in_array($x, ...)
- xml2[$x] -> xml2array($x)

Also remove accidentally committed .omc session files and add
.omc/ to .gitignore.

Signed-off-by: Thomas Vincent <thomasvincent@gmail.com>
Revert bulk array()->[] rewrite damage affecting:
- is_array, in_array, xml2array
- call_user_func_array, filter_var_array
- Function declarations with _array suffix

Signed-off-by: Thomas Vincent <thomasvincent@gmail.com>
Signed-off-by: Thomas Vincent <thomasvincent@gmail.com>
Signed-off-by: Thomas Vincent <thomasvincent@gmail.com>
@somethingwithproof somethingwithproof marked this pull request as draft April 11, 2026 00:09
@somethingwithproof
Copy link
Copy Markdown
Contributor Author

Converted to draft to serialize the stack in this repo. Blocked by #324; will un-draft after that merges to avoid cross-PR merge conflicts.

@somethingwithproof somethingwithproof marked this pull request as ready for review April 24, 2026 19:24
- Wrap all db-sourced option text in html_escape() across six view files
  and lib/mactrack_functions.php (site_name, device_name, hostname,
  description, sysDescr_match)
- Add allowed_classes:false to unserialize() of user-supplied mac/ip
  map in mactrack_view_macs.php (object injection mitigation)
- Cast device_id and site_id to int before passthru() in rescan and
  site-scan helpers; add nosemgrep annotations
- Convert two raw SQL concatenations in mactrack_actions.php to
  db_fetch_row_prepared(); convert mac_track_interfaces query in
  mactrack_functions.php to db_fetch_assoc_prepared()

Signed-off-by: Thomas Vincent <thomasvincent@gmail.com>
52 tests covering:
- XSS html_escape at all output boundaries (unit + handoff)
- unserialize allowed_classes hardening (unit + mutation)
- passthru int-cast regression (unit + mutation)
- prepared statement consistency scan (integration)
- PHP syntax of all plugin files (smoke)
- Required Cacti plugin hook functions (smoke + handoff)

Signed-off-by: Thomas Vincent <thomasvincent@gmail.com>
Wrap both passthru command_string arguments with cacti_escapeshellarg().
Admin-configured paths with spaces would break silently without escaping.

Also document why bare escapeshellarg() is used in smoke tests: the Cacti
bootstrap (and thus cacti_escapeshellarg) requires a DB connection that is
unavailable in the Pest test context; $file values are glob-returned
server-local paths, not user input.

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

$command_string held only bare filesystem paths at both passthru sites,
but the name implied it could carry embedded shell arguments. Renaming to
$script_path makes the invariant explicit and aligns with the nosemgrep
comment.

Remove the $extra_args assignments that became dead code when the passthru
commands were rewritten with inline (int) casts.

Add a comment documenting that --web is unconditional in mactrack_site_scan:
the function is only reachable via AJAX (mactrack_ajax.php), never from CLI,
so the asymmetry with mactrack_device_rescan is intentional.

Add two tests asserting the bare-path invariant and the unconditional --web
behaviour so future changes cannot silently regress either property.

Signed-off-by: Thomas Vincent <thomasvincent@gmail.com>
@somethingwithproof somethingwithproof changed the title refactor: safe PHP 7.4 modernization fix(security): XSS, SQL injection, object injection, and command injection hardening May 17, 2026
somethingwithproof added a commit to somethingwithproof/plugin_mactrack that referenced this pull request May 17, 2026
…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>
@somethingwithproof
Copy link
Copy Markdown
Contributor Author

All hardening content from this PR has been cherry-picked into #334 (hardening/mactrack). Closing in favour of that consolidated branch.

somethingwithproof added a commit to somethingwithproof/plugin_mactrack that referenced this pull request May 17, 2026
…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>
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