fix(security): defense-in-depth hardening for plugin_webseer#66
fix(security): defense-in-depth hardening for plugin_webseer#66somethingwithproof wants to merge 7 commits intoCacti:developfrom
Conversation
Automated fixes: - XSS: escape request variables in HTML value attributes - 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
This PR aims to harden plugin_webseer against deserialization attacks by restricting classes during unserialize() of remote data.
Changes:
- Updates server list refresh to add an
allowed_classesrestriction during deserialization. - Updates URL list refresh to add an
allowed_classesrestriction during deserialization.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| if (substr($r, 0, 8) == 'SERVERS=') { | ||
| $servers = substr($r, 8); | ||
| $servers = unserialize(base64_decode($servers)); | ||
| $servers = unserialize(base64_decode($servers, array('allowed_classes' => false))); |
There was a problem hiding this comment.
base64_decode()'s second parameter is the boolean $strict, not an options array. Passing array('allowed_classes' => false) will be treated as true (strict mode) and the allowed_classes restriction will not be applied to unserialize(). Move the options array to the second argument of unserialize() (and keep base64_decode($servers, true) if you want strict decoding).
| $servers = unserialize(base64_decode($servers, array('allowed_classes' => false))); | |
| $servers = unserialize(base64_decode($servers, true), array('allowed_classes' => false)); |
There was a problem hiding this comment.
Fixed; options array moved to unserialize() second param.
| if (substr($r, 0, 5) == 'URLS=') { | ||
| $urls = substr($r, 5); | ||
| $urls = unserialize(base64_decode($urls)); | ||
| $urls = unserialize(base64_decode($urls, array('allowed_classes' => false))); | ||
|
|
There was a problem hiding this comment.
Same issue as above: base64_decode() does not accept an allowed_classes option array, so this change does not actually harden unserialize(). Pass array('allowed_classes' => false) as the 2nd parameter to unserialize() (and optionally use strict mode in base64_decode()).
There was a problem hiding this comment.
Fixed; options array moved to unserialize() second param.
Replace .click(fn) with .on('click', fn), .change(fn) with
.on('change', fn), .submit(fn) with .on('submit', fn), .unbind()
with .off(), and .resize(fn) with .on('resize', fn).
These shorthands were deprecated in jQuery 3.3 and will be removed
in jQuery 4.0. Cacti core ships jQuery 3.x on develop.
Signed-off-by: Thomas Vincent <thomasvincent@gmail.com>
The options array was mistakenly passed as the second arg to base64_decode(), where it is interpreted as the boolean $strict parameter. The allowed_classes restriction is never applied. Move the options array to the second arg of unserialize(). Signed-off-by: Thomas Vincent <thomasvincent@gmail.com>
- Use strict mode for base64_decode - Check decoded result before unserialize - Suppress unserialize warnings with @ and check is_array - Log and break on any failure Signed-off-by: Thomas Vincent <thomasvincent@gmail.com>
- 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>
Signed-off-by: Thomas Vincent <thomasvincent@gmail.com>
|
Converted to draft to serialize the stack in this repo. Blocked by #65; will un-draft after that merges to avoid cross-PR merge conflicts. |
Summary
Defense-in-depth hardening addressing 26 security audit findings.
html_escape_request_var()allowed_classes => falsetounserialize()All changes PHP 7.0+ compatible.
Test plan