|
| 1 | +# Security Audit: plugin_thold |
| 2 | + |
| 3 | +Audit date: 2026-03-09 |
| 4 | +Auditor: static analysis (grep + manual code review) |
| 5 | +Cacti environment: not available; findings based on source only. |
| 6 | + |
| 7 | +--- |
| 8 | + |
| 9 | +## Findings |
| 10 | + |
| 11 | +### FIND-001: [SQL] Column name interpolation in setup.php |
| 12 | + |
| 13 | +- **Severity**: Medium |
| 14 | +- **Confidence**: High |
| 15 | +- **File**: setup.php |
| 16 | +- **Line**: 345, 350, 386, 391 |
| 17 | +- **Evidence**: |
| 18 | + ``` |
| 19 | + 345: db_fetch_cell_prepared('SELECT thold_' . $matches[1] . ' FROM thold_data WHERE data_template_rrd_id = ?', [$data_template_rrd_id]) |
| 20 | + 350: db_fetch_cell_prepared('SELECT time_' . $matches[1] . ' FROM thold_data WHERE data_template_rrd_id = ?', [$data_template_rrd_id]) |
| 21 | + 386: db_fetch_cell_prepared('SELECT thold_' . $matches[1] . ' FROM thold_data WHERE data_template_rrd_id = ?', [$data_template_rrd_id]) |
| 22 | + 391: db_fetch_cell_prepared('SELECT time_' . $matches[1] . ' FROM thold_data WHERE data_template_rrd_id = ?', [$data_template_rrd_id]) |
| 23 | + ``` |
| 24 | +- **Description**: `$matches[1]` is interpolated directly into the column name portion of the query. The value comes from two regex captures: `/\|thold\:(hi|low)\:/` (captures `hi` or `low`) and `/\|thold\:(warning_hi|warning_low)\:/` (captures `warning_hi` or `warning_low`). PDO prepared statements cannot parameterise column names, so if the regex capture were ever widened or reused elsewhere with a different pattern, SQL injection becomes possible. |
| 25 | +- **Exploitability**: Low in current form — the regex strictly constrains captures to a four-value set. Risk is that the pattern is copy-pasted or the regex is relaxed without updating the column-name guard. |
| 26 | +- **Remediation**: Replace interpolation with an explicit allowlist check: |
| 27 | + ```php |
| 28 | + $allowed_thold = ['hi', 'low', 'warning_hi', 'warning_low']; |
| 29 | + if (!in_array($matches[1], $allowed_thold, true)) { |
| 30 | + cacti_log('WARNING: unexpected thold column suffix: ' . $matches[1], false, 'THOLD'); |
| 31 | + break; |
| 32 | + } |
| 33 | + $value = db_fetch_cell_prepared('SELECT thold_' . $matches[1] . ' FROM thold_data WHERE data_template_rrd_id = ?', [$data_template_rrd_id]); |
| 34 | + ``` |
| 35 | +- **TDD status**: Ready (tests in tests/Security/XssOutputTest.php, allowlist describe block) |
| 36 | + |
| 37 | +--- |
| 38 | + |
| 39 | +### FIND-002: [SQL] Unparameterised DELETE in notify_lists.php |
| 40 | + |
| 41 | +- **Severity**: Low |
| 42 | +- **Confidence**: High |
| 43 | +- **File**: notify_lists.php |
| 44 | +- **Line**: 484, 488 |
| 45 | +- **Evidence**: |
| 46 | + ``` |
| 47 | + 484: db_execute('DELETE FROM plugin_thold_threshold_contact WHERE thold_id=' . $selected_items[$i]); |
| 48 | + 488: db_execute('UPDATE thold_data SET notify_alert=' . get_request_var('id') . ' WHERE id=' . $selected_items[$i]); |
| 49 | + ``` |
| 50 | +- **Description**: Integer values are concatenated rather than bound. `$selected_items` comes from `sanitize_unserialize_selected_items()` which validates each element with `input_validate_input_number()`, so the current exploitability is low. The pattern is inconsistent with the rest of the codebase which uses prepared statements. |
| 51 | +- **Exploitability**: Low — requires bypass of `sanitize_unserialize_selected_items()` integer validation. |
| 52 | +- **Remediation**: |
| 53 | + ```php |
| 54 | + db_execute_prepared('DELETE FROM plugin_thold_threshold_contact WHERE thold_id = ?', [(int) $selected_items[$i]]); |
| 55 | + db_execute_prepared('UPDATE thold_data SET notify_alert = ? WHERE id = ?', [(int) get_filter_request_var('id'), (int) $selected_items[$i]]); |
| 56 | + ``` |
| 57 | +- **TDD status**: Ready |
| 58 | + |
| 59 | +--- |
| 60 | + |
| 61 | +### FIND-003: [Auth] thold_webapi.php — review endpoint auth coverage |
| 62 | + |
| 63 | +- **Severity**: Medium |
| 64 | +- **Confidence**: Medium |
| 65 | +- **File**: thold_webapi.php |
| 66 | +- **Line**: 1 (file entry), all action handlers |
| 67 | +- **Evidence**: File is included via `thold.php:39: include_once(...thold_webapi.php)`. Auth is inherited from `thold.php` which includes `auth.php`. Direct HTTP access to thold_webapi.php is not blocked since it lacks `cli_check.php` or a standalone auth header. |
| 68 | +- **Description**: If thold_webapi.php can be accessed directly (not via thold.php), its action handlers run without the auth check applied by thold.php's include chain. |
| 69 | +- **Exploitability**: Medium — requires direct URL access to thold_webapi.php. Cacti's web server configuration may mitigate this if the plugins directory requires auth at the vhost level. |
| 70 | +- **Remediation**: Add at the top of thold_webapi.php (or confirm it is never directly web-accessible): |
| 71 | + ```php |
| 72 | + if (!defined('IN_CACTI_CONTEXT')) { |
| 73 | + die('<br><strong>This file is not meant to be accessed directly.</strong>'); |
| 74 | + } |
| 75 | + ``` |
| 76 | +- **TDD status**: Seam required first |
| 77 | + |
| 78 | +--- |
| 79 | + |
| 80 | +### FIND-004: [CSRF] State-changing POST handlers lack explicit token check |
| 81 | + |
| 82 | +- **Severity**: Medium |
| 83 | +- **Confidence**: Medium |
| 84 | +- **File**: notify_lists.php, thold.php, thold_templates.php |
| 85 | +- **Line**: notify_lists.php:537, thold.php:386, thold_templates.php:241 |
| 86 | +- **Evidence**: |
| 87 | + ``` |
| 88 | + notify_lists.php:537: foreach ($_POST as $var => $val) { if (preg_match('/^chk_([0-9]+)$/', ... |
| 89 | + thold.php:386: foreach ($_POST as $var => $val) { |
| 90 | + ``` |
| 91 | +- **Description**: Mass-action POST handlers (bulk delete, bulk associate) do not call an explicit CSRF token verification function. Cacti core provides `form_verify_token()` / `check_request_token()` — these plugins do not call it before processing state-changing actions. |
| 92 | +- **Exploitability**: Medium — requires a logged-in admin to visit an attacker-controlled page. Cacti's SameSite cookie settings may mitigate in modern browsers. |
| 93 | +- **Remediation**: Add token check at the top of each action handler: |
| 94 | + ```php |
| 95 | + if (!form_verify_token()) { |
| 96 | + header('HTTP/1.1 400 Bad Request'); |
| 97 | + exit; |
| 98 | + } |
| 99 | + ``` |
| 100 | +- **TDD status**: Seam required first (need to stub `form_verify_token`) |
| 101 | + |
| 102 | +--- |
| 103 | + |
| 104 | +## Unknowns |
| 105 | + |
| 106 | +- Cannot confirm whether Cacti's auth layer enforces session checks before any plugin file is served — no live Cacti environment available. |
| 107 | +- `thold_daemon.php` uses `exec_background()` which calls PHP's `exec`; could not confirm whether any user-controlled data reaches `$process` arguments without full call-graph tracing. |
| 108 | + |
| 109 | +## Blind Spots |
| 110 | + |
| 111 | +- No dynamic analysis performed. Race conditions in threshold evaluation not assessed. |
| 112 | +- Email/notification template injection (thold_functions.php notification rendering) not fully traced. |
| 113 | +- No review of JavaScript files in themes/ for DOM-based XSS. |
| 114 | + |
| 115 | +## Assumptions |
| 116 | + |
| 117 | +- Cacti's `sanitize_unserialize_selected_items()` correctly validates all elements as integers. |
| 118 | +- `auth.php` include at thold.php top correctly redirects unauthenticated requests. |
| 119 | +- `db_qstr()` used in notify_queue.php filter queries provides adequate escaping. |
| 120 | + |
| 121 | +## Seams Needed |
| 122 | + |
| 123 | +1. Extract column-name resolution (FIND-001) to `src/TholdColumnResolver.php`. |
| 124 | +2. Extract bulk-action DB calls (FIND-002) to `src/NotificationListRepository.php`. |
| 125 | +3. Extract webapi action dispatch (FIND-003) to `src/WebApiDispatcher.php` with auth gate. |
| 126 | +4. Stub `form_verify_token` in CactiStubs.php to enable CSRF tests (FIND-004). |
0 commit comments