fix(hardening): migrate RLIKE to db_qstr for SQL injection prevention#764
fix(hardening): migrate RLIKE to db_qstr for SQL injection prevention#764somethingwithproof wants to merge 1 commit intoCacti:developfrom
Conversation
Wrap all get_request_var('rfilter') values with db_qstr() in RLIKE
SQL clauses across thold.php, thold_graph.php, and notify_lists.php.
10 sites total.
RLIKE accepts regex from rfilter which passes FILTER_VALIDATE_IS_REGEX
but is not SQL-safe. A single quote breaks the SQL string context.
Closes Cacti#761, Cacti#763
Signed-off-by: Thomas Vincent <thomasvincent@gmail.com>
There was a problem hiding this comment.
Pull request overview
Hardens SQL construction for regex-based filters by ensuring rfilter values used in RLIKE clauses are properly SQL-quoted via db_qstr(), preventing single-quote breakouts and injection.
Changes:
- Replaced direct string interpolation of
get_request_var('rfilter')inRLIKEclauses withdb_qstr(get_request_var('rfilter'))across the UI listing and log views. - Updated threshold, host, log, and notification-list filtering queries to use safe SQL literals for regex filters.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 6 comments.
| File | Description |
|---|---|
thold.php |
Quotes rfilter safely in the Threshold Management list RLIKE filter. |
thold_graph.php |
Quotes rfilter safely in Threshold Status, Device Status, and log filtering RLIKE clauses. |
notify_lists.php |
Quotes rfilter safely in associated thresholds/templates and notification list filtering RLIKE clauses. |
|
|
||
| if (get_request_var('rfilter') != '') { | ||
| $sql_where .= ($sql_where == '' ? '(' : ' AND ') . " td.name_cache RLIKE '" . get_request_var('rfilter') . "'"; | ||
| $sql_where .= ($sql_where == '' ? '(' : ' AND ') . " td.name_cache RLIKE " . db_qstr(get_request_var('rfilter')) . ""; |
There was a problem hiding this comment.
The trailing string concatenation (. "") is redundant here and slightly obscures the intent of the SQL fragment. Consider removing the final . "" (and the leading space before td.name_cache if not needed) to keep the clause construction consistent and easier to read.
| $sql_where .= ($sql_where == '' ? '(' : ' AND ') . " td.name_cache RLIKE " . db_qstr(get_request_var('rfilter')) . ""; | |
| $sql_where .= ($sql_where == '' ? '(' : ' AND ') . 'td.name_cache RLIKE ' . db_qstr(get_request_var('rfilter')); |
|
|
||
| if (get_request_var('rfilter') != '') { | ||
| $sql_where .= ($sql_where == '' ? '(' : ' AND ') . " td.name_cache RLIKE '" . get_request_var('rfilter') . "'"; | ||
| $sql_where .= ($sql_where == '' ? '(' : ' AND ') . " td.name_cache RLIKE " . db_qstr(get_request_var('rfilter')) . ""; |
There was a problem hiding this comment.
The trailing . "" concatenation is unnecessary and can be removed to simplify this SQL clause construction (db_qstr already returns the fully quoted value).
| $sql_where .= ($sql_where == '' ? '(' : ' AND ') . " td.name_cache RLIKE " . db_qstr(get_request_var('rfilter')) . ""; | |
| $sql_where .= ($sql_where == '' ? '(' : ' AND ') . " td.name_cache RLIKE " . db_qstr(get_request_var('rfilter')); |
|
|
||
| if (get_request_var('rfilter') != '') { | ||
| $sql_where .= ($sql_where == '' ? '' : ' AND') . " tl.description RLIKE '" . get_request_var('rfilter') . "'"; | ||
| $sql_where .= ($sql_where == '' ? '' : ' AND') . " tl.description RLIKE " . db_qstr(get_request_var('rfilter')) . ""; |
There was a problem hiding this comment.
The trailing . "" concatenation at the end of this expression is redundant. Removing it will make the WHERE clause assembly clearer without changing behavior.
| $sql_where .= ($sql_where == '' ? '' : ' AND') . " tl.description RLIKE " . db_qstr(get_request_var('rfilter')) . ""; | |
| $sql_where .= ($sql_where == '' ? '' : ' AND') . " tl.description RLIKE " . db_qstr(get_request_var('rfilter')); |
|
|
||
| if (get_request_var('rfilter') != '') { | ||
| $sql_where .= ($sql_where == '' ? '' : ' AND') . " tl.description RLIKE '" . get_request_var('rfilter') . "'"; | ||
| $sql_where .= ($sql_where == '' ? '' : ' AND') . " tl.description RLIKE " . db_qstr(get_request_var('rfilter')) . ""; |
There was a problem hiding this comment.
This expression ends with a redundant . "" concatenation. Consider dropping it to keep the SQL filter construction concise and consistent with the surrounding code.
| $sql_where .= ($sql_where == '' ? '' : ' AND') . " tl.description RLIKE " . db_qstr(get_request_var('rfilter')) . ""; | |
| $sql_where .= ($sql_where == '' ? '' : ' AND') . " tl.description RLIKE " . db_qstr(get_request_var('rfilter')); |
|
|
||
| if (strlen(get_request_var('rfilter'))) { | ||
| $sql_where .= (!strlen($sql_where) ? '' : ' AND ') . "td.name_cache RLIKE '" . get_request_var('rfilter') . "'"; | ||
| $sql_where .= (!strlen($sql_where) ? '' : ' AND ') . "td.name_cache RLIKE " . db_qstr(get_request_var('rfilter')) . ""; |
There was a problem hiding this comment.
The final . "" concatenation is redundant here. Removing it would improve readability (db_qstr already returns a complete quoted SQL literal).
| $sql_where .= (!strlen($sql_where) ? '' : ' AND ') . "td.name_cache RLIKE " . db_qstr(get_request_var('rfilter')) . ""; | |
| $sql_where .= (!strlen($sql_where) ? '' : ' AND ') . 'td.name_cache RLIKE ' . db_qstr(get_request_var('rfilter')); |
|
|
||
| if (strlen(get_request_var('rfilter'))) { | ||
| $sql_where .= (!strlen($sql_where) ? 'WHERE ' : ' AND ') . "thold_template.name RLIKE '" . get_request_var('rfilter') . "'"; | ||
| $sql_where .= (!strlen($sql_where) ? 'WHERE ' : ' AND ') . "thold_template.name RLIKE " . db_qstr(get_request_var('rfilter')) . ""; |
There was a problem hiding this comment.
This SQL fragment ends with a no-op . "" concatenation. Consider removing it to keep the clause assembly straightforward and consistent.
| $sql_where .= (!strlen($sql_where) ? 'WHERE ' : ' AND ') . "thold_template.name RLIKE " . db_qstr(get_request_var('rfilter')) . ""; | |
| $sql_where .= (!strlen($sql_where) ? 'WHERE ' : ' AND ') . "thold_template.name RLIKE " . db_qstr(get_request_var('rfilter')); |
Summary
Wrap all get_request_var('rfilter') values with db_qstr() in RLIKE SQL clauses. 10 sites across 3 files.
rfilter passes FILTER_VALIDATE_IS_REGEX (valid PCRE) but is not SQL-safe. A single quote in the regex breaks the SQL string context, enabling injection.
Files changed
Test plan
Closes #761, #763