From 75d816a3352cbe71cf07a67531d036a5095bb69e Mon Sep 17 00:00:00 2001 From: TheWitness Date: Sat, 3 Oct 2026 14:20:33 -0400 Subject: [PATCH] security: consolidate GHSA batch 11 fixes for develop (fq9x, hh7r, v99q, f5f5) (#8165) * security: consolidate GHSA batch 11 fixes for develop (fq9x, hh7r, v99q, f5f5) Mirrors the 1.2.x batch-11 security consolidation. Items already fixed on the develop tip are credited on their advisories and omitted here: - GHSA-54hx-hxc4-jq7j: TICK/SHIFT values already pass through strip_alpha(). - GHSA-hh7r CSV portion: already guarded via cacti_csv_cell(); only the Content-Disposition filename needed sanitizing (included below). - GHSA-mx5c: REST scaffold already deny-by-default (404 unless api_enabled), enforced by tests/Unit/ApiDenyByDefaultTest.php. - GHSA-rjvj/pjq5/fmhq/mfw8: already addressed on develop. Fixes applied here: - GHSA-fq9x-x3vf-3vf2 (High): add substitute_script_path() single-pass token substitution and refactor get_full_script_path()/get_full_test_script_path() to use a pre-escaped value map, closing the second-order re-substitution OS command injection. - GHSA-hh7r-464j-rf39: sanitize the Content-Disposition filename in graph_xport.php. - GHSA-v99q-57w7-mpmg: escape the page title rendered in the HTML . - GHSA-f5f5-h85v-6m3h: render user admin tooltip content as escaped text. * security: add batch-11 consolidation regression tests for develop Addresses PR #8165 review feedback by adding regression coverage for all four fixed sinks: - GHSA-fq9x: behavioral single-pass proof that substitute_script_path() does not re-substitute a field value containing another field token, plus source guards that both path builders use the helper. - GHSA-hh7r: Content-Disposition filename strips CR/LF/quote/backslash. - GHSA-v99q: the HTML <title> is html_escape()'d. - GHSA-f5f5: both the user_admin.php and user_group_admin.php tooltips render data-tooltip as escaped text. * style: apply php-cs-fixer formatting to substitute_script_path (CI coding standards) * docs: require composer run-script php-cs-fixit before creating any pull request * docs: require PHPStan (level 8, satisfies Level 6 floor) to pass before creating any pull request --------- Co-authored-by: Larry Adams <TheWitness@users.noreply.github.com> --- .github/copilot-instructions.md | 4 + CHANGELOG | 4 + graph_xport.php | 6 +- lib/functions.php | 61 ++++++++-- lib/html.php | 2 +- .../Batch11SecurityRegressionTest.php | 113 ++++++++++++++++++ user_admin.php | 4 +- user_group_admin.php | 4 +- 8 files changed, 185 insertions(+), 13 deletions(-) create mode 100644 tests/Unit/Security/Batch11SecurityRegressionTest.php diff --git a/.github/copilot-instructions.md b/.github/copilot-instructions.md index 6f5ac6f2f..1bee420fa 100644 --- a/.github/copilot-instructions.md +++ b/.github/copilot-instructions.md @@ -69,6 +69,10 @@ - Use `cacti_redirect($path, $params)` for local redirects; it rejects absolute and protocol-relative destinations (checking both raw and percent-decoded forms) and terminates the request. - Escape output for HTML attribute contexts with `html_escape_attr()`, not `htmle()`/`html_escape()`; only the attribute helper double-encodes to stop pre-encoded entities from breaking out of the attribute. +## Pull request pre-flight (MANDATORY) +- **Before creating any pull request you MUST run `composer run-script php-cs-fixit`** to apply the repo's `php-cs-fixer` formatting rules, then commit any resulting changes. This script fixes in place (unlike the dry-run `composer phpcsfixer`), so creating a PR without running it is what trips the coding-standards job in CI. Run it from WSL (see Windows/WSL note below). Do not open the PR until the working tree is clean after the fixer runs. +- **Every commit must also pass PHPStan static analysis.** Run `composer run-script phpstan -- --memory-limit=2G` from WSL and confirm it reports `No errors` before opening the PR. The default 128M limit crashes a parallel worker, so always pass `--memory-limit=2G` (or higher). The repo config (`.phpstan.neon`) analyses at **level 8**, which is cumulative and therefore satisfies the Level 6 floor; `tests/*` is excluded from analysis, so test-only files will not surface here but production code (e.g. `lib/`) will. + ## Workflows you’ll actually use - **Windows/WSL**: When working in VS Code from a Windows machine, run all fixes, builds, linters, tests, and git operations inside WSL (a Linux distro) rather than native Windows. The toolchain (PHP 8.3, Composer, `php-cs-fixer`, Pest) and the repo's tab indentation / `\n` line-ending conventions are Linux-first; running them on native Windows produces spurious diffs and failures. Edit the WSL-mounted checkout (e.g. under `/mnt/c/...` or a native WSL path) and invoke `composer` scripts such as `composer php-cs-fixit` from the WSL shell. - Install deps: `composer install` (CI validates via `.github/workflows/syntax.yml`). diff --git a/CHANGELOG b/CHANGELOG index e7e49e306..49d1b3673 100644 --- a/CHANGELOG +++ b/CHANGELOG @@ -413,6 +413,10 @@ Cacti CHANGELOG -feature#7681: Add fail2ban support with a stable authentication-failure log line and a bundled filter 1.2.32 +-security#GHSA-fq9x-x3vf-3vf2: Substitute data-input field tokens in a single pass so a composed value cannot re-inject an escaped payload into the script command line +-security#GHSA-hh7r-464j-rf39: Sanitize the Content-Disposition filename in the graph data export +-security#GHSA-v99q-57w7-mpmg: Escape the page title rendered in the HTML title element +-security#GHSA-f5f5-h85v-6m3h: Render user administration tooltip content as escaped text -security#GHSA-p3rg-7pc3-2h86: Dummy-verify passwords for unknown users to close a login-timing enumeration side channel -security#GHSA-92jm-r83q-cjr8: Validate the hgd graph-template id as a number before the tree-view graph SQL filter -security#GHSA-4j2v-vxgp-83ch: Validate the hgd graph-template id for host-leaf Graph Template grouping before the SQL filter diff --git a/graph_xport.php b/graph_xport.php index 0bd96d0b7..f91fcc9b3 100644 --- a/graph_xport.php +++ b/graph_xport.php @@ -122,8 +122,10 @@ if ($xport_array['meta']['step'] > 0) { $xport_array['meta']['missing_rows'] = max(0, $xport_array['meta']['expected_rows'] - $xport_array['meta']['rows']); } -// Make graph title the suggested file name -$filename = $xport_array['meta']['title_cache'] . '.csv'; +// Make graph title the suggested file name; strip characters that could break +// out of the quoted filename in the Content-Disposition header (CR/LF header +// injection, quote escaping). +$filename = str_replace(["\r", "\n", '"', '\\'], '', (string) $xport_array['meta']['title_cache']) . '.csv'; header('Content-type: application/vnd.ms-excel; charset=UTF-8'); header('Content-Transfer-Encoding: binary'); diff --git a/lib/functions.php b/lib/functions.php index db78ff200..ffcce22dd 100644 --- a/lib/functions.php +++ b/lib/functions.php @@ -3167,6 +3167,29 @@ function test_data_source(int $data_template_id, int $host_id, int $snmp_query_i return false; } +/** + * substitute_script_path - performs a single-pass substitution of <field> tokens + * in a data-input command template using pre-escaped values. + * + * Each <name> token in the ORIGINAL template is replaced at most once from the + * supplied map; substituted values are never re-scanned, so a field whose value + * contains another field's <token> cannot splice an already-escaped payload into + * a neighbouring quoted region (second-order breakout, GHSA-fq9x-x3vf-3vf2). + * + * @param string $template The command template containing <field> tokens + * @param array $escaped_values Map of field name => already-escaped value + * + * @return string The template with all known tokens substituted once; unknown + * tokens are left intact for the caller's trailing cleanup to strip + */ +function substitute_script_path(string $template, array $escaped_values) : string { + return preg_replace_callback('/<([A-Za-z0-9_]+)>/', + function (array $matches) use ($escaped_values) : string { + return array_key_exists($matches[1], $escaped_values) ? $escaped_values[$matches[1]] : $matches[0]; + }, + $template) ?? ''; +} + /** * Gets the full path to the script to execute to obtain data for a * given data template for testing. this function does not work on @@ -3207,6 +3230,8 @@ function get_full_test_script_path(int $data_template_id, int $host_id) : mixed $host = db_fetch_row_prepared('SELECT * FROM host WHERE id = ?', [$host_id]); + $escaped_values = []; + if (cacti_sizeof($data) && is_array($host)) { foreach ($data as $item) { if (isset($host[$item['data_name']])) { @@ -3220,13 +3245,22 @@ function get_full_test_script_path(int $data_template_id, int $host_id) : mixed $value = cacti_escapeshellarg_cmd((string) $item['value']); } - $full_path = str_replace('<' . $item['data_name'] . '>', $value, $full_path); + $escaped_values[$item['data_name']] = $value; } } - $search = ['<path_cacti>', '<path_snmpget>', '<path_php_binary>']; - $replace = [CACTI_PATH_BASE, read_config_option('path_snmpget'), read_config_option('path_php_binary')]; - $full_path = str_replace($search, $replace, $full_path); + /* the well-known path tokens resolve to trusted configuration values; field + * names take precedence so behaviour matches the historical field-first order */ + $escaped_values += [ + 'path_cacti' => CACTI_PATH_BASE, + 'path_snmpget' => read_config_option('path_snmpget'), + 'path_php_binary' => read_config_option('path_php_binary'), + ]; + + /* single-pass substitution over the original template prevents a field whose + * value contains another field's <token> from re-injecting an escaped payload + * into an already-quoted region (GHSA-fq9x-x3vf-3vf2) */ + $full_path = substitute_script_path($full_path, $escaped_values); /** * sometimes a certain input value will not have anything entered... null out these fields @@ -3278,6 +3312,8 @@ function get_full_script_path(int $local_data_id) : mixed { $full_path = $data_source['input_string']; + $escaped_values = []; + if (cacti_sizeof($data)) { foreach ($data as $item) { /* only hostname-class fields may legitimately need percent @@ -3289,13 +3325,22 @@ function get_full_script_path(int $local_data_id) : mixed { $value = "''"; } - $full_path = str_replace('<' . $item['data_name'] . '>', $value, $full_path); + $escaped_values[$item['data_name']] = $value; } } - $search = ['<path_cacti>', '<path_snmpget>', '<path_php_binary>']; - $replace = [CACTI_PATH_BASE, read_config_option('path_snmpget'), read_config_option('path_php_binary')]; - $full_path = str_replace($search, $replace, $full_path); + /* the well-known path tokens resolve to trusted configuration values; field + * names take precedence so behaviour matches the historical field-first order */ + $escaped_values += [ + 'path_cacti' => CACTI_PATH_BASE, + 'path_snmpget' => read_config_option('path_snmpget'), + 'path_php_binary' => read_config_option('path_php_binary'), + ]; + + /* single-pass substitution over the original template prevents a field whose + * value contains another field's <token> from re-injecting an escaped payload + * into an already-quoted region (GHSA-fq9x-x3vf-3vf2) */ + $full_path = substitute_script_path($full_path, $escaped_values); /* sometimes a certain input value will not have anything entered... null out these fields in the input string so we don't mess up the script */ diff --git a/lib/html.php b/lib/html.php index 54ea1ee41..4175c0019 100644 --- a/lib/html.php +++ b/lib/html.php @@ -3265,7 +3265,7 @@ function html_common_header(string $title, string $selectedTheme = '') : void { <meta http-equiv="Content-Security-Policy" content="default-src *; img-src 'self' https://api.qrserver.com <?php print $alternates; ?> data: blob:; style-src 'self' 'unsafe-inline' <?php print $alternates; ?>; <?php print $script_src; ?>; worker-src 'self' <?php print $alternates; ?>;"> - <title><?php print $title; ?> + <?php print html_escape($title); ?>