security: SQLi and XSS hardening (main)#319
security: SQLi and XSS hardening (main)#319somethingwithproof wants to merge 1 commit intoCacti:mainfrom
Conversation
There was a problem hiding this comment.
Pull request overview
This PR hardens the syslog plugin against SQL injection in alert evaluation and reduces stored-XSS risk when rendering syslog statistics, aligning with VULN-008 / VULN-009.
Changes:
- Converted several syslog alert match queries to use prepared statements with bound parameters.
- Added a
syslog_db_fetch_assoc_prepared()wrapper to run prepared SELECTs against the syslog DB connection. - Escaped host/program values when rendering the statistics table.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
syslog.php |
Escapes host/program output in the statistics table rendering. |
syslog_process.php |
Reworks alert match SQL to use parameter binding and prepared fetch helpers. |
database.php |
Adds a prepared-query wrapper for associative fetches on the syslog DB connection. |
| } else if ($alert['type'] == 'sql') { | ||
| $sql = 'SELECT * FROM `' . $syslogdb_default . '`.`syslog_incoming` | ||
| WHERE (' . $alert['message'] . ') | ||
| AND status=' . $uniqueID; | ||
| WHERE (?) | ||
| AND status=?'; | ||
| $params[] = $alert['message']; | ||
| $params[] = $uniqueID; | ||
| } |
There was a problem hiding this comment.
The 'sql' alert type query uses WHERE (?), but prepared-statement placeholders can only bind values, not SQL expressions. This will not apply the intended predicate (and may always evaluate truthy/falsey depending on DB), effectively breaking 'sql' alerts and not addressing SQL injection for this path. Consider either (a) removing/disabled the 'sql' type, (b) changing the feature to store a safe template with placeholders + separate bound params, or (c) parsing/whitelisting allowed columns/operators and building the WHERE clause with bound values (keeping status=? as a parameter).
| form_alternate_row(); | ||
| echo '<td>' . (get_request_var('host') != '-2' ? $r['host']:'-') . '</td>'; | ||
| echo '<td>' . (get_request_var('host') != '-2' ? htmle($r['host']):'-') . '</td>'; | ||
| echo '<td>' . (get_request_var('facility') != '-2' ? ucfirst($r['facility']):'-') . '</td>'; | ||
| echo '<td>' . (get_request_var('priority') != '-2' ? ucfirst($r['priority']):'-') . '</td>'; | ||
| echo '<td>' . (get_request_var('program') != '-2' ? ucfirst($r['program']):'-') . '</td>'; | ||
| echo '<td>' . (get_request_var('program') != '-2' ? htmle(ucfirst($r['program'])):'-') . '</td>'; |
There was a problem hiding this comment.
htmle() is introduced here, but this plugin repository does not define it (and existing output escaping in this file uses htmlspecialchars(...)). Unless htmle() is guaranteed by the hosting Cacti version, this will cause a runtime fatal error. Prefer using the established htmlspecialchars(..., ENT_QUOTES, 'UTF-8') pattern (or another known core escape helper already used/required by the plugin) to avoid an undeclared dependency.
| /* syslog_db_fetch_assoc_prepared - run a 'select' sql query and return the first column of the | ||
| first row found | ||
| @arg $sql - the sql query to run | ||
| @arg $params - the parameters to bind to the query | ||
| @arg $log - whether or not to log the query to the cacti log | ||
| @returns - the result of the query */ | ||
| function syslog_db_fetch_assoc_prepared($sql, $params = array(), $log = TRUE) { | ||
| global $syslog_cnn; | ||
|
|
||
| return db_fetch_assoc_prepared($sql, $params, $log, $syslog_cnn); | ||
| } |
There was a problem hiding this comment.
The new docblock for syslog_db_fetch_assoc_prepared() says it "return[s] the first column of the first row", but the function name and implementation indicate it returns an associative result set (via db_fetch_assoc_prepared). Please update the comment/@returns description to match the actual return type/behavior to avoid misleading future callers.
Implements prepared statements for SQL alerts and systematic HTML escaping for syslog statistics to prevent SQLi and stored XSS. Addresses VULN-008 and VULN-009.