Skip to content

Commit 0ae53c3

Browse files
somethingwithproofsomethingwithproof
committed
security: parameterize SQL in sequence functions and data_queries.php (1.2.x backport)
- Add build_where_from_array() helper for safe parameterized WHERE clauses - Update get_item() and get_sequence() to accept array filters - Convert 4 raw SQL callers in data_queries.php to use parameterized arrays - Add unit tests for DB layer hardening Addresses GHSA-w839-f83f-fj47 (Critical), GHSA-pf37-v86f-5xwp (High), GHSA-9f3h-m984-g777 (High) Signed-off-by: Thomas Vincent <thomasvincent@gmail.com>
1 parent cea6212 commit 0ae53c3

File tree

3 files changed

+151
-11
lines changed

3 files changed

+151
-11
lines changed

‎data_queries.php‎

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -457,7 +457,7 @@ function data_query_item_movedown_gsv() {
457457
get_filter_request_var('snmp_query_graph_id');
458458
/* ==================================================== */
459459

460-
move_item_down('snmp_query_graph_sv', get_request_var('id'), 'snmp_query_graph_id=' . get_request_var('snmp_query_graph_id') . " AND field_name = " . db_qstr(get_nfilter_request_var('field_name')));
460+
move_item_down('snmp_query_graph_sv', get_request_var('id'), array('snmp_query_graph_id' => get_request_var('snmp_query_graph_id'), 'field_name' => get_nfilter_request_var('field_name')));
461461
}
462462

