fix(security): defense-in-depth hardening for plugin_reportit#129
fix(security): defense-in-depth hardening for plugin_reportit#129somethingwithproof wants to merge 3 commits intoCacti:developfrom
Conversation
Automated fixes: - XSS: escape request variables in HTML output - SQLi: convert string-concat queries to prepared statements - Deserialization: add allowed_classes=>false - Temp files: replace rand() with tempnam() Signed-off-by: Thomas Vincent <thomasvincent@gmail.com>
There was a problem hiding this comment.
Pull request overview
Defense-in-depth security hardening for the ReportIt plugin UI and shared utilities, primarily focusing on reducing XSS risk in rendered HTML and tightening request parameter handling.
Changes:
- Escapes
filterrequest values when rendering into HTML<input value=...>attributes. - Tightens
idpropagation into URLs by switching to filtered request access and casting to integer in a few UI flows. - Attempts to harden
unserialize()usage to disallow object instantiation (but currently contains a runtime-breaking bug).
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
view.php |
Escapes filter in input fields; uses filtered/cast id when building JS navigation URLs. |
templates.php |
Escapes filter in template search input value. |
reportit.php |
Escapes filter in report filters and item search; uses filtered/cast id in form action + JS URLs. |
lib/funct_shared.php |
Attempts to add allowed_classes => false to unserialize() in cache/table transformation logic (implementation currently incorrect). |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| foreach ($array as $key => $value) { | ||
| if ($key == 'data_template_alias') { | ||
| $value = base64_encode(json_encode(unserialize(stripslashes($value)))); | ||
| $value = base64_encode(json_encode(unserialize(stripslashes($value, array('allowed_classes' => false))))); |
There was a problem hiding this comment.
The new allowed_classes hardening is applied to stripslashes() instead of unserialize(), and also adds an extra argument to stripslashes() (which only accepts 1 parameter). This will trigger runtime warnings/errors and the unserialize will still run without allowed_classes => false. Pass the options array as the second argument to unserialize() instead (after calling stripslashes($value)).
| $value = base64_encode(json_encode(unserialize(stripslashes($value, array('allowed_classes' => false))))); | |
| $value = base64_encode(json_encode(unserialize(stripslashes($value), array('allowed_classes' => false)))); |
- Change Dependabot ecosystem from npm to composer (PHP-only repo) - Remove PHP from CodeQL paths-ignore so security PRs get analysis - Remove committed .omc session artifacts, add .omc/ to .gitignore Signed-off-by: Thomas Vincent <thomasvincent@gmail.com>
Signed-off-by: Thomas Vincent <thomasvincent@gmail.com>
|
Converted to draft to serialize the stack in this repo. Blocked by #128; will un-draft after that merges to avoid cross-PR merge conflicts. |
Summary
Automated defense-in-depth hardening addressing 45 security audit findings.
html_escape_request_var()allowed_classes => falsetounserialize()callsrand()withtempnam()All changes are PHP 7.0+ compatible for Cacti 1.2.x.
Test plan