Files
Thomas Vincent bd1577a6c7 ci(security): add Semgrep rules for stored XSS and form_selectable_cell (#8550)
The XSS rule only modelled request variables, so stored text such as device
descriptions, graph titles, notes, SNMP strings and plugin hook output was
never reported. form_selectable_cell() prints its contents verbatim and had no
rule at all.

Signed-off-by: Thomas Vincent <thomasvincent@gmail.com>
2026-11-01 17:42:28 +00:00
..

Cacti-aware Semgrep rules

Off-the-shelf PHP SAST does not know Cacti's own primitives, so it misses the request-variable to SQL / shell / RRDtool flows that have produced real advisories, and it flags htmle()-escaped output as false positives. These rules teach the scanner Cacti's sources, sanitizers and sinks.

Rules (cacti-security.yml)

id class catches
cacti-request-var-in-sql SQLi (CWE-89) grv()/gnrv() into a non-prepared db_*/get_allowed_* string. Independently flags lib/html_graph.php:176 (GHSA-4899, host_id).
cacti-request-var-in-shell-or-rrdtool OS/RRDtool cmd (CWE-78) request var into shell_exec/exec/popen/proc_open/rrdtool_execute without cacti_escapeshellarg().
cacti-request-var-echoed-unescaped XSS (CWE-79) request var print/echo'd without html_escape()/htmle()/json_encode().
cacti-stored-value-echoed-unescaped Stored XSS (CWE-79) stored text print/echo'd without escaping: db_fetch_cell*() results, text columns of fetched rows (description, hostname, title_cache, notes, message, snmp_sysDescr, ...), SNMP responses, api_plugin_hook_function() returns and tail_file() lines.
cacti-unescaped-form-selectable-cell XSS (CWE-79) request or stored text passed as the contents of form_selectable_cell(), which prints it verbatim. form_selectable_ecell() is the escaping variant.

Modelled: sources grv, gnrv, get_request_var, get_nfilter_request_var; sanitizers db_qstr[_rlike], sanitize_sql_column, (int)/intval, gfrv/get_filter_request_var, cacti_escapeshellarg, html_escape/htmle, json_encode; sinks as above.

The stored-text rules match fetched rows by column name rather than tainting every db_fetch_assoc()/db_fetch_row() result, because a request variable in the WHERE clause would otherwise taint numeric columns too. They also treat array indexes as safe ($labels[$row['status']] is a static lookup) and skip cli/ and scripts/, which write plain text. A text column that is not in the name list is not reported.

Running

# scan
semgrep --config tests/security/semgrep/cacti-security.yml lib/ include/ ./*.php

# self-test the rules against the annotated fixtures
semgrep --test tests/security/semgrep/

CI (.github/workflows/semgrep.yml)

Two steps:

  1. Rule self-test (blocking) — semgrep --test tests/security/semgrep/ runs the rules against the annotated fixtures and fails if a rule stops matching what it should (or starts matching what it should not). This keeps the rules honest without depending on a whole-tree scan.
  2. Advisory changed-file scan (non-blocking) — compares the checked-out commit with the pull request base, the previous pushed commit, or the merge group's base. Manual runs use the previous commit when one exists; on a parentless commit they use the current commit and therefore have no changed PHP files. The scan includes only changed PHP files outside include/vendor, plugins, and tests. It does not fail the build: grv() returns the validated value when the page registered a filter, which Semgrep cannot see, so a whole-tree result includes known-safe sites, and taint analysis over Cacti's large files is not perfectly deterministic run to run. File-scoped results keep that historical noise out of unrelated pull requests while still surfacing request-to-sink flows in changed code.

A finding is a prompt to confirm, not proof of a bug: check whether the source is actually unvalidated on that path, then fix (bind the value, cast, or escape).