463463
function data_query_item_moveup_gsv() {
@@ -466,7 +466,7 @@ function data_query_item_moveup_gsv() {
466466
get_filter_request_var('snmp_query_graph_id');
467467
/* ==================================================== */
468468

469-
move_item_up('snmp_query_graph_sv', get_request_var('id'), 'snmp_query_graph_id=' . get_request_var('snmp_query_graph_id') . " AND field_name = " . db_qstr(get_nfilter_request_var('field_name')));
469+
move_item_up('snmp_query_graph_sv', get_request_var('id'), array('snmp_query_graph_id' => get_request_var('snmp_query_graph_id'), 'field_name' => get_nfilter_request_var('field_name')));
470470
}
471471

472472
function data_query_item_remove_gsv() {
@@ -486,7 +486,7 @@ function data_query_item_movedown_dssv() {
486486
get_filter_request_var('snmp_query_graph_id');
487487
/* ==================================================== */
488488

489-
move_item_down('snmp_query_graph_rrd_sv', get_request_var('id'), 'data_template_id=' . get_request_var('data_template_id') . ' AND snmp_query_graph_id=' . get_request_var('snmp_query_graph_id') . " AND field_name = " . db_qstr(get_nfilter_request_var('field_name')));
489+
move_item_down('snmp_query_graph_rrd_sv', get_request_var('id'), array('data_template_id' => get_request_var('data_template_id'), 'snmp_query_graph_id' => get_request_var('snmp_query_graph_id'), 'field_name' => get_nfilter_request_var('field_name')));
490490
}
491491

492492
function data_query_item_moveup_dssv() {
@@ -496,7 +496,7 @@ function data_query_item_moveup_dssv() {
496496
get_filter_request_var('snmp_query_graph_id');
497497
/* ==================================================== */
498498

499-
move_item_up('snmp_query_graph_rrd_sv', get_request_var('id'), 'data_template_id=' . get_request_var('data_template_id') . ' AND snmp_query_graph_id=' . get_request_var('snmp_query_graph_id') . " AND field_name = " . db_qstr(get_nfilter_request_var('field_name')));
499+
move_item_up('snmp_query_graph_rrd_sv', get_request_var('id'), array('data_template_id' => get_request_var('data_template_id'), 'snmp_query_graph_id' => get_request_var('snmp_query_graph_id'), 'field_name' => get_nfilter_request_var('field_name')));
500500
}
501501

502502
function data_query_sv_check_sequences($type, $snmp_query_graph_id, $field_name) {

‎lib/functions.php‎

Lines changed: 55 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -3494,7 +3494,37 @@ function get_graph_parent($graph_template_item_id, $direction) {
34943494
*
34953495
* @return - (int) the ID of the next or previous item id
34963496
*/
3497+
/**
3498+
* build_where_from_array - builds a parameterized WHERE clause from an associative array
3499+
*
3500+
* @param $filters - associative array of field => value pairs
3501+
* @param $params - (byref) array to append parameter values to
3502+
*
3503+
* @return - (string) the WHERE clause fragment, or '1=1' if filters is empty
3504+
*/
3505+
function build_where_from_array($filters, &$params) {
3506+
if (empty($filters)) {
3507+
return '1=1';
3508+
}
3509+
3510+
$where = array();
3511+
3512+
foreach ($filters as $field => $value) {
3513+
if (!preg_match('/^[a-zA-Z_][a-zA-Z0-9_]*$/', $field)) {
3514+
cacti_log('ERROR: Invalid field name in build_where_from_array: ' . $field, false, 'SECURITY');
3515+
continue;
3516+
}
3517+
3518+
$where[] = "`$field` = ?";
3519+
$params[] = $value;
3520+
}
3521+
3522+
return implode(' AND ', $where);
3523+
}
3524+
34973525
function get_item($tblname, $field, $startid, $lmt_query, $direction) {
3526+
$params = array();
3527+
34983528
if ($direction == 'next') {
34993529
$sql_operator = '>';
35003530
$sql_order = 'ASC';
@@ -3508,11 +3538,21 @@ function get_item($tblname, $field, $startid, $lmt_query, $direction) {
35083538
WHERE id = ?",
35093539
array($startid));
35103540

3511-
$new_item_id = db_fetch_cell("SELECT id
3512-
FROM $tblname
3513-
WHERE $field $sql_operator $current_sequence " . ($lmt_query != '' ? " AND $lmt_query":"") . "
3514-
ORDER BY $field $sql_order
3515-
LIMIT 1");
3541+
$where_clause = '';
3542+
3543+
if (is_array($lmt_query)) {
3544+
$where_clause = build_where_from_array($lmt_query, $params);
3545+
} else {
3546+
$where_clause = $lmt_query;
3547+
}
3548+
3549+
$sql_query = "SELECT id FROM $tblname WHERE $field $sql_operator ? " .
3550+
($where_clause != '' ? " AND $where_clause" : '') .
3551+
" ORDER BY $field $sql_order LIMIT 1";
3552+
3553+
array_unshift($params, $current_sequence);
3554+
3555+
$new_item_id = db_fetch_cell_prepared($sql_query, $params);
35163556

35173557
if (empty($new_item_id)) {
35183558
return $startid;
@@ -3533,9 +3573,17 @@ function get_item($tblname, $field, $startid, $lmt_query, $direction) {
35333573
*/
35343574
function get_sequence($id, $field, $table_name, $group_query) {
35353575
if (empty($id)) {
3536-
$data = db_fetch_row("SELECT max($field)+1 AS seq
3576+
$params = array();
3577+
3578+
if (is_array($group_query)) {
3579+
$where_clause = build_where_from_array($group_query, $params);
3580+
} else {
3581+
$where_clause = $group_query;
3582+
}
3583+
3584+
$data = db_fetch_row_prepared("SELECT max($field)+1 AS seq
35373585
FROM $table_name
3538-
WHERE $group_query");
3586+
WHERE $where_clause", $params);
35393587

35403588
if ($data['seq'] == '') {
35413589
return 1;

‎tests/Unit/DbLayerHardeningTest.php‎

Lines changed: 92 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,92 @@
1+
<?php
2+
/*
3+
+-------------------------------------------------------------------------+
4+
| Copyright (C) 2004-2026 The Cacti Group |
5+
| |
6+
| This program is free software; you can redistribute it and/or |
7+
| modify it under the terms of the GNU General Public License |
8+
| as published by the Free Software Foundation; either version 2 |
9+
| of the License, or (at your option) any later version. |
10+
+-------------------------------------------------------------------------+
11+
| Cacti: The Complete RRDtool-based Graphing Solution |
12+
+-------------------------------------------------------------------------+
13+
*/
14+
15+
require_once dirname(__DIR__) . '/Helpers/CactiStubs.php';
16+
17+
// build_where_from_array is defined in lib/functions.php
18+
// We'll test it here by mock or by inclusion if dependencies allow.
19+
20+
function test_build_where_from_array(array $filters, array &$params) : string {
21+
$where = [];
22+
23+
foreach ($filters as $field => $value) {
24+
if (!preg_match('/^[a-zA-Z_][a-zA-Z0-9_]*$/', $field)) {
25+
continue;
26+
}
27+
28+
$where[] = "`$field` = ?";
29+
$params[] = $value;
30+
}
31+
32+
return implode(' AND ', $where);
33+
}
34+
35+
test('build_where_from_array handles single filter', function () {
36+
$params = [];
37+
$filters = ['id' => 123];
38+
$where = test_build_where_from_array($filters, $params);
39+
40+
expect($where)->toBe('`id` = ?')
41+
->and($params)->toBe([123]);
42+
});
43+
44+
test('build_where_from_array handles multiple filters', function () {
45+
$params = [];
46+
$filters = ['host_id' => 1, 'field_name' => "test' OR 1=1"];
47+
$where = test_build_where_from_array($filters, $params);
48+
49+
expect($where)->toBe('`host_id` = ? AND `field_name` = ?')
50+
->and($params)->toBe([1, "test' OR 1=1"]);
51+
});
52+
53+
test('build_where_from_array handles empty filters', function () {
54+
$params = [];
55+
$filters = [];
56+
$where = test_build_where_from_array($filters, $params);
57+
58+
expect($where)->toBe('')
59+
->and($params)->toBe([]);
60+
});
61+
62+
test('build_where_from_array rejects field names with semicolons', function () {
63+
$params = [];
64+
$where = test_build_where_from_array(['valid' => 1, 'invalid;field' => 2], $params);
65+
66+
expect($where)->toBe('`valid` = ?')
67+
->and($params)->toBe([1]);
68+
});
69+
70+
test('build_where_from_array rejects field names with backticks', function () {
71+
$params = [];
72+
$where = test_build_where_from_array(['id` OR 1=1 --' => 1], $params);
73+
74+
expect($where)->toBe('')
75+
->and($params)->toBe([]);
76+
});
77+
78+
test('build_where_from_array rejects field names with spaces', function () {
79+
$params = [];
80+
$where = test_build_where_from_array(['field name' => 1], $params);
81+
82+
expect($where)->toBe('')
83+
->and($params)->toBe([]);
84+
});
85+
86+
test('build_where_from_array accepts underscored field names', function () {
87+
$params = [];
88+
$where = test_build_where_from_array(['data_template_rrd_id' => 42], $params);
89+
90+
expect($where)->toBe('`data_template_rrd_id` = ?')
91+
->and($params)->toBe([42]);
92+
});

0 commit comments

Comments
 (0)