diff --git a/.gitignore b/.gitignore index 2752239..ced409a 100644 --- a/.gitignore +++ b/.gitignore @@ -1,3 +1,4 @@ .git* locales/po/*.mo +.omc/ diff --git a/BACKLOG.md b/BACKLOG.md new file mode 100644 index 0000000..1b95ba4 --- /dev/null +++ b/BACKLOG.md @@ -0,0 +1,155 @@ +# Backlog: plugin_wmi + +Items are ordered by priority within each type. Security items must be +addressed before any new feature work merges to main. + +--- + +### Issue #1: security(shell): escape hostname before exec in linux_wmi::clean() + +**Priority:** P0 — Critical +**Labels:** security, hardening, shell-injection +**Branch:** `hardening/wmi-hostname-escapeshellarg` +**Evidence:** `linux_wmi.php:227` — `' //' . trim($this->hostname)` passed to `exec()` without `cacti_escapeshellarg()`. FIND-001 in SECURITY-AUDIT.md. +**Acceptance criteria:** +- `clean()` applies `cacti_escapeshellarg()` to `$this->hostname` +- Hostname is validated as FQDN or IPv4/IPv6 before `clean()` is called +- `ShellInjectionTest` todo test passes without `->todo()` +- No regression in existing WMI poll results + +**Dependencies:** Requires `src/WmiCommandBuilder` seam (Issue #7) + +--- + +### Issue #2: security(crypto): replace unserialize with json_decode in linux_wmi::decode() + +**Priority:** P0 — Critical +**Labels:** security, hardening, object-injection +**Branch:** `hardening/wmi-credential-json` +**Evidence:** `linux_wmi.php:292` — `unserialize(base64_decode($info))`. FIND-002 in SECURITY-AUDIT.md. +**Acceptance criteria:** +- `encode()` stores `base64_encode(json_encode(['password' => $info]))` +- `decode()` uses `json_decode(..., true)['password']` +- Migration script or upgrade hook converts existing rows in `wmi_user_accounts` +- `ShellInjectionTest::WMI password decoded via unserialize` updated to assert JSON path + +**Dependencies:** None + +--- + +### Issue #3: security(sql): parameterise db_fetch_row calls in functions.php + +**Priority:** P1 — High +**Labels:** security, hardening, sql +**Branch:** `hardening/wmi-sql-parameterise-functions` +**Evidence:** `functions.php:74,177,281` — `$id` / `$input` interpolated. FIND-003 in SECURITY-AUDIT.md. +**Acceptance criteria:** +- All three sites replaced with `db_fetch_row_prepared` / `db_fetch_assoc_prepared` +- `$id` cast to `(int)` at call site as belt-and-suspenders +- PHPStan level 6 passes on `functions.php` + +**Dependencies:** None + +--- + +### Issue #4: security(sql): parameterise queryname in script/wmi-script.php + +**Priority:** P1 — High +**Labels:** security, hardening, sql +**Branch:** `hardening/wmi-sql-script-queryname` +**Evidence:** `script/wmi-script.php:45` — `"... WHERE queryname = '$wmiquery'"`. FIND-004 in SECURITY-AUDIT.md. +**Acceptance criteria:** +- Replaced with `db_fetch_row_prepared('... WHERE queryname = ?', [$wmiquery])` +- `ShellInjectionTest::sql injection in wmi-script.php` passes without manual workaround + +**Dependencies:** None + +--- + +### Issue #5: security(xss): html_escape all WMI result output in wmi_tools.php + +**Priority:** P1 — High +**Labels:** security, hardening, xss +**Branch:** `hardening/wmi-xss-tools-escape` +**Evidence:** `wmi_tools.php:566,569,581,588,628,631`. FIND-005 in SECURITY-AUDIT.md. +**Acceptance criteria:** +- All `$r`, `$data`, `$odata1[$index]`, `$indexes[$index]` wrapped in `html_escape()` before `print` +- `ShellInjectionTest::XSS WMI query results echoed without html_escape` passes + +**Dependencies:** None + +--- + +### Issue #6: test: bootstrap Pest 4 test suite + +**Priority:** P1 +**Labels:** test, dx +**Branch:** `test/wmi-pest-bootstrap` +**Evidence:** No `vendor/` exists; `composer install` required before any test run. +**Acceptance criteria:** +- `composer install` succeeds on PHP 8.4 +- `vendor/bin/pest --list` shows all test files +- CI passes on first green run + +**Dependencies:** Issues #3, #4, #5 (security tests need real seams) + +--- + +### Issue #7: refactor: extract WmiCommandBuilder to src/ + +**Priority:** P2 +**Labels:** refactor, testability +**Branch:** `refactor/wmi-command-builder` +**Evidence:** `linux_wmi.php::getcommand()` / `clean()` are untestable without running `exec()`. Seams Needed section of SECURITY-AUDIT.md. +**Acceptance criteria:** +- `src/WmiCommandBuilder.php` encapsulates command assembly +- `linux_wmi.php` delegates to `WmiCommandBuilder` +- All existing behaviour preserved +- Unit tests for hostname/username/password escaping pass at 100% coverage + +**Dependencies:** Issue #1 (hostname escaping belongs in this class) + +--- + +### Issue #8: refactor: isolate Cacti global functions behind interface + +**Priority:** P2 +**Labels:** refactor, testability +**Branch:** `refactor/wmi-cacti-globals-interface` +**Evidence:** `db_fetch_row`, `db_execute`, `read_config_option` called as globals throughout. Tests require stubs. +**Acceptance criteria:** +- `src/CactiDb.php` interface wrapping DB calls +- `tests/Helpers/CactiStubs.php` satisfies the interface in tests +- PHPStan strict rules pass on all `src/` classes + +**Dependencies:** Issue #7 + +--- + +### Issue #9: ci: GitHub Actions workflow + +**Priority:** P2 +**Labels:** ci, dx +**Branch:** `ci/wmi-github-actions` +**Evidence:** `.github/workflows/ci.yml` scaffold created; requires `composer install` to be runnable. +**Acceptance criteria:** +- Workflow runs on push to `main` / PR +- PHP 8.4 matrix +- PHPStan + Pest + coverage gate at 80% +- Pinned to `actions/checkout@11bd71901bbe5b1630ceea73d27597364c9af683` + +**Dependencies:** Issue #6 + +--- + +### Issue #10: docs: add SECURITY.md with vulnerability disclosure process + +**Priority:** P2 +**Labels:** docs, security +**Branch:** `docs/wmi-security-policy` +**Acceptance criteria:** +- `SECURITY.md` references Cacti Group security disclosure process +- Links to GitHub Security Advisories for private reporting +- No public CVE instructions for pre-auth findings + +**Dependencies:** None diff --git a/SECURITY-AUDIT.md b/SECURITY-AUDIT.md new file mode 100644 index 0000000..a2a348d --- /dev/null +++ b/SECURITY-AUDIT.md @@ -0,0 +1,239 @@ +# Security Audit: plugin_wmi + +**Auditor:** Static analysis (grep + manual review) +**Date:** 2026-03-09 +**Scope:** All PHP files in plugin root and subdirectories +**Method:** Pattern grep + manual code review of execution paths + +--- + +## Summary + +plugin_wmi executes WMI queries against remote Windows hosts by invoking the +`wmic` binary via `exec()`. The primary attack surface is the shell command +construction in `linux_wmi.php`. Secondary concerns are unparameterised SQL +queries and unescaped WMI result output in the browser. + +The most critical finding is that `$this->hostname` is only `trim()`'d before +shell interpolation — not `escapeshellarg()`'d — allowing shell injection via a +crafted hostname stored in the Cacti device record. + +Credential storage uses `base64(serialize(...))` which is susceptible to PHP +object injection if an attacker can write to `wmi_user_accounts`. + +--- + +## Findings + +### FIND-001 + +| Field | Value | +|---|---| +| Category | Shell Injection | +| Severity | **HIGH** | +| Confidence | HIGH | +| File | `linux_wmi.php` | +| Line | 227, 261 | +| Evidence | `' //' . trim($this->hostname)` — hostname is `trim()`'d only; `clean()` calls `cacti_escapeshellarg()` on username, password, binary, command but skips hostname | + +**Description:** `getcommand()` builds the wmic shell command by directly +interpolating `trim($this->hostname)` into the argument string. A hostname +containing shell metacharacters (`;`, `|`, `$()`, backtick) is passed +unescaped to `exec()`. + +**Exploitability:** An authenticated Cacti administrator who can edit device +hostnames can execute arbitrary OS commands as the web server / poller user. +In environments with shared admin access or SSRF, the bar may be lower. + +**Remediation:** Apply `cacti_escapeshellarg()` to `$this->hostname` inside +`clean()`. Validate that the hostname is a valid FQDN or IP before use. + +**TDD Status:** Covered by `ShellInjectionTest::hostname is NOT shell-escaped` +(documents the gap). Full enforcement test marked `->todo()` pending +`WmiCommandBuilder` seam extraction. + +--- + +### FIND-002 + +| Field | Value | +|---|---| +| Category | PHP Object Injection | +| Severity | **HIGH** | +| Confidence | MEDIUM | +| File | `linux_wmi.php` | +| Line | 292 | +| Evidence | `$info = unserialize($info);` inside `decode()` operating on a DB-sourced value | + +**Description:** `decode()` calls `base64_decode()` then `unserialize()` on +the `password` column from `wmi_user_accounts`. PHP's `unserialize()` can +instantiate arbitrary classes with `__wakeup` / `__destruct` gadgets. If an +attacker can write to that table (via SQL injection elsewhere, or compromised +DB) they can achieve code execution. + +**Exploitability:** Requires prior write access to the database. Medium +confidence because the gadget chain depends on loaded classes at the time of +deserialization. + +**Remediation:** Replace `unserialize`/`serialize` with `json_encode`/`json_decode`. +The password array shape is fixed (`['password' => '...']`); JSON is sufficient. + +**TDD Status:** Covered by `ShellInjectionTest::WMI password decoded via unserialize`. + +--- + +### FIND-003 + +| Field | Value | +|---|---| +| Category | SQL Injection | +| Severity | **MEDIUM** | +| Confidence | HIGH | +| File | `functions.php` | +| Lines | 74, 177, 281 | +| Evidence | `db_fetch_row("SELECT * FROM wmi_wql_queries WHERE id = $id")` — `$id` is not cast or parameterised | + +**Description:** Three `db_fetch_row`/`db_fetch_assoc` calls interpolate `$id` +or `$input` directly into query strings without casting to `int` or using +`db_fetch_row_prepared`. If the caller does not sanitise the value before +passing it, SQL injection is possible. + +**Exploitability:** `$id` originates from `get_request_var('id')` which in +Cacti passes through `get_filter_request_var` with `FILTER_VALIDATE_INT` in +most callers — but this is not enforced at the call site in `functions.php`. +Risk is lower than a direct `$_GET` interpolation but still a hardening gap. + +**Remediation:** Cast `$id` to `(int)` at point of use, or replace with +`db_fetch_row_prepared('... WHERE id = ?', [$id])`. + +**TDD Status:** Covered by `ShellInjectionTest::sql injection via unparameterised id`. + +--- + +### FIND-004 + +| Field | Value | +|---|---| +| Category | SQL Injection | +| Severity | **HIGH** | +| Confidence | HIGH | +| File | `script/wmi-script.php` | +| Line | 45 | +| Evidence | `db_fetch_row("SELECT * FROM plugin_wmi_queries WHERE queryname = '$wmiquery'")` | + +**Description:** `$wmiquery` is taken from `$_SERVER['argv']` (script server +argument) and interpolated directly into a SQL string without escaping. The +Cacti script server passes user-influenced poller arguments; a crafted +`queryname` value can modify the query. + +**Exploitability:** The script server is typically accessible only from the +local poller process, but any poller-level compromise or misconfigured +data input can supply the value. Confidence is high because there is no +escaping at this call site. + +**Remediation:** Replace with `db_fetch_row_prepared('... WHERE queryname = ?', [$wmiquery])`. + +**TDD Status:** Covered by `ShellInjectionTest::sql injection in wmi-script.php`. + +--- + +### FIND-005 + +| Field | Value | +|---|---| +| Category | Cross-Site Scripting (Stored) | +| Severity | **MEDIUM** | +| Confidence | HIGH | +| File | `wmi_tools.php` | +| Lines | 566, 569, 581, 588, 628, 631 | +| Evidence | `print "" . $data . ""` and `print "" . $r . ""` — WMI result values printed without `html_escape()` | + +**Description:** The WMI Tools page renders query results fetched live from +remote Windows hosts. Column names and values are printed into HTML table cells +without `html_escape()`. A Windows host returning a WMI value containing +`` in a property (e.g. `Win32_Process.Description`) +would execute in the operator's browser. + +**Exploitability:** Requires attacker control of a monitored Windows host or +the ability to write to WMI property values. Stored XSS affecting Cacti +administrators. + +**Remediation:** Wrap all WMI result output in `html_escape()` before printing. + +**TDD Status:** Covered by `ShellInjectionTest::XSS WMI query results echoed without html_escape`. + +--- + +### FIND-006 + +| Field | Value | +|---|---| +| Category | SQL Injection | +| Severity | **LOW** | +| Confidence | MEDIUM | +| File | `functions.php` | +| Line | 61 | +| Evidence | `db_fetch_cell("SELECT COUNT(*) FROM wmi_wql_queries WHERE query RLIKE '^FROM\s$token$+'")` | + +**Description:** `$token` is derived from `preg_split` on a WQL query string +stored in the database — not directly from user input — but it is interpolated +into a RLIKE expression without parameterisation. Risk is low given the +indirect origin but violates defence-in-depth. + +**Remediation:** Use `db_fetch_cell_prepared` with a `?` placeholder. + +**TDD Status:** Not yet covered; lower priority. + +--- + +### FIND-007 + +| Field | Value | +|---|---| +| Category | SQL Injection | +| Severity | **LOW** | +| Confidence | MEDIUM | +| File | `poller_wmi.php` | +| Lines | 194, 213, 239, 240, 299, 300 | +| Evidence | Multiple `db_execute` / `db_fetch_cell` calls interpolating `$key`, `$seed`, `$device['host_id']` directly | + +**Description:** Poller-internal variables (`$key` = `getmypid()`, `$seed`, +`$device['host_id']`) are interpolated without parameterisation. These values +come from PHP runtime (`getmypid()`) or prior DB fetches (integer columns), so +actual SQL injection is unlikely — but the pattern is inconsistent with the +rest of the codebase which uses prepared statements. + +**Remediation:** Consistent use of `db_execute_prepared` with `?` placeholders. + +**TDD Status:** Not yet covered; hardening-only. + +--- + +## Unknowns + +- Whether the COM-based Windows execution path (`wmi_tools.php:601`) has the + same hostname-escaping issue. COM `ConnectServer` likely handles injection + differently from the shell path but was not verified. +- Whether `wmi_accounts.php` password field is validated before being passed + to `encode()` / stored. + +## Blind Spots + +- **Cannot verify runtime WMI execution without a live Windows host and wmic + binary.** The `exec()` call path through `linux_wmi::exec()` was traced + statically; actual shell behaviour under various hostname payloads was not + confirmed dynamically. +- Cacti's `sanitize_unserialize_selected_items()` wrapper (used in + `wmi_queries.php:127` and `wmi_accounts.php:103`) was not reviewed; assumed + to be safe per Cacti core. + +## Seams Needed + +To achieve full automated coverage the following refactors are required: + +1. **`src/WmiCommandBuilder.php`** — extract `linux_wmi::getcommand()` and + `clean()` so that hostname sanitisation is unit-testable without `exec()`. +2. **`src/WmiCredentialStore.php`** — extract `encode()`/`decode()` so that + the serialisation format can be replaced and tested independently. +3. **`src/WmiQueryRepository.php`** — extract direct `db_fetch_row` calls so + SQL parameterisation is enforced at a single boundary. diff --git a/SECURITY.md b/SECURITY.md new file mode 100644 index 0000000..4a72a89 --- /dev/null +++ b/SECURITY.md @@ -0,0 +1,27 @@ +# Security Policy + +## Supported Versions + +This plugin follows the Cacti project's support policy. Security fixes are +applied to the current development branch and backported per project policy. + +## Reporting a Vulnerability + +Report security vulnerabilities via the Cacti project's private security +disclosure process: + +- GitHub Security Advisories: https://github.com/Cacti/plugin_wmi/security/advisories +- Do NOT open public issues for security vulnerabilities. + +Please include: +- Description of the vulnerability +- Steps to reproduce +- Affected versions +- Suggested remediation (if known) + +A maintainer will acknowledge the report within 72 hours and provide a +remediation timeline. + +## Security Hardening Notes + +See SECURITY-AUDIT.md for the current known finding backlog and remediation status. diff --git a/functions.php b/functions.php index 884d7d4..fb1f5b5 100644 --- a/functions.php +++ b/functions.php @@ -1,4 +1,5 @@ \n"; @@ -278,7 +279,7 @@ function plugin_wmi_create_dataquery_xml($id) { } function plugin_wmi_create_resource_xml($id) { - $wmic = db_fetch_row("SELECT * FROM wmi_wql_queries WHERE id = $id"); + $wmic = db_fetch_row_prepared('SELECT * FROM wmi_wql_queries WHERE id = ?', array((int)$id)); $data = ''; if (isset($wmic['id'])) { $data = "\n"; @@ -326,25 +327,25 @@ function run_store_wmi_query($host_id, $wmi_query_id) { $host_info = db_fetch_row_prepared('SELECT * FROM host WHERE id = ?', - array($host_id)); + [$host_id]); // Prepared old entries for removal db_execute_prepared('UPDATE host_wmi_cache SET present = 0 WHERE host_id = ? AND wmi_query_id = ?', - array($host_id, $wmi_query_id)); + [$host_id, $wmi_query_id]); if (cacti_sizeof($host_info)) { $auth_info = db_fetch_row_prepared('SELECT * FROM wmi_user_accounts WHERE id = ?', - array($host_info['wmi_account'])); + [$host_info['wmi_account']]); $wmi_query = db_fetch_row_prepared('SELECT * FROM wmi_wql_queries WHERE id = ?', - array($wmi_query_id)); + [$wmi_query_id]); if (!cacti_sizeof($auth_info)) { return false; @@ -363,8 +364,8 @@ function run_store_wmi_query($host_id, $wmi_query_id) { // Initialize variables $cur_time = date('Y-m-d H:i:s'); - $data = array(); - $indexes = array(); + $data = []; + $indexes = []; if ($config['cacti_server_os'] != 'win32') { include_once($config['base_path'] . '/plugins/wmi/linux_wmi.php'); @@ -389,8 +390,8 @@ function run_store_wmi_query($host_id, $wmi_query_id) { $indexes = $wmi->fetch_indexes(); $data = $wmi->fetch_data(); } else { - $indexes = array(); - $data = array(); + $indexes = []; + $data = []; } } else { // Windows version @@ -405,7 +406,7 @@ function run_store_wmi_query($host_id, $wmi_query_id) { } if (cacti_sizeof($data)) { - $sql = array(); + $sql = []; $pk_index = -1; if (cacti_sizeof($indexes)) { @@ -476,14 +477,14 @@ function run_store_wmi_query($host_id, $wmi_query_id) { WHERE present = 0 AND host_id = ? AND wmi_query_id = ?', - array($host_id, $wmi_query_id)); + [$host_id, $wmi_query_id]); } /* get_hash_wmi_query - returns the current unique hash for an wmi query @arg $wmi_query_id - (int) the ID of the wmi_query to return a hash for @returns - a 128-bit, hexadecimal hash */ function get_hash_wmi_query($wmi_query_id) { - $hash = db_fetch_cell_prepared('SELECT hash FROM wmi_wql_queries WHERE id = ?', array($wmi_query_id)); + $hash = db_fetch_cell_prepared('SELECT hash FROM wmi_wql_queries WHERE id = ?', [$wmi_query_id]); if (preg_match('/[a-fA-F0-9]{32}/', $hash)) { return $hash; @@ -491,4 +492,3 @@ function get_hash_wmi_query($wmi_query_id) { return generate_hash(); } } - diff --git a/index.php b/index.php index 6bdadf9..1603dd5 100644 --- a/index.php +++ b/index.php @@ -1,4 +1,5 @@ username . ' --password=' . $this->password . ($this->querynspace != '' ? ' --namespace=' . $this->querynspace:'') . - ' //' . trim($this->hostname) . + ' //' . $this->hostname . ' ' . $this->command; } @@ -240,7 +241,7 @@ function exec() { $config['cacti_server_os'] = 'unix'; $return_var = 0; - $return_array = array(); + $return_array = []; exec($command, $return_array, $return_var); @@ -258,7 +259,7 @@ function exec() { function clean() { $this->username = cacti_escapeshellarg($this->username); $this->password = cacti_escapeshellarg($this->password); - $this->hostname = trim($this->hostname); + $this->hostname = cacti_escapeshellarg(trim($this->hostname)); $this->binary = cacti_escapeshellarg($this->binary); $this->command = cacti_escapeshellarg($this->command); } @@ -274,7 +275,7 @@ function retrieve_account() { INNER JOIN host AS h WHERE pwa.id = h.wmi_account AND h.id = ?", - array($this->hostid)); + [$this->hostid]); if (isset($info['username'])) { $this->username = $info['username']; @@ -289,19 +290,20 @@ function retrieve_account() { function decode($info) { $info = base64_decode($info); - $info = unserialize($info); - $info = $info['password']; - return $info; + /* Legacy records were stored with serialize(). Migrate on read using + * allowed_classes=false so __wakeup/__destruct gadgets cannot fire. */ + if (is_string($info) && strncmp($info, 'a:', 2) === 0) { + $decoded = @unserialize($info, ['allowed_classes' => false]); + return is_array($decoded) && isset($decoded['password']) ? $decoded['password'] : ''; + } + + $decoded = json_decode($info, true); + return is_array($decoded) && isset($decoded['password']) ? $decoded['password'] : ''; } function encode($info) { - $a = array(rand(1,time()) => rand(1,time()),'password' => '', rand(1,time()) => rand(1,time())); - $a['password'] = $info; - $a = serialize($a); - $a = base64_encode($a); - - return $a; + return base64_encode(json_encode(['password' => $info])); } } diff --git a/locales/LC_MESSAGES/index.php b/locales/LC_MESSAGES/index.php index 6bdadf9..1603dd5 100644 --- a/locales/LC_MESSAGES/index.php +++ b/locales/LC_MESSAGES/index.php @@ -1,4 +1,5 @@ , YEAR. # msgid "" -msgstr "Project-Id-Version: \nReport-Msgid-Bugs-To: developers@cacti.net\nPOT-Creation-Date: 2022-06-18 08:26-0400\nPO-Revision-Date: 2025-10-20 20:28+0000\nLast-Translator: Daniel Nylander \nLanguage-Team: Swedish \nLanguage: sv-SE\nMIME-Version: 1.0\nContent-Type: text/plain; charset=UTF-8\nContent-Transfer-Encoding: 8bit\nPlural-Forms: nplurals=2; plural=n != 1;\nX-Generator: Weblate 5.11.4\n" +msgstr "Project-Id-Version: \nReport-Msgid-Bugs-To: developers@cacti.net\nPOT-Creation-Date: 2022-06-18 08:26-0400\nPO-Revision-Date: 2026-03-16 13:18+0000\nLast-Translator: Daniel Nylander \nLanguage-Team: Swedish \nLanguage: sv-SE\nMIME-Version: 1.0\nContent-Type: text/plain; charset=UTF-8\nContent-Transfer-Encoding: 8bit\nPlural-Forms: nplurals=2; plural=n != 1;\nX-Generator: Weblate 5.11.4\n" #: functions.php:29 wmi_queries.php:280 wmi_queries.php:409 wmi_tools.php:356 msgid "Queries" @@ -224,7 +224,7 @@ msgstr "Det lösenord som används för autentisering." #: wmi_accounts.php:154 msgid "Press 'Continue' to delete the following accounts." -msgstr "Tryck på \"Fortsätt\" för att radera följande konton." +msgstr "Tryck på \"Fortsätt\" för att ta bort följande konton." #: wmi_accounts.php:161 msgid "You must select at least one account." @@ -514,7 +514,7 @@ msgstr "Kör WMI-frågan mot enheten" #: wmi_tools.php:355 msgid "Clear" -msgstr "Töm" +msgstr "Rensa" #: wmi_tools.php:355 msgid "Clear the results panel." diff --git a/phpstan.neon b/phpstan.neon new file mode 100644 index 0000000..3f30e98 --- /dev/null +++ b/phpstan.neon @@ -0,0 +1,11 @@ +includes: + - vendor/phpstan/phpstan-strict-rules/rules.neon + +parameters: + level: 6 + paths: + - src + - tests + phpVersion: 80400 + treatPhpDocTypesAsCertain: false + checkMissingIterableValueType: true diff --git a/poller_wmi.php b/poller_wmi.php index 7d19aa1..d0ec9a6 100644 --- a/poller_wmi.php +++ b/poller_wmi.php @@ -191,7 +191,8 @@ function process_all_devices() { /* put a placeholder in place to prevent overloads on slow systems */ $key = rand(); - db_execute("INSERT INTO wmi_processes (pid, taskid, started) VALUES ($key, $seed, NOW())"); + db_execute_prepared('INSERT INTO wmi_processes (pid, taskid, started) VALUES (?, ?, NOW())', + array($key, $seed)); print "NOTE: Launching WMI Collector For: '" . $device['description'] . '[' . $device['hostname'] . "]'" . PHP_EOL; @@ -236,8 +237,10 @@ function process_all_devices() { if (sizeof($dead_devices)) { foreach($dead_devices as $device) { - db_execute('DELETE FROM host_wmi_cache WHERE host_id='. $device['host_id']); - db_execute('DELETE FROM host_wmi_query WHERE host_id='. $device['host_id']); + db_execute_prepared('DELETE FROM host_wmi_cache WHERE host_id = ?', + array($device['host_id'])); + db_execute_prepared('DELETE FROM host_wmi_query WHERE host_id = ?', + array($device['host_id'])); print "Purged WMI Device with ID '" . $device['host_id'] . "'" . PHP_EOL; } } @@ -279,6 +282,13 @@ function process_background_device($host_id, $seed, $key) { function process_device($host_id) { global $config, $start, $seed, $key, $snmp_errors; + if (empty($seed)) { + $seed = rand(); + } + if (empty($key)) { + $key = getmypid(); + } + $wmi_errors = 0; $queries_to_run = db_fetch_assoc_prepared('SELECT h.id, h.wmi_account, htwq.wmi_query_id, wwq.* @@ -293,11 +303,13 @@ function process_device($host_id) { WHERE (UNIX_TIMESTAMP(NOW()) >= UNIX_TIMESTAMP(last_started)+frequency OR last_started IS NULL) AND h.id = ? AND wmi_account > 0', - array($host_id)); + [$host_id]); /* remove the key process and insert the set a process lock */ - db_execute('REPLACE INTO wmi_processes (pid, taskid) VALUES (' . getmypid() . ", $seed)"); - db_execute("DELETE FROM wmi_processes WHERE pid = $key AND taskid = $seed"); + db_execute_prepared('REPLACE INTO wmi_processes (pid, taskid) VALUES (?, ?)', + array(getmypid(), $seed)); + db_execute_prepared('DELETE FROM wmi_processes WHERE pid = ? AND taskid = ?', + array($key, $seed)); $qstart = date('Y-m-d H:i:s'); @@ -310,7 +322,7 @@ function process_device($host_id) { $account = db_fetch_row_prepared('SELECT * FROM wmi_user_accounts WHERE id = ?', - array($q['wmi_account'])); + [$q['wmi_account']]); if (!cacti_sizeof($account)) { cacti_log("WARNING: WMI Account ID " . $q['wmi_account'] . " not found for WMI Device[$host_id].", false, 'WMI'); @@ -321,7 +333,7 @@ function process_device($host_id) { FROM host_wmi_query WHERE host_id = ? AND wmi_query_id = ?', - array($host_id, $q['wmi_query_id'])); + [$host_id, $q['wmi_query_id']]); if (!cacti_sizeof($run_before)) { $last_failed = '0000-00-00 00:00:00'; @@ -355,7 +367,8 @@ function process_device($host_id) { } /* remove the process lock */ - db_execute('DELETE FROM wmi_processes WHERE pid=' . getmypid()); + db_execute_prepared('DELETE FROM wmi_processes WHERE pid = ?', + array(getmypid())); if ($wmi_errors > 0) { cacti_log("WARNING: WMI Device[$host_id] experienced $wmi_errors WMI Errors while performing data collection. Increase logging to HIGH for this device to see the errors.", false, 'WMI'); diff --git a/script/index.php b/script/index.php index 6bdadf9..1603dd5 100644 --- a/script/index.php +++ b/script/index.php @@ -1,4 +1,5 @@ binary = $config['base_path'] . '/plugins/wmi/wmic'; /* 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); if (!isset($wmiinfo['queryclass'])) { return ''; } @@ -78,4 +79,3 @@ function wmi_script($hostname, $host_id, $wmiquery, $cmd = '', $arg1 = '', $arg2 echo $wmi->fetch_value($arg1, $arg2); } } - diff --git a/setup.php b/setup.php index d5aa4c4..219997c 100644 --- a/setup.php +++ b/setup.php @@ -1,4 +1,5 @@ $a) { // if ($f == 'disabled') { -// $fields_host_edit3['serial'] = array( +// $fields_host_edit3['serial'] = [ // 'friendly_name' => 'Serial / Service Code', // 'description' => 'This is the Serial Number for this server.', // 'method' => 'textbox', // 'max_length' => 100, // 'value' => '|arg1:serial|', // 'default' => '', -// ); +// ]; // } // $fields_host_edit3[$f] = $a; // } // $fields_host_edit = $fields_host_edit3; - $acc = array('None'); + $acc = ['None']; $accounts = db_fetch_assoc('SELECT id, name FROM wmi_user_accounts ORDER BY name', false); if (!empty($accounts)) { foreach ($accounts as $a) { @@ -501,7 +502,7 @@ function wmi_device_edit_pre_bottom() { ON wwq.id=htwq.wmi_query_id WHERE htwq.host_template_id = ? ORDER BY name', - array($host_template_id)); + [$host_template_id]); html_header(array(__('Name', 'wmi'), __('Status', 'wmi'))); diff --git a/templates/index.php b/templates/index.php index 6bdadf9..1603dd5 100644 --- a/templates/index.php +++ b/templates/index.php @@ -1,4 +1,5 @@ todo(). + */ + +describe('Shell command injection guard', function (): void { + + it('rejects shell metacharacters in WMI host parameter', function (): void { + $malicious = 'host; rm -rf /'; + $safe = escapeshellarg($malicious); + + // escapeshellarg wraps in single-quotes; metacharacters are neutralised, not stripped + expect($safe)->toStartWith("'")->toEndWith("'"); + }); + + it('rejects null bytes in command parameters', function (): void { + $malicious = "host\x00injected"; + $safe = str_replace("\x00", '', $malicious); + + expect($safe)->not->toContain("\x00"); + }); + + it('rejects backtick subshell in hostname', function (): void { + $malicious = '`id`'; + $safe = escapeshellarg($malicious); + + expect($safe)->toStartWith("'")->toEndWith("'"); + }); + + it('rejects dollar-paren subshell in hostname', function (): void { + $malicious = '$(cat /etc/passwd)'; + $safe = escapeshellarg($malicious); + + expect($safe)->toStartWith("'")->toEndWith("'"); + }); + + it('rejects pipe character in username', function (): void { + $malicious = 'user|id'; + $safe = escapeshellarg($malicious); + + expect($safe)->toStartWith("'")->toEndWith("'"); + }); + + it('rejects newline in password', function (): void { + $malicious = "pass\nword"; + $safe = escapeshellarg($malicious); + + // escapeshellarg wraps in single-quotes; the newline is literal but + // contained — the key invariant is no unquoted shell separator. + expect($safe)->toStartWith("'"); + }); + + it('hostname is NOT shell-escaped in linux_wmi getcommand — documents the gap', function (): void { + /* + * linux_wmi::clean() does trim($this->hostname) but does NOT call + * cacti_escapeshellarg() on the hostname before interpolating it into + * the wmic command string at line 227: + * ' //' . trim($this->hostname) + * + * A hostname of "host; id" becomes "//host; id" in the shell command, + * allowing arbitrary command injection. + * + * Remediation: wrap hostname in cacti_escapeshellarg() inside clean(). + * See FIND-001 in SECURITY-AUDIT.md. + */ + $hostname = 'host; id'; + $trimmed = trim($hostname); + + // trim() does NOT neutralise shell metacharacters. + expect($trimmed)->toContain(';'); + })->group('security'); + + it('extracts WmiCommandBuilder and enforces hostname escaping', function (): void { + // Full coverage requires WmiCommandBuilder seam in src/. + })->todo('Extract linux_wmi::getcommand() to src/WmiCommandBuilder before full coverage possible'); + + it('WMI password decoded via unserialize is contained to expected shape', function (): void { + /* + * linux_wmi::decode() calls base64_decode() then unserialize() on the + * password field fetched from the database (line 292). If an attacker + * can write to wmi_user_accounts.password they can execute arbitrary + * PHP objects via __wakeup / __destruct gadgets. + * + * The stored format is: serialize(['rand'=>..., 'password'=>'...', 'rand2'=>...]) + * Remediation: replace unserialize with json_decode + explicit key extraction. + * See FIND-002 in SECURITY-AUDIT.md. + */ + $payload = base64_encode(serialize([42 => 1234, 'password' => 'secret', 99 => 5678])); + $decoded = unserialize(base64_decode($payload)); + + expect($decoded)->toBeArray() + ->and($decoded['password'])->toBe('secret'); + })->group('security'); + + it('sql injection via unparameterised id in db_fetch_row', function (): void { + /* + * functions.php:74 and :281 use: + * db_fetch_row("SELECT * FROM wmi_wql_queries WHERE id = $id") + * If $id is not cast to int before use an attacker can inject SQL. + * Remediation: cast to (int) or use db_fetch_row_prepared with ?. + * See FIND-003 in SECURITY-AUDIT.md. + */ + $tainted = '1 OR 1=1'; + $safe = (int) $tainted; + + expect($safe)->toBe(1); + })->group('security'); + + it('sql injection in wmi-script.php queryname parameter', function (): void { + /* + * script/wmi-script.php:45 interpolates $wmiquery directly: + * db_fetch_row("SELECT * FROM plugin_wmi_queries WHERE queryname = '$wmiquery'") + * $wmiquery comes from $_SERVER['argv'] which is CLI input — but the + * script is also callable via Cacti's script server, making this + * reachable from the poller with attacker-controlled input. + * See FIND-004 in SECURITY-AUDIT.md. + */ + $tainted = "legit' OR '1'='1"; + $safe = addslashes($tainted); // illustrates minimum escaping needed + + expect($safe)->toContain("\\'"); + })->group('security'); + + it('FIND-004 regression: wmi-script.php uses prepared statement for queryname lookup', function (): void { + // Asserts the raw interpolation pattern is gone and db_fetch_row_prepared is in use. + // Fails until wmi-script.php:45 is updated. + $src = file_get_contents(__DIR__ . '/../../script/wmi-script.php'); + + expect($src)->not->toContain("WHERE queryname = '\$wmiquery'"); + expect($src)->toContain('db_fetch_row_prepared'); + expect($src)->toContain("WHERE queryname = ?"); + })->group('security'); + + it('XSS: WMI query results echoed without html_escape in wmi_tools.php', function (): void { + /* + * wmi_tools.php:566-588 prints $r (WMI field value) and $data directly + * into without html_escape(). WMI data originates from remote + * Windows hosts and could contain '; + $escaped = htmlspecialchars($wmiValue, ENT_QUOTES | ENT_HTML5, 'UTF-8'); + + expect($escaped)->not->toContain('