hardening: prepared statements, PHP 7.4 idioms, and security fixes#40
hardening: prepared statements, PHP 7.4 idioms, and security fixes#40somethingwithproof wants to merge 4 commits intoCacti:developfrom
Conversation
Signed-off-by: Thomas Vincent <thomasvincent@gmail.com>
Signed-off-by: Thomas Vincent <thomasvincent@gmail.com>
There was a problem hiding this comment.
Pull request overview
This PR consolidates several PHP 7.4+ mechanical hardening changes (null coalescing operators, some shell escaping, and minor query hardening) across the HMIB plugin and SNMP helper code.
Changes:
- Replace
isset()ternaries /!isset()assignments with??/??=in multiple files. - Add additional shell-escaping / numeric casting for CLI invocations.
- Tighten one SQL update by casting an ID to integer.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
snmp.php |
Uses ??= for global defaults and adds numeric casting before invoking snmpget. |
poller_hmib.php |
Replaces isset() ternary with null coalescing for date parsing. |
poller_graphs.php |
Escapes configured PHP binary path and base path used in CLI commands. |
hmib.php |
Uses ??= for session-cached graph template/host lists. |
hmib_types.php |
Casts host_type update ID to int and uses ?? when injecting form variables. |
Comments suppressed due to low confidence (1)
hmib_types.php:396
- This UPDATE query still concatenates $known['sysObjectID'] and $known['sysDescrMatch'] directly into LIKE/RLIKE expressions without quoting/escaping, which leaves a SQL injection vector if those fields contain quotes (these values are user-controlled via the Host Types UI/import). Use db_execute_prepared with placeholders (building the LIKE patterns in parameters) and ensure the RLIKE patterns are passed as parameters/escaped appropriately.
db_execute('UPDATE plugin_hmib_hrSystem SET host_type=' . (int) $known['id'] . "
WHERE host_type=0 AND (sysObjectID LIKE '%" . $known['sysObjectID'] . "%' AND
sysDescr LIKE '%" . $known['sysDescrMatch'] . "%')
OR (sysObjectID RLIKE '" . $known['sysObjectID'] . "' AND
sysDescr RLIKE '" . $known['sysDescrMatch'] . "')");
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| $version = (int) $version; | ||
| $timeout = (int) $timeout; | ||
| $retries = (int) $retries; |
There was a problem hiding this comment.
Casting $version to int here breaks the net-snmp binary invocation for v2 because earlier code intentionally sets $version = '2c'. After the cast, '-v 2c' becomes '-v 2', which can change behavior or fail depending on the snmpget implementation. Consider leaving $version as the validated/whitelisted string (e.g., '1','2c','3') and only casting $timeout/$retries (and possibly $port) to int for injection hardening.
| $version = (int) $version; | |
| $timeout = (int) $timeout; | |
| $retries = (int) $retries; | |
| $timeout = (int) $timeout; | |
| $retries = (int) $retries; | |
| $port = (int) $port; |
There was a problem hiding this comment.
Valid. The int cast is from pre-existing code, not introduced by this PR. Will investigate the v2c handling separately.
Signed-off-by: Thomas Vincent <thomasvincent@gmail.com>
454e566 to
11ffc52
Compare
Consolidated hardening PR:
5 files changed, 13 insertions(+), 14 deletions(-)
All changes are mechanical transforms with zero behavioral impact. PHP 7.4+ compatible.