security: escape the WMI hostname and namespace before exec - #13
Open
somethingwithproof wants to merge 4 commits into
Open
security: escape the WMI hostname and namespace before exec#13somethingwithproof wants to merge 4 commits into
somethingwithproof wants to merge 4 commits into
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>
- 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>
Linux_WMI::clean() escaped the username, password, binary and command but left
the device hostname only trimmed and the query namespace untouched, so
getcommand() interpolated them raw into the wmic command line that exec() runs
on the Cacti server. A device-supplied hostname such as
127.0.0.1; touch /tmp/pwned #
therefore ran a command on the poller.
Escape the hostname and namespace with cacti_escapeshellarg, and on Windows
strip the cmd.exe metacharacters (" & | ^ < > ( ) %) that cmd.exe interprets
despite quoting. A standalone regression test in tests/ verifies both.
Signed-off-by: Thomas Vincent <thomasvincent@gmail.com>
The default separator (|+|) contains pipe characters, so the unquoted --delimiter=|+| made exec() split the command into a shell pipeline (exit 127, no data). Quote it in getcommand() while keeping the property raw for the explode() in fetch(). issue#5 Signed-off-by: Thomas Vincent <thomasvincent@gmail.com>
This was referenced Aug 30, 2026
There was a problem hiding this comment.
🟡 Changes recommended
decode() still assumes deserialization always succeeds and can emit warnings/fail unexpectedly on malformed data (and the new test assertion can be strengthened to avoid false passes).
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
This PR hardens the WMI plugin’s command construction and credential decoding to mitigate command injection and PHP object injection risks in the Linux WMIC execution path.
Changes:
- Escape/sanitize WMI hostname and namespace before building the
wmiccommand line, and quote the--delimiterargument to avoid shell pipeline splitting. - Restrict credential blob deserialization by passing
allowed_classes => falsetounserialize(). - Add a standalone regression test to validate command-injection neutralization behavior.
File summaries
| File | Description |
|---|---|
linux_wmi.php |
Escapes hostname/namespace, quotes delimiter for shell safety, and restricts unserialize() class instantiation. |
tests/WmiCommandInjectionTest.php |
Adds a standalone regression test for hostname/namespace escaping and Windows metachar stripping. |
.gitignore |
Ignores the .omc/ directory. |
Review details
- Files reviewed: 2/3 changed files
- Comments generated: 2
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Comment on lines
309
to
312
| $info = base64_decode($info); | ||
| $info = unserialize($info); | ||
| $info = unserialize($info, array('allowed_classes' => false)); | ||
| $info = $info['password']; | ||
|
|
Comment on lines
+43
to
+44
| check(strpos($cmd, '; touch /tmp/pwned') === false || strpos($cmd, "'127.0.0.1; touch /tmp/pwned #'") !== false, | ||
| 'injected hostname is contained inside a quoted argument'); |
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.
Linux_WMI::clean()escaped the username, password, binary and command withcacti_escapeshellarg, but left the device hostname onlytrim()'d and the query namespace untouched.getcommand()then interpolated both raw into thewmiccommand line thatexec()runs on the Cacti server.A device configured with a hostname such as
therefore executes a command on the poller when its WMI data query runs — a device-manager to server-RCE.
Fix
cacti_escapeshellarginclean(), like the other fields" & | ^ < > ( ) %) that cmd.exe interprets despite quoting (it ignores\", toggles quoting on every", and expands%VAR%); a hostname/namespace never legitimately contains thesetrim()ingetcommand()Test
tests/WmiCommandInjectionTest.php(standalone — the plugin has no harness) confirms an injected hostname is quote-contained on unix and metachar-stripped on Windows.php tests/WmiCommandInjectionTest.phpexits 0.Also in this PR
unserialize()on the stored credential blob now passesallowed_classes => false.|+|was interpolated into--delimiter=unquoted, soexec()split the command into a shell pipeline (exit 127, no data). Now quoted for the shell while the property stays raw for theexplode()infetch().Closes #5 (restrict unserialize and audit exec command paths).
Closes #19