security: escape stored WMI output, bind remaining SQL, scope the augmented role - #14
Open
somethingwithproof wants to merge 2 commits into
Open
Conversation
Escape account/query/tool output on render (html_escape/__esc), bind the remaining WMI queries as prepared statements, and drop wmi_accounts.php and wmi_tools.php from the Template Editor auth augment so credential management and the live query tool stay behind the WMI Management realm. Signed-off-by: Thomas Vincent <thomasvincent@gmail.com>
Signed-off-by: Thomas Vincent <thomasvincent@gmail.com>
This was referenced Aug 30, 2026
There was a problem hiding this comment.
🟡 Changes recommended
There are a few correctness/runtime issues in changed lines (notably an unsafe/invalid RLIKE pattern build and an inconsistent db_fetch_row_prepared() call signature) that should be corrected before approval.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
This PR hardens the WMI plugin’s post-auth surfaces by escaping user/host-sourced values at render time (to prevent stored/reflected XSS), converting remaining string-interpolated SQL to prepared statements, and tightening authorization by scoping Template Editor access away from credential management and live execution tools.
Changes:
- Escape WMI query/account/tool outputs on render (using
html_escape/__esc) to close XSS paths. - Bind remaining SQL queries as prepared statements in polling/script/helper code.
- Restrict Template Editor role augmentation to query definition only (
wmi_queries.php).
File summaries
| File | Description |
|---|---|
| wmi_tools.php | Escapes reflected device/tool output; updates the results header rendering. |
| wmi_queries.php | Escapes stored query fields in list view and edit header. |
| wmi_accounts.php | Escapes account edit header and username list output. |
| setup.php | Narrows Template Editor auth augmentation to wmi_queries.php only. |
| script/wmi-script.php | Converts the query lookup to a prepared statement. |
| poller_wmi.php | Converts process tracking SQL to prepared statements. |
| functions.php | Converts remaining query lookups to prepared statements (incl. regex lookup). |
| CHANGELOG.md | Adds a changelog entry describing the security-related changes. |
Review details
- Files reviewed: 8/8 changed files
- Comments generated: 3
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
|
|
||
| /* Fetch the info for this WMI query from the database, exit if not found */ | ||
| $wmiinfo = db_fetch_row("SELECT * FROM plugin_wmi_queries WHERE queryname = '$wmiquery'", FALSE); | ||
| $wmiinfo = db_fetch_row_prepared('SELECT * FROM plugin_wmi_queries WHERE queryname = ?', array($wmiquery), FALSE); |
| foreach($tokens as $token) { | ||
| if ($next_ic) { | ||
| $exists = db_fetch_cell("SELECT COUNT(*) FROM wmi_wql_queries WHERE query RLIKE '^FROM\s$token$+'"); | ||
| $exists = db_fetch_cell_prepared('SELECT COUNT(*) FROM wmi_wql_queries WHERE query RLIKE ?', array('^FROM\s' . $token . '$+')); |
| print "<table style='width:100%'><tr><td>"; | ||
|
|
||
| print "<h4>" . __('WMI Query Results for Device: %s, Class: %s, Columns: %s, Rows: %s', $host, $namespace, sizeof($indexes), sizeof($data), 'wmi') . "</h4>"; | ||
| print "<h4>" . __esc('WMI Query Results for Device: %s, Class: %s, Columns: %s, Rows: %s', $host, $namespace, sizeof($indexes), sizeof($data), 'wmi') . "</h4>"; |
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.
All post-auth (WMI Management realm). Output-escaping model: values stay raw at rest, escaped on render, so WQL strings are unchanged.
Stored/reflected XSS
SQL
Authorization
No schema, storage-format, or behavioral change. Complements #13 (hostname/namespace escaping in exec).