From 2fb549b7d1b4328adc57b7ccb94cca06ab51b901 Mon Sep 17 00:00:00 2001 From: Thomas Vincent Date: Sun, 15 Mar 2026 16:14:53 -0700 Subject: [PATCH 1/3] security: migrate gexport SQL helpers to prepared variants --- gexport.php | 59 +++++++++++++++++++++++------- setup.php | 11 ++++-- tests/test_prepared_statements.php | 59 ++++++++++++++++++++++++++++++ 3 files changed, 113 insertions(+), 16 deletions(-) create mode 100644 tests/test_prepared_statements.php diff --git a/gexport.php b/gexport.php index fa23718..0d20b22 100644 --- a/gexport.php +++ b/gexport.php @@ -181,17 +181,22 @@ function export_form_actions() { if (get_nfilter_request_var('drp_action') === '1') { /* delete */ /* do a referential integrity check */ if (sizeof($selected_items)) { + $export_ids = array(); + foreach($selected_items as $export_id) { /* ================= input validation ================= */ input_validate_input_number($export_id); /* ==================================================== */ - $export_ids[] = $export_id; + $export_ids[] = (int)$export_id; } } - if (isset($export_ids)) { - db_execute('DELETE FROM graph_exports WHERE ' . array_to_sql_or($export_ids, 'id')); + if (isset($export_ids) && cacti_sizeof($export_ids)) { + $placeholders = implode(',', array_fill(0, cacti_sizeof($export_ids), '?')); + db_execute_prepared("DELETE FROM graph_exports + WHERE id IN ($placeholders)", + $export_ids); } } elseif (get_nfilter_request_var('drp_action') === '2') { /* enable */ for ($i=0;($i 0 - AND status > 0'); + AND status > 0', + array()); if ($running == 0) { set_request_var('refresh', 99999999); @@ -881,7 +895,17 @@ function gexport() { form_selectable_cell(__('All Sites', 'gexport'), $export['id'], '', 'text-align:right'); } else { if ($export['graph_site'] != '') { - $sites = db_fetch_cell('SELECT GROUP_CONCAT(name ORDER BY name SEPARATOR ", ") FROM sites WHERE id IN(' . $export['graph_site'] . ')'); + $site_ids = array_values(array_filter(array_map('intval', preg_split('/\s*,\s*/', $export['graph_site'], -1, PREG_SPLIT_NO_EMPTY)))); + + if (cacti_sizeof($site_ids)) { + $placeholders = implode(',', array_fill(0, cacti_sizeof($site_ids), '?')); + $sites = db_fetch_cell_prepared("SELECT GROUP_CONCAT(name ORDER BY name SEPARATOR ', ') + FROM sites + WHERE id IN ($placeholders)", + $site_ids); + } else { + $sites = ''; + } } else { $sites = ''; } @@ -892,7 +916,17 @@ function gexport() { form_selectable_cell(__('All Trees', 'gexport'), $export['id'], '', 'text-align:right'); } else { if ($export['graph_tree'] != '') { - $trees = db_fetch_cell('SELECT GROUP_CONCAT(name ORDER BY name SEPARATOR ", ") FROM graph_tree WHERE id IN(' . $export['graph_tree'] . ')'); + $tree_ids = array_values(array_filter(array_map('intval', preg_split('/\s*,\s*/', $export['graph_tree'], -1, PREG_SPLIT_NO_EMPTY)))); + + if (cacti_sizeof($tree_ids)) { + $placeholders = implode(',', array_fill(0, cacti_sizeof($tree_ids), '?')); + $trees = db_fetch_cell_prepared("SELECT GROUP_CONCAT(name ORDER BY name SEPARATOR ', ') + FROM graph_tree + WHERE id IN ($placeholders)", + $tree_ids); + } else { + $trees = ''; + } } else { $trees = ''; } @@ -937,4 +971,3 @@ function gexport() { form_end(); } - diff --git a/setup.php b/setup.php index 5a26d25..b19056e 100644 --- a/setup.php +++ b/setup.php @@ -55,7 +55,10 @@ function gexport_poller_bottom() { /* graph export */ if ($config['poller_id'] == 1) { - $exports = db_fetch_assoc('SELECT * FROM graph_exports WHERE enabled="on"'); + $exports = db_fetch_assoc_prepared('SELECT * + FROM graph_exports + WHERE enabled = ?', + array('on')); if (sizeof($exports)) { $command_string = read_config_option('path_php_binary'); $extra_args = '-q "' . $config['base_path'] . '/plugins/gexport/poller_export.php"'; @@ -78,7 +81,10 @@ function gexport_check_upgrade() { $info = plugin_gexport_version (); $current = $info['version']; - $old = db_fetch_cell("SELECT version FROM plugin_config WHERE directory='gexport'"); + $old = db_fetch_cell_prepared('SELECT version + FROM plugin_config + WHERE directory = ?', + array('gexport')); if (cacti_version_compare($old,$current,'<')) { if (api_plugin_is_enabled('gexport')) { @@ -610,4 +616,3 @@ function gexport_draw_navigation_text($nav) { return $nav; } - diff --git a/tests/test_prepared_statements.php b/tests/test_prepared_statements.php new file mode 100644 index 0000000..4c39bf0 --- /dev/null +++ b/tests/test_prepared_statements.php @@ -0,0 +1,59 @@ += 4 +); +assert_true( + 'gexport.php parameterizes site/tree group concat lookups', + preg_match('/GROUP_CONCAT\(name ORDER BY name SEPARATOR \', \'\)\s+FROM sites\s+WHERE id IN \(\$placeholders\)/s', $gexport_contents) === 1 && + preg_match('/GROUP_CONCAT\(name ORDER BY name SEPARATOR \', \'\)\s+FROM graph_tree\s+WHERE id IN \(\$placeholders\)/s', $gexport_contents) === 1 +); + +echo "\n"; +echo "Results: $pass passed, $fail failed\n"; + +exit($fail > 0 ? 1 : 0); From 2361e7e8e35457230e602afabe29fd669eccd454 Mon Sep 17 00:00:00 2001 From: Thomas Vincent Date: Sun, 15 Mar 2026 17:17:56 -0700 Subject: [PATCH 2/3] test: guard gexport prepared regression file reads --- tests/test_prepared_statements.php | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/tests/test_prepared_statements.php b/tests/test_prepared_statements.php index 4c39bf0..2de54d3 100644 --- a/tests/test_prepared_statements.php +++ b/tests/test_prepared_statements.php @@ -27,6 +27,12 @@ function assert_true($label, $value) { $setup_contents = file_get_contents(__DIR__ . '/../setup.php'); $gexport_contents = file_get_contents(__DIR__ . '/../gexport.php'); +assert_true('setup.php is readable', $setup_contents !== false); +assert_true('gexport.php is readable', $gexport_contents !== false); + +$setup_contents = ($setup_contents === false ? '' : $setup_contents); +$gexport_contents = ($gexport_contents === false ? '' : $gexport_contents); + assert_true( 'setup.php uses prepared enabled exports query', preg_match('/db_fetch_assoc_prepared\s*\(\s*\'SELECT \*\s+FROM graph_exports\s+WHERE enabled = \?/s', $setup_contents) === 1 From 3f6b043338acaab93c809ba8b55fa3a443858ed2 Mon Sep 17 00:00:00 2001 From: Thomas Vincent Date: Sun, 15 Mar 2026 19:16:38 -0700 Subject: [PATCH 3/3] test: add grok follow-up guard checks for gexport deletes --- tests/test_prepared_statements.php | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/tests/test_prepared_statements.php b/tests/test_prepared_statements.php index 2de54d3..01f5697 100644 --- a/tests/test_prepared_statements.php +++ b/tests/test_prepared_statements.php @@ -45,6 +45,14 @@ function assert_true($label, $value) { 'gexport.php uses prepared bulk delete', preg_match('/db_execute_prepared\s*\(\s*"DELETE FROM graph_exports\s+WHERE id IN \(/s', $gexport_contents) === 1 ); +assert_true( + 'gexport.php guards bulk delete on non-empty export id lists', + preg_match('/if\s*\(isset\(\$export_ids\)\s*&&\s*cacti_sizeof\(\$export_ids\)\s*\)/', $gexport_contents) === 1 +); +assert_true( + 'gexport.php builds export-id placeholders from item count', + strpos($gexport_contents, '$placeholders = implode(\',\', array_fill(0, cacti_sizeof($export_ids), \'?\'));') !== false +); assert_true( 'gexport.php no longer uses array_to_sql_or for export delete', strpos($gexport_contents, "array_to_sql_or(\$export_ids, 'id')") === false