DRAFT: fix(security): PR #313 follow-up - partition correctness, CSV/XSS hardening #315
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -241,14 +241,22 @@ function syslog_partition_manage() { | |
| // Always create the partition an hour ahead of time | ||
| $time = time() + 3600; | ||
|
|
||
| /* | ||
| * Only run the retention prune when the next partition is ready. | ||
| * If maintenance cannot safely create it, leave dMaxValue in place | ||
| * as the write-path safety net and avoid dropping old partitions | ||
| * without a replacement. | ||
| */ | ||
| if (syslog_partition_check('syslog', $time)) { | ||
| if (syslog_partition_create('syslog', $time)) { | ||
| $syslog_deleted = syslog_partition_remove('syslog'); | ||
| } | ||
| } | ||
|
|
||
| if (syslog_partition_check('syslog_removed', $time)) { | ||
| if (syslog_partition_create('syslog_removed', $time)) { | ||
| $syslog_deleted += syslog_partition_remove('syslog_removed'); | ||
| } | ||
| } | ||
|
|
||
| return $syslog_deleted; | ||
|
|
@@ -291,10 +299,27 @@ function syslog_partition_create($table, $time = null) { | |
| return false; | ||
| } | ||
|
|
||
| if (!preg_match('/^[a-zA-Z0-9_]+$/', $syslogdb_default)) { | ||
| cacti_log("SYSLOG ERROR: Invalid database name; partition create aborted", false, 'SYSLOG'); | ||
|
|
||
| return false; | ||
| } | ||
|
|
||
| if ($time === null) { | ||
| $time = time() + 3600; | ||
| } | ||
|
|
||
| // Reject non-numeric, negative, or far-future timestamps; boundary | ||
| // math assumes a non-negative UTC epoch within 64-bit safe range so | ||
| // extreme inputs cannot underflow or overflow to float. | ||
| if (!is_numeric($time) || (int)$time < 0 || (int)$time > 4102444800) { | ||
| cacti_log("SYSLOG ERROR: syslog_partition_create called with invalid time '$time' for table '$table'", false, 'SYSLOG'); | ||
|
|
||
| return false; | ||
| } | ||
|
|
||
| $time = (int)$time; | ||
|
|
||
| // Hash to guarantee the lock name stays within MySQL's 64-byte limit. | ||
| $lock_name = substr(hash('sha256', $syslogdb_default . '.syslog_partition_create.' . $table), 0, 60); | ||
|
|
||
|
|
@@ -318,10 +343,32 @@ function syslog_partition_create($table, $time = null) { | |
| return false; | ||
| } | ||
|
|
||
| $success = false; | ||
|
|
||
| try { | ||
| /* | ||
| * Boundary arithmetic is done in PHP against the UTC epoch so the | ||
| * result is independent of both the PHP and MySQL session time zones. | ||
| * $boundary_epoch is the next UTC midnight strictly after $time; it | ||
| * becomes the VALUES LESS THAN literal for UNIX_TIMESTAMP partitions | ||
| * and the source for the date string passed to TO_DAYS. | ||
| */ | ||
| $boundary_epoch = ((int)($time / 86400) + 1) * 86400; | ||
|
|
||
| if ($boundary_epoch <= 0 || $boundary_epoch <= $time) { | ||
| cacti_log("SYSLOG ERROR: Boundary epoch computation failed for '$table' (time=$time); leaving writes in dMaxValue until maintenance recovers", false, 'SYSLOG'); | ||
|
|
||
| return false; | ||
| } | ||
|
|
||
| $cformat = 'd' . gmdate('Ymd', $time); | ||
| $boundary_date = gmdate('Y-m-d', $boundary_epoch); | ||
|
|
||
| if (!preg_match('/^d\d{8}$/', $cformat) || !preg_match('/^\d{4}-\d{2}-\d{2}$/', $boundary_date)) { | ||
| cacti_log("SYSLOG ERROR: Derived partition values failed format validation for '$table'; leaving writes in dMaxValue until maintenance recovers", false, 'SYSLOG'); | ||
|
|
||
| return false; | ||
| } | ||
|
|
||
| $exists = syslog_db_fetch_row_prepared('SELECT * | ||
| FROM `information_schema`.`partitions` | ||
|
|
@@ -340,30 +387,41 @@ function syslog_partition_create($table, $time = null) { | |
| * MySQL does not support parameter binding for DDL identifiers | ||
| * or partition definitions. $table is safe because it passed | ||
| * syslog_partition_table_allowed() (two-value allowlist plus | ||
| * regex guard). $cformat, $boundary_epoch, and $boundary_date | ||
| * derive from integer arithmetic and gmdate(), so they contain | ||
| * only digits, hyphens, and the letter 'd'. | ||
| */ | ||
| $create_syntax = syslog_db_fetch_row_prepared("SHOW CREATE TABLE `$syslogdb_default`.`$table`"); | ||
|
|
||
| if (!cacti_sizeof($create_syntax) || empty($create_syntax['Create Table'])) { | ||
| cacti_log("SYSLOG ERROR: SHOW CREATE TABLE returned no rows for '$table'; leaving writes in dMaxValue until maintenance recovers", false, 'SYSLOG'); | ||
|
|
||
| return false; | ||
| } | ||
|
|
||
| $create_sql = $create_syntax['Create Table']; | ||
|
|
||
| if (stripos($create_sql, 'TO_DAYS') !== false) { | ||
| syslog_db_execute_prepared("ALTER TABLE `$syslogdb_default`.`$table` REORGANIZE PARTITION dMaxValue INTO ( | ||
| PARTITION $cformat VALUES LESS THAN (TO_DAYS('$boundary_date')), | ||
| PARTITION dMaxValue VALUES LESS THAN MAXVALUE)"); | ||
| } elseif (stripos($create_sql, 'UNIX_TIMESTAMP') !== false) { | ||
| syslog_db_execute_prepared("ALTER TABLE `$syslogdb_default`.`$table` REORGANIZE PARTITION dMaxValue INTO ( | ||
| PARTITION $cformat VALUES LESS THAN ($boundary_epoch), | ||
| PARTITION dMaxValue VALUES LESS THAN MAXVALUE)"); | ||
| } else { | ||
| cacti_log("SYSLOG ERROR: Unable to determine partition expression (neither TO_DAYS nor UNIX_TIMESTAMP) for '$table'; leaving writes in dMaxValue until maintenance recovers", false, 'SYSLOG'); | ||
|
|
||
|
Comment on lines
+402
to
+414
There was a problem hiding this comment. Partition-expression detection uses case-sensitive strpos() checks for 'TO_DAYS' / 'UNIX_TIMESTAMP'. SHOW CREATE TABLE preserves the original expression casing, so a table partitioned with e.g. unix_timestamp(...) or to_days(...) would now hard-fail rotation even though the expression is supported. Use a case-insensitive check (e.g., stripos) or a case-insensitive regex when detecting the partition expression.
Contributor
Author
There was a problem hiding this comment. Fixed, and the PR scope has since narrowed further. Detection now uses stripos() for both TO_DAYS and UNIX_TIMESTAMP, so lowercase or mixed-case SHOW CREATE TABLE output no longer causes partition maintenance to miss a supported expression. The draft PR no longer includes the SQL-rule gating/settings work; it is now focused on the partition, callback, JS-emission, and report-query fixes. |
||
| return false; | ||
| } | ||
| } | ||
|
|
||
| $success = true; | ||
| } finally { | ||
| syslog_db_fetch_cell_prepared('SELECT RELEASE_LOCK(?)', [$lock_name]); | ||
| } | ||
|
|
||
| return $success; | ||
| } | ||
|
|
||
| /** | ||
|
|
@@ -380,6 +438,12 @@ function syslog_partition_remove($table) { | |
| return 0; | ||
| } | ||
|
|
||
| if (!preg_match('/^[a-zA-Z0-9_]+$/', $syslogdb_default)) { | ||
| cacti_log("SYSLOG ERROR: Invalid database name; partition remove aborted", false, 'SYSLOG'); | ||
|
|
||
| return 0; | ||
| } | ||
|
|
||
| $lock_name = substr(hash('sha256', $syslogdb_default . '.syslog_partition_remove.' . $table), 0, 60); | ||
|
|
||
| $locked = syslog_db_fetch_cell_prepared('SELECT GET_LOCK(?, 10)', [$lock_name]); | ||
|
|
@@ -418,11 +482,24 @@ function syslog_partition_remove($table) { | |
| while ($user_partitions > $days) { | ||
| $oldest = $number_of_partitions[$i]; | ||
|
|
||
| $part_name = $oldest['PARTITION_NAME']; | ||
|
|
||
| if (!preg_match('/^[a-zA-Z0-9_]+$/', $part_name)) { | ||
| cacti_log("SYSLOG ERROR: Invalid partition name '$part_name' for '$table'; skipping drop", false, 'SYSLOG'); | ||
| break; | ||
| } | ||
|
|
||
| cacti_log("SYSLOG: Removing old partition '" . $part_name . "'", false, 'SYSTEM'); | ||
|
|
||
| syslog_debug("Removing partition '" . $part_name . "'"); | ||
|
|
||
| /* $table passed syslog_partition_table_allowed() at function entry; $part_name is regex-validated above. DDL identifiers cannot be parameterized. */ | ||
| $result = syslog_db_execute_prepared("ALTER TABLE `$syslogdb_default`.`$table` DROP PARTITION `$part_name`"); | ||
|
|
||
| if ($result === false) { | ||
| cacti_log("SYSLOG ERROR: Failed to drop partition '$part_name' from '$table' after $i successful drop(s); aborting further drops", false, 'SYSLOG'); | ||
| break; | ||
| } | ||
|
|
||
| $i++; | ||
| $user_partitions--; | ||
|
|
@@ -785,6 +862,41 @@ function sql_hosts_where($tab) { | |
| } | ||
| } | ||
|
|
||
| /** | ||
| * Defuse CSV formula injection without mutating content. | ||
| * | ||
| * Spreadsheet applications (Excel, LibreOffice, Google Sheets) interpret any | ||
| * cell starting with =, +, -, @, TAB, or CR as a formula. Prepending a | ||
| * single quote tells them to treat the cell as literal text. The quote is | ||
| * visible in the cell but does not alter the underlying data, unlike | ||
| * trimming which loses characters. | ||
| * | ||
| * See OWASP CSV Injection Prevention Cheat Sheet. | ||
| */ | ||
| function syslog_csv_safe($value) { | ||
| if (!is_string($value) || $value === '') { | ||
| return $value; | ||
| } | ||
|
|
||
| // Some CSV importers strip leading spaces before parsing as a | ||
| // formula, so " =SUM(A1)" is still dangerous. Only strip literal | ||
| // spaces here; tabs and carriage returns are themselves triggers | ||
| // and must remain detectable as the first character. | ||
| $stripped = ltrim($value, ' '); | ||
|
|
||
| if ($stripped === '') { | ||
| return $value; | ||
| } | ||
|
|
||
| $first = $stripped[0]; | ||
|
|
||
| if ($first === '=' || $first === '+' || $first === '-' || $first === '@' || $first === "\t" || $first === "\r") { | ||
| return "'" . $value; | ||
| } | ||
|
|
||
| return $value; | ||
| } | ||
|
|
||
| function syslog_export($tab) { | ||
| global $syslog_incoming_config, $severities; | ||
| global $syslogdb_default; | ||
|
|
@@ -851,20 +963,20 @@ function syslog_export($tab) { | |
| } | ||
|
|
||
| if (isset($hosts[$message['host_id']])) { | ||
| $host = $hosts[$message['host_id']]; | ||
| } else { | ||
| $host = 'Unknown'; | ||
| } | ||
|
|
||
| $logmsg = $message[$syslog_incoming_config['textField']]; | ||
|
|
||
| $line = [ | ||
| syslog_csv_safe($host), | ||
|
Member
There was a problem hiding this comment. I took this out for a reason. The fputcsv() does all the escaping with the exception of formula's. I would prefer we stick with the builtin functions. The Facility, Priority are static known values mapped from an ID even.
Contributor
Author
There was a problem hiding this comment. Agreed that fputcsv() remains the CSV escaping mechanism. The helper here is only for spreadsheet-formula defusing before values reach fputcsv(), not a replacement for builtin escaping. The PR is now draft and narrower overall; the SQL-rule gating/settings changes are removed, while the CSV thread is still limited to the formula-defusing behavior. |
||
| syslog_csv_safe(ucfirst($facility)), | ||
| syslog_csv_safe(ucfirst($priority)), | ||
| syslog_csv_safe(ucfirst($program)), | ||
| $message['logtime'], | ||
| syslog_csv_safe($logmsg) | ||
| ]; | ||
|
|
||
| fputcsv($fp, $line); | ||
|
|
@@ -894,17 +1006,14 @@ function syslog_export($tab) { | |
| $severity = 'Unknown'; | ||
| } | ||
|
|
||
| $line = [ | ||
| syslog_csv_safe($message['name']), | ||
| syslog_csv_safe($severity), | ||
| $message['logtime'], | ||
| syslog_csv_safe($message['logmsg']), | ||
| syslog_csv_safe($message['host']), | ||
| syslog_csv_safe(ucfirst($message['facility'])), | ||
| syslog_csv_safe(ucfirst($message['priority'])), | ||
| $message['count'] | ||
| ]; | ||
|
|
||
|
|
@@ -920,7 +1029,7 @@ function syslog_debug($message) { | |
| global $debug; | ||
|
|
||
| if ($debug) { | ||
| print date('H:i:s') . ' SYSLOG DEBUG: ' . trim($message) . PHP_EOL; | ||
|
|
||
| } | ||
| } | ||
|
|
||
|
|
@@ -985,6 +1094,19 @@ function syslog_manage_items($from_table, $to_table) { | |
| global $config, $syslog_cnn, $syslog_incoming_config; | ||
| global $syslogdb_default; | ||
|
|
||
| /* | ||
|
Member
There was a problem hiding this comment. I'm not sure this adds value as it's an internal function and not for general use.
Contributor
Author
There was a problem hiding this comment. Fair point. The value I saw here was defense-in-depth at the interpolation sink, since those table names are used directly in DML and DDL. The PR has since been moved to draft and narrowed by removing the SQL-rule gating/settings work, so what remains is the smaller correctness and hardening set. |
||
| * Table names are interpolated into DDL/DML below because MySQL does | ||
| * not bind identifiers. Reject anything outside the static allowlist | ||
| * so a future caller cannot turn this into a SQL injection surface. | ||
| */ | ||
| $allowed_tables = ['syslog', 'syslog_incoming', 'syslog_removed']; | ||
|
|
||
| if (!in_array($from_table, $allowed_tables, true) || !in_array($to_table, $allowed_tables, true)) { | ||
| cacti_log("SYSLOG ERROR: syslog_manage_items called with disallowed tables from='$from_table' to='$to_table'", false, 'SYSLOG'); | ||
|
|
||
| return ['removed' => 0, 'xferred' => 0]; | ||
| } | ||
|
|
||
| // Select filters to work on | ||
| $rows = syslog_db_fetch_assoc("SELECT * FROM `$syslogdb_default`.`syslog_remove` WHERE enabled = 'on'"); | ||
|
|
||
|
|
@@ -1054,10 +1176,10 @@ function syslog_manage_items($from_table, $to_table) { | |
| } elseif ($remove['type'] == 'sql') { | ||
| if ($remove['method'] != 'del') { | ||
| $sql_sel = "SELECT seq FROM `$syslogdb_default`.`$from_table` | ||
| WHERE (" . $remove['message'] . ')'; | ||
|
Member
There was a problem hiding this comment. I kind of agree with this for complex queries. It's going to be difficult though to match a host or a facility or priority due to the fact that they are id mapped. However, it's a breaking change. So, it has to be improved. So, before we do this we need a design review to talk about how we do SQL expressions safely and then create the pull request off of that.
Contributor
Author
There was a problem hiding this comment. Agreed, and the branch now reflects that. The SQL-rule gating/settings work has been removed from this PR. The draft PR is now limited to the narrower correctness and hardening changes, with SQL-expression safety deferred out of this branch. |
||
| } else { | ||
| $sql_dlt = "DELETE FROM `$syslogdb_default`.`$from_table` | ||
| WHERE (" . $remove['message'] . ')'; | ||
| } | ||
| } | ||
|
|
||
|
|
@@ -1767,7 +1889,6 @@ function syslog_get_alert_sql(&$alert, $max_seq) { | |
| $params[] = $alert['message']; | ||
| $params[] = $max_seq; | ||
| } elseif ($alert['type'] == 'sql') { | ||
| $sql = "SELECT * | ||
| FROM `$syslogdb_default`.`syslog_incoming` | ||
| WHERE ({$alert['message']}) | ||
|
|
@@ -2241,9 +2362,9 @@ function syslog_process_reports() { | |
| $date1 = date('Y-m-d H:i:s', $current_time - $time_span); | ||
| $sql .= ' AND logtime BETWEEN ? AND ?'; | ||
| $sql .= ' ORDER BY logtime DESC'; | ||
| $items = syslog_db_fetch_assoc_prepared($sql, [$date1, $date2]); | ||
|
|
||
| syslog_debug('We have ' . cacti_sizeof($items) . ' items for the Report'); | ||
|
|
||
| $classes = ['even', 'odd']; | ||
|
|
||
|
|
||
Oops, something went wrong.
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess I'm okay with this. It means that the database is down though. So, bigger problems.